Message ID | b3606e76af42f7ecf65b1bfc2a5ed30a@codeaurora.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Relax CPU features sanity checking on heterogeneous architectures | expand |
On 11/10/2019 07:49, Sai Prakash Ranjan wrote: > diff --git a/arch/arm64/kernel/cpufeature.c > b/arch/arm64/kernel/cpufeature.c > index cabebf1a7976..207197692caa 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -164,8 +164,8 @@ static const struct arm64_ftr_bits ftr_id_aa64pfr0[] > = { > S_ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, > ID_AA64PFR0_FP_SHIFT, 4, ID_AA64PFR0_FP_NI), > /* Linux doesn't care about the EL3 */ > ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, > ID_AA64PFR0_EL3_SHIFT, 4, 0), > - ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, > ID_AA64PFR0_EL2_SHIFT, 4, 0), > - ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, > ID_AA64PFR0_EL1_SHIFT, 4, ID_AA64PFR0_EL1_64BIT_ONLY), > + ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, > ID_AA64PFR0_EL2_SHIFT, 4, 0), > + ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, > ID_AA64PFR0_EL1_SHIFT, 4, ID_AA64PFR0_EL1_64BIT_ONLY), > ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, > ID_AA64PFR0_EL0_SHIFT, 4, ID_AA64PFR0_EL0_64BIT_ONLY), > ARM64_FTR_END, > }; > @@ -345,10 +345,10 @@ static const struct arm64_ftr_bits > ftr_generic_32bits[] = { > ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 24, 4, > 0), > ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 20, 4, > 0), > ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 16, 4, > 0), > - ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 12, 4, > 0), > + ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, 12, 4, > 0), > ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 8, 4, 0), > - ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 4, 4, 0), > - ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 0, 4, 0), > + ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, 4, 4, > 0), > + ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, 0, 4, > 0), > ARM64_FTR_END, > }; Hello Sai, Could you configure your webmail client to not wrap "long" lines? Wrapping might break the patch, and the kernel logs would look better in their original form. Regards.
Hi Marc, On 2019-10-11 14:49, Marc Gonzalez wrote: > > Hello Sai, > > Could you configure your webmail client to not wrap "long" lines? > > Wrapping might break the patch, and the kernel logs would look better > in their original form. > Oh right, sorry did not see that. I use git send-email for patches, so no problem of wrapping in those cases. Here I just used my webmail to compose the message and just pasted the diff, will take care of it in future. Thanks, Sai
Hi, On Fri, Oct 11, 2019 at 11:19:00AM +0530, Sai Prakash Ranjan wrote: > On latest QCOM SoCs like SM8150 and SC7180 with big.LITTLE arch, below > warnings are observed during bootup of big cpu cores. For reference, which CPUs are in those SoCs? > SM8150: > > [ 0.271177] CPU features: SANITY CHECK: Unexpected variation in > SYS_ID_AA64PFR0_EL1. Boot CPU: 0x00000011112222, CPU4: 0x00000011111112 The differing fields are EL3, EL2, and EL1: the boot CPU supports AArch64 and AArch32 at those exception levels, while the secondary only supports AArch64. Do we handle this variation in KVM? > [ 0.271184] CPU features: SANITY CHECK: Unexpected variation in > SYS_ID_ISAR4_EL1. Boot CPU: 0x00000000011142, CPU4: 0x00000000010142 The differing field is (AArch32) SMC: present on the boot CPU, but missing on the secondary CPU. This is mandated to be zero when AArch32 isn' implemented at EL1. > [ 0.271189] CPU features: SANITY CHECK: Unexpected variation in > SYS_ID_PFR1_EL1. Boot CPU: 0x00000010011011, CPU4: 0x00000010010000 The differing fields are (AArch32) Virtualization, Security, and ProgMod: all present on the boot CPU, but missing on the secondary CPU. All mandated to be zero when AArch32 isn' implemented at EL1. > SC7180: > > [ 0.812770] CPU features: SANITY CHECK: Unexpected variation in > SYS_CTR_EL0. Boot CPU: 0x00000084448004, CPU6: 0x0000009444c004 The differing fields are: * IDC: present only on the secondary CPU. This is a worrying mismatch because it could mean that required cache maintenance is missed in some cases. Does the secondary CPU definitely broadcast PoU maintenance to the boot CPU that requires it? * L1Ip: VIPT on the boot CPU, PIPT on the secondary CPU. > [ 0.812838] CPU features: SANITY CHECK: Unexpected variation in > SYS_ID_AA64MMFR2_EL1. Boot CPU: 0x00000000001011, CPU6: 0x00000000000011 The differing field is IESB: presend on the boot CPU, missing on the secondary CPU. > [ 0.812876] CPU features: SANITY CHECK: Unexpected variation in > SYS_ID_AA64PFR0_EL1. Boot CPU: 0x00000011112222, CPU6: 0x1100000011111112 > [ 0.812924] CPU features: SANITY CHECK: Unexpected variation in > SYS_ID_ISAR4_EL1. Boot CPU: 0x00000000011142, CPU6: 0x00000000010142 > [ 0.812950] CPU features: SANITY CHECK: Unexpected variation in > SYS_ID_PFR0_EL1. Boot CPU: 0x00000010000131, CPU6: 0x00000010010131 > [ 0.812977] CPU features: SANITY CHECK: Unexpected variation in > SYS_ID_PFR1_EL1. Boot CPU: 0x00000010011011, CPU6: 0x00000010010000 These are the same story as for SM8150. > Can we relax some sanity checking for these by making it FTR_NONSTRICT or by > some other means? I just tried below roughly for SM8150 but I guess this is > not correct, > maybe for ftr_generic_32bits we should be checking bootcpu and nonboot cpu > partnum(to identify big.LITTLE) and then make it nonstrict? > These are all my wild assumptions, please correct me if I am wrong. Before we make any changes, we need to check whether we do actually handle this variation in a safe way, and we need to consider what this means w.r.t. late CPU hotplug. Even if we can handle variation at boot time, once we've determined the set of system-wide features we cannot allow those to regress, and I believe we'll need new code to enforce that. I don't think it's sufficient to mark these as NONSTRICT, though we might do that with other changes. We shouldn't look at the part number at all here. We care about variation across CPUs regardless of whether this is big.LITTLE or some variation in tie-offs, etc. Thanks, Mark. > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index cabebf1a7976..207197692caa 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -164,8 +164,8 @@ static const struct arm64_ftr_bits ftr_id_aa64pfr0[] = { > S_ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, > ID_AA64PFR0_FP_SHIFT, 4, ID_AA64PFR0_FP_NI), > /* Linux doesn't care about the EL3 */ > ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, > ID_AA64PFR0_EL3_SHIFT, 4, 0), > - ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, > ID_AA64PFR0_EL2_SHIFT, 4, 0), > - ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, > ID_AA64PFR0_EL1_SHIFT, 4, ID_AA64PFR0_EL1_64BIT_ONLY), > + ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, > ID_AA64PFR0_EL2_SHIFT, 4, 0), > + ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, > ID_AA64PFR0_EL1_SHIFT, 4, ID_AA64PFR0_EL1_64BIT_ONLY), > ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, > ID_AA64PFR0_EL0_SHIFT, 4, ID_AA64PFR0_EL0_64BIT_ONLY), > ARM64_FTR_END, > }; > @@ -345,10 +345,10 @@ static const struct arm64_ftr_bits > ftr_generic_32bits[] = { > ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 24, 4, 0), > ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 20, 4, 0), > ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 16, 4, 0), > - ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 12, 4, 0), > + ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, 12, 4, 0), > ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 8, 4, 0), > - ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 4, 4, 0), > - ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 0, 4, 0), > + ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, 4, 4, 0), > + ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, 0, 4, 0), > ARM64_FTR_END, > }; > > > Thanks, > Sai > > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation
On 11/10/2019 12:50, Mark Rutland wrote: > Before we make any changes, we need to check whether we do actually > handle this variation in a safe way, and we need to consider what this > means w.r.t. late CPU hotplug. > > Even if we can handle variation at boot time, once we've determined the > set of system-wide features we cannot allow those to regress, and I > believe we'll need new code to enforce that. I don't think it's > sufficient to mark these as NONSTRICT, though we might do that with > other changes. > > We shouldn't look at the part number at all here. We care about > variation across CPUs regardless of whether this is big.LITTLE or some > variation in tie-offs, etc. See also the "Unexpected variation in SYS_ID_AA64MMFR0_EL1" thread from a year ago: (that was on msm8998) https://www.spinics.net/lists/arm-kernel/msg691242.html Regards.
Hi Mark, Thanks a lot for the detailed explanations, I did have a look at all the variations before posting this. On 2019-10-11 16:20, Mark Rutland wrote: > Hi, > > On Fri, Oct 11, 2019 at 11:19:00AM +0530, Sai Prakash Ranjan wrote: >> On latest QCOM SoCs like SM8150 and SC7180 with big.LITTLE arch, below >> warnings are observed during bootup of big cpu cores. > > For reference, which CPUs are in those SoCs? > SM8150 is based on Cortex-A55(little cores) and Cortex-A76(big cores). I'm afraid I cannot give details about SC7180 yet. >> SM8150: >> >> [ 0.271177] CPU features: SANITY CHECK: Unexpected variation in >> SYS_ID_AA64PFR0_EL1. Boot CPU: 0x00000011112222, CPU4: >> 0x00000011111112 > > The differing fields are EL3, EL2, and EL1: the boot CPU supports > AArch64 and AArch32 at those exception levels, while the secondary only > supports AArch64. > > Do we handle this variation in KVM? We do not support KVM. > >> [ 0.271184] CPU features: SANITY CHECK: Unexpected variation in >> SYS_ID_ISAR4_EL1. Boot CPU: 0x00000000011142, CPU4: 0x00000000010142 > > The differing field is (AArch32) SMC: present on the boot CPU, but > missing on the secondary CPU. > > This is mandated to be zero when AArch32 isn' implemented at EL1. > So this need not be strict? >> [ 0.271189] CPU features: SANITY CHECK: Unexpected variation in >> SYS_ID_PFR1_EL1. Boot CPU: 0x00000010011011, CPU4: 0x00000010010000 > > The differing fields are (AArch32) Virtualization, Security, and > ProgMod: all present on the boot CPU, but missing on the secondary > CPU. > > All mandated to be zero when AArch32 isn' implemented at EL1. > Same here, this need not be strict? >> SC7180: >> >> [ 0.812770] CPU features: SANITY CHECK: Unexpected variation in >> SYS_CTR_EL0. Boot CPU: 0x00000084448004, CPU6: 0x0000009444c004 > > The differing fields are: > > * IDC: present only on the secondary CPU. This is a worrying mismatch > because it could mean that required cache maintenance is missed in > some cases. Does the secondary CPU definitely broadcast PoU > maintenance to the boot CPU that requires it? > I will get some more details from internal cpu team about this one. > * L1Ip: VIPT on the boot CPU, PIPT on the secondary CPU. > >> [ 0.812838] CPU features: SANITY CHECK: Unexpected variation in >> SYS_ID_AA64MMFR2_EL1. Boot CPU: 0x00000000001011, CPU6: >> 0x00000000000011 > > The differing field is IESB: presend on the boot CPU, missing on the > secondary CPU. > >> [ 0.812876] CPU features: SANITY CHECK: Unexpected variation in >> SYS_ID_AA64PFR0_EL1. Boot CPU: 0x00000011112222, CPU6: > 0x1100000011111112 >> [ 0.812924] CPU features: SANITY CHECK: Unexpected variation in >> SYS_ID_ISAR4_EL1. Boot CPU: 0x00000000011142, CPU6: 0x00000000010142 >> [ 0.812950] CPU features: SANITY CHECK: Unexpected variation in >> SYS_ID_PFR0_EL1. Boot CPU: 0x00000010000131, CPU6: 0x00000010010131 >> [ 0.812977] CPU features: SANITY CHECK: Unexpected variation in >> SYS_ID_PFR1_EL1. Boot CPU: 0x00000010011011, CPU6: 0x00000010010000 > > These are the same story as for SM8150. > >> Can we relax some sanity checking for these by making it FTR_NONSTRICT > or by >> some other means? I just tried below roughly for SM8150 but I guess >> this > is >> not correct, >> maybe for ftr_generic_32bits we should be checking bootcpu and nonboot > cpu >> partnum(to identify big.LITTLE) and then make it nonstrict? >> These are all my wild assumptions, please correct me if I am wrong. > > Before we make any changes, we need to check whether we do actually > handle this variation in a safe way, and we need to consider what this > means w.r.t. late CPU hotplug. > > Even if we can handle variation at boot time, once we've determined the > set of system-wide features we cannot allow those to regress, and I > believe we'll need new code to enforce that. I don't think it's > sufficient to mark these as NONSTRICT, though we might do that with > other changes. > > We shouldn't look at the part number at all here. We care about > variation across CPUs regardless of whether this is big.LITTLE or some > variation in tie-offs, etc. > Thanks, Sai
On Fri, 11 Oct 2019 11:50:11 +0100 Mark Rutland <mark.rutland@arm.com> wrote: > Hi, > > On Fri, Oct 11, 2019 at 11:19:00AM +0530, Sai Prakash Ranjan wrote: > > On latest QCOM SoCs like SM8150 and SC7180 with big.LITTLE arch, below > > warnings are observed during bootup of big cpu cores. > > For reference, which CPUs are in those SoCs? > > > SM8150: > > > > [ 0.271177] CPU features: SANITY CHECK: Unexpected variation in > > SYS_ID_AA64PFR0_EL1. Boot CPU: 0x00000011112222, CPU4: 0x00000011111112 > > The differing fields are EL3, EL2, and EL1: the boot CPU supports > AArch64 and AArch32 at those exception levels, while the secondary only > supports AArch64. > > Do we handle this variation in KVM? We do, at least at vcpu creation time (see kvm_reset_vcpu). But if one of the !AArch32 CPU comes in late in the game (after we've started a guest), all bets are off (we'll schedule the 32bit guest on that CPU, enter the guest, immediately take an Illegal Exception Return, and return to userspace with KVM_EXIT_FAIL_ENTRY). Not sure we could do better, given the HW. My preference would be to fail these CPUs if they aren't present at boot time. M.
On 2019-10-11 16:39, Marc Gonzalez wrote: > On 11/10/2019 12:50, Mark Rutland wrote: > >> Before we make any changes, we need to check whether we do actually >> handle this variation in a safe way, and we need to consider what this >> means w.r.t. late CPU hotplug. >> >> Even if we can handle variation at boot time, once we've determined >> the >> set of system-wide features we cannot allow those to regress, and I >> believe we'll need new code to enforce that. I don't think it's >> sufficient to mark these as NONSTRICT, though we might do that with >> other changes. >> >> We shouldn't look at the part number at all here. We care about >> variation across CPUs regardless of whether this is big.LITTLE or some >> variation in tie-offs, etc. > > See also the "Unexpected variation in SYS_ID_AA64MMFR0_EL1" thread > from a year ago: (that was on msm8998) > > https://www.spinics.net/lists/arm-kernel/msg691242.html > I think, it was fixed by commit: 5717fe5ab38f ("arm64: cpufeature: Don't treat granule sizes as strict") Thanks, Sai
On Fri, 11 Oct 2019 18:47:39 +0530 Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org> wrote: > Hi Mark, > > Thanks a lot for the detailed explanations, I did have a look at all the variations before posting this. > > On 2019-10-11 16:20, Mark Rutland wrote: > > Hi, > > > > On Fri, Oct 11, 2019 at 11:19:00AM +0530, Sai Prakash Ranjan wrote: > >> On latest QCOM SoCs like SM8150 and SC7180 with big.LITTLE arch, below > >> warnings are observed during bootup of big cpu cores. > > > > For reference, which CPUs are in those SoCs? > > > > SM8150 is based on Cortex-A55(little cores) and Cortex-A76(big cores). I'm afraid I cannot give details about SC7180 yet. > > >> SM8150: > >> >> [ 0.271177] CPU features: SANITY CHECK: Unexpected variation in > >> SYS_ID_AA64PFR0_EL1. Boot CPU: 0x00000011112222, CPU4: >> 0x00000011111112 > > > > The differing fields are EL3, EL2, and EL1: the boot CPU supports > > AArch64 and AArch32 at those exception levels, while the secondary only > > supports AArch64. > > > > Do we handle this variation in KVM? > > We do not support KVM. Mainline does. You don't get to pick and choose what is supported or not. Thanks, M.
On 2019-10-11 19:04, Marc Zyngier wrote: > On Fri, 11 Oct 2019 18:47:39 +0530 > Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org> wrote: > >> Hi Mark, >> >> Thanks a lot for the detailed explanations, I did have a look at all >> the variations before posting this. >> >> On 2019-10-11 16:20, Mark Rutland wrote: >> > Hi, >> > >> > On Fri, Oct 11, 2019 at 11:19:00AM +0530, Sai Prakash Ranjan wrote: >> >> On latest QCOM SoCs like SM8150 and SC7180 with big.LITTLE arch, below >> >> warnings are observed during bootup of big cpu cores. >> > >> > For reference, which CPUs are in those SoCs? >> > >> >> SM8150 is based on Cortex-A55(little cores) and Cortex-A76(big cores). >> I'm afraid I cannot give details about SC7180 yet. >> >> >> SM8150: >> >> >> [ 0.271177] CPU features: SANITY CHECK: Unexpected variation in >> >> SYS_ID_AA64PFR0_EL1. Boot CPU: 0x00000011112222, CPU4: >> 0x00000011111112 >> > >> > The differing fields are EL3, EL2, and EL1: the boot CPU supports >> > AArch64 and AArch32 at those exception levels, while the secondary only >> > supports AArch64. >> > >> > Do we handle this variation in KVM? >> >> We do not support KVM. > > Mainline does. You don't get to pick and choose what is supported or > not. > Ok thats good. Thanks, Sai
On Fri, Oct 11, 2019 at 02:33:43PM +0100, Marc Zyngier wrote: > On Fri, 11 Oct 2019 11:50:11 +0100 > Mark Rutland <mark.rutland@arm.com> wrote: > > > Hi, > > > > On Fri, Oct 11, 2019 at 11:19:00AM +0530, Sai Prakash Ranjan wrote: > > > On latest QCOM SoCs like SM8150 and SC7180 with big.LITTLE arch, below > > > warnings are observed during bootup of big cpu cores. > > > > For reference, which CPUs are in those SoCs? > > > > > SM8150: > > > > > > [ 0.271177] CPU features: SANITY CHECK: Unexpected variation in > > > SYS_ID_AA64PFR0_EL1. Boot CPU: 0x00000011112222, CPU4: 0x00000011111112 > > > > The differing fields are EL3, EL2, and EL1: the boot CPU supports > > AArch64 and AArch32 at those exception levels, while the secondary only > > supports AArch64. > > > > Do we handle this variation in KVM? > > We do, at least at vcpu creation time (see kvm_reset_vcpu). But if one > of the !AArch32 CPU comes in late in the game (after we've started a > guest), all bets are off (we'll schedule the 32bit guest on that CPU, > enter the guest, immediately take an Illegal Exception Return, and > return to userspace with KVM_EXIT_FAIL_ENTRY). Ouch. We certainly can't remove the warning untill we deal with that somehow, then. > Not sure we could do better, given the HW. My preference would be to > fail these CPUs if they aren't present at boot time. I agree; I think we need logic to check the ID register fields against their EXACT, {LOWER,HIGHER}_SAFE, etc rules regardless of whether we have an associated cap. That can then abort a late onlining of a CPU which violates those rules w.r.t. the finalised system value. I suspect that we may want to split the notion of safe-for-{user,kernel-guest} in the feature tables, as if nothing else it will force us to consider those cases separately when adding new stuff. Thanks, Mark.
On Fri, 11 Oct 2019 14:54:31 +0100 Mark Rutland <mark.rutland@arm.com> wrote: > On Fri, Oct 11, 2019 at 02:33:43PM +0100, Marc Zyngier wrote: > > On Fri, 11 Oct 2019 11:50:11 +0100 > > Mark Rutland <mark.rutland@arm.com> wrote: > > > > > Hi, > > > > > > On Fri, Oct 11, 2019 at 11:19:00AM +0530, Sai Prakash Ranjan wrote: > > > > On latest QCOM SoCs like SM8150 and SC7180 with big.LITTLE arch, below > > > > warnings are observed during bootup of big cpu cores. > > > > > > For reference, which CPUs are in those SoCs? > > > > > > > SM8150: > > > > > > > > [ 0.271177] CPU features: SANITY CHECK: Unexpected variation in > > > > SYS_ID_AA64PFR0_EL1. Boot CPU: 0x00000011112222, CPU4: 0x00000011111112 > > > > > > The differing fields are EL3, EL2, and EL1: the boot CPU supports > > > AArch64 and AArch32 at those exception levels, while the secondary only > > > supports AArch64. > > > > > > Do we handle this variation in KVM? > > > > We do, at least at vcpu creation time (see kvm_reset_vcpu). But if one > > of the !AArch32 CPU comes in late in the game (after we've started a > > guest), all bets are off (we'll schedule the 32bit guest on that CPU, > > enter the guest, immediately take an Illegal Exception Return, and > > return to userspace with KVM_EXIT_FAIL_ENTRY). > > Ouch. We certainly can't remove the warning untill we deal with that > somehow, then. Indeed. Same thing applies for hot-removing the AArch32-capable CPUs, by the way. You'd end-up in a situation where guests can't run, despite the initial contract that we're happy that configuration. > > Not sure we could do better, given the HW. My preference would be to > > fail these CPUs if they aren't present at boot time. > > I agree; I think we need logic to check the ID register fields against > their EXACT, {LOWER,HIGHER}_SAFE, etc rules regardless of whether we > have an associated cap. That can then abort a late onlining of a CPU > which violates those rules w.r.t. the finalised system value. > > I suspect that we may want to split the notion of > safe-for-{user,kernel-guest} in the feature tables, as if nothing else > it will force us to consider those cases separately when adding new > stuff. Probably. There are bizarre overlaps, in the sense that some capabilities (such as this AArch32 EL1 support) are firmly kernel related, and yet have a direct impact on userspace. KVM blurs the lines in "interesting" ways... :-(. Thanks, M.
Quoting Sai Prakash Ranjan (2019-10-11 06:40:13) > On 2019-10-11 19:04, Marc Zyngier wrote: > > On Fri, 11 Oct 2019 18:47:39 +0530 > > Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org> wrote: > > > >> Hi Mark, > >> > >> Thanks a lot for the detailed explanations, I did have a look at all > >> the variations before posting this. > >> > >> On 2019-10-11 16:20, Mark Rutland wrote: > >> > Hi, > >> > > >> > On Fri, Oct 11, 2019 at 11:19:00AM +0530, Sai Prakash Ranjan wrote: > >> >> On latest QCOM SoCs like SM8150 and SC7180 with big.LITTLE arch, below > >> >> warnings are observed during bootup of big cpu cores. > >> > > >> > For reference, which CPUs are in those SoCs? > >> > > >> > >> SM8150 is based on Cortex-A55(little cores) and Cortex-A76(big cores). > >> I'm afraid I cannot give details about SC7180 yet. > >> > >> >> SM8150: > >> >> >> [ 0.271177] CPU features: SANITY CHECK: Unexpected variation in > >> >> SYS_ID_AA64PFR0_EL1. Boot CPU: 0x00000011112222, CPU4: >> 0x00000011111112 > >> > > >> > The differing fields are EL3, EL2, and EL1: the boot CPU supports > >> > AArch64 and AArch32 at those exception levels, while the secondary only > >> > supports AArch64. > >> > > >> > Do we handle this variation in KVM? > >> > >> We do not support KVM. > > > > Mainline does. You don't get to pick and choose what is supported or > > not. > > > > Ok thats good. > I want KVM on sc7180. How do I get it? Is something going to not work?
Hi, On 10/11/19 8:54 AM, Mark Rutland wrote: > On Fri, Oct 11, 2019 at 02:33:43PM +0100, Marc Zyngier wrote: >> On Fri, 11 Oct 2019 11:50:11 +0100 >> Mark Rutland <mark.rutland@arm.com> wrote: >> >>> Hi, >>> >>> On Fri, Oct 11, 2019 at 11:19:00AM +0530, Sai Prakash Ranjan wrote: >>>> On latest QCOM SoCs like SM8150 and SC7180 with big.LITTLE arch, below >>>> warnings are observed during bootup of big cpu cores. >>> >>> For reference, which CPUs are in those SoCs? >>> >>>> SM8150: >>>> >>>> [ 0.271177] CPU features: SANITY CHECK: Unexpected variation in >>>> SYS_ID_AA64PFR0_EL1. Boot CPU: 0x00000011112222, CPU4: 0x00000011111112 >>> >>> The differing fields are EL3, EL2, and EL1: the boot CPU supports >>> AArch64 and AArch32 at those exception levels, while the secondary only >>> supports AArch64. >>> >>> Do we handle this variation in KVM? >> >> We do, at least at vcpu creation time (see kvm_reset_vcpu). But if one >> of the !AArch32 CPU comes in late in the game (after we've started a >> guest), all bets are off (we'll schedule the 32bit guest on that CPU, >> enter the guest, immediately take an Illegal Exception Return, and >> return to userspace with KVM_EXIT_FAIL_ENTRY). > > Ouch. We certainly can't remove the warning untill we deal with that > somehow, then. > >> Not sure we could do better, given the HW. My preference would be to >> fail these CPUs if they aren't present at boot time. > > I agree; I think we need logic to check the ID register fields against > their EXACT, {LOWER,HIGHER}_SAFE, etc rules regardless of whether we > have an associated cap. That can then abort a late onlining of a CPU > which violates those rules w.r.t. the finalised system value. Except one of the cases is the user who doesn't care about aarch32 @ el2/1 and just wants to add another core to their 64-bit "clean" OS. So my $.02 is the online should only fail if someone has actually started a 32-bit guest on the machine. > > I suspect that we may want to split the notion of > safe-for-{user,kernel-guest} in the feature tables, as if nothing else > it will force us to consider those cases separately when adding new > stuff. As i'm sure everyone knows, this is all going to happen again with el0 support. I wonder if some of this more "advanced" functionality should be buried behind EXPERT. At least on ACPI its possible to tell at early boot if the machine is heterogeneous (not necessarily in which ways) and just automatically sanitize away 32-bit support and some of the stickier things when a heterogeneous machine is detected.
On 2019-10-17 21:00, Stephen Boyd wrote: > Quoting Sai Prakash Ranjan (2019-10-11 06:40:13) >> On 2019-10-11 19:04, Marc Zyngier wrote: >> > On Fri, 11 Oct 2019 18:47:39 +0530 >> > Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org> wrote: >> > >> >> Hi Mark, >> >> >> >> Thanks a lot for the detailed explanations, I did have a look at >> all >> >> the variations before posting this. >> >> >> >> On 2019-10-11 16:20, Mark Rutland wrote: >> >> > Hi, >> >> > >> >> > On Fri, Oct 11, 2019 at 11:19:00AM +0530, Sai Prakash Ranjan >> wrote: >> >> >> On latest QCOM SoCs like SM8150 and SC7180 with big.LITTLE >> arch, below >> >> >> warnings are observed during bootup of big cpu cores. >> >> > >> >> > For reference, which CPUs are in those SoCs? >> >> > >> >> >> >> SM8150 is based on Cortex-A55(little cores) and Cortex-A76(big >> cores). >> >> I'm afraid I cannot give details about SC7180 yet. >> >> >> >> >> SM8150: >> >> >> >> [ 0.271177] CPU features: SANITY CHECK: Unexpected >> variation in >> >> >> SYS_ID_AA64PFR0_EL1. Boot CPU: 0x00000011112222, CPU4: >> >> 0x00000011111112 >> >> > >> >> > The differing fields are EL3, EL2, and EL1: the boot CPU >> supports >> >> > AArch64 and AArch32 at those exception levels, while the >> secondary only >> >> > supports AArch64. >> >> > >> >> > Do we handle this variation in KVM? >> >> >> >> We do not support KVM. >> > >> > Mainline does. You don't get to pick and choose what is supported >> or >> > not. >> > >> >> Ok thats good. >> > > I want KVM on sc7180. How do I get it? Is something going to not > work? If this SoC is anythinig like SM8150, 32bit guests will be hit and miss, depending on the CPU your guest runs on, or is migrated to. We need to either drop capabilities from the 32bit-capable CPU, or prevent the non-32bit capable CPU from booting if a 32bit guest has been started. You just have to hope that the kernel is entered at EL2, and that QC's "value add" has been moved somewhere else... M.
On Thu, Oct 17, 2019 at 04:39:23PM -0500, Jeremy Linton wrote: > On 10/11/19 8:54 AM, Mark Rutland wrote: > > On Fri, Oct 11, 2019 at 02:33:43PM +0100, Marc Zyngier wrote: > > > On Fri, 11 Oct 2019 11:50:11 +0100 > > > Mark Rutland <mark.rutland@arm.com> wrote: > > > > On Fri, Oct 11, 2019 at 11:19:00AM +0530, Sai Prakash Ranjan wrote: > > > > > On latest QCOM SoCs like SM8150 and SC7180 with big.LITTLE arch, below > > > > > warnings are observed during bootup of big cpu cores. > > > > > > > > For reference, which CPUs are in those SoCs? > > > > > > > > > SM8150: > > > > > > > > > > [ 0.271177] CPU features: SANITY CHECK: Unexpected variation in > > > > > SYS_ID_AA64PFR0_EL1. Boot CPU: 0x00000011112222, CPU4: 0x00000011111112 > > > > > > > > The differing fields are EL3, EL2, and EL1: the boot CPU supports > > > > AArch64 and AArch32 at those exception levels, while the secondary only > > > > supports AArch64. > > > > > > > > Do we handle this variation in KVM? > > > > > > We do, at least at vcpu creation time (see kvm_reset_vcpu). But if one > > > of the !AArch32 CPU comes in late in the game (after we've started a > > > guest), all bets are off (we'll schedule the 32bit guest on that CPU, > > > enter the guest, immediately take an Illegal Exception Return, and > > > return to userspace with KVM_EXIT_FAIL_ENTRY). > > > > Ouch. We certainly can't remove the warning untill we deal with that > > somehow, then. Luckily, qemu refuses to start a guest on two different CPU types. > > > Not sure we could do better, given the HW. My preference would be to > > > fail these CPUs if they aren't present at boot time. That's my preference as well. > > I agree; I think we need logic to check the ID register fields against > > their EXACT, {LOWER,HIGHER}_SAFE, etc rules regardless of whether we > > have an associated cap. That can then abort a late onlining of a CPU > > which violates those rules w.r.t. the finalised system value. > > Except one of the cases is the user who doesn't care about aarch32 @ el2/1 > and just wants to add another core to their 64-bit "clean" OS. > > So my $.02 is the online should only fail if someone has actually started a > 32-bit guest on the machine. I don't really think it's worth the hassle. This could even be racy (32-bit guest starting at the same time with a CPU being onlined), so it needs extra care. If you have such platform, just make sure that you don't have incompatible CPUs coming up late (during boot it should be fine). > > I suspect that we may want to split the notion of > > safe-for-{user,kernel-guest} in the feature tables, as if nothing else > > it will force us to consider those cases separately when adding new > > stuff. > > As i'm sure everyone knows, this is all going to happen again with el0 > support. I wonder if some of this more "advanced" functionality should be > buried behind EXPERT. At least on ACPI its possible to tell at early boot if > the machine is heterogeneous (not necessarily in which ways) and just > automatically sanitize away 32-bit support and some of the stickier things > when a heterogeneous machine is detected. We should improve (remove) the warnings for things we know the kernel can handled during boot. For example, 32-bit not available on all CPUs during boot should be fine as we just disable the feature. However, late onlining of a CPU that does not support the already advertised features should be blocked.
On 2019-10-18 01:30, Stephen Boyd wrote: > Quoting Sai Prakash Ranjan (2019-10-11 06:40:13) >> On 2019-10-11 19:04, Marc Zyngier wrote: >> > On Fri, 11 Oct 2019 18:47:39 +0530 >> > Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org> wrote: >> > >> >> Hi Mark, >> >> >> >> Thanks a lot for the detailed explanations, I did have a look at all >> >> the variations before posting this. >> >> >> >> On 2019-10-11 16:20, Mark Rutland wrote: >> >> > Hi, >> >> > >> >> > On Fri, Oct 11, 2019 at 11:19:00AM +0530, Sai Prakash Ranjan wrote: >> >> >> On latest QCOM SoCs like SM8150 and SC7180 with big.LITTLE arch, below >> >> >> warnings are observed during bootup of big cpu cores. >> >> > >> >> > For reference, which CPUs are in those SoCs? >> >> > >> >> >> >> SM8150 is based on Cortex-A55(little cores) and Cortex-A76(big cores). >> >> I'm afraid I cannot give details about SC7180 yet. >> >> >> >> >> SM8150: >> >> >> >> [ 0.271177] CPU features: SANITY CHECK: Unexpected variation in >> >> >> SYS_ID_AA64PFR0_EL1. Boot CPU: 0x00000011112222, CPU4: >> 0x00000011111112 >> >> > >> >> > The differing fields are EL3, EL2, and EL1: the boot CPU supports >> >> > AArch64 and AArch32 at those exception levels, while the secondary only >> >> > supports AArch64. >> >> > >> >> > Do we handle this variation in KVM? >> >> >> >> We do not support KVM. >> > >> > Mainline does. You don't get to pick and choose what is supported or >> > not. >> > >> >> Ok thats good. >> > > I want KVM on sc7180. How do I get it? Is something going to not work? I meant KVM is not supported for downstream android case where we do not have kernel booting from EL2. And obviously I am wrong because SC7180 is not for android, so my bad. I think Mark R's question about handling KVM variation was for Marc Z not me :p As for something not going to work, as Mark said this warning does indicate that 32 bit EL1 guests won't be able to run on big CPU cores. - Sai
Quoting Marc Zyngier (2019-10-18 00:20:56) > > If this SoC is anythinig like SM8150, 32bit guests will be hit and > miss, > depending on the CPU your guest runs on, or is migrated to. We need to > either drop capabilities from the 32bit-capable CPU, or prevent the > non-32bit capable CPU from booting if a 32bit guest has been started. > > You just have to hope that the kernel is entered at EL2, and that QC's > "value add" has been moved somewhere else... > Ok that's good.
On Fri, 18 Oct 2019 15:33:29 +0100, Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Marc Zyngier (2019-10-18 00:20:56) > > > > If this SoC is anythinig like SM8150, 32bit guests will be hit and > > miss, > > depending on the CPU your guest runs on, or is migrated to. We need to > > either drop capabilities from the 32bit-capable CPU, or prevent the > > non-32bit capable CPU from booting if a 32bit guest has been started. > > > > You just have to hope that the kernel is entered at EL2, and that QC's > > "value add" has been moved somewhere else... > > > > Ok that's good. I need a new signature. M.
Hi Mark, On 2019-10-11 19:24, Mark Rutland wrote: > On Fri, Oct 11, 2019 at 02:33:43PM +0100, Marc Zyngier wrote: >> On Fri, 11 Oct 2019 11:50:11 +0100 >> Mark Rutland <mark.rutland@arm.com> wrote: >> >> > Hi, >> > >> > On Fri, Oct 11, 2019 at 11:19:00AM +0530, Sai Prakash Ranjan wrote: >> > > On latest QCOM SoCs like SM8150 and SC7180 with big.LITTLE arch, below >> > > warnings are observed during bootup of big cpu cores. >> > >> > For reference, which CPUs are in those SoCs? >> > >> > > SM8150: >> > > >> > > [ 0.271177] CPU features: SANITY CHECK: Unexpected variation in >> > > SYS_ID_AA64PFR0_EL1. Boot CPU: 0x00000011112222, CPU4: 0x00000011111112 >> > >> > The differing fields are EL3, EL2, and EL1: the boot CPU supports >> > AArch64 and AArch32 at those exception levels, while the secondary only >> > supports AArch64. >> > >> > Do we handle this variation in KVM? >> >> We do, at least at vcpu creation time (see kvm_reset_vcpu). But if one >> of the !AArch32 CPU comes in late in the game (after we've started a >> guest), all bets are off (we'll schedule the 32bit guest on that CPU, >> enter the guest, immediately take an Illegal Exception Return, and >> return to userspace with KVM_EXIT_FAIL_ENTRY). > > Ouch. We certainly can't remove the warning untill we deal with that > somehow, then. > >> Not sure we could do better, given the HW. My preference would be to >> fail these CPUs if they aren't present at boot time. > > I agree; I think we need logic to check the ID register fields against > their EXACT, {LOWER,HIGHER}_SAFE, etc rules regardless of whether we > have an associated cap. That can then abort a late onlining of a CPU > which violates those rules w.r.t. the finalised system value. > > I suspect that we may want to split the notion of > safe-for-{user,kernel-guest} in the feature tables, as if nothing else > it will force us to consider those cases separately when adding new > stuff. > I can help with testing these if you have any sample patches. Thanks, Sai
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index cabebf1a7976..207197692caa 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -164,8 +164,8 @@ static const struct arm64_ftr_bits ftr_id_aa64pfr0[] = { S_ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64PFR0_FP_SHIFT, 4, ID_AA64PFR0_FP_NI), /* Linux doesn't care about the EL3 */ ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL3_SHIFT, 4, 0), - ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL2_SHIFT, 4, 0), - ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL1_SHIFT, 4, ID_AA64PFR0_EL1_64BIT_ONLY),