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

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Eduardo Habkost <ehabkost@redhat.com> writes:

> 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.

Will do, thanks!

-- 
Vitaly