linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/7] KVM: x86: hyper-v: make KVM_GET_SUPPORTED_HV_CPUID more useful
@ 2020-09-24 14:57 Vitaly Kuznetsov
  2020-09-24 14:57 ` [PATCH v2 1/7] KVM: x86: hyper-v: Mention SynDBG CPUID leaves in api.rst Vitaly Kuznetsov
                   ` (7 more replies)
  0 siblings, 8 replies; 13+ messages in thread
From: Vitaly Kuznetsov @ 2020-09-24 14:57 UTC (permalink / raw)
  To: kvm, Paolo Bonzini
  Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Eduardo Habkost,
	Jon Doron, linux-kernel

Changes since v1:
- Rebased to kvm/queue [KVM_CAP_SYS_HYPERV_CPUID -> 188]

QEMU series using the feature:
https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg02017.html

Original description:

KVM_GET_SUPPORTED_HV_CPUID was initially implemented as a vCPU ioctl but
this is not very useful when VMM is just trying to query which Hyper-V
features are supported by the host prior to creating VM/vCPUs. The data
in KVM_GET_SUPPORTED_HV_CPUID is mostly static with a few exceptions but
it seems we can change this. Add support for KVM_GET_SUPPORTED_HV_CPUID as
a system ioctl as well.

QEMU specific description:
In some cases QEMU needs to collect the information about which Hyper-V
features are supported by KVM and pass it up the stack. For non-hyper-v
features this is done with system-wide KVM_GET_SUPPORTED_CPUID/
KVM_GET_MSRS ioctls but Hyper-V specific features don't get in the output
(as Hyper-V CPUIDs intersect with KVM's). In QEMU, CPU feature expansion
happens before any KVM vcpus are created so KVM_GET_SUPPORTED_HV_CPUID
can't be used in its current shape.

Vitaly Kuznetsov (7):
  KVM: x86: hyper-v: Mention SynDBG CPUID leaves in api.rst
  KVM: x86: hyper-v: disallow configuring SynIC timers with no SynIC
  KVM: x86: hyper-v: make KVM_GET_SUPPORTED_HV_CPUID output independent
    of eVMCS enablement
  KVM: x86: hyper-v: always advertise HV_STIMER_DIRECT_MODE_AVAILABLE
  KVM: x86: hyper-v: drop now unneeded vcpu parameter from
    kvm_vcpu_ioctl_get_hv_cpuid()
  KVM: x86: hyper-v: allow KVM_GET_SUPPORTED_HV_CPUID as a system ioctl
  KVM: selftests: test KVM_GET_SUPPORTED_HV_CPUID as a system ioctl

 Documentation/virt/kvm/api.rst                | 12 +--
 arch/x86/include/asm/kvm_host.h               |  2 +-
 arch/x86/kvm/hyperv.c                         | 30 ++++----
 arch/x86/kvm/hyperv.h                         |  3 +-
 arch/x86/kvm/vmx/evmcs.c                      |  8 +-
 arch/x86/kvm/vmx/evmcs.h                      |  2 +-
 arch/x86/kvm/x86.c                            | 44 ++++++-----
 include/uapi/linux/kvm.h                      |  3 +-
 .../testing/selftests/kvm/include/kvm_util.h  |  2 +
 tools/testing/selftests/kvm/lib/kvm_util.c    | 26 +++++++
 .../selftests/kvm/x86_64/hyperv_cpuid.c       | 77 +++++++++----------
 11 files changed, 120 insertions(+), 89 deletions(-)

-- 
2.25.4


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 1/7] KVM: x86: hyper-v: Mention SynDBG CPUID leaves in api.rst
  2020-09-24 14:57 [PATCH v2 0/7] KVM: x86: hyper-v: make KVM_GET_SUPPORTED_HV_CPUID more useful Vitaly Kuznetsov
@ 2020-09-24 14:57 ` Vitaly Kuznetsov
  2020-09-24 14:57 ` [PATCH v2 2/7] KVM: x86: hyper-v: disallow configuring SynIC timers with no SynIC Vitaly Kuznetsov
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Vitaly Kuznetsov @ 2020-09-24 14:57 UTC (permalink / raw)
  To: kvm, Paolo Bonzini
  Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Eduardo Habkost,
	Jon Doron, linux-kernel

We forgot to update KVM_GET_SUPPORTED_HV_CPUID's documentation in api.rst
when SynDBG leaves were added.

While on it, fix 'KVM_GET_SUPPORTED_CPUID' copy-paste error.

Fixes: f97f5a56f597 ("x86/kvm/hyper-v: Add support for synthetic debugger interface")
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 Documentation/virt/kvm/api.rst | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index d2b733dc7892..f0e9582c6b44 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -4498,11 +4498,14 @@ Currently, the following list of CPUID leaves are returned:
  - HYPERV_CPUID_ENLIGHTMENT_INFO
  - HYPERV_CPUID_IMPLEMENT_LIMITS
  - HYPERV_CPUID_NESTED_FEATURES
+ - HYPERV_CPUID_SYNDBG_VENDOR_AND_MAX_FUNCTIONS
+ - HYPERV_CPUID_SYNDBG_INTERFACE
+ - HYPERV_CPUID_SYNDBG_PLATFORM_CAPABILITIES
 
 HYPERV_CPUID_NESTED_FEATURES leaf is only exposed when Enlightened VMCS was
 enabled on the corresponding vCPU (KVM_CAP_HYPERV_ENLIGHTENED_VMCS).
 
-Userspace invokes KVM_GET_SUPPORTED_CPUID by passing a kvm_cpuid2 structure
+Userspace invokes KVM_GET_SUPPORTED_HV_CPUID by passing a kvm_cpuid2 structure
 with the 'nent' field indicating the number of entries in the variable-size
 array 'entries'.  If the number of entries is too low to describe all Hyper-V
 feature leaves, an error (E2BIG) is returned. If the number is more or equal
