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

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

[Kaya, Metin]

[-- Attachment #1: Type: text/plain, Size: 0 bytes --]



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-KVM-x86-xen-add-support-for-32-bit-guests-in-SCHEDOP.patch --]
[-- Type: text/x-patch; name="0001-KVM-x86-xen-add-support-for-32-bit-guests-in-SCHEDOP.patch", Size: 2871 bytes --]

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

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

[Kaya, Metin]

[-- Attachment #1: Type: text/plain, Size: 354 bytes --]

v2: Updated a comment and added a new one.
________________________________________
From: Kaya, Metin
Sent: 21 March 2022 16:46
To: Paolo Bonzini; kvm@vger.kernel.org
Cc: Woodhouse, David; Durrant, Paul; Boris Ostrovsky; linux-kernel@vger.kernel.org; x86@kernel.org
Subject: [PATCH 1/1] KVM: x86/xen: add support for 32-bit guests in SCHEDOP_poll

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-KVM-x86-xen-add-support-for-32-bit-guests-in-SCHEDOP.patch --]
[-- Type: text/x-patch; name="0001-KVM-x86-xen-add-support-for-32-bit-guests-in-SCHEDOP.patch", Size: 2919 bytes --]

From f19a8832e2e56f843fdc62740db1381d50946be3 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 | 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))
-- 
2.32.0

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

[Kaya, Metin]

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))
-- 
2.32.0

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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 3241 bytes --]

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?

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 731 bytes --]

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


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]