| Message ID | 20220715192956.1873315-10-pgonda@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | *** KVM: selftests: Add simple SEV test | expand |
On Fri, Jul 15, 2022 at 12:29:54PM -0700, Peter Gonda wrote: > Add support for encrypted, SEV, guests in the ucall framework. If > encryption is enabled set up a pool of ucall structs in the guests' > shared memory region. This was suggested in the thread on "[RFC PATCH > 00/10] KVM: selftests: Add support for test-selectable ucall > implementations". Using a listed as suggested there doesn't work well list > because the list is setup using HVAs not GVAs so use a bitmap + array > solution instead to get the same pool result. > > Suggested-by:Sean Christopherson <seanjc@google.com> > Signed-off-by: Peter Gonda <pgonda@google.com> > > --- > tools/testing/selftests/kvm/Makefile | 1 + > .../selftests/kvm/include/kvm_util_base.h | 30 +++-- > .../selftests/kvm/include/ucall_common.h | 14 +-- > .../testing/selftests/kvm/lib/ucall_common.c | 115 ++++++++++++++++-- > .../testing/selftests/kvm/lib/x86_64/ucall.c | 2 +- > 5 files changed, 134 insertions(+), 28 deletions(-) > > diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile > index 61e85892dd9b..3d9f2a017389 100644 > --- a/tools/testing/selftests/kvm/Makefile > +++ b/tools/testing/selftests/kvm/Makefile > @@ -47,6 +47,7 @@ LIBKVM += lib/rbtree.c > LIBKVM += lib/sparsebit.c > LIBKVM += lib/test_util.c > LIBKVM += lib/ucall_common.c > +LIBKVM += $(top_srcdir)/tools/lib/find_bit.c Why is this file being added? > > LIBKVM_x86_64 += lib/x86_64/apic.c > LIBKVM_x86_64 += lib/x86_64/handlers.S > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h > index 60b604ac9fa9..77aff2356d64 100644 > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h > @@ -65,6 +65,7 @@ struct userspace_mem_regions { > /* Memory encryption policy/configuration. */ > struct vm_memcrypt { > bool enabled; > + bool encrypted; > int8_t enc_by_default; > bool has_enc_bit; > int8_t enc_bit; > @@ -99,6 +100,8 @@ struct kvm_vm { > int stats_fd; > struct kvm_stats_header stats_header; > struct kvm_stats_desc *stats_desc; > + > + struct list_head ucall_list; > }; > > > @@ -141,21 +144,21 @@ enum vm_guest_mode { > > extern enum vm_guest_mode vm_mode_default; > > -#define VM_MODE_DEFAULT vm_mode_default > -#define MIN_PAGE_SHIFT 12U > -#define ptes_per_page(page_size) ((page_size) / 8) > +#define VM_MODE_DEFAULT vm_mode_default > +#define MIN_PAGE_SHIFT 12U > +#define ptes_per_page(page_size) ((page_size) / 8) > > #elif defined(__x86_64__) > > -#define VM_MODE_DEFAULT VM_MODE_PXXV48_4K > -#define MIN_PAGE_SHIFT 12U > -#define ptes_per_page(page_size) ((page_size) / 8) > +#define VM_MODE_DEFAULT VM_MODE_PXXV48_4K > +#define MIN_PAGE_SHIFT 12U > +#define ptes_per_page(page_size) ((page_size) / 8) > > #elif defined(__s390x__) > > -#define VM_MODE_DEFAULT VM_MODE_P44V64_4K > -#define MIN_PAGE_SHIFT 12U > -#define ptes_per_page(page_size) ((page_size) / 16) > +#define VM_MODE_DEFAULT VM_MODE_P44V64_4K > +#define MIN_PAGE_SHIFT 12U > +#define ptes_per_page(page_size) ((page_size) / 16) > > #elif defined(__riscv) > > @@ -163,9 +166,9 @@ extern enum vm_guest_mode vm_mode_default; > #error "RISC-V 32-bit kvm selftests not supported" > #endif > > -#define VM_MODE_DEFAULT VM_MODE_P40V48_4K > -#define MIN_PAGE_SHIFT 12U > -#define ptes_per_page(page_size) ((page_size) / 8) > +#define VM_MODE_DEFAULT VM_MODE_P40V48_4K > +#define MIN_PAGE_SHIFT 12U > +#define ptes_per_page(page_size) ((page_size) / 8) Looks like your editor decided to change all the above defines to use spaces instead of tabs. You might want to double check the other patches as well to ensure lines added use tabs vs. spaces and that there are no other random whitespace changes. > > #endif > > @@ -802,6 +805,9 @@ vm_paddr_t addr_arch_gva2gpa(struct kvm_vm *vm, vm_vaddr_t gva); > > static inline vm_paddr_t addr_gva2gpa(struct kvm_vm *vm, vm_vaddr_t gva) > { > + TEST_ASSERT( > + !vm->memcrypt.encrypted, > + "Encrypted guests have their page tables encrypted so gva2* conversions are not possible."); > return addr_arch_gva2gpa(vm, gva); > } > > diff --git a/tools/testing/selftests/kvm/include/ucall_common.h b/tools/testing/selftests/kvm/include/ucall_common.h > index cb9b37282701..d8ac16a68c0a 100644 > --- a/tools/testing/selftests/kvm/include/ucall_common.h > +++ b/tools/testing/selftests/kvm/include/ucall_common.h > @@ -21,6 +21,10 @@ enum { > struct ucall { > uint64_t cmd; > uint64_t args[UCALL_MAX_ARGS]; > + > + /* For encrypted guests. */ > + uint64_t idx; > + struct ucall *hva; > }; > > void ucall_arch_init(struct kvm_vm *vm, void *arg); > @@ -31,15 +35,9 @@ void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu); > void ucall(uint64_t cmd, int nargs, ...); > uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc); > > -static inline void ucall_init(struct kvm_vm *vm, void *arg) > -{ > - ucall_arch_init(vm, arg); > -} > +void ucall_init(struct kvm_vm *vm, void *arg); > > -static inline void ucall_uninit(struct kvm_vm *vm) > -{ > - ucall_arch_uninit(vm); > -} > +void ucall_uninit(struct kvm_vm *vm); > > #define GUEST_SYNC_ARGS(stage, arg1, arg2, arg3, arg4) \ > ucall(UCALL_SYNC, 6, "hello", stage, arg1, arg2, arg3, arg4) > diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c > index c488ed23d0dd..8e660b10f9b2 100644 > --- a/tools/testing/selftests/kvm/lib/ucall_common.c > +++ b/tools/testing/selftests/kvm/lib/ucall_common.c > @@ -1,22 +1,123 @@ > // SPDX-License-Identifier: GPL-2.0-only > #include "kvm_util.h" > +#include "linux/types.h" > +#include "linux/bitmap.h" > +#include "linux/bitops.h" > +#include "linux/atomic.h" Do we really need bitmap.h, bitops.h, and atomic.h? I see we use clear_bit(), which I think is from atomic.h, and atomic_test_and_set_bit(), which I have no idea where it comes from... > + > +struct ucall_header { > + DECLARE_BITMAP(in_use, KVM_MAX_VCPUS); > + struct ucall ucalls[KVM_MAX_VCPUS]; > +}; > + > +static bool encrypted_guest; > +static struct ucall_header *ucall_hdr; > + > +void ucall_init(struct kvm_vm *vm, void *arg) > +{ > + struct ucall *uc; > + struct ucall_header *hdr; > + vm_vaddr_t vaddr; > + int i; > + > + encrypted_guest = vm->memcrypt.enabled; > + sync_global_to_guest(vm, encrypted_guest); > + if (!encrypted_guest) > + goto out; > + > + TEST_ASSERT(!ucall_hdr, > + "Only a single encrypted guest at a time for ucalls."); > + vaddr = vm_vaddr_alloc_shared(vm, sizeof(*hdr), vm->page_size); > + hdr = (struct ucall_header *)addr_gva2hva(vm, vaddr); > + memset(hdr, 0, sizeof(*hdr)); > + > + for (i = 0; i < KVM_MAX_VCPUS; ++i) { > + uc = &hdr->ucalls[i]; > + uc->hva = uc; > + uc->idx = i; > + } > + > + ucall_hdr = (struct ucall_header *)vaddr; > + sync_global_to_guest(vm, ucall_hdr); > + > +out: > + ucall_arch_init(vm, arg); > +} > + > +void ucall_uninit(struct kvm_vm *vm) > +{ > + encrypted_guest = false; > + ucall_hdr = NULL; > + > + ucall_arch_uninit(vm); > +} > + > +static struct ucall *ucall_alloc(void) > +{ > + struct ucall *uc = NULL; > + int i; > + > + if (!encrypted_guest) > + goto out; > + > + for (i = 0; i < KVM_MAX_VCPUS; ++i) { > + if (atomic_test_and_set_bit(i, ucall_hdr->in_use)) > + continue; > + > + uc = &ucall_hdr->ucalls[i]; Doesn't this just mark all buffers as in-use and return the last one? I think we want for (i = 0; i < KVM_MAX_VCPUS; ++i) { if (!atomic_test_and_set_bit(i, ucall_hdr->in_use)) { uc = &ucall_hdr->ucalls[i]; break; } } > + } > + > +out: > + return uc; > +} > + > +static void ucall_free(struct ucall *uc) > +{ > + if (!encrypted_guest) > + return; > + > + clear_bit(uc->idx, ucall_hdr->in_use); > +} > + > +static vm_vaddr_t get_ucall_addr(struct ucall *uc) > +{ > + if (encrypted_guest) > + return (vm_vaddr_t)uc->hva; > + > + return (vm_vaddr_t)uc; > +} > > void ucall(uint64_t cmd, int nargs, ...) > { > - struct ucall uc = { > - .cmd = cmd, > - }; > + struct ucall *uc; > + struct ucall tmp; > va_list va; > int i; > > + uc = ucall_alloc(); > + if (!uc) > + uc = &tmp; > + > + uc->cmd = cmd; > + > nargs = min(nargs, UCALL_MAX_ARGS); > > va_start(va, nargs); > for (i = 0; i < nargs; ++i) > - uc.args[i] = va_arg(va, uint64_t); > + uc->args[i] = va_arg(va, uint64_t); > va_end(va); > > - ucall_arch_do_ucall((vm_vaddr_t)&uc); > + ucall_arch_do_ucall(get_ucall_addr(uc)); > + > + ucall_free(uc); > +} > + > +void *get_ucall_hva(struct kvm_vm *vm, void *uc) > +{ > + if (encrypted_guest) > + return uc; > + > + return addr_gva2hva(vm, (vm_vaddr_t)uc); > } > > uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc) > @@ -27,9 +128,9 @@ uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc) > if (!uc) > uc = &ucall; > > - addr = ucall_arch_get_ucall(vcpu); > + addr = get_ucall_hva(vcpu->vm, ucall_arch_get_ucall(vcpu)); Hmm... so now it's expected that ucall_arch_get_ucall() returns a gva... > if (addr) { > - memcpy(uc, addr, sizeof(*uc)); > + memcpy(uc, addr, sizeof(struct ucall)); Why make this change? > vcpu_run_complete_io(vcpu); > } else { > memset(uc, 0, sizeof(*uc)); > diff --git a/tools/testing/selftests/kvm/lib/x86_64/ucall.c b/tools/testing/selftests/kvm/lib/x86_64/ucall.c > index ec53a406f689..ea6b2e3a8e39 100644 > --- a/tools/testing/selftests/kvm/lib/x86_64/ucall.c > +++ b/tools/testing/selftests/kvm/lib/x86_64/ucall.c > @@ -30,7 +30,7 @@ void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu) > struct kvm_regs regs; > > vcpu_regs_get(vcpu, ®s); > - return addr_gva2hva(vcpu->vm, (vm_vaddr_t)regs.rdi); > + return (void *)regs.rdi; ...we're only updating x86's ucall_arch_get_ucall() to return gvas. What about the other architectures? Anyway, I'd rather we don't change ucall_arch_get_ucall() to return gvas. They should continue returning hvas and any trickery needed to translate a pool uc to an hva should be put inside ucall_arch_get_ucall(). > } > return NULL; > } > -- > 2.37.0.170.g444d1eabd0-goog > I'm not a big fan of mixing the concept of encrypted guests into ucalls. I think we should have two types of ucalls, those have a uc pool in memory shared with the host and those that don't. Encrypted guests pick the pool version. Thanks, drew
On Tue, Jul 19, 2022 at 9:43 AM Andrew Jones <andrew.jones@linux.dev> wrote: > > On Fri, Jul 15, 2022 at 12:29:54PM -0700, Peter Gonda wrote: > > Add support for encrypted, SEV, guests in the ucall framework. If > > encryption is enabled set up a pool of ucall structs in the guests' > > shared memory region. This was suggested in the thread on "[RFC PATCH > > 00/10] KVM: selftests: Add support for test-selectable ucall > > implementations". Using a listed as suggested there doesn't work well > > list > > > because the list is setup using HVAs not GVAs so use a bitmap + array > > solution instead to get the same pool result. > > > > Suggested-by:Sean Christopherson <seanjc@google.com> > > Signed-off-by: Peter Gonda <pgonda@google.com> > > > > --- > > tools/testing/selftests/kvm/Makefile | 1 + > > .../selftests/kvm/include/kvm_util_base.h | 30 +++-- > > .../selftests/kvm/include/ucall_common.h | 14 +-- > > .../testing/selftests/kvm/lib/ucall_common.c | 115 ++++++++++++++++-- > > .../testing/selftests/kvm/lib/x86_64/ucall.c | 2 +- > > 5 files changed, 134 insertions(+), 28 deletions(-) > > > > diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile > > index 61e85892dd9b..3d9f2a017389 100644 > > --- a/tools/testing/selftests/kvm/Makefile > > +++ b/tools/testing/selftests/kvm/Makefile > > @@ -47,6 +47,7 @@ LIBKVM += lib/rbtree.c > > LIBKVM += lib/sparsebit.c > > LIBKVM += lib/test_util.c > > LIBKVM += lib/ucall_common.c > > +LIBKVM += $(top_srcdir)/tools/lib/find_bit.c > > Why is this file being added? This is a mistake, I'll revert. I was originally trying to use find_first_zero_bit(). > > > > > LIBKVM_x86_64 += lib/x86_64/apic.c > > LIBKVM_x86_64 += lib/x86_64/handlers.S > > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h > > index 60b604ac9fa9..77aff2356d64 100644 > > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h > > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h > > @@ -65,6 +65,7 @@ struct userspace_mem_regions { > > /* Memory encryption policy/configuration. */ > > struct vm_memcrypt { > > bool enabled; > > + bool encrypted; > > int8_t enc_by_default; > > bool has_enc_bit; > > int8_t enc_bit; > > @@ -99,6 +100,8 @@ struct kvm_vm { > > int stats_fd; > > struct kvm_stats_header stats_header; > > struct kvm_stats_desc *stats_desc; > > + > > + struct list_head ucall_list; > > }; > > > > > > @@ -141,21 +144,21 @@ enum vm_guest_mode { > > > > extern enum vm_guest_mode vm_mode_default; > > > > -#define VM_MODE_DEFAULT vm_mode_default > > -#define MIN_PAGE_SHIFT 12U > > -#define ptes_per_page(page_size) ((page_size) / 8) > > +#define VM_MODE_DEFAULT vm_mode_default > > +#define MIN_PAGE_SHIFT 12U > > +#define ptes_per_page(page_size) ((page_size) / 8) > > > > #elif defined(__x86_64__) > > > > -#define VM_MODE_DEFAULT VM_MODE_PXXV48_4K > > -#define MIN_PAGE_SHIFT 12U > > -#define ptes_per_page(page_size) ((page_size) / 8) > > +#define VM_MODE_DEFAULT VM_MODE_PXXV48_4K > > +#define MIN_PAGE_SHIFT 12U > > +#define ptes_per_page(page_size) ((page_size) / 8) > > > > #elif defined(__s390x__) > > > > -#define VM_MODE_DEFAULT VM_MODE_P44V64_4K > > -#define MIN_PAGE_SHIFT 12U > > -#define ptes_per_page(page_size) ((page_size) / 16) > > +#define VM_MODE_DEFAULT VM_MODE_P44V64_4K > > +#define MIN_PAGE_SHIFT 12U > > +#define ptes_per_page(page_size) ((page_size) / 16) > > > > #elif defined(__riscv) > > > > @@ -163,9 +166,9 @@ extern enum vm_guest_mode vm_mode_default; > > #error "RISC-V 32-bit kvm selftests not supported" > > #endif > > > > -#define VM_MODE_DEFAULT VM_MODE_P40V48_4K > > -#define MIN_PAGE_SHIFT 12U > > -#define ptes_per_page(page_size) ((page_size) / 8) > > +#define VM_MODE_DEFAULT VM_MODE_P40V48_4K > > +#define MIN_PAGE_SHIFT 12U > > +#define ptes_per_page(page_size) ((page_size) / 8) > > Looks like your editor decided to change all the above defines to use > spaces instead of tabs. You might want to double check the other > patches as well to ensure lines added use tabs vs. spaces and that > there are no other random whitespace changes. Will fix. > > > > > #endif > > > > @@ -802,6 +805,9 @@ vm_paddr_t addr_arch_gva2gpa(struct kvm_vm *vm, vm_vaddr_t gva); > > > > static inline vm_paddr_t addr_gva2gpa(struct kvm_vm *vm, vm_vaddr_t gva) > > { > > + TEST_ASSERT( > > + !vm->memcrypt.encrypted, > > + "Encrypted guests have their page tables encrypted so gva2* conversions are not possible."); > > return addr_arch_gva2gpa(vm, gva); > > } > > > > diff --git a/tools/testing/selftests/kvm/include/ucall_common.h b/tools/testing/selftests/kvm/include/ucall_common.h > > index cb9b37282701..d8ac16a68c0a 100644 > > --- a/tools/testing/selftests/kvm/include/ucall_common.h > > +++ b/tools/testing/selftests/kvm/include/ucall_common.h > > @@ -21,6 +21,10 @@ enum { > > struct ucall { > > uint64_t cmd; > > uint64_t args[UCALL_MAX_ARGS]; > > + > > + /* For encrypted guests. */ > > + uint64_t idx; > > + struct ucall *hva; > > }; > > > > void ucall_arch_init(struct kvm_vm *vm, void *arg); > > @@ -31,15 +35,9 @@ void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu); > > void ucall(uint64_t cmd, int nargs, ...); > > uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc); > > > > -static inline void ucall_init(struct kvm_vm *vm, void *arg) > > -{ > > - ucall_arch_init(vm, arg); > > -} > > +void ucall_init(struct kvm_vm *vm, void *arg); > > > > -static inline void ucall_uninit(struct kvm_vm *vm) > > -{ > > - ucall_arch_uninit(vm); > > -} > > +void ucall_uninit(struct kvm_vm *vm); > > > > #define GUEST_SYNC_ARGS(stage, arg1, arg2, arg3, arg4) \ > > ucall(UCALL_SYNC, 6, "hello", stage, arg1, arg2, arg3, arg4) > > diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c > > index c488ed23d0dd..8e660b10f9b2 100644 > > --- a/tools/testing/selftests/kvm/lib/ucall_common.c > > +++ b/tools/testing/selftests/kvm/lib/ucall_common.c > > @@ -1,22 +1,123 @@ > > // SPDX-License-Identifier: GPL-2.0-only > > #include "kvm_util.h" > > +#include "linux/types.h" > > +#include "linux/bitmap.h" > > +#include "linux/bitops.h" > > +#include "linux/atomic.h" > > Do we really need bitmap.h, bitops.h, and atomic.h? I see we use > clear_bit(), which I think is from atomic.h, and > atomic_test_and_set_bit(), which I have no idea where it comes from... I added that function to atomic.h in "RFC V1 07/10] tools: Add atomic_test_and_set_bit()". You are right, I should revert the other header additions. > > > + > > +struct ucall_header { > > + DECLARE_BITMAP(in_use, KVM_MAX_VCPUS); > > + struct ucall ucalls[KVM_MAX_VCPUS]; > > +}; > > + > > +static bool encrypted_guest; > > +static struct ucall_header *ucall_hdr; > > + > > +void ucall_init(struct kvm_vm *vm, void *arg) > > +{ > > + struct ucall *uc; > > + struct ucall_header *hdr; > > + vm_vaddr_t vaddr; > > + int i; > > + > > + encrypted_guest = vm->memcrypt.enabled; > > + sync_global_to_guest(vm, encrypted_guest); > > + if (!encrypted_guest) > > + goto out; > > + > > + TEST_ASSERT(!ucall_hdr, > > + "Only a single encrypted guest at a time for ucalls."); > > + vaddr = vm_vaddr_alloc_shared(vm, sizeof(*hdr), vm->page_size); > > + hdr = (struct ucall_header *)addr_gva2hva(vm, vaddr); > > + memset(hdr, 0, sizeof(*hdr)); > > + > > + for (i = 0; i < KVM_MAX_VCPUS; ++i) { > > + uc = &hdr->ucalls[i]; > > + uc->hva = uc; > > + uc->idx = i; > > + } > > + > > + ucall_hdr = (struct ucall_header *)vaddr; > > + sync_global_to_guest(vm, ucall_hdr); > > + > > +out: > > + ucall_arch_init(vm, arg); > > +} > > + > > +void ucall_uninit(struct kvm_vm *vm) > > +{ > > + encrypted_guest = false; > > + ucall_hdr = NULL; > > + > > + ucall_arch_uninit(vm); > > +} > > + > > +static struct ucall *ucall_alloc(void) > > +{ > > + struct ucall *uc = NULL; > > + int i; > > + > > + if (!encrypted_guest) > > + goto out; > > + > > + for (i = 0; i < KVM_MAX_VCPUS; ++i) { > > + if (atomic_test_and_set_bit(i, ucall_hdr->in_use)) > > + continue; > > + > > + uc = &ucall_hdr->ucalls[i]; > > Doesn't this just mark all buffers as in-use and return the last one? > I think we want > > for (i = 0; i < KVM_MAX_VCPUS; ++i) { > if (!atomic_test_and_set_bit(i, ucall_hdr->in_use)) { > uc = &ucall_hdr->ucalls[i]; > break; > } > } Yes, that looks correct. I'll update this. > > > + } > > + > > +out: > > + return uc; > > +} > > + > > +static void ucall_free(struct ucall *uc) > > +{ > > + if (!encrypted_guest) > > + return; > > + > > + clear_bit(uc->idx, ucall_hdr->in_use); > > +} > > + > > +static vm_vaddr_t get_ucall_addr(struct ucall *uc) > > +{ > > + if (encrypted_guest) > > + return (vm_vaddr_t)uc->hva; > > + > > + return (vm_vaddr_t)uc; > > +} > > > > void ucall(uint64_t cmd, int nargs, ...) > > { > > - struct ucall uc = { > > - .cmd = cmd, > > - }; > > + struct ucall *uc; > > + struct ucall tmp; > > va_list va; > > int i; > > > > + uc = ucall_alloc(); > > + if (!uc) > > + uc = &tmp; > > + > > + uc->cmd = cmd; > > + > > nargs = min(nargs, UCALL_MAX_ARGS); > > > > va_start(va, nargs); > > for (i = 0; i < nargs; ++i) > > - uc.args[i] = va_arg(va, uint64_t); > > + uc->args[i] = va_arg(va, uint64_t); > > va_end(va); > > > > - ucall_arch_do_ucall((vm_vaddr_t)&uc); > > + ucall_arch_do_ucall(get_ucall_addr(uc)); > > + > > + ucall_free(uc); > > +} > > + > > +void *get_ucall_hva(struct kvm_vm *vm, void *uc) > > +{ > > + if (encrypted_guest) > > + return uc; > > + > > + return addr_gva2hva(vm, (vm_vaddr_t)uc); > > } > > > > uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc) > > @@ -27,9 +128,9 @@ uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc) > > if (!uc) > > uc = &ucall; > > > > - addr = ucall_arch_get_ucall(vcpu); > > + addr = get_ucall_hva(vcpu->vm, ucall_arch_get_ucall(vcpu)); > > Hmm... so now it's expected that ucall_arch_get_ucall() returns a gva... > > > if (addr) { > > - memcpy(uc, addr, sizeof(*uc)); > > + memcpy(uc, addr, sizeof(struct ucall)); > > Why make this change? I'll revert this. > > > vcpu_run_complete_io(vcpu); > > } else { > > memset(uc, 0, sizeof(*uc)); > > diff --git a/tools/testing/selftests/kvm/lib/x86_64/ucall.c b/tools/testing/selftests/kvm/lib/x86_64/ucall.c > > index ec53a406f689..ea6b2e3a8e39 100644 > > --- a/tools/testing/selftests/kvm/lib/x86_64/ucall.c > > +++ b/tools/testing/selftests/kvm/lib/x86_64/ucall.c > > @@ -30,7 +30,7 @@ void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu) > > struct kvm_regs regs; > > > > vcpu_regs_get(vcpu, ®s); > > - return addr_gva2hva(vcpu->vm, (vm_vaddr_t)regs.rdi); > > + return (void *)regs.rdi; > > ...we're only updating x86's ucall_arch_get_ucall() to return gvas. > What about the other architectures? Anyway, I'd rather we don't > change ucall_arch_get_ucall() to return gvas. They should continue > returning hvas and any trickery needed to translate a pool uc to > an hva should be put inside ucall_arch_get_ucall(). Makes sense. I'll maintain returning HVAs in both cases and let the _arch_ calls handle the translations. > > > } > > return NULL; > > } > > -- > > 2.37.0.170.g444d1eabd0-goog > > > > I'm not a big fan of mixing the concept of encrypted guests into ucalls. I > think we should have two types of ucalls, those have a uc pool in memory > shared with the host and those that don't. Encrypted guests pick the pool > version. Sean suggested this version where encrypted guests and normal guests used the same ucall macros/functions. I am fine with adding a second interface for encrypted VM ucall, do you think macros like ENCRYPTED_GUEST_SYNC, ENCRYPTED_GUEST_ASSERT, and get_encrypted_ucall() ? > > Thanks, > drew
On Wed, Jul 27, 2022 at 07:38:29AM -0600, Peter Gonda wrote: > On Tue, Jul 19, 2022 at 9:43 AM Andrew Jones <andrew.jones@linux.dev> wrote: > > I'm not a big fan of mixing the concept of encrypted guests into ucalls. I > > think we should have two types of ucalls, those have a uc pool in memory > > shared with the host and those that don't. Encrypted guests pick the pool > > version. > > Sean suggested this version where encrypted guests and normal guests > used the same ucall macros/functions. I am fine with adding a second > interface for encrypted VM ucall, do you think macros like > ENCRYPTED_GUEST_SYNC, ENCRYPTED_GUEST_ASSERT, and > get_encrypted_ucall() ? > It's fine to add new functionality to ucall in order to keep the interfaces the same, except for initializing with some sort of indication that the "uc pool" version is needed. I just don't like all the references to encrypted guests inside ucall. ucall should implement uc pools without the current motivation for uc pools creeping into its implementation. Thanks, drew
On Wed, Jul 27, 2022 at 7:56 AM Andrew Jones <andrew.jones@linux.dev> wrote: > > On Wed, Jul 27, 2022 at 07:38:29AM -0600, Peter Gonda wrote: > > On Tue, Jul 19, 2022 at 9:43 AM Andrew Jones <andrew.jones@linux.dev> wrote: > > > I'm not a big fan of mixing the concept of encrypted guests into ucalls. I > > > think we should have two types of ucalls, those have a uc pool in memory > > > shared with the host and those that don't. Encrypted guests pick the pool > > > version. > > > > Sean suggested this version where encrypted guests and normal guests > > used the same ucall macros/functions. I am fine with adding a second > > interface for encrypted VM ucall, do you think macros like > > ENCRYPTED_GUEST_SYNC, ENCRYPTED_GUEST_ASSERT, and > > get_encrypted_ucall() ? > > > > It's fine to add new functionality to ucall in order to keep the > interfaces the same, except for initializing with some sort of indication > that the "uc pool" version is needed. I just don't like all the references > to encrypted guests inside ucall. ucall should implement uc pools without > the current motivation for uc pools creeping into its implementation. Ah that makes sense. So maybe instead of checking for 'if (vm->memcrypt.enabled)' I should just add a new field in kvm_vm to select for use of the uc pool? Something like kvm_vm.enable_uc_pool? Thanks Drew!
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile index 61e85892dd9b..3d9f2a017389 100644 --- a/tools/testing/selftests/kvm/Makefile +++ b/tools/testing/selftests/kvm/Makefile @@ -47,6 +47,7 @@ LIBKVM += lib/rbtree.c LIBKVM += lib/sparsebit.c LIBKVM += lib/test_util.c LIBKVM += lib/ucall_common.c +LIBKVM += $(top_srcdir)/tools/lib/find_bit.c LIBKVM_x86_64 += lib/x86_64/apic.c LIBKVM_x86_64 += lib/x86_64/handlers.S diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h index 60b604ac9fa9..77aff2356d64 100644 --- a/tools/testing/selftests/kvm/include/kvm_util_base.h +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h @@ -65,6 +65,7 @@ struct userspace_mem_regions { /* Memory encryption policy/configuration. */ struct vm_memcrypt { bool enabled; + bool encrypted; int8_t enc_by_default; bool has_enc_bit; int8_t enc_bit; @@ -99,6 +100,8 @@ struct kvm_vm { int stats_fd; struct kvm_stats_header stats_header; struct kvm_stats_desc *stats_desc; + + struct list_head ucall_list; }; @@ -141,21 +144,21 @@ enum vm_guest_mode { extern enum vm_guest_mode vm_mode_default; -#define VM_MODE_DEFAULT vm_mode_default -#define MIN_PAGE_SHIFT 12U -#define ptes_per_page(page_size) ((page_size) / 8) +#define VM_MODE_DEFAULT vm_mode_default +#define MIN_PAGE_SHIFT 12U +#define ptes_per_page(page_size) ((page_size) / 8) #elif defined(__x86_64__) -#define VM_MODE_DEFAULT VM_MODE_PXXV48_4K -#define MIN_PAGE_SHIFT 12U -#define ptes_per_page(page_size) ((page_size) / 8) +#define VM_MODE_DEFAULT VM_MODE_PXXV48_4K +#define MIN_PAGE_SHIFT 12U +#define ptes_per_page(page_size) ((page_size) / 8) #elif defined(__s390x__) -#define VM_MODE_DEFAULT VM_MODE_P44V64_4K -#define MIN_PAGE_SHIFT 12U -#define ptes_per_page(page_size) ((page_size) / 16) +#define VM_MODE_DEFAULT VM_MODE_P44V64_4K +#define MIN_PAGE_SHIFT 12U +#define ptes_per_page(page_size) ((page_size) / 16) #elif defined(__riscv) @@ -163,9 +166,9 @@ extern enum vm_guest_mode vm_mode_default; #error "RISC-V 32-bit kvm selftests not supported" #endif -#define VM_MODE_DEFAULT VM_MODE_P40V48_4K -#define MIN_PAGE_SHIFT 12U -#define ptes_per_page(page_size) ((page_size) / 8) +#define VM_MODE_DEFAULT VM_MODE_P40V48_4K +#define MIN_PAGE_SHIFT 12U +#define ptes_per_page(page_size) ((page_size) / 8) #endif @@ -802,6 +805,9 @@ vm_paddr_t addr_arch_gva2gpa(struct kvm_vm *vm, vm_vaddr_t gva); static inline vm_paddr_t addr_gva2gpa(struct kvm_vm *vm, vm_vaddr_t gva) { + TEST_ASSERT( + !vm->memcrypt.encrypted, + "Encrypted guests have their page tables encrypted so gva2* conversions are not possible."); return addr_arch_gva2gpa(vm, gva); } diff --git a/tools/testing/selftests/kvm/include/ucall_common.h b/tools/testing/selftests/kvm/include/ucall_common.h index cb9b37282701..d8ac16a68c0a 100644 --- a/tools/testing/selftests/kvm/include/ucall_common.h +++ b/tools/testing/selftests/kvm/include/ucall_common.h @@ -21,6 +21,10 @@ enum { struct ucall { uint64_t cmd; uint64_t args[UCALL_MAX_ARGS]; + + /* For encrypted guests. */ + uint64_t idx; + struct ucall *hva; }; void ucall_arch_init(struct kvm_vm *vm, void *arg); @@ -31,15 +35,9 @@ void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu); void ucall(uint64_t cmd, int nargs, ...); uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc); -static inline void ucall_init(struct kvm_vm *vm, void *arg) -{ - ucall_arch_init(vm, arg); -} +void ucall_init(struct kvm_vm *vm, void *arg); -static inline void ucall_uninit(struct kvm_vm *vm) -{ - ucall_arch_uninit(vm); -} +void ucall_uninit(struct kvm_vm *vm); #define GUEST_SYNC_ARGS(stage, arg1, arg2, arg3, arg4) \ ucall(UCALL_SYNC, 6, "hello", stage, arg1, arg2, arg3, arg4) diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c index c488ed23d0dd..8e660b10f9b2 100644 --- a/tools/testing/selftests/kvm/lib/ucall_common.c +++ b/tools/testing/selftests/kvm/lib/ucall_common.c @@ -1,22 +1,123 @@ // SPDX-License-Identifier: GPL-2.0-only #include "kvm_util.h" +#include "linux/types.h" +#include "linux/bitmap.h" +#include "linux/bitops.h" +#include "linux/atomic.h" + +struct ucall_header { + DECLARE_BITMAP(in_use, KVM_MAX_VCPUS); + struct ucall ucalls[KVM_MAX_VCPUS]; +}; + +static bool encrypted_guest; +static struct ucall_header *ucall_hdr; + +void ucall_init(struct kvm_vm *vm, void *arg) +{ + struct ucall *uc; + struct ucall_header *hdr; + vm_vaddr_t vaddr; + int i; + + encrypted_guest = vm->memcrypt.enabled; + sync_global_to_guest(vm, encrypted_guest); + if (!encrypted_guest) + goto out; + + TEST_ASSERT(!ucall_hdr, + "Only a single encrypted guest at a time for ucalls."); + vaddr = vm_vaddr_alloc_shared(vm, sizeof(*hdr), vm->page_size); + hdr = (struct ucall_header *)addr_gva2hva(vm, vaddr); + memset(hdr, 0, sizeof(*hdr)); + + for (i = 0; i < KVM_MAX_VCPUS; ++i) { + uc = &hdr->ucalls[i]; + uc->hva = uc; + uc->idx = i; + } + + ucall_hdr = (struct ucall_header *)vaddr; + sync_global_to_guest(vm, ucall_hdr); + +out: + ucall_arch_init(vm, arg); +} + +void ucall_uninit(struct kvm_vm *vm) +{ + encrypted_guest = false; + ucall_hdr = NULL; + + ucall_arch_uninit(vm); +} + +static struct ucall *ucall_alloc(void) +{ + struct ucall *uc = NULL; + int i; + + if (!encrypted_guest) + goto out; + + for (i = 0; i < KVM_MAX_VCPUS; ++i) { + if (atomic_test_and_set_bit(i, ucall_hdr->in_use)) + continue; + + uc = &ucall_hdr->ucalls[i]; + } + +out: + return uc; +} + +static void ucall_free(struct ucall *uc) +{ + if (!encrypted_guest) + return; + + clear_bit(uc->idx, ucall_hdr->in_use); +} + +static vm_vaddr_t get_ucall_addr(struct ucall *uc) +{ + if (encrypted_guest) + return (vm_vaddr_t)uc->hva; + + return (vm_vaddr_t)uc; +} void ucall(uint64_t cmd, int nargs, ...) { - struct ucall uc = { - .cmd = cmd, - }; + struct ucall *uc; + struct ucall tmp; va_list va; int i; + uc = ucall_alloc(); + if (!uc) + uc = &tmp; + + uc->cmd = cmd; + nargs = min(nargs, UCALL_MAX_ARGS); va_start(va, nargs); for (i = 0; i < nargs; ++i) - uc.args[i] = va_arg(va, uint64_t); + uc->args[i] = va_arg(va, uint64_t); va_end(va); - ucall_arch_do_ucall((vm_vaddr_t)&uc); + ucall_arch_do_ucall(get_ucall_addr(uc)); + + ucall_free(uc); +} + +void *get_ucall_hva(struct kvm_vm *vm, void *uc) +{ + if (encrypted_guest) + return uc; + + return addr_gva2hva(vm, (vm_vaddr_t)uc); } uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc) @@ -27,9 +128,9 @@ uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc) if (!uc) uc = &ucall; - addr = ucall_arch_get_ucall(vcpu); + addr = get_ucall_hva(vcpu->vm, ucall_arch_get_ucall(vcpu)); if (addr) { - memcpy(uc, addr, sizeof(*uc)); + memcpy(uc, addr, sizeof(struct ucall)); vcpu_run_complete_io(vcpu); } else { memset(uc, 0, sizeof(*uc)); diff --git a/tools/testing/selftests/kvm/lib/x86_64/ucall.c b/tools/testing/selftests/kvm/lib/x86_64/ucall.c index ec53a406f689..ea6b2e3a8e39 100644 --- a/tools/testing/selftests/kvm/lib/x86_64/ucall.c +++ b/tools/testing/selftests/kvm/lib/x86_64/ucall.c @@ -30,7 +30,7 @@ void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu) struct kvm_regs regs; vcpu_regs_get(vcpu, ®s); - return addr_gva2hva(vcpu->vm, (vm_vaddr_t)regs.rdi); + return (void *)regs.rdi; } return NULL; }
Add support for encrypted, SEV, guests in the ucall framework. If encryption is enabled set up a pool of ucall structs in the guests' shared memory region. This was suggested in the thread on "[RFC PATCH 00/10] KVM: selftests: Add support for test-selectable ucall implementations". Using a listed as suggested there doesn't work well because the list is setup using HVAs not GVAs so use a bitmap + array solution instead to get the same pool result. Suggested-by:Sean Christopherson <seanjc@google.com> Signed-off-by: Peter Gonda <pgonda@google.com> --- tools/testing/selftests/kvm/Makefile | 1 + .../selftests/kvm/include/kvm_util_base.h | 30 +++-- .../selftests/kvm/include/ucall_common.h | 14 +-- .../testing/selftests/kvm/lib/ucall_common.c | 115 ++++++++++++++++-- .../testing/selftests/kvm/lib/x86_64/ucall.c | 2 +- 5 files changed, 134 insertions(+), 28 deletions(-)