Re: [PATCH v8 27/28] arm64: mte: Kconfig entry

Re: [PATCH v8 27/28] arm64: mte: Kconfig entry

[Randy Dunlap]

Hi,

On 8/24/20 11:27 AM, Catalin Marinas wrote:
> From: Vincenzo Frascino <vincenzo.frascino@arm.com>
> 
> Add Memory Tagging Extension support to the arm64 kbuild.
> 
> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> Co-developed-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> ---
> 
> Notes:
>     v7:
>     - Binutils gained initial support for MTE in 2.32.0. However, a late
>       architecture addition (LDGM/STGM) is only supported in the newer
>       2.32.x and 2.33 versions. Change the AS_HAS_MTE option to also check
>       for stgm in addition to .arch armv8.5-a+memtag.
>     
>  arch/arm64/Kconfig | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 6d232837cbee..10cf81d70657 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1664,6 +1664,37 @@ config ARCH_RANDOM
>  	  provides a high bandwidth, cryptographically secure
>  	  hardware random number generator.
>  
> +config ARM64_AS_HAS_MTE
> +	# Binutils gained initial support for MTE in 2.32.0. However, a
> +	# late architecture addition (LDGM/STGM) is only supported in
> +	# the newer 2.32.x and 2.33 versions.
> +	def_bool $(as-instr,.arch armv8.5-a+memtag\nstgm xzr$(comma)[x0])

Would you mind translating that for me?
Yes, I read the v7 Notes, but that only helped a little bit.


> +
> +config ARM64_MTE
> +	bool "Memory Tagging Extension support"
> +	default y
> +	depends on ARM64_AS_HAS_MTE && ARM64_TAGGED_ADDR_ABI
> +	select ARCH_USES_HIGH_VMA_FLAGS
> +	help
> +	  Memory Tagging (part of the ARMv8.5 Extensions) provides
> +	  architectural support for run-time, always-on detection of

	                            runtime,
as is used below.

> +	  various classes of memory error to aid with software debugging
> +	  to eliminate vulnerabilities arising from memory-unsafe
> +	  languages.
> +
> +	  This option enables the support for the Memory Tagging
> +	  Extension at EL0 (i.e. for userspace).
> +
> +	  Selecting this option allows the feature to be detected at
> +	  runtime. Any secondary CPU not implementing this feature will
> +	  not be allowed a late bring-up.
> +
> +	  Userspace binaries that want to use this feature must
> +	  explicitly opt in. The mechanism for the userspace is
> +	  described in:
> +
> +	  Documentation/arm64/memory-tagging-extension.rst.
> +
>  endmenu
>  
>  config ARM64_SVE
> 

thanks.
-- 
~Randy


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marc Zyngier]

