| Message ID | bf909c1f4904a22be0804cae9fd6f38ba4862563.1596768588.git.haibo.xu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Add vSPE support to KVM guest | expand |
On Fri, Aug 07, 2020 at 08:10:37AM +0000, Haibo Xu wrote: > Turn on the spe cpu property by default when working with host > cpu type in KVM mode, i.e. we can now do '-cpu host' to add the > vSPE, and '-cpu host,spe=off' to remove it. -cpu max with KVM should also enable it by default > > Signed-off-by: Haibo Xu <haibo.xu@linaro.org> > --- > target/arm/cpu.c | 4 ++++ > target/arm/kvm64.c | 9 +++++++++ > 2 files changed, 13 insertions(+) > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index 67ab0089fd..42fa99953c 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -1719,6 +1719,10 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) > cpu->pmceid1 = 0; > } > > + if (!cpu->has_spe || !kvm_enabled()) { > + unset_feature(env, ARM_FEATURE_SPE); > + } I don't think this should be necessary. > + > if (!arm_feature(env, ARM_FEATURE_EL2)) { > /* Disable the hypervisor feature bits in the processor feature > * registers if we don't have EL2. These are id_pfr1[15:12] and > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c > index be045ccc5f..4ea58afc1d 100644 > --- a/target/arm/kvm64.c > +++ b/target/arm/kvm64.c > @@ -679,6 +679,7 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) > features |= 1ULL << ARM_FEATURE_AARCH64; > features |= 1ULL << ARM_FEATURE_PMU; > features |= 1ULL << ARM_FEATURE_GENERIC_TIMER; > + features |= 1ULL << ARM_FEATURE_SPE; No, SPE is not a feature we assume is present in v8.0 CPUs. > > ahcf->features = features; > > @@ -826,6 +827,14 @@ int kvm_arch_init_vcpu(CPUState *cs) > } else { > env->features &= ~(1ULL << ARM_FEATURE_PMU); > } > + if (!kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_SPE_V1)) { > + cpu->has_spe = false; > + } > + if (cpu->has_spe) { > + cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SPE_V1; > + } else { > + env->features &= ~(1ULL << ARM_FEATURE_SPE); > + } The PMU code above this isn't a good pattern to copy. The SVE code below is better. SVE uses an ID bit and doesn't do the redundant KVM cap check. It'd be nice to cleanup the PMU code (with a separate patch) and then add SPE in a better way. > if (cpu_isar_feature(aa64_sve, cpu)) { > assert(kvm_arm_sve_supported()); > cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SVE; > -- > 2.17.1 > Thanks, drew
On Mon, 10 Aug 2020 at 19:16, Andrew Jones <drjones@redhat.com> wrote: > > On Fri, Aug 07, 2020 at 08:10:37AM +0000, Haibo Xu wrote: > > Turn on the spe cpu property by default when working with host > > cpu type in KVM mode, i.e. we can now do '-cpu host' to add the > > vSPE, and '-cpu host,spe=off' to remove it. > > -cpu max with KVM should also enable it by default > Ok, will fix it! > > > > Signed-off-by: Haibo Xu <haibo.xu@linaro.org> > > --- > > target/arm/cpu.c | 4 ++++ > > target/arm/kvm64.c | 9 +++++++++ > > 2 files changed, 13 insertions(+) > > > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > > index 67ab0089fd..42fa99953c 100644 > > --- a/target/arm/cpu.c > > +++ b/target/arm/cpu.c > > @@ -1719,6 +1719,10 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) > > cpu->pmceid1 = 0; > > } > > > > + if (!cpu->has_spe || !kvm_enabled()) { > > + unset_feature(env, ARM_FEATURE_SPE); > > + } > > I don't think this should be necessary. > Yes, I have tried to remove this check, and the vSPE can still work correctly. But I don't know whether there are some corner cases that trigger an error. The similar logic is added in commit 929e754d5a to enable vPMU support. > > + > > if (!arm_feature(env, ARM_FEATURE_EL2)) { > > /* Disable the hypervisor feature bits in the processor feature > > * registers if we don't have EL2. These are id_pfr1[15:12] and > > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c > > index be045ccc5f..4ea58afc1d 100644 > > --- a/target/arm/kvm64.c > > +++ b/target/arm/kvm64.c > > @@ -679,6 +679,7 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) > > features |= 1ULL << ARM_FEATURE_AARCH64; > > features |= 1ULL << ARM_FEATURE_PMU; > > features |= 1ULL << ARM_FEATURE_GENERIC_TIMER; > > + features |= 1ULL << ARM_FEATURE_SPE; > > No, SPE is not a feature we assume is present in v8.0 CPUs. > Yes, SPE is an optional feature for v8.2. How about changing to the following logic: spe_supported = ioctl(fdarray[0], KVM_CHECK_EXTENSION, KVM_CAP_ARM_SPE_V1) > 0; if (spe_supported) { features |= 1ULL << ARM_FEATURE_SPE; } > > > > ahcf->features = features; > > > > @@ -826,6 +827,14 @@ int kvm_arch_init_vcpu(CPUState *cs) > > } else { > > env->features &= ~(1ULL << ARM_FEATURE_PMU); > > } > > + if (!kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_SPE_V1)) { > > + cpu->has_spe = false; > > + } > > + if (cpu->has_spe) { > > + cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SPE_V1; > > + } else { > > + env->features &= ~(1ULL << ARM_FEATURE_SPE); > > + } > > The PMU code above this isn't a good pattern to copy. The SVE code below > is better. SVE uses an ID bit and doesn't do the redundant KVM cap check. > It'd be nice to cleanup the PMU code (with a separate patch) and then add > SPE in a better way. > I noticed that Peter had sent out a mail <https://www.mail-archive.com/qemu-devel@nongnu.org/msg727640.html> to talk about the feature-identification strategy. So shall we adapt it to the vPMU and vSPE feature? > > if (cpu_isar_feature(aa64_sve, cpu)) { > > assert(kvm_arm_sve_supported()); > > cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SVE; > > -- > > 2.17.1 > > > > Thanks, > drew > <div dir="ltr">On Mon, 10 Aug 2020 at 19:16, Andrew Jones <<a href="mailto:drjones@redhat.com">drjones@redhat.com</a>> wrote:<br>><br>> On Fri, Aug 07, 2020 at 08:10:37AM +0000, Haibo Xu wrote:<br>> > Turn on the spe cpu property by default when working with host<br>> > cpu type in KVM mode, i.e. we can now do '-cpu host' to add the<br>> > vSPE, and '-cpu host,spe=off' to remove it.<br>><br>> -cpu max with KVM should also enable it by default<br>><br><br>Ok, will fix it!<br><br>> ><br>> > Signed-off-by: Haibo Xu <<a href="mailto:haibo.xu@linaro.org">haibo.xu@linaro.org</a>><br>> > ---<br>> > target/arm/cpu.c | 4 ++++<br>> > target/arm/kvm64.c | 9 +++++++++<br>> > 2 files changed, 13 insertions(+)<br>> ><br>> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c<br>> > index 67ab0089fd..42fa99953c 100644<br>> > --- a/target/arm/cpu.c<br>> > +++ b/target/arm/cpu.c<br>> > @@ -1719,6 +1719,10 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)<br>> > cpu->pmceid1 = 0;<br>> > } <br>> ><br>> > + if (!cpu->has_spe || !kvm_enabled()) {<br>> > + unset_feature(env, ARM_FEATURE_SPE);<br>> > + }<br>><br>> I don't think this should be necessary.<br>><br><br>Yes, I have tried to remove this check, and the vSPE can still work correctly.<br>But I don't know whether there are some corner cases that trigger an error.<br>The similar logic is added in commit 929e754d5a to enable vPMU support.<br><br>> > +<br>> > if (!arm_feature(env, ARM_FEATURE_EL2)) {<br>> > /* Disable the hypervisor feature bits in the processor feature<br>> > * registers if we don't have EL2. These are id_pfr1[15:12] and<br>> > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c<br>> > index be045ccc5f..4ea58afc1d 100644<br>> > --- a/target/arm/kvm64.c<br>> > +++ b/target/arm/kvm64.c<br>> > @@ -679,6 +679,7 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)<br>> > features |= 1ULL << ARM_FEATURE_AARCH64;<br>> > features |= 1ULL << ARM_FEATURE_PMU;<br>> > features |= 1ULL << ARM_FEATURE_GENERIC_TIMER;<br>> > + features |= 1ULL << ARM_FEATURE_SPE;<br>><br>> No, SPE is not a feature we assume is present in v8.0 CPUs.<br>><br><br>Yes, SPE is an optional feature for v8.2. How about changing to the following logic:<br><br>spe_supported = ioctl(fdarray[0], KVM_CHECK_EXTENSION, KVM_CAP_ARM_SPE_V1) > 0;<br>if (spe_supported) {<br> features |= 1ULL << ARM_FEATURE_SPE;<br>}<br><br>> ><br>> > ahcf->features = features;<br>> ><br>> > @@ -826,6 +827,14 @@ int kvm_arch_init_vcpu(CPUState *cs)<br>> > } else {<br>> > env->features &= ~(1ULL << ARM_FEATURE_PMU);<br>> > }<br>> > + if (!kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_SPE_V1)) {<br>> > + cpu->has_spe = false;<br>> > + }<br>> > + if (cpu->has_spe) {<br>> > + cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SPE_V1;<br>> > + } else {<br>> > + env->features &= ~(1ULL << ARM_FEATURE_SPE);<br>> > + }<br>><br>> The PMU code above this isn't a good pattern to copy. The SVE code below<br>> is better. SVE uses an ID bit and doesn't do the redundant KVM cap check.<br>> It'd be nice to cleanup the PMU code (with a separate patch) and then add<br>> SPE in a better way.<br>><br><br>I noticed that Peter had sent out a <a href="https://www.mail-archive.com/qemu-devel@nongnu.org/msg727640.html">mail</a> to talk about the feature-identification strategy.<div>So shall we adapt it to the vPMU and vSPE feature?<br><br>> > if (cpu_isar_feature(aa64_sve, cpu)) {<br>> > assert(kvm_arm_sve_supported());<br>> > cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SVE;<br>> > --<br>> > 2.17.1<br>> ><br>><br>> Thanks,<br>> drew<br>></div></div>
On Tue, Aug 11, 2020 at 11:15:42AM +0800, Haibo Xu wrote: > > > + if (!cpu->has_spe || !kvm_enabled()) { > > > + unset_feature(env, ARM_FEATURE_SPE); > > > + } > > > > I don't think this should be necessary. > > > > Yes, I have tried to remove this check, and the vSPE can still work > correctly. > But I don't know whether there are some corner cases that trigger an error. > The similar logic is added in commit 929e754d5a to enable vPMU support. I think the PMU logic needs a cleanup, rather than to be imitated. > > > > + > > > if (!arm_feature(env, ARM_FEATURE_EL2)) { > > > /* Disable the hypervisor feature bits in the processor feature > > > * registers if we don't have EL2. These are id_pfr1[15:12] and > > > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c > > > index be045ccc5f..4ea58afc1d 100644 > > > --- a/target/arm/kvm64.c > > > +++ b/target/arm/kvm64.c > > > @@ -679,6 +679,7 @@ bool > kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) > > > features |= 1ULL << ARM_FEATURE_AARCH64; > > > features |= 1ULL << ARM_FEATURE_PMU; > > > features |= 1ULL << ARM_FEATURE_GENERIC_TIMER; > > > + features |= 1ULL << ARM_FEATURE_SPE; > > > > No, SPE is not a feature we assume is present in v8.0 CPUs. > > > > Yes, SPE is an optional feature for v8.2. How about changing to the > following logic: > > spe_supported = ioctl(fdarray[0], KVM_CHECK_EXTENSION, KVM_CAP_ARM_SPE_V1) > > 0; > if (spe_supported) { > features |= 1ULL << ARM_FEATURE_SPE; > } Yes, except you need to drop the ARM_FEATURE_SPE define and use the ID register bit instead like "sve_supported" does. > > > > > > > ahcf->features = features; > > > > > > @@ -826,6 +827,14 @@ int kvm_arch_init_vcpu(CPUState *cs) > > > } else { > > > env->features &= ~(1ULL << ARM_FEATURE_PMU); > > > } > > > + if (!kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_SPE_V1)) { > > > + cpu->has_spe = false; > > > + } > > > + if (cpu->has_spe) { > > > + cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SPE_V1; > > > + } else { > > > + env->features &= ~(1ULL << ARM_FEATURE_SPE); > > > + } > > > > The PMU code above this isn't a good pattern to copy. The SVE code below > > is better. SVE uses an ID bit and doesn't do the redundant KVM cap check. > > It'd be nice to cleanup the PMU code (with a separate patch) and then add > > SPE in a better way. > > > > I noticed that Peter had sent out a mail > <https://www.mail-archive.com/qemu-devel@nongnu.org/msg727640.html> to talk > about the feature-identification strategy. > So shall we adapt it to the vPMU and vSPE feature? At least SPE. You'll have to double check that it makes sense to do for PMU. But, if so, then it should be done with a separate series. Thanks, drew
On Wed, 12 Aug 2020 at 00:50, Andrew Jones <drjones@redhat.com> wrote: > > On Tue, Aug 11, 2020 at 11:15:42AM +0800, Haibo Xu wrote: > > > > + if (!cpu->has_spe || !kvm_enabled()) { > > > > + unset_feature(env, ARM_FEATURE_SPE); > > > > + } > > > > > > I don't think this should be necessary. > > > > > > > Yes, I have tried to remove this check, and the vSPE can still work > > correctly. > > But I don't know whether there are some corner cases that trigger an error. > > The similar logic is added in commit 929e754d5a to enable vPMU support. > > I think the PMU logic needs a cleanup, rather than to be imitated. > > > > > > > + > > > > if (!arm_feature(env, ARM_FEATURE_EL2)) { > > > > /* Disable the hypervisor feature bits in the processor feature > > > > * registers if we don't have EL2. These are id_pfr1[15:12] and > > > > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c > > > > index be045ccc5f..4ea58afc1d 100644 > > > > --- a/target/arm/kvm64.c > > > > +++ b/target/arm/kvm64.c > > > > @@ -679,6 +679,7 @@ bool > > kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) > > > > features |= 1ULL << ARM_FEATURE_AARCH64; > > > > features |= 1ULL << ARM_FEATURE_PMU; > > > > features |= 1ULL << ARM_FEATURE_GENERIC_TIMER; > > > > + features |= 1ULL << ARM_FEATURE_SPE; > > > > > > No, SPE is not a feature we assume is present in v8.0 CPUs. > > > > > > > Yes, SPE is an optional feature for v8.2. How about changing to the > > following logic: > > > > spe_supported = ioctl(fdarray[0], KVM_CHECK_EXTENSION, KVM_CAP_ARM_SPE_V1) > > > 0; > > if (spe_supported) { > > features |= 1ULL << ARM_FEATURE_SPE; > > } > > Yes, except you need to drop the ARM_FEATURE_SPE define and use the ID > register bit instead like "sve_supported" does. > > > > > > > > > > > ahcf->features = features; > > > > > > > > @@ -826,6 +827,14 @@ int kvm_arch_init_vcpu(CPUState *cs) > > > > } else { > > > > env->features &= ~(1ULL << ARM_FEATURE_PMU); > > > > } > > > > + if (!kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_SPE_V1)) { > > > > + cpu->has_spe = false; > > > > + } > > > > + if (cpu->has_spe) { > > > > + cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SPE_V1; > > > > + } else { > > > > + env->features &= ~(1ULL << ARM_FEATURE_SPE); > > > > + } > > > > > > The PMU code above this isn't a good pattern to copy. The SVE code below > > > is better. SVE uses an ID bit and doesn't do the redundant KVM cap check. > > > It'd be nice to cleanup the PMU code (with a separate patch) and then add > > > SPE in a better way. > > > > > > > I noticed that Peter had sent out a mail > > <https://www.mail-archive.com/qemu-devel@nongnu.org/msg727640.html> to talk > > about the feature-identification strategy. > > So shall we adapt it to the vPMU and vSPE feature? > > At least SPE. You'll have to double check that it makes sense to do for > PMU. But, if so, then it should be done with a separate series. > Ok, will adapt the SPE support to this new feature-identification strategy first, and investigate whether it makes sense to do so for PMU later. Thank you very much for helping review the patch series! > Thanks, > drew >
On 8/11/20 9:49 AM, Andrew Jones wrote: > Yes, except you need to drop the ARM_FEATURE_SPE define and use the ID > register bit instead like "sve_supported" does. On a related note, I think we have a latent bug, or at least a mis-feature: sve_supported = ioctl(fdarray[0], KVM_CHECK_EXTENSION, KVM_CAP_ARM_SVE) > 0; ... /* Add feature bits that can't appear until after VCPU init. */ if (sve_supported) { t = ahcf->isar.id_aa64pfr0; t = FIELD_DP64(t, ID_AA64PFR0, SVE, 1); ahcf->isar.id_aa64pfr0 = t; } Should it in fact be if (!sve_supported) { t = ahcf->isar.id_aa64pfr0; t = FIELD_DP64(t, ID_AA64PFR0, SVE, 0); ahcf->isar.id_aa64pfr0 = t; } ? Forcing the value to 1 here is going to be wrong the moment we have an SVE2 enabled cpu. Similarly, SPE has more than one "enabled" value for PMSVer. r~
On Fri, Aug 14, 2020 at 12:28:25PM -0700, Richard Henderson wrote: > On 8/11/20 9:49 AM, Andrew Jones wrote: > > Yes, except you need to drop the ARM_FEATURE_SPE define and use the ID > > register bit instead like "sve_supported" does. > > On a related note, I think we have a latent bug, or at least a mis-feature: > > sve_supported = ioctl(fdarray[0], KVM_CHECK_EXTENSION, KVM_CAP_ARM_SVE) > 0; > ... > /* Add feature bits that can't appear until after VCPU init. */ > if (sve_supported) { > t = ahcf->isar.id_aa64pfr0; > t = FIELD_DP64(t, ID_AA64PFR0, SVE, 1); > ahcf->isar.id_aa64pfr0 = t; > } > > > Should it in fact be > > if (!sve_supported) { > t = ahcf->isar.id_aa64pfr0; > t = FIELD_DP64(t, ID_AA64PFR0, SVE, 0); > ahcf->isar.id_aa64pfr0 = t; > } > > ? > > Forcing the value to 1 here is going to be wrong the moment we have an SVE2 > enabled cpu. Indeed. I think KVM should add a KVM_CAP_ARM_SVE2 cap. Then, we keep the code similar to the way it is now, but set ID_AA64PFR0 appropriately based on which CAPs are present. I think we probably need that CAP anyway in order to ensure a guest that is using SVE2 cannot be migrated to a host that only has SVE. > > Similarly, SPE has more than one "enabled" value for PMSVer. > I'll take a look at the KVM series for SPE next week to see if the UAPI will allow us to determine what values are possible. Thanks for the heads up about this. drew
diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 67ab0089fd..42fa99953c 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -1719,6 +1719,10 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) cpu->pmceid1 = 0; } + if (!cpu->has_spe || !kvm_enabled()) { + unset_feature(env, ARM_FEATURE_SPE); + } + if (!arm_feature(env, ARM_FEATURE_EL2)) { /* Disable the hypervisor feature bits in the processor feature * registers if we don't have EL2. These are id_pfr1[15:12] and diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c index be045ccc5f..4ea58afc1d 100644 --- a/target/arm/kvm64.c +++ b/target/arm/kvm64.c @@ -679,6 +679,7 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) features |= 1ULL << ARM_FEATURE_AARCH64; features |= 1ULL << ARM_FEATURE_PMU; features |= 1ULL << ARM_FEATURE_GENERIC_TIMER; + features |= 1ULL << ARM_FEATURE_SPE; ahcf->features = features; @@ -826,6 +827,14 @@ int kvm_arch_init_vcpu(CPUState *cs) } else { env->features &= ~(1ULL << ARM_FEATURE_PMU); } + if (!kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_SPE_V1)) { + cpu->has_spe = false; + } + if (cpu->has_spe) { + cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SPE_V1; + } else { + env->features &= ~(1ULL << ARM_FEATURE_SPE); + } if (cpu_isar_feature(aa64_sve, cpu)) { assert(kvm_arm_sve_supported()); cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SVE;
Turn on the spe cpu property by default when working with host cpu type in KVM mode, i.e. we can now do '-cpu host' to add the vSPE, and '-cpu host,spe=off' to remove it. Signed-off-by: Haibo Xu <haibo.xu@linaro.org> --- target/arm/cpu.c | 4 ++++ target/arm/kvm64.c | 9 +++++++++ 2 files changed, 13 insertions(+) -- 2.17.1