On 2/17/26 12:37 PM, Jan Beulich wrote:
On 13.02.2026 17:28, Oleksii Kurochko wrote:
Some hypervisor CSRs expose optional functionality and may not implement
all architectural bits. Writing unsupported bits can either be ignored
or raise an exception depending on the platform.
Detect the set of writable bits for selected hypervisor CSRs at boot and
store the resulting masks for later use. This allows safely programming
these CSRs during vCPU context switching and avoids relying on hardcoded
architectural assumptions.
Note that csr_set() is used instead of csr_write() to write all ones to
the mask, as the CSRRS instruction, according to the RISC-V specification,
sets only those bits that are writable (note that the quote consider only
non-read-only CSRs as writing to read-only CSRs according to the spec.
will raise an exception):
Any bit that is high in rs1 will cause the corresponding bit to be set
in the CSR, if that CSR bit is writable.
In contrast, the CSRRW instruction does not take CSR bit writability into
account, which could lead to unintended side effects when writing all ones
to a CSR.
Masks are calculated at the moment only for hedeleg, henvcfg, hideleg,
hstateen0 registers as only them are going to be used in the follow up
patch.
If the Smstateen extension is not implemented, hstateen0 cannot be read
because the register is considered non-existent. Instructions that attempt
to access a CSR that is not implemented or not visible in the current mode
are reserved and will raise an illegal-instruction exception.
Signed-off-by: Oleksii Kurochko <[email protected]>
---
Changes in V4:
- Move csr_masks defintion to domain.c. Make it static as at the moment
it is going to be used only in domain.c.
Except that ...
--- a/xen/arch/riscv/domain.c
+++ b/xen/arch/riscv/domain.c
@@ -2,9 +2,14 @@
#include <xen/init.h>
#include <xen/mm.h>
+#include <xen/sections.h>
#include <xen/sched.h>
#include <xen/vmap.h>
+#include <asm/setup.h>
+
+struct csr_masks __ro_after_init csr_masks;
... it's not static here and even has ...
--- a/xen/arch/riscv/include/asm/setup.h
+++ b/xen/arch/riscv/include/asm/setup.h
@@ -5,6 +5,15 @@
#include <xen/types.h>
+struct csr_masks {
+ register_t hedeleg;
+ register_t henvcfg;
+ register_t hideleg;
+ register_t hstateen0;
+};
+
+extern struct csr_masks csr_masks;
... a declaration here. If you want to keep it non-static, it (and the struct
decl) likely wants moving elsewhere. Whereas if you truly want it to be static,
the struct decl would want moving to domain.c as well.
Wrong patch version. I made it static so csr_masks declaration and struct
csr_masks
were in domain.c. Also, init_csr_masks() was moved to domain.c too.
I will update the patch version next time.
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -70,6 +70,25 @@ static void * __init relocate_fdt(paddr_t dtb_paddr, size_t
dtb_size)
return fdt;
}
+void __init init_csr_masks(void)
+{
+ register_t old;
As indicated before, this would better be ...
+#define INIT_CSR_MASK(csr, field) do { \
+ old = csr_read(CSR_##csr); \
+ csr_set(CSR_##csr, ULONG_MAX); \
+ csr_masks.field = csr_read(CSR_##csr); \
+ csr_write(CSR_##csr, old); \
+} while (0)
... local to the scope the macro introduces. IOW with both this and the
earlier remark, let's try to strive to have scope and exposure of variables
as narrow as possible (unless of course there are clear reasons not to).
And btw, wouldn't you again better use csr_swap() here?
It makes sense, I'll use csr_swap() instead of pair csr_read() and csr_write().
Thanks.
~ Oleksii