-- 
2.25.4


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 2/7] KVM: x86: hyper-v: disallow configuring SynIC timers with no SynIC
  2020-09-24 14:57 [PATCH v2 0/7] KVM: x86: hyper-v: make KVM_GET_SUPPORTED_HV_CPUID more useful Vitaly Kuznetsov
  2020-09-24 14:57 ` [PATCH v2 1/7] KVM: x86: hyper-v: Mention SynDBG CPUID leaves in api.rst Vitaly Kuznetsov
@ 2020-09-24 14:57 ` Vitaly Kuznetsov
  2020-09-24 14:57 ` [PATCH v2 3/7] KVM: x86: hyper-v: make KVM_GET_SUPPORTED_HV_CPUID output independent of eVMCS enablement Vitaly Kuznetsov
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Vitaly Kuznetsov @ 2020-09-24 14:57 UTC (permalink / raw)
  To: kvm, Paolo Bonzini
  Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Eduardo Habkost,
	Jon Doron, linux-kernel

Hyper-V Synthetic timers require SynIC but we don't seem to check that
upon HV_X64_MSR_STIMER[X]_CONFIG/HV_X64_MSR_STIMER0_COUNT writes. Make
the behavior match synic_set_msr().

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/kvm/hyperv.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index 814d3aee5cef..9527fa9685ad 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -633,6 +633,11 @@ static int stimer_set_config(struct kvm_vcpu_hv_stimer *stimer, u64 config,
 {
 	union hv_stimer_config new_config = {.as_uint64 = config},
 		old_config = {.as_uint64 = stimer->config.as_uint64};
+	struct kvm_vcpu *vcpu = stimer_to_vcpu(stimer);
+	struct kvm_vcpu_hv_synic *synic = vcpu_to_synic(vcpu);
+
+	if (!synic->active && !host)
+		return 1;
 
 	trace_kvm_hv_stimer_set_config(stimer_to_vcpu(stimer)->vcpu_id,
 				       stimer->index, config, host);
@@ -652,6 +657,12 @@ static int stimer_set_config(struct kvm_vcpu_hv_stimer *stimer, u64 config,
 static int stimer_set_count(struct kvm_vcpu_hv_stimer *stimer, u64 count,
 			    bool host)
 {
+	struct kvm_vcpu *vcpu = stimer_to_vcpu(stimer);
+	struct kvm_vcpu_hv_synic *synic = vcpu_to_synic(vcpu);
+
+	if (!synic->active && !host)
+		return 1;
+
 	trace_kvm_hv_stimer_set_count(stimer_to_vcpu(stimer)->vcpu_id,
 				      stimer->index, count, host);
 
-- 
2.25.4


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 3/7] KVM: x86: hyper-v: make KVM_GET_SUPPORTED_HV_CPUID output independent of eVMCS enablement
  2020-09-24 14:57 [PATCH v2 0/7] KVM: x86: hyper-v: make KVM_GET_SUPPORTED_HV_CPUID more useful Vitaly Kuznetsov
  2020-09-24 14:57 ` [PATCH v2 1/7] KVM: x86: hyper-v: Mention SynDBG CPUID leaves in api.rst Vitaly Kuznetsov
  2020-09-24 14:57 ` [PATCH v2 2/7] KVM: x86: hyper-v: disallow configuring SynIC timers with no SynIC Vitaly Kuznetsov
@ 2020-09-24 14:57 ` Vitaly Kuznetsov
  2020-09-24 14:57 ` [PATCH v2 4/7] KVM: x86: hyper-v: always advertise HV_STIMER_DIRECT_MODE_AVAILABLE Vitaly Kuznetsov
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Vitaly Kuznetsov @ 2020-09-24 14:57 UTC (permalink / raw)
  To: kvm, Paolo Bonzini
  Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Eduardo Habkost,
	Jon Doron, linux-kernel

It is really inconvenient when KVM_GET_SUPPORTED_HV_CPUID output depends on
whether some other capabilities are enabled as VMM needs to be aware of the
correct sequence (which becomes an API).

Enlightened VMCS is not the only feature which requires explicit enablement.
In particular, SynIC has to be enabled explicitly too but the corresponding
features are always present in KVM_GET_SUPPORTED_HV_CPUID, this is
inconsistent. In any case, userspace can't blindly copy the output of
KVM_GET_SUPPORTED_HV_CPUID to guest's CPUIDs and expect everything to work.

Previously, we were also using EVMCS enablement to decide whether or not to
report HYPERV_CPUID_NESTED_FEATURES feature leaf as the last available leaf
in HYPERV_CPUID_SYNDBG_PLATFORM_CAPABILITIES but this is already gone with
SYNDBG and nobody seemed to complain.

As a preparation to making KVM_GET_SUPPORTED_HV_CPUID a system ioctl, make
EVMCS feature bits work the same way as all other bits, nothing should break
(famous last words).

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 Documentation/virt/kvm/api.rst                |  3 --
 arch/x86/include/asm/kvm_host.h               |  2 +-
 arch/x86/kvm/hyperv.c                         |  8 ++---
 arch/x86/kvm/vmx/evmcs.c                      |  8 ++---
 arch/x86/kvm/vmx/evmcs.h                      |  2 +-
 .../selftests/kvm/x86_64/hyperv_cpuid.c       | 35 +++++--------------
 6 files changed, 15 insertions(+), 43 deletions(-)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index f0e9582c6b44..a552d41308e1 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -4502,9 +4502,6 @@ Currently, the following list of CPUID leaves are returned:
  - HYPERV_CPUID_SYNDBG_INTERFACE
  - HYPERV_CPUID_SYNDBG_PLATFORM_CAPABILITIES
 
-HYPERV_CPUID_NESTED_FEATURES leaf is only exposed when Enlightened VMCS was
-enabled on the corresponding vCPU (KVM_CAP_HYPERV_ENLIGHTENED_VMCS).
-
 Userspace invokes KVM_GET_SUPPORTED_HV_CPUID by passing a kvm_cpuid2 structure
 with the 'nent' field indicating the number of entries in the variable-size
 array 'entries'.  If the number of entries is too low to describe all Hyper-V
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 5303dbc5c9bc..ef971ae5f217 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1243,7 +1243,7 @@ struct kvm_x86_nested_ops {
 
 	int (*enable_evmcs)(struct kvm_vcpu *vcpu,
 			    uint16_t *vmcs_version);
-	uint16_t (*get_evmcs_version)(struct kvm_vcpu *vcpu);
+	uint16_t (*get_evmcs_version)(void);
 };
 
 struct kvm_x86_init_ops {
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index 9527fa9685ad..6da20f91cd59 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -1962,19 +1962,15 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
 		{ .function = HYPERV_CPUID_FEATURES },
 		{ .function = HYPERV_CPUID_ENLIGHTMENT_INFO },
 		{ .function = HYPERV_CPUID_IMPLEMENT_LIMITS },
+		{ .function = HYPERV_CPUID_NESTED_FEATURES },
 		{ .function = HYPERV_CPUID_SYNDBG_VENDOR_AND_MAX_FUNCTIONS },
 		{ .function = HYPERV_CPUID_SYNDBG_INTERFACE },
 		{ .function = HYPERV_CPUID_SYNDBG_PLATFORM_CAPABILITIES	},
-		{ .function = HYPERV_CPUID_NESTED_FEATURES },
 	};
 	int i, nent = ARRAY_SIZE(cpuid_entries);
 
 	if (kvm_x86_ops.nested_ops->get_evmcs_version)
-		evmcs_ver = kvm_x86_ops.nested_ops->get_evmcs_version(vcpu);
-
-	/* Skip NESTED_FEATURES if eVMCS is not supported */
-	if (!evmcs_ver)
-		--nent;
+		evmcs_ver = kvm_x86_ops.nested_ops->get_evmcs_version();
 
 	if (cpuid->nent < nent)
 		return -E2BIG;
diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c
index e5325bd0f304..4559d5851bf0 100644
--- a/arch/x86/kvm/vmx/evmcs.c
+++ b/arch/x86/kvm/vmx/evmcs.c
@@ -325,17 +325,15 @@ bool nested_enlightened_vmentry(struct kvm_vcpu *vcpu, u64 *evmcs_gpa)
 	return true;
 }
 
-uint16_t nested_get_evmcs_version(struct kvm_vcpu *vcpu)
+uint16_t nested_get_evmcs_version(void)
 {
-	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	/*
 	 * vmcs_version represents the range of supported Enlightened VMCS
 	 * versions: lower 8 bits is the minimal version, higher 8 bits is the
 	 * maximum supported version. KVM supports versions from 1 to
 	 * KVM_EVMCS_VERSION.
 	 */
-	if (kvm_cpu_cap_get(X86_FEATURE_VMX) &&
-	    vmx->nested.enlightened_vmcs_enabled)
+	if (kvm_cpu_cap_get(X86_FEATURE_VMX))
 		return (KVM_EVMCS_VERSION << 8) | 1;
 
 	return 0;
@@ -427,7 +425,7 @@ int nested_enable_evmcs(struct kvm_vcpu *vcpu,
 	vmx->nested.enlightened_vmcs_enabled = true;
 
 	if (vmcs_version)
-		*vmcs_version = nested_get_evmcs_version(vcpu);
+		*vmcs_version = nested_get_evmcs_version();
 
 	return 0;
 }
diff --git a/arch/x86/kvm/vmx/evmcs.h b/arch/x86/kvm/vmx/evmcs.h
index e5f7a7ebf27d..8e6429ec9ace 100644
--- a/arch/x86/kvm/vmx/evmcs.h
+++ b/arch/x86/kvm/vmx/evmcs.h
@@ -206,7 +206,7 @@ enum nested_evmptrld_status {
 };
 
 bool nested_enlightened_vmentry(struct kvm_vcpu *vcpu, u64 *evmcs_gpa);
-uint16_t nested_get_evmcs_version(struct kvm_vcpu *vcpu);
+uint16_t nested_get_evmcs_version(void);
 int nested_enable_evmcs(struct kvm_vcpu *vcpu,
 			uint16_t *vmcs_version);
 void nested_evmcs_filter_control_msr(u32 msr_index, u64 *pdata);
diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_cpuid.c b/tools/testing/selftests/kvm/x86_64/hyperv_cpuid.c
index 745b708c2d3b..8b24cb2e6a19 100644
--- a/tools/testing/selftests/kvm/x86_64/hyperv_cpuid.c
+++ b/tools/testing/selftests/kvm/x86_64/hyperv_cpuid.c
@@ -45,20 +45,16 @@ static bool smt_possible(void)
 	return res;
 }
 
-static void test_hv_cpuid(struct kvm_cpuid2 *hv_cpuid_entries,
-			  bool evmcs_enabled)
+static void test_hv_cpuid(struct kvm_cpuid2 *hv_cpuid_entries)
 {
 	int i;
-	int nent = 9;
+	int nent = 10;
 	u32 test_val;
 
-	if (evmcs_enabled)
-		nent += 1; /* 0x4000000A */
-
 	TEST_ASSERT(hv_cpuid_entries->nent == nent,
 		    "KVM_GET_SUPPORTED_HV_CPUID should return %d entries"
-		    " with evmcs=%d (returned %d)",
-		    nent, evmcs_enabled, hv_cpuid_entries->nent);
+		    " (returned %d)",
+		    nent, hv_cpuid_entries->nent);
 
 	for (i = 0; i < hv_cpuid_entries->nent; i++) {
 		struct kvm_cpuid_entry2 *entry = &hv_cpuid_entries->entries[i];
@@ -68,9 +64,6 @@ static void test_hv_cpuid(struct kvm_cpuid2 *hv_cpuid_entries,
 			    "function %x is our of supported range",
 			    entry->function);
 
-		TEST_ASSERT(evmcs_enabled || (entry->function != 0x4000000A),
-			    "0x4000000A leaf should not be reported");
-
 		TEST_ASSERT(entry->index == 0,
 			    ".index field should be zero");
 
@@ -85,9 +78,8 @@ static void test_hv_cpuid(struct kvm_cpuid2 *hv_cpuid_entries,
 			test_val = 0x40000082;
 
 			TEST_ASSERT(entry->eax == test_val,
-				    "Wrong max leaf report in 0x40000000.EAX: %x"
-				    " (evmcs=%d)",
-				    entry->eax, evmcs_enabled
+				    "Wrong max leaf report in 0x40000000.EAX: %x",
+				    entry->eax
 				);
 			break;
 		case 0x40000004:
@@ -148,7 +140,6 @@ int main(int argc, char *argv[])
 	struct kvm_vm *vm;
 	int rv, stage;
 	struct kvm_cpuid2 *hv_cpuid_entries;
-	bool evmcs_enabled;
 
 	/* Tell stdout not to buffer its content */
 	setbuf(stdout, NULL);
@@ -159,8 +150,7 @@ int main(int argc, char *argv[])
 		exit(KSFT_SKIP);
 	}
 
-	for (stage = 0; stage < 3; stage++) {
-		evmcs_enabled = false;
+	for (stage = 0; stage < 2; stage++) {
 
 		vm = vm_create_default(VCPU_ID, 0, guest_code);
 		switch (stage) {
@@ -169,19 +159,10 @@ int main(int argc, char *argv[])
 			continue;
 		case 1:
 			break;
-		case 2:
-			if (!nested_vmx_supported() ||
-			    !kvm_check_cap(KVM_CAP_HYPERV_ENLIGHTENED_VMCS)) {
-				print_skip("Enlightened VMCS is unsupported");
-				continue;
-			}
-			vcpu_enable_evmcs(vm, VCPU_ID);
-			evmcs_enabled = true;
-			break;
 		}
 
 		hv_cpuid_entries = kvm_get_supported_hv_cpuid(vm);
-		test_hv_cpuid(hv_cpuid_entries, evmcs_enabled);
+		test_hv_cpuid(hv_cpuid_entries);
 		free(hv_cpuid_entries);
 		kvm_vm_free(vm);
 	}
-- 
2.25.4


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 4/7] KVM: x86: hyper-v: always advertise HV_STIMER_DIRECT_MODE_AVAILABLE
  2020-09-24 14:57 [PATCH v2 0/7] KVM: x86: hyper-v: make KVM_GET_SUPPORTED_HV_CPUID more useful Vitaly Kuznetsov
                   ` (2 preceding siblings ...)
  2020-09-24 14:57 ` [PATCH v2 3/7] KVM: x86: hyper-v: make KVM_GET_SUPPORTED_HV_CPUID output independent of eVMCS enablement Vitaly Kuznetsov
@ 2020-09-24 14:57 ` Vitaly Kuznetsov
  2020-09-25 20:32   ` Paolo Bonzini
  2020-09-24 14:57 ` [PATCH v2 5/7] KVM: x86: hyper-v: drop now unneeded vcpu parameter from kvm_vcpu_ioctl_get_hv_cpuid() Vitaly Kuznetsov
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 13+ messages in thread
From: Vitaly Kuznetsov @ 2020-09-24 14:57 UTC (permalink / raw)
  To: kvm, Paolo Bonzini
  Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Eduardo Habkost,
	Jon Doron, linux-kernel