On 2020-08-24 19:27, Catalin Marinas wrote:
> From: Vincenzo Frascino <vincenzo.frascino@arm.com>
> 
> Add the cpufeature and hwcap entries to detect the presence of MTE. Any
> secondary CPU not supporting the feature, if detected on the boot CPU,
> will be parked.
> 
> Add the minimum SCTLR_EL1 and HCR_EL2 bits for enabling MTE. The Normal
> Tagged memory type is configured in MAIR_EL1 before the MMU is enabled
> in order to avoid disrupting other CPUs in the CnP domain.
> 
> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> Co-developed-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Suzuki K Poulose <Suzuki.Poulose@arm.com>
> ---
> 
> Notes:
>     v8:
>     - Move the SCTLR_EL1, MAIR_EL1, GCR_EL1 and TFSR*_EL1 
> initialisation to
>       __cpu_setup before the MMU is enabled. While early MAIR_EL1 is
>       desirable to avoid conflicting with other CPUs in a CnP domain 
> the
>       TFSR_EL1 and GCR_EL1 will only come in handy later when support 
> for
>       in-kernel MTE is added.
> 
>     v7:
>     - Hide the MTE ID register field for guests until MTE gains support 
> for KVM.
> 
>  arch/arm64/include/asm/cpucaps.h    |  3 ++-
>  arch/arm64/include/asm/cpufeature.h |  6 ++++++
>  arch/arm64/include/asm/hwcap.h      |  2 +-
>  arch/arm64/include/asm/kvm_arm.h    |  2 +-
>  arch/arm64/include/asm/sysreg.h     |  1 +
>  arch/arm64/include/uapi/asm/hwcap.h |  2 +-
>  arch/arm64/kernel/cpufeature.c      | 17 +++++++++++++++++
>  arch/arm64/kernel/cpuinfo.c         |  2 +-
>  arch/arm64/kvm/sys_regs.c           |  2 ++
>  arch/arm64/mm/proc.S                | 24 ++++++++++++++++++++++++
>  10 files changed, 56 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/cpucaps.h 
> b/arch/arm64/include/asm/cpucaps.h
> index 07b643a70710..1937653b05a3 100644
> --- a/arch/arm64/include/asm/cpucaps.h
> +++ b/arch/arm64/include/asm/cpucaps.h
> @@ -64,7 +64,8 @@
>  #define ARM64_BTI				54
>  #define ARM64_HAS_ARMv8_4_TTL			55
>  #define ARM64_HAS_TLB_RANGE			56
> +#define ARM64_MTE				57
> 
> -#define ARM64_NCAPS				57
> +#define ARM64_NCAPS				58
> 
>  #endif /* __ASM_CPUCAPS_H */
> diff --git a/arch/arm64/include/asm/cpufeature.h
> b/arch/arm64/include/asm/cpufeature.h
> index 89b4f0142c28..680b5b36ddd5 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -681,6 +681,12 @@ static __always_inline bool
> system_uses_irq_prio_masking(void)
>  	       cpus_have_const_cap(ARM64_HAS_IRQ_PRIO_MASKING);
>  }
> 
> +static inline bool system_supports_mte(void)
> +{
> +	return IS_ENABLED(CONFIG_ARM64_MTE) &&
> +		cpus_have_const_cap(ARM64_MTE);
> +}
> +
>  static inline bool system_has_prio_mask_debugging(void)
>  {
>  	return IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) &&
> diff --git a/arch/arm64/include/asm/hwcap.h 
> b/arch/arm64/include/asm/hwcap.h
> index 22f73fe09030..0d4a6741b6a5 100644
> --- a/arch/arm64/include/asm/hwcap.h
> +++ b/arch/arm64/include/asm/hwcap.h
> @@ -95,7 +95,7 @@
>  #define KERNEL_HWCAP_DGH		__khwcap2_feature(DGH)
>  #define KERNEL_HWCAP_RNG		__khwcap2_feature(RNG)
>  #define KERNEL_HWCAP_BTI		__khwcap2_feature(BTI)
> -/* reserved for KERNEL_HWCAP_MTE	__khwcap2_feature(MTE) */
> +#define KERNEL_HWCAP_MTE		__khwcap2_feature(MTE)
> 
>  /*
>   * This yields a mask that user programs can use to figure out what
> diff --git a/arch/arm64/include/asm/kvm_arm.h 
> b/arch/arm64/include/asm/kvm_arm.h
> index 8a1cbfd544d6..6c3b2fc922bb 100644
> --- a/arch/arm64/include/asm/kvm_arm.h
> +++ b/arch/arm64/include/asm/kvm_arm.h
> @@ -78,7 +78,7 @@
>  			 HCR_AMO | HCR_SWIO | HCR_TIDCP | HCR_RW | HCR_TLOR | \
>  			 HCR_FMO | HCR_IMO)
>  #define HCR_VIRT_EXCP_MASK (HCR_VSE | HCR_VI | HCR_VF)
> -#define HCR_HOST_NVHE_FLAGS (HCR_RW | HCR_API | HCR_APK)
> +#define HCR_HOST_NVHE_FLAGS (HCR_RW | HCR_API | HCR_APK | HCR_ATA)
>  #define HCR_HOST_VHE_FLAGS (HCR_RW | HCR_TGE | HCR_E2H)

Why is HCR_ATA only set for nVHE? HCR_EL2.ATA seems to apply to both,
doesn't it?

> 
>  /* TCR_EL2 Registers bits */

[...]

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

