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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 19/09/2023 14:10, Julien Grall wrote:
> 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.
I forgot to mention that this likely want to be a 
clean_dcache_and_invalidate_va_range() (see my comment in patch #2).
Cheers,

--
Julien Grall