|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 03/16] xen/riscv: implement vcpu_csr_init()
Le 22/01/2026 à 17:49, Oleksii Kurochko a écrit :
> Introduce vcpu_csr_init() to set up the initial hypervisor-visible CSR
> state for a vCPU before it is first scheduled.
To me, "hypervisor-visible CSR state" sounds like some state of the
guest the hypervisor has view on. But to what I read, it's more
hypervisor state CSRs regarding what to intercept and various
virtualization behavior.
> The function configures trap and interrupt delegation to VS-mode by
> setting the appropriate bits in the hedeleg and hideleg registers,
> initializes hstatus so that execution enters VS-mode when control is
> passed to the guest, and restricts guest access to hardware performance
> counters by initializing hcounteren, as unrestricted access would
> require additional handling in Xen.
> When the Smstateen and SSAIA extensions are available, access to AIA
> CSRs and IMSIC guest interrupt files is enabled by setting the
> corresponding bits in hstateen0, avoiding unnecessary traps into Xen
> (note that SVSLCT(Supervisor Virtual Select) name is used intead of
> CSRIND as OpenSBI uses such name and riscv_encoding.h is mostly based
> on it).
> If the Svpbmt extension is supported, the PBMTE bit is set in
> henvcfg to allow its use for VS-stage address translation. Guest
> access to the ENVCFG CSR is also enabled by setting ENVCFG bit in
> hstateen0, as a guest may need to control certain characteristics of
> the U-mode (VU-mode when V=1) execution environment.
>
> For CSRs that may contain read-only bits (e.g. hedeleg, hideleg,
> hstateen0), the written value is re-read from hardware and cached in
> vcpu->arch to avoid divergence between the software state and the actual
> CSR contents.
>
AFAIU from RISC-V isa document, at least for some CSRs the read-only-0
bits are well-defined and means "can't be configured" due to not having
a meaning (e.g hedeleg defines as read-only "Environment call from
HS-mode" because guest can't be in a "actual" HS-mode).
> As hstatus is not part of struct arch_vcpu (it already resides in
> struct cpu_user_regs), introduce vcpu_guest_cpu_user_regs() to provide
> a uniform way to access hstatus and other guest CPU user registers.
>
> This establishes a consistent and well-defined initial CSR state for
> vCPUs prior to their first context switch.
>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
> Changes in v2:
> - As hstatus isn't a part of arch_vcpu structure (as it is already a part of
> cpu_user_regs) introduce vcpu_guest_cpu_user_regs() to be able to access
> hstatus and other CPU user regs.
Sounds like hstatus wants to be splitted from guest_cpu_user_regs (which
are supposed to be GPR ?).
> - Sort hideleg bit setting by value. Drop a stray blank.
> - Drop | when the first initialization of hcounteren and hennvcfg happen.
> - Introduce HEDELEG_DEFAULT. Sort set bits by value and use BIT() macros
> instead of open-coding it.
> - Apply pattern csr_write() -> csr_read() for hedeleg and hideleg instead
> of direct bit setting in v->arch.h{i,e}deleg as it could be that for some
> reason some bits of hedeleg and hideleg are r/o.
> The similar patter is used for hstateen0 as some of the bits could be r/o.
> - Add check that SSAIA is avaialable before setting of SMSTATEEN0_AIA |
> SMSTATEEN0_IMSIC | SMSTATEEN0_SVSLCT bits.
> - Drop local variables hstatus, hideleg and hedeleg as they aren't used
> anymore.
> ---
> xen/arch/riscv/cpufeature.c | 1 +
> xen/arch/riscv/domain.c | 73 +++++++++++++++++++++
> xen/arch/riscv/include/asm/cpufeature.h | 1 +
> xen/arch/riscv/include/asm/current.h | 2 +
> xen/arch/riscv/include/asm/riscv_encoding.h | 2 +
> 5 files changed, 79 insertions(+)
>
> diff --git a/xen/arch/riscv/cpufeature.c b/xen/arch/riscv/cpufeature.c
> index 02b68aeaa49f..03e27b037be0 100644
> --- a/xen/arch/riscv/cpufeature.c
> +++ b/xen/arch/riscv/cpufeature.c
> @@ -137,6 +137,7 @@ const struct riscv_isa_ext_data __initconst
> riscv_isa_ext[] = {
> RISCV_ISA_EXT_DATA(zbb),
> RISCV_ISA_EXT_DATA(zbs),
> RISCV_ISA_EXT_DATA(smaia),
> + RISCV_ISA_EXT_DATA(smstateen),
> RISCV_ISA_EXT_DATA(ssaia),
> RISCV_ISA_EXT_DATA(svade),
> RISCV_ISA_EXT_DATA(svpbmt),
> diff --git a/xen/arch/riscv/domain.c b/xen/arch/riscv/domain.c
> index 9c546267881b..3ae5fa3a8805 100644
> --- a/xen/arch/riscv/domain.c
> +++ b/xen/arch/riscv/domain.c
> @@ -5,6 +5,77 @@
> #include <xen/sched.h>
> #include <xen/vmap.h>
>
> +#include <asm/cpufeature.h>
> +#include <asm/csr.h>
> +#include <asm/riscv_encoding.h>
> +
> +#define HEDELEG_DEFAULT (BIT(CAUSE_MISALIGNED_FETCH, U) | \
> + BIT(CAUSE_FETCH_ACCESS, U) | \
> + BIT(CAUSE_ILLEGAL_INSTRUCTION, U) | \
> + BIT(CAUSE_BREAKPOINT, U) | \
> + BIT(CAUSE_MISALIGNED_LOAD, U) | \
> + BIT(CAUSE_LOAD_ACCESS, U) | \
> + BIT(CAUSE_MISALIGNED_STORE, U) | \
> + BIT(CAUSE_STORE_ACCESS, U) | \
> + BIT(CAUSE_USER_ECALL, U) | \
> + BIT(CAUSE_FETCH_PAGE_FAULT, U) | \
> + BIT(CAUSE_LOAD_PAGE_FAULT, U) | \
> + BIT(CAUSE_STORE_PAGE_FAULT, U))
> +
> +#define HIDELEG_DEFAULT (MIP_VSSIP | MIP_VSTIP | MIP_VSEIP)
> +
> +static void vcpu_csr_init(struct vcpu *v)
> +{
> + register_t hstateen0;
> +
> + csr_write(CSR_HEDELEG, HEDELEG_DEFAULT);
> + v->arch.hedeleg = csr_read(CSR_HEDELEG);
> +
> + vcpu_guest_cpu_user_regs(v)->hstatus = HSTATUS_SPV | HSTATUS_SPVP;
> +
> + csr_write(CSR_HIDELEG, HIDELEG_DEFAULT);
> + v->arch.hideleg = csr_read(CSR_HIDELEG);
> +
To me, that feels odd to set machine CSR here. Especially if (I guess)
that we would update them anyway during context switches.
I think it would be better to have :
- vcpu_csr_init -> sets initial state CSR in vcpu structure, doesn't
touch machine CSR
- context switching logic -> loads vcpu-specific machine CSR from vcpu
structure
We would have to make a context switch to enter the vcpu for the first
time; but that's to be expected.
With my proposal, we would also want to move the vcpu_csr_init()
invocation to the place we initialize the vcpu_arch structure rather
than the first time we schedule inside that vcpu.
That would also allow to take account of per-domain configuration if we
need to (e.g feature flags).
> + /*
> + * VS should access only the time counter directly.
> + * Everything else should trap.
> + */
> + v->arch.hcounteren = HCOUNTEREN_TM;
> +
> + if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svpbmt) )
> + {
> + csr_write(CSR_HENVCFG, ENVCFG_PBMTE);
> + v->arch.henvcfg = csr_read(CSR_HENVCFG);
> + }
> +
> + if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_smstateen) )
> + {
> + if (riscv_isa_extension_available(NULL, RISCV_ISA_EXT_ssaia))
> + /*
> + * If the hypervisor extension is implemented, the same three
> + * bitsare defined also in hypervisor CSR hstateen0 but concern
> + * only the state potentially accessible to a virtual machine
> + * executing in privilege modes VS and VU:
> + * bit 60 CSRs siselect and sireg (really vsiselect and
> + * vsireg)
> + * bit 59 CSRs siph and sieh (RV32 only) and stopi (really
> + * vsiph, vsieh, and vstopi)
> + * bit 58 all state of IMSIC guest interrupt files,
> including
> + * CSR stopei (really vstopei)
> + * If one of these bits is zero in hstateen0, and the same bit is
> + * one in mstateen0, then an attempt to access the corresponding
> + * state from VS or VU-mode raises a virtual instruction
> exception.
> + */
> + hstateen0 = SMSTATEEN0_AIA | SMSTATEEN0_IMSIC |
> SMSTATEEN0_SVSLCT;
> +
> + /* Allow guest to access CSR_ENVCFG */
> + hstateen0 |= SMSTATEEN0_HSENVCFG;
> +
> + csr_write(CSR_HSTATEEN0, hstateen0);
> + v->arch.hstateen0 = csr_read(CSR_HSTATEEN0);
> + }
> +}
> +
> static void continue_new_vcpu(struct vcpu *prev)
> {
> BUG_ON("unimplemented\n");
> @@ -33,6 +104,8 @@ int arch_vcpu_create(struct vcpu *v)
> v->arch.xen_saved_context.sp = (register_t)v->arch.cpu_info;
> v->arch.xen_saved_context.ra = (register_t)continue_new_vcpu;
>
> + vcpu_csr_init(v);
> +
> /* Idle VCPUs don't need the rest of this setup */
> if ( is_idle_vcpu(v) )
> return rc;
> diff --git a/xen/arch/riscv/include/asm/cpufeature.h
> b/xen/arch/riscv/include/asm/cpufeature.h
> index b69616038888..ef02a3e26d2c 100644
> --- a/xen/arch/riscv/include/asm/cpufeature.h
> +++ b/xen/arch/riscv/include/asm/cpufeature.h
> @@ -36,6 +36,7 @@ enum riscv_isa_ext_id {
> RISCV_ISA_EXT_zbb,
> RISCV_ISA_EXT_zbs,
> RISCV_ISA_EXT_smaia,
> + RISCV_ISA_EXT_smstateen,
> RISCV_ISA_EXT_ssaia,
> RISCV_ISA_EXT_svade,
> RISCV_ISA_EXT_svpbmt,
> diff --git a/xen/arch/riscv/include/asm/current.h
> b/xen/arch/riscv/include/asm/current.h
> index 58c9f1506b7c..5fbee8182caa 100644
> --- a/xen/arch/riscv/include/asm/current.h
> +++ b/xen/arch/riscv/include/asm/current.h
> @@ -48,6 +48,8 @@ DECLARE_PER_CPU(struct vcpu *, curr_vcpu);
> #define get_cpu_current(cpu) per_cpu(curr_vcpu, cpu)
>
> #define guest_cpu_user_regs() ({ BUG_ON("unimplemented"); NULL; })
> +#define vcpu_guest_cpu_user_regs(vcpu) \
> + (&(vcpu)->arch.cpu_info->guest_cpu_user_regs)
> > #define switch_stack_and_jump(stack, fn) do { \
> asm volatile ( \
> diff --git a/xen/arch/riscv/include/asm/riscv_encoding.h
> b/xen/arch/riscv/include/asm/riscv_encoding.h
> index 1f7e612366f8..dd15731a86fa 100644
> --- a/xen/arch/riscv/include/asm/riscv_encoding.h
> +++ b/xen/arch/riscv/include/asm/riscv_encoding.h
> @@ -228,6 +228,8 @@
> #define ENVCFG_CBIE_INV _UL(0x3)
> #define ENVCFG_FIOM _UL(0x1)
>
> +#define HCOUNTEREN_TM BIT(1, U)
> +
> /* ===== User-level CSRs ===== */
>
> /* User Trap Setup (N-extension) */
--
Teddy Astie | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |