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

[Steffen Eiden <seiden@linux.ibm.com>]

On Mon, Jul 06, 2026 at 06:44:06PM +0100, Marc Zyngier wrote:
> 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).
>

I never looked into AArch32 thanks for pointing this out.

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

Shall I add a comment into the commit message that this splits pc from
the gprs for arm32?



...

> >  
> > +#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.

Ok, makes sense. I'll do the refactroings first and then add these
markers.

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

Ok I'llsplit this up:

1. vcpu_gp_regs change
2. accessor use

> 
> >  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).

thanks.

> 
> >  
> >  	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?

Great suggestion I do not like this macro hell myself. I'll implement it
that way. Should I convert all compat_* regs to this concept or just the
ones I need?

	Steffen