Re: [kvm-unit-tests PATCH v3 08/14] arm/arm64: ITS: its_enable_defaults

[Zenghui Yu]

Hi Eric,

On 2020/3/4 22:26, Auger Eric wrote:
> Hi Zenghui,
> On 2/7/20 4:20 AM, Zenghui Yu wrote:
> > Hi Eric,
> >
> > On 2020/1/28 18:34, Eric Auger wrote:
> > > its_enable_defaults() is the top init function that allocates the
> > > command queue and all the requested tables (device, collection,
> > > lpi config and pending tables), enable LPIs at distributor level
> > > and ITS level.
> > >
> > > gicv3_enable_defaults must be called before.
> > >
> > > Signed-off-by: Eric Auger <address@hidden>
> > >
> > > ---
> > >
> > > v2 -> v3:
> > > - introduce its_setup_baser in this patch
> > > - squash "arm/arm64: ITS: Init the command queue" in this patch.
> > > ---
> > >    lib/arm/asm/gic-v3-its.h |  8 ++++
> > >    lib/arm/gic-v3-its.c     | 89 ++++++++++++++++++++++++++++++++++++++++
> > >    2 files changed, 97 insertions(+)
> > >
> > > diff --git a/lib/arm/asm/gic-v3-its.h b/lib/arm/asm/gic-v3-its.h
> > > index 815c515..fe73c04 100644
> > > --- a/lib/arm/asm/gic-v3-its.h
> > > +++ b/lib/arm/asm/gic-v3-its.h
> > > @@ -36,6 +36,8 @@ struct its_data {
> > >        void *base;
> > >        struct its_typer typer;
> > >        struct its_baser baser[GITS_BASER_NR_REGS];
> > > +    struct its_cmd_block *cmd_base;
> > > +    struct its_cmd_block *cmd_write;
> > >    };
> > >      extern struct its_data its_data;
> > > @@ -88,10 +90,16 @@ extern struct its_data its_data;
> > >    #define GITS_BASER_TYPE_DEVICE        1
> > >    #define GITS_BASER_TYPE_COLLECTION    4
> > >    +
> > > +struct its_cmd_block {
> > > +    u64 raw_cmd[4];
> > > +};
> > > +
> > >    extern void its_parse_typer(void);
> > >    extern void its_init(void);
> > >    extern int its_parse_baser(int i, struct its_baser *baser);
> > >    extern struct its_baser *its_lookup_baser(int type);
> > > +extern void its_enable_defaults(void);
> > >      #else /* __arm__ */
> > >    diff --git a/lib/arm/gic-v3-its.c b/lib/arm/gic-v3-its.c
> > > index 2c0ce13..d1e7e52 100644
> > > --- a/lib/arm/gic-v3-its.c
> > > +++ b/lib/arm/gic-v3-its.c
> > > @@ -86,3 +86,92 @@ void its_init(void)
> > >            its_parse_baser(i, &its_data.baser[i]);
> > >    }
> > >    +static void its_setup_baser(int i, struct its_baser *baser)
> > > +{
> > > +    unsigned long n = (baser->nr_pages * baser->psz) >> PAGE_SHIFT;
> > > +    unsigned long order = is_power_of_2(n) ? fls(n) : fls(n) + 1;
> > > +    u64 val;
> > > +
> > > +    baser->table_addr = (u64)virt_to_phys(alloc_pages(order));
> > > +
> > > +    val = ((u64)baser->table_addr                    |
> > > +        ((u64)baser->type    << GITS_BASER_TYPE_SHIFT)    |
> > > +        ((u64)(baser->esz - 1)    << GITS_BASER_ENTRY_SIZE_SHIFT)    |
> > > +        ((baser->nr_pages - 1)    << GITS_BASER_PAGES_SHIFT)    |
> > > +        (u64)baser->indirect    << 62                |
> >
> > I haven't seen the 'nr_pages' and 'indirect' are programmed anywhere
> > except in its_parse_baser(). It looks like they're treated as RO (but
> > they shouldn't) and I now don't think it makes sense to parse them in
> > its_parse_baser(), in patch#5.
>
> First of all please forgive me for the delay.

Never mind.

> I agree with you on nr_pages. However indirect also indicates the BASER
> capability to support or not 2 level tables. So I think it makes sense
> to read it on init.

Yes, you're right. As the spec says, the Indirect field "is RAZ/WI for
GIC implementations that only support flat tables".


Thanks,
Zenghui