[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v1 4/6] xen/riscv: introduce cache management operations (CMO)
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
|
