Message ID | 1523953455-28053-1-git-send-email-wanpengli@tencent.com |
---|---|
State | New |
Headers | show |
Series | [RESEND,v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS | expand |
Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 1523953455-28053-1-git-send-email-wanpengli@tencent.com Subject: [Qemu-devel] [PATCH RESEND v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/1523953455-28053-1-git-send-email-wanpengli@tencent.com -> patchew/1523953455-28053-1-git-send-email-wanpengli@tencent.com Switched to a new branch 'test' 61dd49b0a1 i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS === OUTPUT BEGIN === Checking PATCH 1/1: i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS... WARNING: line over 80 characters #65: FILE: target/i386/kvm.c:1033: + int disable_exits = kvm_check_extension(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS); ERROR: line over 90 characters #75: FILE: target/i386/kvm.c:1043: + if (kvm_vm_enable_cap(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS, 0, disable_exits)) { total: 1 errors, 1 warnings, 48 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
On Tue, Apr 17, 2018 at 01:24:15AM -0700, Wanpeng Li wrote: > From: Wanpeng Li <wanpengli@tencent.com> > > This patch adds support for KVM_CAP_X86_DISABLE_EXITS. Provides userspace with > per-VM capability(KVM_CAP_X86_DISABLE_EXITS) to not intercept MWAIT/HLT/PAUSE > in order that to improve latency in some workloads. > > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Radim Krčmář <rkrcmar@redhat.com> > Cc: Eduardo Habkost <ehabkost@redhat.com> > Signed-off-by: Wanpeng Li <wanpengli@tencent.com> > --- > > linux-headers/linux/kvm.h | 6 +++++- > target/i386/cpu.h | 2 ++ > target/i386/kvm.c | 16 ++++++++++++++++ > 3 files changed, 23 insertions(+), 1 deletion(-) > > diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h > index a167be8..857df15 100644 > --- a/linux-headers/linux/kvm.h > +++ b/linux-headers/linux/kvm.h > @@ -925,7 +925,7 @@ struct kvm_ppc_resize_hpt { > #define KVM_CAP_S390_GS 140 > #define KVM_CAP_S390_AIS 141 > #define KVM_CAP_SPAPR_TCE_VFIO 142 > -#define KVM_CAP_X86_GUEST_MWAIT 143 > +#define KVM_CAP_X86_DISABLE_EXITS 143 > #define KVM_CAP_ARM_USER_IRQ 144 > #define KVM_CAP_S390_CMMA_MIGRATION 145 > #define KVM_CAP_PPC_FWNMI 146 > @@ -1508,6 +1508,10 @@ struct kvm_assigned_msix_entry { > #define KVM_X2APIC_API_USE_32BIT_IDS (1ULL << 0) > #define KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK (1ULL << 1) > > +#define KVM_X86_DISABLE_EXITS_MWAIT (1 << 0) > +#define KVM_X86_DISABLE_EXITS_HLT (1 << 1) > +#define KVM_X86_DISABLE_EXITS_PAUSE (1 << 2) > + > /* Available with KVM_CAP_ARM_USER_IRQ */ > > /* Bits for run->s.regs.device_irq_level */ > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > index 1b219fa..965de1b 100644 > --- a/target/i386/cpu.h > +++ b/target/i386/cpu.h > @@ -685,6 +685,8 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS]; > #define CPUID_7_0_EDX_AVX512_4FMAPS (1U << 3) /* AVX512 Multiply Accumulation Single Precision */ > #define CPUID_7_0_EDX_SPEC_CTRL (1U << 26) /* Speculation Control */ > > +#define KVM_PV_UNHALT (1U << 7) > + Why don't we use KVM_FEATURE_PV_UNHALT from kvm_para.h? > #define KVM_HINTS_DEDICATED (1U << 0) > BTW I wonder whether we should switch to a value from kvm_para.h? I'll send a version to do it, pls take a look. > #define CPUID_8000_0008_EBX_IBPB (1U << 12) /* Indirect Branch Prediction Barrier */ > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > index 6c49954..3e99830 100644 > --- a/target/i386/kvm.c > +++ b/target/i386/kvm.c > @@ -1029,6 +1029,22 @@ int kvm_arch_init_vcpu(CPUState *cs) > } > } > > + if (env->features[FEAT_KVM_HINTS] & KVM_HINTS_DEDICATED) { > + int disable_exits = kvm_check_extension(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS); > + > + if (disable_exits) { > + disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT | > + KVM_X86_DISABLE_EXITS_HLT | > + KVM_X86_DISABLE_EXITS_PAUSE); > + if (env->user_features[FEAT_KVM] & KVM_PV_UNHALT) { > + disable_exits &= ~KVM_X86_DISABLE_EXITS_HLT; > + } > + } > + if (kvm_vm_enable_cap(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS, 0, disable_exits)) { > + error_report("kvm: DISABLE EXITS not supported"); > + } > + } > + > qemu_add_vm_change_state_handler(cpu_update_state, env); > > c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0); > -- > 2.7.4
On Tue, Apr 17, 2018 at 01:24:15AM -0700, Wanpeng Li wrote: > From: Wanpeng Li <wanpengli@tencent.com> > > This patch adds support for KVM_CAP_X86_DISABLE_EXITS. Provides userspace with > per-VM capability(KVM_CAP_X86_DISABLE_EXITS) to not intercept MWAIT/HLT/PAUSE > in order that to improve latency in some workloads. > > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Radim Krčmář <rkrcmar@redhat.com> > Cc: Eduardo Habkost <ehabkost@redhat.com> > Signed-off-by: Wanpeng Li <wanpengli@tencent.com> > --- > > linux-headers/linux/kvm.h | 6 +++++- > target/i386/cpu.h | 2 ++ > target/i386/kvm.c | 16 ++++++++++++++++ > 3 files changed, 23 insertions(+), 1 deletion(-) > > diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h > index a167be8..857df15 100644 > --- a/linux-headers/linux/kvm.h > +++ b/linux-headers/linux/kvm.h > @@ -925,7 +925,7 @@ struct kvm_ppc_resize_hpt { > #define KVM_CAP_S390_GS 140 > #define KVM_CAP_S390_AIS 141 > #define KVM_CAP_SPAPR_TCE_VFIO 142 > -#define KVM_CAP_X86_GUEST_MWAIT 143 > +#define KVM_CAP_X86_DISABLE_EXITS 143 > #define KVM_CAP_ARM_USER_IRQ 144 > #define KVM_CAP_S390_CMMA_MIGRATION 145 > #define KVM_CAP_PPC_FWNMI 146 > @@ -1508,6 +1508,10 @@ struct kvm_assigned_msix_entry { > #define KVM_X2APIC_API_USE_32BIT_IDS (1ULL << 0) > #define KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK (1ULL << 1) > > +#define KVM_X86_DISABLE_EXITS_MWAIT (1 << 0) > +#define KVM_X86_DISABLE_EXITS_HLT (1 << 1) > +#define KVM_X86_DISABLE_EXITS_PAUSE (1 << 2) > + > /* Available with KVM_CAP_ARM_USER_IRQ */ > > /* Bits for run->s.regs.device_irq_level */ > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > index 1b219fa..965de1b 100644 > --- a/target/i386/cpu.h > +++ b/target/i386/cpu.h > @@ -685,6 +685,8 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS]; > #define CPUID_7_0_EDX_AVX512_4FMAPS (1U << 3) /* AVX512 Multiply Accumulation Single Precision */ > #define CPUID_7_0_EDX_SPEC_CTRL (1U << 26) /* Speculation Control */ > > +#define KVM_PV_UNHALT (1U << 7) > + > #define KVM_HINTS_DEDICATED (1U << 0) > > #define CPUID_8000_0008_EBX_IBPB (1U << 12) /* Indirect Branch Prediction Barrier */ > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > index 6c49954..3e99830 100644 > --- a/target/i386/kvm.c > +++ b/target/i386/kvm.c > @@ -1029,6 +1029,22 @@ int kvm_arch_init_vcpu(CPUState *cs) > } > } > > + if (env->features[FEAT_KVM_HINTS] & KVM_HINTS_DEDICATED) { > + int disable_exits = kvm_check_extension(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS); > + > + if (disable_exits) { > + disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT | > + KVM_X86_DISABLE_EXITS_HLT | > + KVM_X86_DISABLE_EXITS_PAUSE); > + if (env->user_features[FEAT_KVM] & KVM_PV_UNHALT) { > + disable_exits &= ~KVM_X86_DISABLE_EXITS_HLT; > + } In the future, if we decide to enable kvm-pv-unhalt by default, should "-cpu ...,kvm-hint-dedicated=on" disable kvm-pv-unhalt automatically, or should we require an explicit "kvm-hint-dedicated=on,kvm-pv-unhalt=off" option? For today's defaults, this patch solves the problem, only one thing is missing before I give my R-b: we need to clearly document what exactly are the consequences and requirements of setting kvm-hint-dedicated=on (I'm not sure if the best place for this is qemu-options.hx, x86_cpu_list(), or somewhere else). > + } > + if (kvm_vm_enable_cap(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS, 0, disable_exits)) { > + error_report("kvm: DISABLE EXITS not supported"); > + } > + } > + > qemu_add_vm_change_state_handler(cpu_update_state, env); > > c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0); > -- > 2.7.4 >
2018-04-18 2:08 GMT+08:00 Michael S. Tsirkin <mst@redhat.com>: > On Tue, Apr 17, 2018 at 01:24:15AM -0700, Wanpeng Li wrote: >> From: Wanpeng Li <wanpengli@tencent.com> >> >> This patch adds support for KVM_CAP_X86_DISABLE_EXITS. Provides userspace with >> per-VM capability(KVM_CAP_X86_DISABLE_EXITS) to not intercept MWAIT/HLT/PAUSE >> in order that to improve latency in some workloads. >> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: Radim Krčmář <rkrcmar@redhat.com> >> Cc: Eduardo Habkost <ehabkost@redhat.com> >> Signed-off-by: Wanpeng Li <wanpengli@tencent.com> >> --- >> >> linux-headers/linux/kvm.h | 6 +++++- >> target/i386/cpu.h | 2 ++ >> target/i386/kvm.c | 16 ++++++++++++++++ >> 3 files changed, 23 insertions(+), 1 deletion(-) >> >> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h >> index a167be8..857df15 100644 >> --- a/linux-headers/linux/kvm.h >> +++ b/linux-headers/linux/kvm.h >> @@ -925,7 +925,7 @@ struct kvm_ppc_resize_hpt { >> #define KVM_CAP_S390_GS 140 >> #define KVM_CAP_S390_AIS 141 >> #define KVM_CAP_SPAPR_TCE_VFIO 142 >> -#define KVM_CAP_X86_GUEST_MWAIT 143 >> +#define KVM_CAP_X86_DISABLE_EXITS 143 >> #define KVM_CAP_ARM_USER_IRQ 144 >> #define KVM_CAP_S390_CMMA_MIGRATION 145 >> #define KVM_CAP_PPC_FWNMI 146 >> @@ -1508,6 +1508,10 @@ struct kvm_assigned_msix_entry { >> #define KVM_X2APIC_API_USE_32BIT_IDS (1ULL << 0) >> #define KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK (1ULL << 1) >> >> +#define KVM_X86_DISABLE_EXITS_MWAIT (1 << 0) >> +#define KVM_X86_DISABLE_EXITS_HLT (1 << 1) >> +#define KVM_X86_DISABLE_EXITS_PAUSE (1 << 2) >> + >> /* Available with KVM_CAP_ARM_USER_IRQ */ >> >> /* Bits for run->s.regs.device_irq_level */ >> diff --git a/target/i386/cpu.h b/target/i386/cpu.h >> index 1b219fa..965de1b 100644 >> --- a/target/i386/cpu.h >> +++ b/target/i386/cpu.h >> @@ -685,6 +685,8 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS]; >> #define CPUID_7_0_EDX_AVX512_4FMAPS (1U << 3) /* AVX512 Multiply Accumulation Single Precision */ >> #define CPUID_7_0_EDX_SPEC_CTRL (1U << 26) /* Speculation Control */ >> >> +#define KVM_PV_UNHALT (1U << 7) >> + > > Why don't we use KVM_FEATURE_PV_UNHALT from kvm_para.h? > >> #define KVM_HINTS_DEDICATED (1U << 0) >> > > BTW I wonder whether we should switch to a value from > kvm_para.h? I'll send a version to do it, pls take a look. Yeah, your patchset looks good. Regards, Wanpeng Li > > >> #define CPUID_8000_0008_EBX_IBPB (1U << 12) /* Indirect Branch Prediction Barrier */ >> diff --git a/target/i386/kvm.c b/target/i386/kvm.c >> index 6c49954..3e99830 100644 >> --- a/target/i386/kvm.c >> +++ b/target/i386/kvm.c >> @@ -1029,6 +1029,22 @@ int kvm_arch_init_vcpu(CPUState *cs) >> } >> } >> >> + if (env->features[FEAT_KVM_HINTS] & KVM_HINTS_DEDICATED) { >> + int disable_exits = kvm_check_extension(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS); >> + >> + if (disable_exits) { >> + disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT | >> + KVM_X86_DISABLE_EXITS_HLT | >> + KVM_X86_DISABLE_EXITS_PAUSE); >> + if (env->user_features[FEAT_KVM] & KVM_PV_UNHALT) { >> + disable_exits &= ~KVM_X86_DISABLE_EXITS_HLT; >> + } >> + } >> + if (kvm_vm_enable_cap(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS, 0, disable_exits)) { >> + error_report("kvm: DISABLE EXITS not supported"); >> + } >> + } >> + >> qemu_add_vm_change_state_handler(cpu_update_state, env); >> >> c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0); >> -- >> 2.7.4
2018-04-18 4:59 GMT+08:00 Eduardo Habkost <ehabkost@redhat.com>: > On Tue, Apr 17, 2018 at 01:24:15AM -0700, Wanpeng Li wrote: [.../...] >> >> + if (env->features[FEAT_KVM_HINTS] & KVM_HINTS_DEDICATED) { >> + int disable_exits = kvm_check_extension(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS); >> + >> + if (disable_exits) { >> + disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT | >> + KVM_X86_DISABLE_EXITS_HLT | >> + KVM_X86_DISABLE_EXITS_PAUSE); >> + if (env->user_features[FEAT_KVM] & KVM_PV_UNHALT) { >> + disable_exits &= ~KVM_X86_DISABLE_EXITS_HLT; >> + } > > In the future, if we decide to enable kvm-pv-unhalt by default, > should "-cpu ...,kvm-hint-dedicated=on" disable kvm-pv-unhalt > automatically, or should we require an explicit > "kvm-hint-dedicated=on,kvm-pv-unhalt=off" option? > > For today's defaults, this patch solves the problem, only one > thing is missing before I give my R-b: we need to clearly > document what exactly are the consequences and requirements of > setting kvm-hint-dedicated=on (I'm not sure if the best place for > this is qemu-options.hx, x86_cpu_list(), or somewhere else). What's your opinion, Paolo? Regards, Wanpeng Li
On 17/04/2018 22:59, Eduardo Habkost wrote: >> + if (disable_exits) { >> + disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT | >> + KVM_X86_DISABLE_EXITS_HLT | >> + KVM_X86_DISABLE_EXITS_PAUSE); >> + if (env->user_features[FEAT_KVM] & KVM_PV_UNHALT) { >> + disable_exits &= ~KVM_X86_DISABLE_EXITS_HLT; >> + } > > In the future, if we decide to enable kvm-pv-unhalt by default, > should "-cpu ...,kvm-hint-dedicated=on" disable kvm-pv-unhalt > automatically, or should we require an explicit > "kvm-hint-dedicated=on,kvm-pv-unhalt=off" option? It should be automatic. > For today's defaults, this patch solves the problem, only one > thing is missing before I give my R-b: we need to clearly > document what exactly are the consequences and requirements of > setting kvm-hint-dedicated=on (I'm not sure if the best place for > this is qemu-options.hx, x86_cpu_list(), or somewhere else). I don't think we have a good place for this kind of documentation, unfortunately. Right now it is mentioned in Documentation/virtual/kvm/cpuid.txt. Paolo
On Thu, Apr 19, 2018 at 05:48:57PM +0200, Paolo Bonzini wrote: > On 17/04/2018 22:59, Eduardo Habkost wrote: > >> + if (disable_exits) { > >> + disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT | > >> + KVM_X86_DISABLE_EXITS_HLT | > >> + KVM_X86_DISABLE_EXITS_PAUSE); > >> + if (env->user_features[FEAT_KVM] & KVM_PV_UNHALT) { > >> + disable_exits &= ~KVM_X86_DISABLE_EXITS_HLT; > >> + } > > > > In the future, if we decide to enable kvm-pv-unhalt by default, > > should "-cpu ...,kvm-hint-dedicated=on" disable kvm-pv-unhalt > > automatically, or should we require an explicit > > "kvm-hint-dedicated=on,kvm-pv-unhalt=off" option? > > It should be automatic. > > > For today's defaults, this patch solves the problem, only one > > thing is missing before I give my R-b: we need to clearly > > document what exactly are the consequences and requirements of > > setting kvm-hint-dedicated=on (I'm not sure if the best place for > > this is qemu-options.hx, x86_cpu_list(), or somewhere else). > > I don't think we have a good place for this kind of documentation, > unfortunately. Right now it is mentioned in > Documentation/virtual/kvm/cpuid.txt. With this patch, the QEMU option will do more than just setting the CPUID bit, that's why I miss more detailed documentation on the QEMU side. But I agree we have no obvious place for that documentation. In the worst case we can just add a code comment on top of feature_word_info[FEAT_KVM_HINTS].feat_names warning that kvm-hint-dedicated won't just enable the flag on CPUID and has other side-effects.
On 19/04/2018 21:56, Eduardo Habkost wrote: > On Thu, Apr 19, 2018 at 05:48:57PM +0200, Paolo Bonzini wrote: >> On 17/04/2018 22:59, Eduardo Habkost wrote: >>>> + if (disable_exits) { >>>> + disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT | >>>> + KVM_X86_DISABLE_EXITS_HLT | >>>> + KVM_X86_DISABLE_EXITS_PAUSE); >>>> + if (env->user_features[FEAT_KVM] & KVM_PV_UNHALT) { >>>> + disable_exits &= ~KVM_X86_DISABLE_EXITS_HLT; >>>> + } >>> >>> In the future, if we decide to enable kvm-pv-unhalt by default, >>> should "-cpu ...,kvm-hint-dedicated=on" disable kvm-pv-unhalt >>> automatically, or should we require an explicit >>> "kvm-hint-dedicated=on,kvm-pv-unhalt=off" option? >> >> It should be automatic. >> >>> For today's defaults, this patch solves the problem, only one >>> thing is missing before I give my R-b: we need to clearly >>> document what exactly are the consequences and requirements of >>> setting kvm-hint-dedicated=on (I'm not sure if the best place for >>> this is qemu-options.hx, x86_cpu_list(), or somewhere else). >> >> I don't think we have a good place for this kind of documentation, >> unfortunately. Right now it is mentioned in >> Documentation/virtual/kvm/cpuid.txt. > > With this patch, the QEMU option will do more than just setting > the CPUID bit, that's why I miss more detailed documentation on > the QEMU side. But I agree we have no obvious place for that > documentation. > > In the worst case we can just add a code comment on top of > feature_word_info[FEAT_KVM_HINTS].feat_names warning that > kvm-hint-dedicated won't just enable the flag on CPUID and has > other side-effects. Maybe we should use "-realtime dedicated=on" instead of, or in addition to kvm-hint-dedicated=on? Thanks, Paolo
On Thu, Apr 19, 2018 at 11:32:16PM +0200, Paolo Bonzini wrote: > On 19/04/2018 21:56, Eduardo Habkost wrote: > > On Thu, Apr 19, 2018 at 05:48:57PM +0200, Paolo Bonzini wrote: > >> On 17/04/2018 22:59, Eduardo Habkost wrote: > >>>> + if (disable_exits) { > >>>> + disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT | > >>>> + KVM_X86_DISABLE_EXITS_HLT | > >>>> + KVM_X86_DISABLE_EXITS_PAUSE); > >>>> + if (env->user_features[FEAT_KVM] & KVM_PV_UNHALT) { > >>>> + disable_exits &= ~KVM_X86_DISABLE_EXITS_HLT; > >>>> + } > >>> > >>> In the future, if we decide to enable kvm-pv-unhalt by default, > >>> should "-cpu ...,kvm-hint-dedicated=on" disable kvm-pv-unhalt > >>> automatically, or should we require an explicit > >>> "kvm-hint-dedicated=on,kvm-pv-unhalt=off" option? > >> > >> It should be automatic. > >> > >>> For today's defaults, this patch solves the problem, only one > >>> thing is missing before I give my R-b: we need to clearly > >>> document what exactly are the consequences and requirements of > >>> setting kvm-hint-dedicated=on (I'm not sure if the best place for > >>> this is qemu-options.hx, x86_cpu_list(), or somewhere else). > >> > >> I don't think we have a good place for this kind of documentation, > >> unfortunately. Right now it is mentioned in > >> Documentation/virtual/kvm/cpuid.txt. > > > > With this patch, the QEMU option will do more than just setting > > the CPUID bit, that's why I miss more detailed documentation on > > the QEMU side. But I agree we have no obvious place for that > > documentation. > > > > In the worst case we can just add a code comment on top of > > feature_word_info[FEAT_KVM_HINTS].feat_names warning that > > kvm-hint-dedicated won't just enable the flag on CPUID and has > > other side-effects. > > Maybe we should use "-realtime dedicated=on" instead of, or in addition > to kvm-hint-dedicated=on? Maybe it's a better idea than overloading an option that is only expected to control a CPUID bit.
On Wed, Apr 18, 2018 at 09:09:19AM +0800, Wanpeng Li wrote: > 2018-04-18 2:08 GMT+08:00 Michael S. Tsirkin <mst@redhat.com>: > > On Tue, Apr 17, 2018 at 01:24:15AM -0700, Wanpeng Li wrote: > >> From: Wanpeng Li <wanpengli@tencent.com> > >> > >> This patch adds support for KVM_CAP_X86_DISABLE_EXITS. Provides userspace with > >> per-VM capability(KVM_CAP_X86_DISABLE_EXITS) to not intercept MWAIT/HLT/PAUSE > >> in order that to improve latency in some workloads. > >> > >> Cc: Paolo Bonzini <pbonzini@redhat.com> > >> Cc: Radim Krčmář <rkrcmar@redhat.com> > >> Cc: Eduardo Habkost <ehabkost@redhat.com> > >> Signed-off-by: Wanpeng Li <wanpengli@tencent.com> > >> --- > >> > >> linux-headers/linux/kvm.h | 6 +++++- > >> target/i386/cpu.h | 2 ++ > >> target/i386/kvm.c | 16 ++++++++++++++++ > >> 3 files changed, 23 insertions(+), 1 deletion(-) > >> > >> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h > >> index a167be8..857df15 100644 > >> --- a/linux-headers/linux/kvm.h > >> +++ b/linux-headers/linux/kvm.h > >> @@ -925,7 +925,7 @@ struct kvm_ppc_resize_hpt { > >> #define KVM_CAP_S390_GS 140 > >> #define KVM_CAP_S390_AIS 141 > >> #define KVM_CAP_SPAPR_TCE_VFIO 142 > >> -#define KVM_CAP_X86_GUEST_MWAIT 143 > >> +#define KVM_CAP_X86_DISABLE_EXITS 143 > >> #define KVM_CAP_ARM_USER_IRQ 144 > >> #define KVM_CAP_S390_CMMA_MIGRATION 145 > >> #define KVM_CAP_PPC_FWNMI 146 > >> @@ -1508,6 +1508,10 @@ struct kvm_assigned_msix_entry { > >> #define KVM_X2APIC_API_USE_32BIT_IDS (1ULL << 0) > >> #define KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK (1ULL << 1) > >> > >> +#define KVM_X86_DISABLE_EXITS_MWAIT (1 << 0) > >> +#define KVM_X86_DISABLE_EXITS_HLT (1 << 1) > >> +#define KVM_X86_DISABLE_EXITS_PAUSE (1 << 2) > >> + > >> /* Available with KVM_CAP_ARM_USER_IRQ */ > >> > >> /* Bits for run->s.regs.device_irq_level */ > >> diff --git a/target/i386/cpu.h b/target/i386/cpu.h > >> index 1b219fa..965de1b 100644 > >> --- a/target/i386/cpu.h > >> +++ b/target/i386/cpu.h > >> @@ -685,6 +685,8 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS]; > >> #define CPUID_7_0_EDX_AVX512_4FMAPS (1U << 3) /* AVX512 Multiply Accumulation Single Precision */ > >> #define CPUID_7_0_EDX_SPEC_CTRL (1U << 26) /* Speculation Control */ > >> > >> +#define KVM_PV_UNHALT (1U << 7) > >> + > > > > Why don't we use KVM_FEATURE_PV_UNHALT from kvm_para.h? > > > >> #define KVM_HINTS_DEDICATED (1U << 0) > >> > > > > BTW I wonder whether we should switch to a value from > > kvm_para.h? I'll send a version to do it, pls take a look. > > Yeah, your patchset looks good. > > Regards, > Wanpeng Li Do you plan to rebase your patch and upstream it or do you expect me to do it? > > > > > >> #define CPUID_8000_0008_EBX_IBPB (1U << 12) /* Indirect Branch Prediction Barrier */ > >> diff --git a/target/i386/kvm.c b/target/i386/kvm.c > >> index 6c49954..3e99830 100644 > >> --- a/target/i386/kvm.c > >> +++ b/target/i386/kvm.c > >> @@ -1029,6 +1029,22 @@ int kvm_arch_init_vcpu(CPUState *cs) > >> } > >> } > >> > >> + if (env->features[FEAT_KVM_HINTS] & KVM_HINTS_DEDICATED) { > >> + int disable_exits = kvm_check_extension(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS); > >> + > >> + if (disable_exits) { > >> + disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT | > >> + KVM_X86_DISABLE_EXITS_HLT | > >> + KVM_X86_DISABLE_EXITS_PAUSE); > >> + if (env->user_features[FEAT_KVM] & KVM_PV_UNHALT) { > >> + disable_exits &= ~KVM_X86_DISABLE_EXITS_HLT; > >> + } > >> + } > >> + if (kvm_vm_enable_cap(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS, 0, disable_exits)) { > >> + error_report("kvm: DISABLE EXITS not supported"); > >> + } > >> + } > >> + > >> qemu_add_vm_change_state_handler(cpu_update_state, env); > >> > >> c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0); > >> -- > >> 2.7.4
On Thu, Apr 19, 2018 at 06:53:20PM -0300, Eduardo Habkost wrote: > On Thu, Apr 19, 2018 at 11:32:16PM +0200, Paolo Bonzini wrote: > > On 19/04/2018 21:56, Eduardo Habkost wrote: > > > On Thu, Apr 19, 2018 at 05:48:57PM +0200, Paolo Bonzini wrote: > > >> On 17/04/2018 22:59, Eduardo Habkost wrote: > > >>>> + if (disable_exits) { > > >>>> + disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT | > > >>>> + KVM_X86_DISABLE_EXITS_HLT | > > >>>> + KVM_X86_DISABLE_EXITS_PAUSE); > > >>>> + if (env->user_features[FEAT_KVM] & KVM_PV_UNHALT) { > > >>>> + disable_exits &= ~KVM_X86_DISABLE_EXITS_HLT; > > >>>> + } > > >>> > > >>> In the future, if we decide to enable kvm-pv-unhalt by default, > > >>> should "-cpu ...,kvm-hint-dedicated=on" disable kvm-pv-unhalt > > >>> automatically, or should we require an explicit > > >>> "kvm-hint-dedicated=on,kvm-pv-unhalt=off" option? > > >> > > >> It should be automatic. > > >> > > >>> For today's defaults, this patch solves the problem, only one > > >>> thing is missing before I give my R-b: we need to clearly > > >>> document what exactly are the consequences and requirements of > > >>> setting kvm-hint-dedicated=on (I'm not sure if the best place for > > >>> this is qemu-options.hx, x86_cpu_list(), or somewhere else). > > >> > > >> I don't think we have a good place for this kind of documentation, > > >> unfortunately. Right now it is mentioned in > > >> Documentation/virtual/kvm/cpuid.txt. > > > > > > With this patch, the QEMU option will do more than just setting > > > the CPUID bit, that's why I miss more detailed documentation on > > > the QEMU side. But I agree we have no obvious place for that > > > documentation. > > > > > > In the worst case we can just add a code comment on top of > > > feature_word_info[FEAT_KVM_HINTS].feat_names warning that > > > kvm-hint-dedicated won't just enable the flag on CPUID and has > > > other side-effects. > > > > Maybe we should use "-realtime dedicated=on" instead of, or in addition > > to kvm-hint-dedicated=on? > > Maybe it's a better idea than overloading an option that is only > expected to control a CPUID bit. Well -realtime would be a bit confusing in that it enables mlock by default. From pure API point of view, hint-dedicated looks good since it seems to say "optimize for a dedicated host CPU" and it's a hint in that guests keep working if you violate this slightly once in a while. But I agree there's a problem: right now "kvm-" means "KVM PV" as opposed to e.g. HV enlightened gusts. So if you enable hyperv and also want to disable halt existing, what then? What should kvm-hint-dedicated=on do? So how about a new hint-dedicated=on cpu flag? We can have that set kvm-hint-dedicated if kvm PV is enabled. > -- > Eduardo
2018-05-12 5:57 GMT+08:00 Michael S. Tsirkin <mst@redhat.com>: > On Wed, Apr 18, 2018 at 09:09:19AM +0800, Wanpeng Li wrote: >> 2018-04-18 2:08 GMT+08:00 Michael S. Tsirkin <mst@redhat.com>: >> > On Tue, Apr 17, 2018 at 01:24:15AM -0700, Wanpeng Li wrote: >> >> From: Wanpeng Li <wanpengli@tencent.com> >> >> >> >> This patch adds support for KVM_CAP_X86_DISABLE_EXITS. Provides userspace with >> >> per-VM capability(KVM_CAP_X86_DISABLE_EXITS) to not intercept MWAIT/HLT/PAUSE >> >> in order that to improve latency in some workloads. >> >> >> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> >> Cc: Radim Krčmář <rkrcmar@redhat.com> >> >> Cc: Eduardo Habkost <ehabkost@redhat.com> >> >> Signed-off-by: Wanpeng Li <wanpengli@tencent.com> >> >> --- >> >> >> >> linux-headers/linux/kvm.h | 6 +++++- >> >> target/i386/cpu.h | 2 ++ >> >> target/i386/kvm.c | 16 ++++++++++++++++ >> >> 3 files changed, 23 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h >> >> index a167be8..857df15 100644 >> >> --- a/linux-headers/linux/kvm.h >> >> +++ b/linux-headers/linux/kvm.h >> >> @@ -925,7 +925,7 @@ struct kvm_ppc_resize_hpt { >> >> #define KVM_CAP_S390_GS 140 >> >> #define KVM_CAP_S390_AIS 141 >> >> #define KVM_CAP_SPAPR_TCE_VFIO 142 >> >> -#define KVM_CAP_X86_GUEST_MWAIT 143 >> >> +#define KVM_CAP_X86_DISABLE_EXITS 143 >> >> #define KVM_CAP_ARM_USER_IRQ 144 >> >> #define KVM_CAP_S390_CMMA_MIGRATION 145 >> >> #define KVM_CAP_PPC_FWNMI 146 >> >> @@ -1508,6 +1508,10 @@ struct kvm_assigned_msix_entry { >> >> #define KVM_X2APIC_API_USE_32BIT_IDS (1ULL << 0) >> >> #define KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK (1ULL << 1) >> >> >> >> +#define KVM_X86_DISABLE_EXITS_MWAIT (1 << 0) >> >> +#define KVM_X86_DISABLE_EXITS_HLT (1 << 1) >> >> +#define KVM_X86_DISABLE_EXITS_PAUSE (1 << 2) >> >> + >> >> /* Available with KVM_CAP_ARM_USER_IRQ */ >> >> >> >> /* Bits for run->s.regs.device_irq_level */ >> >> diff --git a/target/i386/cpu.h b/target/i386/cpu.h >> >> index 1b219fa..965de1b 100644 >> >> --- a/target/i386/cpu.h >> >> +++ b/target/i386/cpu.h >> >> @@ -685,6 +685,8 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS]; >> >> #define CPUID_7_0_EDX_AVX512_4FMAPS (1U << 3) /* AVX512 Multiply Accumulation Single Precision */ >> >> #define CPUID_7_0_EDX_SPEC_CTRL (1U << 26) /* Speculation Control */ >> >> >> >> +#define KVM_PV_UNHALT (1U << 7) >> >> + >> > >> > Why don't we use KVM_FEATURE_PV_UNHALT from kvm_para.h? >> > >> >> #define KVM_HINTS_DEDICATED (1U << 0) >> >> >> > >> > BTW I wonder whether we should switch to a value from >> > kvm_para.h? I'll send a version to do it, pls take a look. >> >> Yeah, your patchset looks good. >> >> Regards, >> Wanpeng Li > > Do you plan to rebase your patch and upstream it or do you expect me to > do it? You can pick it since I am too busy recently. Thanks Michael! Regards, Wanpeng Li
On Sat, May 12, 2018 at 01:12:59AM +0300, Michael S. Tsirkin wrote: > On Thu, Apr 19, 2018 at 06:53:20PM -0300, Eduardo Habkost wrote: > > On Thu, Apr 19, 2018 at 11:32:16PM +0200, Paolo Bonzini wrote: > > > On 19/04/2018 21:56, Eduardo Habkost wrote: > > > > On Thu, Apr 19, 2018 at 05:48:57PM +0200, Paolo Bonzini wrote: > > > >> On 17/04/2018 22:59, Eduardo Habkost wrote: > > > >>>> + if (disable_exits) { > > > >>>> + disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT | > > > >>>> + KVM_X86_DISABLE_EXITS_HLT | > > > >>>> + KVM_X86_DISABLE_EXITS_PAUSE); > > > >>>> + if (env->user_features[FEAT_KVM] & KVM_PV_UNHALT) { > > > >>>> + disable_exits &= ~KVM_X86_DISABLE_EXITS_HLT; > > > >>>> + } > > > >>> > > > >>> In the future, if we decide to enable kvm-pv-unhalt by default, > > > >>> should "-cpu ...,kvm-hint-dedicated=on" disable kvm-pv-unhalt > > > >>> automatically, or should we require an explicit > > > >>> "kvm-hint-dedicated=on,kvm-pv-unhalt=off" option? > > > >> > > > >> It should be automatic. > > > >> > > > >>> For today's defaults, this patch solves the problem, only one > > > >>> thing is missing before I give my R-b: we need to clearly > > > >>> document what exactly are the consequences and requirements of > > > >>> setting kvm-hint-dedicated=on (I'm not sure if the best place for > > > >>> this is qemu-options.hx, x86_cpu_list(), or somewhere else). > > > >> > > > >> I don't think we have a good place for this kind of documentation, > > > >> unfortunately. Right now it is mentioned in > > > >> Documentation/virtual/kvm/cpuid.txt. > > > > > > > > With this patch, the QEMU option will do more than just setting > > > > the CPUID bit, that's why I miss more detailed documentation on > > > > the QEMU side. But I agree we have no obvious place for that > > > > documentation. > > > > > > > > In the worst case we can just add a code comment on top of > > > > feature_word_info[FEAT_KVM_HINTS].feat_names warning that > > > > kvm-hint-dedicated won't just enable the flag on CPUID and has > > > > other side-effects. > > > > > > Maybe we should use "-realtime dedicated=on" instead of, or in addition > > > to kvm-hint-dedicated=on? > > > > Maybe it's a better idea than overloading an option that is only > > expected to control a CPUID bit. > > Well -realtime would be a bit confusing in that it enables mlock by > default. > > From pure API point of view, hint-dedicated looks good since > it seems to say "optimize for a dedicated host CPU" and > it's a hint in that guests keep working if you violate this > slightly once in a while. > > But I agree there's a problem: right now "kvm-" means "KVM PV" > as opposed to e.g. HV enlightened gusts. > So if you enable hyperv and also want to disable halt existing, > what then? What should kvm-hint-dedicated=on do? > > So how about a new hint-dedicated=on cpu flag? We can have that set > kvm-hint-dedicated if kvm PV is enabled. Using a boolean flag that is _not_ considered a CPUID feature flag would be better. Using the CPUID feature flag name risks having management software enabling the flag by accident (as it will get included in query-cpu-model-* queries). A separate boolean flag would make this clearer.
On 12/05/2018 00:12, Michael S. Tsirkin wrote: >> Maybe it's a better idea than overloading an option that is only >> expected to control a CPUID bit. > Well -realtime would be a bit confusing in that it enables mlock by > default. Currently, the only suboption of "-realtime" is mlock, which means that the only three valid uses of it are -realtime mlock=on -realtime mlock=off -realtime '' We can change the default, I think. Only the third would change meaning, and it's a slightly crazy way to use the option. > From pure API point of view, hint-dedicated looks good since > it seems to say "optimize for a dedicated host CPU" and > it's a hint in that guests keep working if you violate this > slightly once in a while. > > But I agree there's a problem: right now "kvm-" means "KVM PV" > as opposed to e.g. HV enlightened gusts. > So if you enable hyperv and also want to disable halt existing, > what then? What should kvm-hint-dedicated=on do? kvm-hint-dedicated=on only sets the CPUID bit, which Linux for example uses that to disable pv spinlocks. "-realtime dedicated-cpus=on" only disables the vmexits. You can use the two independently. Thanks, Paolo > So how about a new hint-dedicated=on cpu flag? We can have that set > kvm-hint-dedicated if kvm PV is enabled. >
On Wed, May 16, 2018 at 02:44:24PM +0200, Paolo Bonzini wrote: > On 12/05/2018 00:12, Michael S. Tsirkin wrote: > >> Maybe it's a better idea than overloading an option that is only > >> expected to control a CPUID bit. > > Well -realtime would be a bit confusing in that it enables mlock by > > default. > > Currently, the only suboption of "-realtime" is mlock, which means that > the only three valid uses of it are > > -realtime mlock=on > -realtime mlock=off > -realtime '' > > We can change the default, I think. Only the third would change > meaning, and it's a slightly crazy way to use the option. > > > From pure API point of view, hint-dedicated looks good since > > it seems to say "optimize for a dedicated host CPU" and > > it's a hint in that guests keep working if you violate this > > slightly once in a while. > > > > But I agree there's a problem: right now "kvm-" means "KVM PV" > > as opposed to e.g. HV enlightened gusts. > > So if you enable hyperv and also want to disable halt existing, > > what then? What should kvm-hint-dedicated=on do? > > kvm-hint-dedicated=on only sets the CPUID bit, which Linux for example > uses that to disable pv spinlocks. "-realtime dedicated-cpus=on" only > disables the vmexits. You can use the two independently. But when would you want to use the two independently? > Thanks, > > Paolo > > > So how about a new hint-dedicated=on cpu flag? We can have that set > > kvm-hint-dedicated if kvm PV is enabled. > >
On Wed, May 16, 2018 at 09:34:52AM -0300, Eduardo Habkost wrote: > On Sat, May 12, 2018 at 01:12:59AM +0300, Michael S. Tsirkin wrote: > > On Thu, Apr 19, 2018 at 06:53:20PM -0300, Eduardo Habkost wrote: > > > On Thu, Apr 19, 2018 at 11:32:16PM +0200, Paolo Bonzini wrote: > > > > On 19/04/2018 21:56, Eduardo Habkost wrote: > > > > > On Thu, Apr 19, 2018 at 05:48:57PM +0200, Paolo Bonzini wrote: > > > > >> On 17/04/2018 22:59, Eduardo Habkost wrote: > > > > >>>> + if (disable_exits) { > > > > >>>> + disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT | > > > > >>>> + KVM_X86_DISABLE_EXITS_HLT | > > > > >>>> + KVM_X86_DISABLE_EXITS_PAUSE); > > > > >>>> + if (env->user_features[FEAT_KVM] & KVM_PV_UNHALT) { > > > > >>>> + disable_exits &= ~KVM_X86_DISABLE_EXITS_HLT; > > > > >>>> + } > > > > >>> > > > > >>> In the future, if we decide to enable kvm-pv-unhalt by default, > > > > >>> should "-cpu ...,kvm-hint-dedicated=on" disable kvm-pv-unhalt > > > > >>> automatically, or should we require an explicit > > > > >>> "kvm-hint-dedicated=on,kvm-pv-unhalt=off" option? > > > > >> > > > > >> It should be automatic. > > > > >> > > > > >>> For today's defaults, this patch solves the problem, only one > > > > >>> thing is missing before I give my R-b: we need to clearly > > > > >>> document what exactly are the consequences and requirements of > > > > >>> setting kvm-hint-dedicated=on (I'm not sure if the best place for > > > > >>> this is qemu-options.hx, x86_cpu_list(), or somewhere else). > > > > >> > > > > >> I don't think we have a good place for this kind of documentation, > > > > >> unfortunately. Right now it is mentioned in > > > > >> Documentation/virtual/kvm/cpuid.txt. > > > > > > > > > > With this patch, the QEMU option will do more than just setting > > > > > the CPUID bit, that's why I miss more detailed documentation on > > > > > the QEMU side. But I agree we have no obvious place for that > > > > > documentation. > > > > > > > > > > In the worst case we can just add a code comment on top of > > > > > feature_word_info[FEAT_KVM_HINTS].feat_names warning that > > > > > kvm-hint-dedicated won't just enable the flag on CPUID and has > > > > > other side-effects. > > > > > > > > Maybe we should use "-realtime dedicated=on" instead of, or in addition > > > > to kvm-hint-dedicated=on? > > > > > > Maybe it's a better idea than overloading an option that is only > > > expected to control a CPUID bit. > > > > Well -realtime would be a bit confusing in that it enables mlock by > > default. > > > > From pure API point of view, hint-dedicated looks good since > > it seems to say "optimize for a dedicated host CPU" and > > it's a hint in that guests keep working if you violate this > > slightly once in a while. > > > > But I agree there's a problem: right now "kvm-" means "KVM PV" > > as opposed to e.g. HV enlightened gusts. > > So if you enable hyperv and also want to disable halt existing, > > what then? What should kvm-hint-dedicated=on do? > > > > So how about a new hint-dedicated=on cpu flag? We can have that set > > kvm-hint-dedicated if kvm PV is enabled. > > Using a boolean flag that is _not_ considered a CPUID feature > flag would be better. Using the CPUID feature flag name risks > having management software enabling the flag by accident (as it > will get included in query-cpu-model-* queries). A separate > boolean flag would make this clearer. Can we remove all hints from query-cpu-model queries? > -- > Eduardo
On 16/05/2018 16:22, Michael S. Tsirkin wrote: >> kvm-hint-dedicated=on only sets the CPUID bit, which Linux for example >> uses that to disable pv spinlocks. "-realtime dedicated-cpus=on" only >> disables the vmexits. You can use the two independently. > > But when would you want to use the two independently? 1) For testing 2) When some of your QEMUs are too old to support kvm-hint-dedicated=on, you may still want to use -realtime dedicated-cpus=on to get better performance on the new one. Paolo
On Wed, May 16, 2018 at 05:04:41PM +0200, Paolo Bonzini wrote: > On 16/05/2018 16:22, Michael S. Tsirkin wrote: > >> kvm-hint-dedicated=on only sets the CPUID bit, which Linux for example > >> uses that to disable pv spinlocks. "-realtime dedicated-cpus=on" only > >> disables the vmexits. You can use the two independently. > > > > But when would you want to use the two independently? > > 1) For testing > > 2) When some of your QEMUs are too old to support kvm-hint-dedicated=on, > you may still want to use -realtime dedicated-cpus=on to get better > performance on the new one. > > Paolo For the second purpose, can't we handle this using machine types?
On Wed, May 16, 2018 at 06:13:17PM +0300, Michael S. Tsirkin wrote: > On Wed, May 16, 2018 at 05:04:41PM +0200, Paolo Bonzini wrote: > > On 16/05/2018 16:22, Michael S. Tsirkin wrote: > > >> kvm-hint-dedicated=on only sets the CPUID bit, which Linux for example > > >> uses that to disable pv spinlocks. "-realtime dedicated-cpus=on" only > > >> disables the vmexits. You can use the two independently. > > > > > > But when would you want to use the two independently? > > > > 1) For testing > > > > 2) When some of your QEMUs are too old to support kvm-hint-dedicated=on, > > you may still want to use -realtime dedicated-cpus=on to get better > > performance on the new one. > > > > Paolo > > For the second purpose, can't we handle this using machine types? Machine-type compatibility code deals with defaults when options are omitted, not for making the meaning of explicit options depend on the machine-type. e.g. having "-machine pc-q35-2.11 -cpu ...,+kvm-hint-dedicated=on" not expose the CPUID bit that was explicitly requested in the command-line would be a bad idea.
On Wed, May 16, 2018 at 12:33:50PM -0300, Eduardo Habkost wrote: > On Wed, May 16, 2018 at 06:13:17PM +0300, Michael S. Tsirkin wrote: > > On Wed, May 16, 2018 at 05:04:41PM +0200, Paolo Bonzini wrote: > > > On 16/05/2018 16:22, Michael S. Tsirkin wrote: > > > >> kvm-hint-dedicated=on only sets the CPUID bit, which Linux for example > > > >> uses that to disable pv spinlocks. "-realtime dedicated-cpus=on" only > > > >> disables the vmexits. You can use the two independently. > > > > > > > > But when would you want to use the two independently? > > > > > > 1) For testing > > > > > > 2) When some of your QEMUs are too old to support kvm-hint-dedicated=on, > > > you may still want to use -realtime dedicated-cpus=on to get better > > > performance on the new one. > > > > > > Paolo > > > > For the second purpose, can't we handle this using machine types? > > Machine-type compatibility code deals with defaults when options > are omitted, not for making the meaning of explicit options > depend on the machine-type. > > e.g. having "-machine pc-q35-2.11 -cpu ...,+kvm-hint-dedicated=on" > not expose the CPUID bit that was explicitly requested in the > command-line would be a bad idea. Why? We have machine type affecting guest visible device behaviours for years. > -- > Eduardo
On Wed, May 16, 2018 at 07:21:04PM +0300, Michael S. Tsirkin wrote: > On Wed, May 16, 2018 at 12:33:50PM -0300, Eduardo Habkost wrote: > > On Wed, May 16, 2018 at 06:13:17PM +0300, Michael S. Tsirkin wrote: > > > On Wed, May 16, 2018 at 05:04:41PM +0200, Paolo Bonzini wrote: > > > > On 16/05/2018 16:22, Michael S. Tsirkin wrote: > > > > >> kvm-hint-dedicated=on only sets the CPUID bit, which Linux for example > > > > >> uses that to disable pv spinlocks. "-realtime dedicated-cpus=on" only > > > > >> disables the vmexits. You can use the two independently. > > > > > > > > > > But when would you want to use the two independently? > > > > > > > > 1) For testing > > > > > > > > 2) When some of your QEMUs are too old to support kvm-hint-dedicated=on, > > > > you may still want to use -realtime dedicated-cpus=on to get better > > > > performance on the new one. > > > > > > > > Paolo > > > > > > For the second purpose, can't we handle this using machine types? > > > > Machine-type compatibility code deals with defaults when options > > are omitted, not for making the meaning of explicit options > > depend on the machine-type. > > > > e.g. having "-machine pc-q35-2.11 -cpu ...,+kvm-hint-dedicated=on" > > not expose the CPUID bit that was explicitly requested in the > > command-line would be a bad idea. > > Why? We have machine type affecting guest visible device behaviours > for years. Do you have an example where a machine-type overrides an option explicitly set by the user?
On Wed, May 16, 2018 at 05:26:40PM +0300, Michael S. Tsirkin wrote: > On Wed, May 16, 2018 at 09:34:52AM -0300, Eduardo Habkost wrote: > > On Sat, May 12, 2018 at 01:12:59AM +0300, Michael S. Tsirkin wrote: > > > On Thu, Apr 19, 2018 at 06:53:20PM -0300, Eduardo Habkost wrote: > > > > On Thu, Apr 19, 2018 at 11:32:16PM +0200, Paolo Bonzini wrote: > > > > > On 19/04/2018 21:56, Eduardo Habkost wrote: > > > > > > On Thu, Apr 19, 2018 at 05:48:57PM +0200, Paolo Bonzini wrote: > > > > > >> On 17/04/2018 22:59, Eduardo Habkost wrote: > > > > > >>>> + if (disable_exits) { > > > > > >>>> + disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT | > > > > > >>>> + KVM_X86_DISABLE_EXITS_HLT | > > > > > >>>> + KVM_X86_DISABLE_EXITS_PAUSE); > > > > > >>>> + if (env->user_features[FEAT_KVM] & KVM_PV_UNHALT) { > > > > > >>>> + disable_exits &= ~KVM_X86_DISABLE_EXITS_HLT; > > > > > >>>> + } > > > > > >>> > > > > > >>> In the future, if we decide to enable kvm-pv-unhalt by default, > > > > > >>> should "-cpu ...,kvm-hint-dedicated=on" disable kvm-pv-unhalt > > > > > >>> automatically, or should we require an explicit > > > > > >>> "kvm-hint-dedicated=on,kvm-pv-unhalt=off" option? > > > > > >> > > > > > >> It should be automatic. > > > > > >> > > > > > >>> For today's defaults, this patch solves the problem, only one > > > > > >>> thing is missing before I give my R-b: we need to clearly > > > > > >>> document what exactly are the consequences and requirements of > > > > > >>> setting kvm-hint-dedicated=on (I'm not sure if the best place for > > > > > >>> this is qemu-options.hx, x86_cpu_list(), or somewhere else). > > > > > >> > > > > > >> I don't think we have a good place for this kind of documentation, > > > > > >> unfortunately. Right now it is mentioned in > > > > > >> Documentation/virtual/kvm/cpuid.txt. > > > > > > > > > > > > With this patch, the QEMU option will do more than just setting > > > > > > the CPUID bit, that's why I miss more detailed documentation on > > > > > > the QEMU side. But I agree we have no obvious place for that > > > > > > documentation. > > > > > > > > > > > > In the worst case we can just add a code comment on top of > > > > > > feature_word_info[FEAT_KVM_HINTS].feat_names warning that > > > > > > kvm-hint-dedicated won't just enable the flag on CPUID and has > > > > > > other side-effects. > > > > > > > > > > Maybe we should use "-realtime dedicated=on" instead of, or in addition > > > > > to kvm-hint-dedicated=on? > > > > > > > > Maybe it's a better idea than overloading an option that is only > > > > expected to control a CPUID bit. > > > > > > Well -realtime would be a bit confusing in that it enables mlock by > > > default. > > > > > > From pure API point of view, hint-dedicated looks good since > > > it seems to say "optimize for a dedicated host CPU" and > > > it's a hint in that guests keep working if you violate this > > > slightly once in a while. > > > > > > But I agree there's a problem: right now "kvm-" means "KVM PV" > > > as opposed to e.g. HV enlightened gusts. > > > So if you enable hyperv and also want to disable halt existing, > > > what then? What should kvm-hint-dedicated=on do? > > > > > > So how about a new hint-dedicated=on cpu flag? We can have that set > > > kvm-hint-dedicated if kvm PV is enabled. > > > > Using a boolean flag that is _not_ considered a CPUID feature > > flag would be better. Using the CPUID feature flag name risks > > having management software enabling the flag by accident (as it > > will get included in query-cpu-model-* queries). A separate > > boolean flag would make this clearer. > > Can we remove all hints from query-cpu-model queries? We already do (see usage of EatureWordInfo::no_autoenable_flags). This is just a matter of making the configuration option decoupled from the CPUID code, to avoid surprises elsewhere.
On Thu, May 17, 2018 at 02:34:28PM -0300, Eduardo Habkost wrote: > On Wed, May 16, 2018 at 05:26:40PM +0300, Michael S. Tsirkin wrote: > > On Wed, May 16, 2018 at 09:34:52AM -0300, Eduardo Habkost wrote: > > > On Sat, May 12, 2018 at 01:12:59AM +0300, Michael S. Tsirkin wrote: > > > > On Thu, Apr 19, 2018 at 06:53:20PM -0300, Eduardo Habkost wrote: > > > > > On Thu, Apr 19, 2018 at 11:32:16PM +0200, Paolo Bonzini wrote: > > > > > > On 19/04/2018 21:56, Eduardo Habkost wrote: > > > > > > > On Thu, Apr 19, 2018 at 05:48:57PM +0200, Paolo Bonzini wrote: > > > > > > >> On 17/04/2018 22:59, Eduardo Habkost wrote: > > > > > > >>>> + if (disable_exits) { > > > > > > >>>> + disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT | > > > > > > >>>> + KVM_X86_DISABLE_EXITS_HLT | > > > > > > >>>> + KVM_X86_DISABLE_EXITS_PAUSE); > > > > > > >>>> + if (env->user_features[FEAT_KVM] & KVM_PV_UNHALT) { > > > > > > >>>> + disable_exits &= ~KVM_X86_DISABLE_EXITS_HLT; > > > > > > >>>> + } > > > > > > >>> > > > > > > >>> In the future, if we decide to enable kvm-pv-unhalt by default, > > > > > > >>> should "-cpu ...,kvm-hint-dedicated=on" disable kvm-pv-unhalt > > > > > > >>> automatically, or should we require an explicit > > > > > > >>> "kvm-hint-dedicated=on,kvm-pv-unhalt=off" option? > > > > > > >> > > > > > > >> It should be automatic. > > > > > > >> > > > > > > >>> For today's defaults, this patch solves the problem, only one > > > > > > >>> thing is missing before I give my R-b: we need to clearly > > > > > > >>> document what exactly are the consequences and requirements of > > > > > > >>> setting kvm-hint-dedicated=on (I'm not sure if the best place for > > > > > > >>> this is qemu-options.hx, x86_cpu_list(), or somewhere else). > > > > > > >> > > > > > > >> I don't think we have a good place for this kind of documentation, > > > > > > >> unfortunately. Right now it is mentioned in > > > > > > >> Documentation/virtual/kvm/cpuid.txt. > > > > > > > > > > > > > > With this patch, the QEMU option will do more than just setting > > > > > > > the CPUID bit, that's why I miss more detailed documentation on > > > > > > > the QEMU side. But I agree we have no obvious place for that > > > > > > > documentation. > > > > > > > > > > > > > > In the worst case we can just add a code comment on top of > > > > > > > feature_word_info[FEAT_KVM_HINTS].feat_names warning that > > > > > > > kvm-hint-dedicated won't just enable the flag on CPUID and has > > > > > > > other side-effects. > > > > > > > > > > > > Maybe we should use "-realtime dedicated=on" instead of, or in addition > > > > > > to kvm-hint-dedicated=on? > > > > > > > > > > Maybe it's a better idea than overloading an option that is only > > > > > expected to control a CPUID bit. > > > > > > > > Well -realtime would be a bit confusing in that it enables mlock by > > > > default. > > > > > > > > From pure API point of view, hint-dedicated looks good since > > > > it seems to say "optimize for a dedicated host CPU" and > > > > it's a hint in that guests keep working if you violate this > > > > slightly once in a while. > > > > > > > > But I agree there's a problem: right now "kvm-" means "KVM PV" > > > > as opposed to e.g. HV enlightened gusts. > > > > So if you enable hyperv and also want to disable halt existing, > > > > what then? What should kvm-hint-dedicated=on do? > > > > > > > > So how about a new hint-dedicated=on cpu flag? We can have that set > > > > kvm-hint-dedicated if kvm PV is enabled. > > > > > > Using a boolean flag that is _not_ considered a CPUID feature > > > flag would be better. Using the CPUID feature flag name risks > > > having management software enabling the flag by accident (as it > > > will get included in query-cpu-model-* queries). A separate > > > boolean flag would make this clearer. > > > > Can we remove all hints from query-cpu-model queries? > > We already do (see usage of EatureWordInfo::no_autoenable_flags). > This is just a matter of making the configuration option > decoupled from the CPUID code, to avoid surprises elsewhere. It it too late to drop the hint flag and rename to a -realtime option? > -- > Eduardo
On Thu, May 17, 2018 at 08:57:04PM +0300, Michael S. Tsirkin wrote: > On Thu, May 17, 2018 at 02:34:28PM -0300, Eduardo Habkost wrote: > > On Wed, May 16, 2018 at 05:26:40PM +0300, Michael S. Tsirkin wrote: > > > On Wed, May 16, 2018 at 09:34:52AM -0300, Eduardo Habkost wrote: > > > > On Sat, May 12, 2018 at 01:12:59AM +0300, Michael S. Tsirkin wrote: > > > > > On Thu, Apr 19, 2018 at 06:53:20PM -0300, Eduardo Habkost wrote: > > > > > > On Thu, Apr 19, 2018 at 11:32:16PM +0200, Paolo Bonzini wrote: > > > > > > > On 19/04/2018 21:56, Eduardo Habkost wrote: > > > > > > > > On Thu, Apr 19, 2018 at 05:48:57PM +0200, Paolo Bonzini wrote: > > > > > > > >> On 17/04/2018 22:59, Eduardo Habkost wrote: > > > > > > > >>>> + if (disable_exits) { > > > > > > > >>>> + disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT | > > > > > > > >>>> + KVM_X86_DISABLE_EXITS_HLT | > > > > > > > >>>> + KVM_X86_DISABLE_EXITS_PAUSE); > > > > > > > >>>> + if (env->user_features[FEAT_KVM] & KVM_PV_UNHALT) { > > > > > > > >>>> + disable_exits &= ~KVM_X86_DISABLE_EXITS_HLT; > > > > > > > >>>> + } > > > > > > > >>> > > > > > > > >>> In the future, if we decide to enable kvm-pv-unhalt by default, > > > > > > > >>> should "-cpu ...,kvm-hint-dedicated=on" disable kvm-pv-unhalt > > > > > > > >>> automatically, or should we require an explicit > > > > > > > >>> "kvm-hint-dedicated=on,kvm-pv-unhalt=off" option? > > > > > > > >> > > > > > > > >> It should be automatic. > > > > > > > >> > > > > > > > >>> For today's defaults, this patch solves the problem, only one > > > > > > > >>> thing is missing before I give my R-b: we need to clearly > > > > > > > >>> document what exactly are the consequences and requirements of > > > > > > > >>> setting kvm-hint-dedicated=on (I'm not sure if the best place for > > > > > > > >>> this is qemu-options.hx, x86_cpu_list(), or somewhere else). > > > > > > > >> > > > > > > > >> I don't think we have a good place for this kind of documentation, > > > > > > > >> unfortunately. Right now it is mentioned in > > > > > > > >> Documentation/virtual/kvm/cpuid.txt. > > > > > > > > > > > > > > > > With this patch, the QEMU option will do more than just setting > > > > > > > > the CPUID bit, that's why I miss more detailed documentation on > > > > > > > > the QEMU side. But I agree we have no obvious place for that > > > > > > > > documentation. > > > > > > > > > > > > > > > > In the worst case we can just add a code comment on top of > > > > > > > > feature_word_info[FEAT_KVM_HINTS].feat_names warning that > > > > > > > > kvm-hint-dedicated won't just enable the flag on CPUID and has > > > > > > > > other side-effects. > > > > > > > > > > > > > > Maybe we should use "-realtime dedicated=on" instead of, or in addition > > > > > > > to kvm-hint-dedicated=on? > > > > > > > > > > > > Maybe it's a better idea than overloading an option that is only > > > > > > expected to control a CPUID bit. > > > > > > > > > > Well -realtime would be a bit confusing in that it enables mlock by > > > > > default. > > > > > > > > > > From pure API point of view, hint-dedicated looks good since > > > > > it seems to say "optimize for a dedicated host CPU" and > > > > > it's a hint in that guests keep working if you violate this > > > > > slightly once in a while. > > > > > > > > > > But I agree there's a problem: right now "kvm-" means "KVM PV" > > > > > as opposed to e.g. HV enlightened gusts. > > > > > So if you enable hyperv and also want to disable halt existing, > > > > > what then? What should kvm-hint-dedicated=on do? > > > > > > > > > > So how about a new hint-dedicated=on cpu flag? We can have that set > > > > > kvm-hint-dedicated if kvm PV is enabled. > > > > > > > > Using a boolean flag that is _not_ considered a CPUID feature > > > > flag would be better. Using the CPUID feature flag name risks > > > > having management software enabling the flag by accident (as it > > > > will get included in query-cpu-model-* queries). A separate > > > > boolean flag would make this clearer. > > > > > > Can we remove all hints from query-cpu-model queries? > > > > We already do (see usage of EatureWordInfo::no_autoenable_flags). > > This is just a matter of making the configuration option > > decoupled from the CPUID code, to avoid surprises elsewhere. > > It it too late to drop the hint flag and rename to a -realtime option? We can't remove support for "-cpu ...,kvm-pv-dedicated=on" without a deprecation notice, as it's already in v2.12.0. But it's not too late to make other side-effects (e.g. disabling VM exits) be controlled by -realtime or other command-line option. It's also not too late to move kvm-pv-dedicated from the feat_names array to x86_cpu_properties to avoid confusion. The existing behavior of "-cpu ...,kvm-hint-dedicated=on" is to only set the CPUID bit and do nothing else that could have unwanted side-effects (like disabling VM exits). Do you see a problem in simply keeping the existing behavior?
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h index a167be8..857df15 100644 --- a/linux-headers/linux/kvm.h +++ b/linux-headers/linux/kvm.h @@ -925,7 +925,7 @@ struct kvm_ppc_resize_hpt { #define KVM_CAP_S390_GS 140 #define KVM_CAP_S390_AIS 141 #define KVM_CAP_SPAPR_TCE_VFIO 142 -#define KVM_CAP_X86_GUEST_MWAIT 143 +#define KVM_CAP_X86_DISABLE_EXITS 143 #define KVM_CAP_ARM_USER_IRQ 144 #define KVM_CAP_S390_CMMA_MIGRATION 145 #define KVM_CAP_PPC_FWNMI 146 @@ -1508,6 +1508,10 @@ struct kvm_assigned_msix_entry { #define KVM_X2APIC_API_USE_32BIT_IDS (1ULL << 0) #define KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK (1ULL << 1) +#define KVM_X86_DISABLE_EXITS_MWAIT (1 << 0) +#define KVM_X86_DISABLE_EXITS_HLT (1 << 1) +#define KVM_X86_DISABLE_EXITS_PAUSE (1 << 2) + /* Available with KVM_CAP_ARM_USER_IRQ */ /* Bits for run->s.regs.device_irq_level */ diff --git a/target/i386/cpu.h b/target/i386/cpu.h index 1b219fa..965de1b 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -685,6 +685,8 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS]; #define CPUID_7_0_EDX_AVX512_4FMAPS (1U << 3) /* AVX512 Multiply Accumulation Single Precision */ #define CPUID_7_0_EDX_SPEC_CTRL (1U << 26) /* Speculation Control */ +#define KVM_PV_UNHALT (1U << 7) + #define KVM_HINTS_DEDICATED (1U << 0) #define CPUID_8000_0008_EBX_IBPB (1U << 12) /* Indirect Branch Prediction Barrier */ diff --git a/target/i386/kvm.c b/target/i386/kvm.c index 6c49954..3e99830 100644 --- a/target/i386/kvm.c +++ b/target/i386/kvm.c @@ -1029,6 +1029,22 @@ int kvm_arch_init_vcpu(CPUState *cs) } } + if (env->features[FEAT_KVM_HINTS] & KVM_HINTS_DEDICATED) { + int disable_exits = kvm_check_extension(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS); + + if (disable_exits) { + disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT | + KVM_X86_DISABLE_EXITS_HLT | + KVM_X86_DISABLE_EXITS_PAUSE); + if (env->user_features[FEAT_KVM] & KVM_PV_UNHALT) { + disable_exits &= ~KVM_X86_DISABLE_EXITS_HLT; + } + } + if (kvm_vm_enable_cap(cs->kvm_state, KVM_CAP_X86_DISABLE_EXITS, 0, disable_exits)) { + error_report("kvm: DISABLE EXITS not supported"); + } + } + qemu_add_vm_change_state_handler(cpu_update_state, env); c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0);