[Qemu-devel] [PATCH RESEND v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[Qemu-devel] [PATCH RESEND v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[Wanpeng Li]

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;
+            }
+        }
+        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

Re: [Qemu-devel] [PATCH RESEND v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[no-reply]

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

Re: [Qemu-devel] [PATCH RESEND v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PATCH RESEND v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[Wanpeng Li]

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

Re: [Qemu-devel] [PATCH RESEND v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PATCH RESEND v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[Wanpeng Li]

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

Re: [Qemu-devel] [PATCH RESEND v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[Eduardo Habkost]

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
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH RESEND v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[Wanpeng Li]

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

Re: [Qemu-devel] [PATCH RESEND v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH RESEND v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[Eduardo Habkost]

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.

-- 
Eduardo

Re: [Qemu-devel] [PATCH RESEND v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH RESEND v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[Eduardo Habkost]

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.

-- 
Eduardo

Re: [Qemu-devel] [PATCH RESEND v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PATCH RESEND v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[Eduardo Habkost]

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.

-- 
Eduardo

Re: [Qemu-devel] [PATCH RESEND v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PATCH RESEND v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[Eduardo Habkost]

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.

-- 
Eduardo

Re: [Qemu-devel] [PATCH RESEND v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PATCH RESEND v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[Eduardo Habkost]

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?

-- 
Eduardo

Re: [Qemu-devel] [PATCH RESEND v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[Paolo Bonzini]

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.
> 

Re: [Qemu-devel] [PATCH RESEND v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[Michael S. Tsirkin]

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.
> > 

Re: [Qemu-devel] [PATCH RESEND v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH RESEND v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[Michael S. Tsirkin]

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?

-- 
MST

Re: [Qemu-devel] [PATCH RESEND v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[Eduardo Habkost]

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.

-- 
Eduardo

Re: [Qemu-devel] [PATCH RESEND v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PATCH RESEND v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[Eduardo Habkost]

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?

-- 
Eduardo