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@xxxxxxxxxx>
>> ---
>> 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
>
