Re: [PATCH v8 03/28] arm64: mte: CPU feature detection and initial sysreg configuration

[Marc Zyngier <maz@kernel.org>]

On 2020-08-26 18:08, Catalin Marinas wrote:
> On Tue, Aug 25, 2020 at 02:53:47PM +0100, Marc Zyngier wrote:
>> On 2020-08-25 11:54, Catalin Marinas wrote:
>> > On Tue, Aug 25, 2020 at 09:53:16AM +0100, Marc Zyngier wrote:
>> > > On 2020-08-24 19:27, Catalin Marinas wrote:
>> > > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> > > > index 077293b5115f..59b91f58efec 100644
>> > > > --- a/arch/arm64/kvm/sys_regs.c
>> > > > +++ b/arch/arm64/kvm/sys_regs.c
>> > > > @@ -1131,6 +1131,8 @@ static u64 read_id_reg(const struct kvm_vcpu
>> > > > *vcpu,
>> > > >  		if (!vcpu_has_sve(vcpu))
>> > > >  			val &= ~(0xfUL << ID_AA64PFR0_SVE_SHIFT);
>> > > >  		val &= ~(0xfUL << ID_AA64PFR0_AMU_SHIFT);
>> > > > +	} else if (id == SYS_ID_AA64PFR1_EL1) {
>> > > > +		val &= ~(0xfUL << ID_AA64PFR1_MTE_SHIFT);
>> > >
>> > > Hiding the capability is fine, but where is the handling of trapping
>> > > instructions done? They should result in an UNDEF being injected.
>> >
>> > They are a few new MTE-specific MSR/MRS which are trapped at EL2 but
>> > since KVM doesn't understand them yet, shouldn't it already inject
>> > undef back at EL1? That would be safer regardless of MTE support.
>> 
>> An UNDEF will be injected, but not without spitting a nastygram in
>> the kernel log (look at emulate_sys_reg()).
>> 
>> The best course of action is to have an entry in the sysreg table
>> that would explicitly do the handling.
> 
> Something like below. I'll put them in a separate patch, to be reverted
> when we get proper MTE support in KVM.
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 59b91f58efec..c7d5d1bae044 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1384,6 +1384,13 @@ static bool access_ccsidr(struct kvm_vcpu
> *vcpu, struct sys_reg_params *p,
>  	return true;
>  }
> 
> +static bool access_mte_regs(struct kvm_vcpu *vcpu, struct 
> sys_reg_params *p,
> +			    const struct sys_reg_desc *r)
> +{
> +	kvm_inject_undefined(vcpu);
> +	return false;
> +}
> +
>  /* sys_reg_desc initialiser for known cpufeature ID registers */
>  #define ID_SANITISED(name) {			\
>  	SYS_DESC(SYS_##name),			\
> @@ -1549,6 +1556,10 @@ static const struct sys_reg_desc sys_reg_descs[] 
> = {
>  	{ SYS_DESC(SYS_SCTLR_EL1), access_vm_reg, reset_val, SCTLR_EL1, 
> 0x00C50078 },
>  	{ SYS_DESC(SYS_ACTLR_EL1), access_actlr, reset_actlr, ACTLR_EL1 },
>  	{ SYS_DESC(SYS_CPACR_EL1), NULL, reset_val, CPACR_EL1, 0 },
> +
> +	{ SYS_DESC(SYS_RGSR_EL1), access_mte_regs },
> +	{ SYS_DESC(SYS_GCR_EL1), access_mte_regs },
> +
>  	{ SYS_DESC(SYS_ZCR_EL1), NULL, reset_val, ZCR_EL1, 0, .visibility =
> sve_visibility },
>  	{ SYS_DESC(SYS_TTBR0_EL1), access_vm_reg, reset_unknown, TTBR0_EL1 },
>  	{ SYS_DESC(SYS_TTBR1_EL1), access_vm_reg, reset_unknown, TTBR1_EL1 },
> @@ -1573,6 +1584,9 @@ static const struct sys_reg_desc sys_reg_descs[] 
> = {
>  	{ SYS_DESC(SYS_ERXMISC0_EL1), trap_raz_wi },
>  	{ SYS_DESC(SYS_ERXMISC1_EL1), trap_raz_wi },
> 
> +	{ SYS_DESC(SYS_TFSR_EL1), access_mte_regs },
> +	{ SYS_DESC(SYS_TFSRE0_EL1), access_mte_regs },
> +
>  	{ SYS_DESC(SYS_FAR_EL1), access_vm_reg, reset_unknown, FAR_EL1 },
>  	{ SYS_DESC(SYS_PAR_EL1), NULL, reset_unknown, PAR_EL1 },

Yup, looks good.

> (still testing, it takes ages to boot a VM inside inside FVP)

You aren't allowed to moan about it until you have tried that with NV! 
;-)

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...