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

Reply via email to