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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 9/19/23 09:14, Julien Grall wrote:
> 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).

I turned it into an unconditional clean_and_invalidate_dcache_va_range() and 
did a quick test, and it still fixes my test case on VCK190. So feel free to 
add:

Tested-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>