On 06.03.2026 17:33, 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. > > Use csr_read()&csr_write() instead of csr_swap()+all ones mask as some > CSR registers have WPRI fields which should be preserved during write > operation. > > Also, ro_one struct is introduced to cover the cases when a bit in CSR > register (at the momemnt, it is only hstateen0) may be r/o-one to have > hypervisor view of register seen by guest correct. > > 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]>
Acked-by: Jan Beulich <[email protected]> I'll commit as-is, yet still a couple of remarks: > --- > Changes in V7: > - Use csr_read_set() in INIT_CSR_MASK() instead of csr_read()+csr_write(). > - Add undef of INIT_CSR_MASK(). > - Move local variable old above INIT_CSR_MASK(). This contradicts ... > - Introduce INIT_RO_ONE_MASK() to init csr_masks.ro_one.* fields. > - Introduce defines for masks intead of constants. > - Move old variable inside macros INIT_CSR_MASK() and INIT_RO_ONE_MASK(). ... this. You may want to prune revlog entries when making incremental changes within one revision. > --- a/xen/arch/riscv/domain.c > +++ b/xen/arch/riscv/domain.c > @@ -2,9 +2,66 @@ > > #include <xen/init.h> > #include <xen/mm.h> > +#include <xen/sections.h> > #include <xen/sched.h> > #include <xen/vmap.h> > > +#include <asm/cpufeature.h> > +#include <asm/csr.h> > + > +struct csr_masks { > + register_t hedeleg; > + register_t henvcfg; > + register_t hideleg; > + register_t hstateen0; > + > + struct { > + register_t hstateen0; > + } ro_one; > +}; > + > +static struct csr_masks __ro_after_init csr_masks; > + > +#define HEDELEG_AVAIL_MASK ULONG_MAX > +#define HIDELEG_AVAIL_MASK ULONG_MAX > +#define HENVCFG_AVAIL_MASK _UL(0xE0000003000000FF) > +#define HSTATEEN0_AVAIL_MASK _UL(0xDE00000000000007) It's not quite clear to me what AVAIL in here is to signal. It's also not quite clear to me why you would use _UL() in #define-s sitting in a C file (and hence not possibly being used in assembly code; even for asm() I'd expect constants to be properly passed in as C operands). > +void __init init_csr_masks(void) > +{ > + /* > + * The mask specifies the bits that may be safely modified without > + * causing side effects. > + * > + * For example, registers such as henvcfg or hstateen0 contain WPRI > + * fields that must be preserved. Any write to the full register must > + * therefore retain the original values of those fields. > + */ > +#define INIT_CSR_MASK(csr, field, mask) do { \ > + register_t old = csr_read_set(CSR_##csr, mask); \ > + csr_masks.field = csr_swap(CSR_##csr, old); \ > + } while (0) > + > +#define INIT_RO_ONE_MASK(csr, field, mask) do { \ > + register_t old = csr_read_clear(CSR_HSTATEEN0, mask); \ > + csr_masks.ro_one.field = csr_swap(CSR_##csr, old) & mask; \ > + } while (0) > + > + INIT_CSR_MASK(HEDELEG, hedeleg, HEDELEG_AVAIL_MASK); > + INIT_CSR_MASK(HIDELEG, hideleg, HIDELEG_AVAIL_MASK); > + > + INIT_CSR_MASK(HENVCFG, henvcfg, HENVCFG_AVAIL_MASK); > + > + if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_smstateen) ) > + { > + INIT_CSR_MASK(HSTATEEN0, hstateen0, HSTATEEN0_AVAIL_MASK); > + INIT_RO_ONE_MASK(HSTATEEN0, hstateen0, HSTATEEN0_AVAIL_MASK); > + } The 3rd macro parameters are now redundant. At the example of INIT_CSR_MASK(), you could now have #define INIT_CSR_MASK(csr, field) do { \ register_t old = csr_read_set(CSR_ ## csr, csr ## _AVAIL_MASK); \ csr_masks.field = csr_swap(CSR_ ## csr, old); \ } while (0) This would reduce the risk of incomplete editing after copy-and-paste, or other typo-ing. Note also that ## being a binary operator, ./CODING_STYLE wants us to put blanks around it just like for non-pre-processor binary operators. I'll try to remember to make that adjustment when committing. Jan
