Re: [PATCH v5 2/8] KVM: arm64: Make vcpu_{read,write}_sys_reg available to HYP code

[Fuad Tabba]

On Tue, 14 Jul 2026 at 17:39, Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> On Tue, 14 Jul 2026 16:32:54 +0100,
> Fuad Tabba <fuad.tabba@xxxxxxxxx> wrote:
> >
> > On Tue, 14 Jul 2026 at 16:19, Marc Zyngier <maz@xxxxxxxxxx> wrote:
> > >
> > > On Tue, 14 Jul 2026 11:15:55 +0100,
> > > Fuad Tabba <fuad.tabba@xxxxxxxxx> wrote:
> > > >
> > > > The vcpu_{read,write}_sys_reg() accessors are only valid on a VHE host,
> > > > so helpers built on them such as kvm_vcpu_set_be()/kvm_vcpu_is_be()
> > > > cannot be shared with hyp code. exception.c already wraps them in local
> > > > helpers that pick the host- or hyp-side accessor via has_vhe().
> > > >
> > > > Rename the host-only implementations to __vcpu_{read,write}_sysreg_vhe()
> > >
> > > I'm a bit puzzled by this. There is nothing that makes these functions
> > > VHE-specific. Look at where they are called from: plenty of non-VHE
> > > uses. These helpers are the canonical accessors for any system
> > > register, and they don't cater for any particular mode.
> >
> > I might have misunderstood what Oliver asked me to do here [1]:
> > > Can you instead name the wrappers vcpu_{read,write}_sys_reg() and rename
> > > the current implementations, like __vcpu_{read,write}_sysreg_vhe()?
> >
> > Since they're only used and gated by has_vhe(), they're vhe-specific?
>
> But they factually aren't VHE specific. Look at sys_reg.c, for
> example. The whole point is that they abstract where the registers are
> located, irrespective of the KVM mode.
>
> The code in exception.c is actually a local specialisation of this
> code for the nVHE code not to explode. And the exact same effect could
> be achieved without repainting the whole thing and renaming *perfectly
> named* accessors! ;-)
>
> Something like below (compile-tested only).
>
>         M.

If it's fine by you and Oliver it works for me. I'll respin with that.

Cheers,
/fuad

>
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index 9d9e7674d45fe..5ea708ff29436 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -526,6 +526,14 @@ static inline unsigned long kvm_vcpu_get_mpidr_aff(struct kvm_vcpu *vcpu)
>         return __vcpu_sys_reg(vcpu, MPIDR_EL1) & MPIDR_HWID_BITMASK;
>  }
>
> +#if   defined (__KVM_NVHE_HYPERVISOR__)
> +#define vcpu_read_sys_reg(v, r)                __vcpu_sys_reg(v, r)
> +#define vcpu_write_sys_reg(v, x, r)    __vcpu_assign_sys_reg(v, x, r)
> +#elif defined (__KVM_VHE_HYPERVISOR__)
> +#define vcpu_read_sys_reg(v, r)                vcpu_read_sys_reg(v, r)
> +#define vcpu_write_sys_reg(v, x, r)    vcpu_write_sys_reg(v, x, r)
> +#endif
> +
>  static inline void kvm_vcpu_set_be(struct kvm_vcpu *vcpu)
>  {
>         if (vcpu_mode_is_32bit(vcpu)) {
> diff --git a/arch/arm64/kvm/hyp/exception.c b/arch/arm64/kvm/hyp/exception.c
> index bef40ddb16dbc..754e2dc1df54a 100644
> --- a/arch/arm64/kvm/hyp/exception.c
> +++ b/arch/arm64/kvm/hyp/exception.c
> @@ -20,22 +20,6 @@
>  #error Hypervisor code only!
>  #endif
>
> -static inline u64 __vcpu_read_sys_reg(const struct kvm_vcpu *vcpu, int reg)
> -{
> -       if (has_vhe())
> -               return vcpu_read_sys_reg(vcpu, reg);
> -
> -       return __vcpu_sys_reg(vcpu, reg);
> -}
> -
> -static inline void __vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, int reg)
> -{
> -       if (has_vhe())
> -               vcpu_write_sys_reg(vcpu, val, reg);
> -       else
> -               __vcpu_assign_sys_reg(vcpu, reg, val);
> -}
> -
>  static void __vcpu_write_spsr(struct kvm_vcpu *vcpu, unsigned long target_mode,
>                               u64 val)
>  {
> @@ -101,14 +85,14 @@ static void enter_exception64(struct kvm_vcpu *vcpu, unsigned long target_mode,
>
>         switch (target_mode) {
>         case PSR_MODE_EL1h:
> -               vbar = __vcpu_read_sys_reg(vcpu, VBAR_EL1);
> -               sctlr = __vcpu_read_sys_reg(vcpu, SCTLR_EL1);
> -               __vcpu_write_sys_reg(vcpu, *vcpu_pc(vcpu), ELR_EL1);
> +               vbar = vcpu_read_sys_reg(vcpu, VBAR_EL1);
> +               sctlr = vcpu_read_sys_reg(vcpu, SCTLR_EL1);
> +               vcpu_write_sys_reg(vcpu, *vcpu_pc(vcpu), ELR_EL1);
>                 break;
>         case PSR_MODE_EL2h:
> -               vbar = __vcpu_read_sys_reg(vcpu, VBAR_EL2);
> -               sctlr = __vcpu_read_sys_reg(vcpu, SCTLR_EL2);
> -               __vcpu_write_sys_reg(vcpu, *vcpu_pc(vcpu), ELR_EL2);
> +               vbar = vcpu_read_sys_reg(vcpu, VBAR_EL2);
> +               sctlr = vcpu_read_sys_reg(vcpu, SCTLR_EL2);
> +               vcpu_write_sys_reg(vcpu, *vcpu_pc(vcpu), ELR_EL2);
>                 break;
>         default:
>                 /* Don't do that */
> @@ -185,7 +169,7 @@ static void enter_exception64(struct kvm_vcpu *vcpu, unsigned long target_mode,
>   */
>  static unsigned long get_except32_cpsr(struct kvm_vcpu *vcpu, u32 mode)
>  {
> -       u32 sctlr = __vcpu_read_sys_reg(vcpu, SCTLR_EL1);
> +       u32 sctlr = vcpu_read_sys_reg(vcpu, SCTLR_EL1);
>         unsigned long old, new;
>
>         old = *vcpu_cpsr(vcpu);
> @@ -281,7 +265,7 @@ static void enter_exception32(struct kvm_vcpu *vcpu, u32 mode, u32 vect_offset)
>  {
>         unsigned long spsr = *vcpu_cpsr(vcpu);
>         bool is_thumb = (spsr & PSR_AA32_T_BIT);
> -       u32 sctlr = __vcpu_read_sys_reg(vcpu, SCTLR_EL1);
> +       u32 sctlr = vcpu_read_sys_reg(vcpu, SCTLR_EL1);
>         u32 return_address;
>
>         *vcpu_cpsr(vcpu) = get_except32_cpsr(vcpu, mode);
> @@ -305,7 +289,7 @@ static void enter_exception32(struct kvm_vcpu *vcpu, u32 mode, u32 vect_offset)
>         if (sctlr & (1 << 13))
>                 vect_offset += 0xffff0000;
>         else /* always have security exceptions */
> -               vect_offset += __vcpu_read_sys_reg(vcpu, VBAR_EL1);
> +               vect_offset += vcpu_read_sys_reg(vcpu, VBAR_EL1);
>
>         *vcpu_pc(vcpu) = vect_offset;
>  }
>
> --
> Without deviation from the norm, progress is not possible.