Re: [PATCH v1 1/3] ARM: GICv3 ITS: issue INVALL command after mapping host events

[Julien Grall <julien@xxxxxxx>]

Hi Volodymyr,

On 19/09/2023 12:28, Volodymyr Babchuk wrote:
> Implement TODO by calling the INVALL command.
I think the TODO was meant to be an optimization because AFAICT we are 
already sending an INV command per event.
>  It is working on real
> HW, so there is no sense in not doing this.
A patch should be justified based from the spec or an errata. Not that 
fact it works on a real HW. At the moment, I don't quite understand why 
you need one because as said above, we are technically already sending 
an INV per event so we should be covered.
Removing the INV and using INVALL would make sense as an optimization. 
Yet given the current code doesn't seem to work, I would like to 
understand what's the problem of using INV.
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> ---
>   xen/arch/arm/gic-v3-its.c | 17 ++++++++++++++++-
>   1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 3aa4edda10..a9c971a55f 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -236,6 +236,19 @@ static int its_send_cmd_inv(struct host_its *its,
>       return its_send_command(its, cmd);
>   }
>   
> +static int its_send_cmd_invall(struct host_its *its,
> +                               uint32_t collection_id)
> +{
> +    uint64_t cmd[4];
> +
> +    cmd[0] = GITS_CMD_INVALL;
> +    cmd[1] = 0x00;
> +    cmd[2] = collection_id;
> +    cmd[3] = 0x00;
> +
> +    return its_send_command(its, cmd);
> +}
> +
>   /* Set up the (1:1) collection mapping for the given host CPU. */
>   int gicv3_its_setup_collection(unsigned int cpu)
>   {
> @@ -593,7 +606,9 @@ static int gicv3_its_map_host_events(struct host_its *its,
>               return ret;
>       }
>   
> -    /* TODO: Consider using INVALL here. Didn't work on the model, though. */
> +    ret = its_send_cmd_invall(its, 0);
> +    if ( ret )
> +        return ret;
>   
>       ret = its_send_cmd_sync(its, 0);
>       if ( ret )
Cheers,

--
Julien Grall