Re: [PATCH v2 5/7] xen/riscv: implement data and instruction cache operations

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 12/17/24 9:32 AM, Jan Beulich wrote:

> On 16.12.2024 18:40, Oleksii Kurochko wrote:
> > On 12/16/24 3:23 PM, Jan Beulich wrote:
> > > On 11.12.2024 18:27, Oleksii Kurochko wrote:
> > > > --- a/xen/arch/riscv/include/asm/page.h
> > > > +++ b/xen/arch/riscv/include/asm/page.h
> > > > @@ -7,6 +7,7 @@
> > > >   
> > > >   #include <xen/bug.h>
> > > >   #include <xen/const.h>
> > > > +#include <xen/errno.h>
> > > >   #include <xen/types.h>
> > > >   
> > > >   #include <asm/atomic.h>
> > > > @@ -148,9 +149,27 @@ static inline bool pte_is_mapping(pte_t p)
> > > >       return (p.pte & PTE_VALID) && (p.pte & PTE_ACCESS_MASK);
> > > >   }
> > > >   
> > > > +static inline int clean_and_invalidate_dcache_va_range(const void *p, unsigned long size)
> > > > +{
> > > > +#ifdef CONFIG_QEMU
> > > > +    return 0;
> > > > +#else
> > > > +    return -EOPNOTSUPP;
> > > > +#endif
> > > > +}
> > > > +
> > > > +static inline int clean_dcache_va_range(const void *p, unsigned long size)
> > > > +{
> > > > +#ifdef CONFIG_QEMU
> > > > +    return 0;
> > > > +#else
> > > > +    return -EOPNOTSUPP;
> > > > +#endif
> > > > +}
> > > >           
> > > So testing on real hardware will then effectively become impossible, until
> > > someone goes and implements these?
> > >         
> > Yes...
> >
> > I am not sure what better we can do. It seems like it will be the best one to check if CMO
> > extensions is supported and use instructions for this extensions to implement these functions as they
> > are in the specification and not expected to be changed.
> >       
> Yes, using CMO when available is certainly the route to go. The main
> question there is what the behavior ought to be when CMO is unavailable.
>     
If CMO ( or SoC specific extension for cache operation ) isn't available then IMO it means that memory is
coherent and nothing specific should be done in the mentioned above functions what means returning 0 should
be fine. Then implementation of these functions could look like:
```
static inline int <.....>(....)
{
#if !defined(CONFIG_QEMU)
> #warning should implementation of <....>  be updated?
#endif

> return 0;
}
```
Or just to be sure that user see the message change #warning -> #error.

~ Oleksii
> > But I want to back a little bit later to this implemntation as this not issue for QEMU as it doesn't model cache and
> > h/w on which I can ask to run Xen has IO cache coherency so for it will be needed just to add a new config
> > and implementation will still be 'return 0'. ( I thought to introduce instead of CONFIG_QEMU something like
> > CONFIG_HAS_CACHE_COHERENCY )
> >
> > And also in the spec it is mentioned:
> > ```
> > This suggests that RISC-V platforms prefer to support full 
> > cache-coherent I/O, but it isn't actually mandatory.
> > As a result, the PMBT and CMO extensions aren't mandatory either, 
> > meaning that some platforms might not
> > have instructions to properly flush, clean, or invalidate the cache.
> > ``` Based on that I also think to implement that in the following way:
> > ```
> >      #ifdef CONFIG_QEMU
> >      static inline int plat_clean_and_invalidate_dcache_va_range() { return 0; }
> >    static inline int plat_clean_dcache_va_range() { return 0; }
> >    #else /* !CONFIG_QEMU */
> >      static inline void plat_clean_and_invalidate_dcache_va_range()
> >    {
> >      printk_once("%s: should it be implemented for your platform?\n", __func__);
> >      return 0;
> >    }
> >
> >    static inline void plat_clean_dcache_va_range()
> >    {
> >      printk_once("%s: should it be implemented for your platform?\n", __func__);
> >      return 0;
> >    }
> >    #endif
> >
> >    static inline int clean_and_invalidate_dcache_va_range(const void *p, unsigned long size)
> >    {
> >        return plat_clean_and_invalidate_dcache_va_range();
> >    }
> > ....
> > ```
> > So we will have a notification for others none-QEMU platforms notification that probably some
> > changes are required.
> >       
> Yet failing to get cache management right can easily result in data corruption.
> I don't think a on-time printk() is appropriate to handle the lack of respective
> implementation. At least not anymore once RISC-V leaves "experimental" status.
>
> > > > --- /dev/null
> > > > +++ b/xen/arch/riscv/platforms/Kconfig
> > > > @@ -0,0 +1,5 @@
> > > > +config QEMU
> > > > +	bool "QEMU aarch virt machine support"
> > > > +	depends on RISCV_64
> > > >           
> > > I understand Arm has it like this, but: Is QEMU really a sufficiently non-
> > > ambiguous name to use?
> > >         
> > Yes, it sounds good to me to have such naming for the platform which are running on top of QEMU.
> >
> > The other option I thought about it is to use CONFIG_VIRT_PLATFORM.
> >       
> I don't think QEMU should be fully omitted from the name. Nor do I think that
> you can generally infer from "virtual platform" that caches aren't modeled.
> What I was rather getting at is to perhaps add some qualifier to QEMU, e.g.
> QEMU_PLATFORM.
>
> Jan
>