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

[Eduardo Habkost <ehabkost@redhat.com>]

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
> 

-- 
Eduardo