Re: [PATCH v6 04/19] i386: stop using env->features[] for filling Hyper-V CPUIDs

[Vitaly Kuznetsov]

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!

-- 
Vitaly