| Message ID | 20181126135958.20956-1-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i386/kvm: expose HV_CPUID_ENLIGHTMENT_INFO.EAX and HV_CPUID_NESTED_FEATURES.EAX as feature words | expand |
On Mon, Nov 26, 2018 at 02:59:58PM +0100, Vitaly Kuznetsov wrote: > It was found that QMP users of QEMU (e.g. libvirt) may need > HV_CPUID_ENLIGHTMENT_INFO.EAX/HV_CPUID_NESTED_FEATURES.EAX information. In > particular, 'hv_tlbflush' and 'hv_evmcs' enlightenments are only exposed in > HV_CPUID_ENLIGHTMENT_INFO.EAX. > > HV_CPUID_NESTED_FEATURES.EAX is exposed for two reasons: convenience > (we don't need to export it from hyperv_handle_properties() and as > future-proof for Enlightened MSR-Bitmap, PV EPT invalidation and > direct virtual flush features. > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > --- > target/i386/cpu.c | 30 +++++++++++++++++ > target/i386/cpu.h | 2 ++ > target/i386/kvm.c | 85 +++++++++++++++++++++++++---------------------- > 3 files changed, 77 insertions(+), 40 deletions(-) Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
On 26/11/18 14:59, Vitaly Kuznetsov wrote: > It was found that QMP users of QEMU (e.g. libvirt) may need > HV_CPUID_ENLIGHTMENT_INFO.EAX/HV_CPUID_NESTED_FEATURES.EAX information. In > particular, 'hv_tlbflush' and 'hv_evmcs' enlightenments are only exposed in > HV_CPUID_ENLIGHTMENT_INFO.EAX. > > HV_CPUID_NESTED_FEATURES.EAX is exposed for two reasons: convenience > (we don't need to export it from hyperv_handle_properties() and as > future-proof for Enlightened MSR-Bitmap, PV EPT invalidation and > direct virtual flush features. > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> Can you add a comment to feature_word_info, explaining why the feat_names are not set? Thanks, Paolo > --- > target/i386/cpu.c | 30 +++++++++++++++++ > target/i386/cpu.h | 2 ++ > target/i386/kvm.c | 85 +++++++++++++++++++++++++---------------------- > 3 files changed, 77 insertions(+), 40 deletions(-) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index f81d35e1f9..8306670e09 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -980,6 +980,36 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > }, > .cpuid = { .eax = 0x40000003, .reg = R_EDX, }, > }, > + [FEAT_HV_RECOMM_EAX] = { > + .type = CPUID_FEATURE_WORD, > + .feat_names = { > + NULL /* hv_recommend_pv_as_switch */, > + NULL /* hv_recommend_pv_tlbflush_local */, > + NULL /* hv_recommend_pv_tlbflush_remote */, > + NULL /* hv_recommend_msr_apic_access */, > + NULL /* hv_recommend_msr_reset */, > + NULL /* hv_recommend_relaxed_timing */, > + NULL /* hv_recommend_dma_remapping */, > + NULL /* hv_recommend_int_remapping */, > + NULL /* hv_recommend_x2apic_msrs */, > + NULL /* hv_recommend_autoeoi_deprecation */, > + NULL /* hv_recommend_pv_ipi */, > + NULL /* hv_recommend_ex_hypercalls */, > + NULL /* hv_hypervisor_is_nested */, > + NULL /* hv_recommend_int_mbec */, > + NULL /* hv_recommend_evmcs */, > + NULL, > + NULL, NULL, NULL, NULL, > + NULL, NULL, NULL, NULL, > + NULL, NULL, NULL, NULL, > + NULL, NULL, NULL, NULL, > + }, > + .cpuid = { .eax = 0x40000004, .reg = R_EAX, }, > + }, > + [FEAT_HV_NESTED_EAX] = { > + .type = CPUID_FEATURE_WORD, > + .cpuid = { .eax = 0x4000000A, .reg = R_EAX, }, > + }, > [FEAT_SVM] = { > .type = CPUID_FEATURE_WORD, > .feat_names = { > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > index 9c52d0cbeb..dd881510ac 100644 > --- a/target/i386/cpu.h > +++ b/target/i386/cpu.h > @@ -497,6 +497,8 @@ typedef enum FeatureWord { > FEAT_HYPERV_EAX, /* CPUID[4000_0003].EAX */ > FEAT_HYPERV_EBX, /* CPUID[4000_0003].EBX */ > FEAT_HYPERV_EDX, /* CPUID[4000_0003].EDX */ > + FEAT_HV_RECOMM_EAX, /* CPUID[4000_0004].EAX */ > + FEAT_HV_NESTED_EAX, /* CPUID[4000_000A].EAX */ > FEAT_SVM, /* CPUID[8000_000A].EDX */ > FEAT_XSAVE, /* CPUID[EAX=0xd,ECX=1].EAX */ > FEAT_6_EAX, /* CPUID[6].EAX */ > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > index f524e7d929..b4d2b40a40 100644 > --- a/target/i386/kvm.c > +++ b/target/i386/kvm.c > @@ -797,6 +797,48 @@ static int hyperv_handle_properties(CPUState *cs) > } > env->features[FEAT_HYPERV_EAX] |= HV_SYNTIMERS_AVAILABLE; > } > + if (cpu->hyperv_relaxed_timing) { > + env->features[FEAT_HV_RECOMM_EAX] |= HV_RELAXED_TIMING_RECOMMENDED; > + } > + if (cpu->hyperv_vapic) { > + env->features[FEAT_HV_RECOMM_EAX] |= HV_APIC_ACCESS_RECOMMENDED; > + } > + if (cpu->hyperv_tlbflush) { > + if (kvm_check_extension(cs->kvm_state, > + KVM_CAP_HYPERV_TLBFLUSH) <= 0) { > + fprintf(stderr, "Hyper-V TLB flush support " > + "(requested by 'hv-tlbflush' cpu flag) " > + " is not supported by kernel\n"); > + return -ENOSYS; > + } > + env->features[FEAT_HV_RECOMM_EAX] |= HV_REMOTE_TLB_FLUSH_RECOMMENDED; > + env->features[FEAT_HV_RECOMM_EAX] |= HV_EX_PROCESSOR_MASKS_RECOMMENDED; > + } > + if (cpu->hyperv_ipi) { > + if (kvm_check_extension(cs->kvm_state, > + KVM_CAP_HYPERV_SEND_IPI) <= 0) { > + fprintf(stderr, "Hyper-V IPI send support " > + "(requested by 'hv-ipi' cpu flag) " > + " is not supported by kernel\n"); > + return -ENOSYS; > + } > + env->features[FEAT_HV_RECOMM_EAX] |= HV_CLUSTER_IPI_RECOMMENDED; > + env->features[FEAT_HV_RECOMM_EAX] |= HV_EX_PROCESSOR_MASKS_RECOMMENDED; > + } > + if (cpu->hyperv_evmcs) { > + uint16_t evmcs_version; > + > + if (kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_ENLIGHTENED_VMCS, 0, > + (uintptr_t)&evmcs_version)) { > + fprintf(stderr, "Hyper-V Enlightened VMCS " > + "(requested by 'hv-evmcs' cpu flag) " > + "is not supported by kernel\n"); > + return -ENOSYS; > + } > + env->features[FEAT_HV_RECOMM_EAX] |= HV_ENLIGHTENED_VMCS_RECOMMENDED; > + env->features[FEAT_HV_NESTED_EAX] = evmcs_version; > + } > + > return 0; > } > > @@ -869,7 +911,6 @@ int kvm_arch_init_vcpu(CPUState *cs) > uint32_t unused; > struct kvm_cpuid_entry2 *c; > uint32_t signature[3]; > - uint16_t evmcs_version; > int kvm_base = KVM_CPUID_SIGNATURE; > int r; > Error *local_err = NULL; > @@ -944,44 +985,8 @@ int kvm_arch_init_vcpu(CPUState *cs) > > c = &cpuid_data.entries[cpuid_i++]; > c->function = HV_CPUID_ENLIGHTMENT_INFO; > - if (cpu->hyperv_relaxed_timing) { > - c->eax |= HV_RELAXED_TIMING_RECOMMENDED; > - } > - if (cpu->hyperv_vapic) { > - c->eax |= HV_APIC_ACCESS_RECOMMENDED; > - } > - if (cpu->hyperv_tlbflush) { > - if (kvm_check_extension(cs->kvm_state, > - KVM_CAP_HYPERV_TLBFLUSH) <= 0) { > - fprintf(stderr, "Hyper-V TLB flush support " > - "(requested by 'hv-tlbflush' cpu flag) " > - " is not supported by kernel\n"); > - return -ENOSYS; > - } > - c->eax |= HV_REMOTE_TLB_FLUSH_RECOMMENDED; > - c->eax |= HV_EX_PROCESSOR_MASKS_RECOMMENDED; > - } > - if (cpu->hyperv_ipi) { > - if (kvm_check_extension(cs->kvm_state, > - KVM_CAP_HYPERV_SEND_IPI) <= 0) { > - fprintf(stderr, "Hyper-V IPI send support " > - "(requested by 'hv-ipi' cpu flag) " > - " is not supported by kernel\n"); > - return -ENOSYS; > - } > - c->eax |= HV_CLUSTER_IPI_RECOMMENDED; > - c->eax |= HV_EX_PROCESSOR_MASKS_RECOMMENDED; > - } > - if (cpu->hyperv_evmcs) { > - if (kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_ENLIGHTENED_VMCS, 0, > - (uintptr_t)&evmcs_version)) { > - fprintf(stderr, "Hyper-V Enlightened VMCS " > - "(requested by 'hv-evmcs' cpu flag) " > - "is not supported by kernel\n"); > - return -ENOSYS; > - } > - c->eax |= HV_ENLIGHTENED_VMCS_RECOMMENDED; > - } > + > + c->eax = env->features[FEAT_HV_RECOMM_EAX]; > c->ebx = cpu->hyperv_spinlock_attempts; > > c = &cpuid_data.entries[cpuid_i++]; > @@ -1005,7 +1010,7 @@ int kvm_arch_init_vcpu(CPUState *cs) > > c = &cpuid_data.entries[cpuid_i++]; > c->function = HV_CPUID_NESTED_FEATURES; > - c->eax = evmcs_version; > + c->eax = env->features[FEAT_HV_NESTED_EAX]; > } > } > >
Paolo Bonzini <pbonzini@redhat.com> writes: > On 26/11/18 14:59, Vitaly Kuznetsov wrote: >> It was found that QMP users of QEMU (e.g. libvirt) may need >> HV_CPUID_ENLIGHTMENT_INFO.EAX/HV_CPUID_NESTED_FEATURES.EAX information. In >> particular, 'hv_tlbflush' and 'hv_evmcs' enlightenments are only exposed in >> HV_CPUID_ENLIGHTMENT_INFO.EAX. >> >> HV_CPUID_NESTED_FEATURES.EAX is exposed for two reasons: convenience >> (we don't need to export it from hyperv_handle_properties() and as >> future-proof for Enlightened MSR-Bitmap, PV EPT invalidation and >> direct virtual flush features. >> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > Can you add a comment to feature_word_info, explaining why the > feat_names are not set? I had to do some code archeology to make sure I understand, I think it goes back to http://lists.gnu.org/archive/html/qemu-devel/2016-06/msg06579.html So the comment (probably added before FEAT_HYPERV_EAX definition) would be ".feat_names are commented out for Hyper-V enlightenments because we don't want to have two different ways for enabling them on QEMU command line. Some features (e.g. "hyperv_time", "hyperv_vapic", ...) require enabling several feature bits simultaneously, exposing these bits individually may just confuse guests." Would do?
On Thu, Nov 29, 2018 at 12:51:55PM +0100, Vitaly Kuznetsov wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > > > On 26/11/18 14:59, Vitaly Kuznetsov wrote: > >> It was found that QMP users of QEMU (e.g. libvirt) may need > >> HV_CPUID_ENLIGHTMENT_INFO.EAX/HV_CPUID_NESTED_FEATURES.EAX information. In > >> particular, 'hv_tlbflush' and 'hv_evmcs' enlightenments are only exposed in > >> HV_CPUID_ENLIGHTMENT_INFO.EAX. > >> > >> HV_CPUID_NESTED_FEATURES.EAX is exposed for two reasons: convenience > >> (we don't need to export it from hyperv_handle_properties() and as > >> future-proof for Enlightened MSR-Bitmap, PV EPT invalidation and > >> direct virtual flush features. > >> > >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > > > Can you add a comment to feature_word_info, explaining why the > > feat_names are not set? > > I had to do some code archeology to make sure I understand, I think it > goes back to > > http://lists.gnu.org/archive/html/qemu-devel/2016-06/msg06579.html > > So the comment (probably added before FEAT_HYPERV_EAX definition) would > be > > ".feat_names are commented out for Hyper-V enlightenments because we > don't want to have two different ways for enabling them on QEMU command > line. Some features (e.g. "hyperv_time", "hyperv_vapic", ...) require > enabling several feature bits simultaneously, exposing these bits > individually may just confuse guests." > > Would do? That's an accurate description. But note that we might still be able to move the existing "hyperv_*" features to feature_word_info[].feat_names. We just need to keep the same semantics (e.g. enable HV_HYPERCALL_AVAILABLE automatically when some features are set). Maybe we can make some of the feature properties read-only. This way we can give them meaningful names for debugging and error messages, even if we don't want to make them configurable directly.
Eduardo Habkost <ehabkost@redhat.com> writes: > On Thu, Nov 29, 2018 at 12:51:55PM +0100, Vitaly Kuznetsov wrote: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >> > On 26/11/18 14:59, Vitaly Kuznetsov wrote: >> >> It was found that QMP users of QEMU (e.g. libvirt) may need >> >> HV_CPUID_ENLIGHTMENT_INFO.EAX/HV_CPUID_NESTED_FEATURES.EAX information. In >> >> particular, 'hv_tlbflush' and 'hv_evmcs' enlightenments are only exposed in >> >> HV_CPUID_ENLIGHTMENT_INFO.EAX. >> >> >> >> HV_CPUID_NESTED_FEATURES.EAX is exposed for two reasons: convenience >> >> (we don't need to export it from hyperv_handle_properties() and as >> >> future-proof for Enlightened MSR-Bitmap, PV EPT invalidation and >> >> direct virtual flush features. >> >> >> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> >> > >> > Can you add a comment to feature_word_info, explaining why the >> > feat_names are not set? >> >> I had to do some code archeology to make sure I understand, I think it >> goes back to >> >> http://lists.gnu.org/archive/html/qemu-devel/2016-06/msg06579.html >> >> So the comment (probably added before FEAT_HYPERV_EAX definition) would >> be >> >> ".feat_names are commented out for Hyper-V enlightenments because we >> don't want to have two different ways for enabling them on QEMU command >> line. Some features (e.g. "hyperv_time", "hyperv_vapic", ...) require >> enabling several feature bits simultaneously, exposing these bits >> individually may just confuse guests." >> >> Would do? > > That's an accurate description. > Thanks, I'll send v2 out with it. > But note that we might still be able to move the existing > "hyperv_*" features to feature_word_info[].feat_names. We just > need to keep the same semantics (e.g. enable > HV_HYPERCALL_AVAILABLE automatically when some features are set). > > Maybe we can make some of the feature properties read-only. This > way we can give them meaningful names for debugging and error > messages, even if we don't want to make them configurable > directly. I'd suggest (if there are no objections of course) we do this separately from this patch. Some time ago when merging direct mode stimers for KVM Paolo suggested we stop adding capabilities to KVM for each individulat feature and replace them with something like KVM_GET_SUPPORTED_HV_CPUID ioctl returning all Hyper-V related feature words. When this is done we can reconsider how Qemu discoveres Hyper-V related KVM features and as part of this work we can take a closer look at feature words and feat_names.
On Mon, Dec 03, 2018 at 03:17:06PM +0100, Vitaly Kuznetsov wrote: > Eduardo Habkost <ehabkost@redhat.com> writes: [...] > > But note that we might still be able to move the existing > > "hyperv_*" features to feature_word_info[].feat_names. We just > > need to keep the same semantics (e.g. enable > > HV_HYPERCALL_AVAILABLE automatically when some features are set). > > > > Maybe we can make some of the feature properties read-only. This > > way we can give them meaningful names for debugging and error > > messages, even if we don't want to make them configurable > > directly. > > I'd suggest (if there are no objections of course) we do this separately > from this patch. [...] Agreed. > [...] Some time ago when merging direct mode stimers for KVM > Paolo suggested we stop adding capabilities to KVM for each individulat > feature and replace them with something like KVM_GET_SUPPORTED_HV_CPUID > ioctl returning all Hyper-V related feature words. When this is done we > can reconsider how Qemu discoveres Hyper-V related KVM features and as > part of this work we can take a closer look at feature words and > feat_names. Why a separate ioctl instead of extending GET_SUPPORTED_CPUID?
Eduardo Habkost <ehabkost@redhat.com> writes: >> [...] Some time ago when merging direct mode stimers for KVM >> Paolo suggested we stop adding capabilities to KVM for each individulat >> feature and replace them with something like KVM_GET_SUPPORTED_HV_CPUID >> ioctl returning all Hyper-V related feature words. When this is done we >> can reconsider how Qemu discoveres Hyper-V related KVM features and as >> part of this work we can take a closer look at feature words and >> feat_names. > > Why a separate ioctl instead of extending GET_SUPPORTED_CPUID? Unfortunatelly both KVM and Hyper-V use feature leaves 0x40000000, 0x40000001 (so it's up to the userspace - qemu in our case - what to expose to the guest) and GET_SUPPORTED_CPUID already returns KVM's. Not sure this can be changed (to e.g. returning these leaves twice with different flags) without breaking userspace. New ioctl is safer.
Eduardo Habkost <ehabkost@redhat.com> writes: > On Mon, Dec 03, 2018 at 03:17:06PM +0100, Vitaly Kuznetsov wrote: >> Eduardo Habkost <ehabkost@redhat.com> writes: > [...] >> > But note that we might still be able to move the existing >> > "hyperv_*" features to feature_word_info[].feat_names. We just >> > need to keep the same semantics (e.g. enable >> > HV_HYPERCALL_AVAILABLE automatically when some features are set). >> > >> > Maybe we can make some of the feature properties read-only. This >> > way we can give them meaningful names for debugging and error >> > messages, even if we don't want to make them configurable >> > directly. >> >> I'd suggest (if there are no objections of course) we do this separately >> from this patch. [...] > > Agreed. > Paolo, Eduardo, in case there are no concerns here, could you please pick this patch up? Thanks!
On Wed, Dec 19, 2018 at 06:25:06PM +0100, Vitaly Kuznetsov wrote: > Eduardo Habkost <ehabkost@redhat.com> writes: > > > On Mon, Dec 03, 2018 at 03:17:06PM +0100, Vitaly Kuznetsov wrote: > >> Eduardo Habkost <ehabkost@redhat.com> writes: > > [...] > >> > But note that we might still be able to move the existing > >> > "hyperv_*" features to feature_word_info[].feat_names. We just > >> > need to keep the same semantics (e.g. enable > >> > HV_HYPERCALL_AVAILABLE automatically when some features are set). > >> > > >> > Maybe we can make some of the feature properties read-only. This > >> > way we can give them meaningful names for debugging and error > >> > messages, even if we don't want to make them configurable > >> > directly. > >> > >> I'd suggest (if there are no objections of course) we do this separately > >> from this patch. [...] > > > > Agreed. > > > > Paolo, Eduardo, > > in case there are no concerns here, could you please pick this patch up? > Thanks! Queued, thanks! Can you please send the comment you wrote about feat_names as a follow-up patch?
Eduardo Habkost <ehabkost@redhat.com> writes: > On Wed, Dec 19, 2018 at 06:25:06PM +0100, Vitaly Kuznetsov wrote: >> Eduardo Habkost <ehabkost@redhat.com> writes: >> >> > On Mon, Dec 03, 2018 at 03:17:06PM +0100, Vitaly Kuznetsov wrote: >> >> Eduardo Habkost <ehabkost@redhat.com> writes: >> > [...] >> >> > But note that we might still be able to move the existing >> >> > "hyperv_*" features to feature_word_info[].feat_names. We just >> >> > need to keep the same semantics (e.g. enable >> >> > HV_HYPERCALL_AVAILABLE automatically when some features are set). >> >> > >> >> > Maybe we can make some of the feature properties read-only. This >> >> > way we can give them meaningful names for debugging and error >> >> > messages, even if we don't want to make them configurable >> >> > directly. >> >> >> >> I'd suggest (if there are no objections of course) we do this separately >> >> from this patch. [...] >> > >> > Agreed. >> > >> >> Paolo, Eduardo, >> >> in case there are no concerns here, could you please pick this patch up? >> Thanks! > > Queued, thanks! > > Can you please send the comment you wrote about feat_names as a > follow-up patch? Oops, sorry, I just realized I promissed to send out v2 with it and aparently never did. Will send out a follow-up patch shortly. Thanks!
Vitaly Kuznetsov <vkuznets@redhat.com> writes: > Eduardo Habkost <ehabkost@redhat.com> writes: > >> On Wed, Dec 19, 2018 at 06:25:06PM +0100, Vitaly Kuznetsov wrote: >>> Eduardo Habkost <ehabkost@redhat.com> writes: >>> >>> > On Mon, Dec 03, 2018 at 03:17:06PM +0100, Vitaly Kuznetsov wrote: >>> >> Eduardo Habkost <ehabkost@redhat.com> writes: >>> > [...] >>> >> > But note that we might still be able to move the existing >>> >> > "hyperv_*" features to feature_word_info[].feat_names. We just >>> >> > need to keep the same semantics (e.g. enable >>> >> > HV_HYPERCALL_AVAILABLE automatically when some features are set). >>> >> > >>> >> > Maybe we can make some of the feature properties read-only. This >>> >> > way we can give them meaningful names for debugging and error >>> >> > messages, even if we don't want to make them configurable >>> >> > directly. >>> >> >>> >> I'd suggest (if there are no objections of course) we do this separately >>> >> from this patch. [...] >>> > >>> > Agreed. >>> > >>> >>> Paolo, Eduardo, >>> >>> in case there are no concerns here, could you please pick this patch up? >>> Thanks! >> >> Queued, thanks! >> >> Can you please send the comment you wrote about feat_names as a >> follow-up patch? > > Oops, sorry, I just realized I promissed to send out v2 with it and > aparently never did. Will send out a follow-up patch shortly. Thanks! Hey Eduardo, any news about the fate of this patch? (Correcting myself: there was v2 with the comment included: https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg00355.html but as I sent the follow-up patch you requested separately too: https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg05463.html )
On Mon, Jan 14, 2019 at 11:54:30AM +0100, Vitaly Kuznetsov wrote: > Vitaly Kuznetsov <vkuznets@redhat.com> writes: > > > Eduardo Habkost <ehabkost@redhat.com> writes: > > > >> On Wed, Dec 19, 2018 at 06:25:06PM +0100, Vitaly Kuznetsov wrote: > >>> Eduardo Habkost <ehabkost@redhat.com> writes: > >>> > >>> > On Mon, Dec 03, 2018 at 03:17:06PM +0100, Vitaly Kuznetsov wrote: > >>> >> Eduardo Habkost <ehabkost@redhat.com> writes: > >>> > [...] > >>> >> > But note that we might still be able to move the existing > >>> >> > "hyperv_*" features to feature_word_info[].feat_names. We just > >>> >> > need to keep the same semantics (e.g. enable > >>> >> > HV_HYPERCALL_AVAILABLE automatically when some features are set). > >>> >> > > >>> >> > Maybe we can make some of the feature properties read-only. This > >>> >> > way we can give them meaningful names for debugging and error > >>> >> > messages, even if we don't want to make them configurable > >>> >> > directly. > >>> >> > >>> >> I'd suggest (if there are no objections of course) we do this separately > >>> >> from this patch. [...] > >>> > > >>> > Agreed. > >>> > > >>> > >>> Paolo, Eduardo, > >>> > >>> in case there are no concerns here, could you please pick this patch up? > >>> Thanks! > >> > >> Queued, thanks! > >> > >> Can you please send the comment you wrote about feat_names as a > >> follow-up patch? > > > > Oops, sorry, I just realized I promissed to send out v2 with it and > > aparently never did. Will send out a follow-up patch shortly. Thanks! > > Hey Eduardo, > > any news about the fate of this patch? > > (Correcting myself: there was v2 with the comment included: > https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg00355.html > > but as I sent the follow-up patch you requested separately too: > https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg05463.html > ) Patch was queued but I took too long to send a pull request, sorry. Pull request is being sent right now.
diff --git a/target/i386/cpu.c b/target/i386/cpu.c index f81d35e1f9..8306670e09 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -980,6 +980,36 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { }, .cpuid = { .eax = 0x40000003, .reg = R_EDX, }, }, + [FEAT_HV_RECOMM_EAX] = { + .type = CPUID_FEATURE_WORD, + .feat_names = { + NULL /* hv_recommend_pv_as_switch */, + NULL /* hv_recommend_pv_tlbflush_local */, + NULL /* hv_recommend_pv_tlbflush_remote */, + NULL /* hv_recommend_msr_apic_access */, + NULL /* hv_recommend_msr_reset */, + NULL /* hv_recommend_relaxed_timing */, + NULL /* hv_recommend_dma_remapping */, + NULL /* hv_recommend_int_remapping */, + NULL /* hv_recommend_x2apic_msrs */, + NULL /* hv_recommend_autoeoi_deprecation */, + NULL /* hv_recommend_pv_ipi */, + NULL /* hv_recommend_ex_hypercalls */, + NULL /* hv_hypervisor_is_nested */, + NULL /* hv_recommend_int_mbec */, + NULL /* hv_recommend_evmcs */, + NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + }, + .cpuid = { .eax = 0x40000004, .reg = R_EAX, }, + }, + [FEAT_HV_NESTED_EAX] = { + .type = CPUID_FEATURE_WORD, + .cpuid = { .eax = 0x4000000A, .reg = R_EAX, }, + }, [FEAT_SVM] = { .type = CPUID_FEATURE_WORD, .feat_names = { diff --git a/target/i386/cpu.h b/target/i386/cpu.h index 9c52d0cbeb..dd881510ac 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -497,6 +497,8 @@ typedef enum FeatureWord { FEAT_HYPERV_EAX, /* CPUID[4000_0003].EAX */ FEAT_HYPERV_EBX, /* CPUID[4000_0003].EBX */ FEAT_HYPERV_EDX, /* CPUID[4000_0003].EDX */ + FEAT_HV_RECOMM_EAX, /* CPUID[4000_0004].EAX */ + FEAT_HV_NESTED_EAX, /* CPUID[4000_000A].EAX */ FEAT_SVM, /* CPUID[8000_000A].EDX */ FEAT_XSAVE, /* CPUID[EAX=0xd,ECX=1].EAX */ FEAT_6_EAX, /* CPUID[6].EAX */ diff --git a/target/i386/kvm.c b/target/i386/kvm.c index f524e7d929..b4d2b40a40 100644 --- a/target/i386/kvm.c +++ b/target/i386/kvm.c @@ -797,6 +797,48 @@ static int hyperv_handle_properties(CPUState *cs) } env->features[FEAT_HYPERV_EAX] |= HV_SYNTIMERS_AVAILABLE; } + if (cpu->hyperv_relaxed_timing) { + env->features[FEAT_HV_RECOMM_EAX] |= HV_RELAXED_TIMING_RECOMMENDED; + } + if (cpu->hyperv_vapic) { + env->features[FEAT_HV_RECOMM_EAX] |= HV_APIC_ACCESS_RECOMMENDED; + } + if (cpu->hyperv_tlbflush) { + if (kvm_check_extension(cs->kvm_state, + KVM_CAP_HYPERV_TLBFLUSH) <= 0) { + fprintf(stderr, "Hyper-V TLB flush support " + "(requested by 'hv-tlbflush' cpu flag) " + " is not supported by kernel\n"); + return -ENOSYS; + } + env->features[FEAT_HV_RECOMM_EAX] |= HV_REMOTE_TLB_FLUSH_RECOMMENDED; + env->features[FEAT_HV_RECOMM_EAX] |= HV_EX_PROCESSOR_MASKS_RECOMMENDED; + } + if (cpu->hyperv_ipi) { + if (kvm_check_extension(cs->kvm_state, + KVM_CAP_HYPERV_SEND_IPI) <= 0) { + fprintf(stderr, "Hyper-V IPI send support " + "(requested by 'hv-ipi' cpu flag) " + " is not supported by kernel\n"); + return -ENOSYS; + } + env->features[FEAT_HV_RECOMM_EAX] |= HV_CLUSTER_IPI_RECOMMENDED; + env->features[FEAT_HV_RECOMM_EAX] |= HV_EX_PROCESSOR_MASKS_RECOMMENDED; + } + if (cpu->hyperv_evmcs) { + uint16_t evmcs_version; + + if (kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_ENLIGHTENED_VMCS, 0, + (uintptr_t)&evmcs_version)) { + fprintf(stderr, "Hyper-V Enlightened VMCS " + "(requested by 'hv-evmcs' cpu flag) " + "is not supported by kernel\n"); + return -ENOSYS; + } + env->features[FEAT_HV_RECOMM_EAX] |= HV_ENLIGHTENED_VMCS_RECOMMENDED; + env->features[FEAT_HV_NESTED_EAX] = evmcs_version; + } + return 0; } @@ -869,7 +911,6 @@ int kvm_arch_init_vcpu(CPUState *cs) uint32_t unused; struct kvm_cpuid_entry2 *c; uint32_t signature[3]; - uint16_t evmcs_version; int kvm_base = KVM_CPUID_SIGNATURE; int r; Error *local_err = NULL; @@ -944,44 +985,8 @@ int kvm_arch_init_vcpu(CPUState *cs) c = &cpuid_data.entries[cpuid_i++]; c->function = HV_CPUID_ENLIGHTMENT_INFO; - if (cpu->hyperv_relaxed_timing) { - c->eax |= HV_RELAXED_TIMING_RECOMMENDED; - } - if (cpu->hyperv_vapic) { - c->eax |= HV_APIC_ACCESS_RECOMMENDED; - } - if (cpu->hyperv_tlbflush) { - if (kvm_check_extension(cs->kvm_state, - KVM_CAP_HYPERV_TLBFLUSH) <= 0) { - fprintf(stderr, "Hyper-V TLB flush support " - "(requested by 'hv-tlbflush' cpu flag) " - " is not supported by kernel\n"); - return -ENOSYS; - } - c->eax |= HV_REMOTE_TLB_FLUSH_RECOMMENDED; - c->eax |= HV_EX_PROCESSOR_MASKS_RECOMMENDED; - } - if (cpu->hyperv_ipi) { - if (kvm_check_extension(cs->kvm_state, - KVM_CAP_HYPERV_SEND_IPI) <= 0) { - fprintf(stderr, "Hyper-V IPI send support " - "(requested by 'hv-ipi' cpu flag) " - " is not supported by kernel\n"); - return -ENOSYS; - } - c->eax |= HV_CLUSTER_IPI_RECOMMENDED; - c->eax |= HV_EX_PROCESSOR_MASKS_RECOMMENDED; - } - if (cpu->hyperv_evmcs) { - if (kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_ENLIGHTENED_VMCS, 0, - (uintptr_t)&evmcs_version)) { - fprintf(stderr, "Hyper-V Enlightened VMCS " - "(requested by 'hv-evmcs' cpu flag) " - "is not supported by kernel\n"); - return -ENOSYS; - } - c->eax |= HV_ENLIGHTENED_VMCS_RECOMMENDED; - } + + c->eax = env->features[FEAT_HV_RECOMM_EAX]; c->ebx = cpu->hyperv_spinlock_attempts; c = &cpuid_data.entries[cpuid_i++]; @@ -1005,7 +1010,7 @@ int kvm_arch_init_vcpu(CPUState *cs) c = &cpuid_data.entries[cpuid_i++]; c->function = HV_CPUID_NESTED_FEATURES; - c->eax = evmcs_version; + c->eax = env->features[FEAT_HV_NESTED_EAX]; } }
It was found that QMP users of QEMU (e.g. libvirt) may need HV_CPUID_ENLIGHTMENT_INFO.EAX/HV_CPUID_NESTED_FEATURES.EAX information. In particular, 'hv_tlbflush' and 'hv_evmcs' enlightenments are only exposed in HV_CPUID_ENLIGHTMENT_INFO.EAX. HV_CPUID_NESTED_FEATURES.EAX is exposed for two reasons: convenience (we don't need to export it from hyperv_handle_properties() and as future-proof for Enlightened MSR-Bitmap, PV EPT invalidation and direct virtual flush features. Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- target/i386/cpu.c | 30 +++++++++++++++++ target/i386/cpu.h | 2 ++ target/i386/kvm.c | 85 +++++++++++++++++++++++++---------------------- 3 files changed, 77 insertions(+), 40 deletions(-)