HV_STIMER_DIRECT_MODE_AVAILABLE is the last conditionally set feature bit
in KVM_GET_SUPPORTED_HV_CPUID but it doesn't have to be conditional: first,
this bit is only an indication to userspace VMM that direct mode stimers
are supported, it still requires manual enablement (enabling SynIC) to
work so no VMM should just blindly copy it to guest CPUIDs. Second,
lapic_in_kernel() is a must for SynIC. Expose the bit unconditionally.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/kvm/hyperv.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index 6da20f91cd59..503829f71270 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -2028,13 +2028,7 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
 			ent->ebx |= HV_DEBUGGING;
 			ent->edx |= HV_X64_GUEST_DEBUGGING_AVAILABLE;
 			ent->edx |= HV_FEATURE_DEBUG_MSRS_AVAILABLE;
-
-			/*
-			 * Direct Synthetic timers only make sense with in-kernel
-			 * LAPIC
-			 */
-			if (lapic_in_kernel(vcpu))
-				ent->edx |= HV_STIMER_DIRECT_MODE_AVAILABLE;
+			ent->edx |= HV_STIMER_DIRECT_MODE_AVAILABLE;
 
 			break;
 
-- 
2.25.4


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 5/7] KVM: x86: hyper-v: drop now unneeded vcpu parameter from kvm_vcpu_ioctl_get_hv_cpuid()
  2020-09-24 14:57 [PATCH v2 0/7] KVM: x86: hyper-v: make KVM_GET_SUPPORTED_HV_CPUID more useful Vitaly Kuznetsov
                   ` (3 preceding siblings ...)
  2020-09-24 14:57 ` [PATCH v2 4/7] KVM: x86: hyper-v: always advertise HV_STIMER_DIRECT_MODE_AVAILABLE Vitaly Kuznetsov
