[PATCH v2] ARM: GICv3 ITS: flush caches for newly allocated ITT

[PATCH v2] ARM: GICv3 ITS: flush caches for newly allocated ITT

[Volodymyr Babchuk]

ITS manages Device Tables and Interrupt Translation Tables on its own,
so generally we are not interested in maintaining any coherence with
CPU's view of those memory regions, except one case: ITS requires that
Interrupt Translation Tables should be initialized with
zeroes. Existing code already does this, but it does not cleans
caches afterwards. This means that ITS may see un-initialized ITT and
CPU can overwrite portions of ITT later, when it finally decides to
flush caches. Visible effect of this issue that there are not
interrupts delivered from a device.

Fix this by calling clean_and_invalidate_dcache_va_range() for newly
allocated ITT.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

---

Changes since v1:
 - Use clean_and_invalidate_dcache_va_range() instead of
   clean_dcache_va_range()
 - Do this unconditionally
 - Do not rename  HOST_ITS_FLUSH_CMD_QUEUE into HOST_ITS_FLUSH_BUFFERS
---
 xen/arch/arm/gic-v3-its.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index 3aa4edda10..8afcd9783b 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -685,6 +685,9 @@ int gicv3_its_map_guest_device(struct domain *d,
     if ( !itt_addr )
         goto out_unlock;
 
+    clean_and_invalidate_dcache_va_range(itt_addr,
+                                         nr_events * hw_its->itte_size);
+
     dev = xzalloc(struct its_device);
     if ( !dev )
         goto out_unlock;
-- 
2.42.0

Re: [PATCH v2] ARM: GICv3 ITS: flush caches for newly allocated ITT

[Stewart Hildebrand]

On 9/22/23 18:27, Volodymyr Babchuk wrote:
> ITS manages Device Tables and Interrupt Translation Tables on its own,
> so generally we are not interested in maintaining any coherence with
> CPU's view of those memory regions, except one case: ITS requires that
> Interrupt Translation Tables should be initialized with
> zeroes. Existing code already does this, but it does not cleans
> caches afterwards. This means that ITS may see un-initialized ITT and
> CPU can overwrite portions of ITT later, when it finally decides to
> flush caches. Visible effect of this issue that there are not
> interrupts delivered from a device.
> 
> Fix this by calling clean_and_invalidate_dcache_va_range() for newly
> allocated ITT.
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

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

> 
> ---
> 
> Changes since v1:
>  - Use clean_and_invalidate_dcache_va_range() instead of
>    clean_dcache_va_range()
>  - Do this unconditionally
>  - Do not rename  HOST_ITS_FLUSH_CMD_QUEUE into HOST_ITS_FLUSH_BUFFERS
> ---
>  xen/arch/arm/gic-v3-its.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 3aa4edda10..8afcd9783b 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -685,6 +685,9 @@ int gicv3_its_map_guest_device(struct domain *d,
>      if ( !itt_addr )
>          goto out_unlock;
> 
> +    clean_and_invalidate_dcache_va_range(itt_addr,
> +                                         nr_events * hw_its->itte_size);
> +
>      dev = xzalloc(struct its_device);
>      if ( !dev )
>          goto out_unlock;
> --
> 2.42.0

[for-4.18] Re: [PATCH v2] ARM: GICv3 ITS: flush caches for newly allocated ITT

[Julien Grall]

Hi,

(Adding [for-4.18] in the title for Henry to spot the request)

On 22/09/2023 23:27, Volodymyr Babchuk wrote:
> ITS manages Device Tables and Interrupt Translation Tables on its own,
> so generally we are not interested in maintaining any coherence with
> CPU's view of those memory regions, except one case: ITS requires that
> Interrupt Translation Tables should be initialized with
> zeroes. Existing code already does this, but it does not cleans
> caches afterwards. This means that ITS may see un-initialized ITT and
> CPU can overwrite portions of ITT later, when it finally decides to
> flush caches. Visible effect of this issue that there are not
> interrupts delivered from a device.
> 
> Fix this by calling clean_and_invalidate_dcache_va_range() for newly
> allocated ITT.
> 

I would consider to add:

Fixes: 69082e1c210d ("ARM: GICv3 ITS: introduce device mapping")

> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

Reviewed-by: Julien Grall <jgrall@amazon.com>

@Henry, this patch should be low-risk. We are cleaning & invalidating 
the cache, so there should be no change for platform not requiring cache 
maintenance. This should hopefully had support for more platform. Note 
that the GICv3 ITS feature is still experimental.

Based on what I wrote above, would you be OK to have this patch in 4.18?

Cheers,

-- 
Julien Grall

Re: [for-4.18] Re: [PATCH v2] ARM: GICv3 ITS: flush caches for newly allocated ITT

[Volodymyr Babchuk]

Hi Julien, Henry,

Julien Grall <julien@xen.org> writes:

> Hi,
>
> (Adding [for-4.18] in the title for Henry to spot the request)
>
> On 22/09/2023 23:27, Volodymyr Babchuk wrote:
>> ITS manages Device Tables and Interrupt Translation Tables on its own,
>> so generally we are not interested in maintaining any coherence with
>> CPU's view of those memory regions, except one case: ITS requires that
>> Interrupt Translation Tables should be initialized with
>> zeroes. Existing code already does this, but it does not cleans
>> caches afterwards. This means that ITS may see un-initialized ITT and
>> CPU can overwrite portions of ITT later, when it finally decides to
>> flush caches. Visible effect of this issue that there are not
>> interrupts delivered from a device.
>> Fix this by calling clean_and_invalidate_dcache_va_range() for newly
>> allocated ITT.
>> 
>
> I would consider to add:
>
> Fixes: 69082e1c210d ("ARM: GICv3 ITS: introduce device mapping")

May I ask you (or Henry?) to add this when you'll commit this change? Or
should I publish an updated version?

>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>
> Reviewed-by: Julien Grall <jgrall@amazon.com>
>
> @Henry, this patch should be low-risk. We are cleaning & invalidating
> the cache, so there should be no change for platform not requiring
> cache maintenance. This should hopefully had support for more
> platform. Note that the GICv3 ITS feature is still experimental.
>
> Based on what I wrote above, would you be OK to have this patch in 4.18?


-- 
WBR, Volodymyr

Re: [for-4.18] Re: [PATCH v2] ARM: GICv3 ITS: flush caches for newly allocated ITT

[Julien Grall]

On 25/09/2023 20:00, Volodymyr Babchuk wrote:
> 
> Hi Julien, Henry,
> 
> Julien Grall <julien@xen.org> writes:
> 
>> Hi,
>>
>> (Adding [for-4.18] in the title for Henry to spot the request)
>>
>> On 22/09/2023 23:27, Volodymyr Babchuk wrote:
>>> ITS manages Device Tables and Interrupt Translation Tables on its own,
>>> so generally we are not interested in maintaining any coherence with
>>> CPU's view of those memory regions, except one case: ITS requires that
>>> Interrupt Translation Tables should be initialized with
>>> zeroes. Existing code already does this, but it does not cleans
>>> caches afterwards. This means that ITS may see un-initialized ITT and
>>> CPU can overwrite portions of ITT later, when it finally decides to
>>> flush caches. Visible effect of this issue that there are not
>>> interrupts delivered from a device.
>>> Fix this by calling clean_and_invalidate_dcache_va_range() for newly
>>> allocated ITT.
>>>
>>
>> I would consider to add:
>>
>> Fixes: 69082e1c210d ("ARM: GICv3 ITS: introduce device mapping")
> 
> May I ask you (or Henry?) to add this when you'll commit this change? Or
> should I publish an updated version?

I can do it on commit.

Cheers,

-- 
Julien Grall

Re: [for-4.18] [PATCH v2] ARM: GICv3 ITS: flush caches for newly allocated ITT

[Henry Wang]

Hi Julien,

> On Sep 26, 2023, at 02:33, Julien Grall <julien@xen.org> wrote:
> 
> Hi,
> 
> (Adding [for-4.18] in the title for Henry to spot the request)

Thanks!

> 
> On 22/09/2023 23:27, Volodymyr Babchuk wrote:
>> ITS manages Device Tables and Interrupt Translation Tables on its own,
>> so generally we are not interested in maintaining any coherence with
>> CPU's view of those memory regions, except one case: ITS requires that
>> Interrupt Translation Tables should be initialized with
>> zeroes. Existing code already does this, but it does not cleans
>> caches afterwards. This means that ITS may see un-initialized ITT and
>> CPU can overwrite portions of ITT later, when it finally decides to
>> flush caches. Visible effect of this issue that there are not
>> interrupts delivered from a device.
>> Fix this by calling clean_and_invalidate_dcache_va_range() for newly
>> allocated ITT.
> 
> I would consider to add:
> 
> Fixes: 69082e1c210d ("ARM: GICv3 ITS: introduce device mapping")
> 
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> 
> Reviewed-by: Julien Grall <jgrall@amazon.com>
> 
> @Henry, this patch should be low-risk. We are cleaning & invalidating the cache, so there should be no change for platform not requiring cache maintenance. This should hopefully had support for more platform. Note that the GICv3 ITS feature is still experimental.
> 
> Based on what I wrote above, would you be OK to have this patch in 4.18?

Yes, I was about to ask the same question but somehow forgot it. This is a quite
low risk patch and I think it is fine to have this in 4.18, so if the "Fixes” tag
can be added on commit, please also add:

Release-acked-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [for-4.18] [PATCH v2] ARM: GICv3 ITS: flush caches for newly allocated ITT

[Julien Grall]

On 25/09/2023 23:19, Henry Wang wrote:
> Hi Julien,

Hi Henry,

> Yes, I was about to ask the same question but somehow forgot it. This is a quite
> low risk patch and I think it is fine to have this in 4.18, so if the "Fixes” tag
> can be added on commit, please also add:
> 
> Release-acked-by: Henry Wang <Henry.Wang@arm.com>

Thanks! It is now committed.

Cheers,

-- 
Julien Grall