[PATCH 0/6] KVM: x86: vcpu->arch.pio* cleanups

[PATCH 0/6] KVM: x86: vcpu->arch.pio* cleanups

[Paolo Bonzini]

These patches complete the job started by commit 3b27de271839 ("KVM:
x86: split the two parts of emulator_pio_in", 2021-10-22).  The commit
message eloquently said that emulator_pio_in_out "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".  Some time
has passed and it's high time to do it.

After this series, in-kernel PIO does not use vcpu->arch.pio* anymore;
it is only used by complete_emulator_pio_in.

Paolo


Paolo Bonzini (6):
  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/kvm/trace.h |   2 +-
 arch/x86/kvm/x86.c   | 119 ++++++++++++++++++-------------------------
 2 files changed, 50 insertions(+), 71 deletions(-)

-- 
2.31.1

[PATCH 1/6] KVM: x86: inline kernel_pio into its sole caller

[Paolo Bonzini]

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 79efdc19b4c8..2f9100f2564e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7415,37 +7415,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.31.1

Re: [PATCH 1/6] KVM: x86: inline kernel_pio into its sole caller

[Sean Christopherson]

On Wed, Jun 08, 2022, 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.

Please avoid pronouns and state what patch does, not what "can" be done.  It's not
clear without reading the actual code whether "The code is not ready for that" means
"KVM is not ready to WARN" or "KVM is not ready to fall back to exiting userspace
if a

E.g.

  WARN if emulated PIO falls back to userspace after successfully handling
  one or more in-kernel iterations.  The port, size, and access type do not
  change, and KVM so it should be impossible for in-kernel PIO to fail on
  subsequent iterations.

That said, I don't think the above statement is true.  KVM is running with SRCU
protection, but the synchronize_srcu_expedited() in kvm_io_bus_unregister_dev()
only protects against use-after-free, it does not prevent two calls to
kvm_io_bus_read() from seeing different incarnations of kvm->buses.

And if I'm right, that could be exploited to create a buffer overrun due to doing
this memcpy with "data = <original data> + i * size".

	else
		memcpy(vcpu->arch.pio_data, data, size * count);

The existing code is arguably wrong too in that it will result in replaying PIO
accesses, but IMO userspace gets to keep the pieces if it unregisters a device
while vCPUs are running.
 
> 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 79efdc19b4c8..2f9100f2564e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -7415,37 +7415,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.31.1
> 
> 

[PATCH 2/6] KVM: x86: move all vcpu->arch.pio* setup in emulator_pio_in_out

[Paolo Bonzini]

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 fd28dd40b813..2877c0e92823 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -154,7 +154,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 2f9100f2564e..8e1e76d0378b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7416,17 +7416,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)
@@ -7454,9 +7459,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)
@@ -7505,9 +7508,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.31.1

Re: [PATCH 2/6] KVM: x86: move all vcpu->arch.pio* setup in emulator_pio_in_out

[Sean Christopherson]

On Wed, Jun 08, 2022, 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

Please add parantheses when referencing functions in shortlogs and changelogs,
I find it tremendously helpful.

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

Same nits about about pronouns.  In a similar vein, be explicit about what "more"
mean; I had no idea what "more" meant until the second sentence.  E.g.

  The vcpu->arch.pio* fields still need to be filled even when the PIO is
  handled in-kernel as __emulator_pio_in() is always followed by
  complete_emulator_pio_in().  But after fixing that, it will be possible to
  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 fd28dd40b813..2877c0e92823 100644
> --- a/arch/x86/kvm/trace.h
> +++ b/arch/x86/kvm/trace.h
> @@ -154,7 +154,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 2f9100f2564e..8e1e76d0378b 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -7416,17 +7416,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;

Oof, passing NULL for @data and then overwriting it below is gross.  It also makes
@in redundant for this one patch.  Might be worth adding a comment, even though
it's transient?

>  
>  	for (i = 0; i < count; i++) {
>  		if (in)
> @@ -7454,9 +7459,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)
> @@ -7505,9 +7508,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.31.1
> 
> 

[PATCH 3/6] KVM: x86: wean in-kernel PIO from vcpu->arch.pio*

[Paolo Bonzini]

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 | 61 ++++++++++++++++++++--------------------------
 1 file changed, 27 insertions(+), 34 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 8e1e76d0378b..3b641cd2ff6f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7423,16 +7423,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);