Thanks,

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Catalin Marinas]

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/include/asm/kvm_arm.h
> > b/arch/arm64/include/asm/kvm_arm.h
> > index 8a1cbfd544d6..6c3b2fc922bb 100644
> > --- a/arch/arm64/include/asm/kvm_arm.h
> > +++ b/arch/arm64/include/asm/kvm_arm.h
> > @@ -78,7 +78,7 @@
> >  			 HCR_AMO | HCR_SWIO | HCR_TIDCP | HCR_RW | HCR_TLOR | \
> >  			 HCR_FMO | HCR_IMO)
> >  #define HCR_VIRT_EXCP_MASK (HCR_VSE | HCR_VI | HCR_VF)
> > -#define HCR_HOST_NVHE_FLAGS (HCR_RW | HCR_API | HCR_APK)
> > +#define HCR_HOST_NVHE_FLAGS (HCR_RW | HCR_API | HCR_APK | HCR_ATA)
> >  #define HCR_HOST_VHE_FLAGS (HCR_RW | HCR_TGE | HCR_E2H)
> 
> Why is HCR_ATA only set for nVHE? HCR_EL2.ATA seems to apply to both,
> doesn't it?

We need HCR_EL2.ATA to be set when !VHE so that the host kernel can use
MTE. That said, I think we need to turn it off when running a guest.
Even if we hide the ID register, the guest may still attempt to enable
tags on some memory that doesn't support it, leading to unpredictable
behaviour (well, only if we expose device memory to guests directly;
Steve's patches will deal with this but for now we just disable MTE in
guests).

With VHE, HCR_EL2.ATA only affects the guests, so it can stay off. The
host's use of tags is controlled by SCTLR_EL1/EL2.ATA (i.e. HCR_EL2.ATA
has no effect if E2H and TGE are both 1; qemu has a bug here which I
discovered yesterday).

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

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v8 27/28] arm64: mte: Kconfig entry

[Catalin Marinas]

On Mon, Aug 24, 2020 at 11:44:50AM -0700, Randy Dunlap wrote:
> On 8/24/20 11:27 AM, Catalin Marinas wrote:
> > index 6d232837cbee..10cf81d70657 100644
> > --- a/arch/arm64/Kconfig
> > +++ b/arch/arm64/Kconfig
> > @@ -1664,6 +1664,37 @@ config ARCH_RANDOM
> >  	  provides a high bandwidth, cryptographically secure
> >  	  hardware random number generator.
> >  
> > +config ARM64_AS_HAS_MTE
> > +	# Binutils gained initial support for MTE in 2.32.0. However, a
> > +	# late architecture addition (LDGM/STGM) is only supported in
> > +	# the newer 2.32.x and 2.33 versions.
> > +	def_bool $(as-instr,.arch armv8.5-a+memtag\nstgm xzr$(comma)[x0])
> 
> Would you mind translating that for me?
> Yes, I read the v7 Notes, but that only helped a little bit.

The initial MTE support in binutils 2.32.0, testable above with ".arch
armv8.5-a+memtag", was incomplete. Historically, I think it was based on
a beta version of the architecture but before the final architecture
release (ARMv8.5), MTE gained a couple of new instructions: STGM/LDGM.

Since there are binutils versions out there which don't understand STGM
even though they claim to support .arch armv8.5-a+memtag, it's better
for the above check to include the STGM instruction.

I'll see if I can make the comment above clearer.

> > +	bool "Memory Tagging Extension support"
> > +	default y
> > +	depends on ARM64_AS_HAS_MTE && ARM64_TAGGED_ADDR_ABI
> > +	select ARCH_USES_HIGH_VMA_FLAGS
> > +	help
> > +	  Memory Tagging (part of the ARMv8.5 Extensions) provides
> > +	  architectural support for run-time, always-on detection of
> 
> 	                            runtime,
> as is used below.

Thanks.

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marc Zyngier]

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/include/asm/kvm_arm.h
>> > b/arch/arm64/include/asm/kvm_arm.h
>> > index 8a1cbfd544d6..6c3b2fc922bb 100644
>> > --- a/arch/arm64/include/asm/kvm_arm.h
>> > +++ b/arch/arm64/include/asm/kvm_arm.h
>> > @@ -78,7 +78,7 @@
>> >  			 HCR_AMO | HCR_SWIO | HCR_TIDCP | HCR_RW | HCR_TLOR | \
>> >  			 HCR_FMO | HCR_IMO)
>> >  #define HCR_VIRT_EXCP_MASK (HCR_VSE | HCR_VI | HCR_VF)
>> > -#define HCR_HOST_NVHE_FLAGS (HCR_RW | HCR_API | HCR_APK)
>> > +#define HCR_HOST_NVHE_FLAGS (HCR_RW | HCR_API | HCR_APK | HCR_ATA)
>> >  #define HCR_HOST_VHE_FLAGS (HCR_RW | HCR_TGE | HCR_E2H)
>> 
>> Why is HCR_ATA only set for nVHE? HCR_EL2.ATA seems to apply to both,
>> doesn't it?
> 
> We need HCR_EL2.ATA to be set when !VHE so that the host kernel can use
> MTE. That said, I think we need to turn it off when running a guest.
> Even if we hide the ID register, the guest may still attempt to enable
> tags on some memory that doesn't support it, leading to unpredictable
> behaviour (well, only if we expose device memory to guests directly;
> Steve's patches will deal with this but for now we just disable MTE in
> guests).
> 
> With VHE, HCR_EL2.ATA only affects the guests, so it can stay off. The
> host's use of tags is controlled by SCTLR_EL1/EL2.ATA (i.e. HCR_EL2.ATA
> has no effect if E2H and TGE are both 1; qemu has a bug here which I
> discovered yesterday).