@ 2020-09-24 14:57 ` Vitaly Kuznetsov
  2020-09-24 14:57 ` [PATCH v2 6/7] KVM: x86: hyper-v: allow KVM_GET_SUPPORTED_HV_CPUID as a system ioctl Vitaly Kuznetsov
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Vitaly Kuznetsov @ 2020-09-24 14:57 UTC (permalink / raw)
  To: kvm, Paolo Bonzini
  Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Eduardo Habkost,
	Jon Doron, linux-kernel

kvm_vcpu_ioctl_get_hv_cpuid() doesn't use its vcpu parameter anymore,
drop it. Also, the function is now untied from vcpu, rename it accordingly.

No functional change intended.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/kvm/hyperv.c | 3 +--
 arch/x86/kvm/hyperv.h | 3 +--
 arch/x86/kvm/x86.c    | 3 +--
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index 503829f71270..34d8a4c76828 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -1951,8 +1951,7 @@ int kvm_vm_ioctl_hv_eventfd(struct kvm *kvm, struct kvm_hyperv_eventfd *args)
 	return kvm_hv_eventfd_assign(kvm, args->conn_id, args->fd);
 }
 
-int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
-				struct kvm_cpuid_entry2 __user *entries)
+int kvm_get_hv_cpuid(struct kvm_cpuid2 *cpuid, struct kvm_cpuid_entry2 __user *entries)
 {
 	uint16_t evmcs_ver = 0;
 	struct kvm_cpuid_entry2 cpuid_entries[] = {
diff --git a/arch/x86/kvm/hyperv.h b/arch/x86/kvm/hyperv.h
index e68c6c2e9649..1fc4997e7f01 100644
--- a/arch/x86/kvm/hyperv.h
+++ b/arch/x86/kvm/hyperv.h
@@ -126,7 +126,6 @@ void kvm_hv_setup_tsc_page(struct kvm *kvm,
 void kvm_hv_init_vm(struct kvm *kvm);
 void kvm_hv_destroy_vm(struct kvm *kvm);
 int kvm_vm_ioctl_hv_eventfd(struct kvm *kvm, struct kvm_hyperv_eventfd *args);
-int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
-				struct kvm_cpuid_entry2 __user *entries);
+int kvm_get_hv_cpuid(struct kvm_cpuid2 *cpuid, struct kvm_cpuid_entry2 __user *entries);
 
 #endif
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 17f4995e80a7..4b1a092d9e51 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4748,8 +4748,7 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		if (copy_from_user(&cpuid, cpuid_arg, sizeof(cpuid)))
 			goto out;
 
-		r = kvm_vcpu_ioctl_get_hv_cpuid(vcpu, &cpuid,
-						cpuid_arg->entries);
+		r = kvm_get_hv_cpuid(&cpuid, cpuid_arg->entries);
 		if (r)
 			goto out;
 
-- 
2.25.4


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 6/7] KVM: x86: hyper-v: allow KVM_GET_SUPPORTED_HV_CPUID as a system ioctl
  2020-09-24 14:57 [PATCH v2 0/7] KVM: x86: hyper-v: make KVM_GET_SUPPORTED_HV_CPUID more useful Vitaly Kuznetsov
                   ` (4 preceding siblings ...)
  2020-09-24 14:57 ` [PATCH v2 5/7] KVM: x86: hyper-v: drop now unneeded vcpu parameter from kvm_vcpu_ioctl_get_hv_cpuid() Vitaly Kuznetsov
@ 2020-09-24 14:57 ` Vitaly Kuznetsov
  2020-09-24 14:57 ` [PATCH v2 7/7] KVM: selftests: test " Vitaly Kuznetsov
  2020-09-25 20:33 ` [PATCH v2 0/7] KVM: x86: hyper-v: make KVM_GET_SUPPORTED_HV_CPUID more useful Paolo Bonzini
  7 siblings, 0 replies; 13+ messages in thread
From: Vitaly Kuznetsov @ 2020-09-24 14:57 UTC (permalink / raw)
  To: kvm, Paolo Bonzini
  Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Eduardo Habkost,
	Jon Doron, linux-kernel

KVM_GET_SUPPORTED_HV_CPUID is a vCPU ioctl but its output is now
independent from vCPU and in some cases VMMs may want to use it as a system
ioctl instead. In particular, QEMU doesn CPU feature expansion before any
vCPU gets created so KVM_GET_SUPPORTED_HV_CPUID can't be used.

Convert KVM_GET_SUPPORTED_HV_CPUID to 'dual' system/vCPU ioctl with the
same meaning.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 Documentation/virt/kvm/api.rst |  4 ++--
 arch/x86/kvm/x86.c             | 43 ++++++++++++++++++++--------------
 include/uapi/linux/kvm.h       |  3 ++-
 3 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index a552d41308e1..c323b346f001 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -4455,9 +4455,9 @@ that KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2 is present.
 4.118 KVM_GET_SUPPORTED_HV_CPUID
 --------------------------------
 
-:Capability: KVM_CAP_HYPERV_CPUID
+:Capability: KVM_CAP_HYPERV_CPUID (vcpu), KVM_CAP_SYS_HYPERV_CPUID (system)
 :Architectures: x86
-:Type: vcpu ioctl
+:Type: system ioctl, vcpu ioctl
 :Parameters: struct kvm_cpuid2 (in/out)
 :Returns: 0 on success, -1 on error
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 4b1a092d9e51..6170489bb823 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3453,6 +3453,26 @@ static inline bool kvm_can_mwait_in_guest(void)
 		boot_cpu_has(X86_FEATURE_ARAT);
 }
 
+static int kvm_ioctl_get_supported_hv_cpuid(struct kvm_cpuid2 __user *cpuid_arg)
+{
+	struct kvm_cpuid2 cpuid;
+	int r;
+
+	r = -EFAULT;
+	if (copy_from_user(&cpuid, cpuid_arg, sizeof(cpuid)))
+		return r;
+
+	r = kvm_get_hv_cpuid(&cpuid, cpuid_arg->entries);
+	if (r)
+		return r;
+
+	r = -EFAULT;
+	if (copy_to_user(cpuid_arg, &cpuid, sizeof(cpuid)))
+		return r;
+
+	return 0;
+}
+
 int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 {
 	int r = 0;
@@ -3489,6 +3509,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_HYPERV_TLBFLUSH:
 	case KVM_CAP_HYPERV_SEND_IPI:
 	case KVM_CAP_HYPERV_CPUID:
+	case KVM_CAP_SYS_HYPERV_CPUID:
 	case KVM_CAP_PCI_SEGMENT:
 	case KVM_CAP_DEBUGREGS:
 	case KVM_CAP_X86_ROBUST_SINGLESTEP:
@@ -3671,6 +3692,9 @@ long kvm_arch_dev_ioctl(struct file *filp,
 	case KVM_GET_MSRS:
 		r = msr_io(NULL, argp, do_get_msr_feature, 1);
 		break;
+	case KVM_GET_SUPPORTED_HV_CPUID:
+		r = kvm_ioctl_get_supported_hv_cpuid(argp);
+		break;
 	default:
 		r = -EINVAL;
 		break;
@@ -4740,24 +4764,9 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		srcu_read_unlock(&vcpu->kvm->srcu, idx);
 		break;
 	}
-	case KVM_GET_SUPPORTED_HV_CPUID: {
-		struct kvm_cpuid2 __user *cpuid_arg = argp;
-		struct kvm_cpuid2 cpuid;
-
-		r = -EFAULT;
-		if (copy_from_user(&cpuid, cpuid_arg, sizeof(cpuid)))
-			goto out;
-
-		r = kvm_get_hv_cpuid(&cpuid, cpuid_arg->entries);
-		if (r)
-			goto out;
-
-		r = -EFAULT;
-		if (copy_to_user(cpuid_arg, &cpuid, sizeof(cpuid)))
-			goto out;
-		r = 0;
+	case KVM_GET_SUPPORTED_HV_CPUID:
+		r = kvm_ioctl_get_supported_hv_cpuid(argp);
 		break;
-	}
 	default:
 		r = -EINVAL;
 	}
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 7d8eced6f459..3ebd48e72cdd 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1037,6 +1037,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_SMALLER_MAXPHYADDR 185
 #define KVM_CAP_S390_DIAG318 186
 #define KVM_CAP_STEAL_TIME 187
+#define KVM_CAP_SYS_HYPERV_CPUID 188
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
@@ -1495,7 +1496,7 @@ struct kvm_enc_region {
 /* Available with KVM_CAP_MANUAL_DIRTY_LOG_PROTECT_2 */
 #define KVM_CLEAR_DIRTY_LOG          _IOWR(KVMIO, 0xc0, struct kvm_clear_dirty_log)
 
-/* Available with KVM_CAP_HYPERV_CPUID */
+/* Available with KVM_CAP_HYPERV_CPUID (vcpu) / KVM_CAP_SYS_HYPERV_CPUID (system) */
 #define KVM_GET_SUPPORTED_HV_CPUID _IOWR(KVMIO, 0xc1, struct kvm_cpuid2)
 
 /* Available with KVM_CAP_ARM_SVE */
-- 
2.25.4


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 7/7] KVM: selftests: test KVM_GET_SUPPORTED_HV_CPUID as a system ioctl
  2020-09-24 14:57 [PATCH v2 0/7] KVM: x86: hyper-v: make KVM_GET_SUPPORTED_HV_CPUID more useful Vitaly Kuznetsov
                   ` (5 preceding siblings ...)
  2020-09-24 14:57 ` [PATCH v2 6/7] KVM: x86: hyper-v: allow KVM_GET_SUPPORTED_HV_CPUID as a system ioctl Vitaly Kuznetsov
@ 2020-09-24 14:57 ` Vitaly Kuznetsov
  2020-09-25 20:33 ` [PATCH v2 0/7] KVM: x86: hyper-v: make KVM_GET_SUPPORTED_HV_CPUID more useful Paolo Bonzini
  7 siblings, 0 replies; 13+ messages in thread
From: Vitaly Kuznetsov @ 2020-09-24 14:57 UTC (permalink / raw)
  To: kvm, Paolo Bonzini
  Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Eduardo Habkost,
	Jon Doron, linux-kernel

KVM_GET_SUPPORTED_HV_CPUID is now supported as both vCPU and VM ioctl,
test that.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 .../testing/selftests/kvm/include/kvm_util.h  |  2 +
 tools/testing/selftests/kvm/lib/kvm_util.c    | 26 +++++++++++
 .../selftests/kvm/x86_64/hyperv_cpuid.c       | 46 +++++++++++++------
 3 files changed, 59 insertions(+), 15 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
index 919e161dd289..59482e4eb308 100644
--- a/tools/testing/selftests/kvm/include/kvm_util.h
+++ b/tools/testing/selftests/kvm/include/kvm_util.h
@@ -112,6 +112,8 @@ void vcpu_ioctl(struct kvm_vm *vm, uint32_t vcpuid, unsigned long ioctl,
 int _vcpu_ioctl(struct kvm_vm *vm, uint32_t vcpuid, unsigned long ioctl,
 		void *arg);
 void vm_ioctl(struct kvm_vm *vm, unsigned long ioctl, void *arg);
+void kvm_ioctl(struct kvm_vm *vm, unsigned long ioctl, void *arg);
+int _kvm_ioctl(struct kvm_vm *vm, unsigned long ioctl, void *arg);
 void vm_mem_region_set_flags(struct kvm_vm *vm, uint32_t slot, uint32_t flags);
 void vm_mem_region_move(struct kvm_vm *vm, uint32_t slot, uint64_t new_gpa);
 void vm_mem_region_delete(struct kvm_vm *vm, uint32_t slot);
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 74776ee228f2..d49e24a15836 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -1518,6 +1518,32 @@ void vm_ioctl(struct kvm_vm *vm, unsigned long cmd, void *arg)
 		cmd, ret, errno, strerror(errno));
 }
 
+/*
+ * KVM system ioctl
+ *
+ * Input Args:
+ *   vm - Virtual Machine
+ *   cmd - Ioctl number
+ *   arg - Argument to pass to the ioctl
+ *
+ * Return: None
+ *
+ * Issues an arbitrary ioctl on a KVM fd.
+ */
+void kvm_ioctl(struct kvm_vm *vm, unsigned long cmd, void *arg)
+{
+	int ret;
+
+	ret = ioctl(vm->kvm_fd, cmd, arg);
+	TEST_ASSERT(ret == 0, "KVM ioctl %lu failed, rc: %i errno: %i (%s)",
+		cmd, ret, errno, strerror(errno));
+}
+
+int _kvm_ioctl(struct kvm_vm *vm, unsigned long cmd, void *arg)
+{
+	return ioctl(vm->kvm_fd, cmd, arg);
+}
+
 /*
  * VM Dump
  *
diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_cpuid.c b/tools/testing/selftests/kvm/x86_64/hyperv_cpuid.c
index 8b24cb2e6a19..414b7587f0f9 100644
--- a/tools/testing/selftests/kvm/x86_64/hyperv_cpuid.c
+++ b/tools/testing/selftests/kvm/x86_64/hyperv_cpuid.c
@@ -102,20 +102,23 @@ static void test_hv_cpuid(struct kvm_cpuid2 *hv_cpuid_entries)
 
 }
 
-void test_hv_cpuid_e2big(struct kvm_vm *vm)
+void test_hv_cpuid_e2big(struct kvm_vm *vm, bool system)
 {
 	static struct kvm_cpuid2 cpuid = {.nent = 0};
 	int ret;
 
-	ret = _vcpu_ioctl(vm, VCPU_ID, KVM_GET_SUPPORTED_HV_CPUID, &cpuid);
+	if (!system)
+		ret = _vcpu_ioctl(vm, VCPU_ID, KVM_GET_SUPPORTED_HV_CPUID, &cpuid);
+	else
+		ret = _kvm_ioctl(vm, KVM_GET_SUPPORTED_HV_CPUID, &cpuid);
 
 	TEST_ASSERT(ret == -1 && errno == E2BIG,
-		    "KVM_GET_SUPPORTED_HV_CPUID didn't fail with -E2BIG when"
-		    " it should have: %d %d", ret, errno);
+		    "%s KVM_GET_SUPPORTED_HV_CPUID didn't fail with -E2BIG when"
+		    " it should have: %d %d", system ? "KVM" : "vCPU", ret, errno);
 }
 
 
-struct kvm_cpuid2 *kvm_get_supported_hv_cpuid(struct kvm_vm *vm)
+struct kvm_cpuid2 *kvm_get_supported_hv_cpuid(struct kvm_vm *vm, bool system)
 {
 	int nent = 20; /* should be enough */
 	static struct kvm_cpuid2 *cpuid;
@@ -129,7 +132,10 @@ struct kvm_cpuid2 *kvm_get_supported_hv_cpuid(struct kvm_vm *vm)
 
 	cpuid->nent = nent;
 
-	vcpu_ioctl(vm, VCPU_ID, KVM_GET_SUPPORTED_HV_CPUID, cpuid);
+	if (!system)
+		vcpu_ioctl(vm, VCPU_ID, KVM_GET_SUPPORTED_HV_CPUID, cpuid);
+	else
+		kvm_ioctl(vm, KVM_GET_SUPPORTED_HV_CPUID, cpuid);
 
 	return cpuid;
 }
@@ -150,22 +156,32 @@ int main(int argc, char *argv[])
 		exit(KSFT_SKIP);
 	}
 
-	for (stage = 0; stage < 2; stage++) {
+	vm = vm_create_default(VCPU_ID, 0, guest_code);
+
+	for (stage = 0; stage < 4; stage++) {
+		bool do_sys_ioctl = stage > 1;
+
+		/* Stages 0/1 test vCPU ioctl, stages 2/3 system ioctl */
+		if (do_sys_ioctl && !kvm_check_cap(KVM_CAP_SYS_HYPERV_CPUID)) {
+			print_skip("KVM_CAP_SYS_HYPERV_CPUID not supported");
+			break;
+		}
 
-		vm = vm_create_default(VCPU_ID, 0, guest_code);
 		switch (stage) {
 		case 0:
-			test_hv_cpuid_e2big(vm);
-			continue;
+		case 2:
+			test_hv_cpuid_e2big(vm, do_sys_ioctl);
+			break;
 		case 1:
+		case 3:
+			hv_cpuid_entries = kvm_get_supported_hv_cpuid(vm, do_sys_ioctl);
+			test_hv_cpuid(hv_cpuid_entries);
+			free(hv_cpuid_entries);
 			break;
 		}
-
-		hv_cpuid_entries = kvm_get_supported_hv_cpuid(vm);
-		test_hv_cpuid(hv_cpuid_entries);
-		free(hv_cpuid_entries);
-		kvm_vm_free(vm);
 	}
 
+	kvm_vm_free(vm);
+
 	return 0;
 }
-- 
2.25.4


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 4/7] KVM: x86: hyper-v: always advertise HV_STIMER_DIRECT_MODE_AVAILABLE
  2020-09-24 14:57 ` [PATCH v2 4/7] KVM: x86: hyper-v: always advertise HV_STIMER_DIRECT_MODE_AVAILABLE Vitaly Kuznetsov
@ 2020-09-25 20:32   ` Paolo Bonzini
  2020-09-29 10:36     ` Vitaly Kuznetsov
  0 siblings, 1 reply; 13+ messages in thread
From: Paolo Bonzini @ 2020-09-25 20:32 UTC (permalink / raw)
  To: Vitaly Kuznetsov, kvm
  Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Eduardo Habkost,
	Jon Doron, linux-kernel

On 24/09/20 16:57, Vitaly Kuznetsov wrote:
> HV_STIMER_DIRECT_MODE_AVAILABLE is the last conditionally set feature bit
> in KVM_GET_SUPPORTED_HV_CPUID but it doesn't have to be conditional: first,
> this bit is only an indication to userspace VMM that direct mode stimers
> are supported, it still requires manual enablement (enabling SynIC) to
> work so no VMM should just blindly copy it to guest CPUIDs. Second,
> lapic_in_kernel() is a must for SynIC. Expose the bit unconditionally.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  arch/x86/kvm/hyperv.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index 6da20f91cd59..503829f71270 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -2028,13 +2028,7 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
>  			ent->ebx |= HV_DEBUGGING;
>  			ent->edx |= HV_X64_GUEST_DEBUGGING_AVAILABLE;
>  			ent->edx |= HV_FEATURE_DEBUG_MSRS_AVAILABLE;
> -
> -			/*
> -			 * Direct Synthetic timers only make sense with in-kernel
> -			 * LAPIC
> -			 */
> -			if (lapic_in_kernel(vcpu))
> -				ent->edx |= HV_STIMER_DIRECT_MODE_AVAILABLE;
> +			ent->edx |= HV_STIMER_DIRECT_MODE_AVAILABLE;
>  
>  			break;
>  
> 

Sorry for the late reply.  I think this is making things worse.  It's
obviously okay to add a system KVM_GET_SUPPORTED_HV_CPUID, and I guess
it makes sense to have bits in there that require to enable a
capability.  For example, KVM_GET_SUPPORTED_CPUID has a couple bits such
as X2APIC, that we return even if they require in-kernel irqchip.

For the vCPU version however we should be able to copy the returned
leaves to KVM_SET_CPUID2, meaning that unsupported features should be
masked.

Paolo


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 0/7] KVM: x86: hyper-v: make KVM_GET_SUPPORTED_HV_CPUID more useful
  2020-09-24 14:57 [PATCH v2 0/7] KVM: x86: hyper-v: make KVM_GET_SUPPORTED_HV_CPUID more useful Vitaly Kuznetsov
                   ` (6 preceding siblings ...)
  2020-09-24 14:57 ` [PATCH v2 7/7] KVM: selftests: test " Vitaly Kuznetsov
