Re: [kvm-unit-tests PATCH 08/16] arm/arm64: ITS: Init the command queue

[Auger Eric <eric.auger@redhat.com>]

Hi Zenghui,

On 12/20/19 8:10 AM, Zenghui Yu wrote:
> Hi Eric,
> 
> On 2019/12/16 22:02, Eric Auger wrote:
>> Allocate the command queue and initialize related registers:
>> CBASER, CREADR, CWRITER.
>>
>> The command queue is 64kB. This aims at not bothing with fullness.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>   lib/arm/asm/gic-v3-its.h |  7 +++++++
>>   lib/arm/gic-v3-its.c     | 37 +++++++++++++++++++++++++++++++++++++
>>   2 files changed, 44 insertions(+)
>>
>> diff --git a/lib/arm/asm/gic-v3-its.h b/lib/arm/asm/gic-v3-its.h
>> index 0d11aed..ed42707 100644
>> --- a/lib/arm/asm/gic-v3-its.h
>> +++ b/lib/arm/asm/gic-v3-its.h
>> @@ -113,10 +113,17 @@ struct its_baser {
>>       int esz;
>>   };
>>   +struct its_cmd_block {
>> +    u64     raw_cmd[4];
>> +};
>> +
>>   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;
>> +    struct its_cmd_block *cmd_readr;
> 
> I think we can just get rid of the 'cmd_readr'. As GITS_CREADR is
> generally manipulated by the ITS, and ...
> 
>>   };
>>     extern struct its_data its_data;
>> diff --git a/lib/arm/gic-v3-its.c b/lib/arm/gic-v3-its.c
>> index 0b5a700..8b6a095 100644
>> --- a/lib/arm/gic-v3-its.c
>> +++ b/lib/arm/gic-v3-its.c
>> @@ -188,3 +188,40 @@ void set_pending_table_bit(int rdist, int n, bool
>> set)
>>           byte &= ~mask;
>>       *ptr = byte;
>>   }
>> +
>> +/**
>> + * init_cmd_queue: Allocate the command queue and initialize
>> + * CBASER, CREADR, CWRITER
>> + */
>> +void init_cmd_queue(void);
>> +void init_cmd_queue(void)
>> +{
>> +    unsigned long n = SZ_64K >> PAGE_SHIFT;
>> +    unsigned long order = fls(n);
>> +    u64 cbaser, tmp;
>> +
>> +    its_data.cmd_base = (void *)virt_to_phys(alloc_pages(order));
>> +
>> +    cbaser = ((u64)its_data.cmd_base    |
>> +         GITS_CBASER_WaWb               |
>> +         GITS_CBASER_InnerShareable     |
>> +         (SZ_64K / SZ_4K - 1) |
>> +         GITS_CBASER_VALID);
>> +
>> +    writeq(cbaser, its_data.base + GITS_CBASER);
> 
> ..."(CREADR) is cleared to 0 when a value is written to GITS_CBASER."
> -- from IHI0069E 9.19.3
> 
>> +    tmp = readq(its_data.base + GITS_CBASER);
>> +
>> +    if ((tmp ^ cbaser) & GITS_CBASER_SHAREABILITY_MASK) {
>> +        if (!(tmp & GITS_CBASER_SHAREABILITY_MASK)) {
>> +            cbaser &= ~(GITS_CBASER_SHAREABILITY_MASK |
>> +                GITS_CBASER_CACHEABILITY_MASK);
>> +            cbaser |= GITS_CBASER_nC;
>> +            writeq(cbaser, its_data.base + GITS_CBASER);
>> +        }
>> +    }
>> +
>> +    its_data.cmd_write = its_data.cmd_base;
>> +    its_data.cmd_readr = its_data.cmd_base;
>> +    writeq(0, its_data.base + GITS_CWRITER);
>> +    writeq(0, its_data.base + GITS_CREADR);
> 
> So this writeq() is also not needed.
> 
> Or I've just missed the point that this is done by intention to test
> "whether the GITS_CREADR implemented by KVM is Write Ignored"?
> If so, please ignore all of the comments above :)
No I must admit this was not done on purpose. I can remove it from the
its_data struct at the moment.

Thanks

Eric
> 
> 
> Thanks,
> Zenghui
>