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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4293 bytes --]


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.

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

>
> Plus of course this gets us into feature detection territory again: If
> RISC-V provided a way to detect presence / absence of certain extensions,
> this really shouldn't be a compile time setting, but be determined
> dynamically.

This is the next patch I plan to send after this patch series:
https://gitlab.com/xen-project/people/olkur/xen/-/commit/f81ae67c42854073da5403210c9e31de6b0ee5bd <https://gitlab.com/xen-project/people/olkur/xen/-/commit/f81ae67c42854073da5403210c9e31de6b0ee5bd>

It "detects" available extensions based on a device tree property. While this is not the best approach
(the ideal solution would be hardware having a register that lists all available extensions), it seems to be
the best option available at the moment.

Another option I considered was introducing a new SBI call, delegating the responsibility to OpenSBI
to provide this information.

>
>>   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
```
and that G is needed to boot Linux kernel ( and so Xen ) I make an assumption that Zifencei will be always
present.

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.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 6066 bytes --]