[PATCH 1/8] KVM: SEV-ES: fix length of string I/O

[PATCH 1/8] KVM: SEV-ES: fix length of string I/O

[Paolo Bonzini]

The size of the data in the scratch buffer is not divided by the size of
each port I/O operation, so vcpu->arch.pio.count ends up being larger
than it should be by a factor of size.

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/svm/sev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index c36b5fe4c27c..e672493b5d8d 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2583,7 +2583,7 @@ int sev_es_string_io(struct vcpu_svm *svm, int size, unsigned int port, int in)
 		return -EINVAL;
 
 	return kvm_sev_es_string_io(&svm->vcpu, size, port,
-				    svm->ghcb_sa, svm->ghcb_sa_len, in);
+				    svm->ghcb_sa, svm->ghcb_sa_len / size, in);
 }
 
 void sev_es_init_vmcb(struct vcpu_svm *svm)
-- 
2.27.0

[PATCH 2/8] KVM: SEV-ES: rename guest_ins_data to sev_pio_data

[Paolo Bonzini]

Since we will be using this for OUTS emulation as well, 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")
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 aabd3a2ec1bc..722f5fcf76e1 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -12369,7 +12369,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;
 
@@ -12401,7 +12401,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

[PATCH 3/8] KVM: x86: leave vcpu->arch.pio.count alone in emulator_pio_in_out

[Paolo Bonzini]

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")
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 722f5fcf76e1..218877e297e5 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

[PATCH 4/8] KVM: SEV-ES: clean up kvm_sev_es_ins/outs

[Paolo Bonzini]

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.

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 | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 218877e297e5..8880dc36a2b4 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -12382,34 +12382,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;
 }
 
@@ -12417,8 +12415,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

[PATCH 5/8] KVM: x86: split the two parts of emulator_pio_in

[Paolo Bonzini]

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.

While at it, remove the void* argument also from emulator_pio_in_out.

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 | 42 +++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 8880dc36a2b4..07d9533b471d 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,31 @@ 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, void *val, unsigned int count)
+static int __emulator_pio_in(struct kvm_vcpu *vcpu, int size,
+			     unsigned short port, unsigned int count)
 {
-	int ret;
-
-	if (vcpu->arch.pio.count)
-		goto data_avail;
-
+	WARN_ON(vcpu->arch.pio.count);
 	memset(vcpu->arch.pio_data, 0, size * count);
+	return emulator_pio_in_out(vcpu, size, port, count, true);
+}
 
-	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;
-	}
+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;
+}
 
-	return 0;
+static int emulator_pio_in(struct kvm_vcpu *vcpu, int size,
+			   unsigned short port, void *val, unsigned int count)
+{
+	if (!vcpu->arch.pio.count && !__emulator_pio_in(vcpu, size, port, count))
+		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 +6970,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

[PATCH 6/8] KVM: x86: remove unnecessary arguments from complete_emulator_pio_in

[Paolo Bonzini]

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")
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/x86.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 07d9533b471d..ef4d6a0de4d8 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)
 {
+	int size = vcpu->arch.pio.size;
 	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);
+	trace_kvm_pio(KVM_PIO_IN, vcpu->arch.pio.port, size,
+		      vcpu->arch.pio.count, vcpu->arch.pio_data);
 	vcpu->arch.pio.count = 0;
 }
 
@@ -6950,7 +6951,7 @@ static int emulator_pio_in(struct kvm_vcpu *vcpu, int size,
 		return 0;
 
 	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

[PATCH 7/8] KVM: SEV-ES: keep INS functions together

[Paolo Bonzini]

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")
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 ef4d6a0de4d8..a485e185ad00 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -12377,15 +12377,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)
 {
@@ -12401,6 +12392,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

[PATCH 8/8] KVM: SEV-ES: go over the sev_pio_data buffer in multiple passes if needed

[Paolo Bonzini]

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>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/x86.c              | 73 +++++++++++++++++++++++++--------
 2 files changed, 57 insertions(+), 17 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 a485e185ad00..09c1e64495d3 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -12378,38 +12378,76 @@ 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);