@ 2020-09-25 20:33 ` Paolo Bonzini
  7 siblings, 0 replies; 13+ messages in thread
From: Paolo Bonzini @ 2020-09-25 20:33 UTC (permalink / raw)
  To: Vitaly Kuznetsov, kvm
  Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Eduardo Habkost,
	Jon Doron, linux-kernel

On 24/09/20 16:57, Vitaly Kuznetsov wrote:
> Changes since v1:
> - Rebased to kvm/queue [KVM_CAP_SYS_HYPERV_CPUID -> 188]
> 
> QEMU series using the feature:
> https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg02017.html
> 
> Original description:
> 
> KVM_GET_SUPPORTED_HV_CPUID was initially implemented as a vCPU ioctl but
> this is not very useful when VMM is just trying to query which Hyper-V
> features are supported by the host prior to creating VM/vCPUs. The data
> in KVM_GET_SUPPORTED_HV_CPUID is mostly static with a few exceptions but
> it seems we can change this. Add support for KVM_GET_SUPPORTED_HV_CPUID as
> a system ioctl as well.
> 
> QEMU specific description:
> In some cases QEMU needs to collect the information about which Hyper-V
> features are supported by KVM and pass it up the stack. For non-hyper-v
> features this is done with system-wide KVM_GET_SUPPORTED_CPUID/
> KVM_GET_MSRS ioctls but Hyper-V specific features don't get in the output
> (as Hyper-V CPUIDs intersect with KVM's). In QEMU, CPU feature expansion
> happens before any KVM vcpus are created so KVM_GET_SUPPORTED_HV_CPUID
> can't be used in its current shape.
> 
> Vitaly Kuznetsov (7):
>   KVM: x86: hyper-v: Mention SynDBG CPUID leaves in api.rst
>   KVM: x86: hyper-v: disallow configuring SynIC timers with no SynIC
>   KVM: x86: hyper-v: make KVM_GET_SUPPORTED_HV_CPUID output independent
>     of eVMCS enablement
>   KVM: x86: hyper-v: always advertise HV_STIMER_DIRECT_MODE_AVAILABLE
>   KVM: x86: hyper-v: drop now unneeded vcpu parameter from
>     kvm_vcpu_ioctl_get_hv_cpuid()
>   KVM: x86: hyper-v: allow KVM_GET_SUPPORTED_HV_CPUID as a system ioctl
>   KVM: selftests: test KVM_GET_SUPPORTED_HV_CPUID as a system ioctl
> 
>  Documentation/virt/kvm/api.rst                | 12 +--
>  arch/x86/include/asm/kvm_host.h               |  2 +-
>  arch/x86/kvm/hyperv.c                         | 30 ++++----
>  arch/x86/kvm/hyperv.h                         |  3 +-
>  arch/x86/kvm/vmx/evmcs.c                      |  8 +-
>  arch/x86/kvm/vmx/evmcs.h                      |  2 +-
>  arch/x86/kvm/x86.c                            | 44 ++++++-----
>  include/uapi/linux/kvm.h                      |  3 +-
>  .../testing/selftests/kvm/include/kvm_util.h  |  2 +
>  tools/testing/selftests/kvm/lib/kvm_util.c    | 26 +++++++
>  .../selftests/kvm/x86_64/hyperv_cpuid.c       | 77 +++++++++----------
>  11 files changed, 120 insertions(+), 89 deletions(-)
> 

Queued patches 1-2 while we discuss the rest, thanks.

Paolo


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 4/7] KVM: x86: hyper-v: always advertise HV_STIMER_DIRECT_MODE_AVAILABLE
  2020-09-25 20:32   ` Paolo Bonzini
