Re: [PATCH 1/5] i386: move hyperv_vendor_id initialization to x86_cpu_realizefn()

[Claudio Fontana <cfontana@suse.de>]

On 3/10/21 12:43 PM, Vitaly Kuznetsov wrote:
> Claudio Fontana <cfontana@suse.de> writes:
> 
>> On 11/19/20 11:32 AM, Vitaly Kuznetsov wrote:
>>> As a preparation to expanding Hyper-V CPU features early, move
>>> hyperv_vendor_id initialization to x86_cpu_realizefn(). Introduce
>>> x86_cpu_hyperv_realize() to not not pollute x86_cpu_realizefn()
>>> itself.
>>>
>>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>>
>>
>> Hi Vitaly,
>>
>> If building for TCG-only, does the x86_cpu_hyperv_realize function and other hyperv related functions
>> make sense to keep in the build?
>>
>> Now that we have per-accelerator subdirs in target/i386, should the hyperv code be moved over?
>>
> 
> Hi Claudio,
> 
> I'm not exactly sure. On one hand, we only implement Hyper-V emulation
> with KVM now.


Hi Vitaly,


> On the other hand Hyper-V specific CPU options are
> available even without it (and as Igor feels strongly against
> introducing custom option parsers, I don't see how we can forbid to use
> them without KVM). x86_cpu_hyperv_realize() is the bare minimum which is
> only needed to set our internal Hyper-V related data in a sane way,
> e.g. set the default to 'cpu->hyperv_vendor_id'. The actual enablement
> code is in target/i386/kvm.c already. Do you see anything besides
> x86_cpu_hyperv_realize() which we could move there?

Currently I don't,
I see a lot of PROPs (hyperv_features), but I assume those are the ones you mention cannot be moved right?


