Re: [PATCH v1 4/6] xen/riscv: introduce cache management operations (CMO)

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.12.2024 13:19, Oleksii Kurochko wrote:
> 
> On 12/9/24 3:38 PM, Jan Beulich wrote:
>> On 27.11.2024 13:50, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/Kconfig
>>> +++ b/xen/arch/riscv/Kconfig
>>> @@ -14,6 +14,9 @@ config ARCH_DEFCONFIG
>>>       string
>>>       default "arch/riscv/configs/tiny64_defconfig"
>>>   +config HAS_CMO # Cache Management Operations
>>> +    bool
>> Hmm, and nothing ever sets this, and hence ...
>>
>>> @@ -148,9 +149,24 @@ static inline bool pte_is_mapping(pte_t p)
>>>       return (p.pte & PTE_VALID) && (p.pte & PTE_ACCESS_MASK);
>>>   }
>>>   +#ifndef HAS_CMO
>>> +static inline int clean_and_invalidate_dcache_va_range(const void *p, 
>>> unsigned long size)
>>> +{
>>> +    return -EOPNOTSUPP;
>>> +}
>>> +
>>> +static inline int clean_dcache_va_range(const void *p, unsigned long size)
>>> +{
>>> +    return -EOPNOTSUPP;
>>> +}
>>> +#else
>>> +int clean_and_invalidate_dcache_va_range(const void *p, unsigned long 
>>> size);
>>> +int clean_dcache_va_range(const void *p, unsigned long size);
>>> +#endif
>> ... all you really provide are stubs and declarations, but no
>> definition anywhere?
> 
> Yes, this was done intentionally because:
> - I don't have hardware with the CMO extension, so I can't test it. ( QEMU 
> doesn't model cache and so
>   there is no need for CMO extension emulation IIUC )
> - The instructions used for these functions may be hardware-specific and 
> exist only for particular devices.
> 
> It seems useful to have something similar to Linux:
> https://elixir.bootlin.com/linux/v6.6.64/source/arch/riscv/include/asm/errata_list.h#L135
>  
> <https://elixir.bootlin.com/linux/v6.6.64/source/arch/riscv/include/asm/errata_list.h#L135>
> (There are also custom instructions for THEAD above this macro.)
> 
> We could use|ALT_CMO_OP(...)| inside|clean_and_invalidate_dcache_va_range()| 
> and|clean_dcache_va_range()|.
> However, I think it would be better to introduce or implement these functions 
> when|HAS_CMO| is set to|y| someday.
> 
> As an alternative, we could implement these functions as|panic("need to be 
> implemented\n")| in case when HAS_CMO=y.

I think this would be well in line with various other stubs you have.

> Another option is to drop|HAS_CMO| entirely for now and keep the current 
> implementation (|return -EOPNOTSUPP|).
> However, with this approach, there's a risk of encountering hard-to-debug 
> issues on platforms with the CMO extension.
> And necessity of implementation of these could be missed because there is no 
> any notification...

Well, callers ought to check return values?

>>>   static inline void invalidate_icache(void)
>>>   {
>>> -    BUG_ON("unimplemented");
>>> +    asm volatile ( "fence.i" ::: "memory" );
>>>   }
>> That's a separate extension, Zifencei, which I don't think you can just
>> assume to be present?
> 
> Based on the specification:
> ```
> Chapter 34. RV32/64G Instruction Set Listings
> One goal of the RISC-V project is that it be used as a stable software 
> development target. For this
> purpose, we define a combination of a base ISA (RV32I or RV64I) plus selected 
> standard extensions
> (IMAFD, Zicsr, Zifencei) as a "general-purpose" ISA, and we use the 
> abbreviation G for the
> IMAFDZicsr_Zifencei combination of instruction-set extensions. This chapter 
> presents opcode maps
> and instruction-set listings for RV32G and RV64G
> ```

Hmm, indeed. That's well hidden in a place I didn't expect it to live at.
Maybe worth a sentence in the description?

> and that G is needed to boot Linux kernel ( and so Xen ) I make an assumption 
> that Zifencei will be always
> present.

I'd be a little careful here. Xen may be used in Linux-free environments.
I notice arch.mk specifies rv64g, yet I'm uncertain we shouldn't relax
that at some point.

> And based on Linux code 
> (https://elixir.bootlin.com/linux/v6.12.4/source/arch/riscv/kernel/cpufeature.c#L676
>  )
> when 'i' is present in riscv,isa property zifencei is present unconditionally.

That looks questionable to me. I don't think Zifencei can be inferred from
I. Historically it was, and imo that's what the comment there says. Plus
it is dependent upon acpi_disabled.

Jan