@@ -7446,6 +7436,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;
@@ -7457,9 +7457,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)
@@ -7482,16 +7486,11 @@ static int emulator_pio_in(struct kvm_vcpu *vcpu, int size,
 		 * shenanigans as KVM doesn't support modifying the rep count,
 		 * and the emulator ensures @count doesn't overflow the buffer.
 		 */
+		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);
 	}
-
-	complete_emulator_pio_in(vcpu, val);
-	return 1;
 }
 
 static int emulator_pio_in_emulated(struct x86_emulate_ctxt *ctxt,
@@ -7506,14 +7505,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,
@@ -13064,20 +13057,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;
@@ -13089,11 +13082,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.31.1

Re: [PATCH 3/6] KVM: x86: wean in-kernel PIO from vcpu->arch.pio*

[Sean Christopherson]

On Wed, Jun 08, 2022, Paolo Bonzini wrote:
>  static void complete_emulator_pio_in(struct kvm_vcpu *vcpu, void *val)
> @@ -7482,16 +7486,11 @@ static int emulator_pio_in(struct kvm_vcpu *vcpu, int size,
>  		 * shenanigans as KVM doesn't support modifying the rep count,
>  		 * and the emulator ensures @count doesn't overflow the buffer.
>  		 */
> +		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);

Any objections to not using an "else"?  I.e.

	if (vcpu->arch.pio.count) {
		/*
		 * Complete a previous iteration that required userspace I/O.
		 * Note, @count isn't guaranteed to match pio.count as userspace
		 * can modify ECX before rerunning the vCPU.  Ignore any such
		 * shenanigans as KVM doesn't support modifying the rep count,
		 * and the emulator ensures @count doesn't overflow the buffer.
		 */
		complete_emulator_pio_in(vcpu, val);
		return 1;
	}
	return __emulator_pio_in(vcpu, size, port, val, count);

>  	}
> -
> -	complete_emulator_pio_in(vcpu, val);
> -	return 1;
>  }
>  
>  static int emulator_pio_in_emulated(struct x86_emulate_ctxt *ctxt,
> @@ -7506,14 +7505,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,
> @@ -13064,20 +13057,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;

Opportunistically use an "unsigned int" if you spin another version?

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

Eww.  The dependency between vcpu->arch.pio.count and complete_emulator_pio_in()
is nasty.  Can you add a comment above count to reduce the likelihood of someone
using vcpu->arch.pio.count directly here instead of making a snapshot?

	/*
	 * Snapshot the count before completing userspace I/O, which will
	 * consume the userspace data and thus clear vcpu->arch.pio.count.
	 */
	unsigned int count = vcpu->arch.pio.count;

>  	if (vcpu->arch.sev_pio_count)
>  		return kvm_sev_es_ins(vcpu, size, port);
>  	return 1;
> @@ -13089,11 +13082,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.31.1
> 
> 

[PATCH 4/6] KVM: x86: wean fast IN from emulator_pio_in

[Paolo Bonzini]

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 3b641cd2ff6f..aefcc71a7040 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8692,11 +8692,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);
@@ -8711,7 +8707,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.31.1

Re: [PATCH 4/6] KVM: x86: wean fast IN from emulator_pio_in

[Sean Christopherson]

On Wed, Jun 08, 2022, Paolo Bonzini wrote:
> Now that __emulator_pio_in already fills "val" for in-kernel PIO, it

For some reason the "already" confused the heck out of me.  I thought it was
referring to a previous patch, which it kind of is, but then I couldn't figure
out the relevance to this patch.

Ah, I know why I got confused, the in-kernel PIO case has nothing to do with the
usage in complete_fast_pio_in(), e.g. complete_fast_pio_in() could be modified to
call complete_emulator_pio_in() directly even without the previous cleanup in
this series.

Can you split this patch in two?  It's comically trivial, but it makes the
changelogs much easier to understand.

  Use __emulator_pio_in() directly for fast PIO instead of bouncing through
  emulator_pio_in() now that __emulator_pio_in() fills "val" when handling
  in-kernel PIO.  vcpu->arch.pio.count is guaranteed to be '0', so this a
  pure nop.

  No functional change intended.

