Re: [PATCH v1 3/3] ARM: GICv3 ITS: flush all buffers, not just command queue

[Julien Grall <julien@xxxxxxx>]

Hi Volodymyr,

On 19/09/2023 12:28, Volodymyr Babchuk wrote:
> ITS manages Device Tables and Interrupt Translation Tables on its own,
> so generally we are not interested which shareability and cacheability
> attributes it uses. But there is one exception: ITS requires that DT
> and ITT must be initialized with zeroes. If ITS belongs to the Inner
> Cacheability domain there is no problem at all.
>
> But in all other cases we need to do clean CPU caches manually, or
> otherwise CPU can overwrite DT and ITT entries. From user perspective
> this looks like interrupts are not delivered from a device.
>
> Also, we will rename HOST_ITS_FLUSH_CMD_QUEUE flag to
> HOST_ITS_FLUSH_BUFFERS because now this flag controls not only command
> queue.
Reading the specification, CBASER will indicate the cacheability for the 
command queue. But I couldn't find any reference saying this will apply 
to the ITT as well.
If such reference doesn't exist then...

> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> ---
>   xen/arch/arm/gic-v3-its.c             | 7 +++++--
>   xen/arch/arm/include/asm/gic_v3_its.h | 2 +-
>   2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 72cf318810..63e28a7706 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -107,7 +107,7 @@ static int its_send_command(struct host_its *hw_its, const 
> void *its_cmd)
>       }
>   
>       memcpy(hw_its->cmd_buf + writep, its_cmd, ITS_CMD_SIZE);
> -    if ( hw_its->flags & HOST_ITS_FLUSH_CMD_QUEUE )
> +    if ( hw_its->flags & HOST_ITS_FLUSH_BUFFERS )
>           clean_dcache_va_range(hw_its->cmd_buf + writep, ITS_CMD_SIZE);
>       else
>           dsb(ishst);
> @@ -335,7 +335,7 @@ static void *its_map_cbaser(struct host_its *its)
>        */
>       if ( !(reg & GITS_BASER_INNER_CACHEABILITY_MASK) )
>       {
> -        its->flags |= HOST_ITS_FLUSH_CMD_QUEUE;
> +        its->flags |= HOST_ITS_FLUSH_BUFFERS;
>           printk(XENLOG_WARNING "using non-cacheable ITS command queue\n");
>       }
>   
> @@ -699,6 +699,9 @@ int gicv3_its_map_guest_device(struct domain *d,
>       if ( !itt_addr )
>           goto out_unlock;
>   
> +    if ( hw_its->flags & HOST_ITS_FLUSH_BUFFERS )
> +        clean_dcache_va_range(itt_addr, nr_events * hw_its->itte_size);
... I think we need to have this flush unconditional like Linux does.

Cheers,

--
Julien Grall