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 <[email protected]>
Signed-off-by: Peter Gonda <[email protected]>
---
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
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;
}
--
2.37.0.170.g444d1eabd0-goog
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 <[email protected]>
> Signed-off-by: Peter Gonda <[email protected]>
>
> ---
> 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 <[email protected]> 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 <[email protected]>
> > Signed-off-by: Peter Gonda <[email protected]>
> >
> > ---
> > 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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!