and

  Use complete_emulator_pio_in() directly when completing fast PIO, there's
  no need to bounce through emulator_pio_in() as the comment about ECX
  changing doesn't apply to fast PIO, which isn't used for string I/O.

  No functional change intended.

> 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 3b641cd2ff6f..aefcc71a7040 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8692,11 +8692,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);
> @@ -8711,7 +8707,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.31.1
> 
> 

[PATCH 5/6] KVM: x86: de-underscorify __emulator_pio_in

[Paolo Bonzini]

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 aefcc71a7040..fd4382602f65 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7456,7 +7456,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);
@@ -7475,9 +7475,11 @@ 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 a previous iteration that required userspace I/O.
@@ -7489,18 +7491,10 @@ static int emulator_pio_in(struct kvm_vcpu *vcpu, int size,
 		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)
@@ -8707,7 +8701,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;
@@ -13078,7 +13072,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.31.1

Re: [PATCH 5/6] KVM: x86: de-underscorify __emulator_pio_in

[Sean Christopherson]

On Wed, Jun 08, 2022, 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.

Wrap changelogs closer to 75 chars, <60 is a bit too aggressive.

> 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 aefcc71a7040..fd4382602f65 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -7456,7 +7456,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)

Align the second line of parameters.  Even gets it below 80 columns ;-)

>  {
>  	int r = emulator_pio_in_out(vcpu, size, port, val, count, true);
> @@ -7475,9 +7475,11 @@ 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,

"int size" fits on the first line, emulator_pio_in_emulated() and
emulator_pio_out_emulated() have different formatting either way.

> +				    unsigned int count)
>  {
> +	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);

Newline after variable declarations.

>  	if (vcpu->arch.pio.count) {
>  		/*
>  		 * Complete a previous iteration that required userspace I/O.
> @@ -7489,18 +7491,10 @@ static int emulator_pio_in(struct kvm_vcpu *vcpu, int size,
>  		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)
> @@ -8707,7 +8701,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;
> @@ -13078,7 +13072,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.31.1
> 
> 

[PATCH 6/6] KVM: SEV-ES: reuse advance_sev_es_emulated_ins for OUT too

[Paolo Bonzini]

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 fd4382602f65..a3651aa74ed7 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -13007,6 +13007,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);
 
@@ -13030,8 +13036,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;
 
@@ -13047,12 +13052,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;
@@ -13060,7 +13059,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;
@@ -13076,7 +13075,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.31.1

Re: [PATCH 6/6] KVM: SEV-ES: reuse advance_sev_es_emulated_ins for OUT too

[Sean Christopherson]

On Wed, Jun 08, 2022, 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 fd4382602f65..a3651aa74ed7 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -13007,6 +13007,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);
>  
> @@ -13030,8 +13036,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);

I think this is a bug fix that should go in a separate patch.  size == vcpu->arch.pio.size
when kvm_sev_es_outs() is called from complete_sev_es_emulated_outs(), but when
it's called from kvm_sev_es_string_io() it will hold the size of the previous PIO.

>  		if (!ret)
>  			break;
>  
> @@ -13047,12 +13052,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;
> @@ -13060,7 +13059,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;
> @@ -13076,7 +13075,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.31.1
> 

Re: [PATCH 6/6] KVM: SEV-ES: reuse advance_sev_es_emulated_ins for OUT too

[Paolo Bonzini]

On 6/10/22 00:50, Sean Christopherson wrote:
> On Wed, Jun 08, 2022, 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 fd4382602f65..a3651aa74ed7 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -13007,6 +13007,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);
>>   
>> @@ -13030,8 +13036,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);
> 
> I think this is a bug fix that should go in a separate patch.  size == vcpu->arch.pio.size
> when kvm_sev_es_outs() is called from complete_sev_es_emulated_outs(), but when
> it's called from kvm_sev_es_string_io() it will hold the size of the previous PIO.

It's not a bugfix for current master, because emulator_pio_out() sets 
vcpu->arch.pio.size = size.

However, it has to be moved before "KVM: x86: wean in-kernel PIO from 
vcpu->arch.pio*" or squashed therein.

Paolo

>>   		if (!ret)
>>   			break;
>>   
>> @@ -13047,12 +13052,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;
>> @@ -13060,7 +13059,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;
>> @@ -13076,7 +13075,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.31.1
>>
>