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

[Zenghui Yu]

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 <address@hidden>
> ---
>   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 :)


Thanks,
Zenghui