[PATCH v4 09/27] KVM: arm64: Access elements of vcpu_gp_regs individually
Steffen Eiden
seiden at linux.ibm.com
Fri Jul 10 04:56:37 PDT 2026
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 at 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
More information about the linux-arm-kernel
mailing list