@ 2020-09-29 10:36     ` Vitaly Kuznetsov
  2020-09-29 11:03       ` Paolo Bonzini
  0 siblings, 1 reply; 13+ messages in thread
From: Vitaly Kuznetsov @ 2020-09-29 10:36 UTC (permalink / raw)
  To: Paolo Bonzini, kvm
  Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Eduardo Habkost,
	Jon Doron, linux-kernel

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 24/09/20 16:57, Vitaly Kuznetsov wrote:
>> HV_STIMER_DIRECT_MODE_AVAILABLE is the last conditionally set feature bit
>> in KVM_GET_SUPPORTED_HV_CPUID but it doesn't have to be conditional: first,
>> this bit is only an indication to userspace VMM that direct mode stimers
>> are supported, it still requires manual enablement (enabling SynIC) to
>> work so no VMM should just blindly copy it to guest CPUIDs. Second,
>> lapic_in_kernel() is a must for SynIC. Expose the bit unconditionally.
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  arch/x86/kvm/hyperv.c | 8 +-------
>>  1 file changed, 1 insertion(+), 7 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
>> index 6da20f91cd59..503829f71270 100644
>> --- a/arch/x86/kvm/hyperv.c
>> +++ b/arch/x86/kvm/hyperv.c
>> @@ -2028,13 +2028,7 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
>>  			ent->ebx |= HV_DEBUGGING;
>>  			ent->edx |= HV_X64_GUEST_DEBUGGING_AVAILABLE;
>>  			ent->edx |= HV_FEATURE_DEBUG_MSRS_AVAILABLE;
>> -
>> -			/*
>> -			 * Direct Synthetic timers only make sense with in-kernel
>> -			 * LAPIC
>> -			 */
>> -			if (lapic_in_kernel(vcpu))
>> -				ent->edx |= HV_STIMER_DIRECT_MODE_AVAILABLE;
>> +			ent->edx |= HV_STIMER_DIRECT_MODE_AVAILABLE;
>>  
>>  			break;
>>  
>> 
>
> Sorry for the late reply.  I think this is making things worse.  It's
> obviously okay to add a system KVM_GET_SUPPORTED_HV_CPUID, and I guess
> it makes sense to have bits in there that require to enable a
> capability.  For example, KVM_GET_SUPPORTED_CPUID has a couple bits such
> as X2APIC, that we return even if they require in-kernel irqchip.
>
> For the vCPU version however we should be able to copy the returned
> leaves to KVM_SET_CPUID2, meaning that unsupported features should be
> masked.

