Re: Return type of clean_and_invalidate_dcache_va_range

[Jan Beulich <jbeulich@xxxxxxxx>]

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