> Could you please
> take a look at v5
> (https://mail.gnu.org/archive/html/qemu-devel/2021-03/msg00158.html)?


I took a look at the series you pointed me to, and it seems it is all pretty much kvm/ work to me.

Which hypervisors do you think would theoretically make sense when looking at hyperv emulation in the future?

Looking at the current code and your patches, in the series you link to,
I'd be tempted to suggest moving the hyperv realize function inside kvm_cpu_realizefn in kvm/kvm-cpu.c .

Ciao,

Claudio



> 
> Thanks!
> 
> 
>> Thanks,
>>
>> Claudio
>>
>>
>>> ---
>>>  target/i386/cpu.c | 23 ++++++++++++++++++++++-
>>>  target/i386/cpu.h |  3 ++-
>>>  target/i386/kvm.c | 25 ++++++++++---------------
>>>  3 files changed, 34 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>>> index 5a8c96072e41..2a6885753378 100644
>>> --- a/target/i386/cpu.c
>>> +++ b/target/i386/cpu.c
>>> @@ -6509,6 +6509,25 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose)
>>>      }
>>>  }
>>>  
>>> +static void x86_cpu_hyperv_realize(X86CPU *cpu)
>>> +{
>>> +    size_t len;
>>> +
>>> +    /* Hyper-V vendor id */
>>> +    if (!cpu->hyperv_vendor) {
>>> +        memcpy(cpu->hyperv_vendor_id, "Microsoft Hv", 12);
>>> +    } else {
>>> +        len = strlen(cpu->hyperv_vendor);
>>> +
>>> +        if (len > 12) {
>>> +            warn_report("hv-vendor-id truncated to 12 characters");
>>> +            len = 12;
>>> +        }
>>> +        memset(cpu->hyperv_vendor_id, 0, 12);
>>> +        memcpy(cpu->hyperv_vendor_id, cpu->hyperv_vendor, len);
>>> +    }
>>> +}
>>> +
>>>  static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>>>  {
>>>      CPUState *cs = CPU(dev);
>>> @@ -6680,6 +6699,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>>>          env->cache_info_amd.l3_cache = &legacy_l3_cache;
>>>      }
>>>  
>>> +    /* Process Hyper-V enlightenments */
>>> +    x86_cpu_hyperv_realize(cpu);
>>>  
>>>      cpu_exec_realizefn(cs, &local_err);
>>>      if (local_err != NULL) {
>>> @@ -7198,7 +7219,7 @@ static Property x86_cpu_properties[] = {
>>>      DEFINE_PROP_UINT32("min-xlevel2", X86CPU, env.cpuid_min_xlevel2, 0),
>>>      DEFINE_PROP_UINT64("ucode-rev", X86CPU, ucode_rev, 0),
>>>      DEFINE_PROP_BOOL("full-cpuid-auto-level", X86CPU, full_cpuid_auto_level, true),
>>> -    DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor_id),
>>> +    DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor),
>>>      DEFINE_PROP_BOOL("cpuid-0xb", X86CPU, enable_cpuid_0xb, true),
>>>      DEFINE_PROP_BOOL("lmce", X86CPU, enable_lmce, false),
>>>      DEFINE_PROP_BOOL("l3-cache", X86CPU, enable_l3_cache, true),
>>> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
>>> index 88e8586f8fb4..be640bf45c29 100644
>>> --- a/target/i386/cpu.h
>>> +++ b/target/i386/cpu.h
>>> @@ -1655,11 +1655,12 @@ struct X86CPU {
>>>      uint64_t ucode_rev;
>>>  
>>>      uint32_t hyperv_spinlock_attempts;
>>> -    char *hyperv_vendor_id;
>>> +    char *hyperv_vendor;
>>>      bool hyperv_synic_kvm_only;
>>>      uint64_t hyperv_features;
>>>      bool hyperv_passthrough;
>>>      OnOffAuto hyperv_no_nonarch_cs;
>>> +    uint32_t hyperv_vendor_id[3];
>>>  
>>>      bool check_cpuid;
>>>      bool enforce_cpuid;
>>> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
>>> index a2934dda027c..788a2cf2ec51 100644
>>> --- a/target/i386/kvm.c
>>> +++ b/target/i386/kvm.c
>>> @@ -1205,6 +1205,13 @@ static int hyperv_handle_properties(CPUState *cs,
>>>          memcpy(cpuid_ent, &cpuid->entries[0],
>>>                 cpuid->nent * sizeof(cpuid->entries[0]));
>>>  
>>> +        c = cpuid_find_entry(cpuid, HV_CPUID_VENDOR_AND_MAX_FUNCTIONS, 0);
>>> +        if (c) {
>>> +            cpu->hyperv_vendor_id[0] = c->ebx;
>>> +            cpu->hyperv_vendor_id[1] = c->ecx;
>>> +            cpu->hyperv_vendor_id[2] = c->edx;
>>> +        }
>>> +
>>>          c = cpuid_find_entry(cpuid, HV_CPUID_FEATURES, 0);
>>>          if (c) {
>>>              env->features[FEAT_HYPERV_EAX] = c->eax;
>>> @@ -1279,23 +1286,11 @@ static int hyperv_handle_properties(CPUState *cs,
>>>  
>>>      c = &cpuid_ent[cpuid_i++];
>>>      c->function = HV_CPUID_VENDOR_AND_MAX_FUNCTIONS;
>>> -    if (!cpu->hyperv_vendor_id) {
>>> -        memcpy(signature, "Microsoft Hv", 12);
>>> -    } else {
>>> -        size_t len = strlen(cpu->hyperv_vendor_id);
>>> -
>>> -        if (len > 12) {
>>> -            error_report("hv-vendor-id truncated to 12 characters");
>>> -            len = 12;
>>> -        }
>>> -        memset(signature, 0, 12);
>>> -        memcpy(signature, cpu->hyperv_vendor_id, len);
>>> -    }
>>>      c->eax = hyperv_feat_enabled(cpu, HYPERV_FEAT_EVMCS) ?
>>>          HV_CPUID_NESTED_FEATURES : HV_CPUID_IMPLEMENT_LIMITS;
>>> -    c->ebx = signature[0];
>>> -    c->ecx = signature[1];
>>> -    c->edx = signature[2];
>>> +    c->ebx = cpu->hyperv_vendor_id[0];
>>> +    c->ecx = cpu->hyperv_vendor_id[1];
>>> +    c->edx = cpu->hyperv_vendor_id[2];
>>>  
>>>      c = &cpuid_ent[cpuid_i++];
>>>      c->function = HV_CPUID_INTERFACE;
>>>
>>
>