| Message ID | 20221208-arm64-sme2-v4-7-f2fa0aef982f@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f122576f35336820259a79847e408b9f807eba15 |
| Headers | show |
| Series | arm64/sme: Support SME 2 and SME 2.1 | expand |
On Mon, 16 Jan 2023 16:04:42 +0000, Mark Brown <broonie@kernel.org> wrote: > > The new register ZT0 introduced by SME2 comes with a new trap, disable it > for the host kernel so that we can implement support for it. > > Signed-off-by: Mark Brown <broonie@kernel.org> > --- > arch/arm64/kernel/hyp-stub.S | 6 ++++++ > arch/arm64/kernel/idreg-override.c | 1 + > 2 files changed, 7 insertions(+) > > diff --git a/arch/arm64/kernel/hyp-stub.S b/arch/arm64/kernel/hyp-stub.S > index 2ee18c860f2a..d31d1acb170d 100644 > --- a/arch/arm64/kernel/hyp-stub.S > +++ b/arch/arm64/kernel/hyp-stub.S > @@ -132,6 +132,12 @@ SYM_CODE_START_LOCAL(__finalise_el2) > orr x0, x0, SMCR_ELx_FA64_MASK > .Lskip_sme_fa64: > > + // ZT0 available? > + __check_override id_aa64smfr0 ID_AA64SMFR0_EL1_SMEver_SHIFT 4 .Linit_sme_zt0 .Lskip_sme_zt0 > +.Linit_sme_zt0: > + orr x0, x0, SMCR_ELx_EZT0_MASK > +.Lskip_sme_zt0: > + I've been looking at this in order to solve a merge conflict in next, and couldn't convince myself that the above actually works. __check_override assumes that the ID_AA64SMFR0_EL1 value is in x1, and I guess that the intent of the code is to reuse value read a few lines above. But as the comment says at the beginning of the macro, x1 will be clobbered, and the checks always fails. I presume we're just lucky that sme2_kernel_enable() does the same thing unconditionally, which probably means this was only ever tested with a VHE kernel (it'd otherwise catch fire). The easiest fix is just to reload the id register before checking it, something like the patch below, compile-tested only. M. From a6c4aaccd33e453ffc8d8ea23a4dd4d9a263cc89 Mon Sep 17 00:00:00 2001 From: Marc Zyngier <maz@kernel.org> Date: Mon, 6 Feb 2023 09:24:40 +0000 Subject: [PATCH] arm64/sme: Fix __finalise_el2 SMEver check When checking for ID_AA64SMFR0_EL1.SMEver, __check_override assumes that the ID_AA64SMFR0_EL1 value is in x1, and the intent of the code is to reuse value read a few lines above. However, as the comment says at the beginning of the macro, x1 will be clobbered, and the checks always fails. The easiest fix is just to reload the id register before checking it. Fixes: f122576f3533 ("arm64/sme: Enable host kernel to access ZT0") Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/kernel/hyp-stub.S | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm64/kernel/hyp-stub.S b/arch/arm64/kernel/hyp-stub.S index d31d1acb170d..111ff33d93ee 100644 --- a/arch/arm64/kernel/hyp-stub.S +++ b/arch/arm64/kernel/hyp-stub.S @@ -133,6 +133,7 @@ SYM_CODE_START_LOCAL(__finalise_el2) .Lskip_sme_fa64: // ZT0 available? + mrs_s x1, SYS_ID_AA64SMFR0_EL1 __check_override id_aa64smfr0 ID_AA64SMFR0_EL1_SMEver_SHIFT 4 .Linit_sme_zt0 .Lskip_sme_zt0 .Linit_sme_zt0: orr x0, x0, SMCR_ELx_EZT0_MASK
On Mon, Feb 06, 2023 at 09:31:20AM +0000, Marc Zyngier wrote: > __check_override assumes that the ID_AA64SMFR0_EL1 value is in x1, and > I guess that the intent of the code is to reuse value read a few lines > above. But as the comment says at the beginning of the macro, x1 will > be clobbered, and the checks always fails. Yes, it looks like this is a victim of rebasing - I didn't spot the change to make x1 clobbered when the override checking was refactored. Thanks for spotting this. > I presume we're just lucky that sme2_kernel_enable() does the same > thing unconditionally, which probably means this was only ever tested > with a VHE kernel (it'd otherwise catch fire). Yes, I'd not be surprised if I'd never run this in nVHE. > The easiest fix is just to reload the id register before checking it, > something like the patch below, compile-tested only. Reviewed-by: Mark Brown <broonie@kernel.org>
On Mon, Feb 06, 2023 at 09:31:20AM +0000, Marc Zyngier wrote: > From a6c4aaccd33e453ffc8d8ea23a4dd4d9a263cc89 Mon Sep 17 00:00:00 2001 > From: Marc Zyngier <maz@kernel.org> > Date: Mon, 6 Feb 2023 09:24:40 +0000 > Subject: [PATCH] arm64/sme: Fix __finalise_el2 SMEver check > > When checking for ID_AA64SMFR0_EL1.SMEver, __check_override assumes > that the ID_AA64SMFR0_EL1 value is in x1, and the intent of the code > is to reuse value read a few lines above. > > However, as the comment says at the beginning of the macro, x1 will > be clobbered, and the checks always fails. > > The easiest fix is just to reload the id register before checking it. > > Fixes: f122576f3533 ("arm64/sme: Enable host kernel to access ZT0") > Signed-off-by: Marc Zyngier <maz@kernel.org> Thanks Marc. I queued it on top of the for-next/sme2 branch.
diff --git a/arch/arm64/kernel/hyp-stub.S b/arch/arm64/kernel/hyp-stub.S index 2ee18c860f2a..d31d1acb170d 100644 --- a/arch/arm64/kernel/hyp-stub.S +++ b/arch/arm64/kernel/hyp-stub.S @@ -132,6 +132,12 @@ SYM_CODE_START_LOCAL(__finalise_el2) orr x0, x0, SMCR_ELx_FA64_MASK .Lskip_sme_fa64: + // ZT0 available? + __check_override id_aa64smfr0 ID_AA64SMFR0_EL1_SMEver_SHIFT 4 .Linit_sme_zt0 .Lskip_sme_zt0 +.Linit_sme_zt0: + orr x0, x0, SMCR_ELx_EZT0_MASK +.Lskip_sme_zt0: + orr x0, x0, #SMCR_ELx_LEN_MASK // Enable full SME vector msr_s SYS_SMCR_EL2, x0 // length for EL1. diff --git a/arch/arm64/kernel/idreg-override.c b/arch/arm64/kernel/idreg-override.c index 95133765ed29..d833d78a7f31 100644 --- a/arch/arm64/kernel/idreg-override.c +++ b/arch/arm64/kernel/idreg-override.c @@ -131,6 +131,7 @@ static const struct ftr_set_desc smfr0 __initconst = { .name = "id_aa64smfr0", .override = &id_aa64smfr0_override, .fields = { + FIELD("smever", ID_AA64SMFR0_EL1_SMEver_SHIFT, NULL), /* FA64 is a one bit field... :-/ */ { "fa64", ID_AA64SMFR0_EL1_FA64_SHIFT, 1, }, {}
The new register ZT0 introduced by SME2 comes with a new trap, disable it for the host kernel so that we can implement support for it. Signed-off-by: Mark Brown <broonie@kernel.org> --- arch/arm64/kernel/hyp-stub.S | 6 ++++++ arch/arm64/kernel/idreg-override.c | 1 + 2 files changed, 7 insertions(+)