KConfig HAS_CMO is introduced to handle if the platform has CMO related
extenstions ( such as Zicbom, Zicboz, Zicbop etc ) or not.
if HAS_CMO isn't set stubs for clean_and_invalidate_dcache_va_range()
and clean_dcache_va_range() are implemented as just returning
-EOPNOTSUPP.
Our current platform is QEMU which doesn't model caches so it should be
fine to follow implementations when HAS_CMO isn't set.
invalidate_icache() is implemented using fence.i instruction as
mentioned in the unpriv spec:
The FENCE.I instruction was designed to support a wide variety of
implementations. A simple implementation can flush the local instruction
cache and the instruction pipeline when the FENCE.I is executed.
A more complex implementation might snoop the instruction (data) cache
on every data (instruction) cache miss, or use an inclusive unified
private L2 cache to invalidate lines from the primary instruction cache
when they are being written by a local store instruction.
If instruction and data caches are kept coherent in this way, or if the
memory system consists of only uncached RAMs, then just the fetch pipeline
needs to be flushed at a FENCE.I.
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
xen/arch/riscv/Kconfig | 3 +++
xen/arch/riscv/include/asm/page.h | 18 +++++++++++++++++-
2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/xen/arch/riscv/Kconfig b/xen/arch/riscv/Kconfig
index 1858004676..4f1fcfd21a 100644
--- 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
+
menu "Architecture Features"
source "arch/Kconfig"
diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h
index bf3f75e85d..0f297141d3 100644
--- 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,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
+
static inline void invalidate_icache(void)
{
- BUG_ON("unimplemented");
+ asm volatile ( "fence.i" ::: "memory" );
}
#define clear_page(page) memset((void *)(page), 0, PAGE_SIZE)
--
2.47.0On 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?
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.
> 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?
Jan
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
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
On 12/10/24 1:39 PM, Jan Beulich wrote:
> 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?
Yeah, callers should check return value but we still have to introduce then a new KConfig ( config QEMU ) and then implementation
will look like:
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
}
>
>>>> 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?
Sure, I will update the description. ( and probably add the comment above invalidate_icache() function )
>> 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.
Agree with you here. And it was the reason why I dropped this if-condition when I ported cpufeature.c to Xen.
~ Oleksii
© 2016 - 2024 Red Hat, Inc.