From: Sean Christopherson <seanjc@google.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
fwilhelm@google.com, oupton@google.com
Subject: Re: [PATCH 0/8] KVM: SEV-ES: fixes for string I/O emulation
Date: Thu, 21 Oct 2021 23:49:26 +0000 [thread overview]
Message-ID: <YXH8hmB64gnwxIx6@google.com> (raw)
In-Reply-To: <20211013165616.19846-1-pbonzini@redhat.com>
[-- 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
next prev parent reply other threads:[~2021-10-21 23:49 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-13 16:56 [PATCH 0/8] KVM: SEV-ES: fixes for string I/O emulation Paolo Bonzini
2021-10-13 16:56 ` [PATCH 1/8] KVM: SEV-ES: fix length of string I/O Paolo Bonzini
2021-10-14 20:13 ` Tom Lendacky
2021-10-21 23:10 ` Maxim Levitsky
2021-10-25 1:31 ` Marc Orr
2021-10-25 8:59 ` Paolo Bonzini
2021-10-13 16:56 ` [PATCH 2/8] KVM: SEV-ES: rename guest_ins_data to sev_pio_data Paolo Bonzini
2021-10-21 23:12 ` Maxim Levitsky
2021-10-13 16:56 ` [PATCH 3/8] KVM: x86: leave vcpu->arch.pio.count alone in emulator_pio_in_out Paolo Bonzini
2021-10-21 23:12 ` Maxim Levitsky
2021-10-13 16:56 ` [PATCH 4/8] KVM: SEV-ES: clean up kvm_sev_es_ins/outs Paolo Bonzini
2021-10-21 23:14 ` Maxim Levitsky
2021-10-22 16:31 ` Paolo Bonzini
2021-10-13 16:56 ` [PATCH 5/8] KVM: x86: split the two parts of emulator_pio_in Paolo Bonzini
2021-10-21 23:14 ` Maxim Levitsky
2021-10-13 16:56 ` [PATCH 6/8] KVM: x86: remove unnecessary arguments from complete_emulator_pio_in Paolo Bonzini
2021-10-21 23:14 ` Maxim Levitsky
2021-10-13 16:56 ` [PATCH 7/8] KVM: SEV-ES: keep INS functions together Paolo Bonzini
2021-10-21 23:14 ` Maxim Levitsky
2021-10-13 16:56 ` [PATCH 8/8] KVM: SEV-ES: go over the sev_pio_data buffer in multiple passes if needed Paolo Bonzini
2021-10-21 23:15 ` Maxim Levitsky
2021-10-21 17:47 ` [PATCH 0/8] KVM: SEV-ES: fixes for string I/O emulation Paolo Bonzini
2021-10-21 20:04 ` Sean Christopherson
2021-10-21 23:49 ` Sean Christopherson [this message]
2021-10-22 13:33 ` Paolo Bonzini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YXH8hmB64gnwxIx6@google.com \
--to=seanjc@google.com \
--cc=fwilhelm@google.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=oupton@google.com \
--cc=pbonzini@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox