Re: Return type of clean_and_invalidate_dcache_va_range

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

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)

Attachment: cache_helpers.patch
Description: Text Data