[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/5] i386: move hyperv_vendor_id initialization to x86_cpu_re
From: |
Vitaly Kuznetsov |
Subject: |
Re: [PATCH 1/5] i386: move hyperv_vendor_id initialization to x86_cpu_realizefn() |
Date: |
Wed, 10 Mar 2021 14:13:59 +0100 |
Claudio Fontana <cfontana@suse.de> writes:
> 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?
>
Right, not without introducing a custom parser checking if KVM is in use.
>
>> 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?
>
In theory, I don't see why we can't emulate hyper-v in TCG, I'm just not
sure how practical that would be). There was a Windows Hypervisor
Platform accelerator (whpx) which uses genuine Hyper-V and can use
Hyper-V enlightenment option to set guest visible CPUIDs but honestly I
don't know much about whpx.
> 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 .
No problem, I wouldn't be against that. I'm waiting for some feedback on
v5 and if I am to do v6 I'll include that. In case I got luck and v5 is
to get merged, I can send a follow-up patch.
>
> 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;
>>>>
>>>
>>
>
--
Vitaly