kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 00/13] fixes and cleanups for string I/O emulation
@ 2021-10-22 15:36 Paolo Bonzini
  2021-10-22 15:36 ` [PATCH 01/13] KVM: SEV-ES: rename guest_ins_data to sev_pio_data Paolo Bonzini
                   ` (12 more replies)
  0 siblings, 13 replies; 20+ messages in thread
From: Paolo Bonzini @ 2021-10-22 15:36 UTC (permalink / raw)
  To: linux-kernel, kvm; +Cc: mlevitsk, seanjc

This series is split in two parts:

- patch 7 fixes a bug in string I/O emulation for SEV-ES, where
  the size of the GHCB buffer can exceed the size of vcpu->arch.pio_data.
  If that happens, we need to go over the GHCB buffer in multiple passes.
  There are some preliminary cleanups in patches 1/3/6 too.

- the other patches clean emulator_pio_in and emulator_pio_in_out.  The
  first changes (patches 2/3/5) are more localized; they make the final
  SEV code a little easier to follow, and they remove unnecessary
  function arguments so that it is clearer which functions consume
  vcpu->arch.pio and which don't.  Patches starting from 8 on, instead,
  remove all usage of vcpu->arch.pio unless a userspace KVM_EXIT_IO is
  required.  In the end, IN is split clearly into a function that
  (if needed) fills in vcpu->arch.pio, and one that is intended for
  completion callbacks and consumes it.

Tested by booting a SEV-ES guest and with "regular" kvm-unit-tests.

Paolo

Paolo Bonzini (13):
  KVM: SEV-ES: rename guest_ins_data to sev_pio_data
  KVM: x86: leave vcpu->arch.pio.count alone in emulator_pio_in_out
  KVM: SEV-ES: clean up kvm_sev_es_ins/outs
  KVM: x86: split the two parts of emulator_pio_in
  KVM: x86: remove unnecessary arguments from complete_emulator_pio_in
  KVM: SEV-ES: keep INS functions together
  KVM: SEV-ES: go over the sev_pio_data buffer in multiple passes if
    needed
  KVM: x86: inline kernel_pio into its sole caller
  KVM: x86: move all vcpu->arch.pio* setup in emulator_pio_in_out
  KVM: x86: wean in-kernel PIO from vcpu->arch.pio*
  KVM: x86: wean fast IN from emulator_pio_in
  KVM: x86: de-underscorify __emulator_pio_in
  KVM: SEV-ES: reuse advance_sev_es_emulated_ins for OUT too

 arch/x86/include/asm/kvm_host.h |   3 +-
 arch/x86/kvm/trace.h            |   2 +-
 arch/x86/kvm/x86.c              | 193 +++++++++++++++++++-------------
 3 files changed, 117 insertions(+), 81 deletions(-)

-- 
2.27.0


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

* [PATCH 01/13] KVM: SEV-ES: rename guest_ins_data to sev_pio_data
  2021-10-22 15:36 [PATCH v2 00/13] fixes and cleanups for string I/O emulation Paolo Bonzini
@ 2021-10-22 15:36 ` Paolo Bonzini
  2021-10-22 15:36 ` [PATCH 02/13] KVM: x86: leave vcpu->arch.pio.count alone in emulator_pio_in_out Paolo Bonzini
                   ` (11 subsequent siblings)
  12 siblings, 0 replies; 20+ messages in thread
From: Paolo Bonzini @ 2021-10-22 15:36 UTC (permalink / raw)
  To: linux-kernel, kvm; +Cc: mlevitsk, seanjc, stable

We will be using this field for OUTS emulation as well, in case the
data that is pushed via OUTS spans more than one page.  In that case,
there will be a need to save the data pointer across exits to userspace.

So, change the name to something that refers to any kind of PIO.
Also spell out what it is used for, namely SEV-ES.

No functional change intended.

Cc: stable@vger.kernel.org
Fixes: 7ed9abfe8e9f ("KVM: SVM: Support string IO operations for an SEV-ES guest")
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/include/asm/kvm_host.h | 2 +-
 arch/x86/kvm/x86.c              | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index f8f48a7ec577..6bed6c416c6c 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -702,7 +702,7 @@ struct kvm_vcpu_arch {
 
 	struct kvm_pio_request pio;
 	void *pio_data;
-	void *guest_ins_data;
+	void *sev_pio_data;
 
 	u8 event_exit_inst_len;
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 381384a54790..379175b725a1 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -12370,7 +12370,7 @@ EXPORT_SYMBOL_GPL(kvm_sev_es_mmio_read);
 
 static int complete_sev_es_emulated_ins(struct kvm_vcpu *vcpu)
 {
-	memcpy(vcpu->arch.guest_ins_data, vcpu->arch.pio_data,
+	memcpy(vcpu->arch.sev_pio_data, vcpu->arch.pio_data,
 	       vcpu->arch.pio.count * vcpu->arch.pio.size);
 	vcpu->arch.pio.count = 0;
 
@@ -12402,7 +12402,7 @@ static int kvm_sev_es_ins(struct kvm_vcpu *vcpu, unsigned int size,
 	if (ret) {
 		vcpu->arch.pio.count = 0;
 	} else {
-		vcpu->arch.guest_ins_data = data;
+		vcpu->arch.sev_pio_data = data;
 		vcpu->arch.complete_userspace_io = complete_sev_es_emulated_ins;
 	}
 
-- 
2.27.0



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

* [PATCH 02/13] KVM: x86: leave vcpu->arch.pio.count alone in emulator_pio_in_out
  2021-10-22 15:36 [PATCH v2 00/13] fixes and cleanups for string I/O emulation Paolo Bonzini
  2021-10-22 15:36 ` [PATCH 01/13] KVM: SEV-ES: rename guest_ins_data to sev_pio_data Paolo Bonzini
@ 2021-10-22 15:36 ` Paolo Bonzini
  2021-10-22 15:36 ` [PATCH 03/13] KVM: SEV-ES: clean up kvm_sev_es_ins/outs Paolo Bonzini
                   ` (10 subsequent siblings)
  12 siblings, 0 replies; 20+ messages in thread
From: Paolo Bonzini @ 2021-10-22 15:36 UTC (permalink / raw)
  To: linux-kernel, kvm; +Cc: mlevitsk, seanjc, stable

Currently emulator_pio_in clears vcpu->arch.pio.count twice if
emulator_pio_in_out performs kernel PIO.  Move the clear into
emulator_pio_out where it is actually necessary.

No functional change intended.

Cc: stable@vger.kernel.org
Fixes: 7ed9abfe8e9f ("KVM: SVM: Support string IO operations for an SEV-ES guest")
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/x86.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 379175b725a1..dff28a4fbb21 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6914,10 +6914,8 @@ static int emulator_pio_in_out(struct kvm_vcpu *vcpu, int size,
 	vcpu->arch.pio.count  = count;
 	vcpu->arch.pio.size = size;
 
-	if (!kernel_pio(vcpu, vcpu->arch.pio_data)) {
-		vcpu->arch.pio.count = 0;
+	if (!kernel_pio(vcpu, vcpu->arch.pio_data))
 		return 1;
-	}
 
 	vcpu->run->exit_reason = KVM_EXIT_IO;
 	vcpu->run->io.direction = in ? KVM_EXIT_IO_IN : KVM_EXIT_IO_OUT;
@@ -6963,9 +6961,16 @@ static int emulator_pio_out(struct kvm_vcpu *vcpu, int size,
 			    unsigned short port, const void *val,
 			    unsigned int count)
 {
+	int ret;
+
 	memcpy(vcpu->arch.pio_data, val, size * count);
 	trace_kvm_pio(KVM_PIO_OUT, port, size, count, vcpu->arch.pio_data);
-	return emulator_pio_in_out(vcpu, size, port, (void *)val, count, false);
+	ret = emulator_pio_in_out(vcpu, size, port, (void *)val, count, false);
+	if (ret)
+                vcpu->arch.pio.count = 0;
+
+        return ret;
+
 }
 
 static int emulator_pio_out_emulated(struct x86_emulate_ctxt *ctxt,
-- 
2.27.0



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

* [PATCH 03/13] KVM: SEV-ES: clean up kvm_sev_es_ins/outs
  2021-10-22 15:36 [PATCH v2 00/13] fixes and cleanups for string I/O emulation Paolo Bonzini
  2021-10-22 15:36 ` [PATCH 01/13] KVM: SEV-ES: rename guest_ins_data to sev_pio_data Paolo Bonzini
  2021-10-22 15:36 ` [PATCH 02/13] KVM: x86: leave vcpu->arch.pio.count alone in emulator_pio_in_out Paolo Bonzini
@ 2021-10-22 15:36 ` Paolo Bonzini
  2021-10-22 15:36 ` [PATCH 04/13] KVM: x86: split the two parts of emulator_pio_in Paolo Bonzini
                   ` (9 subsequent siblings)
  12 siblings, 0 replies; 20+ messages in thread
From: Paolo Bonzini @ 2021-10-22 15:36 UTC (permalink / raw)
  To: linux-kernel, kvm; +Cc: mlevitsk, seanjc, stable

A few very small cleanups to the functions, smushed together because
the patch is already very small like this:

- inline emulator_pio_in_emulated and emulator_pio_out_emulated,
  since we already have the vCPU

- remove the data argument and pull setting vcpu->arch.sev_pio_data into
  the caller

- remove unnecessary clearing of vcpu->arch.pio.count when
  emulation is done by the kernel (and therefore vcpu->arch.pio.count
  is already clear on exit from emulator_pio_in and emulator_pio_out).

No functional change intended.

