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?
#endifreturn 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_64I 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
