Re: [PATCH v4 09/27] KVM: arm64: Access elements of vcpu_gp_regs individually

[Marc Zyngier <maz@kernel.org>]

On Mon, 06 Jul 2026 09:52:09 +0100,
Steffen Eiden <seiden@linux.ibm.com> wrote:
> 
> While for arm64 the members of vcpu_gp_regs are allocated continuous
> this is not necessarily true for other architectures implementing ARM.
> 
> Let vcpu_gp_regs() no longer return the address of the user_pt_regs in
> the vcpu context but the address of the gp-register array field in the
> user_pt_reg struct.

That's an interesting change of semantics, because this excludes PC
from the GPRs. This is valid on AArch64, but wrong for AArch32 (PC
really is R15, and is just another GPR).

It isn't a huge deal, and nothing breaks, but that's something that
you may want to capture.

>
> Share the gp register functions with s390.
> 
> No functional change.
> 
> Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> Signed-off-by: Steffen Eiden <seiden@linux.ibm.com>
> ---
>  arch/arm64/include/asm/kvm_emulate.h       | 13 +++++++++----
>  arch/arm64/include/asm/kvm_host.h          |  8 +++++++-
>  arch/arm64/kvm/guest.c                     | 19 +++++++++++--------
>  arch/arm64/kvm/hyp/exception.c             |  7 +++++--
>  arch/arm64/kvm/hyp/include/hyp/adjust_pc.h |  4 ++--
>  arch/arm64/kvm/hyp/include/hyp/switch.h    |  6 +++---
>  arch/arm64/kvm/reset.c                     |  6 ++++--
>  7 files changed, 41 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index a1c92d2436ae..15d6d6a08d37 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -140,12 +140,17 @@ static inline void vcpu_set_vsesr(struct kvm_vcpu *vcpu, u64 vsesr)
>  
>  static __always_inline unsigned long *vcpu_pc(const struct kvm_vcpu *vcpu)
>  {
> -	return (unsigned long *)&vcpu_gp_regs(vcpu)->pc;
> +	return (unsigned long *)&vcpu->arch.ctxt.regs.pc;
>  }
>  
>  static __always_inline unsigned long *vcpu_cpsr(const struct kvm_vcpu *vcpu)
>  {
> -	return (unsigned long *)&vcpu_gp_regs(vcpu)->pstate;
> +	return (unsigned long *)&vcpu->arch.ctxt.regs.pstate;
> +}
> +
> +static __always_inline unsigned long *vcpu_sp_el0(const struct kvm_vcpu *vcpu)
> +{
> +	return (unsigned long *)&vcpu->arch.ctxt.regs.sp;
>  }
>  
>  static __always_inline bool vcpu_mode_is_32bit(const struct kvm_vcpu *vcpu)
> @@ -175,14 +180,14 @@ static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu)
>  static __always_inline unsigned long vcpu_get_reg(const struct kvm_vcpu *vcpu,
>  					 u8 reg_num)
>  {
> -	return (reg_num == 31) ? 0 : vcpu_gp_regs(vcpu)->regs[reg_num];
> +	return (reg_num == 31) ? 0 : vcpu_gp_regs(vcpu)[reg_num];
>  }
>  
>  static __always_inline void vcpu_set_reg(struct kvm_vcpu *vcpu, u8 reg_num,
>  				unsigned long val)
>  {
>  	if (reg_num != 31)
> -		vcpu_gp_regs(vcpu)->regs[reg_num] = val;
> +		vcpu_gp_regs(vcpu)[reg_num] = val;
>  }
>  
>  #endif /* ARM64_S390_COMMON */
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index ae9f76378218..2fce38fd9152 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -1170,7 +1170,7 @@ struct kvm_vcpu_arch {
>  #define vcpu_clear_on_unsupported_cpu(vcpu)				\
>  	vcpu_clear_flag(vcpu, ON_UNSUPPORTED_CPU)
>  
> -#define vcpu_gp_regs(v)		(&(v)->arch.ctxt.regs)
> +#define vcpu_gp_regs(v)		((v)->arch.ctxt.regs.regs)
>
>  /*
>   * Only use __vcpu_sys_reg/ctxt_sys_reg if you know you want the
> @@ -1201,6 +1201,12 @@ static inline u64 *___ctxt_sys_reg(const struct kvm_cpu_context *ctxt, int r)
>  
>  #define ctxt_sys_reg(c,r)	(*__ctxt_sys_reg(c,r))
>  
> +#define kvm_vcpu_get_sp_el1(__vcpu) (__ctxt_sys_reg(&(__vcpu)->arch.ctxt, SP_EL1))
> +#define kvm_vcpu_get_vreg(__vcpu, _n) (&(__vcpu)->arch.ctxt.fp_regs.vregs[_n])
> +#define kvm_vcpu_get_vregs(__vcpu) (&(__vcpu)->arch.ctxt.fp_regs.vregs)
> +#define kvm_vcpu_get_fpsr(__vcpu) (&(__vcpu)->arch.ctxt.fp_regs.fpsr)
> +#define kvm_vcpu_get_fpcr(__vcpu) (&(__vcpu)->arch.ctxt.fp_regs.fpcr)
> +
>  u64 kvm_vcpu_apply_reg_masks(const struct kvm_vcpu *, enum vcpu_sysreg, u64);
>  
>  #define __vcpu_assign_sys_reg(v, r, val)				\
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index 5a202cfd27bc..5e1e1faa98c2 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -62,6 +62,7 @@ const struct kvm_stats_header kvm_vcpu_stats_header = {
>  		       sizeof(kvm_vcpu_stats_desc),
>  };
>  
> +#ifdef ARM64_S390_COMMON

I really think this patch (and a few others) needs splitting. What I'd
like to see is a prefix to this series adding the required arm64
rework, and only in a subsequent patch add the "make this shared"
attributes.

Also, quite a lot of this patch is about using the existing accessors
instead of an open-coded version. These changes should be standalone.

>  static bool core_reg_offset_is_vreg(u64 off)
>  {
>  	return off >= KVM_REG_ARM_CORE_REG(fp_regs.vregs) &&
> @@ -134,19 +135,19 @@ static void *core_reg_addr(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  	     KVM_REG_ARM_CORE_REG(regs.regs[30]):
>  		off -= KVM_REG_ARM_CORE_REG(regs.regs[0]);
>  		off /= 2;
> -		return &vcpu->arch.ctxt.regs.regs[off];
> +		return &vcpu_gp_regs(vcpu)[off];
>  
>  	case KVM_REG_ARM_CORE_REG(regs.sp):
> -		return &vcpu->arch.ctxt.regs.sp;
> +		return vcpu_sp_el0(vcpu);
>  
>  	case KVM_REG_ARM_CORE_REG(regs.pc):
> -		return &vcpu->arch.ctxt.regs.pc;
> +		return vcpu_pc(vcpu);
>  
>  	case KVM_REG_ARM_CORE_REG(regs.pstate):
> -		return &vcpu->arch.ctxt.regs.pstate;
> +		return vcpu_cpsr(vcpu);
>  
>  	case KVM_REG_ARM_CORE_REG(sp_el1):
> -		return __ctxt_sys_reg(&vcpu->arch.ctxt, SP_EL1);
> +		return  kvm_vcpu_get_sp_el1(vcpu);
>  
>  	case KVM_REG_ARM_CORE_REG(elr_el1):
>  		return __ctxt_sys_reg(&vcpu->arch.ctxt, ELR_EL1);
> @@ -170,13 +171,13 @@ static void *core_reg_addr(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  	     KVM_REG_ARM_CORE_REG(fp_regs.vregs[31]):
>  		off -= KVM_REG_ARM_CORE_REG(fp_regs.vregs[0]);
>  		off /= 4;
> -		return &vcpu->arch.ctxt.fp_regs.vregs[off];
> +		return kvm_vcpu_get_vreg(vcpu, off);
>  
>  	case KVM_REG_ARM_CORE_REG(fp_regs.fpsr):
> -		return &vcpu->arch.ctxt.fp_regs.fpsr;
> +		return  kvm_vcpu_get_fpsr(vcpu);
>  
>  	case KVM_REG_ARM_CORE_REG(fp_regs.fpcr):
> -		return &vcpu->arch.ctxt.fp_regs.fpcr;
> +		return  kvm_vcpu_get_fpcr(vcpu);

Odd additional spaces (3 instances).

>  
>  	default:
>  		return NULL;
> @@ -306,6 +307,8 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  	return err;
>  }
>  
> +#endif /* ARM64_S390_COMMON */
> +
>  #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64)
>  #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64)
>  #define vq_present(vqs, vq) (!!((vqs)[vq_word(vq)] & vq_mask(vq)))
> diff --git a/arch/arm64/kvm/hyp/exception.c b/arch/arm64/kvm/hyp/exception.c
> index bef40ddb16db..82611442a2d1 100644
> --- a/arch/arm64/kvm/hyp/exception.c
> +++ b/arch/arm64/kvm/hyp/exception.c
> @@ -277,6 +277,9 @@ static const u8 return_offsets[8][2] = {
>  	[7] = { 4, 4 },		/* FIQ, unused */
>  };
>  
> +#define OFFSETOF_PT_REG(__r) offsetof(struct user_pt_regs, __r)
> +#define COMPAT_IDX(__c) ((OFFSETOF_PT_REG(__c) - OFFSETOF_PT_REG(regs[0])) / sizeof(u64))
> +

Oh $gawd, this is... awful.

>  static void enter_exception32(struct kvm_vcpu *vcpu, u32 mode, u32 vect_offset)
>  {
>  	unsigned long spsr = *vcpu_cpsr(vcpu);
> @@ -292,12 +295,12 @@ static void enter_exception32(struct kvm_vcpu *vcpu, u32 mode, u32 vect_offset)
>  	switch(mode) {
>  	case PSR_AA32_MODE_ABT:
>  		__vcpu_write_spsr_abt(vcpu, host_spsr_to_spsr32(spsr));
> -		vcpu_gp_regs(vcpu)->compat_lr_abt = return_address;
> +		vcpu_gp_regs(vcpu)[COMPAT_IDX(compat_lr_abt)] = return_address;
>  		break;

Stupid idea: why don't you simply have new #defines that make the
register number standalone, and make ptrace.h use that? Something line
this (which can obviously be extended to all the compat registers):

diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index 39582511ad72f..2d3d324d2598e 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -104,6 +104,10 @@
 #define COMPAT_USER_SZ	296
 
 /* Architecturally defined mapping between AArch32 and AArch64 registers */
+enum aarch32_reg_mapping {
+	__compat_lr_und = 22,
+};
+
 #define compat_usr(x)	regs[(x)]
 #define compat_fp	regs[11]
 #define compat_sp	regs[13]
@@ -115,7 +119,7 @@
 #define compat_sp_svc	regs[19]
 #define compat_lr_abt	regs[20]
 #define compat_sp_abt	regs[21]
-#define compat_lr_und	regs[22]
+#define compat_lr_und	regs[__compat_lr_und]
 #define compat_sp_und	regs[23]
 #define compat_r8_fiq	regs[24]
 #define compat_r9_fiq	regs[25]

and then your #defines from hell can go?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.