Re: [PATCH v6 15/19] i386: expand Hyper-V features during CPU feature expansion time

[Eduardo Habkost]

On Mon, May 24, 2021 at 02:13:09PM +0200, Vitaly Kuznetsov wrote:
[...]
> >> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> >> index a42263b24fca..d5551c4ab5cf 100644
> >> --- a/target/i386/kvm/kvm.c
> >> +++ b/target/i386/kvm/kvm.c
> >> @@ -1216,13 +1216,22 @@ static uint32_t hv_build_cpuid_leaf(CPUState *cs, 
> >> uint32_t func, int reg)
> >>   * of 'hv_passthrough' mode and fills the environment with all supported
> >>   * Hyper-V features.
> >>   */
> >> -static void hyperv_expand_features(CPUState *cs, Error **errp)
> >> +void kvm_hyperv_expand_features(X86CPU *cpu, Error **errp)
> >>  {
> >> -    X86CPU *cpu = X86_CPU(cs);
> >> +    CPUState *cs = CPU(cpu);
> >>  
> >>      if (!hyperv_enabled(cpu))
> >>          return;
> >>  
> >> +    /*
> >> +     * When kvm_hyperv_expand_features is called at CPU feature expansion
> >> +     * time per-CPU kvm_state is not available yet so we can only proceed
> >> +     * when KVM_CAP_SYS_HYPERV_CPUID is supported.
> >> +     */
> >> +    if (!cs->kvm_state &&
> >> +        !kvm_check_extension(kvm_state, KVM_CAP_SYS_HYPERV_CPUID))
> >> +        return;
> >> +
> >>      if (cpu->hyperv_passthrough) {
> >>          cpu->hyperv_vendor_id[0] =
> >>              hv_cpuid_get_host(cs, HV_CPUID_VENDOR_AND_MAX_FUNCTIONS, 
> >> R_EBX);
> >> @@ -1556,7 +1565,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >>      env->apic_bus_freq = KVM_APIC_BUS_FREQUENCY;
> >>  
> >>      /* Paravirtualization CPUIDs */
> >> -    hyperv_expand_features(cs, &local_err);
> >> +    kvm_hyperv_expand_features(cpu, &local_err);
> >
> > Do we still need to call the function again here?
> >
> > If the first expansion isn't expanding everything, I'm afraid
> > this second call will hide bugs in query-cpu-model-expansion.
> >
> 
> The first expansion will do nothing if KVM_CAP_SYS_HYPERV_CPUID is not
> supported, calling it here allows us to proceed. The series makes
> 'query-cpu-model-expansion' output correct only with
> KVM_CAP_SYS_HYPERV_CPUID, without it we don't seem to be able to do much
> (unless we decide to create a 'scratch' CPU or something like that).

Oh, I see.  I suggest adding a comment explaining that.
Developers might be tempted to delete it and not notice it breaks
under older kernels.

-- 
Eduardo