Cc: stable@vger.kernel.org
Fixes: 7ed9abfe8e9f ("KVM: SVM: Support string IO operations for an SEV-ES guest")
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/x86.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index dff28a4fbb21..78ed0fe9fa1e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -12383,34 +12383,32 @@ static int complete_sev_es_emulated_ins(struct kvm_vcpu *vcpu)
 }
 
 static int kvm_sev_es_outs(struct kvm_vcpu *vcpu, unsigned int size,
-			   unsigned int port, void *data,  unsigned int count)
+			   unsigned int port, unsigned int count)
 {
-	int ret;
+	int ret = emulator_pio_out(vcpu, size, port,
+				   vcpu->arch.sev_pio_data, count);
 
-	ret = emulator_pio_out_emulated(vcpu->arch.emulate_ctxt, size, port,
-					data, count);
-	if (ret)
+	if (ret) {
+		/* Emulation done by the kernel.  */
 		return ret;
+	}
 
 	vcpu->arch.pio.count = 0;
-
 	return 0;
 }
 
 static int kvm_sev_es_ins(struct kvm_vcpu *vcpu, unsigned int size,
-			  unsigned int port, void *data, unsigned int count)
+			  unsigned int port, unsigned int count)
 {
-	int ret;
+	int ret = emulator_pio_in(vcpu, size, port,
+				  vcpu->arch.sev_pio_data, count);
 
-	ret = emulator_pio_in_emulated(vcpu->arch.emulate_ctxt, size, port,
-				       data, count);
 	if (ret) {
-		vcpu->arch.pio.count = 0;
-	} else {
-		vcpu->arch.sev_pio_data = data;
-		vcpu->arch.complete_userspace_io = complete_sev_es_emulated_ins;
+		/* Emulation done by the kernel.  */
+		return ret;
 	}
 
+	vcpu->arch.complete_userspace_io = complete_sev_es_emulated_ins;
 	return 0;
 }
 
@@ -12418,8 +12416,9 @@ int kvm_sev_es_string_io(struct kvm_vcpu *vcpu, unsigned int size,
 			 unsigned int port, void *data,  unsigned int count,
 			 int in)
 {
-	return in ? kvm_sev_es_ins(vcpu, size, port, data, count)
-		  : kvm_sev_es_outs(vcpu, size, port, data, count);
+	vcpu->arch.sev_pio_data = data;
+	return in ? kvm_sev_es_ins(vcpu, size, port, count)
+		  : kvm_sev_es_outs(vcpu, size, port, count);
 }
 EXPORT_SYMBOL_GPL(kvm_sev_es_string_io);
 
-- 
2.27.0



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

* [PATCH 04/13] KVM: x86: split the two parts of emulator_pio_in
  2021-10-22 15:36 [PATCH v2 00/13] fixes and cleanups for string I/O emulation Paolo Bonzini
                   ` (2 preceding siblings ...)
  2021-10-22 15:36 ` [PATCH 03/13] KVM: SEV-ES: clean up kvm_sev_es_ins/outs Paolo Bonzini
@ 2021-10-22 15:36 ` Paolo Bonzini
  2021-10-22 15:36 ` [PATCH 05/13] KVM: x86: remove unnecessary arguments from complete_emulator_pio_in Paolo Bonzini
                   ` (8 subsequent siblings)
  12 siblings, 0 replies; 20+ messages in thread
From: Paolo Bonzini @ 2021-10-22 15:36 UTC (permalink / raw)
  To: linux-kernel, kvm; +Cc: mlevitsk, seanjc, stable

emulator_pio_in handles both the case where the data is pending in
vcpu->arch.pio.count, and the case where I/O has to be done via either
an in-kernel device or a userspace exit.  For SEV-ES we would like
to split these, to identify clearly the moment at which the
sev_pio_data is consumed.  To this end, create two different
functions: __emulator_pio_in fills in vcpu->arch.pio.count, while
complete_emulator_pio_in clears it and releases vcpu->arch.pio.data.

Because this patch has to be backported, things are left a bit messy.
kernel_pio() operates on vcpu->arch.pio, which leads to emulator_pio_in()
having with two calls to complete_emulator_pio_in().  It will be fixed
in the next release.

While at it, remove the unused void* val argument of emulator_pio_in_out.
The function currently hardcodes vcpu->arch.pio_data as the
source/destination buffer, which sucks but will be fixed after the more
severe SEV-ES buffer overflow.

No functional change intended.

Cc: stable@vger.kernel.org
Fixes: 7ed9abfe8e9f ("KVM: SVM: Support string IO operations for an SEV-ES guest")
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/x86.c | 45 ++++++++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 78ed0fe9fa1e..c51ea81019e3 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6906,7 +6906,7 @@ static int kernel_pio(struct kvm_vcpu *vcpu, void *pd)
 }
 
 static int emulator_pio_in_out(struct kvm_vcpu *vcpu, int size,
-			       unsigned short port, void *val,
+			       unsigned short port,
 			       unsigned int count, bool in)
 {
 	vcpu->arch.pio.port = port;
@@ -6927,26 +6927,38 @@ static int emulator_pio_in_out(struct kvm_vcpu *vcpu, int size,
 	return 0;
 }
 
+static int __emulator_pio_in(struct kvm_vcpu *vcpu, int size,
+			     unsigned short port, unsigned int count)
+{
+	WARN_ON(vcpu->arch.pio.count);
+	memset(vcpu->arch.pio_data, 0, size * count);
+	return emulator_pio_in_out(vcpu, size, port, count, true);
+}
+
+static void complete_emulator_pio_in(struct kvm_vcpu *vcpu, int size,
+				    unsigned short port, void *val)
+{
+	memcpy(val, vcpu->arch.pio_data, size * vcpu->arch.pio.count);
+	trace_kvm_pio(KVM_PIO_IN, port, size, vcpu->arch.pio.count, vcpu->arch.pio_data);
+	vcpu->arch.pio.count = 0;
+}
+
 static int emulator_pio_in(struct kvm_vcpu *vcpu, int size,
 			   unsigned short port, void *val, unsigned int count)
 {
-	int ret;
+	if (vcpu->arch.pio.count) {
+		/* Complete previous iteration.  */
+	} else {
+		int r = __emulator_pio_in(vcpu, size, port, count);
+		if (!r)
+			return r;
 
-	if (vcpu->arch.pio.count)
-		goto data_avail;
-
-	memset(vcpu->arch.pio_data, 0, size * count);
-
-	ret = emulator_pio_in_out(vcpu, size, port, val, count, true);
-	if (ret) {
-data_avail:
-		memcpy(val, vcpu->arch.pio_data, size * count);
-		trace_kvm_pio(KVM_PIO_IN, port, size, count, vcpu->arch.pio_data);
-		vcpu->arch.pio.count = 0;
-		return 1;
+		/* Results already available, fall through.  */
 	}
 
-	return 0;
+	WARN_ON(count != vcpu->arch.pio.count);
+	complete_emulator_pio_in(vcpu, size, port, val);
+	return 1;
 }
 
 static int emulator_pio_in_emulated(struct x86_emulate_ctxt *ctxt,
@@ -6965,12 +6977,11 @@ static int emulator_pio_out(struct kvm_vcpu *vcpu, int size,
 
 	memcpy(vcpu->arch.pio_data, val, size * count);
 	trace_kvm_pio(KVM_PIO_OUT, port, size, count, vcpu->arch.pio_data);
-	ret = emulator_pio_in_out(vcpu, size, port, (void *)val, count, false);
+	ret = emulator_pio_in_out(vcpu, size, port, count, false);
 	if (ret)
                 vcpu->arch.pio.count = 0;
 
         return ret;
-
 }
 
 static int emulator_pio_out_emulated(struct x86_emulate_ctxt *ctxt,
-- 
2.27.0



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

* [PATCH 05/13] KVM: x86: remove unnecessary arguments from complete_emulator_pio_in
  2021-10-22 15:36 [PATCH v2 00/13] fixes and cleanups for string I/O emulation Paolo Bonzini
                   ` (3 preceding siblings ...)
  2021-10-22 15:36 ` [PATCH 04/13] KVM: x86: split the two parts of emulator_pio_in Paolo Bonzini
@ 2021-10-22 15:36 ` Paolo Bonzini
  2021-10-22 15:36 ` [PATCH 06/13] KVM: SEV-ES: keep INS functions together Paolo Bonzini
                   ` (7 subsequent siblings)
  12 siblings, 0 replies; 20+ messages in thread
From: Paolo Bonzini @ 2021-10-22 15:36 UTC (permalink / raw)
  To: linux-kernel, kvm; +Cc: mlevitsk, seanjc, stable

complete_emulator_pio_in can expect that vcpu->arch.pio has been filled in,
and therefore does not need the size and count arguments.  This makes things
nicer when the function is called directly from a complete_userspace_io
callback.

No functional change intended.

Cc: stable@vger.kernel.org
Fixes: 7ed9abfe8e9f ("KVM: SVM: Support string IO operations for an SEV-ES guest")
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/x86.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c51ea81019e3..63f9cb33cc19 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6935,11 +6935,12 @@ static int __emulator_pio_in(struct kvm_vcpu *vcpu, int size,
 	return emulator_pio_in_out(vcpu, size, port, count, true);
 }
 
-static void complete_emulator_pio_in(struct kvm_vcpu *vcpu, int size,
-				    unsigned short port, void *val)
+static void complete_emulator_pio_in(struct kvm_vcpu *vcpu, void *val)
 {
-	memcpy(val, vcpu->arch.pio_data, size * vcpu->arch.pio.count);
-	trace_kvm_pio(KVM_PIO_IN, port, size, vcpu->arch.pio.count, vcpu->arch.pio_data);
+	int size = vcpu->arch.pio.size;
+	unsigned count = vcpu->arch.pio.count;
+	memcpy(val, vcpu->arch.pio_data, size * count);
+	trace_kvm_pio(KVM_PIO_IN, vcpu->arch.pio.port, size, count, vcpu->arch.pio_data);
 	vcpu->arch.pio.count = 0;
 }
 
@@ -6957,7 +6958,7 @@ static int emulator_pio_in(struct kvm_vcpu *vcpu, int size,
 	}
 
 	WARN_ON(count != vcpu->arch.pio.count);
-	complete_emulator_pio_in(vcpu, size, port, val);
+	complete_emulator_pio_in(vcpu, val);
 	return 1;
 }
 
-- 
2.27.0



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

* [PATCH 06/13] KVM: SEV-ES: keep INS functions together
  2021-10-22 15:36 [PATCH v2 00/13] fixes and cleanups for string I/O emulation Paolo Bonzini
                   ` (4 preceding siblings ...)
  2021-10-22 15:36 ` [PATCH 05/13] KVM: x86: remove unnecessary arguments from complete_emulator_pio_in Paolo Bonzini
