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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.