| Message ID | 20210422161130.652779-5-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i386: KVM: expand Hyper-V features early | expand |
On Thu, Apr 22, 2021 at 06:11:15PM +0200, Vitaly Kuznetsov wrote: > As a preparatory patch to dropping Hyper-V CPUID leaves from > feature_word_info[] stop using env->features[] as a temporary > storage of Hyper-V CPUIDs, just build Hyper-V CPUID leaves directly > from kvm_hyperv_properties[] data. > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > --- > target/i386/cpu.h | 1 + > target/i386/kvm/kvm.c | 80 +++++++++++++++++++++++-------------------- > 2 files changed, 43 insertions(+), 38 deletions(-) > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > index 570f916878f9..c8295aa2a1e7 100644 > --- a/target/i386/cpu.h > +++ b/target/i386/cpu.h > @@ -1684,6 +1684,7 @@ struct X86CPU { > uint32_t hyperv_interface_id[4]; > uint32_t hyperv_version_id[4]; > uint32_t hyperv_limits[3]; > + uint32_t hyperv_nested[4]; > > bool check_cpuid; > bool enforce_cpuid; > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c > index 7c751185491f..f791baa29acf 100644 > --- a/target/i386/kvm/kvm.c > +++ b/target/i386/kvm/kvm.c > @@ -1111,7 +1111,6 @@ static int hv_cpuid_check_and_set(CPUState *cs, struct kvm_cpuid2 *cpuid, > int feature) > { > X86CPU *cpu = X86_CPU(cs); > - CPUX86State *env = &cpu->env; > uint32_t r, fw, bits; > uint64_t deps; > int i, dep_feat; > @@ -1151,8 +1150,6 @@ static int hv_cpuid_check_and_set(CPUState *cs, struct kvm_cpuid2 *cpuid, > return 0; > } > } > - > - env->features[fw] |= bits; > } > > if (cpu->hyperv_passthrough) { > @@ -1162,6 +1159,29 @@ static int hv_cpuid_check_and_set(CPUState *cs, struct kvm_cpuid2 *cpuid, > return 0; > } > > +static uint32_t hv_build_cpuid_leaf(CPUState *cs, uint32_t fw) > +{ > + X86CPU *cpu = X86_CPU(cs); > + uint32_t r = 0; > + int i, j; > + > + for (i = 0; i < ARRAY_SIZE(kvm_hyperv_properties); i++) { > + if (!hyperv_feat_enabled(cpu, i)) { > + continue; > + } > + > + for (j = 0; j < ARRAY_SIZE(kvm_hyperv_properties[i].flags); j++) { > + if (kvm_hyperv_properties[i].flags[j].fw != fw) { > + continue; > + } > + > + r |= kvm_hyperv_properties[i].flags[j].bits; > + } > + } > + > + return r; > +} > + > /* > * Fill in Hyper-V CPUIDs. Returns the number of entries filled in cpuid_ent in > * case of success, errno < 0 in case of failure and 0 when no Hyper-V > @@ -1171,9 +1191,8 @@ static int hyperv_handle_properties(CPUState *cs, > struct kvm_cpuid_entry2 *cpuid_ent) > { > X86CPU *cpu = X86_CPU(cs); > - CPUX86State *env = &cpu->env; > struct kvm_cpuid2 *cpuid; > - struct kvm_cpuid_entry2 *c; > + struct kvm_cpuid_entry2 *c, *c2; > uint32_t cpuid_i = 0; > int r; > > @@ -1194,9 +1213,7 @@ static int hyperv_handle_properties(CPUState *cs, > } > > if (!r) { I think I now I understand why removing FEAT_HYPERV_* makes sense: The rules mapping hyperv features to CPUID bits were encoded twice in the code: in both hyperv_handle_properties() and kvm_hyperv_properties[]. More work to maintain the rules, and too easy to accidentally make them inconsistent. Now, let me see if I can prove to myself that the new code works: > - env->features[FEAT_HV_RECOMM_EAX] |= > - HV_ENLIGHTENED_VMCS_RECOMMENDED; [Line1] The only code reading env->features[FEAT_HV_RECOMM_EAX] seems to be [Line2] below: eax = env->features[FEAT_HV_RECOMM_EAX]; which is replaced with [Line3]: c->eax = hv_build_cpuid_leaf(cs, FEAT_HV_RECOMM_EAX); so if hv_build_cpuid_leaf() do its job and set return HV_ENLIGHTENED_VMCS_RECOMMENDED set at [Line2], we can safely delete [Line1]. Will hv_build_cpuid_leaf() set HV_ENLIGHTENED_VMCS_RECOMMENDED in [Line2] for all cases where [Line1] was being executed? Let's check what's necessary to make hv_build_cpuid_leaf() set HV_ENLIGHTENED_VMCS_RECOMMENDED: There's only one entry with FEAT_HV_RECOMM_EAX/HV_ENLIGHTENED_VMCS_RECOMMENDED at kvm_hyperv_properties: [HYPERV_FEAT_EVMCS] = { .desc = "enlightened VMCS (hv-evmcs)", .flags = { {.fw = FEAT_HV_RECOMM_EAX, .bits = HV_ENLIGHTENED_VMCS_RECOMMENDED} }, .dependencies = BIT(HYPERV_FEAT_VAPIC) }, The logic at hv_build_cpuid_leaf() will make HV_ENLIGHTENED_VMCS_RECOMMENDED be set only if hyperv_feat_enabled(HYPERV_FEAT_EVMCS) is true. Which is what I expected, because the line of code you are removing is inside a hyperv_feat_enabled(cpu, HYPERV_FEAT_EVMCS) conditional. For reference, hyperv_feat_enabled(cpu, feat) returns: !!(cpu->hyperv_features & BIT(feat)) I don't see any code _clearing_ hyperv_features, except for properties that could change hyperv_features. I don't expect the "hv-evmcs" QOM property to be touched by this function, so we should be safe: if hyperv_feat_enabled(cpu, HYPERV_FEAT_EVMCS) returned before executing [Line1], it will return true when executing [Line2]. This means hv_build_cpuid_leaf() will set HV_ENLIGHTENED_VMCS_RECOMMENDED if hyperv_feat_enabled(HYPERV_FEAT_EVMCS) is true, and hyperv_feat_enabled(HYPERV_FEAT_EVMCS) will be true at [Line2] on all cases when [Line1] was being executed. We also need to be sure the HV_ENLIGHTENED_VMCS_RECOMMENDED will be _unset_ at hv_build_cpuid_leaf() when expected, but the rules are trickier (due to hyperv_passthrough). I'll try to prove that later. > - env->features[FEAT_HV_NESTED_EAX] = evmcs_version; [Line4] Can we delete [Line4]? The only code reading env->features[FEAT_HV_NESTED_EAX] is [Line5]: c->eax = env->features[FEAT_HV_NESTED_EAX]; which is replaced with [Line6]: c->eax = cpu->hyperv_nested[0]; We are also replacing env->features[FEAT_HV_NESTED_EAX] with cpu->hyperv_nested[0], here: > + cpu->hyperv_nested[0] = evmcs_version; This will make [Line6] set c->eax to evmcs_version, unless other code writes to cpu->hyperv_nested[0]. I don't see any other code writing to hyperv_nested in this patch, so we are good. > } > } > > @@ -1235,13 +1252,6 @@ static int hyperv_handle_properties(CPUState *cs, > cpu->hyperv_version_id[3] = c->edx; > } > > - c = cpuid_find_entry(cpuid, HV_CPUID_FEATURES, 0); > - if (c) { > - env->features[FEAT_HYPERV_EAX] = c->eax; [Line7A] > - env->features[FEAT_HYPERV_EBX] = c->ebx; [Line7B] > - env->features[FEAT_HYPERV_EDX] = c->edx; [Line7D] Can we delete [Line7*]? The only code reading env->features[FEAT_HYPERV_*] seems to be [Line8*]: c->eax = env->features[FEAT_HYPERV_EAX]; c->ebx = env->features[FEAT_HYPERV_EBX]; c->edx = env->features[FEAT_HYPERV_EDX]; which is replaced by [Line9*]: c->eax = hv_build_cpuid_leaf(cs, FEAT_HYPERV_EAX); c->ebx = hv_build_cpuid_leaf(cs, FEAT_HYPERV_EBX); c->edx = hv_build_cpuid_leaf(cs, FEAT_HYPERV_EDX); So we need to make sure hv_build_cpuid_leaf() will return the right values at [Line9*]. This one will be trickier to evaluate: there are lots of entries in kvm_hyperv_properties[] that affect FEAT_HYPERV_EAX. I will pause here and continue next week. :) This smells like it could have been split into smaller patches, but I'm not sure if it would be possible. Maybe the existing code was too tangled for splitting this refactor into smaller patches. > - } > - > c = cpuid_find_entry(cpuid, HV_CPUID_IMPLEMENT_LIMITS, 0); > if (c) { > cpu->hv_max_vps = c->eax; > @@ -1252,23 +1262,8 @@ static int hyperv_handle_properties(CPUState *cs, > > c = cpuid_find_entry(cpuid, HV_CPUID_ENLIGHTMENT_INFO, 0); > if (c) { > - env->features[FEAT_HV_RECOMM_EAX] = c->eax; > cpu->hyperv_spinlock_attempts = c->ebx; > } > - c = cpuid_find_entry(cpuid, HV_CPUID_NESTED_FEATURES, 0); > - if (c) { > - env->features[FEAT_HV_NESTED_EAX] = c->eax; > - } > - } > - > - if (cpu->hyperv_no_nonarch_cs == ON_OFF_AUTO_ON) { > - env->features[FEAT_HV_RECOMM_EAX] |= HV_NO_NONARCH_CORESHARING; > - } else if (cpu->hyperv_no_nonarch_cs == ON_OFF_AUTO_AUTO) { > - c = cpuid_find_entry(cpuid, HV_CPUID_ENLIGHTMENT_INFO, 0); > - if (c) { > - env->features[FEAT_HV_RECOMM_EAX] |= > - c->eax & HV_NO_NONARCH_CORESHARING; > - } > } > > /* Features */ > @@ -1298,9 +1293,6 @@ static int hyperv_handle_properties(CPUState *cs, > r |= 1; > } > > - /* Not exposed by KVM but needed to make CPU hotplug in Windows work */ > - env->features[FEAT_HYPERV_EDX] |= HV_CPU_DYNAMIC_PARTITIONING_AVAILABLE; > - > if (r) { > r = -ENOSYS; > goto free; > @@ -1330,15 +1322,27 @@ static int hyperv_handle_properties(CPUState *cs, > > c = &cpuid_ent[cpuid_i++]; > c->function = HV_CPUID_FEATURES; > - c->eax = env->features[FEAT_HYPERV_EAX]; [Line8A] > - c->ebx = env->features[FEAT_HYPERV_EBX]; [Line8B] > - c->edx = env->features[FEAT_HYPERV_EDX]; [Line8D] > + c->eax = hv_build_cpuid_leaf(cs, FEAT_HYPERV_EAX); [Line9A] > + c->ebx = hv_build_cpuid_leaf(cs, FEAT_HYPERV_EBX); [Line9B] > + c->edx = hv_build_cpuid_leaf(cs, FEAT_HYPERV_EDX); [Line9D] > + > + /* Not exposed by KVM but needed to make CPU hotplug in Windows work */ > + c->edx |= HV_CPU_DYNAMIC_PARTITIONING_AVAILABLE; > > c = &cpuid_ent[cpuid_i++]; > c->function = HV_CPUID_ENLIGHTMENT_INFO; > - c->eax = env->features[FEAT_HV_RECOMM_EAX]; [Line2] > + c->eax = hv_build_cpuid_leaf(cs, FEAT_HV_RECOMM_EAX); [Line3] > c->ebx = cpu->hyperv_spinlock_attempts; > > + if (cpu->hyperv_no_nonarch_cs == ON_OFF_AUTO_ON) { > + c->eax |= HV_NO_NONARCH_CORESHARING; > + } else if (cpu->hyperv_no_nonarch_cs == ON_OFF_AUTO_AUTO) { > + c2 = cpuid_find_entry(cpuid, HV_CPUID_ENLIGHTMENT_INFO, 0); > + if (c2) { > + c->eax |= c2->eax & HV_NO_NONARCH_CORESHARING; > + } > + } > + > c = &cpuid_ent[cpuid_i++]; > c->function = HV_CPUID_IMPLEMENT_LIMITS; > c->eax = cpu->hv_max_vps; > @@ -1358,7 +1362,7 @@ static int hyperv_handle_properties(CPUState *cs, > > c = &cpuid_ent[cpuid_i++]; > c->function = HV_CPUID_NESTED_FEATURES; > - c->eax = env->features[FEAT_HV_NESTED_EAX]; > + c->eax = cpu->hyperv_nested[0]; > } > r = cpuid_i; > > -- > 2.30.2 >
On Fri, Apr 30, 2021 at 08:34:40PM -0400, Eduardo Habkost wrote: > On Thu, Apr 22, 2021 at 06:11:15PM +0200, Vitaly Kuznetsov wrote: > > As a preparatory patch to dropping Hyper-V CPUID leaves from > > feature_word_info[] stop using env->features[] as a temporary > > storage of Hyper-V CPUIDs, just build Hyper-V CPUID leaves directly > > from kvm_hyperv_properties[] data. > > > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > --- > > target/i386/cpu.h | 1 + > > target/i386/kvm/kvm.c | 80 +++++++++++++++++++++++-------------------- > > 2 files changed, 43 insertions(+), 38 deletions(-) > > > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > > index 570f916878f9..c8295aa2a1e7 100644 > > --- a/target/i386/cpu.h > > +++ b/target/i386/cpu.h > > @@ -1684,6 +1684,7 @@ struct X86CPU { > > uint32_t hyperv_interface_id[4]; > > uint32_t hyperv_version_id[4]; > > uint32_t hyperv_limits[3]; > > + uint32_t hyperv_nested[4]; > > > > bool check_cpuid; > > bool enforce_cpuid; > > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c > > index 7c751185491f..f791baa29acf 100644 > > --- a/target/i386/kvm/kvm.c > > +++ b/target/i386/kvm/kvm.c > > @@ -1111,7 +1111,6 @@ static int hv_cpuid_check_and_set(CPUState *cs, struct kvm_cpuid2 *cpuid, > > int feature) > > { > > X86CPU *cpu = X86_CPU(cs); > > - CPUX86State *env = &cpu->env; > > uint32_t r, fw, bits; > > uint64_t deps; > > int i, dep_feat; > > @@ -1151,8 +1150,6 @@ static int hv_cpuid_check_and_set(CPUState *cs, struct kvm_cpuid2 *cpuid, > > return 0; > > } > > } > > - > > - env->features[fw] |= bits; > > } > > > > if (cpu->hyperv_passthrough) { > > @@ -1162,6 +1159,29 @@ static int hv_cpuid_check_and_set(CPUState *cs, struct kvm_cpuid2 *cpuid, > > return 0; > > } > > > > +static uint32_t hv_build_cpuid_leaf(CPUState *cs, uint32_t fw) > > +{ > > + X86CPU *cpu = X86_CPU(cs); > > + uint32_t r = 0; > > + int i, j; > > + > > + for (i = 0; i < ARRAY_SIZE(kvm_hyperv_properties); i++) { > > + if (!hyperv_feat_enabled(cpu, i)) { > > + continue; > > + } > > + > > + for (j = 0; j < ARRAY_SIZE(kvm_hyperv_properties[i].flags); j++) { > > + if (kvm_hyperv_properties[i].flags[j].fw != fw) { > > + continue; > > + } > > + > > + r |= kvm_hyperv_properties[i].flags[j].bits; > > + } > > + } > > + > > + return r; > > +} > > + > > /* > > * Fill in Hyper-V CPUIDs. Returns the number of entries filled in cpuid_ent in > > * case of success, errno < 0 in case of failure and 0 when no Hyper-V > > @@ -1171,9 +1191,8 @@ static int hyperv_handle_properties(CPUState *cs, > > struct kvm_cpuid_entry2 *cpuid_ent) > > { > > X86CPU *cpu = X86_CPU(cs); > > - CPUX86State *env = &cpu->env; > > struct kvm_cpuid2 *cpuid; > > - struct kvm_cpuid_entry2 *c; > > + struct kvm_cpuid_entry2 *c, *c2; > > uint32_t cpuid_i = 0; > > int r; > > > > @@ -1194,9 +1213,7 @@ static int hyperv_handle_properties(CPUState *cs, > > } > > > > if (!r) { > > I think I now I understand why removing FEAT_HYPERV_* makes sense: > > The rules mapping hyperv features to CPUID bits were encoded > twice in the code: in both hyperv_handle_properties() and > kvm_hyperv_properties[]. More work to maintain the rules, and > too easy to accidentally make them inconsistent. > > > Now, let me see if I can prove to myself that the new code works: > > > - env->features[FEAT_HV_RECOMM_EAX] |= > > - HV_ENLIGHTENED_VMCS_RECOMMENDED; > > [Line1] > > The only code reading env->features[FEAT_HV_RECOMM_EAX] seems to > be [Line2] below: > > eax = env->features[FEAT_HV_RECOMM_EAX]; > > which is replaced with [Line3]: > > c->eax = hv_build_cpuid_leaf(cs, FEAT_HV_RECOMM_EAX); > > so if hv_build_cpuid_leaf() do its job and set return > HV_ENLIGHTENED_VMCS_RECOMMENDED set at [Line2], we can safely > delete [Line1]. > > Will hv_build_cpuid_leaf() set HV_ENLIGHTENED_VMCS_RECOMMENDED in > [Line2] for all cases where [Line1] was being executed? > > Let's check what's necessary to make hv_build_cpuid_leaf() > set HV_ENLIGHTENED_VMCS_RECOMMENDED: > > There's only one entry with > FEAT_HV_RECOMM_EAX/HV_ENLIGHTENED_VMCS_RECOMMENDED at > kvm_hyperv_properties: > > [HYPERV_FEAT_EVMCS] = { > .desc = "enlightened VMCS (hv-evmcs)", > .flags = { > {.fw = FEAT_HV_RECOMM_EAX, > .bits = HV_ENLIGHTENED_VMCS_RECOMMENDED} > }, > .dependencies = BIT(HYPERV_FEAT_VAPIC) > }, > > The logic at hv_build_cpuid_leaf() will make > HV_ENLIGHTENED_VMCS_RECOMMENDED be set only if > hyperv_feat_enabled(HYPERV_FEAT_EVMCS) is true. Which is what I > expected, because the line of code you are removing is inside a > hyperv_feat_enabled(cpu, HYPERV_FEAT_EVMCS) conditional. > > For reference, hyperv_feat_enabled(cpu, feat) returns: > !!(cpu->hyperv_features & BIT(feat)) > > I don't see any code _clearing_ hyperv_features, except for > properties that could change hyperv_features. I don't expect the > "hv-evmcs" QOM property to be touched by this function, so we > should be safe: if hyperv_feat_enabled(cpu, HYPERV_FEAT_EVMCS) > returned before executing [Line1], it will return true when > executing [Line2]. > > This means hv_build_cpuid_leaf() will set > HV_ENLIGHTENED_VMCS_RECOMMENDED if > hyperv_feat_enabled(HYPERV_FEAT_EVMCS) is true, and > hyperv_feat_enabled(HYPERV_FEAT_EVMCS) will be true at [Line2] on > all cases when [Line1] was being executed. > > We also need to be sure the HV_ENLIGHTENED_VMCS_RECOMMENDED will > be _unset_ at hv_build_cpuid_leaf() when expected, but the rules > are trickier (due to hyperv_passthrough). I'll try to prove that > later. > > > > - env->features[FEAT_HV_NESTED_EAX] = evmcs_version; > > [Line4] > > > Can we delete [Line4]? > > The only code reading env->features[FEAT_HV_NESTED_EAX] > is [Line5]: > c->eax = env->features[FEAT_HV_NESTED_EAX]; > which is replaced with [Line6]: > c->eax = cpu->hyperv_nested[0]; > > We are also replacing env->features[FEAT_HV_NESTED_EAX] with > cpu->hyperv_nested[0], here: > > > + cpu->hyperv_nested[0] = evmcs_version; > > This will make [Line6] set c->eax to evmcs_version, unless other > code writes to cpu->hyperv_nested[0]. > > I don't see any other code writing to hyperv_nested in this > patch, so we are good. > > > } > > } > > > > @@ -1235,13 +1252,6 @@ static int hyperv_handle_properties(CPUState *cs, > > cpu->hyperv_version_id[3] = c->edx; > > } > > > > - c = cpuid_find_entry(cpuid, HV_CPUID_FEATURES, 0); > > - if (c) { > > - env->features[FEAT_HYPERV_EAX] = c->eax; > > [Line7A] > > > - env->features[FEAT_HYPERV_EBX] = c->ebx; > > [Line7B] > > > - env->features[FEAT_HYPERV_EDX] = c->edx; > > [Line7D] > > > Can we delete [Line7*]? > > The only code reading env->features[FEAT_HYPERV_*] seems to be > [Line8*]: > c->eax = env->features[FEAT_HYPERV_EAX]; > c->ebx = env->features[FEAT_HYPERV_EBX]; > c->edx = env->features[FEAT_HYPERV_EDX]; > which is replaced by [Line9*]: > c->eax = hv_build_cpuid_leaf(cs, FEAT_HYPERV_EAX); > c->ebx = hv_build_cpuid_leaf(cs, FEAT_HYPERV_EBX); > c->edx = hv_build_cpuid_leaf(cs, FEAT_HYPERV_EDX); > > So we need to make sure hv_build_cpuid_leaf() will return the > right values at [Line9*]. > > This one will be trickier to evaluate: there are lots of entries in > kvm_hyperv_properties[] that affect FEAT_HYPERV_EAX. > > I will pause here and continue next week. :) > Continuing: So, [Line7*] above was for hyperv_passthrough mode only. This means now hv-passthrough will enable only known features (the ones at kvm_hyperv_properties). The same comment I sent to the previous patch applies here: """ This makes hv-passthrough less useful for debugging and development, but safer for using in production. I assume we want to eventually make hv-passthrough supported in production when live migration support isn't required. I'll trust your judgement here and assume this is really a good change, but maybe this should be documented more explicitly in the hv-passthrough section at docs/hyperv.txt? """ > This smells like it could have been split into smaller patches, > but I'm not sure if it would be possible. Maybe the existing > code was too tangled for splitting this refactor into smaller > patches. > > > > - } > > - > > c = cpuid_find_entry(cpuid, HV_CPUID_IMPLEMENT_LIMITS, 0); > > if (c) { > > cpu->hv_max_vps = c->eax; > > @@ -1252,23 +1262,8 @@ static int hyperv_handle_properties(CPUState *cs, > > > > c = cpuid_find_entry(cpuid, HV_CPUID_ENLIGHTMENT_INFO, 0); > > if (c) { > > - env->features[FEAT_HV_RECOMM_EAX] = c->eax; Same as above: this is hv-passthrough code, and the comment above applies. > > cpu->hyperv_spinlock_attempts = c->ebx; > > } > > - c = cpuid_find_entry(cpuid, HV_CPUID_NESTED_FEATURES, 0); > > - if (c) { > > - env->features[FEAT_HV_NESTED_EAX] = c->eax; > > - } Same as above: this is hv-passthrough code, and the comment above applies. > > - } > > - Now we're outside the hv-passthrough code: > > - if (cpu->hyperv_no_nonarch_cs == ON_OFF_AUTO_ON) { > > - env->features[FEAT_HV_RECOMM_EAX] |= HV_NO_NONARCH_CORESHARING; > > - } else if (cpu->hyperv_no_nonarch_cs == ON_OFF_AUTO_AUTO) { > > - c = cpuid_find_entry(cpuid, HV_CPUID_ENLIGHTMENT_INFO, 0); > > - if (c) { > > - env->features[FEAT_HV_RECOMM_EAX] |= > > - c->eax & HV_NO_NONARCH_CORESHARING; > > - } > > } The hack above is copied to [Line10] below. Looks OK. > > > > /* Features */ > > @@ -1298,9 +1293,6 @@ static int hyperv_handle_properties(CPUState *cs, > > r |= 1; > > } > > > > - /* Not exposed by KVM but needed to make CPU hotplug in Windows work */ > > - env->features[FEAT_HYPERV_EDX] |= HV_CPU_DYNAMIC_PARTITIONING_AVAILABLE; The hack above is copied to [Line10] below. Looks OK. > > - > > if (r) { > > r = -ENOSYS; > > goto free; > > @@ -1330,15 +1322,27 @@ static int hyperv_handle_properties(CPUState *cs, > > > > c = &cpuid_ent[cpuid_i++]; > > c->function = HV_CPUID_FEATURES; > > - c->eax = env->features[FEAT_HYPERV_EAX]; > > [Line8A] > > > - c->ebx = env->features[FEAT_HYPERV_EBX]; > > [Line8B] > > > - c->edx = env->features[FEAT_HYPERV_EDX]; > > [Line8D] > > > + c->eax = hv_build_cpuid_leaf(cs, FEAT_HYPERV_EAX); > > [Line9A] > > > + c->ebx = hv_build_cpuid_leaf(cs, FEAT_HYPERV_EBX); > > [Line9B] > > > + c->edx = hv_build_cpuid_leaf(cs, FEAT_HYPERV_EDX); > > [Line9D] Already reviewed at [Line7*] above. > > > + > > + /* Not exposed by KVM but needed to make CPU hotplug in Windows work */ > > + c->edx |= HV_CPU_DYNAMIC_PARTITIONING_AVAILABLE; [Line11] Looks OK, but I wonder if this could be encoded in kvm_hyperv_properties somehow. > > > > c = &cpuid_ent[cpuid_i++]; > > c->function = HV_CPUID_ENLIGHTMENT_INFO; > > - c->eax = env->features[FEAT_HV_RECOMM_EAX]; > > [Line2] > > > + c->eax = hv_build_cpuid_leaf(cs, FEAT_HV_RECOMM_EAX); > > [Line3] > > > c->ebx = cpu->hyperv_spinlock_attempts; > > > > + if (cpu->hyperv_no_nonarch_cs == ON_OFF_AUTO_ON) { > > + c->eax |= HV_NO_NONARCH_CORESHARING; > > + } else if (cpu->hyperv_no_nonarch_cs == ON_OFF_AUTO_AUTO) { > > + c2 = cpuid_find_entry(cpuid, HV_CPUID_ENLIGHTMENT_INFO, 0); > > + if (c2) { > > + c->eax |= c2->eax & HV_NO_NONARCH_CORESHARING; > > + } > > + } [Line10] Matches the code above. > > + > > c = &cpuid_ent[cpuid_i++]; > > c->function = HV_CPUID_IMPLEMENT_LIMITS; > > c->eax = cpu->hv_max_vps; > > @@ -1358,7 +1362,7 @@ static int hyperv_handle_properties(CPUState *cs, > > > > c = &cpuid_ent[cpuid_i++]; > > c->function = HV_CPUID_NESTED_FEATURES; > > - c->eax = env->features[FEAT_HV_NESTED_EAX]; > > + c->eax = cpu->hyperv_nested[0]; > > } > > r = cpuid_i; > > > > -- > > 2.30.2 > > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> Sorry for the long delay in reviewing this!
Eduardo Habkost <ehabkost@redhat.com> writes: > On Fri, Apr 30, 2021 at 08:34:40PM -0400, Eduardo Habkost wrote: >> On Thu, Apr 22, 2021 at 06:11:15PM +0200, Vitaly Kuznetsov wrote: >> > As a preparatory patch to dropping Hyper-V CPUID leaves from >> > feature_word_info[] stop using env->features[] as a temporary >> > storage of Hyper-V CPUIDs, just build Hyper-V CPUID leaves directly >> > from kvm_hyperv_properties[] data. >> > >> > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> >> > --- >> > target/i386/cpu.h | 1 + >> > target/i386/kvm/kvm.c | 80 +++++++++++++++++++++++-------------------- >> > 2 files changed, 43 insertions(+), 38 deletions(-) >> > >> > diff --git a/target/i386/cpu.h b/target/i386/cpu.h >> > index 570f916878f9..c8295aa2a1e7 100644 >> > --- a/target/i386/cpu.h >> > +++ b/target/i386/cpu.h >> > @@ -1684,6 +1684,7 @@ struct X86CPU { >> > uint32_t hyperv_interface_id[4]; >> > uint32_t hyperv_version_id[4]; >> > uint32_t hyperv_limits[3]; >> > + uint32_t hyperv_nested[4]; >> > >> > bool check_cpuid; >> > bool enforce_cpuid; >> > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c >> > index 7c751185491f..f791baa29acf 100644 >> > --- a/target/i386/kvm/kvm.c >> > +++ b/target/i386/kvm/kvm.c >> > @@ -1111,7 +1111,6 @@ static int hv_cpuid_check_and_set(CPUState *cs, struct kvm_cpuid2 *cpuid, >> > int feature) >> > { >> > X86CPU *cpu = X86_CPU(cs); >> > - CPUX86State *env = &cpu->env; >> > uint32_t r, fw, bits; >> > uint64_t deps; >> > int i, dep_feat; >> > @@ -1151,8 +1150,6 @@ static int hv_cpuid_check_and_set(CPUState *cs, struct kvm_cpuid2 *cpuid, >> > return 0; >> > } >> > } >> > - >> > - env->features[fw] |= bits; >> > } >> > >> > if (cpu->hyperv_passthrough) { >> > @@ -1162,6 +1159,29 @@ static int hv_cpuid_check_and_set(CPUState *cs, struct kvm_cpuid2 *cpuid, >> > return 0; >> > } >> > >> > +static uint32_t hv_build_cpuid_leaf(CPUState *cs, uint32_t fw) >> > +{ >> > + X86CPU *cpu = X86_CPU(cs); >> > + uint32_t r = 0; >> > + int i, j; >> > + >> > + for (i = 0; i < ARRAY_SIZE(kvm_hyperv_properties); i++) { >> > + if (!hyperv_feat_enabled(cpu, i)) { >> > + continue; >> > + } >> > + >> > + for (j = 0; j < ARRAY_SIZE(kvm_hyperv_properties[i].flags); j++) { >> > + if (kvm_hyperv_properties[i].flags[j].fw != fw) { >> > + continue; >> > + } >> > + >> > + r |= kvm_hyperv_properties[i].flags[j].bits; >> > + } >> > + } >> > + >> > + return r; >> > +} >> > + >> > /* >> > * Fill in Hyper-V CPUIDs. Returns the number of entries filled in cpuid_ent in >> > * case of success, errno < 0 in case of failure and 0 when no Hyper-V >> > @@ -1171,9 +1191,8 @@ static int hyperv_handle_properties(CPUState *cs, >> > struct kvm_cpuid_entry2 *cpuid_ent) >> > { >> > X86CPU *cpu = X86_CPU(cs); >> > - CPUX86State *env = &cpu->env; >> > struct kvm_cpuid2 *cpuid; >> > - struct kvm_cpuid_entry2 *c; >> > + struct kvm_cpuid_entry2 *c, *c2; >> > uint32_t cpuid_i = 0; >> > int r; >> > >> > @@ -1194,9 +1213,7 @@ static int hyperv_handle_properties(CPUState *cs, >> > } >> > >> > if (!r) { >> >> I think I now I understand why removing FEAT_HYPERV_* makes sense: >> >> The rules mapping hyperv features to CPUID bits were encoded >> twice in the code: in both hyperv_handle_properties() and >> kvm_hyperv_properties[]. More work to maintain the rules, and >> too easy to accidentally make them inconsistent. >> >> >> Now, let me see if I can prove to myself that the new code works: >> >> > - env->features[FEAT_HV_RECOMM_EAX] |= >> > - HV_ENLIGHTENED_VMCS_RECOMMENDED; >> >> [Line1] >> >> The only code reading env->features[FEAT_HV_RECOMM_EAX] seems to >> be [Line2] below: >> >> eax = env->features[FEAT_HV_RECOMM_EAX]; >> >> which is replaced with [Line3]: >> >> c->eax = hv_build_cpuid_leaf(cs, FEAT_HV_RECOMM_EAX); >> >> so if hv_build_cpuid_leaf() do its job and set return >> HV_ENLIGHTENED_VMCS_RECOMMENDED set at [Line2], we can safely >> delete [Line1]. >> >> Will hv_build_cpuid_leaf() set HV_ENLIGHTENED_VMCS_RECOMMENDED in >> [Line2] for all cases where [Line1] was being executed? >> >> Let's check what's necessary to make hv_build_cpuid_leaf() >> set HV_ENLIGHTENED_VMCS_RECOMMENDED: >> >> There's only one entry with >> FEAT_HV_RECOMM_EAX/HV_ENLIGHTENED_VMCS_RECOMMENDED at >> kvm_hyperv_properties: >> >> [HYPERV_FEAT_EVMCS] = { >> .desc = "enlightened VMCS (hv-evmcs)", >> .flags = { >> {.fw = FEAT_HV_RECOMM_EAX, >> .bits = HV_ENLIGHTENED_VMCS_RECOMMENDED} >> }, >> .dependencies = BIT(HYPERV_FEAT_VAPIC) >> }, >> >> The logic at hv_build_cpuid_leaf() will make >> HV_ENLIGHTENED_VMCS_RECOMMENDED be set only if >> hyperv_feat_enabled(HYPERV_FEAT_EVMCS) is true. Which is what I >> expected, because the line of code you are removing is inside a >> hyperv_feat_enabled(cpu, HYPERV_FEAT_EVMCS) conditional. >> >> For reference, hyperv_feat_enabled(cpu, feat) returns: >> !!(cpu->hyperv_features & BIT(feat)) >> >> I don't see any code _clearing_ hyperv_features, except for >> properties that could change hyperv_features. I don't expect the >> "hv-evmcs" QOM property to be touched by this function, so we >> should be safe: if hyperv_feat_enabled(cpu, HYPERV_FEAT_EVMCS) >> returned before executing [Line1], it will return true when >> executing [Line2]. >> >> This means hv_build_cpuid_leaf() will set >> HV_ENLIGHTENED_VMCS_RECOMMENDED if >> hyperv_feat_enabled(HYPERV_FEAT_EVMCS) is true, and >> hyperv_feat_enabled(HYPERV_FEAT_EVMCS) will be true at [Line2] on >> all cases when [Line1] was being executed. >> >> We also need to be sure the HV_ENLIGHTENED_VMCS_RECOMMENDED will >> be _unset_ at hv_build_cpuid_leaf() when expected, but the rules >> are trickier (due to hyperv_passthrough). I'll try to prove that >> later. >> >> >> > - env->features[FEAT_HV_NESTED_EAX] = evmcs_version; >> >> [Line4] >> >> >> Can we delete [Line4]? >> >> The only code reading env->features[FEAT_HV_NESTED_EAX] >> is [Line5]: >> c->eax = env->features[FEAT_HV_NESTED_EAX]; >> which is replaced with [Line6]: >> c->eax = cpu->hyperv_nested[0]; >> >> We are also replacing env->features[FEAT_HV_NESTED_EAX] with >> cpu->hyperv_nested[0], here: >> >> > + cpu->hyperv_nested[0] = evmcs_version; >> >> This will make [Line6] set c->eax to evmcs_version, unless other >> code writes to cpu->hyperv_nested[0]. >> >> I don't see any other code writing to hyperv_nested in this >> patch, so we are good. >> >> > } >> > } >> > >> > @@ -1235,13 +1252,6 @@ static int hyperv_handle_properties(CPUState *cs, >> > cpu->hyperv_version_id[3] = c->edx; >> > } >> > >> > - c = cpuid_find_entry(cpuid, HV_CPUID_FEATURES, 0); >> > - if (c) { >> > - env->features[FEAT_HYPERV_EAX] = c->eax; >> >> [Line7A] >> >> > - env->features[FEAT_HYPERV_EBX] = c->ebx; >> >> [Line7B] >> >> > - env->features[FEAT_HYPERV_EDX] = c->edx; >> >> [Line7D] >> >> >> Can we delete [Line7*]? >> >> The only code reading env->features[FEAT_HYPERV_*] seems to be >> [Line8*]: >> c->eax = env->features[FEAT_HYPERV_EAX]; >> c->ebx = env->features[FEAT_HYPERV_EBX]; >> c->edx = env->features[FEAT_HYPERV_EDX]; >> which is replaced by [Line9*]: >> c->eax = hv_build_cpuid_leaf(cs, FEAT_HYPERV_EAX); >> c->ebx = hv_build_cpuid_leaf(cs, FEAT_HYPERV_EBX); >> c->edx = hv_build_cpuid_leaf(cs, FEAT_HYPERV_EDX); >> >> So we need to make sure hv_build_cpuid_leaf() will return the >> right values at [Line9*]. >> >> This one will be trickier to evaluate: there are lots of entries in >> kvm_hyperv_properties[] that affect FEAT_HYPERV_EAX. >> >> I will pause here and continue next week. :) >> > > Continuing: > > So, [Line7*] above was for hyperv_passthrough mode only. This > means now hv-passthrough will enable only known features (the > ones at kvm_hyperv_properties). > > The same comment I sent to the previous patch applies here: > > """ > This makes hv-passthrough less useful for debugging and > development, but safer for using in production. I assume we want > to eventually make hv-passthrough supported in production when > live migration support isn't required. > > I'll trust your judgement here and assume this is really a good > change, but maybe this should be documented more explicitly in > the hv-passthrough section at docs/hyperv.txt? > """ > Makes sense, will do. > > >> This smells like it could have been split into smaller patches, >> but I'm not sure if it would be possible. Maybe the existing >> code was too tangled for splitting this refactor into smaller >> patches. >> >> >> > - } >> > - >> > c = cpuid_find_entry(cpuid, HV_CPUID_IMPLEMENT_LIMITS, 0); >> > if (c) { >> > cpu->hv_max_vps = c->eax; >> > @@ -1252,23 +1262,8 @@ static int hyperv_handle_properties(CPUState *cs, >> > >> > c = cpuid_find_entry(cpuid, HV_CPUID_ENLIGHTMENT_INFO, 0); >> > if (c) { >> > - env->features[FEAT_HV_RECOMM_EAX] = c->eax; > > Same as above: this is hv-passthrough code, and the comment above > applies. > >> > cpu->hyperv_spinlock_attempts = c->ebx; >> > } >> > - c = cpuid_find_entry(cpuid, HV_CPUID_NESTED_FEATURES, 0); >> > - if (c) { >> > - env->features[FEAT_HV_NESTED_EAX] = c->eax; >> > - } > > Same as above: this is hv-passthrough code, and the comment above > applies. > >> > - } >> > - > > Now we're outside the hv-passthrough code: > >> > - if (cpu->hyperv_no_nonarch_cs == ON_OFF_AUTO_ON) { >> > - env->features[FEAT_HV_RECOMM_EAX] |= HV_NO_NONARCH_CORESHARING; >> > - } else if (cpu->hyperv_no_nonarch_cs == ON_OFF_AUTO_AUTO) { >> > - c = cpuid_find_entry(cpuid, HV_CPUID_ENLIGHTMENT_INFO, 0); >> > - if (c) { >> > - env->features[FEAT_HV_RECOMM_EAX] |= >> > - c->eax & HV_NO_NONARCH_CORESHARING; >> > - } >> > } > > The hack above is copied to [Line10] below. Looks OK. > >> > >> > /* Features */ >> > @@ -1298,9 +1293,6 @@ static int hyperv_handle_properties(CPUState *cs, >> > r |= 1; >> > } >> > >> > - /* Not exposed by KVM but needed to make CPU hotplug in Windows work */ >> > - env->features[FEAT_HYPERV_EDX] |= HV_CPU_DYNAMIC_PARTITIONING_AVAILABLE; > > The hack above is copied to [Line10] below. Looks OK. > >> > - >> > if (r) { >> > r = -ENOSYS; >> > goto free; >> > @@ -1330,15 +1322,27 @@ static int hyperv_handle_properties(CPUState *cs, >> > >> > c = &cpuid_ent[cpuid_i++]; >> > c->function = HV_CPUID_FEATURES; >> > - c->eax = env->features[FEAT_HYPERV_EAX]; >> >> [Line8A] >> >> > - c->ebx = env->features[FEAT_HYPERV_EBX]; >> >> [Line8B] >> >> > - c->edx = env->features[FEAT_HYPERV_EDX]; >> >> [Line8D] >> >> > + c->eax = hv_build_cpuid_leaf(cs, FEAT_HYPERV_EAX); >> >> [Line9A] >> >> > + c->ebx = hv_build_cpuid_leaf(cs, FEAT_HYPERV_EBX); >> >> [Line9B] >> >> > + c->edx = hv_build_cpuid_leaf(cs, FEAT_HYPERV_EDX); >> >> [Line9D] > > Already reviewed at [Line7*] above. > >> >> > + >> > + /* Not exposed by KVM but needed to make CPU hotplug in Windows work */ >> > + c->edx |= HV_CPU_DYNAMIC_PARTITIONING_AVAILABLE; > > [Line11] > > Looks OK, but I wonder if this could be encoded in > kvm_hyperv_properties somehow. > (The patch series changes nothing in this regard) this is not a separate feature as we enable it unconditionally. It is similar to e.g HV_HYPERCALL_AVAILABLE which we add unconditionally. The only reason why we can't just move it to kvm_hyperv_properties[] is because it is not exposed by KVM (and I'm not sure why to be honest but it is already 'legacy'). > >> > >> > c = &cpuid_ent[cpuid_i++]; >> > c->function = HV_CPUID_ENLIGHTMENT_INFO; >> > - c->eax = env->features[FEAT_HV_RECOMM_EAX]; >> >> [Line2] >> >> > + c->eax = hv_build_cpuid_leaf(cs, FEAT_HV_RECOMM_EAX); >> >> [Line3] >> >> > c->ebx = cpu->hyperv_spinlock_attempts; >> > >> > + if (cpu->hyperv_no_nonarch_cs == ON_OFF_AUTO_ON) { >> > + c->eax |= HV_NO_NONARCH_CORESHARING; >> > + } else if (cpu->hyperv_no_nonarch_cs == ON_OFF_AUTO_AUTO) { >> > + c2 = cpuid_find_entry(cpuid, HV_CPUID_ENLIGHTMENT_INFO, 0); >> > + if (c2) { >> > + c->eax |= c2->eax & HV_NO_NONARCH_CORESHARING; >> > + } >> > + } > > [Line10] > > Matches the code above. > >> > + >> > c = &cpuid_ent[cpuid_i++]; >> > c->function = HV_CPUID_IMPLEMENT_LIMITS; >> > c->eax = cpu->hv_max_vps; >> > @@ -1358,7 +1362,7 @@ static int hyperv_handle_properties(CPUState *cs, >> > >> > c = &cpuid_ent[cpuid_i++]; >> > c->function = HV_CPUID_NESTED_FEATURES; >> > - c->eax = env->features[FEAT_HV_NESTED_EAX]; >> > + c->eax = cpu->hyperv_nested[0]; >> > } >> > r = cpuid_i; >> > >> > -- >> > 2.30.2 >> > >> > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > Thanks!
diff --git a/target/i386/cpu.h b/target/i386/cpu.h index 570f916878f9..c8295aa2a1e7 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -1684,6 +1684,7 @@ struct X86CPU { uint32_t hyperv_interface_id[4]; uint32_t hyperv_version_id[4]; uint32_t hyperv_limits[3]; + uint32_t hyperv_nested[4]; bool check_cpuid; bool enforce_cpuid; diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c index 7c751185491f..f791baa29acf 100644 --- a/target/i386/kvm/kvm.c +++ b/target/i386/kvm/kvm.c @@ -1111,7 +1111,6 @@ static int hv_cpuid_check_and_set(CPUState *cs, struct kvm_cpuid2 *cpuid, int feature) { X86CPU *cpu = X86_CPU(cs); - CPUX86State *env = &cpu->env; uint32_t r, fw, bits; uint64_t deps; int i, dep_feat; @@ -1151,8 +1150,6 @@ static int hv_cpuid_check_and_set(CPUState *cs, struct kvm_cpuid2 *cpuid, return 0; } } - - env->features[fw] |= bits; } if (cpu->hyperv_passthrough) { @@ -1162,6 +1159,29 @@ static int hv_cpuid_check_and_set(CPUState *cs, struct kvm_cpuid2 *cpuid, return 0; } +static uint32_t hv_build_cpuid_leaf(CPUState *cs, uint32_t fw) +{ + X86CPU *cpu = X86_CPU(cs); + uint32_t r = 0; + int i, j; + + for (i = 0; i < ARRAY_SIZE(kvm_hyperv_properties); i++) { + if (!hyperv_feat_enabled(cpu, i)) { + continue; + } + + for (j = 0; j < ARRAY_SIZE(kvm_hyperv_properties[i].flags); j++) { + if (kvm_hyperv_properties[i].flags[j].fw != fw) { + continue; + } + + r |= kvm_hyperv_properties[i].flags[j].bits; + } + } + + return r; +} + /* * Fill in Hyper-V CPUIDs. Returns the number of entries filled in cpuid_ent in * case of success, errno < 0 in case of failure and 0 when no Hyper-V @@ -1171,9 +1191,8 @@ static int hyperv_handle_properties(CPUState *cs, struct kvm_cpuid_entry2 *cpuid_ent) { X86CPU *cpu = X86_CPU(cs); - CPUX86State *env = &cpu->env; struct kvm_cpuid2 *cpuid; - struct kvm_cpuid_entry2 *c; + struct kvm_cpuid_entry2 *c, *c2; uint32_t cpuid_i = 0; int r; @@ -1194,9 +1213,7 @@ static int hyperv_handle_properties(CPUState *cs, } if (!r) { - env->features[FEAT_HV_RECOMM_EAX] |= - HV_ENLIGHTENED_VMCS_RECOMMENDED; - env->features[FEAT_HV_NESTED_EAX] = evmcs_version; + cpu->hyperv_nested[0] = evmcs_version; } } @@ -1235,13 +1252,6 @@ static int hyperv_handle_properties(CPUState *cs, cpu->hyperv_version_id[3] = c->edx; } - c = cpuid_find_entry(cpuid, HV_CPUID_FEATURES, 0); - if (c) { - env->features[FEAT_HYPERV_EAX] = c->eax; - env->features[FEAT_HYPERV_EBX] = c->ebx; - env->features[FEAT_HYPERV_EDX] = c->edx; - } - c = cpuid_find_entry(cpuid, HV_CPUID_IMPLEMENT_LIMITS, 0); if (c) { cpu->hv_max_vps = c->eax; @@ -1252,23 +1262,8 @@ static int hyperv_handle_properties(CPUState *cs, c = cpuid_find_entry(cpuid, HV_CPUID_ENLIGHTMENT_INFO, 0); if (c) { - env->features[FEAT_HV_RECOMM_EAX] = c->eax; cpu->hyperv_spinlock_attempts = c->ebx; } - c = cpuid_find_entry(cpuid, HV_CPUID_NESTED_FEATURES, 0); - if (c) { - env->features[FEAT_HV_NESTED_EAX] = c->eax; - } - } - - if (cpu->hyperv_no_nonarch_cs == ON_OFF_AUTO_ON) { - env->features[FEAT_HV_RECOMM_EAX] |= HV_NO_NONARCH_CORESHARING; - } else if (cpu->hyperv_no_nonarch_cs == ON_OFF_AUTO_AUTO) { - c = cpuid_find_entry(cpuid, HV_CPUID_ENLIGHTMENT_INFO, 0); - if (c) { - env->features[FEAT_HV_RECOMM_EAX] |= - c->eax & HV_NO_NONARCH_CORESHARING; - } } /* Features */ @@ -1298,9 +1293,6 @@ static int hyperv_handle_properties(CPUState *cs, r |= 1; } - /* Not exposed by KVM but needed to make CPU hotplug in Windows work */ - env->features[FEAT_HYPERV_EDX] |= HV_CPU_DYNAMIC_PARTITIONING_AVAILABLE; - if (r) { r = -ENOSYS; goto free; @@ -1330,15 +1322,27 @@ static int hyperv_handle_properties(CPUState *cs, c = &cpuid_ent[cpuid_i++]; c->function = HV_CPUID_FEATURES; - c->eax = env->features[FEAT_HYPERV_EAX]; - c->ebx = env->features[FEAT_HYPERV_EBX]; - c->edx = env->features[FEAT_HYPERV_EDX]; + c->eax = hv_build_cpuid_leaf(cs, FEAT_HYPERV_EAX); + c->ebx = hv_build_cpuid_leaf(cs, FEAT_HYPERV_EBX); + c->edx = hv_build_cpuid_leaf(cs, FEAT_HYPERV_EDX); + + /* Not exposed by KVM but needed to make CPU hotplug in Windows work */ + c->edx |= HV_CPU_DYNAMIC_PARTITIONING_AVAILABLE; c = &cpuid_ent[cpuid_i++]; c->function = HV_CPUID_ENLIGHTMENT_INFO; - c->eax = env->features[FEAT_HV_RECOMM_EAX]; + c->eax = hv_build_cpuid_leaf(cs, FEAT_HV_RECOMM_EAX); c->ebx = cpu->hyperv_spinlock_attempts; + if (cpu->hyperv_no_nonarch_cs == ON_OFF_AUTO_ON) { + c->eax |= HV_NO_NONARCH_CORESHARING; + } else if (cpu->hyperv_no_nonarch_cs == ON_OFF_AUTO_AUTO) { + c2 = cpuid_find_entry(cpuid, HV_CPUID_ENLIGHTMENT_INFO, 0); + if (c2) { + c->eax |= c2->eax & HV_NO_NONARCH_CORESHARING; + } + } + c = &cpuid_ent[cpuid_i++]; c->function = HV_CPUID_IMPLEMENT_LIMITS; c->eax = cpu->hv_max_vps; @@ -1358,7 +1362,7 @@ static int hyperv_handle_properties(CPUState *cs, c = &cpuid_ent[cpuid_i++]; c->function = HV_CPUID_NESTED_FEATURES; - c->eax = env->features[FEAT_HV_NESTED_EAX]; + c->eax = cpu->hyperv_nested[0]; } r = cpuid_i;
As a preparatory patch to dropping Hyper-V CPUID leaves from feature_word_info[] stop using env->features[] as a temporary storage of Hyper-V CPUIDs, just build Hyper-V CPUID leaves directly from kvm_hyperv_properties[] data. Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- target/i386/cpu.h | 1 + target/i386/kvm/kvm.c | 80 +++++++++++++++++++++++-------------------- 2 files changed, 43 insertions(+), 38 deletions(-)