+	vcpu->arch.pio.count = 0;
+	if (vcpu->arch.sev_pio_count)
+		return kvm_sev_es_outs(vcpu,
+				       vcpu->arch.pio.size,
+				       vcpu->arch.pio.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;
+		vcpu->arch.pio.count = 0;
+		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 complete_sev_es_emulated_ins(struct kvm_vcpu *vcpu)
+static int kvm_sev_es_ins(struct kvm_vcpu *vcpu, unsigned int size,
+			  unsigned int port);
+
+static void __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;
+	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)
+{
+	__complete_sev_es_emulated_ins(vcpu);
+	if (vcpu->arch.sev_pio_count)
+		return kvm_sev_es_ins(vcpu,
+				      vcpu->arch.pio.size,
+				      vcpu->arch.pio.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;
+		__complete_sev_es_emulated_ins(vcpu);
+		if (!vcpu->arch.sev_pio_count)
+			return 1;
 	}
 
 	vcpu->arch.complete_userspace_io = complete_sev_es_emulated_ins;
@@ -12421,8 +12459,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

Re: [PATCH 1/8] KVM: SEV-ES: fix length of string I/O

[Tom Lendacky]

On 10/13/21 11:56 AM, Paolo Bonzini wrote:
> The size of the data in the scratch buffer is not divided by the size of
> each port I/O operation, so vcpu->arch.pio.count ends up being larger
> than it should be by a factor of size.
> 
> 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>

Ugh, can't believe I did that...

Acked-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
>   arch/x86/kvm/svm/sev.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index c36b5fe4c27c..e672493b5d8d 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2583,7 +2583,7 @@ int sev_es_string_io(struct vcpu_svm *svm, int size, unsigned int port, int in)
>   		return -EINVAL;
>   
>   	return kvm_sev_es_string_io(&svm->vcpu, size, port,
> -				    svm->ghcb_sa, svm->ghcb_sa_len, in);
> +				    svm->ghcb_sa, svm->ghcb_sa_len / size, in);
>   }
>   
>   void sev_es_init_vmcb(struct vcpu_svm *svm)
> 

Re: [PATCH 1/8] KVM: SEV-ES: fix length of string I/O

[Maxim Levitsky]

On Wed, 2021-10-13 at 12:56 -0400, Paolo Bonzini wrote:
> The size of the data in the scratch buffer is not divided by the size of
> each port I/O operation, so vcpu->arch.pio.count ends up being larger
> than it should be by a factor of size.
> 
> 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/svm/sev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index c36b5fe4c27c..e672493b5d8d 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2583,7 +2583,7 @@ int sev_es_string_io(struct vcpu_svm *svm, int size, unsigned int port, int in)
>  		return -EINVAL;
>  
>  	return kvm_sev_es_string_io(&svm->vcpu, size, port,
> -				    svm->ghcb_sa, svm->ghcb_sa_len, in);
> +				    svm->ghcb_sa, svm->ghcb_sa_len / size, in);
>  }
>  
>  void sev_es_init_vmcb(struct vcpu_svm *svm)

This ends in kvm_sev_es_ins/outs and both indeed expect count of operations which they pass to emulator_pio_{out|in}_emulated

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

Best regards,
	Maxim Levitsky

Re: [PATCH 2/8] KVM: SEV-ES: rename guest_ins_data to sev_pio_data

[Maxim Levitsky]

On Wed, 2021-10-13 at 12:56 -0400, Paolo Bonzini wrote:
> Since we will be using this for OUTS emulation as well, 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")
> 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 aabd3a2ec1bc..722f5fcf76e1 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -12369,7 +12369,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;
>  
> @@ -12401,7 +12401,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;
>  	}
>  

It might be worth to mention here why we will soon need this field for the outs emulation:

INS reads the data from the userspace (or in-kernel) PIO emulation which is provided in vcpu->arch.pio_data
which is then copied to GHCB, but for OUTS, the data is pushed from GHCB to userspace/in-kernel PIO emulation,
so there is no need to do anything SEV specific

But if the data is pushed via outs spans more that one page, next few patches will split it, so there will be a need
to save the data pointer.


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

Best regards,
	Maxim Levitsky

Re: [PATCH 3/8] KVM: x86: leave vcpu->arch.pio.count alone in emulator_pio_in_out

[Maxim Levitsky]

On Wed, 2021-10-13 at 12:56 -0400, Paolo Bonzini wrote:
> 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")
> 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 722f5fcf76e1..218877e297e5 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,

Makes sense, now that both emulator_pio_in and emulator_pio_out clear the arch.pio.count once.

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

Best regards,
	Maxim Levitsky

Re: [PATCH 4/8] KVM: SEV-ES: clean up kvm_sev_es_ins/outs

[Maxim Levitsky]

On Wed, 2021-10-13 at 12:56 -0400, Paolo Bonzini wrote:
> 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.