@ 2021-10-22 15:36 ` Paolo Bonzini
  2021-10-22 15:36 ` [PATCH 07/13] KVM: SEV-ES: go over the sev_pio_data buffer in multiple passes if needed Paolo Bonzini
                   ` (6 subsequent siblings)
  12 siblings, 0 replies; 20+ messages in thread
From: Paolo Bonzini @ 2021-10-22 15:36 UTC (permalink / raw)
  To: linux-kernel, kvm; +Cc: mlevitsk, seanjc, stable

Make the diff a little nicer when we actually get to fixing
the bug.  No functional change intended.

Cc: stable@vger.kernel.org
Fixes: 7ed9abfe8e9f ("KVM: SVM: Support string IO operations for an SEV-ES guest")
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/x86.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 63f9cb33cc19..23e772412134 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -12385,15 +12385,6 @@ int kvm_sev_es_mmio_read(struct kvm_vcpu *vcpu, gpa_t gpa, unsigned int bytes,
 }
 EXPORT_SYMBOL_GPL(kvm_sev_es_mmio_read);
 
-static int complete_sev_es_emulated_ins(struct kvm_vcpu *vcpu)
-{
-	memcpy(vcpu->arch.sev_pio_data, vcpu->arch.pio_data,
-	       vcpu->arch.pio.count * vcpu->arch.pio.size);
-	vcpu->arch.pio.count = 0;
-
-	return 1;
-}
-
 static int kvm_sev_es_outs(struct kvm_vcpu *vcpu, unsigned int size,
 			   unsigned int port, unsigned int count)
 {
@@ -12409,6 +12400,15 @@ static int kvm_sev_es_outs(struct kvm_vcpu *vcpu, unsigned int size,
 	return 0;
 }
 
+static int complete_sev_es_emulated_ins(struct kvm_vcpu *vcpu)
+{
+	memcpy(vcpu->arch.sev_pio_data, vcpu->arch.pio_data,
+	       vcpu->arch.pio.count * vcpu->arch.pio.size);
+	vcpu->arch.pio.count = 0;
+
+	return 1;
+}
+
 static int kvm_sev_es_ins(struct kvm_vcpu *vcpu, unsigned int size,
 			  unsigned int port, unsigned int count)
 {
-- 
2.27.0



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

* [PATCH 07/13] KVM: SEV-ES: go over the sev_pio_data buffer in multiple passes if needed
  2021-10-22 15:36 [PATCH v2 00/13] fixes and cleanups for string I/O emulation Paolo Bonzini
                   ` (5 preceding siblings ...)
  2021-10-22 15:36 ` [PATCH 06/13] KVM: SEV-ES: keep INS functions together Paolo Bonzini
@ 2021-10-22 15:36 ` Paolo Bonzini
  2021-10-22 15:36 ` [PATCH 08/13] KVM: x86: inline kernel_pio into its sole caller Paolo Bonzini
                   ` (5 subsequent siblings)
  12 siblings, 0 replies; 20+ messages in thread
From: Paolo Bonzini @ 2021-10-22 15:36 UTC (permalink / raw)
  To: linux-kernel, kvm; +Cc: mlevitsk, seanjc, stable, Felix Wilhelm

The PIO scratch buffer is larger than a single page, and therefore
it is not possible to copy it in a single step to vcpu->arch/pio_data.
Bound each call to emulator_pio_in/out to a single page; keep
track of how many I/O operations are left in vcpu->arch.sev_pio_count,
so that the operation can be restarted in the complete_userspace_io
callback.

For OUT, this means that the previous kvm_sev_es_outs implementation
becomes an iterator of the loop, and we can consume the sev_pio_data
buffer before leaving to userspace.

For IN, instead, consuming the buffer and decreasing sev_pio_count
is always done in the complete_userspace_io callback, because that
is when the memcpy is done into sev_pio_data.

