[PATCH 0/8] KVM: SEV-ES: fixes for string I/O emulation

[PATCH 0/8] KVM: SEV-ES: fixes for string I/O emulation

[Paolo Bonzini]

This series, namely patches 1 and 8, fix two bugs in string I/O
emulation for SEV-ES:

- first, the length is completely off for "rep ins" and "rep outs"
  operation of size > 1.  After setup_vmgexit_scratch, svm->ghcb_sa_len
  is in bytes, but kvm_sev_es_string_io expects the number of PIO
  operations.

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

The second bug was reported by Felix Wilhelm.  The first was found by
me by code inspection; on one hand it seems *too* egregious so I'll be
gladly proven wrong on this, on the other hand... I know I'm bad at code
review, but not _that_ bad.

Patches 2 to 7 are a bunch of cleanups to emulator_pio_in and
emulator_pio_in_out, so that the final SEV code is a little easier
to reason on.  Just a little, no big promises.

Tested by booting a SEV-ES guest and with "regular" kvm-unit-tests.
For SEV-ES I also bounded the limit to 12 bytes, and checked in the
resulting trace that both the read and write paths were hit when
booting a guest.

Paolo


Paolo Bonzini (8):
  KVM: SEV-ES: fix length of string I/O
  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

 arch/x86/include/asm/kvm_host.h |   3 +-
 arch/x86/kvm/svm/sev.c          |   2 +-
 arch/x86/kvm/x86.c              | 136 +++++++++++++++++++++-----------
 3 files changed, 95 insertions(+), 46 deletions(-)

-- 
2.27.0

