Re: [PATCH v2 02/28] arm64: KVM: Hide unsupported AArch64 CPU features from guests

[Dave Martin <Dave.Martin@arm.com>]

On Wed, Sep 13, 2017 at 03:37:42PM +0100, Alex Bennée wrote:
> 
> Dave Martin <Dave.Martin@arm.com> writes:

[...]

> > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> > index 945e79c..35a90b8 100644
> > --- a/arch/arm64/kvm/hyp/switch.c
> > +++ b/arch/arm64/kvm/hyp/switch.c
> > @@ -81,11 +81,17 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu)
> >  	 * it will cause an exception.
> >  	 */
> >  	val = vcpu->arch.hcr_el2;
> > +
> >  	if (!(val & HCR_RW) && system_supports_fpsimd()) {
> >  		write_sysreg(1 << 30, fpexc32_el2);
> >  		isb();
> >  	}
> > +
> > +	if (val & HCR_RW) /* for AArch64 only: */
> > +		val |= HCR_TID3; /* TID3: trap feature register accesses */
> > +
> 
> I wondered as this is the hyp switch can we make use of testing val &
> HCR_RW for both this and above. But it seems minimal in the generated
> code so probably not.

I figured that the code was cleaner ths way, since they're independent
bits of code that both happen to be applicable only to AArch64 guests.

[...]

> > +
> > +	/*
> > +	 * ID regs: all ID_SANITISED() entries here must have corresponding
> > +	 * entries in arm64_ftr_regs[].
> > +	 */
> 
> arm64_ftr_regs isn't updated in this commit. Does this break bisection?

This commit only adds ID_SANITISED() entries for regs that are already
present in arm64_ftr_regs[].  (If you spot any that are missing, give me
a shout...)

SVE only adds one new ID register, ID_AA64ZFR0_EL1 -- but SVE defines no
fields in there yet, so I just leave it ID_UNALLOCATED() which will
cause it to read as zero for the guest.

> > +
> > +	/* AArch64 mappings of the AArch32 ID registers */
> > +	/* CRm=1 */
> > +	ID_SANITISED(ID_PFR0_EL1),
> > +	ID_SANITISED(ID_PFR1_EL1),

[...]

> > +	/* CRm=7 */
> > +	ID_SANITISED(ID_AA64MMFR0_EL1),
> > +	ID_SANITISED(ID_AA64MMFR1_EL1),
> > +	ID_SANITISED(ID_AA64MMFR2_EL1),
> > +	ID_UNALLOCATED(7,3),
> > +	ID_UNALLOCATED(7,4),
> > +	ID_UNALLOCATED(7,5),
> > +	ID_UNALLOCATED(7,6),
> > +	ID_UNALLOCATED(7,7),
> > +
> 
> I think it might be worthwhile adding a test to kvm-unit-tests to walk
> all the ID registers to check this.

Sounds sensible, I'll take a look at that.

[...]

Cheers
---Dave