Claudio Fontana <cfont...@suse.de> writes: > On 3/10/21 12:43 PM, Vitaly Kuznetsov wrote: >> Claudio Fontana <cfont...@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 <vkuzn...@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