Cc: stable@vger.kernel.org
Fixes: 7ed9abfe8e9f ("KVM: SVM: Support string IO operations for an SEV-ES guest")
Reported-by: Felix Wilhelm <fwilhelm@google.com>
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/x86.c              | 72 +++++++++++++++++++++++++--------
 2 files changed, 57 insertions(+), 16 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 6bed6c416c6c..5a0298aa56ba 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -703,6 +703,7 @@ struct kvm_vcpu_arch {
 	struct kvm_pio_request pio;
 	void *pio_data;
 	void *sev_pio_data;
+	unsigned sev_pio_count;
 
 	u8 event_exit_inst_len;
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 23e772412134..b26647a5ea22 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -12386,38 +12386,77 @@ int kvm_sev_es_mmio_read(struct kvm_vcpu *vcpu, gpa_t gpa, unsigned int bytes,
 EXPORT_SYMBOL_GPL(kvm_sev_es_mmio_read);
 
 static int kvm_sev_es_outs(struct kvm_vcpu *vcpu, unsigned int size,
-			   unsigned int port, unsigned int count)
+			   unsigned int port);
+
+static int complete_sev_es_emulated_outs(struct kvm_vcpu *vcpu)
 {
-	int ret = emulator_pio_out(vcpu, size, port,
-				   vcpu->arch.sev_pio_data, count);
+	int size = vcpu->arch.pio.size;
+	int port = vcpu->arch.pio.port;
+
+	vcpu->arch.pio.count = 0;
+	if (vcpu->arch.sev_pio_count)
+		return kvm_sev_es_outs(vcpu, size, port);
+	return 1;
+}
+
+static int kvm_sev_es_outs(struct kvm_vcpu *vcpu, unsigned int size,
+			   unsigned int port)
+{
+	for (;;) {
+		unsigned int count =
+			min_t(unsigned int, PAGE_SIZE / size, vcpu->arch.sev_pio_count);
+		int ret = emulator_pio_out(vcpu, size, port, vcpu->arch.sev_pio_data, count);
+
+		/* memcpy done already by emulator_pio_out.  */
+		vcpu->arch.sev_pio_count -= count;
+		vcpu->arch.sev_pio_data += count * vcpu->arch.pio.size;
+		if (!ret)
+			break;
 
-	if (ret) {
 		/* Emulation done by the kernel.  */
-		return ret;
+		if (!vcpu->arch.sev_pio_count)
+			return 1;
 	}
 
-	vcpu->arch.pio.count = 0;
+	vcpu->arch.complete_userspace_io = complete_sev_es_emulated_outs;
 	return 0;
 }
 
+static int kvm_sev_es_ins(struct kvm_vcpu *vcpu, unsigned int size,
+			  unsigned int port);
+
+static void advance_sev_es_emulated_ins(struct kvm_vcpu *vcpu)
+{
+	unsigned count = vcpu->arch.pio.count;
+	complete_emulator_pio_in(vcpu, vcpu->arch.sev_pio_data);
+	vcpu->arch.sev_pio_count -= count;
+	vcpu->arch.sev_pio_data += count * vcpu->arch.pio.size;
+}
+
 static int complete_sev_es_emulated_ins(struct kvm_vcpu *vcpu)
 {
-	memcpy(vcpu->arch.sev_pio_data, vcpu->arch.pio_data,
-	       vcpu->arch.pio.count * vcpu->arch.pio.size);
-	vcpu->arch.pio.count = 0;
+	int size = vcpu->arch.pio.size;
+	int port = vcpu->arch.pio.port;
 
+	advance_sev_es_emulated_ins(vcpu);
+	if (vcpu->arch.sev_pio_count)
+		return kvm_sev_es_ins(vcpu, size, port);
 	return 1;
 }
 
 static int kvm_sev_es_ins(struct kvm_vcpu *vcpu, unsigned int size,
-			  unsigned int port, unsigned int count)
+			  unsigned int port)
 {
-	int ret = emulator_pio_in(vcpu, size, port,
-				  vcpu->arch.sev_pio_data, count);
+	for (;;) {
+		unsigned int count =
+			min_t(unsigned int, PAGE_SIZE / size, vcpu->arch.sev_pio_count);
+		if (!__emulator_pio_in(vcpu, size, port, count))
+			break;
 
-	if (ret) {
 		/* Emulation done by the kernel.  */
-		return ret;
+		advance_sev_es_emulated_ins(vcpu);
+		if (!vcpu->arch.sev_pio_count)
+			return 1;
 	}
 
 	vcpu->arch.complete_userspace_io = complete_sev_es_emulated_ins;
@@ -12429,8 +12468,9 @@ int kvm_sev_es_string_io(struct kvm_vcpu *vcpu, unsigned int size,
 			 int in)
 {
 	vcpu->arch.sev_pio_data = data;
-	return in ? kvm_sev_es_ins(vcpu, size, port, count)
-		  : kvm_sev_es_outs(vcpu, size, port, count);
+	vcpu->arch.sev_pio_count = count;
+	return in ? kvm_sev_es_ins(vcpu, size, port)
+		  : kvm_sev_es_outs(vcpu, size, port);
 }
 EXPORT_SYMBOL_GPL(kvm_sev_es_string_io);
 
-- 
2.27.0



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

* [PATCH 08/13] KVM: x86: inline kernel_pio into its sole caller
  2021-10-22 15:36 [PATCH v2 00/13] fixes and cleanups for string I/O emulation Paolo Bonzini
                   ` (6 preceding siblings ...)
  2021-10-22 15:36 ` [PATCH 07/13] KVM: SEV-ES: go over the sev_pio_data buffer in multiple passes if needed Paolo Bonzini
@ 2021-10-22 15:36 ` Paolo Bonzini
  2021-10-26 13:55   ` Maxim Levitsky
  2021-10-22 15:36 ` [PATCH 09/13] KVM: x86: move all vcpu->arch.pio* setup in emulator_pio_in_out Paolo Bonzini
                   ` (4 subsequent siblings)
  12 siblings, 1 reply; 20+ messages in thread
From: Paolo Bonzini @ 2021-10-22 15:36 UTC (permalink / raw)
  To: linux-kernel, kvm; +Cc: mlevitsk, seanjc

The caller of kernel_pio already has arguments for most of what kernel_pio
fishes out of vcpu->arch.pio.  This is the first step towards ensuring that
vcpu->arch.pio.* is only used when exiting to userspace.

We can now also WARN if emulated PIO performs successful in-kernel iterations
before having to fall back to userspace.  The code is not ready for that, and
it should never happen.

No functional change intended.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/x86.c | 39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b26647a5ea22..d6b8df7cea80 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6886,37 +6886,32 @@ static int emulator_cmpxchg_emulated(struct x86_emulate_ctxt *ctxt,
 	return emulator_write_emulated(ctxt, addr, new, bytes, exception);
 }
 
-static int kernel_pio(struct kvm_vcpu *vcpu, void *pd)
-{
-	int r = 0, i;
-
-	for (i = 0; i < vcpu->arch.pio.count; i++) {
-		if (vcpu->arch.pio.in)
-			r = kvm_io_bus_read(vcpu, KVM_PIO_BUS, vcpu->arch.pio.port,
-					    vcpu->arch.pio.size, pd);
-		else
-			r = kvm_io_bus_write(vcpu, KVM_PIO_BUS,
-					     vcpu->arch.pio.port, vcpu->arch.pio.size,
-					     pd);
-		if (r)
-			break;
-		pd += vcpu->arch.pio.size;
-	}
-	return r;
-}
-
 static int emulator_pio_in_out(struct kvm_vcpu *vcpu, int size,
 			       unsigned short port,
 			       unsigned int count, bool in)
 {
+	void *data = vcpu->arch.pio_data;
+	unsigned i;
+	int r;
+
 	vcpu->arch.pio.port = port;
 	vcpu->arch.pio.in = in;
-	vcpu->arch.pio.count  = count;
+	vcpu->arch.pio.count = count;
 	vcpu->arch.pio.size = size;
 
-	if (!kernel_pio(vcpu, vcpu->arch.pio_data))
-		return 1;
+	for (i = 0; i < count; i++) {
+		if (in)
+			r = kvm_io_bus_read(vcpu, KVM_PIO_BUS, port, size, data);
+		else
+			r = kvm_io_bus_write(vcpu, KVM_PIO_BUS, port, size, data);
+		if (r)
+			goto userspace_io;
+		data += size;
+	}
+	return 1;
 
+userspace_io:
+	WARN_ON(i != 0);
 	vcpu->run->exit_reason = KVM_EXIT_IO;
 	vcpu->run->io.direction = in ? KVM_EXIT_IO_IN : KVM_EXIT_IO_OUT;
 	vcpu->run->io.size = size;
-- 
2.27.0



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

* [PATCH 09/13] KVM: x86: move all vcpu->arch.pio* setup in emulator_pio_in_out
  2021-10-22 15:36 [PATCH v2 00/13] fixes and cleanups for string I/O emulation Paolo Bonzini
                   ` (7 preceding siblings ...)
  2021-10-22 15:36 ` [PATCH 08/13] KVM: x86: inline kernel_pio into its sole caller Paolo Bonzini
@ 2021-10-22 15:36 ` Paolo Bonzini
  2021-10-26 13:56   ` Maxim Levitsky
  2021-10-22 15:36 ` [PATCH 10/13] KVM: x86: wean in-kernel PIO from vcpu->arch.pio* Paolo Bonzini
                   ` (3 subsequent siblings)
  12 siblings, 1 reply; 20+ messages in thread
From: Paolo Bonzini @ 2021-10-22 15:36 UTC (permalink / raw)
  To: linux-kernel, kvm; +Cc: mlevitsk, seanjc

For now, this is basically an excuse to add back the void* argument to
the function, while removing some knowledge of vcpu->arch.pio* from
its callers.  The WARN that vcpu->arch.pio.count is zero is also
extended to OUT operations.

We cannot do more as long as we have __emulator_pio_in always followed
by complete_emulator_pio_in, which uses the vcpu->arch.pio* fields.
But after fixing that, it will be possible to only populate the
vcpu->arch.pio* fields on userspace exits.

No functional change intended.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/trace.h |  2 +-
 arch/x86/kvm/x86.c   | 18 ++++++++++--------
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index 03ebe368333e..1b0167ae9e24 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -152,7 +152,7 @@ TRACE_EVENT(kvm_xen_hypercall,
 
 TRACE_EVENT(kvm_pio,
 	TP_PROTO(unsigned int rw, unsigned int port, unsigned int size,
-		 unsigned int count, void *data),
+		 unsigned int count, const void *data),
 	TP_ARGS(rw, port, size, count, data),
 
 	TP_STRUCT__entry(
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d6b8df7cea80..7c421d9fbcb6 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6887,17 +6887,22 @@ static int emulator_cmpxchg_emulated(struct x86_emulate_ctxt *ctxt,
 }
 
 static int emulator_pio_in_out(struct kvm_vcpu *vcpu, int size,
-			       unsigned short port,
+			       unsigned short port, void *data,
 			       unsigned int count, bool in)
 {
-	void *data = vcpu->arch.pio_data;
 	unsigned i;
 	int r;
 
+	WARN_ON_ONCE(vcpu->arch.pio.count);
 	vcpu->arch.pio.port = port;
 	vcpu->arch.pio.in = in;
 	vcpu->arch.pio.count = count;
 	vcpu->arch.pio.size = size;
+	if (in)
+		memset(vcpu->arch.pio_data, 0, size * count);
+	else
+		memcpy(vcpu->arch.pio_data, data, size * count);
+	data = vcpu->arch.pio_data;
 
 	for (i = 0; i < count; i++) {
 		if (in)
@@ -6925,9 +6930,7 @@ static int emulator_pio_in_out(struct kvm_vcpu *vcpu, int size,
 static int __emulator_pio_in(struct kvm_vcpu *vcpu, int size,
 			     unsigned short port, unsigned int count)
 {
-	WARN_ON(vcpu->arch.pio.count);
-	memset(vcpu->arch.pio_data, 0, size * count);
-	return emulator_pio_in_out(vcpu, size, port, count, true);
+	return emulator_pio_in_out(vcpu, size, port, NULL, count, true);
 }
 
 static void complete_emulator_pio_in(struct kvm_vcpu *vcpu, void *val)
@@ -6971,9 +6974,8 @@ static int emulator_pio_out(struct kvm_vcpu *vcpu, int size,
 {
 	int ret;
 
-	memcpy(vcpu->arch.pio_data, val, size * count);
-	trace_kvm_pio(KVM_PIO_OUT, port, size, count, vcpu->arch.pio_data);
-	ret = emulator_pio_in_out(vcpu, size, port, count, false);
+	trace_kvm_pio(KVM_PIO_OUT, port, size, count, val);
+	ret = emulator_pio_in_out(vcpu, size, port, (void *)val, count, false);
 	if (ret)
                 vcpu->arch.pio.count = 0;
 
-- 
2.27.0



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

* [PATCH 10/13] KVM: x86: wean in-kernel PIO from vcpu->arch.pio*
  2021-10-22 15:36 [PATCH v2 00/13] fixes and cleanups for string I/O emulation Paolo Bonzini
                   ` (8 preceding siblings ...)
  2021-10-22 15:36 ` [PATCH 09/13] KVM: x86: move all vcpu->arch.pio* setup in emulator_pio_in_out Paolo Bonzini
@ 2021-10-22 15:36 ` Paolo Bonzini
  2021-10-26 13:56   ` Maxim Levitsky
  2021-10-22 15:36 ` [PATCH 11/13] KVM: x86: wean fast IN from emulator_pio_in Paolo Bonzini
                   ` (2 subsequent siblings)
  12 siblings, 1 reply; 20+ messages in thread
From: Paolo Bonzini @ 2021-10-22 15:36 UTC (permalink / raw)
  To: linux-kernel, kvm; +Cc: mlevitsk, seanjc

Make emulator_pio_in_out operate directly on the provided buffer
as long as PIO is handled inside KVM.

For input operations, this means that, in the case of in-kernel
PIO, __emulator_pio_in does not have to be always followed
by complete_emulator_pio_in.  This affects emulator_pio_in and
kvm_sev_es_ins; for the latter, that is why the call moves from
advance_sev_es_emulated_ins to complete_sev_es_emulated_ins.

For output, it means that vcpu->pio.count is never set unnecessarily
and there is no need to clear it.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/x86.c | 63 +++++++++++++++++++++-------------------------
 1 file changed, 28 insertions(+), 35 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 7c421d9fbcb6..e3d3c13fe803 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6894,16 +6894,6 @@ static int emulator_pio_in_out(struct kvm_vcpu *vcpu, int size,
 	int r;
 
 	WARN_ON_ONCE(vcpu->arch.pio.count);
-	vcpu->arch.pio.port = port;
-	vcpu->arch.pio.in = in;
-	vcpu->arch.pio.count = count;
-	vcpu->arch.pio.size = size;
-	if (in)
-		memset(vcpu->arch.pio_data, 0, size * count);
-	else
-		memcpy(vcpu->arch.pio_data, data, size * count);
-	data = vcpu->arch.pio_data;
-
 	for (i = 0; i < count; i++) {
 		if (in)
 			r = kvm_io_bus_read(vcpu, KVM_PIO_BUS, port, size, data);
@@ -6917,6 +6907,16 @@ static int emulator_pio_in_out(struct kvm_vcpu *vcpu, int size,
 
 userspace_io:
 	WARN_ON(i != 0);
+	vcpu->arch.pio.port = port;
+	vcpu->arch.pio.in = in;
+	vcpu->arch.pio.count = count;
+	vcpu->arch.pio.size = size;
+
+	if (in)
+		memset(vcpu->arch.pio_data, 0, size * count);
+	else
+		memcpy(vcpu->arch.pio_data, data, size * count);
+
 	vcpu->run->exit_reason = KVM_EXIT_IO;
 	vcpu->run->io.direction = in ? KVM_EXIT_IO_IN : KVM_EXIT_IO_OUT;
 	vcpu->run->io.size = size;
@@ -6928,9 +6928,13 @@ static int emulator_pio_in_out(struct kvm_vcpu *vcpu, int size,
 }
 
 static int __emulator_pio_in(struct kvm_vcpu *vcpu, int size,
-			     unsigned short port, unsigned int count)
+			     unsigned short port, void *val, unsigned int count)
 {
-	return emulator_pio_in_out(vcpu, size, port, NULL, count, true);
+	int r = emulator_pio_in_out(vcpu, size, port, val, count, true);
+	if (r)
+		trace_kvm_pio(KVM_PIO_IN, port, size, count, val);
+
+	return r;
 }
 
 static void complete_emulator_pio_in(struct kvm_vcpu *vcpu, void *val)
@@ -6947,17 +6951,12 @@ static int emulator_pio_in(struct kvm_vcpu *vcpu, int size,
 {
 	if (vcpu->arch.pio.count) {
 		/* Complete previous iteration.  */
+		WARN_ON(count != vcpu->arch.pio.count);
+		complete_emulator_pio_in(vcpu, val);
+		return 1;
 	} else {
-		int r = __emulator_pio_in(vcpu, size, port, count);
-		if (!r)
-			return r;
-
-		/* Results already available, fall through.  */
+		return __emulator_pio_in(vcpu, size, port, val, count);
 	}
-
-	WARN_ON(count != vcpu->arch.pio.count);
-	complete_emulator_pio_in(vcpu, val);
-	return 1;
 }
 
 static int emulator_pio_in_emulated(struct x86_emulate_ctxt *ctxt,
@@ -6972,14 +6971,8 @@ static int emulator_pio_out(struct kvm_vcpu *vcpu, int size,
 			    unsigned short port, const void *val,
 			    unsigned int count)
 {
-	int ret;
-
 	trace_kvm_pio(KVM_PIO_OUT, port, size, count, val);
-	ret = emulator_pio_in_out(vcpu, size, port, (void *)val, count, false);
-	if (ret)
-                vcpu->arch.pio.count = 0;
-
-        return ret;
+	return emulator_pio_in_out(vcpu, size, port, (void *)val, count, false);
 }
 
 static int emulator_pio_out_emulated(struct x86_emulate_ctxt *ctxt,
@@ -12422,20 +12415,20 @@ static int kvm_sev_es_outs(struct kvm_vcpu *vcpu, unsigned int size,
 static int kvm_sev_es_ins(struct kvm_vcpu *vcpu, unsigned int size,
 			  unsigned int port);
 
-static void advance_sev_es_emulated_ins(struct kvm_vcpu *vcpu)
+static void advance_sev_es_emulated_ins(struct kvm_vcpu *vcpu, unsigned count, int size)
 {
-	unsigned count = vcpu->arch.pio.count;
-	complete_emulator_pio_in(vcpu, vcpu->arch.sev_pio_data);
 	vcpu->arch.sev_pio_count -= count;
-	vcpu->arch.sev_pio_data += count * vcpu->arch.pio.size;
+	vcpu->arch.sev_pio_data += count * size;
 }
 
 static int complete_sev_es_emulated_ins(struct kvm_vcpu *vcpu)
 {
+	unsigned count = vcpu->arch.pio.count;
 	int size = vcpu->arch.pio.size;
 	int port = vcpu->arch.pio.port;
 
-	advance_sev_es_emulated_ins(vcpu);
+	complete_emulator_pio_in(vcpu, vcpu->arch.sev_pio_data);
+	advance_sev_es_emulated_ins(vcpu, count, size);
 	if (vcpu->arch.sev_pio_count)
 		return kvm_sev_es_ins(vcpu, size, port);
 	return 1;
@@ -12447,11 +12440,11 @@ static int kvm_sev_es_ins(struct kvm_vcpu *vcpu, unsigned int size,
 	for (;;) {
 		unsigned int count =
 			min_t(unsigned int, PAGE_SIZE / size, vcpu->arch.sev_pio_count);
-		if (!__emulator_pio_in(vcpu, size, port, count))
+		if (!__emulator_pio_in(vcpu, size, port, vcpu->arch.sev_pio_data, count))
 			break;
 
 		/* Emulation done by the kernel.  */
-		advance_sev_es_emulated_ins(vcpu);
+		advance_sev_es_emulated_ins(vcpu, count, size);
 		if (!vcpu->arch.sev_pio_count)
 			return 1;
 	}
-- 
2.27.0



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

* [PATCH 11/13] KVM: x86: wean fast IN from emulator_pio_in
  2021-10-22 15:36 [PATCH v2 00/13] fixes and cleanups for string I/O emulation Paolo Bonzini
                   ` (9 preceding siblings ...)
  2021-10-22 15:36 ` [PATCH 10/13] KVM: x86: wean in-kernel PIO from vcpu->arch.pio* Paolo Bonzini
@ 2021-10-22 15:36 ` Paolo Bonzini
  2021-10-26 13:56   ` Maxim Levitsky
  2021-10-22 15:36 ` [PATCH 12/13] KVM: x86: de-underscorify __emulator_pio_in Paolo Bonzini
  2021-10-22 15:36 ` [PATCH 13/13] KVM: SEV-ES: reuse advance_sev_es_emulated_ins for OUT too Paolo Bonzini
  12 siblings, 1 reply; 20+ messages in thread
From: Paolo Bonzini @ 2021-10-22 15:36 UTC (permalink / raw)
  To: linux-kernel, kvm; +Cc: mlevitsk, seanjc

Now that __emulator_pio_in already fills "val" for in-kernel PIO, it
is both simpler and clearer not to use emulator_pio_in.
Use the appropriate function in kvm_fast_pio_in and complete_fast_pio_in,
respectively __emulator_pio_in and complete_emulator_pio_in.

emulator_pio_in_emulated is now the last caller of emulator_pio_in.

No functional change intended.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/x86.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e3d3c13fe803..42826087afd9 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8061,11 +8061,7 @@ static int complete_fast_pio_in(struct kvm_vcpu *vcpu)
 	/* For size less than 4 we merge, else we zero extend */
 	val = (vcpu->arch.pio.size < 4) ? kvm_rax_read(vcpu) : 0;
 
-	/*
-	 * Since vcpu->arch.pio.count == 1 let emulator_pio_in perform
-	 * the copy and tracing
-	 */
-	emulator_pio_in(vcpu, vcpu->arch.pio.size, vcpu->arch.pio.port, &val, 1);
+	complete_emulator_pio_in(vcpu, &val);
 	kvm_rax_write(vcpu, val);
 
 	return kvm_skip_emulated_instruction(vcpu);
@@ -8080,7 +8076,7 @@ static int kvm_fast_pio_in(struct kvm_vcpu *vcpu, int size,
 	/* For size less than 4 we merge, else we zero extend */
 	val = (size < 4) ? kvm_rax_read(vcpu) : 0;
 
-	ret = emulator_pio_in(vcpu, size, port, &val, 1);
+	ret = __emulator_pio_in(vcpu, size, port, &val, 1);
 	if (ret) {
 		kvm_rax_write(vcpu, val);
 		return ret;
-- 
2.27.0



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

* [PATCH 12/13] KVM: x86: de-underscorify __emulator_pio_in
  2021-10-22 15:36 [PATCH v2 00/13] fixes and cleanups for string I/O emulation Paolo Bonzini
                   ` (10 preceding siblings ...)
  2021-10-22 15:36 ` [PATCH 11/13] KVM: x86: wean fast IN from emulator_pio_in Paolo Bonzini
@ 2021-10-22 15:36 ` Paolo Bonzini
  2021-10-26 13:56   ` Maxim Levitsky
  2021-10-22 15:36 ` [PATCH 13/13] KVM: SEV-ES: reuse advance_sev_es_emulated_ins for OUT too Paolo Bonzini
  12 siblings, 1 reply; 20+ messages in thread
From: Paolo Bonzini @ 2021-10-22 15:36 UTC (permalink / raw)
  To: linux-kernel, kvm; +Cc: mlevitsk, seanjc

Now all callers except emulator_pio_in_emulated are using
__emulator_pio_in/complete_emulator_pio_in explicitly.
Move the "either copy the result or attempt PIO" logic in
emulator_pio_in_emulated, and rename __emulator_pio_in to
just emulator_pio_in.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/x86.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 42826087afd9..c3a2f479604d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6927,7 +6927,7 @@ static int emulator_pio_in_out(struct kvm_vcpu *vcpu, int size,
 	return 0;
 }
 
-static int __emulator_pio_in(struct kvm_vcpu *vcpu, int size,
+static int emulator_pio_in(struct kvm_vcpu *vcpu, int size,
 			     unsigned short port, void *val, unsigned int count)
 {
 	int r = emulator_pio_in_out(vcpu, size, port, val, count, true);
@@ -6946,27 +6946,21 @@ static void complete_emulator_pio_in(struct kvm_vcpu *vcpu, void *val)
 	vcpu->arch.pio.count = 0;
 }
 
-static int emulator_pio_in(struct kvm_vcpu *vcpu, int size,
-			   unsigned short port, void *val, unsigned int count)
+static int emulator_pio_in_emulated(struct x86_emulate_ctxt *ctxt,
+				    int size, unsigned short port, void *val,
+				    unsigned int count)
 {
+	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
 	if (vcpu->arch.pio.count) {
 		/* Complete previous iteration.  */
 		WARN_ON(count != vcpu->arch.pio.count);
 		complete_emulator_pio_in(vcpu, val);
 		return 1;
 	} else {
-		return __emulator_pio_in(vcpu, size, port, val, count);
+		return emulator_pio_in(vcpu, size, port, val, count);
 	}
 }
 
-static int emulator_pio_in_emulated(struct x86_emulate_ctxt *ctxt,
-				    int size, unsigned short port, void *val,
-				    unsigned int count)
-{
-	return emulator_pio_in(emul_to_vcpu(ctxt), size, port, val, count);
-
-}
-
 static int emulator_pio_out(struct kvm_vcpu *vcpu, int size,
 			    unsigned short port, const void *val,
 			    unsigned int count)
@@ -8076,7 +8070,7 @@ static int kvm_fast_pio_in(struct kvm_vcpu *vcpu, int size,
 	/* For size less than 4 we merge, else we zero extend */
 	val = (size < 4) ? kvm_rax_read(vcpu) : 0;
 
-	ret = __emulator_pio_in(vcpu, size, port, &val, 1);
+	ret = emulator_pio_in(vcpu, size, port, &val, 1);
 	if (ret) {
 		kvm_rax_write(vcpu, val);
 		return ret;
@@ -12436,7 +12430,7 @@ static int kvm_sev_es_ins(struct kvm_vcpu *vcpu, unsigned int size,
 	for (;;) {
 		unsigned int count =
 			min_t(unsigned int, PAGE_SIZE / size, vcpu->arch.sev_pio_count);
-		if (!__emulator_pio_in(vcpu, size, port, vcpu->arch.sev_pio_data, count))
+		if (!emulator_pio_in(vcpu, size, port, vcpu->arch.sev_pio_data, count))
 			break;
 
 		/* Emulation done by the kernel.  */
-- 
2.27.0



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

* [PATCH 13/13] KVM: SEV-ES: reuse advance_sev_es_emulated_ins for OUT too
  2021-10-22 15:36 [PATCH v2 00/13] fixes and cleanups for string I/O emulation Paolo Bonzini
                   ` (11 preceding siblings ...)
  2021-10-22 15:36 ` [PATCH 12/13] KVM: x86: de-underscorify __emulator_pio_in Paolo Bonzini
@ 2021-10-22 15:36 ` Paolo Bonzini
  2021-10-26 13:57   ` Maxim Levitsky
  12 siblings, 1 reply; 20+ messages in thread
From: Paolo Bonzini @ 2021-10-22 15:36 UTC (permalink / raw)
  To: linux-kernel, kvm; +Cc: mlevitsk, seanjc

complete_emulator_pio_in only has to be called by
complete_sev_es_emulated_ins now; therefore, all that the function does
now is adjust sev_pio_count and sev_pio_data.  Which is the same for
both IN and OUT.

No functional change intended.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/x86.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c3a2f479604d..b9ce4cfec121 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -12365,6 +12365,12 @@ int kvm_sev_es_mmio_read(struct kvm_vcpu *vcpu, gpa_t gpa, unsigned int bytes,
 }
 EXPORT_SYMBOL_GPL(kvm_sev_es_mmio_read);
 
+static void advance_sev_es_emulated_pio(struct kvm_vcpu *vcpu, unsigned count, int size)
+{
+	vcpu->arch.sev_pio_count -= count;
+	vcpu->arch.sev_pio_data += count * size;
+}
+
 static int kvm_sev_es_outs(struct kvm_vcpu *vcpu, unsigned int size,
 			   unsigned int port);
 
@@ -12388,8 +12394,7 @@ static int kvm_sev_es_outs(struct kvm_vcpu *vcpu, unsigned int size,
 		int ret = emulator_pio_out(vcpu, size, port, vcpu->arch.sev_pio_data, count);
 
 		/* memcpy done already by emulator_pio_out.  */
-		vcpu->arch.sev_pio_count -= count;
-		vcpu->arch.sev_pio_data += count * vcpu->arch.pio.size;
+		advance_sev_es_emulated_pio(vcpu, count, size);
 		if (!ret)
 			break;
 
@@ -12405,12 +12410,6 @@ static int kvm_sev_es_outs(struct kvm_vcpu *vcpu, unsigned int size,
 static int kvm_sev_es_ins(struct kvm_vcpu *vcpu, unsigned int size,
 			  unsigned int port);
 
-static void advance_sev_es_emulated_ins(struct kvm_vcpu *vcpu, unsigned count, int size)
-{
-	vcpu->arch.sev_pio_count -= count;
-	vcpu->arch.sev_pio_data += count * size;
-}
-
 static int complete_sev_es_emulated_ins(struct kvm_vcpu *vcpu)
 {
 	unsigned count = vcpu->arch.pio.count;
@@ -12418,7 +12417,7 @@ static int complete_sev_es_emulated_ins(struct kvm_vcpu *vcpu)
 	int port = vcpu->arch.pio.port;
 
 	complete_emulator_pio_in(vcpu, vcpu->arch.sev_pio_data);
-	advance_sev_es_emulated_ins(vcpu, count, size);
+	advance_sev_es_emulated_pio(vcpu, count, size);
 	if (vcpu->arch.sev_pio_count)
 		return kvm_sev_es_ins(vcpu, size, port);
 	return 1;
@@ -12434,7 +12433,7 @@ static int kvm_sev_es_ins(struct kvm_vcpu *vcpu, unsigned int size,
 			break;
 
 		/* Emulation done by the kernel.  */
-		advance_sev_es_emulated_ins(vcpu, count, size);
+		advance_sev_es_emulated_pio(vcpu, count, size);
 		if (!vcpu->arch.sev_pio_count)
 			return 1;
 	}
-- 
2.27.0


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

* Re: [PATCH 08/13] KVM: x86: inline kernel_pio into its sole caller
  2021-10-22 15:36 ` [PATCH 08/13] KVM: x86: inline kernel_pio into its sole caller Paolo Bonzini
@ 2021-10-26 13:55   ` Maxim Levitsky
  0 siblings, 0 replies; 20+ messages in thread
From: Maxim Levitsky @ 2021-10-26 13:55 UTC (permalink / raw)
  To: Paolo Bonzini, linux-kernel, kvm; +Cc: seanjc

On Fri, 2021-10-22 at 11:36 -0400, Paolo Bonzini wrote:
> The caller of kernel_pio already has arguments for most of what kernel_pio
> fishes out of vcpu->arch.pio.  This is the first step towards ensuring that
> vcpu->arch.pio.* is only used when exiting to userspace.
> 
> We can now also WARN if emulated PIO performs successful in-kernel iterations
> before having to fall back to userspace.  The code is not ready for that, and
> it should never happen.
> 
> No functional change intended.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/x86.c | 39 +++++++++++++++++----------------------
>  1 file changed, 17 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b26647a5ea22..d6b8df7cea80 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6886,37 +6886,32 @@ static int emulator_cmpxchg_emulated(struct x86_emulate_ctxt *ctxt,
>  	return emulator_write_emulated(ctxt, addr, new, bytes, exception);
>  }
>  
> -static int kernel_pio(struct kvm_vcpu *vcpu, void *pd)
> -{
> -	int r = 0, i;
> -
> -	for (i = 0; i < vcpu->arch.pio.count; i++) {
> -		if (vcpu->arch.pio.in)
> -			r = kvm_io_bus_read(vcpu, KVM_PIO_BUS, vcpu->arch.pio.port,
> -					    vcpu->arch.pio.size, pd);
> -		else
> -			r = kvm_io_bus_write(vcpu, KVM_PIO_BUS,
> -					     vcpu->arch.pio.port, vcpu->arch.pio.size,
> -					     pd);
> -		if (r)
> -			break;
> -		pd += vcpu->arch.pio.size;
> -	}
> -	return r;
> -}
> -
>  static int emulator_pio_in_out(struct kvm_vcpu *vcpu, int size,
>  			       unsigned short port,
>  			       unsigned int count, bool in)
>  {
> +	void *data = vcpu->arch.pio_data;
> +	unsigned i;
> +	int r;
> +
>  	vcpu->arch.pio.port = port;
>  	vcpu->arch.pio.in = in;
> -	vcpu->arch.pio.count  = count;
> +	vcpu->arch.pio.count = count;
>  	vcpu->arch.pio.size = size;
>  
> -	if (!kernel_pio(vcpu, vcpu->arch.pio_data))
> -		return 1;
> +	for (i = 0; i < count; i++) {
> +		if (in)
> +			r = kvm_io_bus_read(vcpu, KVM_PIO_BUS, port, size, data);
> +		else
> +			r = kvm_io_bus_write(vcpu, KVM_PIO_BUS, port, size, data);
> +		if (r)
> +			goto userspace_io;
> +		data += size;
> +	}
> +	return 1;
>  
> +userspace_io:
> +	WARN_ON(i != 0);
>  	vcpu->run->exit_reason = KVM_EXIT_IO;
>  	vcpu->run->io.direction = in ? KVM_EXIT_IO_IN : KVM_EXIT_IO_OUT;
>  	vcpu->run->io.size = size;

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky


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

* Re: [PATCH 09/13] KVM: x86: move all vcpu->arch.pio* setup in emulator_pio_in_out
  2021-10-22 15:36 ` [PATCH 09/13] KVM: x86: move all vcpu->arch.pio* setup in emulator_pio_in_out Paolo Bonzini
@ 2021-10-26 13:56   ` Maxim Levitsky
  0 siblings, 0 replies; 20+ messages in thread
From: Maxim Levitsky @ 2021-10-26 13:56 UTC (permalink / raw)
  To: Paolo Bonzini, linux-kernel, kvm; +Cc: seanjc

On Fri, 2021-10-22 at 11:36 -0400, Paolo Bonzini wrote:
> For now, this is basically an excuse to add back the void* argument to
> the function, while removing some knowledge of vcpu->arch.pio* from
> its callers.  The WARN that vcpu->arch.pio.count is zero is also
> extended to OUT operations.
> 
> We cannot do more as long as we have __emulator_pio_in always followed
> by complete_emulator_pio_in, which uses the vcpu->arch.pio* fields.
> But after fixing that, it will be possible to only populate the
> vcpu->arch.pio* fields on userspace exits.
> 
> No functional change intended.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/trace.h |  2 +-
>  arch/x86/kvm/x86.c   | 18 ++++++++++--------
>  2 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
> index 03ebe368333e..1b0167ae9e24 100644
> --- a/arch/x86/kvm/trace.h
> +++ b/arch/x86/kvm/trace.h
> @@ -152,7 +152,7 @@ TRACE_EVENT(kvm_xen_hypercall,
>  
>  TRACE_EVENT(kvm_pio,
>  	TP_PROTO(unsigned int rw, unsigned int port, unsigned int size,
> -		 unsigned int count, void *data),
> +		 unsigned int count, const void *data),
>  	TP_ARGS(rw, port, size, count, data),
>  
>  	TP_STRUCT__entry(
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index d6b8df7cea80..7c421d9fbcb6 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6887,17 +6887,22 @@ static int emulator_cmpxchg_emulated(struct x86_emulate_ctxt *ctxt,
>  }
>  
>  static int emulator_pio_in_out(struct kvm_vcpu *vcpu, int size,
> -			       unsigned short port,
> +			       unsigned short port, void *data,
>  			       unsigned int count, bool in)
>  {
> -	void *data = vcpu->arch.pio_data;
>  	unsigned i;
>  	int r;
>  
> +	WARN_ON_ONCE(vcpu->arch.pio.count);
>  	vcpu->arch.pio.port = port;
>  	vcpu->arch.pio.in = in;
>  	vcpu->arch.pio.count = count;
>  	vcpu->arch.pio.size = size;

It won't hurt to add the assert that size * count < PAGE_SIZE here.

> +	if (in)
> +		memset(vcpu->arch.pio_data, 0, size * count);
> +	else
> +		memcpy(vcpu->arch.pio_data, data, size * count);
> +	data = vcpu->arch.pio_data;
>  
>  	for (i = 0; i < count; i++) {
>  		if (in)
> @@ -6925,9 +6930,7 @@ static int emulator_pio_in_out(struct kvm_vcpu *vcpu, int size,
>  static int __emulator_pio_in(struct kvm_vcpu *vcpu, int size,
>  			     unsigned short port, unsigned int count)
>  {
> -	WARN_ON(vcpu->arch.pio.count);
> -	memset(vcpu->arch.pio_data, 0, size * count);
> -	return emulator_pio_in_out(vcpu, size, port, count, true);
> +	return emulator_pio_in_out(vcpu, size, port, NULL, count, true);
>  }
>  
>  static void complete_emulator_pio_in(struct kvm_vcpu *vcpu, void *val)
> @@ -6971,9 +6974,8 @@ static int emulator_pio_out(struct kvm_vcpu *vcpu, int size,
>  {
>  	int ret;
>  
> -	memcpy(vcpu->arch.pio_data, val, size * count);
> -	trace_kvm_pio(KVM_PIO_OUT, port, size, count, vcpu->arch.pio_data);
> -	ret = emulator_pio_in_out(vcpu, size, port, count, false);
> +	trace_kvm_pio(KVM_PIO_OUT, port, size, count, val);
> +	ret = emulator_pio_in_out(vcpu, size, port, (void *)val, count, false);
>  	if (ret)
>                  vcpu->arch.pio.count = 0;
>  

Makes sense.
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky



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

* Re: [PATCH 10/13] KVM: x86: wean in-kernel PIO from vcpu->arch.pio*
  2021-10-22 15:36 ` [PATCH 10/13] KVM: x86: wean in-kernel PIO from vcpu->arch.pio* Paolo Bonzini
@ 2021-10-26 13:56   ` Maxim Levitsky
  0 siblings, 0 replies; 20+ messages in thread
From: Maxim Levitsky @ 2021-10-26 13:56 UTC (permalink / raw)
  To: Paolo Bonzini, linux-kernel, kvm; +Cc: seanjc

On Fri, 2021-10-22 at 11:36 -0400, Paolo Bonzini wrote:
> Make emulator_pio_in_out operate directly on the provided buffer
> as long as PIO is handled inside KVM.
> 
> For input operations, this means that, in the case of in-kernel
> PIO, __emulator_pio_in does not have to be always followed
> by complete_emulator_pio_in.  This affects emulator_pio_in and
> kvm_sev_es_ins; for the latter, that is why the call moves from
> advance_sev_es_emulated_ins to complete_sev_es_emulated_ins.
> 
> For output, it means that vcpu->pio.count is never set unnecessarily
> and there is no need to clear it.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/x86.c | 63 +++++++++++++++++++++-------------------------
>  1 file changed, 28 insertions(+), 35 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 7c421d9fbcb6..e3d3c13fe803 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6894,16 +6894,6 @@ static int emulator_pio_in_out(struct kvm_vcpu *vcpu, int size,
>  	int r;
>  
>  	WARN_ON_ONCE(vcpu->arch.pio.count);
> -	vcpu->arch.pio.port = port;
> -	vcpu->arch.pio.in = in;
> -	vcpu->arch.pio.count = count;
> -	vcpu->arch.pio.size = size;
> -	if (in)
> -		memset(vcpu->arch.pio_data, 0, size * count);
> -	else
> -		memcpy(vcpu->arch.pio_data, data, size * count);
> -	data = vcpu->arch.pio_data;
> -
>  	for (i = 0; i < count; i++) {
>  		if (in)
>  			r = kvm_io_bus_read(vcpu, KVM_PIO_BUS, port, size, data);
> @@ -6917,6 +6907,16 @@ static int emulator_pio_in_out(struct kvm_vcpu *vcpu, int size,
>  
>  userspace_io:
>  	WARN_ON(i != 0);
> +	vcpu->arch.pio.port = port;
> +	vcpu->arch.pio.in = in;
> +	vcpu->arch.pio.count = count;
> +	vcpu->arch.pio.size = size;
> +
> +	if (in)
> +		memset(vcpu->arch.pio_data, 0, size * count);
> +	else
> +		memcpy(vcpu->arch.pio_data, data, size * count);
> +
>  	vcpu->run->exit_reason = KVM_EXIT_IO;
>  	vcpu->run->io.direction = in ? KVM_EXIT_IO_IN : KVM_EXIT_IO_OUT;
>  	vcpu->run->io.size = size;
> @@ -6928,9 +6928,13 @@ static int emulator_pio_in_out(struct kvm_vcpu *vcpu, int size,
>  }
>  
>  static int __emulator_pio_in(struct kvm_vcpu *vcpu, int size,
> -			     unsigned short port, unsigned int count)
> +			     unsigned short port, void *val, unsigned int count)
>  {
> -	return emulator_pio_in_out(vcpu, size, port, NULL, count, true);
> +	int r = emulator_pio_in_out(vcpu, size, port, val, count, true);
> +	if (r)
> +		trace_kvm_pio(KVM_PIO_IN, port, size, count, val);

I have an idea to move the tracepoints that happen before userspace exit (this
one when we have in-kernel emulation, and one in emulator_pio_out to  emulator_pio_in_out.

> +
> +	return r;
>  }
>  
>  static void complete_emulator_pio_in(struct kvm_vcpu *vcpu, void *val)
> @@ -6947,17 +6951,12 @@ static int emulator_pio_in(struct kvm_vcpu *vcpu, int size,
>  {
>  	if (vcpu->arch.pio.count) {
>  		/* Complete previous iteration.  */
> +		WARN_ON(count != vcpu->arch.pio.count);
> +		complete_emulator_pio_in(vcpu, val);
> +		return 1;
>  	} else {
> -		int r = __emulator_pio_in(vcpu, size, port, count);
> -		if (!r)
> -			return r;
> -
> -		/* Results already available, fall through.  */
> +		return __emulator_pio_in(vcpu, size, port, val, count);
>  	}
> -
> -	WARN_ON(count != vcpu->arch.pio.count);
> -	complete_emulator_pio_in(vcpu, val);
> -	return 1;
>  }
>  
>  static int emulator_pio_in_emulated(struct x86_emulate_ctxt *ctxt,
> @@ -6972,14 +6971,8 @@ static int emulator_pio_out(struct kvm_vcpu *vcpu, int size,
>  			    unsigned short port, const void *val,
>  			    unsigned int count)
>  {
> -	int ret;
> -
>  	trace_kvm_pio(KVM_PIO_OUT, port, size, count, val);
> -	ret = emulator_pio_in_out(vcpu, size, port, (void *)val, count, false);
> -	if (ret)
> -                vcpu->arch.pio.count = 0;
> -
> -        return ret;
> +	return emulator_pio_in_out(vcpu, size, port, (void *)val, count, false);
>  }
>  
>  static int emulator_pio_out_emulated(struct x86_emulate_ctxt *ctxt,
> @@ -12422,20 +12415,20 @@ static int kvm_sev_es_outs(struct kvm_vcpu *vcpu, unsigned int size,
>  static int kvm_sev_es_ins(struct kvm_vcpu *vcpu, unsigned int size,
>  			  unsigned int port);
>  
> -static void advance_sev_es_emulated_ins(struct kvm_vcpu *vcpu)
> +static void advance_sev_es_emulated_ins(struct kvm_vcpu *vcpu, unsigned count, int size)
>  {
> -	unsigned count = vcpu->arch.pio.count;
> -	complete_emulator_pio_in(vcpu, vcpu->arch.sev_pio_data);
>  	vcpu->arch.sev_pio_count -= count;
> -	vcpu->arch.sev_pio_data += count * vcpu->arch.pio.size;
> +	vcpu->arch.sev_pio_data += count * size;
>  }
>  
>  static int complete_sev_es_emulated_ins(struct kvm_vcpu *vcpu)
>  {
> +	unsigned count = vcpu->arch.pio.count;
>  	int size = vcpu->arch.pio.size;
>  	int port = vcpu->arch.pio.port;
>  
> -	advance_sev_es_emulated_ins(vcpu);
> +	complete_emulator_pio_in(vcpu, vcpu->arch.sev_pio_data);
> +	advance_sev_es_emulated_ins(vcpu, count, size);
>  	if (vcpu->arch.sev_pio_count)
>  		return kvm_sev_es_ins(vcpu, size, port);
>  	return 1;
> @@ -12447,11 +12440,11 @@ static int kvm_sev_es_ins(struct kvm_vcpu *vcpu, unsigned int size,
>  	for (;;) {
>  		unsigned int count =
>  			min_t(unsigned int, PAGE_SIZE / size, vcpu->arch.sev_pio_count);
> -		if (!__emulator_pio_in(vcpu, size, port, count))
> +		if (!__emulator_pio_in(vcpu, size, port, vcpu->arch.sev_pio_data, count))
>  			break;
>  
>  		/* Emulation done by the kernel.  */
> -		advance_sev_es_emulated_ins(vcpu);
> +		advance_sev_es_emulated_ins(vcpu, count, size);
>  		if (!vcpu->arch.sev_pio_count)
>  			return 1;
>  	}

Looks good, was a bit difficult to follow, so I might have missed something.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky




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

* Re: [PATCH 11/13] KVM: x86: wean fast IN from emulator_pio_in
  2021-10-22 15:36 ` [PATCH 11/13] KVM: x86: wean fast IN from emulator_pio_in Paolo Bonzini
@ 2021-10-26 13:56   ` Maxim Levitsky
  0 siblings, 0 replies; 20+ messages in thread
From: Maxim Levitsky @ 2021-10-26 13:56 UTC (permalink / raw)
  To: Paolo Bonzini, linux-kernel, kvm; +Cc: seanjc

On Fri, 2021-10-22 at 11:36 -0400, Paolo Bonzini wrote:
> Now that __emulator_pio_in already fills "val" for in-kernel PIO, it
> is both simpler and clearer not to use emulator_pio_in.
> Use the appropriate function in kvm_fast_pio_in and complete_fast_pio_in,
> respectively __emulator_pio_in and complete_emulator_pio_in.
> 
> emulator_pio_in_emulated is now the last caller of emulator_pio_in.
> 
> No functional change intended.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/x86.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e3d3c13fe803..42826087afd9 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8061,11 +8061,7 @@ static int complete_fast_pio_in(struct kvm_vcpu *vcpu)
>  	/* For size less than 4 we merge, else we zero extend */
>  	val = (vcpu->arch.pio.size < 4) ? kvm_rax_read(vcpu) : 0;
>  
> -	/*
> -	 * Since vcpu->arch.pio.count == 1 let emulator_pio_in perform
> -	 * the copy and tracing
> -	 */
> -	emulator_pio_in(vcpu, vcpu->arch.pio.size, vcpu->arch.pio.port, &val, 1);
> +	complete_emulator_pio_in(vcpu, &val);
>  	kvm_rax_write(vcpu, val);
>  
>  	return kvm_skip_emulated_instruction(vcpu);
> @@ -8080,7 +8076,7 @@ static int kvm_fast_pio_in(struct kvm_vcpu *vcpu, int size,
>  	/* For size less than 4 we merge, else we zero extend */
>  	val = (size < 4) ? kvm_rax_read(vcpu) : 0;
>  
> -	ret = emulator_pio_in(vcpu, size, port, &val, 1);
> +	ret = __emulator_pio_in(vcpu, size, port, &val, 1);
>  	if (ret) {
>  		kvm_rax_write(vcpu, val);
>  		return ret;

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky


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

* Re: [PATCH 12/13] KVM: x86: de-underscorify __emulator_pio_in
  2021-10-22 15:36 ` [PATCH 12/13] KVM: x86: de-underscorify __emulator_pio_in Paolo Bonzini
@ 2021-10-26 13:56   ` Maxim Levitsky
  0 siblings, 0 replies; 20+ messages in thread
From: Maxim Levitsky @ 2021-10-26 13:56 UTC (permalink / raw)
  To: Paolo Bonzini, linux-kernel, kvm; +Cc: seanjc

On Fri, 2021-10-22 at 11:36 -0400, Paolo Bonzini wrote:
> Now all callers except emulator_pio_in_emulated are using
> __emulator_pio_in/complete_emulator_pio_in explicitly.
> Move the "either copy the result or attempt PIO" logic in
> emulator_pio_in_emulated, and rename __emulator_pio_in to
> just emulator_pio_in.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/x86.c | 22 ++++++++--------------
>  1 file changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 42826087afd9..c3a2f479604d 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6927,7 +6927,7 @@ static int emulator_pio_in_out(struct kvm_vcpu *vcpu, int size,
>  	return 0;
>  }
>  
> -static int __emulator_pio_in(struct kvm_vcpu *vcpu, int size,
> +static int emulator_pio_in(struct kvm_vcpu *vcpu, int size,
>  			     unsigned short port, void *val, unsigned int count)
>  {
>  	int r = emulator_pio_in_out(vcpu, size, port, val, count, true);
> @@ -6946,27 +6946,21 @@ static void complete_emulator_pio_in(struct kvm_vcpu *vcpu, void *val)
>  	vcpu->arch.pio.count = 0;
>  }
>  
> -static int emulator_pio_in(struct kvm_vcpu *vcpu, int size,
> -			   unsigned short port, void *val, unsigned int count)
> +static int emulator_pio_in_emulated(struct x86_emulate_ctxt *ctxt,
> +				    int size, unsigned short port, void *val,
> +				    unsigned int count)
>  {
> +	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
>  	if (vcpu->arch.pio.count) {
>  		/* Complete previous iteration.  */
>  		WARN_ON(count != vcpu->arch.pio.count);
>  		complete_emulator_pio_in(vcpu, val);
>  		return 1;
>  	} else {
> -		return __emulator_pio_in(vcpu, size, port, val, count);
> +		return emulator_pio_in(vcpu, size, port, val, count);
>  	}
>  }
>  
> -static int emulator_pio_in_emulated(struct x86_emulate_ctxt *ctxt,
> -				    int size, unsigned short port, void *val,
> -				    unsigned int count)
> -{
> -	return emulator_pio_in(emul_to_vcpu(ctxt), size, port, val, count);
> -
> -}
> -
>  static int emulator_pio_out(struct kvm_vcpu *vcpu, int size,
>  			    unsigned short port, const void *val,
>  			    unsigned int count)
> @@ -8076,7 +8070,7 @@ static int kvm_fast_pio_in(struct kvm_vcpu *vcpu, int size,
>  	/* For size less than 4 we merge, else we zero extend */
>  	val = (size < 4) ? kvm_rax_read(vcpu) : 0;
>  
> -	ret = __emulator_pio_in(vcpu, size, port, &val, 1);
> +	ret = emulator_pio_in(vcpu, size, port, &val, 1);
>  	if (ret) {
>  		kvm_rax_write(vcpu, val);
>  		return ret;
> @@ -12436,7 +12430,7 @@ static int kvm_sev_es_ins(struct kvm_vcpu *vcpu, unsigned int size,
>  	for (;;) {
>  		unsigned int count =
>  			min_t(unsigned int, PAGE_SIZE / size, vcpu->arch.sev_pio_count);
> -		if (!__emulator_pio_in(vcpu, size, port, vcpu->arch.sev_pio_data, count))
> +		if (!emulator_pio_in(vcpu, size, port, vcpu->arch.sev_pio_data, count))
>  			break;
>  
>  		/* Emulation done by the kernel.  */
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky


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

* Re: [PATCH 13/13] KVM: SEV-ES: reuse advance_sev_es_emulated_ins for OUT too
  2021-10-22 15:36 ` [PATCH 13/13] KVM: SEV-ES: reuse advance_sev_es_emulated_ins for OUT too Paolo Bonzini
@ 2021-10-26 13:57   ` Maxim Levitsky
  0 siblings, 0 replies; 20+ messages in thread
From: Maxim Levitsky @ 2021-10-26 13:57 UTC (permalink / raw)
  To: Paolo Bonzini, linux-kernel, kvm; +Cc: seanjc

On Fri, 2021-10-22 at 11:36 -0400, Paolo Bonzini wrote:
> complete_emulator_pio_in only has to be called by
> complete_sev_es_emulated_ins now; therefore, all that the function does
> now is adjust sev_pio_count and sev_pio_data.  Which is the same for
> both IN and OUT.
> 
> No functional change intended.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/x86.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c3a2f479604d..b9ce4cfec121 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -12365,6 +12365,12 @@ int kvm_sev_es_mmio_read(struct kvm_vcpu *vcpu, gpa_t gpa, unsigned int bytes,
>  }
>  EXPORT_SYMBOL_GPL(kvm_sev_es_mmio_read);
>  
> +static void advance_sev_es_emulated_pio(struct kvm_vcpu *vcpu, unsigned count, int size)
> +{
> +	vcpu->arch.sev_pio_count -= count;
> +	vcpu->arch.sev_pio_data += count * size;
> +}
> +
>  static int kvm_sev_es_outs(struct kvm_vcpu *vcpu, unsigned int size,
>  			   unsigned int port);
>  
> @@ -12388,8 +12394,7 @@ static int kvm_sev_es_outs(struct kvm_vcpu *vcpu, unsigned int size,
>  		int ret = emulator_pio_out(vcpu, size, port, vcpu->arch.sev_pio_data, count);
>  
>  		/* memcpy done already by emulator_pio_out.  */
> -		vcpu->arch.sev_pio_count -= count;
> -		vcpu->arch.sev_pio_data += count * vcpu->arch.pio.size;
> +		advance_sev_es_emulated_pio(vcpu, count, size);
>  		if (!ret)
>  			break;
>  
> @@ -12405,12 +12410,6 @@ static int kvm_sev_es_outs(struct kvm_vcpu *vcpu, unsigned int size,
>  static int kvm_sev_es_ins(struct kvm_vcpu *vcpu, unsigned int size,
>  			  unsigned int port);
>  
> -static void advance_sev_es_emulated_ins(struct kvm_vcpu *vcpu, unsigned count, int size)
> -{
> -	vcpu->arch.sev_pio_count -= count;
> -	vcpu->arch.sev_pio_data += count * size;
> -}
> -
>  static int complete_sev_es_emulated_ins(struct kvm_vcpu *vcpu)
>  {
>  	unsigned count = vcpu->arch.pio.count;
> @@ -12418,7 +12417,7 @@ static int complete_sev_es_emulated_ins(struct kvm_vcpu *vcpu)
>  	int port = vcpu->arch.pio.port;
>  
>  	complete_emulator_pio_in(vcpu, vcpu->arch.sev_pio_data);
> -	advance_sev_es_emulated_ins(vcpu, count, size);
> +	advance_sev_es_emulated_pio(vcpu, count, size);
>  	if (vcpu->arch.sev_pio_count)
>  		return kvm_sev_es_ins(vcpu, size, port);
>  	return 1;
> @@ -12434,7 +12433,7 @@ static int kvm_sev_es_ins(struct kvm_vcpu *vcpu, unsigned int size,
>  			break;
>  
>  		/* Emulation done by the kernel.  */
> -		advance_sev_es_emulated_ins(vcpu, count, size);
> +		advance_sev_es_emulated_pio(vcpu, count, size);
>  		if (!vcpu->arch.sev_pio_count)
>  			return 1;
>  	}

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky


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

end of thread, other threads:[~2021-10-26 13:57 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-22 15:36 [PATCH v2 00/13] fixes and cleanups for string I/O emulation Paolo Bonzini
2021-10-22 15:36 ` [PATCH 01/13] KVM: SEV-ES: rename guest_ins_data to sev_pio_data Paolo Bonzini
2021-10-22 15:36 ` [PATCH 02/13] KVM: x86: leave vcpu->arch.pio.count alone in emulator_pio_in_out Paolo Bonzini
2021-10-22 15:36 ` [PATCH 03/13] KVM: SEV-ES: clean up kvm_sev_es_ins/outs Paolo Bonzini
2021-10-22 15:36 ` [PATCH 04/13] KVM: x86: split the two parts of emulator_pio_in Paolo Bonzini
2021-10-22 15:36 ` [PATCH 05/13] KVM: x86: remove unnecessary arguments from complete_emulator_pio_in Paolo Bonzini
2021-10-22 15:36 ` [PATCH 06/13] KVM: SEV-ES: keep INS functions together Paolo Bonzini
2021-10-22 15:36 ` [PATCH 07/13] KVM: SEV-ES: go over the sev_pio_data buffer in multiple passes if needed Paolo Bonzini
2021-10-22 15:36 ` [PATCH 08/13] KVM: x86: inline kernel_pio into its sole caller Paolo Bonzini
2021-10-26 13:55   ` Maxim Levitsky
2021-10-22 15:36 ` [PATCH 09/13] KVM: x86: move all vcpu->arch.pio* setup in emulator_pio_in_out Paolo Bonzini
2021-10-26 13:56   ` Maxim Levitsky
2021-10-22 15:36 ` [PATCH 10/13] KVM: x86: wean in-kernel PIO from vcpu->arch.pio* Paolo Bonzini
2021-10-26 13:56   ` Maxim Levitsky
2021-10-22 15:36 ` [PATCH 11/13] KVM: x86: wean fast IN from emulator_pio_in Paolo Bonzini
2021-10-26 13:56   ` Maxim Levitsky
2021-10-22 15:36 ` [PATCH 12/13] KVM: x86: de-underscorify __emulator_pio_in Paolo Bonzini
2021-10-26 13:56   ` Maxim Levitsky
2021-10-22 15:36 ` [PATCH 13/13] KVM: SEV-ES: reuse advance_sev_es_emulated_ins for OUT too Paolo Bonzini
2021-10-26 13:57   ` Maxim Levitsky

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