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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> 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