Ah, I missed that too.

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

Thanks,

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Catalin Marinas]

On Tue, Aug 25, 2020 at 11:54:50AM +0100, 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/include/asm/kvm_arm.h
> > > b/arch/arm64/include/asm/kvm_arm.h
> > > index 8a1cbfd544d6..6c3b2fc922bb 100644
> > > --- a/arch/arm64/include/asm/kvm_arm.h
> > > +++ b/arch/arm64/include/asm/kvm_arm.h
> > > @@ -78,7 +78,7 @@
> > >  			 HCR_AMO | HCR_SWIO | HCR_TIDCP | HCR_RW | HCR_TLOR | \
> > >  			 HCR_FMO | HCR_IMO)
> > >  #define HCR_VIRT_EXCP_MASK (HCR_VSE | HCR_VI | HCR_VF)
> > > -#define HCR_HOST_NVHE_FLAGS (HCR_RW | HCR_API | HCR_APK)
> > > +#define HCR_HOST_NVHE_FLAGS (HCR_RW | HCR_API | HCR_APK | HCR_ATA)
> > >  #define HCR_HOST_VHE_FLAGS (HCR_RW | HCR_TGE | HCR_E2H)
> > 
> > Why is HCR_ATA only set for nVHE? HCR_EL2.ATA seems to apply to both,
> > doesn't it?
> 
> We need HCR_EL2.ATA to be set when !VHE so that the host kernel can use
> MTE. That said, I think we need to turn it off when running a guest.
> Even if we hide the ID register, the guest may still attempt to enable
> tags on some memory that doesn't support it, leading to unpredictable
> behaviour (well, only if we expose device memory to guests directly;
> Steve's patches will deal with this but for now we just disable MTE in
> guests).

So if we want to properly disable MTE for guests when !VHE (not just the
ID reg), I came up with the diff below. However, given that Steven is
already working on KVM support, I wonder whether we could just make MTE
depend on !VHE temporarily, remove it once we get the full MTE KVM
support. It's up to you (either way, I still need to solve the undef
injection since that affects both VHE and !VHE; patch to follow).

diff --git a/arch/arm64/kvm/hyp/nvhe/tlb.c b/arch/arm64/kvm/hyp/nvhe/tlb.c
index 69eae608d670..51204ac30154 100644
--- a/arch/arm64/kvm/hyp/nvhe/tlb.c
+++ b/arch/arm64/kvm/hyp/nvhe/tlb.c
@@ -32,10 +32,23 @@ static void __tlb_switch_to_guest(struct kvm_s2_mmu *mmu,
 	}
 
 	__load_guest_stage2(mmu);
+
+	/* MTE is not supported in guests yet, disable access to tags */
+	if (system_supports_mte()) {
+		u64 val = read_sysreg(hcr_el2);
+		val &= ~HCR_ATA;
+		write_sysreg(val, hcr_el2);
+	}
 }
 
 static void __tlb_switch_to_host(struct tlb_inv_context *cxt)
 {
+	/* Re-enable MTE for the host kernel */
+	if (system_supports_mte()) {
+		u64 val = read_sysreg(hcr_el2);
+		write_sysreg(val | HCR_ATA, hcr_el2);
+	}
+
 	write_sysreg(0, vttbr_el2);
 
 	if (cpus_have_final_cap(ARM64_WORKAROUND_SPECULATIVE_AT)) {

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Catalin Marinas]

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

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

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marc Zyngier]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel