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

[Marc Zyngier]

On Fri, 10 Jul 2026 12:56:37 +0100,
Steffen Eiden <seiden@xxxxxxxxxxxxx> wrote:
> 
> 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@xxxxxxxxxxxxx> 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?

Yeah, I really want this stuff to be correctly documented, just in
case we break something and have to go back in time to find out why.
That's overall something that could be improved in this series.

[...]

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

All of them, please., I value consistency. As you are doing this,
consider adding a reference to I_PYKVS, which describes the mapping in
the ARM ARM.

Thanks,

	M.

-- 
Jazz isn't dead. It just smells funny.