You forgot to mention that you inlined the call to emulator_pio_out_emulated/emulator_pio_in_emulated.


> 
> 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 | 31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 218877e297e5..8880dc36a2b4 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -12382,34 +12382,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.  */
^^ This is a very good comment to have here!
>  		return ret;
> +	}
>  
>  	vcpu->arch.pio.count = 0;
^^^
I wonder what the rules are for clearing vcpu->arch.pio.count for userspace PIO vm exits.
Looks like complete_fast_pio_out clears it, but otherwise the only other place
that clears it in this case is x86_emulate_instruction when it restarts the instuction.
Do I miss something?


> -
>  	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;
>  }
>  
> @@ -12417,8 +12415,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);
>  

Looks OK to me, I might have missed something though.

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


Best regards,
	Maxim Levitsky

Re: [PATCH 5/8] KVM: x86: split the two parts of emulator_pio_in

[Maxim Levitsky]

On Wed, 2021-10-13 at 12:56 -0400, Paolo Bonzini wrote:
> 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.
> 
> While at it, remove the void* argument also from emulator_pio_in_out.
s/remove the void* argument/remove the unused 'void* val' argument/ maybe?
> 
> 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 | 42 +++++++++++++++++++++++-------------------
>  1 file changed, 23 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 8880dc36a2b4..07d9533b471d 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,31 @@ 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, void *val, unsigned int count)
> +static int __emulator_pio_in(struct kvm_vcpu *vcpu, int size,
> +			     unsigned short port, unsigned int count)
>  {
> -	int ret;
> -
> -	if (vcpu->arch.pio.count)
> -		goto data_avail;
> -
> +	WARN_ON(vcpu->arch.pio.count);
>  	memset(vcpu->arch.pio_data, 0, size * count);
> +	return emulator_pio_in_out(vcpu, size, port, count, true);
> +}
>  
> -	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;
> -	}
> +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;
> +}
>  
> -	return 0;
> +static int emulator_pio_in(struct kvm_vcpu *vcpu, int size,
> +			   unsigned short port, void *val, unsigned int count)
> +{
> +	if (!vcpu->arch.pio.count && !__emulator_pio_in(vcpu, size, port, count))
> +		return 0;
^^ maybe I would add a comment here about the fact that kernel completed the
emulation when returing here.

> +
> +	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 +6970,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,



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

Best regards,
	Maxim Levitsky

Re: [PATCH 6/8] KVM: x86: remove unnecessary arguments from complete_emulator_pio_in

[Maxim Levitsky]

On Wed, 2021-10-13 at 12:56 -0400, Paolo Bonzini wrote:
> 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")
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/x86.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 07d9533b471d..ef4d6a0de4d8 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)
>  {
> +	int size = vcpu->arch.pio.size;
>  	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);
> +	trace_kvm_pio(KVM_PIO_IN, vcpu->arch.pio.port, size,
> +		      vcpu->arch.pio.count, vcpu->arch.pio_data);
>  	vcpu->arch.pio.count = 0;
>  }
>  
> @@ -6950,7 +6951,7 @@ static int emulator_pio_in(struct kvm_vcpu *vcpu, int size,
>  		return 0;
>  
>  	WARN_ON(count != vcpu->arch.pio.count);
> -	complete_emulator_pio_in(vcpu, size, port, val);
> +	complete_emulator_pio_in(vcpu, val);
>  	return 1;
>  }
>  

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

Best regards,
	Maxim Levitsky

Re: [PATCH 7/8] KVM: SEV-ES: keep INS functions together

[Maxim Levitsky]

On Wed, 2021-10-13 at 12:56 -0400, Paolo Bonzini wrote:
> 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")
> 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 ef4d6a0de4d8..a485e185ad00 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -12377,15 +12377,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)
>  {
> @@ -12401,6 +12392,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)
>  {
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
	Maxim Levitsky

Re: [PATCH 8/8] KVM: SEV-ES: go over the sev_pio_data buffer in multiple passes if needed

[Maxim Levitsky]

On Wed, 2021-10-13 at 12:56 -0400, Paolo Bonzini wrote:
> 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>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  1 +
>  arch/x86/kvm/x86.c              | 73 +++++++++++++++++++++++++--------
>  2 files changed, 57 insertions(+), 17 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 a485e185ad00..09c1e64495d3 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -12378,38 +12378,76 @@ 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);
> +	vcpu->arch.pio.count = 0;
> +	if (vcpu->arch.sev_pio_count)
> +		return kvm_sev_es_outs(vcpu,
> +				       vcpu->arch.pio.size,
> +				       vcpu->arch.pio.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;
> +		vcpu->arch.pio.count = 0;
> +		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 complete_sev_es_emulated_ins(struct kvm_vcpu *vcpu)
> +static int kvm_sev_es_ins(struct kvm_vcpu *vcpu, unsigned int size,
> +			  unsigned int port);
> +
> +static void __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;
> +	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)
> +{
> +	__complete_sev_es_emulated_ins(vcpu);
> +	if (vcpu->arch.sev_pio_count)
> +		return kvm_sev_es_ins(vcpu,
> +				      vcpu->arch.pio.size,
> +				      vcpu->arch.pio.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;
> +		__complete_sev_es_emulated_ins(vcpu);
> +		if (!vcpu->arch.sev_pio_count)
> +			return 1;
>  	}
>  
>  	vcpu->arch.complete_userspace_io = complete_sev_es_emulated_ins;
> @@ -12421,8 +12459,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);
>  

