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

[Zenghui Yu <yuzenghui@huawei.com>]

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 <eric.auger@redhat.com>
>>>
>>> ---
>>>
>>> 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