Re: [PATCH v3 06/25] KVM: arm64: Save/restore POE registers

[Marc Zyngier <maz@kernel.org>]

On Wed, 29 Nov 2023 15:11:23 +0000,
Joey Gouly <joey.gouly@arm.com> wrote:
> 
> Hi Marc,
> 
> Thanks for taking a look.
> 
> On Mon, Nov 27, 2023 at 06:01:18PM +0000, Marc Zyngier wrote:
> > On Fri, 24 Nov 2023 16:34:51 +0000,
> > Joey Gouly <joey.gouly@arm.com> wrote:
> > > 
> > > Define the new system registers that POE introduces and context switch them.
> > 
> > I would really like to see a discussion on the respective lifetimes of
> > these two registers (see below).
> > 
> > >
> > > Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> > > Cc: Marc Zyngier <maz@kernel.org>
> > > Cc: Oliver Upton <oliver.upton@linux.dev>
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > Cc: Will Deacon <will@kernel.org>
> > > ---
> > >  arch/arm64/include/asm/kvm_arm.h           |  4 ++--
> > >  arch/arm64/include/asm/kvm_host.h          |  4 ++++
> > >  arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h | 10 ++++++++++
> > >  arch/arm64/kvm/sys_regs.c                  |  2 ++
> > >  4 files changed, 18 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> > > index b85f46a73e21..597470e0b87b 100644
> > > --- a/arch/arm64/include/asm/kvm_arm.h
> > > +++ b/arch/arm64/include/asm/kvm_arm.h
> > > @@ -346,14 +346,14 @@
> > >   */
> > >  #define __HFGRTR_EL2_RES0	(GENMASK(63, 56) | GENMASK(53, 51))
> > >  #define __HFGRTR_EL2_MASK	GENMASK(49, 0)
> > > -#define __HFGRTR_EL2_nMASK	(GENMASK(58, 57) | GENMASK(55, 54) | BIT(50))
> > > +#define __HFGRTR_EL2_nMASK	(GENMASK(60, 57) | GENMASK(55, 54) | BIT(50))
> > >  
> > >  #define __HFGWTR_EL2_RES0	(GENMASK(63, 56) | GENMASK(53, 51) |	\
> > >  				 BIT(46) | BIT(42) | BIT(40) | BIT(28) | \
> > >  				 GENMASK(26, 25) | BIT(21) | BIT(18) |	\
> > >  				 GENMASK(15, 14) | GENMASK(10, 9) | BIT(2))
> > >  #define __HFGWTR_EL2_MASK	GENMASK(49, 0)
> > > -#define __HFGWTR_EL2_nMASK	(GENMASK(58, 57) | GENMASK(55, 54) | BIT(50))
> > > +#define __HFGWTR_EL2_nMASK	(GENMASK(60, 57) | GENMASK(55, 54) | BIT(50))
> > >  
> > >  #define __HFGITR_EL2_RES0	GENMASK(63, 57)
> > >  #define __HFGITR_EL2_MASK	GENMASK(54, 0)
> > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > index 824f29f04916..fa9ebd8fce40 100644
> > > --- a/arch/arm64/include/asm/kvm_host.h
> > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > @@ -401,6 +401,10 @@ enum vcpu_sysreg {
> > >  	PIR_EL1,       /* Permission Indirection Register 1 (EL1) */
> > >  	PIRE0_EL1,     /*  Permission Indirection Register 0 (EL1) */
> > >  
> > > +	/* Permission Overlay Extension registers */
> > > +	POR_EL1,	/* Permission Overlay Register 1 (EL1) */
> > > +	POR_EL0,	/* Permission Overlay Register 0 (EL0) */
> > > +
> > >  	/* 32bit specific registers. */
> > >  	DACR32_EL2,	/* Domain Access Control Register */
> > >  	IFSR32_EL2,	/* Instruction Fault Status Register */
> > > diff --git a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> > > index bb6b571ec627..22f07ee43e7e 100644
> > > --- a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> > > +++ b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> > > @@ -19,6 +19,9 @@
> > >  static inline void __sysreg_save_common_state(struct kvm_cpu_context *ctxt)
> > >  {
> > >  	ctxt_sys_reg(ctxt, MDSCR_EL1)	= read_sysreg(mdscr_el1);
> > > +
> > > +	if (system_supports_poe())
> > > +		ctxt_sys_reg(ctxt, POR_EL0)	= read_sysreg_s(SYS_POR_EL0);
> > 
> > So this is saved as eagerly as it gets. Why? If it only affects EL0,
> > it can be saved/restored in a much lazier way.
> 
> Just to confirm I understand what you mean, the current code looks like:
> 
> 	vcpu_load()                // Lazy save
> 
> 	while (ret > 0)
> 		check_vcpu_requests()
> 		kvm_arm_vcpu_enter_exit()  // Eager save/restore
> 		ret = handle_exit()
> 
> 	vcpu_put()                // Lazy restore

Yes, with the additional caveat that VHE and nVHE/hVHE have different
views of what can be lazy or not.

>
> POR_EL0 does affect EL2, if it does some form of {get,put}_user.
> This happens in vgic_its_process_commands (as part of handle_exit), also the
> stolen time code (in check_vcpu_requests) and could possibly happen if perf
> tries to walk the user stack.
>
> So I think that it does need to happen eagerly, such that the host-userspace's
> POR_EL0 is used to access the VM's memory, not the guest-userspace's POR_EL0.

OK, I didn't quite see that initially, thanks for the explanation.
I find it rather ugly that userspace can directly affect these
functionalities, but hey, why not.

> Does that make sense? It will need a comment, I agree.

Yes, that'd be good indeed.

> 
> > 
> > >  }
> > >  
> > >  static inline void __sysreg_save_user_state(struct kvm_cpu_context *ctxt)
> > > @@ -59,6 +62,8 @@ static inline void __sysreg_save_el1_state(struct kvm_cpu_context *ctxt)
> > >  		ctxt_sys_reg(ctxt, PIR_EL1)	= read_sysreg_el1(SYS_PIR);
> > >  		ctxt_sys_reg(ctxt, PIRE0_EL1)	= read_sysreg_el1(SYS_PIRE0);
> > 
> > And the fact that you only touch PIRE0_EL1 here seems to be a good
> > indication that the above can be relaxed.
> 
> PIREO_EL1 is not directly accessible from EL0. I'll have a think
> about this a bit more, and if there is a potential similar issue
> here.

Ah, re-reading the spec, I see that there is a PIRE0_EL2 that is in
use for VHE, meaning that in that case restoring POR_EL0 is enough to
get the correct permissions. For nVHE/hVHE, this function is part of
the 'eager' lot, so it doesn't matter.

So this code is correct in the end, and all that's missing is some
comments and the NV stuff.

Thanks,

	M.

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