What I don't quite like about exposing HV_STIMER_DIRECT_MODE_AVAILABLE
conditionally is that we're requiring userspace to have a certain
control flow: first, it needs to create irqchip and only then call
KVM_GET_SUPPORTED_HV_CPUID or it won't know that
HV_STIMER_DIRECT_MODE_AVAILABLE is supported. 

Also, are you only concerned about HV_STIMER_DIRECT_MODE_AVAILABLE? E.g.
PATCH3 of this series is somewhat similar, it exposes eVMCS even when
the corresponding CAP wasn't enabled.

While I slightly prefer to get rid of this conditional feature exposure
once and for all, I don't really feel very strong about it. We can have
the system ioctl which always exposes all supported features and vCPU
version which only exposes what is currently enabled. We would, however,
need to preserve some inconsistency as a legacy: e.g. SynIC bits are now
exposed unconditionally, even before KVM_CAP_HYPERV_SYNIC[2] is enabled
(and if we change that we will break at least QEMU).

-- 
Vitaly


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 4/7] KVM: x86: hyper-v: always advertise HV_STIMER_DIRECT_MODE_AVAILABLE
  2020-09-29 10:36     ` Vitaly Kuznetsov
@ 2020-09-29 11:03       ` Paolo Bonzini
  2020-09-29 11:26         ` Vitaly Kuznetsov
  0 siblings, 1 reply; 13+ messages in thread
From: Paolo Bonzini @ 2020-09-29 11:03 UTC (permalink / raw)
  To: Vitaly Kuznetsov, kvm
  Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Eduardo Habkost,
	Jon Doron, linux-kernel

On 29/09/20 12:36, Vitaly Kuznetsov wrote:
>> Sorry for the late reply.  I think this is making things worse.  It's
>> obviously okay to add a system KVM_GET_SUPPORTED_HV_CPUID, and I guess
>> it makes sense to have bits in there that require to enable a
>> capability.  For example, KVM_GET_SUPPORTED_CPUID has a couple bits such
>> as X2APIC, that we return even if they require in-kernel irqchip.
>>
>> For the vCPU version however we should be able to copy the returned
>> leaves to KVM_SET_CPUID2, meaning that unsupported features should be
>> masked.
> What I don't quite like about exposing HV_STIMER_DIRECT_MODE_AVAILABLE
> conditionally is that we're requiring userspace to have a certain
> control flow: first, it needs to create irqchip and only then call
> KVM_GET_SUPPORTED_HV_CPUID or it won't know that
> HV_STIMER_DIRECT_MODE_AVAILABLE is supported. 
> 
> Also, are you only concerned about HV_STIMER_DIRECT_MODE_AVAILABLE? E.g.
> PATCH3 of this series is somewhat similar, it exposes eVMCS even when
> the corresponding CAP wasn't enabled.

All of them, but this was only about the vCPU ioctl.  I agree with you
that the system ioctl should return everything unconditionally.

But perhaps the best thing to do is to deprecate the vCPU ioctl and just
leave it as is with all its quirks.

Paolo

> While I slightly prefer to get rid of this conditional feature exposure
> once and for all, I don't really feel very strong about it. We can have
> the system ioctl which always exposes all supported features and vCPU
> version which only exposes what is currently enabled. We would, however,
> need to preserve some inconsistency as a legacy: e.g. SynIC bits are now
> exposed unconditionally, even before KVM_CAP_HYPERV_SYNIC[2] is enabled
> (and if we change that we will break at least QEMU).


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 4/7] KVM: x86: hyper-v: always advertise HV_STIMER_DIRECT_MODE_AVAILABLE
  2020-09-29 11:03       ` Paolo Bonzini
