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

[Wanpeng Li]

From: Wanpeng Li <***@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 <***@redhat.com>
Cc: Radim Krčmář <***@redhat.com>
Cc: Eduardo Habkost <***@redhat.com>
Signed-off-by: Wanpeng Li <***@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);

[n***@patchew.org]

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-***@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-***@tencent.com -> patchew/1523953455-28053-1-git-send-email-***@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

[Michael S. Tsirkin]

Post by Wanpeng Li
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.
---
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?

Post by Wanpeng Li
#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.

Post by 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

[Wanpeng Li]

Post by Michael S. Tsirkin

Post by Wanpeng Li
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.
---
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?

Post by Wanpeng Li
#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

Post by Michael S. Tsirkin

Post by 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

[Wanpeng Li]

Post by Wanpeng Li

Post by Michael S. Tsirkin

Post by Wanpeng Li
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.
---
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?

Post by Wanpeng Li
#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

[Eduardo Habkost]

Post by Wanpeng Li
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.
---
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).

Post by Wanpeng Li
+ }
+ 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

[Wanpeng Li]

[.../...]

Post by Eduardo Habkost

Post by Wanpeng Li
+ 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

[Paolo Bonzini]

Post by Eduardo Habkost

Post by Wanpeng Li
+ 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.

Post by Eduardo Habkost
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

[Eduardo Habkost]

Post by Paolo Bonzini

Post by Eduardo Habkost

Post by Wanpeng Li
+ 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.

Post by Eduardo Habkost
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.

[Paolo Bonzini]

Post by Eduardo Habkost

Post by Paolo Bonzini

Post by Eduardo Habkost

Post by Wanpeng Li
+ 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.

Post by Eduardo Habkost
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

[Eduardo Habkost]

Post by Paolo Bonzini

Post by Eduardo Habkost

Post by Paolo Bonzini

Post by Eduardo Habkost

Post by Wanpeng Li
+ 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.

Post by Eduardo Habkost
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.

[Michael S. Tsirkin]

Post by Eduardo Habkost

Post by Paolo Bonzini

Post by Eduardo Habkost

Post by Paolo Bonzini

Post by Eduardo Habkost

Post by Wanpeng Li
+ 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.

Post by Eduardo Habkost
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.

Post by Eduardo Habkost
--
Eduardo

[Eduardo Habkost]

Post by Michael S. Tsirkin

Post by Eduardo Habkost

Post by Paolo Bonzini

Post by Eduardo Habkost

Post by Paolo Bonzini

Post by Eduardo Habkost

Post by Wanpeng Li
+ 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.

Post by Eduardo Habkost
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.

[Michael S. Tsirkin]

Post by Eduardo Habkost

Post by Michael S. Tsirkin

Post by Eduardo Habkost

Post by Paolo Bonzini

Post by Eduardo Habkost

Post by Paolo Bonzini

Post by Eduardo Habkost

Post by Wanpeng Li
+ 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.

Post by Eduardo Habkost
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?

Post by Eduardo Habkost
--
Eduardo

[Paolo Bonzini]

Post by Michael S. Tsirkin

Post by Eduardo Habkost
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.

Post by Michael S. Tsirkin
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

Post by Michael S. Tsirkin
So how about a new hint-dedicated=on cpu flag? We can have that set
kvm-hint-dedicated if kvm PV is enabled.

[Michael S. Tsirkin]

Post by Paolo Bonzini

Post by Michael S. Tsirkin

Post by Eduardo Habkost
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.

Post by Michael S. Tsirkin
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?

Post by Paolo Bonzini
Thanks,
Paolo

Post by Michael S. Tsirkin
So how about a new hint-dedicated=on cpu flag? We can have that set
kvm-hint-dedicated if kvm PV is enabled.

[Michael S. Tsirkin]

Post by Michael S. Tsirkin

Post by Paolo Bonzini
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?

[Eduardo Habkost]

Post by Michael S. Tsirkin

Post by Michael S. Tsirkin

Post by Paolo Bonzini
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.

[Michael S. Tsirkin]

Post by Eduardo Habkost

Post by Michael S. Tsirkin

Post by Michael S. Tsirkin

Post by Paolo Bonzini
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.

Post by Eduardo Habkost
--
Eduardo

[Eduardo Habkost]

Post by Michael S. Tsirkin

Post by Eduardo Habkost

Post by Michael S. Tsirkin

Post by Michael S. Tsirkin

Post by Paolo Bonzini
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?