Re: [PATCH 14/27] KVM: arm64: Restructure FGT register switching

[Marc Zyngier <maz@kernel.org>]

On Tue, 25 Jul 2023 17:39:52 +0100,
Eric Auger <eric.auger@redhat.com> wrote:
> 
> Hi Marc,
> 
> On 7/12/23 16:57, Marc Zyngier wrote:
> > As we're about to majorly extend the handling of FGT registers,
> > restructure the code to actually save/restore the registers
> > as required. This is made easy thanks to the previous addition
> > of the EL2 registers, allowing us to use the host context for
> > this purpose.
> >
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/include/asm/kvm_arm.h        | 21 ++++++++++
> >  arch/arm64/kvm/hyp/include/hyp/switch.h | 55 +++++++++++++------------
> >  2 files changed, 49 insertions(+), 27 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> > index 028049b147df..85908aa18908 100644
> > --- a/arch/arm64/include/asm/kvm_arm.h
> > +++ b/arch/arm64/include/asm/kvm_arm.h
> > @@ -333,6 +333,27 @@
> >  				 BIT(18) |		\
> >  				 GENMASK(16, 15))
> >  
> > +/*
> > + * FGT register definitions
> > + *
> > + * RES0 and polarity masks as of DDI0487J.a, to be updated as needed.
> > + * We're not using the generated masks as they are usually ahead of
> > + * the published ARM ARM, which we use as a reference.
> > + *
> > + * Once we get to a point where the two describe the same thing, we'll
> > + * merge the definitions. One day.
> > + */
> > +#define __HFGRTR_EL2_RES0	(GENMASK(63, 56) | GENMASK(53, 51))
> > +#define __HFGRTR_EL2_MASK	GENMASK(49, 0)
> > +#define __HFGRTR_EL2_nMASK	(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(55, 54) | BIT(50))
> > +
> >  /* Hyp Prefetch Fault Address Register (HPFAR/HDFAR) */
> >  #define HPFAR_MASK	(~UL(0xf))
> >  /*
> > diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
> > index 4bddb8541bec..9781e79a5127 100644
> > --- a/arch/arm64/kvm/hyp/include/hyp/switch.h
> > +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
> > @@ -70,20 +70,19 @@ static inline void __activate_traps_fpsimd32(struct kvm_vcpu *vcpu)
> >  	}
> >  }
> >  
> > -static inline bool __hfgxtr_traps_required(void)
> > -{
> > -	if (cpus_have_final_cap(ARM64_SME))
> > -		return true;
> > -
> > -	if (cpus_have_final_cap(ARM64_WORKAROUND_AMPERE_AC03_CPU_38))
> > -		return true;
> >  
> > -	return false;
> > -}
> >  
> > -static inline void __activate_traps_hfgxtr(void)
> > +static inline void __activate_traps_hfgxtr(struct kvm_vcpu *vcpu)
> >  {
> > +	struct kvm_cpu_context *hctxt = &this_cpu_ptr(&kvm_host_data)->host_ctxt;
> >  	u64 r_clr = 0, w_clr = 0, r_set = 0, w_set = 0, tmp;
> > +	u64 r_val, w_val;
> > +
> > +	if (!cpus_have_final_cap(ARM64_HAS_FGT))
> > +		return;
> > +
> > +	ctxt_sys_reg(hctxt, HFGRTR_EL2) = read_sysreg_s(SYS_HFGRTR_EL2);
> > +	ctxt_sys_reg(hctxt, HFGWTR_EL2) = read_sysreg_s(SYS_HFGWTR_EL2);
> >  
> >  	if (cpus_have_final_cap(ARM64_SME)) {
> >  		tmp = HFGxTR_EL2_nSMPRI_EL1_MASK | HFGxTR_EL2_nTPIDR2_EL0_MASK;
> > @@ -98,26 +97,30 @@ static inline void __activate_traps_hfgxtr(void)
> >  	if (cpus_have_final_cap(ARM64_WORKAROUND_AMPERE_AC03_CPU_38))
> >  		w_set |= HFGxTR_EL2_TCR_EL1_MASK;
> >  
> > -	sysreg_clear_set_s(SYS_HFGRTR_EL2, r_clr, r_set);
> > -	sysreg_clear_set_s(SYS_HFGWTR_EL2, w_clr, w_set);
> > +
> > +	r_val = __HFGRTR_EL2_nMASK & ~HFGxTR_EL2_nACCDATA_EL1;
> I don't get why you do
> 
> & ~HFGxTR_EL2_nACCDATA_EL1 as this latter also has a negative polarity. 
> 
> Please could you explain what is special with this bit/add a comment?

Nothing is really special with this bit.

But it is currently always cleared (we blindly write a big fat zero),
and I wanted to explicitly show all the instructions for which we
enable trapping for (ACCDATA_EL1 being the only one that is currently
documented in the ARM ARM, although there are more already).

So the construct I came up with is the above, initialising the
register value with the nMASK bits (i.e. not trapping the
corresponding instructions), and then clearing the bit for the stuff
we want to trap. Maybe adding something like:

/* Default to no trapping anything but ACCDATA_EL1 */

would help?

Thanks,

	M.

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