[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

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

[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

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

[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

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

[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

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

[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

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

[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

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

[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

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

[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 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 0/8] KVM: SEV-ES: fixes for string I/O emulation

[Paolo Bonzini]

On 13/10/21 18:56, Paolo Bonzini wrote:
> This series, namely patches 1 and 8, fix two bugs in string I/O
> emulation for SEV-ES:
> 
> - first, the length is completely off for "rep ins" and "rep outs"
>    operation of size > 1.  After setup_vmgexit_scratch, svm->ghcb_sa_len
>    is in bytes, but kvm_sev_es_string_io expects the number of PIO
>    operations.
> 
> - second, 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.
> 
> The second bug was reported by Felix Wilhelm.  The first was found by
> me by code inspection; on one hand it seems *too* egregious so I'll be
> gladly proven wrong on this, on the other hand... I know I'm bad at code
> review, but not _that_ bad.

Ping.

Paolo

Re: [PATCH 0/8] KVM: SEV-ES: fixes for string I/O emulation

[Sean Christopherson]

On Thu, Oct 21, 2021, Paolo Bonzini wrote:
> On 13/10/21 18:56, Paolo Bonzini wrote:
> > This series, namely patches 1 and 8, fix two bugs in string I/O
> > emulation for SEV-ES:
> > 
> > - first, the length is completely off for "rep ins" and "rep outs"
> >    operation of size > 1.  After setup_vmgexit_scratch, svm->ghcb_sa_len
> >    is in bytes, but kvm_sev_es_string_io expects the number of PIO
> >    operations.
> > 
> > - second, 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.
> > 
> > The second bug was reported by Felix Wilhelm.  The first was found by
> > me by code inspection; on one hand it seems *too* egregious so I'll be
> > gladly proven wrong on this, on the other hand... I know I'm bad at code
> > review, but not _that_ bad.

String I/O was completely busted on the Linux guest side as well, I wouldn't be
the least bit surprised if KVM were completely broken as well (reviewing now...).

Re: [PATCH 0/8] KVM: SEV-ES: fixes for string I/O emulation

[Sean Christopherson]

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

On Wed, Oct 13, 2021, Paolo Bonzini wrote:
> Patches 2 to 7 are a bunch of cleanups to emulator_pio_in and
> emulator_pio_in_out, so that the final SEV code is a little easier
> to reason on.  Just a little, no big promises.

IMO, this series goes in the wrong direction and doesn't make the mess any better,
just different.

The underlying issue is that kernel_pio() does the completely horrendous thing
of consuming vcpu->arch.pio.  That leads to the juggling that this series tries
to clean up, but it's essentially an impossible problem to solve because the
approach itself is broken.

The _only_ reason vcpu->arch.pio (the structure) exists is to snapshot a port I/O
operation that didn't originate from the emulator before exiting to userspace,
i.e. "fast" I/O and now SEV-ES.  Ignoring those two, all info comes from the
emulator and a single flag or even the cui pointer would suffice.

Ditto for pio_data, it's purely needed to let userspace read/write values, its
use directly in any code except those specific paths is just bad code.

So instead of juggling vcpu->arch.pio.count in weird places, just don't set the
damn thing in the first place.

Untested patches attached that frame in where I think we should go with this.

I'll be offline until Monday, apologies for the inconvenience.

[-- Attachment #2: 0001-KVM-x86-Don-t-exit-to-userspace-when-SEV-ES-INS-is-s.patch --]
[-- Type: text/x-diff, Size: 818 bytes --]

From 17384716129668b6636237b410a3885aaf32efb3 Mon Sep 17 00:00:00 2001
From: Sean Christopherson <seanjc@google.com>
Date: Thu, 21 Oct 2021 16:22:27 -0700
Subject: [PATCH 1/6] KVM: x86: Don't exit to userspace when SEV-ES INS is
 successful

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c59b63c56af9..c245edfd974c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -12509,7 +12509,7 @@ static int kvm_sev_es_ins(struct kvm_vcpu *vcpu, unsigned int size,
 		vcpu->arch.complete_userspace_io = complete_sev_es_emulated_ins;
 	}
 
-	return 0;
+	return ret;
 }
 
 int kvm_sev_es_string_io(struct kvm_vcpu *vcpu, unsigned int size,
-- 
2.33.0.1079.g6e70778dc9-goog


[-- Attachment #3: 0002-KVM-x86-WARN-if-emulated-kernel-port-I-O-fails-after.patch --]
[-- Type: text/x-diff, Size: 1356 bytes --]

From cdb6bceeceda3eb3bd3755b99f00d526e2b9045e Mon Sep 17 00:00:00 2001
From: Sean Christopherson <seanjc@google.com>
Date: Thu, 21 Oct 2021 15:40:36 -0700
Subject: [PATCH 2/6] KVM: x86: WARN if emulated kernel port I/O fails after a
 successful iteration

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c245edfd974c..13a21a05a75d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7046,7 +7046,7 @@ static int emulator_cmpxchg_emulated(struct x86_emulate_ctxt *ctxt,
 
 static int kernel_pio(struct kvm_vcpu *vcpu, void *pd)
 {
-	int r = 0, i;
+	int r, i;
 
 	for (i = 0; i < vcpu->arch.pio.count; i++) {
 		if (vcpu->arch.pio.in)
@@ -7056,11 +7056,17 @@ static int kernel_pio(struct kvm_vcpu *vcpu, void *pd)
 			r = kvm_io_bus_write(vcpu, KVM_PIO_BUS,
 					     vcpu->arch.pio.port, vcpu->arch.pio.size,
 					     pd);
-		if (r)
-			break;
+		if (r) {
+			/*
+			 * The port doesn't change on subsequent iterations and
+			 * the kernel I/O device should not disappear.
+			 */
+			WARN_ON_ONCE(i);
+			return r;
+		}
 		pd += vcpu->arch.pio.size;
 	}
-	return r;
+	return 0;
 }
 
 static int emulator_pio_in_out(struct kvm_vcpu *vcpu, int size,
-- 
2.33.0.1079.g6e70778dc9-goog


[-- Attachment #4: 0003-KVM-x86-Use-an-unsigned-int-when-emulating-string-po.patch --]
[-- Type: text/x-diff, Size: 819 bytes --]

From b538f779f15ba63e5e32fd3cce6fae6e530cde40 Mon Sep 17 00:00:00 2001
From: Sean Christopherson <seanjc@google.com>
Date: Thu, 21 Oct 2021 16:45:21 -0700
Subject: [PATCH 3/6] KVM: x86: Use an 'unsigned int' when emulating string
 port I/O

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 13a21a05a75d..a126b1129348 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7046,7 +7046,8 @@ static int emulator_cmpxchg_emulated(struct x86_emulate_ctxt *ctxt,
 
 static int kernel_pio(struct kvm_vcpu *vcpu, void *pd)
 {
-	int r, i;
+	unsigned int i;
+	int r;
 
 	for (i = 0; i < vcpu->arch.pio.count; i++) {
 		if (vcpu->arch.pio.in)
-- 
2.33.0.1079.g6e70778dc9-goog


[-- Attachment #5: 0004-KVM-x86-Fill-kvm_pio_request-if-and-only-if-KVM-is-e.patch --]
[-- Type: text/x-diff, Size: 5732 bytes --]

From 21f4d5d9048e84d01137ba2a9fbb3d691141dc16 Mon Sep 17 00:00:00 2001
From: Sean Christopherson <seanjc@google.com>
Date: Thu, 21 Oct 2021 15:41:18 -0700
Subject: [PATCH 4/6] KVM: x86: Fill kvm_pio_request if and only if KVM is
 exiting to userspace

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 89 +++++++++++++++++++++++-----------------------
 1 file changed, 45 insertions(+), 44 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a126b1129348..a20a790ce586 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7044,19 +7044,17 @@ 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)
+static int kernel_pio(struct kvm_vcpu *vcpu, int size, unsigned short port,
+		      void *data, unsigned int count, bool in)
 {
 	unsigned int i;
 	int r;
 
-	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);
+	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,
-					     vcpu->arch.pio.port, vcpu->arch.pio.size,
-					     pd);
+			r = kvm_io_bus_write(vcpu, KVM_PIO_BUS, port, size, data);
 		if (r) {
 			/*
 			 * The port doesn't change on subsequent iterations and
@@ -7065,24 +7063,33 @@ static int kernel_pio(struct kvm_vcpu *vcpu, void *pd)
 			WARN_ON_ONCE(i);
 			return r;
 		}
-		pd += vcpu->arch.pio.size;
+		data += size;
 	}
 	return 0;
 }
 
 static int emulator_pio_in_out(struct kvm_vcpu *vcpu, int size,
-			       unsigned short port, void *val,
+			       unsigned short port, void *data,
 			       unsigned int count, bool in)
 {
+	if (!kernel_pio(vcpu, port, size, data, count, in))
+		return 1;
+
+	/*
+	 * I/O was not handled in kernel, forward the operation to userespace.
+	 * Snapshot the port, size, etc... in kernel memory as some callers,
+	 * e.g. "fast" port I/O and SEV-ES, don't flow through the emulator and
+	 * will have lost the original information when KVM regains control.
+	 * The info stored in the run page can't be trusted as userspace has
+	 * write access to the run page.
+	 */
 	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)) {
-		vcpu->arch.pio.count = 0;
-		return 1;
-	}
+	if (!in)
+		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;
@@ -7090,30 +7097,27 @@ static int emulator_pio_in_out(struct kvm_vcpu *vcpu, int size,
 	vcpu->run->io.data_offset = KVM_PIO_PAGE_OFFSET * PAGE_SIZE;
 	vcpu->run->io.count = count;
 	vcpu->run->io.port = port;
-
 	return 0;
 }
 
 static int emulator_pio_in(struct kvm_vcpu *vcpu, int size,
-			   unsigned short port, void *val, unsigned int count)
+			   unsigned short port, void *data, unsigned int count)
 {
-	int ret;
-
-	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);
+	if (vcpu->arch.pio.count) {
+		/*
+		 * Complete port I/O when re-emulating the instruction after
+		 * userspace has provided the requested data.
+		 *
+		 * FIXME: this will copy garbage if count > vcpu->arch.pio.count.
+		 */
 		vcpu->arch.pio.count = 0;
-		return 1;
+		memcpy(data, vcpu->arch.pio_data, size * count);
+	} else if (!emulator_pio_in_out(vcpu, size, port, data, count, true)) {
+		return 0;
 	}
 
-	return 0;
+	trace_kvm_pio(KVM_PIO_IN, port, size, count, data);
+	return 1;
 }
 
 static int emulator_pio_in_emulated(struct x86_emulate_ctxt *ctxt,
@@ -7125,19 +7129,18 @@ static int emulator_pio_in_emulated(struct x86_emulate_ctxt *ctxt,
 }
 
 static int emulator_pio_out(struct kvm_vcpu *vcpu, int size,
-			    unsigned short port, const void *val,
+			    unsigned short port, void *val,
 			    unsigned int count)
 {
-	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);
+	trace_kvm_pio(KVM_PIO_OUT, port, size, count, val);
+	return emulator_pio_in_out(vcpu, size, port, val, count, false);
 }
 
 static int emulator_pio_out_emulated(struct x86_emulate_ctxt *ctxt,
 				     int size, unsigned short port,
-				     const void *val, unsigned int count)
+				     const void *data, unsigned int count)
 {
-	return emulator_pio_out(emul_to_vcpu(ctxt), size, port, val, count);
+	return emulator_pio_out(emul_to_vcpu(ctxt), size, port, (void *)data, count);
 }
 
 static unsigned long get_segment_base(struct kvm_vcpu *vcpu, int seg)
@@ -12509,14 +12512,12 @@ static int kvm_sev_es_ins(struct kvm_vcpu *vcpu, unsigned int size,
 
 	ret = emulator_pio_in_emulated(vcpu->arch.emulate_ctxt, size, port,
 				       data, count);
-	if (ret) {
-		vcpu->arch.pio.count = 0;
-	} else {
-		vcpu->arch.guest_ins_data = data;
-		vcpu->arch.complete_userspace_io = complete_sev_es_emulated_ins;
-	}
+	if (ret)
+		return ret;
 
-	return ret;
+	vcpu->arch.guest_ins_data = data;
+	vcpu->arch.complete_userspace_io = complete_sev_es_emulated_ins;
+	return 0;
 }
 
 int kvm_sev_es_string_io(struct kvm_vcpu *vcpu, unsigned int size,
-- 
2.33.0.1079.g6e70778dc9-goog


[-- Attachment #6: 0005-KVM-x86-Stop-being-clever-and-use-a-completion-handl.patch --]
[-- Type: text/x-diff, Size: 1197 bytes --]

From b134b231b49563ae2fca54dbd4f85356b10aaf53 Mon Sep 17 00:00:00 2001
From: Sean Christopherson <seanjc@google.com>
Date: Thu, 21 Oct 2021 16:29:18 -0700
Subject: [PATCH 5/6] KVM: x86: Stop being clever and use a "completion"
 handler for SEV-ES OUTS

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a20a790ce586..fad2c7192aa3 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -12481,6 +12481,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 int complete_sev_es_emulated_outs(struct kvm_vcpu *vcpu)
+{
+	vcpu->arch.pio.count = 0;
+	return 1;
+}
+
 static int complete_sev_es_emulated_ins(struct kvm_vcpu *vcpu)
 {
 	memcpy(vcpu->arch.guest_ins_data, vcpu->arch.pio_data,
@@ -12500,8 +12506,7 @@ static int kvm_sev_es_outs(struct kvm_vcpu *vcpu, unsigned int size,
 	if (ret)
 		return ret;
 
-	vcpu->arch.pio.count = 0;
-
+	vcpu->arch.complete_userspace_io = complete_sev_es_emulated_outs;
 	return 0;
 }
 
-- 
2.33.0.1079.g6e70778dc9-goog


[-- Attachment #7: 0006-KVM-x86-Move-pointer-for-SEV-ES-fast-string-I-O-into.patch --]
[-- Type: text/x-diff, Size: 1802 bytes --]

From b0ac37af659b6ce4cb556adc3bda3752db129724 Mon Sep 17 00:00:00 2001
From: Sean Christopherson <seanjc@google.com>
Date: Thu, 21 Oct 2021 16:40:41 -0700
Subject: [PATCH 6/6] KVM: x86: Move pointer for SEV-ES/fast string I/O into
 kvm_pio_request

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/kvm_host.h | 4 +++-
 arch/x86/kvm/x86.c              | 4 ++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 80f4b8a9233c..ae15a32cc9aa 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -385,6 +385,9 @@ struct kvm_pio_request {
 	int in;
 	int port;
 	int size;
+
+	/* Used to handle string I/O that doesn't originate in the emulator. */
+	void *string_data;
 };
 
 #define PT64_ROOT_MAX_LEVEL 5
@@ -701,7 +704,6 @@ struct kvm_vcpu_arch {
 
 	struct kvm_pio_request pio;
 	void *pio_data;
-	void *guest_ins_data;
 
 	u8 event_exit_inst_len;
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index fad2c7192aa3..c4fb8a332111 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -12489,7 +12489,7 @@ static int complete_sev_es_emulated_outs(struct kvm_vcpu *vcpu)
 
 static int complete_sev_es_emulated_ins(struct kvm_vcpu *vcpu)
 {
-	memcpy(vcpu->arch.guest_ins_data, vcpu->arch.pio_data,
+	memcpy(vcpu->arch.pio.string_data, vcpu->arch.pio_data,
 	       vcpu->arch.pio.count * vcpu->arch.pio.size);
 	vcpu->arch.pio.count = 0;
 
@@ -12520,7 +12520,7 @@ static int kvm_sev_es_ins(struct kvm_vcpu *vcpu, unsigned int size,
 	if (ret)
 		return ret;
 
-	vcpu->arch.guest_ins_data = data;
+	vcpu->arch.string_data = data;
 	vcpu->arch.complete_userspace_io = complete_sev_es_emulated_ins;
 	return 0;
 }
-- 
2.33.0.1079.g6e70778dc9-goog

Re: [PATCH 0/8] KVM: SEV-ES: fixes for string I/O emulation

[Paolo Bonzini]

On 22/10/21 01:49, Sean Christopherson wrote:
> On Wed, Oct 13, 2021, Paolo Bonzini wrote:
>> Patches 2 to 7 are a bunch of cleanups to emulator_pio_in and
>> emulator_pio_in_out, so that the final SEV code is a little easier
>> to reason on.  Just a little, no big promises.
> IMO, this series goes in the wrong direction and doesn't make the mess any better,
> just different.
> 
> The underlying issue is that kernel_pio() does the completely horrendous thing
> of consuming vcpu->arch.pio.  That leads to the juggling that this series tries
> to clean up, but it's essentially an impossible problem to solve because the
> approach itself is broken.

I agree on this, but I disagree that the series does not make the mess 
any better.  At the very least, the new signatures for 
__emulator_pio_in, complete_emulator_pio_in and emulator_pio_in_out are 
improvements regarding the _role_ of vcpu->arch.pio*:

- complete_emulator_pio_in clearly takes the values from vcpu->arch.pio, 
which _is_ the right thing to do for a complete_userspace_io function. 
This is not clear of emulator_pio_in before the patch

- __emulator_pio_in and emulator_pio_in_out do not take anymore the 
buffer argument, making it clear that they operate on the internal 
pio_data buffer and only complete_emulator_pio_in copies out of it. 
Which yes is horrible, but at least it is clearly visible in the code now.

I managed to clean things up quite satisfactorily with just 6 patches on 
top of these eight, so I'll post the full series as soon as I finish 
testing them.  5.15 can then include these to fix the bug at hand.

Paolo