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
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: [email protected]
Fixes: 7ed9abfe8e9f ("KVM: SVM: Support string IO operations for an SEV-ES guest")
Signed-off-by: Paolo Bonzini <[email protected]>
---
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
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: [email protected]
Fixes: 7ed9abfe8e9f ("KVM: SVM: Support string IO operations for an SEV-ES guest")
Signed-off-by: Paolo Bonzini <[email protected]>
---
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
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: [email protected]
Fixes: 7ed9abfe8e9f ("KVM: SVM: Support string IO operations for an SEV-ES guest")
Signed-off-by: Paolo Bonzini <[email protected]>
---
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
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: [email protected]
Fixes: 7ed9abfe8e9f ("KVM: SVM: Support string IO operations for an SEV-ES guest")
Signed-off-by: Paolo Bonzini <[email protected]>
---
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
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: [email protected]
Fixes: 7ed9abfe8e9f ("KVM: SVM: Support string IO operations for an SEV-ES guest")
Signed-off-by: Paolo Bonzini <[email protected]>
---
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
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: [email protected]
Fixes: 7ed9abfe8e9f ("KVM: SVM: Support string IO operations for an SEV-ES guest")
Signed-off-by: Paolo Bonzini <[email protected]>
---
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
Make the diff a little nicer when we actually get to fixing
the bug. No functional change intended.
Cc: [email protected]
Fixes: 7ed9abfe8e9f ("KVM: SVM: Support string IO operations for an SEV-ES guest")
Signed-off-by: Paolo Bonzini <[email protected]>
---
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
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: [email protected]
> Fixes: 7ed9abfe8e9f ("KVM: SVM: Support string IO operations for an SEV-ES guest")
> Signed-off-by: Paolo Bonzini <[email protected]>
Ugh, can't believe I did that...
Acked-by: Tom Lendacky <[email protected]>
> ---
> 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)
>
[Please ignore this email if it already reported ]
Following build errors noticed while building Linux next 20211018
with gcc-11 for i386 architecture.
i686-linux-gnu-ld: arch/x86/kvm/svm/sev.o: in function `sev_es_string_io':
sev.c:(.text+0x110f): undefined reference to `__udivdi3'
make[1]: *** [/builds/linux/Makefile:1247: vmlinux] Error 1
make[1]: Target '__all' not remade because of errors.
make: *** [Makefile:226: __sub-make] Error 2
Build config:
https://builds.tuxbuild.com/1zftLfjR2AyF4rsIfyUCnCTLKFs/config
Reported-by: Linux Kernel Functional Testing <[email protected]>
meta data:
-----------
git_describe: next-20211018
git_repo: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
git_sha: 60e8840126bdcb60bccef74c3f962742183c681f
git_short_log: 60e8840126bd (\"Add linux-next specific files for 20211018\")
kernel_version: 5.15.0-rc5
target_arch: i386
toolchain: gcc-11
steps to reproduce:
https://builds.tuxbuild.com/1zftLfjR2AyF4rsIfyUCnCTLKFs/tuxmake_reproducer.sh
--
Linaro LKFT
https://lkft.linaro.org
On 18/10/21 12:21, Naresh Kamboju wrote:
> [Please ignore this email if it already reported ]
>
> Following build errors noticed while building Linux next 20211018
> with gcc-11 for i386 architecture.
>
> i686-linux-gnu-ld: arch/x86/kvm/svm/sev.o: in function `sev_es_string_io':
> sev.c:(.text+0x110f): undefined reference to `__udivdi3'
> make[1]: *** [/builds/linux/Makefile:1247: vmlinux] Error 1
> make[1]: Target '__all' not remade because of errors.
> make: *** [Makefile:226: __sub-make] Error 2
Thank you very much, I have sent a simple fix of changing the variable
to u32.
Paolo
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
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...).
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: [email protected]
> Fixes: 7ed9abfe8e9f ("KVM: SVM: Support string IO operations for an SEV-ES guest")
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> 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 <[email protected]>
Best regards,
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: [email protected]
> Fixes: 7ed9abfe8e9f ("KVM: SVM: Support string IO operations for an SEV-ES guest")
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> 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 <[email protected]>
Best regards,
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: [email protected]
> Fixes: 7ed9abfe8e9f ("KVM: SVM: Support string IO operations for an SEV-ES guest")
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> 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 <[email protected]>
Best regards,
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: [email protected]
> Fixes: 7ed9abfe8e9f ("KVM: SVM: Support string IO operations for an SEV-ES guest")
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> 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 <[email protected]>
Best regards,
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: [email protected]
> Fixes: 7ed9abfe8e9f ("KVM: SVM: Support string IO operations for an SEV-ES guest")
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> 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 <[email protected]>
Best regards,
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: [email protected]
> Fixes: 7ed9abfe8e9f ("KVM: SVM: Support string IO operations for an SEV-ES guest")
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> 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 <[email protected]>
Best regards,
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: [email protected]
> Fixes: 7ed9abfe8e9f ("KVM: SVM: Support string IO operations for an SEV-ES guest")
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> 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 <[email protected]>
Best regards,
Maxim Levitsky
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.
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
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
On Wed, Oct 13, 2021 at 9:56 AM Paolo Bonzini <[email protected]> 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: [email protected]
> Fixes: 7ed9abfe8e9f ("KVM: SVM: Support string IO operations for an SEV-ES guest")
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> 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.
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