Return type of clean_and_invalidate_dcache_va_range

Return type of clean_and_invalidate_dcache_va_range

[Nicola Vetrini]

Hi all,

In the context of violations of MISRA C:2012 Rule 17.7: "The value 
returned by a function having non-void return type shall be used", I was 
looking at the function "clean_and_invalidate_dcache_va_range". It has 
the following signature on both arm and x86:

static inline int clean_and_invalidate_dcache_va_range
     (const void *p, unsigned long size)

The commit that introduced it for Arm ~9 years ago (71d64afe3e12: "arm: 
return int from *_dcache_va_range") [1] mentions that on Arm it can't 
fail, but supposedly it can on x86.

However, as far as I can tell, for both arch-es the implementation now 
always returns 0 [2][3], so perhaps the mention of -EOPNOTSUPP for x86 
is no longer true (I wasn't able to reconstruct if there was a time at 
which this was true, even in the same commit that changed the return 
type to int).

The question is: should the return type be void, since it appears that 
every user is ignoring the returned value (violating the rule), except 
the one in common/grant_table.c [4]?

The other two resolution paths are either allowing this function's 
result to be ignored or cast all ignored invocations to void, with the 
first being cleaner from a code readability perspective.

[1] These functions cannot really fail on ARM, but their x86 equivalents 
can (-EOPNOTSUPP). Change the prototype to return int.
[2] 
https://gitlab.com/xen-project/xen/-/blob/staging/xen/arch/arm/include/asm/page.h#L218
[3] 
https://gitlab.com/xen-project/xen/-/blob/staging/xen/arch/x86/include/asm/flushtlb.h#L188
[4] 
https://gitlab.com/xen-project/xen/-/blob/staging/xen/common/grant_table.c#L3576

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: Return type of clean_and_invalidate_dcache_va_range

[Stefano Stabellini]

On Fri, 9 Feb 2024, Nicola Vetrini wrote:
> Hi all,
> 
> In the context of violations of MISRA C:2012 Rule 17.7: "The value returned by
> a function having non-void return type shall be used", I was looking at the
> function "clean_and_invalidate_dcache_va_range". It has the following
> signature on both arm and x86:
> 
> static inline int clean_and_invalidate_dcache_va_range
>     (const void *p, unsigned long size)
> 
> The commit that introduced it for Arm ~9 years ago (71d64afe3e12: "arm: return
> int from *_dcache_va_range") [1] mentions that on Arm it can't fail, but
> supposedly it can on x86.
> 
> However, as far as I can tell, for both arch-es the implementation now always
> returns 0 [2][3], so perhaps the mention of -EOPNOTSUPP for x86 is no longer
> true (I wasn't able to reconstruct if there was a time at which this was true,
> even in the same commit that changed the return type to int).
> 
> The question is: should the return type be void, since it appears that every
> user is ignoring the returned value (violating the rule), except the one in
> common/grant_table.c [4]?

Looking at the implementation on both ARM and x86, I am in favor of
changing the return type to void



> The other two resolution paths are either allowing this function's result to
> be ignored or cast all ignored invocations to void, with the first being
> cleaner from a code readability perspective.
> 
> [1] These functions cannot really fail on ARM, but their x86 equivalents can
> (-EOPNOTSUPP). Change the prototype to return int.
> [2]
> https://gitlab.com/xen-project/xen/-/blob/staging/xen/arch/arm/include/asm/page.h#L218
> [3]
> https://gitlab.com/xen-project/xen/-/blob/staging/xen/arch/x86/include/asm/flushtlb.h#L188
> [4]
> https://gitlab.com/xen-project/xen/-/blob/staging/xen/common/grant_table.c#L3576
> 
> -- 
> Nicola Vetrini, BSc
> Software Engineer, BUGSENG srl (https://bugseng.com)
> 

Re: Return type of clean_and_invalidate_dcache_va_range

[Julien Grall]

Hi,

On 09/02/2024 22:02, Stefano Stabellini wrote:
> On Fri, 9 Feb 2024, Nicola Vetrini wrote:
>> Hi all,
>>
>> In the context of violations of MISRA C:2012 Rule 17.7: "The value returned by
>> a function having non-void return type shall be used", I was looking at the
>> function "clean_and_invalidate_dcache_va_range". It has the following
>> signature on both arm and x86:
>>
>> static inline int clean_and_invalidate_dcache_va_range
>>      (const void *p, unsigned long size)
>>
>> The commit that introduced it for Arm ~9 years ago (71d64afe3e12: "arm: return
>> int from *_dcache_va_range") [1] mentions that on Arm it can't fail, but
>> supposedly it can on x86.
>>
>> However, as far as I can tell, for both arch-es the implementation now always
>> returns 0 [2][3], so perhaps the mention of -EOPNOTSUPP for x86 is no longer
>> true (I wasn't able to reconstruct if there was a time at which this was true,
>> even in the same commit that changed the return type to int).
>>
>> The question is: should the return type be void, since it appears that every
>> user is ignoring the returned value (violating the rule), except the one in
>> common/grant_table.c [4]?
> 
> Looking at the implementation on both ARM and x86, I am in favor of
> changing the return type to void
I think we need some consistency between all the cache flush helpers 
(clean_and_invalidate_dcache_va_range, invalidate_dcache_va_range() and 
clean_dcache_va_range()). They should all return a values or not return any.

That said, we have two other architectures in development. Are we saying 
this helpers will not need to (initially) return -EOPNOTSUPP?

If we need to return a value for any of the cache helpers, then I would 
consider to provide an extra helpers that would only be used by the 
grant-table code and return a value. Then transform all the existing 
helpers to return void.

Cheers,

-- 
Julien Grall

Re: Return type of clean_and_invalidate_dcache_va_range

[Jan Beulich]

On 10.02.2024 11:17, Julien Grall wrote:
> Hi,
> 
> On 09/02/2024 22:02, Stefano Stabellini wrote:
>> On Fri, 9 Feb 2024, Nicola Vetrini wrote:
>>> Hi all,
>>>
>>> In the context of violations of MISRA C:2012 Rule 17.7: "The value returned by
>>> a function having non-void return type shall be used", I was looking at the
>>> function "clean_and_invalidate_dcache_va_range". It has the following
>>> signature on both arm and x86:
>>>
>>> static inline int clean_and_invalidate_dcache_va_range
>>>      (const void *p, unsigned long size)
>>>
>>> The commit that introduced it for Arm ~9 years ago (71d64afe3e12: "arm: return
>>> int from *_dcache_va_range") [1] mentions that on Arm it can't fail, but
>>> supposedly it can on x86.
>>>
>>> However, as far as I can tell, for both arch-es the implementation now always
>>> returns 0 [2][3], so perhaps the mention of -EOPNOTSUPP for x86 is no longer
>>> true (I wasn't able to reconstruct if there was a time at which this was true,
>>> even in the same commit that changed the return type to int).
>>>
>>> The question is: should the return type be void, since it appears that every
>>> user is ignoring the returned value (violating the rule), except the one in
>>> common/grant_table.c [4]?
>>
>> Looking at the implementation on both ARM and x86, I am in favor of
>> changing the return type to void
> I think we need some consistency between all the cache flush helpers 
> (clean_and_invalidate_dcache_va_range, invalidate_dcache_va_range() and 
> clean_dcache_va_range()). They should all return a values or not return any.

+1

> That said, we have two other architectures in development. Are we saying 
> this helpers will not need to (initially) return -EOPNOTSUPP?

For "(initially)" that's not an issue - such a stub can as well be filled
for BUG_ON("unimplemented"). The question there is what the ultimate
implementations are going to look like.

Jan

Re: Return type of clean_and_invalidate_dcache_va_range

[Nicola Vetrini]

On 2024-02-12 09:26, Jan Beulich wrote:
> On 10.02.2024 11:17, Julien Grall wrote:
>> Hi,
>> 
>> On 09/02/2024 22:02, Stefano Stabellini wrote:
>>> On Fri, 9 Feb 2024, Nicola Vetrini wrote:
>>>> Hi all,
>>>> 
>>>> In the context of violations of MISRA C:2012 Rule 17.7: "The value 
>>>> returned by
>>>> a function having non-void return type shall be used", I was looking 
>>>> at the
>>>> function "clean_and_invalidate_dcache_va_range". It has the 
>>>> following
>>>> signature on both arm and x86:
>>>> 
>>>> static inline int clean_and_invalidate_dcache_va_range
>>>>      (const void *p, unsigned long size)
>>>> 
>>>> The commit that introduced it for Arm ~9 years ago (71d64afe3e12: 
>>>> "arm: return
>>>> int from *_dcache_va_range") [1] mentions that on Arm it can't fail, 
>>>> but
>>>> supposedly it can on x86.
>>>> 
>>>> However, as far as I can tell, for both arch-es the implementation 
>>>> now always
>>>> returns 0 [2][3], so perhaps the mention of -EOPNOTSUPP for x86 is 
>>>> no longer
>>>> true (I wasn't able to reconstruct if there was a time at which this 
>>>> was true,
>>>> even in the same commit that changed the return type to int).
>>>> 
>>>> The question is: should the return type be void, since it appears 
>>>> that every
>>>> user is ignoring the returned value (violating the rule), except the 
>>>> one in
>>>> common/grant_table.c [4]?
>>> 
>>> Looking at the implementation on both ARM and x86, I am in favor of
>>> changing the return type to void
>> I think we need some consistency between all the cache flush helpers
>> (clean_and_invalidate_dcache_va_range, invalidate_dcache_va_range() 
>> and
>> clean_dcache_va_range()). They should all return a values or not 
>> return any.
> 
> +1
> 

I agree. I took this helper as an example, but e.g. 
invalidate_dcache_va_range returns -EOPNOTSUPP on x86 and it's only used 
in common/grant_table.
Perhaps the signatures should remain as is for consistency, especially 
given the remark below about the other architectures, and this would 
entail a deviation.

>> That said, we have two other architectures in development. Are we 
>> saying
>> this helpers will not need to (initially) return -EOPNOTSUPP?
> 
> For "(initially)" that's not an issue - such a stub can as well be 
> filled
> for BUG_ON("unimplemented"). The question there is what the ultimate
> implementations are going to look like.
> 

Should I CC them in this thread?

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: Return type of clean_and_invalidate_dcache_va_range

[Julien Grall]

Hi Nicola,

On 12/02/2024 14:56, Nicola Vetrini wrote:
> On 2024-02-12 09:26, Jan Beulich wrote:
>> On 10.02.2024 11:17, Julien Grall wrote:
>>> Hi,
>>>
>>> On 09/02/2024 22:02, Stefano Stabellini wrote:
>>>> On Fri, 9 Feb 2024, Nicola Vetrini wrote:
>>>>> Hi all,
>>>>>
>>>>> In the context of violations of MISRA C:2012 Rule 17.7: "The value 
>>>>> returned by
>>>>> a function having non-void return type shall be used", I was 
>>>>> looking at the
>>>>> function "clean_and_invalidate_dcache_va_range". It has the following
>>>>> signature on both arm and x86:
>>>>>
>>>>> static inline int clean_and_invalidate_dcache_va_range
>>>>>      (const void *p, unsigned long size)
>>>>>
>>>>> The commit that introduced it for Arm ~9 years ago (71d64afe3e12: 
>>>>> "arm: return
>>>>> int from *_dcache_va_range") [1] mentions that on Arm it can't 
>>>>> fail, but
>>>>> supposedly it can on x86.
>>>>>
>>>>> However, as far as I can tell, for both arch-es the implementation 
>>>>> now always
>>>>> returns 0 [2][3], so perhaps the mention of -EOPNOTSUPP for x86 is 
>>>>> no longer
>>>>> true (I wasn't able to reconstruct if there was a time at which 
>>>>> this was true,
>>>>> even in the same commit that changed the return type to int).
>>>>>
>>>>> The question is: should the return type be void, since it appears 
>>>>> that every
>>>>> user is ignoring the returned value (violating the rule), except 
>>>>> the one in
>>>>> common/grant_table.c [4]?
>>>>
>>>> Looking at the implementation on both ARM and x86, I am in favor of
>>>> changing the return type to void
>>> I think we need some consistency between all the cache flush helpers
>>> (clean_and_invalidate_dcache_va_range, invalidate_dcache_va_range() and
>>> clean_dcache_va_range()). They should all return a values or not 
>>> return any.
>>
>> +1
>>
> 
> I agree. I took this helper as an example, but e.g. 
> invalidate_dcache_va_range returns -EOPNOTSUPP on x86 and it's only used 
> in common/grant_table.
> Perhaps the signatures should remain as is for consistency, especially 
> given the remark below about the other architectures, and this would 
> entail a deviation.

In general, I am not in favor of adding a deviation if the code can be 
changed. In this case, we could have (untested and just a hack to show 
my point):

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 5721eab22561..ae9ccf5388fc 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -3573,7 +3573,13 @@ static int _cache_flush(const 
gnttab_cache_flush_t *cflush, grant_ref_t *cur_ref
      v += cflush->offset;

      if ( (cflush->op & GNTTAB_CACHE_INVAL) && (cflush->op & 
GNTTAB_CACHE_CLEAN) )
-        ret = clean_and_invalidate_dcache_va_range(v, cflush->length);
+    {
+#ifdef clean_and_invalidate_dcache_va_range
+        clean_and_invalidate_dcache_va_range(v, cflush->length);
+        ret = 0;
+#else
+        ret = -ENOSYS;
+#endif
      else if ( cflush->op & GNTTAB_CACHE_INVAL )
          ret = invalidate_dcache_va_range(v, cflush->length);
      else if ( cflush->op & GNTTAB_CACHE_CLEAN )

An alternative would be to introduced arch_grant_cache_flush() and move 
the if/else logic there. Something like:

diff --git a/xen/arch/arm/include/asm/page.h 
b/xen/arch/arm/include/asm/page.h
index 69f817d1e68a..4a3de49762a1 100644
--- a/xen/arch/arm/include/asm/page.h
+++ b/xen/arch/arm/include/asm/page.h
@@ -281,6 +281,19 @@ static inline void write_pte(lpae_t *p, lpae_t pte)
      dsb(sy);
  }

+static inline arch_grant_cache_flush(unsigned int op, const void *p, 
unsigned long size)
+{
+    unsigned int order = get_order_from_bytes(size);
+
+    if ( (cflush->op & GNTTAB_CACHE_INVAL) && (cflush->op & 
GNTTAB_CACHE_CLEAN) )
+        clean_and_invalidate_dcache_va_range(v, cflush->length);
+    else if ( cflush->op & GNTTAB_CACHE_INVAL )
+        invalidate_dcache_va_range(v, cflush->length);
+    else if ( cflush->op & GNTTAB_CACHE_CLEAN )
+        clean_dcache_va_range(v, cflush->length);
+
+    return 0;
+}

  /* Flush the dcache for an entire page. */
  void flush_page_to_ram(unsigned long mfn, bool sync_icache);
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 424744ad5e1a..647e1522466d 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -735,8 +735,7 @@ void asmlinkage __init start_xen(unsigned long 
boot_phys_offset,
                fdt_paddr);

      /* Register Xen's load address as a boot module. */
-    xen_bootmodule = add_boot_module(BOOTMOD_XEN,
-                             virt_to_maddr(_start),
+    xen_bootmodule = add_boot_module(BOOTMOD_XEN, virt_to_maddr(_start),
                               (paddr_t)(uintptr_t)(_end - _start), false);
      BUG_ON(!xen_bootmodule);

diff --git a/xen/arch/x86/include/asm/flushtlb.h 
b/xen/arch/x86/include/asm/flushtlb.h
index bb0ad58db49b..dfe51cddde90 100644
--- a/xen/arch/x86/include/asm/flushtlb.h
+++ b/xen/arch/x86/include/asm/flushtlb.h
@@ -182,23 +182,22 @@ void flush_area_mask(const cpumask_t *mask, const 
void *va,
  }

  static inline void flush_page_to_ram(unsigned long mfn, bool 
sync_icache) {}
-static inline int invalidate_dcache_va_range(const void *p,
-                                             unsigned long size)
-{ return -EOPNOTSUPP; }
-static inline int clean_and_invalidate_dcache_va_range(const void *p,
-                                                       unsigned long size)
+
+unsigned int guest_flush_tlb_flags(const struct domain *d);
+void guest_flush_tlb_mask(const struct domain *d, const cpumask_t *mask);
+
+static inline arch_grant_cache_flush(unsigned int op, const void *p, 
unsigned long size)
  {
-    unsigned int order = get_order_from_bytes(size);
+    unsigned int order;
+
+    if ( !(cflush->op & GNTTAB_CACHE_CLEAN) )
+        return -EOPNOTSUPP;
+
+    order = get_order_from_bytes(size);
      /* sub-page granularity support needs to be added if necessary */
      flush_area_local(p, FLUSH_CACHE|FLUSH_ORDER(order));
+
      return 0;
  }
-static inline int clean_dcache_va_range(const void *p, unsigned long size)
-{
-    return clean_and_invalidate_dcache_va_range(p, size);
-}
-
-unsigned int guest_flush_tlb_flags(const struct domain *d);
-void guest_flush_tlb_mask(const struct domain *d, const cpumask_t *mask);

  #endif /* __FLUSHTLB_H__ */

I have a slight preference for the latter. I would like to hear the 
opinion of the others.

Cheers,

-- 
Julien Grall

Re: Return type of clean_and_invalidate_dcache_va_range

[Jan Beulich]

On 12.02.2024 19:38, Julien Grall wrote:
> An alternative would be to introduced arch_grant_cache_flush() and move 
> the if/else logic there. Something like:
> 
> diff --git a/xen/arch/arm/include/asm/page.h 
> b/xen/arch/arm/include/asm/page.h
> index 69f817d1e68a..4a3de49762a1 100644
> --- a/xen/arch/arm/include/asm/page.h
> +++ b/xen/arch/arm/include/asm/page.h
> @@ -281,6 +281,19 @@ static inline void write_pte(lpae_t *p, lpae_t pte)
>       dsb(sy);
>   }
> 
> +static inline arch_grant_cache_flush(unsigned int op, const void *p, 
> unsigned long size)
> +{
> +    unsigned int order = get_order_from_bytes(size);
> +
> +    if ( (cflush->op & GNTTAB_CACHE_INVAL) && (cflush->op & 
> GNTTAB_CACHE_CLEAN) )
> +        clean_and_invalidate_dcache_va_range(v, cflush->length);
> +    else if ( cflush->op & GNTTAB_CACHE_INVAL )
> +        invalidate_dcache_va_range(v, cflush->length);
> +    else if ( cflush->op & GNTTAB_CACHE_CLEAN )
> +        clean_dcache_va_range(v, cflush->length);
> +
> +    return 0;
> +}
> 
>   /* Flush the dcache for an entire page. */
>   void flush_page_to_ram(unsigned long mfn, bool sync_icache);
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 424744ad5e1a..647e1522466d 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -735,8 +735,7 @@ void asmlinkage __init start_xen(unsigned long 
> boot_phys_offset,
>                 fdt_paddr);
> 
>       /* Register Xen's load address as a boot module. */
> -    xen_bootmodule = add_boot_module(BOOTMOD_XEN,
> -                             virt_to_maddr(_start),
> +    xen_bootmodule = add_boot_module(BOOTMOD_XEN, virt_to_maddr(_start),
>                                (paddr_t)(uintptr_t)(_end - _start), false);
>       BUG_ON(!xen_bootmodule);
> 
> diff --git a/xen/arch/x86/include/asm/flushtlb.h 
> b/xen/arch/x86/include/asm/flushtlb.h
> index bb0ad58db49b..dfe51cddde90 100644
> --- a/xen/arch/x86/include/asm/flushtlb.h
> +++ b/xen/arch/x86/include/asm/flushtlb.h
> @@ -182,23 +182,22 @@ void flush_area_mask(const cpumask_t *mask, const 
> void *va,
>   }
> 
>   static inline void flush_page_to_ram(unsigned long mfn, bool 
> sync_icache) {}
> -static inline int invalidate_dcache_va_range(const void *p,
> -                                             unsigned long size)
> -{ return -EOPNOTSUPP; }
> -static inline int clean_and_invalidate_dcache_va_range(const void *p,
> -                                                       unsigned long size)
> +
> +unsigned int guest_flush_tlb_flags(const struct domain *d);
> +void guest_flush_tlb_mask(const struct domain *d, const cpumask_t *mask);
> +
> +static inline arch_grant_cache_flush(unsigned int op, const void *p, 
> unsigned long size)
>   {
> -    unsigned int order = get_order_from_bytes(size);
> +    unsigned int order;
> +
> +    if ( !(cflush->op & GNTTAB_CACHE_CLEAN) )
> +        return -EOPNOTSUPP;
> +
> +    order = get_order_from_bytes(size);
>       /* sub-page granularity support needs to be added if necessary */
>       flush_area_local(p, FLUSH_CACHE|FLUSH_ORDER(order));
> +
>       return 0;
>   }
> -static inline int clean_dcache_va_range(const void *p, unsigned long size)
> -{
> -    return clean_and_invalidate_dcache_va_range(p, size);
> -}
> -
> -unsigned int guest_flush_tlb_flags(const struct domain *d);
> -void guest_flush_tlb_mask(const struct domain *d, const cpumask_t *mask);
> 
>   #endif /* __FLUSHTLB_H__ */
> 
> I have a slight preference for the latter. I would like to hear the 
> opinion of the others.

I would prefer this 2nd form, too, assuming the setup.c change wasn't
really meant to be there. The one thing that doesn't become clear: In
the sketch above arch_grant_cache_flush() has no return type, yet has
"return 0". This raises a question towards the one that's at the root
of this thread: Do you mean the function to have a return value, and
if so will it be (sensibly) used?

Jan

Re: Return type of clean_and_invalidate_dcache_va_range

[Julien Grall]

Hi Jan,

On 13/02/2024 07:13, Jan Beulich wrote:
> On 12.02.2024 19:38, Julien Grall wrote:
>> An alternative would be to introduced arch_grant_cache_flush() and move
>> the if/else logic there. Something like:
>>
>> diff --git a/xen/arch/arm/include/asm/page.h
>> b/xen/arch/arm/include/asm/page.h
>> index 69f817d1e68a..4a3de49762a1 100644
>> --- a/xen/arch/arm/include/asm/page.h
>> +++ b/xen/arch/arm/include/asm/page.h
>> @@ -281,6 +281,19 @@ static inline void write_pte(lpae_t *p, lpae_t pte)
>>        dsb(sy);
>>    }
>>
>> +static inline arch_grant_cache_flush(unsigned int op, const void *p,
>> unsigned long size)
>> +{
>> +    unsigned int order = get_order_from_bytes(size);
>> +
>> +    if ( (cflush->op & GNTTAB_CACHE_INVAL) && (cflush->op &
>> GNTTAB_CACHE_CLEAN) )
>> +        clean_and_invalidate_dcache_va_range(v, cflush->length);
>> +    else if ( cflush->op & GNTTAB_CACHE_INVAL )
>> +        invalidate_dcache_va_range(v, cflush->length);
>> +    else if ( cflush->op & GNTTAB_CACHE_CLEAN )
>> +        clean_dcache_va_range(v, cflush->length);
>> +
>> +    return 0;
>> +}
>>
>>    /* Flush the dcache for an entire page. */
>>    void flush_page_to_ram(unsigned long mfn, bool sync_icache);
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index 424744ad5e1a..647e1522466d 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -735,8 +735,7 @@ void asmlinkage __init start_xen(unsigned long
>> boot_phys_offset,
>>                  fdt_paddr);
>>
>>        /* Register Xen's load address as a boot module. */
>> -    xen_bootmodule = add_boot_module(BOOTMOD_XEN,
>> -                             virt_to_maddr(_start),
>> +    xen_bootmodule = add_boot_module(BOOTMOD_XEN, virt_to_maddr(_start),
>>                                 (paddr_t)(uintptr_t)(_end - _start), false);
>>        BUG_ON(!xen_bootmodule);
>>
>> diff --git a/xen/arch/x86/include/asm/flushtlb.h
>> b/xen/arch/x86/include/asm/flushtlb.h
>> index bb0ad58db49b..dfe51cddde90 100644
>> --- a/xen/arch/x86/include/asm/flushtlb.h
>> +++ b/xen/arch/x86/include/asm/flushtlb.h
>> @@ -182,23 +182,22 @@ void flush_area_mask(const cpumask_t *mask, const
>> void *va,
>>    }
>>
>>    static inline void flush_page_to_ram(unsigned long mfn, bool
>> sync_icache) {}
>> -static inline int invalidate_dcache_va_range(const void *p,
>> -                                             unsigned long size)
>> -{ return -EOPNOTSUPP; }
>> -static inline int clean_and_invalidate_dcache_va_range(const void *p,
>> -                                                       unsigned long size)
>> +
>> +unsigned int guest_flush_tlb_flags(const struct domain *d);
>> +void guest_flush_tlb_mask(const struct domain *d, const cpumask_t *mask);
>> +
>> +static inline arch_grant_cache_flush(unsigned int op, const void *p,
>> unsigned long size)
>>    {
>> -    unsigned int order = get_order_from_bytes(size);
>> +    unsigned int order;
>> +
>> +    if ( !(cflush->op & GNTTAB_CACHE_CLEAN) )
>> +        return -EOPNOTSUPP;
>> +
>> +    order = get_order_from_bytes(size);
>>        /* sub-page granularity support needs to be added if necessary */
>>        flush_area_local(p, FLUSH_CACHE|FLUSH_ORDER(order));
>> +
>>        return 0;
>>    }
>> -static inline int clean_dcache_va_range(const void *p, unsigned long size)
>> -{
>> -    return clean_and_invalidate_dcache_va_range(p, size);
>> -}
>> -
>> -unsigned int guest_flush_tlb_flags(const struct domain *d);
>> -void guest_flush_tlb_mask(const struct domain *d, const cpumask_t *mask);
>>
>>    #endif /* __FLUSHTLB_H__ */
>>
>> I have a slight preference for the latter. I would like to hear the
>> opinion of the others.
> 
> I would prefer this 2nd form, too, assuming the setup.c change wasn't
> really meant to be there. 

Indeed. I had another previous change I didn't and forgot to remove it.

> The one thing that doesn't become clear: In
> the sketch above arch_grant_cache_flush() has no return type, yet has
> "return 0". This raises a question towards the one that's at the root
> of this thread: Do you mean the function to have a return value, and
> if so will it be (sensibly) used?

Sorry I should have double checked the code before sending it. 
arch_grant_cache_flush() should return a value. So each arch can decide 
if they handle a given operation.

Cheers,

-- 
Julien Grall

Re: Return type of clean_and_invalidate_dcache_va_range

[Nicola Vetrini]

[-- Attachment #1: Type: text/plain, Size: 4820 bytes --]

Hi Julien,

On 2024-02-13 18:14, Julien Grall wrote:
> Hi Jan,
> 
> On 13/02/2024 07:13, Jan Beulich wrote:
>> On 12.02.2024 19:38, Julien Grall wrote:
>>> An alternative would be to introduced arch_grant_cache_flush() and 
>>> move
>>> the if/else logic there. Something like:
>>> 
>>> diff --git a/xen/arch/arm/include/asm/page.h
>>> b/xen/arch/arm/include/asm/page.h
>>> index 69f817d1e68a..4a3de49762a1 100644
>>> --- a/xen/arch/arm/include/asm/page.h
>>> +++ b/xen/arch/arm/include/asm/page.h
>>> @@ -281,6 +281,19 @@ static inline void write_pte(lpae_t *p, lpae_t 
>>> pte)
>>>        dsb(sy);
>>>    }
>>> 
>>> +static inline arch_grant_cache_flush(unsigned int op, const void *p,
>>> unsigned long size)
>>> +{
>>> +    unsigned int order = get_order_from_bytes(size);
>>> +
>>> +    if ( (cflush->op & GNTTAB_CACHE_INVAL) && (cflush->op &
>>> GNTTAB_CACHE_CLEAN) )
>>> +        clean_and_invalidate_dcache_va_range(v, cflush->length);
>>> +    else if ( cflush->op & GNTTAB_CACHE_INVAL )
>>> +        invalidate_dcache_va_range(v, cflush->length);
>>> +    else if ( cflush->op & GNTTAB_CACHE_CLEAN )
>>> +        clean_dcache_va_range(v, cflush->length);
>>> +
>>> +    return 0;
>>> +}
>>> 
>>>    /* Flush the dcache for an entire page. */
>>>    void flush_page_to_ram(unsigned long mfn, bool sync_icache);
>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>>> index 424744ad5e1a..647e1522466d 100644
>>> --- a/xen/arch/arm/setup.c
>>> +++ b/xen/arch/arm/setup.c
>>> @@ -735,8 +735,7 @@ void asmlinkage __init start_xen(unsigned long
>>> boot_phys_offset,
>>>                  fdt_paddr);
>>> 
>>>        /* Register Xen's load address as a boot module. */
>>> -    xen_bootmodule = add_boot_module(BOOTMOD_XEN,
>>> -                             virt_to_maddr(_start),
>>> +    xen_bootmodule = add_boot_module(BOOTMOD_XEN, 
>>> virt_to_maddr(_start),
>>>                                 (paddr_t)(uintptr_t)(_end - _start), 
>>> false);
>>>        BUG_ON(!xen_bootmodule);
>>> 
>>> diff --git a/xen/arch/x86/include/asm/flushtlb.h
>>> b/xen/arch/x86/include/asm/flushtlb.h
>>> index bb0ad58db49b..dfe51cddde90 100644
>>> --- a/xen/arch/x86/include/asm/flushtlb.h
>>> +++ b/xen/arch/x86/include/asm/flushtlb.h
>>> @@ -182,23 +182,22 @@ void flush_area_mask(const cpumask_t *mask, 
>>> const
>>> void *va,
>>>    }
>>> 
>>>    static inline void flush_page_to_ram(unsigned long mfn, bool
>>> sync_icache) {}
>>> -static inline int invalidate_dcache_va_range(const void *p,
>>> -                                             unsigned long size)
>>> -{ return -EOPNOTSUPP; }
>>> -static inline int clean_and_invalidate_dcache_va_range(const void 
>>> *p,
>>> -                                                       unsigned long 
>>> size)
>>> +
>>> +unsigned int guest_flush_tlb_flags(const struct domain *d);
>>> +void guest_flush_tlb_mask(const struct domain *d, const cpumask_t 
>>> *mask);
>>> +
>>> +static inline arch_grant_cache_flush(unsigned int op, const void *p,
>>> unsigned long size)
>>>    {
>>> -    unsigned int order = get_order_from_bytes(size);
>>> +    unsigned int order;
>>> +
>>> +    if ( !(cflush->op & GNTTAB_CACHE_CLEAN) )
>>> +        return -EOPNOTSUPP;
>>> +
>>> +    order = get_order_from_bytes(size);
>>>        /* sub-page granularity support needs to be added if necessary 
>>> */
>>>        flush_area_local(p, FLUSH_CACHE|FLUSH_ORDER(order));
>>> +
>>>        return 0;
>>>    }
>>> -static inline int clean_dcache_va_range(const void *p, unsigned long 
>>> size)
>>> -{
>>> -    return clean_and_invalidate_dcache_va_range(p, size);
>>> -}
>>> -
>>> -unsigned int guest_flush_tlb_flags(const struct domain *d);
>>> -void guest_flush_tlb_mask(const struct domain *d, const cpumask_t 
>>> *mask);
>>> 
>>>    #endif /* __FLUSHTLB_H__ */
>>> 
>>> I have a slight preference for the latter. I would like to hear the
>>> opinion of the others.
>> 
>> I would prefer this 2nd form, too, assuming the setup.c change wasn't
>> really meant to be there.
> 
> Indeed. I had another previous change I didn't and forgot to remove it.
> 
>> The one thing that doesn't become clear: In
>> the sketch above arch_grant_cache_flush() has no return type, yet has
>> "return 0". This raises a question towards the one that's at the root
>> of this thread: Do you mean the function to have a return value, and
>> if so will it be (sensibly) used?
> 
> Sorry I should have double checked the code before sending it. 
> arch_grant_cache_flush() should return a value. So each arch can decide 
> if they handle a given operation.
> 
> Cheers,

I do like the idea. I applied some of the suggestions to this proof of 
concept patch (attached). Still not compile-tested, since the CI seems a 
bit slow today.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: cache_helpers.patch --]
[-- Type: text/x-diff; name=cache_helpers.patch, Size: 5715 bytes --]

diff --git a/xen/arch/arm/include/asm/page.h b/xen/arch/arm/include/asm/page.h
index 69f817d1e68a..5f9357632164 100644
--- a/xen/arch/arm/include/asm/page.h
+++ b/xen/arch/arm/include/asm/page.h
@@ -159,13 +159,13 @@ static inline size_t read_dcache_line_bytes(void)
  * if 'range' is large enough we might want to use model-specific
  * full-cache flushes. */
 
-static inline int invalidate_dcache_va_range(const void *p, unsigned long size)
+static inline void invalidate_dcache_va_range(const void *p, unsigned long size)
 {
     size_t cacheline_mask = dcache_line_bytes - 1;
     unsigned long idx = 0;
 
     if ( !size )
-        return 0;
+        return;
 
     /* Passing a region that wraps around is illegal */
     ASSERT(((uintptr_t)p + size - 1) >= (uintptr_t)p);
@@ -188,17 +188,15 @@ static inline int invalidate_dcache_va_range(const void *p, unsigned long size)
         asm volatile (__clean_and_invalidate_dcache_one(0) : : "r" (p + idx));
 
     dsb(sy);           /* So we know the flushes happen before continuing */
-
-    return 0;
 }
 
-static inline int clean_dcache_va_range(const void *p, unsigned long size)
+static inline void clean_dcache_va_range(const void *p, unsigned long size)
 {
     size_t cacheline_mask = dcache_line_bytes - 1;
     unsigned long idx = 0;
 
     if ( !size )
-        return 0;
+        return;
 
     /* Passing a region that wraps around is illegal */
     ASSERT(((uintptr_t)p + size - 1) >= (uintptr_t)p);
@@ -211,18 +209,16 @@ static inline int clean_dcache_va_range(const void *p, unsigned long size)
             idx += dcache_line_bytes, size -= dcache_line_bytes )
         asm volatile (__clean_dcache_one(0) : : "r" (p + idx));
     dsb(sy);           /* So we know the flushes happen before continuing */
-    /* ARM callers assume that dcache_* functions cannot fail. */
-    return 0;
 }
 
-static inline int clean_and_invalidate_dcache_va_range
+static inline void clean_and_invalidate_dcache_va_range
     (const void *p, unsigned long size)
 {
     size_t cacheline_mask = dcache_line_bytes - 1;
     unsigned long idx = 0;
 
     if ( !size )
-        return 0;
+        return;
 
     /* Passing a region that wraps around is illegal */
     ASSERT(((uintptr_t)p + size - 1) >= (uintptr_t)p);
@@ -235,8 +231,6 @@ static inline int clean_and_invalidate_dcache_va_range
             idx += dcache_line_bytes, size -= dcache_line_bytes )
         asm volatile (__clean_and_invalidate_dcache_one(0) : : "r" (p + idx));
     dsb(sy);         /* So we know the flushes happen before continuing */
-    /* ARM callers assume that dcache_* functions cannot fail. */
-    return 0;
 }
 
 /* Macros for flushing a single small item.  The predicate is always
@@ -266,6 +260,20 @@ static inline int clean_and_invalidate_dcache_va_range
             : : "r" (_p), "m" (*_p));                                   \
 } while (0)
 
+static inline int arch_grant_cache_flush(unsigned int op, const void *p,
+                                         unsigned long size)
+{
+    if ( (op & GNTTAB_CACHE_INVAL) && (op & GNTTAB_CACHE_CLEAN) )
+        clean_and_invalidate_dcache_va_range(v, size);
+    else if ( op & GNTTAB_CACHE_INVAL )
+        invalidate_dcache_va_range(v, size);
+    else if ( op & GNTTAB_CACHE_CLEAN )
+        clean_dcache_va_range(v, size);
+
+    /* ARM callers assume that dcache_* functions cannot fail. */
+    return 0;
+}
+
 /*
  * Write a pagetable entry.
  *
diff --git a/xen/arch/x86/include/asm/flushtlb.h b/xen/arch/x86/include/asm/flushtlb.h
index bb0ad58db49b..7c71fe377757 100644
--- a/xen/arch/x86/include/asm/flushtlb.h
+++ b/xen/arch/x86/include/asm/flushtlb.h
@@ -182,21 +182,21 @@ void flush_area_mask(const cpumask_t *mask, const void *va,
 }
 
 static inline void flush_page_to_ram(unsigned long mfn, bool sync_icache) {}
-static inline int invalidate_dcache_va_range(const void *p,
-                                             unsigned long size)
-{ return -EOPNOTSUPP; }
-static inline int clean_and_invalidate_dcache_va_range(const void *p,
-                                                       unsigned long size)
+
+static inline arch_grant_cache_flush(unsigned int op, const void *p,
+                                     unsigned long size)
 {
-    unsigned int order = get_order_from_bytes(size);
+    unsigned int order;
+
+    if ( !(cflush->op & GNTTAB_CACHE_CLEAN) )
+        return -EOPNOTSUPP;
+
+    order = get_order_from_bytes(size);
     /* sub-page granularity support needs to be added if necessary */
     flush_area_local(p, FLUSH_CACHE|FLUSH_ORDER(order));
+
     return 0;
 }
-static inline int clean_dcache_va_range(const void *p, unsigned long size)
-{
-    return clean_and_invalidate_dcache_va_range(p, size);
-}
 
 unsigned int guest_flush_tlb_flags(const struct domain *d);
 void guest_flush_tlb_mask(const struct domain *d, const cpumask_t *mask);
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 5721eab22561..8615ea144bb3 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -3572,14 +3572,7 @@ static int _cache_flush(const gnttab_cache_flush_t *cflush, grant_ref_t *cur_ref
     v = map_domain_page(mfn);
     v += cflush->offset;
 
-    if ( (cflush->op & GNTTAB_CACHE_INVAL) && (cflush->op & GNTTAB_CACHE_CLEAN) )
-        ret = clean_and_invalidate_dcache_va_range(v, cflush->length);
-    else if ( cflush->op & GNTTAB_CACHE_INVAL )
-        ret = invalidate_dcache_va_range(v, cflush->length);
-    else if ( cflush->op & GNTTAB_CACHE_CLEAN )
-        ret = clean_dcache_va_range(v, cflush->length);
-    else
-        ret = 0;
+    ret = arch_grant_cache_flush(cflush->op, v, cflush->length);
 
     if ( d != owner )
     {

Re: Return type of clean_and_invalidate_dcache_va_range

[Nicola Vetrini]

> 
> I do like the idea. I applied some of the suggestions to this proof of 
> concept patch (attached). Still not compile-tested, since the CI seems 
> a bit slow today.

arch_grant_cache_flush is missing the return type on x86, sorry.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)