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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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. 

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.
> > --- /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.

>  Is the RISCV_64 dependency really needed?
>     
At the moment , we are testing only RISCV_64 so not to have none-verified config I added RISCV_64 but
probably it is too much and `depends on` should be dropped at all.

Thanks.

~ Oleksii
> > +	help
> > +	  Enable all the required drivers for QEMU riscv64 virt emulated machine.
> >       
> This line looks to be slightly too long now (after you apparently unwrapped
> what Arm has).
>
> Jan
>