I might have missed something, but it looks OK to me.
i mostly checked how the code looks after applying this patch.

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

Note that I haven't run tested the patches as I don't currently test SEV-ES.

Best regards,
	Maxim Levitsky

Re: [PATCH 4/8] KVM: SEV-ES: clean up kvm_sev_es_ins/outs

[Paolo Bonzini]

On 22/10/21 01:14, Maxim Levitsky wrote:
>>   
>>   	vcpu->arch.pio.count = 0;
> ^^^
> I wonder what the rules are for clearing vcpu->arch.pio.count for userspace PIO vm exits.
> Looks like complete_fast_pio_out clears it, but otherwise the only other place
> that clears it in this case is x86_emulate_instruction when it restarts the instuction.
> Do I miss something?

For IN, it is cleared by the completion callback.

For OUT, it can be cleared either by the completion callback or before 
calling it, because the completion callback will not need it.  I would 
like to standardize towards clearing it in the callback for out, too, 
even if sometimes it's unnecessary to have a callback in the first 
place; this is what patch 8 does for example.  This way 
vcpu->arch.pio.count > 0 tells you whether the other fields have a 
recent value.

Paolo

Re: [PATCH 1/8] KVM: SEV-ES: fix length of string I/O

[Marc Orr]

On Wed, Oct 13, 2021 at 9:56 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> The size of the data in the scratch buffer is not divided by the size of
> each port I/O operation, so vcpu->arch.pio.count ends up being larger
> than it should be by a factor of size.
>
> 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/svm/sev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index c36b5fe4c27c..e672493b5d8d 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2583,7 +2583,7 @@ int sev_es_string_io(struct vcpu_svm *svm, int size, unsigned int port, int in)
>                 return -EINVAL;
>
>         return kvm_sev_es_string_io(&svm->vcpu, size, port,
> -                                   svm->ghcb_sa, svm->ghcb_sa_len, in);
> +                                   svm->ghcb_sa, svm->ghcb_sa_len / size, in);
>  }
>
>  void sev_es_init_vmcb(struct vcpu_svm *svm)
> --
> 2.27.0
>
>

I could be missing something, but I'm pretty sure that this is wrong.
The GHCB spec says that `exit_info_2` is the `rep` count. Not the
string length.

For example, given a `rep outsw` instruction, with `ECX` set to `8`,
the rep count written into `SW_EXITINFO2` should be eight x86 words
(i.e., 16 bytes) and the IO size should be one x86 word (i.e., 2
bytes). In other words, the code was correct before this patch. This
patch is incorrectly dividing the rep count by the IO size, causing
the string IO to be truncated.

Re: [PATCH 1/8] KVM: SEV-ES: fix length of string I/O

[Paolo Bonzini]

On 25/10/21 03:31, Marc Orr wrote:
> I could be missing something, but I'm pretty sure that this is wrong.
> The GHCB spec says that `exit_info_2` is the `rep` count. Not the
> string length.
> 
> For example, given a `rep outsw` instruction, with `ECX` set to `8`,
> the rep count written into `SW_EXITINFO2` should be eight x86 words
> (i.e., 16 bytes) and the IO size should be one x86 word (i.e., 2
> bytes). In other words, the code was correct before this patch. This
> patch is incorrectly dividing the rep count by the IO size, causing
> the string IO to be truncated.

Then what's wrong is _also_ the call to setup_vmgexit_scratch, because 
that one definitely expects bytes:

                 scratch_va = kzalloc(len, GFP_KERNEL_ACCOUNT);

Paolo