diff mbox series

[1/1] KVM: x86/xen: add support for 32-bit guests in SCHEDOP_poll

Message ID 1647881191688.60603@amazon.com (mailing list archive)
State New, archived
Headers show
Series [1/1] KVM: x86/xen: add support for 32-bit guests in SCHEDOP_poll | expand

Commit Message

Kaya, Metin March 21, 2022, 4:46 p.m. UTC

Comments

Kaya, Metin March 21, 2022, 5:15 p.m. UTC | #1 
v2: Updated a comment and added a new one.
Kaya, Metin March 21, 2022, 5:27 p.m. UTC | #2 
From: Metin Kaya <metikaya@amazon.com>

This patch introduces compat version of struct sched_poll for
SCHEDOP_poll sub-operation of sched_op hypercall, reads correct amount
of data (16 bytes in 32-bit case, 24 bytes otherwise) by using new
compat_sched_poll struct, copies it to sched_poll properly, and lets
rest of the code run as is.

Signed-off-by: Metin Kaya <metikaya@amazon.com>
---
 arch/x86/kvm/xen.c | 31 +++++++++++++++++++++++++++----
 arch/x86/kvm/xen.h |  7 +++++++
 2 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index 7d01983d1087..2d0a5d2ca6f1 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -998,20 +998,43 @@ static bool kvm_xen_schedop_poll(struct kvm_vcpu *vcpu, bool longmode,
 	evtchn_port_t port, *ports;
 	gpa_t gpa;
 
-	if (!longmode || !lapic_in_kernel(vcpu) ||
+	if (!lapic_in_kernel(vcpu) ||
 	    !(vcpu->kvm->arch.xen_hvm_config.flags & KVM_XEN_HVM_CONFIG_EVTCHN_SEND))
 		return false;
 
 	idx = srcu_read_lock(&vcpu->kvm->srcu);
 	gpa = kvm_mmu_gva_to_gpa_system(vcpu, param, NULL);
 	srcu_read_unlock(&vcpu->kvm->srcu, idx);
-
-	if (!gpa || kvm_vcpu_read_guest(vcpu, gpa, &sched_poll,
-					sizeof(sched_poll))) {
+	if (!gpa) {
 		*r = -EFAULT;
 		return true;
 	}
 
+	if (IS_ENABLED(CONFIG_64BIT) && longmode) {
+		if (kvm_vcpu_read_guest(vcpu, gpa, &sched_poll,
+					sizeof(sched_poll))) {
+			*r = -EFAULT;
+			return true;
+		}
+	} else {
+		struct compat_sched_poll sp;
+
+		/*
+		 * Sanity check that __packed trick works fine and size of
+		 * compat_sched_poll is 16 bytes just like in the real Xen
+		 * 32-bit case.
+		 */
+		BUILD_BUG_ON(sizeof(struct compat_sched_poll) != 16);
+
+		if (kvm_vcpu_read_guest(vcpu, gpa, &sp, sizeof(sp))) {
+			*r = -EFAULT;
+			return true;
+		}
+		sched_poll.ports = (evtchn_port_t *)(unsigned long)(sp.ports);
+		sched_poll.nr_ports = sp.nr_ports;
+		sched_poll.timeout = sp.timeout;
+	}
+
 	if (unlikely(sched_poll.nr_ports > 1)) {
 		/* Xen (unofficially) limits number of pollers to 128 */
 		if (sched_poll.nr_ports > 128) {
diff --git a/arch/x86/kvm/xen.h b/arch/x86/kvm/xen.h
index ee5c4ae0755c..8b36d346fc9c 100644
--- a/arch/x86/kvm/xen.h
+++ b/arch/x86/kvm/xen.h
@@ -196,6 +196,13 @@ struct compat_shared_info {
 	struct compat_arch_shared_info arch;
 };
 
+struct compat_sched_poll {
+	/* This is actually a guest virtual address which points to ports. */
+	uint32_t ports;
+	unsigned int nr_ports;
+	uint64_t timeout;
+} __packed;
+
 #define COMPAT_EVTCHN_2L_NR_CHANNELS (8 *				\
 				      sizeof_field(struct compat_shared_info, \
 						   evtchn_pending))
David Woodhouse Nov. 24, 2022, 12:33 a.m. UTC | #3 
On Mon, 2022-03-21 at 17:27 +0000, Kaya, Metin wrote:
> From: Metin Kaya <metikaya@amazon.com>
> 
> This patch introduces compat version of struct sched_poll for
> SCHEDOP_poll sub-operation of sched_op hypercall, reads correct amount
> of data (16 bytes in 32-bit case, 24 bytes otherwise) by using new
> compat_sched_poll struct, copies it to sched_poll properly, and lets
> rest of the code run as is.
> 
> Signed-off-by: Metin Kaya <metikaya@amazon.com>

Could do with a test case. It's fairly simple to flip the 'longmode'
config even when the guest is actually in 64-bit mode, so it should be
fairly easy to add this in the xen_shinfo_test.

Other minor nits below...

> ---
>  arch/x86/kvm/xen.c | 31 +++++++++++++++++++++++++++----
>  arch/x86/kvm/xen.h |  7 +++++++
>  2 files changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> index 7d01983d1087..2d0a5d2ca6f1 100644
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c
> @@ -998,20 +998,43 @@ static bool kvm_xen_schedop_poll(struct kvm_vcpu *vcpu, bool longmode,
>  	evtchn_port_t port, *ports;
>  	gpa_t gpa;
>  
> -	if (!longmode || !lapic_in_kernel(vcpu) ||
> +	if (!lapic_in_kernel(vcpu) ||
>  	    !(vcpu->kvm->arch.xen_hvm_config.flags & KVM_XEN_HVM_CONFIG_EVTCHN_SEND))
>  		return false;
>  
>  	idx = srcu_read_lock(&vcpu->kvm->srcu);
>  	gpa = kvm_mmu_gva_to_gpa_system(vcpu, param, NULL);
>  	srcu_read_unlock(&vcpu->kvm->srcu, idx);
> -
> -	if (!gpa || kvm_vcpu_read_guest(vcpu, gpa, &sched_poll,
> -					sizeof(sched_poll))) {
> +	if (!gpa) {
>  		*r = -EFAULT;
>  		return true;
>  	}
>  
> +	if (IS_ENABLED(CONFIG_64BIT) && longmode) {


Make this 'if (!IS_ENABLED(CONFIG_64BIT || longmode) {'

You want to take this trivial path for the 32-bit host kernel too,
since struct sched_poll and its compat version are identical there.

> +		if (kvm_vcpu_read_guest(vcpu, gpa, &sched_poll,
> +					sizeof(sched_poll))) {
> +			*r = -EFAULT;
> +			return true;
> +		}
> +	} else {

Then this code path is only for IS_ENABLED(CONFIG_64BIT) && !longmode,
which is what we want.

> +		struct compat_sched_poll sp;
> +
> +		/*
> +		 * Sanity check that __packed trick works fine and size of
> +		 * compat_sched_poll is 16 bytes just like in the real Xen
> +		 * 32-bit case.
> +		 */
> +		BUILD_BUG_ON(sizeof(struct compat_sched_poll) != 16);
> +
> +		if (kvm_vcpu_read_guest(vcpu, gpa, &sp, sizeof(sp))) {
> +			*r = -EFAULT;
> +			return true;
> +		}
> +		sched_poll.ports = (evtchn_port_t *)(unsigned long)(sp.ports);
> +		sched_poll.nr_ports = sp.nr_ports;
> +		sched_poll.timeout = sp.timeout;
> +	}


> --- a/arch/x86/kvm/xen.h
> +++ b/arch/x86/kvm/xen.h
> @@ -196,6 +196,13 @@ struct compat_shared_info {
>  	struct compat_arch_shared_info arch;
>  };
>  
> +struct compat_sched_poll {
> +	/* This is actually a guest virtual address which points to ports. */
> +	uint32_t ports;
> +	unsigned int nr_ports;
> +	uint64_t timeout;
> +} __packed;
> +

We use __attribute__((packed)) elsewhere in the same file. I don't much
care, but consistency is good. If you want to use __packed, can you
change the other one too?
David Woodhouse Nov. 25, 2022, 12:34 p.m. UTC | #4 
On Thu, 2022-11-24 at 00:33 +0000, David Woodhouse wrote:
> Could do with a test case. It's fairly simple to flip the 'longmode'
> config even when the guest is actually in 64-bit mode, so it should be
> fairly easy to add this in the xen_shinfo_test.

Hm, not so. This doesn't depend on the 'longmode' flag; that's only for
the structs in shared memory. For an actual hypercall, the compat
behaviour depends more sensibly on the actual CPU mode in which the
hypercall was made.

I'm not sure I see how to use 32-bit guest mode from the KVM self
tests. Even KVM_HC_SEND_IPI doesn't seem to have a 32-bit test... or a
positive test at all, in fact. All we seem to do is test that it
returns -ENOSYS when *disabled*.
diff mbox series

Patch

From 49113959550525be40c23e8bfc4addf69edeca47 Mon Sep 17 00:00:00 2001
From: Metin Kaya <metikaya@amazon.com>
Date: Mon, 21 Mar 2022 11:05:32 +0000
Subject: [PATCH] KVM: x86/xen: add support for 32-bit guests in SCHEDOP_poll

This patch introduces compat version of struct sched_poll for
SCHEDOP_poll sub-operation of sched_op hypercall, reads correct amount
of data (16 bytes in 32-bit case, 24 bytes otherwise) by using new
compat_sched_poll struct, copies it to sched_poll properly, and lets
rest of the code run as is.

Signed-off-by: Metin Kaya <metikaya@amazon.com>
Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
Reviewed-by: Paul Durrant <pdurrant@amazon.co.uk>
---
 arch/x86/kvm/xen.c | 30 ++++++++++++++++++++++++++----
 arch/x86/kvm/xen.h |  7 +++++++
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index 7d01983d1087..c02163bf1a97 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -998,20 +998,42 @@  static bool kvm_xen_schedop_poll(struct kvm_vcpu *vcpu, bool longmode,
 	evtchn_port_t port, *ports;
 	gpa_t gpa;
 
-	if (!longmode || !lapic_in_kernel(vcpu) ||
+	if (!lapic_in_kernel(vcpu) ||
 	    !(vcpu->kvm->arch.xen_hvm_config.flags & KVM_XEN_HVM_CONFIG_EVTCHN_SEND))
 		return false;
 
 	idx = srcu_read_lock(&vcpu->kvm->srcu);
 	gpa = kvm_mmu_gva_to_gpa_system(vcpu, param, NULL);
 	srcu_read_unlock(&vcpu->kvm->srcu, idx);
-
-	if (!gpa || kvm_vcpu_read_guest(vcpu, gpa, &sched_poll,
-					sizeof(sched_poll))) {
+	if (!gpa) {
 		*r = -EFAULT;
 		return true;
 	}
 
+	if (IS_ENABLED(CONFIG_64BIT) && longmode) {
+		if (kvm_vcpu_read_guest(vcpu, gpa, &sched_poll,
+					sizeof(sched_poll))) {
+			*r = -EFAULT;
+			return true;
+		}
+	} else {
+		struct compat_sched_poll sp;
+
+		/*
+		 * We assume size of compat_sched_poll is 16 bytes in 32-bit
+		 * environment. Let's be honest.
+		 */
+		BUILD_BUG_ON(sizeof(struct compat_sched_poll) != 16);
+
+		if (kvm_vcpu_read_guest(vcpu, gpa, &sp, sizeof(sp))) {
+			*r = -EFAULT;
+			return true;
+		}
+		sched_poll.ports = (evtchn_port_t *)(unsigned long)(sp.ports);
+		sched_poll.nr_ports = sp.nr_ports;
+		sched_poll.timeout = sp.timeout;
+	}
+
 	if (unlikely(sched_poll.nr_ports > 1)) {
 		/* Xen (unofficially) limits number of pollers to 128 */
 		if (sched_poll.nr_ports > 128) {
diff --git a/arch/x86/kvm/xen.h b/arch/x86/kvm/xen.h
index ee5c4ae0755c..b5b208cd8c9f 100644
--- a/arch/x86/kvm/xen.h
+++ b/arch/x86/kvm/xen.h
@@ -196,6 +196,13 @@  struct compat_shared_info {
 	struct compat_arch_shared_info arch;
 };
 
+struct compat_sched_poll {
+	/* This is actually a pointer which has to be 4 bytes in size. */
+	uint32_t ports;
+	unsigned int nr_ports;
+	uint64_t timeout;
+} __packed;
+
 #define COMPAT_EVTCHN_2L_NR_CHANNELS (8 *				\
 				      sizeof_field(struct compat_shared_info, \
 						   evtchn_pending))
-- 
2.32.0