@ 2020-09-29 11:26         ` Vitaly Kuznetsov
  0 siblings, 0 replies; 13+ messages in thread
From: Vitaly Kuznetsov @ 2020-09-29 11:26 UTC (permalink / raw)
  To: Paolo Bonzini, kvm
  Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Eduardo Habkost,
	Jon Doron, linux-kernel

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 29/09/20 12:36, Vitaly Kuznetsov wrote:
>>> Sorry for the late reply.  I think this is making things worse.  It's
>>> obviously okay to add a system KVM_GET_SUPPORTED_HV_CPUID, and I guess
>>> it makes sense to have bits in there that require to enable a
>>> capability.  For example, KVM_GET_SUPPORTED_CPUID has a couple bits such
>>> as X2APIC, that we return even if they require in-kernel irqchip.
>>>
>>> For the vCPU version however we should be able to copy the returned
>>> leaves to KVM_SET_CPUID2, meaning that unsupported features should be
>>> masked.
>> What I don't quite like about exposing HV_STIMER_DIRECT_MODE_AVAILABLE
>> conditionally is that we're requiring userspace to have a certain
>> control flow: first, it needs to create irqchip and only then call
>> KVM_GET_SUPPORTED_HV_CPUID or it won't know that
>> HV_STIMER_DIRECT_MODE_AVAILABLE is supported. 
>> 
>> Also, are you only concerned about HV_STIMER_DIRECT_MODE_AVAILABLE? E.g.
>> PATCH3 of this series is somewhat similar, it exposes eVMCS even when
>> the corresponding CAP wasn't enabled.
>
> All of them, but this was only about the vCPU ioctl.  I agree with you
> that the system ioctl should return everything unconditionally.
>
> But perhaps the best thing to do is to deprecate the vCPU ioctl and just
> leave it as is with all its quirks.
>

Ok, I'll do exactly that. I'm not sure if there are any
KVM_GET_SUPPORTED_HV_CPUID users out there bisedes QEMU/selftest but
let's take the 'safest' approach.

-- 
Vitaly


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2020-09-29 11:32 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-24 14:57 [PATCH v2 0/7] KVM: x86: hyper-v: make KVM_GET_SUPPORTED_HV_CPUID more useful Vitaly Kuznetsov
2020-09-24 14:57 ` [PATCH v2 1/7] KVM: x86: hyper-v: Mention SynDBG CPUID leaves in api.rst Vitaly Kuznetsov
2020-09-24 14:57 ` [PATCH v2 2/7] KVM: x86: hyper-v: disallow configuring SynIC timers with no SynIC Vitaly Kuznetsov
2020-09-24 14:57 ` [PATCH v2 3/7] KVM: x86: hyper-v: make KVM_GET_SUPPORTED_HV_CPUID output independent of eVMCS enablement Vitaly Kuznetsov
2020-09-24 14:57 ` [PATCH v2 4/7] KVM: x86: hyper-v: always advertise HV_STIMER_DIRECT_MODE_AVAILABLE Vitaly Kuznetsov
2020-09-25 20:32   ` Paolo Bonzini
2020-09-29 10:36     ` Vitaly Kuznetsov
2020-09-29 11:03       ` Paolo Bonzini
2020-09-29 11:26         ` Vitaly Kuznetsov
2020-09-24 14:57 ` [PATCH v2 5/7] KVM: x86: hyper-v: drop now unneeded vcpu parameter from kvm_vcpu_ioctl_get_hv_cpuid() Vitaly Kuznetsov
2020-09-24 14:57 ` [PATCH v2 6/7] KVM: x86: hyper-v: allow KVM_GET_SUPPORTED_HV_CPUID as a system ioctl Vitaly Kuznetsov
2020-09-24 14:57 ` [PATCH v2 7/7] KVM: selftests: test " Vitaly Kuznetsov
2020-09-25 20:33 ` [PATCH v2 0/7] KVM: x86: hyper-v: make KVM_GET_SUPPORTED_HV_CPUID more useful Paolo Bonzini

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).