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
