Re: Return type of clean_and_invalidate_dcache_va_range

[Julien Grall <julien@xxxxxxx>]

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