| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZihJOorvMU8GpUBN@google.com>
Date: Tue, 23 Apr 2024 16:50:18 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Peter Gonda <pgonda@...gle.com>
Cc: linux-kernel@...r.kernel.org, Vishal Annapurve <vannapurve@...gle.com>,
Ackerley Tng <ackerleytng@...gle.com>, Paolo Bonzini <pbonzini@...hat.com>,
Claudio Imbrenda <imbrenda@...ux.ibm.com>, Carlos Bilbao <carlos.bilbao@....com>,
Tom Lendacky <thomas.lendacky@....com>, Michael Roth <michael.roth@....com>, kvm@...r.kernel.org,
linux-kselftest@...r.kernel.org
Subject: Re: [PATCH 6/6] Add ability for SEV-ES guests to use ucalls via GHCB
On Tue, Apr 09, 2024, Peter Gonda wrote:
> Modifies ucall handling for SEV-ES VMs.
Please follow the preferred changelog style as described in
Documentation/process/maintainer-kvm-x86.rst
> Instead of using an out instruction and storing the ucall pointer in RDI,
> SEV-ES guests use a outsb VMGEXIT to move the ucall pointer as the data.
Explain _why_. After poking around, I think I agree that string I/O is the least
awful choice, but string I/O is generally unpleasant. E.g. my initial reaction
to this
addr = *(uint64_t *)((uint8_t *)(run) + run->io.data_offset);
was quite literally, "LOL, what?".
We could use MMIO, because there is no *real* instruction in the guest, it's all
make believe, i.e. there doesn't actually need to be MMIO anywhere. But then we
need to define an address; it could simply be the ucall address, but then SEV-ES
ends up with a completely different flow then the regular magic I/O port.
The changelog should capture explain why string I/O was chosen over the "obvious"
alternatives so that readers and reviewers aren't left wondering why on earth
we *chose* to use string I/O.
> Allows for SEV-ES to use ucalls instead of relying the SEV-ES MSR based
> termination protocol.
>
> Cc: Vishal Annapurve <vannapurve@...gle.com>
> Cc: Ackerley Tng <ackerleytng@...gle.com>
> Cc: Paolo Bonzini <pbonzini@...hat.com>
> Cc: Claudio Imbrenda <imbrenda@...ux.ibm.com>
> Cc: Sean Christopherson <seanjc@...gle.com>
> Cc: Carlos Bilbao <carlos.bilbao@....com>
> Cc: Tom Lendacky <thomas.lendacky@....com>
> Cc: Michael Roth <michael.roth@....com>
> Cc: kvm@...r.kernel.org
> Cc: linux-kselftest@...r.kernel.org
> Signed-off-by: Peter Gonda <pgonda@...gle.com>
> ---
> .../selftests/kvm/include/x86_64/sev.h | 2 +
> tools/testing/selftests/kvm/lib/x86_64/sev.c | 67 ++++++++++++++++++-
> .../testing/selftests/kvm/lib/x86_64/ucall.c | 17 +++++
> .../selftests/kvm/x86_64/sev_smoke_test.c | 17 +----
> 4 files changed, 84 insertions(+), 19 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/include/x86_64/sev.h b/tools/testing/selftests/kvm/include/x86_64/sev.h
> index 691dc005e2a1..26447caccd40 100644
> --- a/tools/testing/selftests/kvm/include/x86_64/sev.h
> +++ b/tools/testing/selftests/kvm/include/x86_64/sev.h
> @@ -109,4 +109,6 @@ static inline void sev_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
> bool is_sev_enabled(void);
> bool is_sev_es_enabled(void);
>
> +void sev_es_ucall_port_write(uint32_t port, uint64_t data);
> +
> #endif /* SELFTEST_KVM_SEV_H */
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/sev.c b/tools/testing/selftests/kvm/lib/x86_64/sev.c
> index 5b3f0a8a931a..276477f2c2cf 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/sev.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/sev.c
> @@ -8,11 +8,18 @@
> #include "svm.h"
> #include "svm_util.h"
>
> +#define IOIO_TYPE_STR (1 << 2)
> +#define IOIO_SEG_DS (1 << 11 | 1 << 10)
> +#define IOIO_DATA_8 (1 << 4)
> +#define IOIO_REP (1 << 3)
> +
> +#define SW_EXIT_CODE_IOIO 0x7b
> +
> struct ghcb_entry {
> struct ghcb ghcb;
>
> /* Guest physical address of this GHCB. */
> - void *gpa;
> + uint64_t gpa;
>
> /* Host virtual address of this struct. */
> struct ghcb_entry *hva;
> @@ -45,16 +52,22 @@ void ghcb_init(struct kvm_vm *vm)
> for (i = 0; i < KVM_MAX_VCPUS; ++i) {
> entry = &hdr->ghcbs[i];
> entry->hva = entry;
> - entry->gpa = addr_hva2gpa(vm, &entry->ghcb);
> + entry->gpa = (uint64_t)addr_hva2gpa(vm, &entry->ghcb);
> }
>
> write_guest_global(vm, ghcb_pool, (struct ghcb_header *)vaddr);
> }
>
> +static void sev_es_terminate(void)
> +{
> + wrmsr(MSR_AMD64_SEV_ES_GHCB, GHCB_MSR_TERM_REQ);
> +}
> +
> static struct ghcb_entry *ghcb_alloc(void)
> {
> return &ghcb_pool->ghcbs[0];
> struct ghcb_entry *entry;
> + struct ghcb *ghcb;
> int i;
>
> if (!ghcb_pool)
> @@ -63,12 +76,18 @@ static struct ghcb_entry *ghcb_alloc(void)
> for (i = 0; i < KVM_MAX_VCPUS; ++i) {
> if (!test_and_set_bit(i, ghcb_pool->in_use)) {
> entry = &ghcb_pool->ghcbs[i];
> - memset(&entry->ghcb, 0, sizeof(entry->ghcb));
> + ghcb = &entry->ghcb;
> +
> + memset(&ghcb, 0, sizeof(*ghcb));
> + ghcb->ghcb_usage = 0;
> + ghcb->protocol_version = 1;
> +
> return entry;
> }
> }
>
> ucall_failed:
> + sev_es_terminate();
> return NULL;
> }
>
> @@ -200,3 +219,45 @@ bool is_sev_es_enabled(void)
> return is_sev_enabled() &&
> rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_ES_ENABLED;
> }
> +
> +static uint64_t setup_exitinfo1_portio(uint32_t port)
> +{
> + uint64_t exitinfo1 = 0;
> +
> + exitinfo1 |= IOIO_TYPE_STR;
> + exitinfo1 |= ((port & 0xffff) << 16);
> + exitinfo1 |= IOIO_SEG_DS;
> + exitinfo1 |= IOIO_DATA_8;
> + exitinfo1 |= IOIO_REP;
> +
> + return exitinfo1;
> +}
> +
> +static void do_vmg_exit(uint64_t ghcb_gpa)
> +{
> + wrmsr(MSR_AMD64_SEV_ES_GHCB, ghcb_gpa);
> + __asm__ __volatile__("rep; vmmcall");
> +}
> +
> +void sev_es_ucall_port_write(uint32_t port, uint64_t data)
> +{
> + struct ghcb_entry *entry;
> + struct ghcb *ghcb;
> + const uint64_t exitinfo1 = setup_exitinfo1_portio(port);
> +
> + entry = ghcb_alloc();
> + ghcb = &entry->ghcb;
> +
> + ghcb_set_sw_exit_code(ghcb, SW_EXIT_CODE_IOIO);
> + ghcb_set_sw_exit_info_1(ghcb, exitinfo1);
> + ghcb_set_sw_exit_info_2(ghcb, sizeof(data));
> +
> + // Setup the SW Stratch buffer pointer.
> + ghcb_set_sw_scratch(ghcb,
> + entry->gpa + offsetof(struct ghcb, shared_buffer));
> + memcpy(&ghcb->shared_buffer, &data, sizeof(data));
> +
> + do_vmg_exit(entry->gpa);
> +
> + ghcb_free(entry);
> +}
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/ucall.c b/tools/testing/selftests/kvm/lib/x86_64/ucall.c
> index 1265cecc7dd1..24da2f4316d8 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/ucall.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/ucall.c
> @@ -5,6 +5,8 @@
> * Copyright (C) 2018, Red Hat, Inc.
> */
> #include "kvm_util.h"
> +#include "processor.h"
> +#include "sev.h"
>
> #define UCALL_PIO_PORT ((uint16_t)0x1000)
>
> @@ -21,6 +23,10 @@ void ucall_arch_do_ucall(vm_vaddr_t uc)
> #define HORRIFIC_L2_UCALL_CLOBBER_HACK \
> "rcx", "rsi", "r8", "r9", "r10", "r11"
>
> + if (is_sev_es_enabled()) {
No curly braces needed.
> + sev_es_ucall_port_write(UCALL_PIO_PORT, uc);
> + }
This will clearly fall through to the standard IN, which I suspect is wrong and
only "works" because the only usage is a single GUEST_DONE(), i.e. no test
actually resumes to this point.
> +
> asm volatile("push %%rbp\n\t"
> "push %%r15\n\t"
> "push %%r14\n\t"
> @@ -48,8 +54,19 @@ void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu)
>
> if (run->exit_reason == KVM_EXIT_IO && run->io.port == UCALL_PIO_PORT) {
> struct kvm_regs regs;
> + uint64_t addr;
> +
> + if (vcpu->vm->subtype == VM_SUBTYPE_SEV_ES) {
> + TEST_ASSERT(
No Google3 style please. I'm going to start charging folks for these violations.
I don't know _how_, but darn it, I'll find a way :-)
> + run->io.count == 8 && run->io.size == 1,
> + "SEV-ES ucall exit requires 8 byte string out\n");
> +
> + addr = *(uint64_t *)((uint8_t *)(run) + run->io.data_offset);
Rather than this amazing bit of casting, I'm tempted to say we should add
kvm_vcpu_arch{} and then map the PIO page in vm_arch_vcpu_add(). Then this
is more sanely:
return *(uint64_t *)vcpu->arch.pio);
where vcpu->arch.pio is a "void *". At least, I think that would work?
> + return (void *)addr;
> + }
>
> vcpu_regs_get(vcpu, ®s);
> +
Spurious whitespace.
Powered by blists - more mailing lists