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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxx> 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@xxxxxxx>
> > Cc: Marc Zyngier <maz@xxxxxxxxxx>
> > Cc: Oliver Upton <oliver.upton@xxxxxxxxx>
> > Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> > Cc: Will Deacon <will@xxxxxxxxxx>
> > ---
> >  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

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.

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

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

> 
> >  	}
> > +	if (system_supports_poe())
> 
> nit: missing new line before the if().
> 
> > +		ctxt_sys_reg(ctxt, POR_EL1)	= read_sysreg_el1(SYS_POR);
> >  	ctxt_sys_reg(ctxt, PAR_EL1)	= read_sysreg_par();
> >  	ctxt_sys_reg(ctxt, TPIDR_EL1)	= read_sysreg(tpidr_el1);
> >  
> > @@ -89,6 +94,9 @@ static inline void __sysreg_save_el2_return_state(struct kvm_cpu_context *ctxt)
> >  static inline void __sysreg_restore_common_state(struct kvm_cpu_context *ctxt)
> >  {
> >  	write_sysreg(ctxt_sys_reg(ctxt, MDSCR_EL1),  mdscr_el1);
> > +
> > +	if (system_supports_poe())
> > +		write_sysreg_s(ctxt_sys_reg(ctxt, POR_EL0),	SYS_POR_EL0);
> 
> Same thing here about the eager restore.
> 
> >  }
> >  
> >  static inline void __sysreg_restore_user_state(struct kvm_cpu_context *ctxt)
> > @@ -135,6 +143,8 @@ static inline void __sysreg_restore_el1_state(struct kvm_cpu_context *ctxt)
> >  		write_sysreg_el1(ctxt_sys_reg(ctxt, PIR_EL1),	SYS_PIR);
> >  		write_sysreg_el1(ctxt_sys_reg(ctxt, PIRE0_EL1),	SYS_PIRE0);
> >  	}
> > +	if (system_supports_poe())
> 
> new line.
> 
> > +		write_sysreg_el1(ctxt_sys_reg(ctxt, POR_EL1),	SYS_POR);
> >  	write_sysreg(ctxt_sys_reg(ctxt, PAR_EL1),	par_el1);
> >  	write_sysreg(ctxt_sys_reg(ctxt, TPIDR_EL1),	tpidr_el1);
> >  
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 4735e1b37fb3..a54e5eadbf29 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -2269,6 +2269,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> >  	{ SYS_DESC(SYS_MAIR_EL1), access_vm_reg, reset_unknown, MAIR_EL1 },
> >  	{ SYS_DESC(SYS_PIRE0_EL1), NULL, reset_unknown, PIRE0_EL1 },
> >  	{ SYS_DESC(SYS_PIR_EL1), NULL, reset_unknown, PIR_EL1 },
> > +	{ SYS_DESC(SYS_POR_EL1), NULL, reset_unknown, POR_EL1 },
> >  	{ SYS_DESC(SYS_AMAIR_EL1), access_vm_reg, reset_amair_el1, AMAIR_EL1 },
> >  
> >  	{ SYS_DESC(SYS_LORSA_EL1), trap_loregion },
> > @@ -2352,6 +2353,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> >  	  .access = access_pmovs, .reg = PMOVSSET_EL0,
> >  	  .get_user = get_pmreg, .set_user = set_pmreg },
> >  
> > +	{ SYS_DESC(SYS_POR_EL0), NULL, reset_unknown, POR_EL0 },
> >  	{ SYS_DESC(SYS_TPIDR_EL0), NULL, reset_unknown, TPIDR_EL0 },
> >  	{ SYS_DESC(SYS_TPIDRRO_EL0), NULL, reset_unknown, TPIDRRO_EL0 },
> >  	{ SYS_DESC(SYS_TPIDR2_EL0), undef_access },
> 
> Another thing that is missing is the trap routing for NV in
> emulated-nested.c. Please fill in the various tables there.
> 
> Thanks,
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.


Thanks,
Joey




[Index of Archives]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux