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 <vkuzn...@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 <ehabk...@redhat.com>
Sorry for the long delay in reviewing this!
--
Eduardo
