Re: [PATCH v6 06/21] arm64: Move VHE-specific SPE setup to mutate_to_vhe()
On 2021-02-04 09:34, Will Deacon wrote: On Thu, Feb 04, 2021 at 09:30:08AM +, Marc Zyngier wrote: [...] I think the following patch addresses the above issue, which I'll squash with the original patch. Please shout if I missed anything. Thanks, M. diff --git a/arch/arm64/kernel/hyp-stub.S b/arch/arm64/kernel/hyp-stub.S index aa7e8d592295..3e08dcc924b5 100644 --- a/arch/arm64/kernel/hyp-stub.S +++ b/arch/arm64/kernel/hyp-stub.S @@ -115,29 +115,9 @@ SYM_CODE_START_LOCAL(mutate_to_vhe) mrs_s x0, SYS_VBAR_EL12 msr vbar_el1, x0 - // Fixup SPE configuration, if supported... - mrs x1, id_aa64dfr0_el1 - ubfxx1, x1, #ID_AA64DFR0_PMSVER_SHIFT, #4 - mov x2, xzr - cbz x1, skip_spe - - // ... and not owned by EL3 - mrs_s x0, SYS_PMBIDR_EL1 - and x0, x0, #(1 << SYS_PMBIDR_EL1_P_SHIFT) - cbnzx0, skip_spe - - // Let the SPE driver in control of the sampling - mrs_s x0, SYS_PMSCR_EL1 - bic x0, x0, #(1 << SYS_PMSCR_EL2_PCT_SHIFT) - bic x0, x0, #(1 << SYS_PMSCR_EL2_PA_SHIFT) - msr_s SYS_PMSCR_EL1, x0 - mov x2, #MDCR_EL2_TPMS - -skip_spe: - // For VHE, use EL2 translation and disable access from EL1 + // Use EL2 translations for SPE and disable access from EL1 mrs x0, mdcr_el2 bic x0, x0, #(MDCR_EL2_E2PB_MASK << MDCR_EL2_E2PB_SHIFT) - orr x0, x0, x2 msr mdcr_el2, x0 Looks a tonne better, thanks! Be nice if somebody could test it for us. SPE-equipped machines are the silicon equivalent of hen's teeth... Alex, any chance you could give this a go? M. -- Jazz is not dead. It just smells funny... ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v6 06/21] arm64: Move VHE-specific SPE setup to mutate_to_vhe()
On Thu, Feb 04, 2021 at 09:30:08AM +, Marc Zyngier wrote: > Hi Will, > > On 2021-02-03 21:13, Will Deacon wrote: > > Hi Marc, > > > > On Mon, Feb 01, 2021 at 11:56:22AM +, Marc Zyngier wrote: > > > There isn't much that a VHE kernel needs on top of whatever has > > > been done for nVHE, so let's move the little we need to the > > > VHE stub (the SPE setup), and drop the init_el2_state macro. > > > > > > No expected functional change. > > > > > > Signed-off-by: Marc Zyngier > > > Acked-by: David Brazdil > > > Acked-by: Catalin Marinas > > > --- > > > arch/arm64/kernel/hyp-stub.S | 28 +--- > > > 1 file changed, 25 insertions(+), 3 deletions(-) > > > > > > diff --git a/arch/arm64/kernel/hyp-stub.S > > > b/arch/arm64/kernel/hyp-stub.S > > > index 373ed2213e1d..6b5c73cf9d52 100644 > > > --- a/arch/arm64/kernel/hyp-stub.S > > > +++ b/arch/arm64/kernel/hyp-stub.S > > > @@ -92,9 +92,6 @@ SYM_CODE_START_LOCAL(mutate_to_vhe) > > > msr hcr_el2, x0 > > > isb > > > > > > - // Doesn't do much on VHE, but still, worth a shot > > > - init_el2_state vhe > > > - > > > // Use the EL1 allocated stack, per-cpu offset > > > mrs x0, sp_el1 > > > mov sp, x0 > > > @@ -107,6 +104,31 @@ SYM_CODE_START_LOCAL(mutate_to_vhe) > > > mrs_s x0, SYS_VBAR_EL12 > > > msr vbar_el1, x0 > > > > > > + // Fixup SPE configuration, if supported... > > > + mrs x1, id_aa64dfr0_el1 > > > + ubfxx1, x1, #ID_AA64DFR0_PMSVER_SHIFT, #4 > > > + mov x2, xzr > > > + cbz x1, skip_spe > > > + > > > + // ... and not owned by EL3 > > > + mrs_s x0, SYS_PMBIDR_EL1 > > > + and x0, x0, #(1 << SYS_PMBIDR_EL1_P_SHIFT) > > > + cbnzx0, skip_spe > > > + > > > + // Let the SPE driver in control of the sampling > > > + mrs_s x0, SYS_PMSCR_EL1 > > > + bic x0, x0, #(1 << SYS_PMSCR_EL2_PCT_SHIFT) > > > + bic x0, x0, #(1 << SYS_PMSCR_EL2_PA_SHIFT) > > > + msr_s SYS_PMSCR_EL1, x0 > > > > Why do we need to touch pmscr_el1 at all? The SPE driver should take > > care of > > that, no? If you drop the pmscr_el1 accesses, I think you can drop the > > pmbidr_el1 check as well. > > That's definitely a brain fart, and is what we need on the nVHE path, > not here. Doing the same thing twice isn't exactly helpful. > > > And actually, then why even check dfr0? Doing the > > bic for the mdcr_el1.e2pb bits is harmless. > > > > > + mov x2, #MDCR_EL2_TPMS > > > + > > > +skip_spe: > > > + // For VHE, use EL2 translation and disable access from EL1 > > > + mrs x0, mdcr_el2 > > > + bic x0, x0, #(MDCR_EL2_E2PB_MASK << MDCR_EL2_E2PB_SHIFT) > > > + orr x0, x0, x2 > > > + msr mdcr_el2, x0 > > > > Doesn't this undo the setting of mdcr_el2.hpmn if SPE is not present or > > unavailable? (This wouldn't be an issue if we removed the skip_spe paths > > altogether). > > I don't think it does. We only clear the E2PB bits (harmless, as you point > out above), and OR something that is either 0 (no SPE) or MDCR_EL2_TPMS. > In any case, the HPMN bits are preserved (having been set during the nVHE > setup). Duh, OR not AND. Yes, sorry. > I think the following patch addresses the above issue, which I'll squash > with the original patch. Please shout if I missed anything. > > Thanks, > > M. > > diff --git a/arch/arm64/kernel/hyp-stub.S b/arch/arm64/kernel/hyp-stub.S > index aa7e8d592295..3e08dcc924b5 100644 > --- a/arch/arm64/kernel/hyp-stub.S > +++ b/arch/arm64/kernel/hyp-stub.S > @@ -115,29 +115,9 @@ SYM_CODE_START_LOCAL(mutate_to_vhe) > mrs_s x0, SYS_VBAR_EL12 > msr vbar_el1, x0 > > - // Fixup SPE configuration, if supported... > - mrs x1, id_aa64dfr0_el1 > - ubfxx1, x1, #ID_AA64DFR0_PMSVER_SHIFT, #4 > - mov x2, xzr > - cbz x1, skip_spe > - > - // ... and not owned by EL3 > - mrs_s x0, SYS_PMBIDR_EL1 > - and x0, x0, #(1 << SYS_PMBIDR_EL1_P_SHIFT) > - cbnzx0, skip_spe > - > - // Let the SPE driver in control of the sampling > - mrs_s x0, SYS_PMSCR_EL1 > - bic x0, x0, #(1 << SYS_PMSCR_EL2_PCT_SHIFT) > - bic x0, x0, #(1 << SYS_PMSCR_EL2_PA_SHIFT) > - msr_s SYS_PMSCR_EL1, x0 > - mov x2, #MDCR_EL2_TPMS > - > -skip_spe: > - // For VHE, use EL2 translation and disable access from EL1 > + // Use EL2 translations for SPE and disable access from EL1 > mrs x0, mdcr_el2 > bic x0, x0, #(MDCR_EL2_E2PB_MASK << MDCR_EL2_E2PB_SHIFT) > - orr x0, x0, x2 > msr mdcr_el2, x0 Looks a tonne better, thanks! Be nice if somebody could test it for us. Will ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v6 06/21] arm64: Move VHE-specific SPE setup to mutate_to_vhe()
Hi Will, On 2021-02-03 21:13, Will Deacon wrote: Hi Marc, On Mon, Feb 01, 2021 at 11:56:22AM +, Marc Zyngier wrote: There isn't much that a VHE kernel needs on top of whatever has been done for nVHE, so let's move the little we need to the VHE stub (the SPE setup), and drop the init_el2_state macro. No expected functional change. Signed-off-by: Marc Zyngier Acked-by: David Brazdil Acked-by: Catalin Marinas --- arch/arm64/kernel/hyp-stub.S | 28 +--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/arch/arm64/kernel/hyp-stub.S b/arch/arm64/kernel/hyp-stub.S index 373ed2213e1d..6b5c73cf9d52 100644 --- a/arch/arm64/kernel/hyp-stub.S +++ b/arch/arm64/kernel/hyp-stub.S @@ -92,9 +92,6 @@ SYM_CODE_START_LOCAL(mutate_to_vhe) msr hcr_el2, x0 isb - // Doesn't do much on VHE, but still, worth a shot - init_el2_state vhe - // Use the EL1 allocated stack, per-cpu offset mrs x0, sp_el1 mov sp, x0 @@ -107,6 +104,31 @@ SYM_CODE_START_LOCAL(mutate_to_vhe) mrs_s x0, SYS_VBAR_EL12 msr vbar_el1, x0 + // Fixup SPE configuration, if supported... + mrs x1, id_aa64dfr0_el1 + ubfxx1, x1, #ID_AA64DFR0_PMSVER_SHIFT, #4 + mov x2, xzr + cbz x1, skip_spe + + // ... and not owned by EL3 + mrs_s x0, SYS_PMBIDR_EL1 + and x0, x0, #(1 << SYS_PMBIDR_EL1_P_SHIFT) + cbnzx0, skip_spe + + // Let the SPE driver in control of the sampling + mrs_s x0, SYS_PMSCR_EL1 + bic x0, x0, #(1 << SYS_PMSCR_EL2_PCT_SHIFT) + bic x0, x0, #(1 << SYS_PMSCR_EL2_PA_SHIFT) + msr_s SYS_PMSCR_EL1, x0 Why do we need to touch pmscr_el1 at all? The SPE driver should take care of that, no? If you drop the pmscr_el1 accesses, I think you can drop the pmbidr_el1 check as well. That's definitely a brain fart, and is what we need on the nVHE path, not here. Doing the same thing twice isn't exactly helpful. And actually, then why even check dfr0? Doing the bic for the mdcr_el1.e2pb bits is harmless. + mov x2, #MDCR_EL2_TPMS + +skip_spe: + // For VHE, use EL2 translation and disable access from EL1 + mrs x0, mdcr_el2 + bic x0, x0, #(MDCR_EL2_E2PB_MASK << MDCR_EL2_E2PB_SHIFT) + orr x0, x0, x2 + msr mdcr_el2, x0 Doesn't this undo the setting of mdcr_el2.hpmn if SPE is not present or unavailable? (This wouldn't be an issue if we removed the skip_spe paths altogether). I don't think it does. We only clear the E2PB bits (harmless, as you point out above), and OR something that is either 0 (no SPE) or MDCR_EL2_TPMS. In any case, the HPMN bits are preserved (having been set during the nVHE setup). I think the following patch addresses the above issue, which I'll squash with the original patch. Please shout if I missed anything. Thanks, M. diff --git a/arch/arm64/kernel/hyp-stub.S b/arch/arm64/kernel/hyp-stub.S index aa7e8d592295..3e08dcc924b5 100644 --- a/arch/arm64/kernel/hyp-stub.S +++ b/arch/arm64/kernel/hyp-stub.S @@ -115,29 +115,9 @@ SYM_CODE_START_LOCAL(mutate_to_vhe) mrs_s x0, SYS_VBAR_EL12 msr vbar_el1, x0 - // Fixup SPE configuration, if supported... - mrs x1, id_aa64dfr0_el1 - ubfxx1, x1, #ID_AA64DFR0_PMSVER_SHIFT, #4 - mov x2, xzr - cbz x1, skip_spe - - // ... and not owned by EL3 - mrs_s x0, SYS_PMBIDR_EL1 - and x0, x0, #(1 << SYS_PMBIDR_EL1_P_SHIFT) - cbnzx0, skip_spe - - // Let the SPE driver in control of the sampling - mrs_s x0, SYS_PMSCR_EL1 - bic x0, x0, #(1 << SYS_PMSCR_EL2_PCT_SHIFT) - bic x0, x0, #(1 << SYS_PMSCR_EL2_PA_SHIFT) - msr_s SYS_PMSCR_EL1, x0 - mov x2, #MDCR_EL2_TPMS - -skip_spe: - // For VHE, use EL2 translation and disable access from EL1 + // Use EL2 translations for SPE and disable access from EL1 mrs x0, mdcr_el2 bic x0, x0, #(MDCR_EL2_E2PB_MASK << MDCR_EL2_E2PB_SHIFT) - orr x0, x0, x2 msr mdcr_el2, x0 // Transfer the MM state from EL1 to EL2 -- Jazz is not dead. It just smells funny... ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v6 06/21] arm64: Move VHE-specific SPE setup to mutate_to_vhe()
Hi Marc, On Mon, Feb 01, 2021 at 11:56:22AM +, Marc Zyngier wrote: > There isn't much that a VHE kernel needs on top of whatever has > been done for nVHE, so let's move the little we need to the > VHE stub (the SPE setup), and drop the init_el2_state macro. > > No expected functional change. > > Signed-off-by: Marc Zyngier > Acked-by: David Brazdil > Acked-by: Catalin Marinas > --- > arch/arm64/kernel/hyp-stub.S | 28 +--- > 1 file changed, 25 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/kernel/hyp-stub.S b/arch/arm64/kernel/hyp-stub.S > index 373ed2213e1d..6b5c73cf9d52 100644 > --- a/arch/arm64/kernel/hyp-stub.S > +++ b/arch/arm64/kernel/hyp-stub.S > @@ -92,9 +92,6 @@ SYM_CODE_START_LOCAL(mutate_to_vhe) > msr hcr_el2, x0 > isb > > - // Doesn't do much on VHE, but still, worth a shot > - init_el2_state vhe > - > // Use the EL1 allocated stack, per-cpu offset > mrs x0, sp_el1 > mov sp, x0 > @@ -107,6 +104,31 @@ SYM_CODE_START_LOCAL(mutate_to_vhe) > mrs_s x0, SYS_VBAR_EL12 > msr vbar_el1, x0 > > + // Fixup SPE configuration, if supported... > + mrs x1, id_aa64dfr0_el1 > + ubfxx1, x1, #ID_AA64DFR0_PMSVER_SHIFT, #4 > + mov x2, xzr > + cbz x1, skip_spe > + > + // ... and not owned by EL3 > + mrs_s x0, SYS_PMBIDR_EL1 > + and x0, x0, #(1 << SYS_PMBIDR_EL1_P_SHIFT) > + cbnzx0, skip_spe > + > + // Let the SPE driver in control of the sampling > + mrs_s x0, SYS_PMSCR_EL1 > + bic x0, x0, #(1 << SYS_PMSCR_EL2_PCT_SHIFT) > + bic x0, x0, #(1 << SYS_PMSCR_EL2_PA_SHIFT) > + msr_s SYS_PMSCR_EL1, x0 Why do we need to touch pmscr_el1 at all? The SPE driver should take care of that, no? If you drop the pmscr_el1 accesses, I think you can drop the pmbidr_el1 check as well. And actually, then why even check dfr0? Doing the bic for the mdcr_el1.e2pb bits is harmless. > + mov x2, #MDCR_EL2_TPMS > + > +skip_spe: > + // For VHE, use EL2 translation and disable access from EL1 > + mrs x0, mdcr_el2 > + bic x0, x0, #(MDCR_EL2_E2PB_MASK << MDCR_EL2_E2PB_SHIFT) > + orr x0, x0, x2 > + msr mdcr_el2, x0 Doesn't this undo the setting of mdcr_el2.hpmn if SPE is not present or unavailable? (This wouldn't be an issue if we removed the skip_spe paths altogether). Will ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH v6 06/21] arm64: Move VHE-specific SPE setup to mutate_to_vhe()
There isn't much that a VHE kernel needs on top of whatever has been done for nVHE, so let's move the little we need to the VHE stub (the SPE setup), and drop the init_el2_state macro. No expected functional change. Signed-off-by: Marc Zyngier Acked-by: David Brazdil Acked-by: Catalin Marinas --- arch/arm64/kernel/hyp-stub.S | 28 +--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/arch/arm64/kernel/hyp-stub.S b/arch/arm64/kernel/hyp-stub.S index 373ed2213e1d..6b5c73cf9d52 100644 --- a/arch/arm64/kernel/hyp-stub.S +++ b/arch/arm64/kernel/hyp-stub.S @@ -92,9 +92,6 @@ SYM_CODE_START_LOCAL(mutate_to_vhe) msr hcr_el2, x0 isb - // Doesn't do much on VHE, but still, worth a shot - init_el2_state vhe - // Use the EL1 allocated stack, per-cpu offset mrs x0, sp_el1 mov sp, x0 @@ -107,6 +104,31 @@ SYM_CODE_START_LOCAL(mutate_to_vhe) mrs_s x0, SYS_VBAR_EL12 msr vbar_el1, x0 + // Fixup SPE configuration, if supported... + mrs x1, id_aa64dfr0_el1 + ubfxx1, x1, #ID_AA64DFR0_PMSVER_SHIFT, #4 + mov x2, xzr + cbz x1, skip_spe + + // ... and not owned by EL3 + mrs_s x0, SYS_PMBIDR_EL1 + and x0, x0, #(1 << SYS_PMBIDR_EL1_P_SHIFT) + cbnzx0, skip_spe + + // Let the SPE driver in control of the sampling + mrs_s x0, SYS_PMSCR_EL1 + bic x0, x0, #(1 << SYS_PMSCR_EL2_PCT_SHIFT) + bic x0, x0, #(1 << SYS_PMSCR_EL2_PA_SHIFT) + msr_s SYS_PMSCR_EL1, x0 + mov x2, #MDCR_EL2_TPMS + +skip_spe: + // For VHE, use EL2 translation and disable access from EL1 + mrs x0, mdcr_el2 + bic x0, x0, #(MDCR_EL2_E2PB_MASK << MDCR_EL2_E2PB_SHIFT) + orr x0, x0, x2 + msr mdcr_el2, x0 + // Transfer the MM state from EL1 to EL2 mrs_s x0, SYS_TCR_EL12 msr tcr_el1, x0 -- 2.29.2 ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
