by Paolo Bonzini

[permalink] [raw]

Subject: Re: [PATCH RFC 00/39] x86/KVM: Xen HVM guest support

On 20/02/19 21:15, Joao Martins wrote:
> 2. PV Driver support (patches 17 - 39)
>
> We start by redirecting hypercalls from the backend to routines
> which emulate the behaviour that PV backends expect i.e. grant
> table and interdomain events. Next, we add support for late
> initialization of xenbus, followed by implementing
> frontend/backend communication mechanisms (i.e. grant tables and
> interdomain event channels). Finally, introduce xen-shim.ko,
> which will setup a limited Xen environment. This uses the added
> functionality of Xen specific shared memory (grant tables) and
> notifications (event channels).

I am a bit worried by the last patches, they seem really brittle and
prone to breakage. I don't know Xen well enough to understand if the
lack of support for GNTMAP_host_map is fixable, but if not, you have to
define a completely different hypercall.

Of course, tests are missing. You should use the
tools/testing/selftests/kvm/ framework, and ideally each patch should
come with coverage for the newly-added code.

Thanks,

Paolo

2019-02-20 23:42:42

smime.p7s (5.05 kB)

2020-12-01 22:34:07

by David Woodhouse

[permalink] [raw]

Subject: Re: [PATCH RFC 02/39] KVM: x86/xen: intercept xen hypercalls if enabled

On Wed, 2019-02-20 at 20:15 +0000, Joao Martins wrote:
> Add a new exit reason for emulator to handle Xen hypercalls.
> Albeit these are injected only if guest has initialized the Xen
> hypercall page

I've reworked this a little.

I didn't like the inconsistency of allowing userspace to provide the
hypercall pages even though the ABI is now defined by the kernel and it
*has* to be VMCALL/VMMCALL.

So I switched it to generate the hypercall page directly from the
kernel, just like we do for the Hyper-V hypercall page.

I introduced a new flag in the xen_hvm_config struct to enable this
behaviour, and advertised it in the KVM_CAP_XEN_HVM return value.

I also added the cpl and support for 6-argument hypercalls, and made it
check the guest RIP when completing the call as discussed (although I
still think that probably ought to be a generic thing).

I adjusted the test case from my version of the patch, and added
support for actually testing the hypercall page MSR.

https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/xenpv

I'll go through and rebase your patch series at least up to patch 16
and collect them in that tree, then probably post them for real once
I've got everything working locally.

=======================
From c037c329c8867b219afe2100e383c62e9db7b06d Mon Sep 17 00:00:00 2001
From: Joao Martins <[email protected]>
Date: Wed, 13 Jun 2018 09:55:44 -0400
Subject: [PATCH] KVM: x86/xen: intercept xen hypercalls if enabled

Add a new exit reason for emulator to handle Xen hypercalls.

Since this means KVM owns the ABI, dispense with the facility for the
VMM to provide its own copy of the hypercall pages; just fill them in
directly using VMCALL/VMMCALL as we do for the Hyper-V hypercall page.

This behaviour is enabled by a new INTERCEPT_HCALL flag in the
KVM_XEN_HVM_CONFIG ioctl structure, and advertised by the same flag
being returned from the KVM_CAP_XEN_HVM check.

Add a test case and shift xen_hvm_config() to the nascent xen.c while
we're at it.

Signed-off-by: Joao Martins <[email protected]>
Signed-off-by: David Woodhouse <[email protected]>
---
arch/x86/include/asm/kvm_host.h | 6 +
arch/x86/kvm/Makefile | 2 +-
arch/x86/kvm/trace.h | 36 +++++
arch/x86/kvm/x86.c | 46 +++---
arch/x86/kvm/xen.c | 140 ++++++++++++++++++
arch/x86/kvm/xen.h | 21 +++
include/uapi/linux/kvm.h | 19 +++
tools/testing/selftests/kvm/Makefile | 1 +
tools/testing/selftests/kvm/lib/kvm_util.c | 1 +
.../selftests/kvm/x86_64/xen_vmcall_test.c | 123 +++++++++++++++
10 files changed, 365 insertions(+), 30 deletions(-)
create mode 100644 arch/x86/kvm/xen.c
create mode 100644 arch/x86/kvm/xen.h
create mode 100644 tools/testing/selftests/kvm/x86_64/xen_vmcall_test.c

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 7e5f33a0d0e2..9de3229e91e1 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -520,6 +520,11 @@ struct kvm_vcpu_hv {
cpumask_t tlb_flush;
};

+/* Xen HVM per vcpu emulation context */
+struct kvm_vcpu_xen {
+ u64 hypercall_rip;
+};
+
struct kvm_vcpu_arch {
/*
* rip and regs accesses must go through
@@ -717,6 +722,7 @@ struct kvm_vcpu_arch {
unsigned long singlestep_rip;

struct kvm_vcpu_hv hyperv;
+ struct kvm_vcpu_xen xen;

cpumask_var_t wbinvd_dirty_mask;

diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile
index b804444e16d4..8bee4afc1fec 100644
--- a/arch/x86/kvm/Makefile
+++ b/arch/x86/kvm/Makefile
@@ -13,7 +13,7 @@ kvm-y += $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o \
$(KVM)/eventfd.o $(KVM)/irqchip.o $(KVM)/vfio.o
kvm-$(CONFIG_KVM_ASYNC_PF) += $(KVM)/async_pf.o

-kvm-y += x86.o emulate.o i8259.o irq.o lapic.o \
+kvm-y += x86.o emulate.o i8259.o irq.o lapic.o xen.o \
i8254.o ioapic.o irq_comm.o cpuid.o pmu.o mtrr.o \
hyperv.o debugfs.o mmu/mmu.o mmu/page_track.o \
mmu/spte.o mmu/tdp_iter.o mmu/tdp_mmu.o
diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index aef960f90f26..d28ecb37b62c 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -92,6 +92,42 @@ TRACE_EVENT(kvm_hv_hypercall,
__entry->outgpa)
);

+/*
+ * Tracepoint for Xen hypercall.
+ */
+TRACE_EVENT(kvm_xen_hypercall,
+ TP_PROTO(unsigned long nr, unsigned long a0, unsigned long a1,
+ unsigned long a2, unsigned long a3, unsigned long a4,
+ unsigned long a5),
+ TP_ARGS(nr, a0, a1, a2, a3, a4, a5),
+
+ TP_STRUCT__entry(
+ __field(unsigned long, nr)
+ __field(unsigned long, a0)
+ __field(unsigned long, a1)
+ __field(unsigned long, a2)
+ __field(unsigned long, a3)
+ __field(unsigned long, a4)
+ __field(unsigned long, a5)
+ ),
+
+ TP_fast_assign(
+ __entry->nr = nr;
+ __entry->a0 = a0;
+ __entry->a1 = a1;
+ __entry->a2 = a2;
+ __entry->a3 = a3;
+ __entry->a4 = a4;
+ __entry->a4 = a5;
+ ),
+
+ TP_printk("nr 0x%lx a0 0x%lx a1 0x%lx a2 0x%lx a3 0x%lx a4 0x%lx a5 %lx",
+ __entry->nr, __entry->a0, __entry->a1, __entry->a2,
+ __entry->a3, __entry->a4, __entry->a5)
+);
+
+
+
/*
* Tracepoint for PIO.
*/
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 0836023963ec..adf04e8cc64a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -29,6 +29,7 @@
#include "pmu.h"
#include "hyperv.h"
#include "lapic.h"
+#include "xen.h"

#include <linux/clocksource.h>
#include <linux/interrupt.h>
@@ -2842,32 +2843,6 @@ static int set_msr_mce(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
return 0;
}

-static int xen_hvm_config(struct kvm_vcpu *vcpu, u64 data)
-{
- struct kvm *kvm = vcpu->kvm;
- int lm = is_long_mode(vcpu);
- u8 *blob_addr = lm ? (u8 *)(long)kvm->arch.xen_hvm_config.blob_addr_64
- : (u8 *)(long)kvm->arch.xen_hvm_config.blob_addr_32;
- u8 blob_size = lm ? kvm->arch.xen_hvm_config.blob_size_64
- : kvm->arch.xen_hvm_config.blob_size_32;
- u32 page_num = data & ~PAGE_MASK;
- u64 page_addr = data & PAGE_MASK;
- u8 *page;
-
- if (page_num >= blob_size)
- return 1;
-
- page = memdup_user(blob_addr + (page_num * PAGE_SIZE), PAGE_SIZE);
- if (IS_ERR(page))
- return PTR_ERR(page);
-
- if (kvm_vcpu_write_guest(vcpu, page_addr, page, PAGE_SIZE)) {
- kfree(page);
- return 1;
- }
- return 0;
-}
-
static inline bool kvm_pv_async_pf_enabled(struct kvm_vcpu *vcpu)
{
u64 mask = KVM_ASYNC_PF_ENABLED | KVM_ASYNC_PF_DELIVERY_AS_INT;
@@ -3002,7 +2977,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
u64 data = msr_info->data;

if (msr && (msr == vcpu->kvm->arch.xen_hvm_config.msr))
- return xen_hvm_config(vcpu, data);
+ return kvm_xen_hvm_config(vcpu, data);

switch (msr) {
case MSR_AMD64_NB_CFG:
@@ -3703,7 +3678,6 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
case KVM_CAP_PIT2:
case KVM_CAP_PIT_STATE2:
case KVM_CAP_SET_IDENTITY_MAP_ADDR:
- case KVM_CAP_XEN_HVM:
case KVM_CAP_VCPU_EVENTS:
case KVM_CAP_HYPERV:
case KVM_CAP_HYPERV_VAPIC:
@@ -3742,6 +3716,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
case KVM_CAP_ENFORCE_PV_FEATURE_CPUID:
r = 1;
break;
+ case KVM_CAP_XEN_HVM:
+ r = 1 | KVM_XEN_HVM_CONFIG_INTERCEPT_HCALL;
+ break;
case KVM_CAP_SYNC_REGS:
r = KVM_SYNC_X86_VALID_FIELDS;
break;
@@ -5603,7 +5580,15 @@ long kvm_arch_vm_ioctl(struct file *filp,
if (copy_from_user(&xhc, argp, sizeof(xhc)))
goto out;
r = -EINVAL;
- if (xhc.flags)
+ if (xhc.flags & ~KVM_XEN_HVM_CONFIG_INTERCEPT_HCALL)
+ goto out;
+ /*
+ * With hypercall interception the kernel generates its own
+ * hypercall page so it must not be provided.
+ */
+ if ((xhc.flags & KVM_XEN_HVM_CONFIG_INTERCEPT_HCALL) &&
+ (xhc.blob_addr_32 || xhc.blob_addr_64 ||
+ xhc.blob_size_32 || xhc.blob_size_64))
goto out;
memcpy(&kvm->arch.xen_hvm_config, &xhc, sizeof(xhc));
r = 0;
@@ -8066,6 +8051,9 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
unsigned long nr, a0, a1, a2, a3, ret;
int op_64_bit;

+ if (kvm_xen_hypercall_enabled(vcpu->kvm))
+ return kvm_xen_hypercall(vcpu);
+
if (kvm_hv_hypercall_enabled(vcpu->kvm))
return kvm_hv_hypercall(vcpu);

diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
new file mode 100644
index 000000000000..6400a4bc8480
--- /dev/null
+++ b/arch/x86/kvm/xen.c
@@ -0,0 +1,140 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright © 2019 Oracle and/or its affiliates. All rights reserved.
+ * Copyright © 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * KVM Xen emulation
+ */
+
+#include "x86.h"
+#include "xen.h"
+
+#include <linux/kvm_host.h>
+
+#include <trace/events/kvm.h>
+
+#include "trace.h"
+
+int kvm_xen_hvm_config(struct kvm_vcpu *vcpu, u64 data)
+ {
+ struct kvm *kvm = vcpu->kvm;
+ u32 page_num = data & ~PAGE_MASK;
+ u64 page_addr = data & PAGE_MASK;
+
+ /*
+ * If Xen hypercall intercept is enabled, fill the hypercall
+ * page with VMCALL/VMMCALL instructions since that's what
+ * we catch. Else the VMM has provided the hypercall pages
+ * with instructions of its own choosing, so use those.
+ */
+ if (kvm_xen_hypercall_enabled(kvm)) {
+ u8 instructions[32];
+ int i;
+
+ if (page_num)
+ return 1;
+
+ /* mov imm32, %eax */
+ instructions[0] = 0xb8;
+
+ /* vmcall / vmmcall */
+ kvm_x86_ops.patch_hypercall(vcpu, instructions + 5);
+
+ /* ret */
+ instructions[8] = 0xc3;
+
+ /* int3 to pad */
+ memset(instructions + 9, 0xcc, sizeof(instructions) - 9);
+
+ for (i = 0; i < PAGE_SIZE / sizeof(instructions); i++) {
+ *(u32 *)&instructions[1] = i;
+ if (kvm_vcpu_write_guest(vcpu,
+ page_addr + (i * sizeof(instructions)),
+ instructions, sizeof(instructions)))
+ return 1;
+ }
+ } else {
+ int lm = is_long_mode(vcpu);
+ u8 *blob_addr = lm ? (u8 *)(long)kvm->arch.xen_hvm_config.blob_addr_64
+ : (u8 *)(long)kvm->arch.xen_hvm_config.blob_addr_32;
+ u8 blob_size = lm ? kvm->arch.xen_hvm_config.blob_size_64
+ : kvm->arch.xen_hvm_config.blob_size_32;
+ u8 *page;
+
+ if (page_num >= blob_size)
+ return 1;
+
+ page = memdup_user(blob_addr + (page_num * PAGE_SIZE), PAGE_SIZE);
+ if (IS_ERR(page))
+ return PTR_ERR(page);
+
+ if (kvm_vcpu_write_guest(vcpu, page_addr, page, PAGE_SIZE)) {
+ kfree(page);
+ return 1;
+ }
+ }
+ return 0;
+}
+
+static int kvm_xen_hypercall_set_result(struct kvm_vcpu *vcpu, u64 result)
+{
+ kvm_rax_write(vcpu, result);
+ return kvm_skip_emulated_instruction(vcpu);
+}
+
+static int kvm_xen_hypercall_complete_userspace(struct kvm_vcpu *vcpu)
+{
+ struct kvm_run *run = vcpu->run;
+
+ if (unlikely(!kvm_is_linear_rip(vcpu, vcpu->arch.xen.hypercall_rip)))
+ return 1;
+
+ return kvm_xen_hypercall_set_result(vcpu, run->xen.u.hcall.result);
+}
+
+int kvm_xen_hypercall(struct kvm_vcpu *vcpu)
+{
+ bool longmode;
+ u64 input, params[6];
+
+ input = (u64)kvm_register_read(vcpu, VCPU_REGS_RAX);
+
+ longmode = is_64_bit_mode(vcpu);
+ if (!longmode) {
+ params[0] = (u32)kvm_rbx_read(vcpu);
+ params[1] = (u32)kvm_rcx_read(vcpu);
+ params[2] = (u32)kvm_rdx_read(vcpu);
+ params[3] = (u32)kvm_rsi_read(vcpu);
+ params[4] = (u32)kvm_rdi_read(vcpu);
+ params[5] = (u32)kvm_rbp_read(vcpu);
+ }
+#ifdef CONFIG_X86_64
+ else {
+ params[0] = (u64)kvm_rdi_read(vcpu);
+ params[1] = (u64)kvm_rsi_read(vcpu);
+ params[2] = (u64)kvm_rdx_read(vcpu);
+ params[3] = (u64)kvm_r10_read(vcpu);
+ params[4] = (u64)kvm_r8_read(vcpu);
+ params[5] = (u64)kvm_r9_read(vcpu);
+ }
+#endif
+ trace_kvm_xen_hypercall(input, params[0], params[1], params[2],
+ params[3], params[4], params[5]);
+
+ vcpu->run->exit_reason = KVM_EXIT_XEN;
+ vcpu->run->xen.type = KVM_EXIT_XEN_HCALL;
+ vcpu->run->xen.u.hcall.longmode = longmode;
+ vcpu->run->xen.u.hcall.cpl = kvm_x86_ops.get_cpl(vcpu);
+ vcpu->run->xen.u.hcall.input = input;
+ vcpu->run->xen.u.hcall.params[0] = params[0];
+ vcpu->run->xen.u.hcall.params[1] = params[1];
+ vcpu->run->xen.u.hcall.params[2] = params[2];
+ vcpu->run->xen.u.hcall.params[3] = params[3];
+ vcpu->run->xen.u.hcall.params[4] = params[4];
+ vcpu->run->xen.u.hcall.params[5] = params[5];
+ vcpu->arch.xen.hypercall_rip = kvm_get_linear_rip(vcpu);
+ vcpu->arch.complete_userspace_io =
+ kvm_xen_hypercall_complete_userspace;
+
+ return 0;
+}
diff --git a/arch/x86/kvm/xen.h b/arch/x86/kvm/xen.h
new file mode 100644
index 000000000000..81e12f716d2e
--- /dev/null
+++ b/arch/x86/kvm/xen.h
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright © 2019 Oracle and/or its affiliates. All rights reserved.
+ * Copyright © 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * KVM Xen emulation
+ */
+
+#ifndef __ARCH_X86_KVM_XEN_H__
+#define __ARCH_X86_KVM_XEN_H__
+
+int kvm_xen_hypercall(struct kvm_vcpu *vcpu);
+int kvm_xen_hvm_config(struct kvm_vcpu *vcpu, u64 data);
+
+static inline bool kvm_xen_hypercall_enabled(struct kvm *kvm)
+{
+ return kvm->arch.xen_hvm_config.flags &
+ KVM_XEN_HVM_CONFIG_INTERCEPT_HCALL;
+}
+
+#endif /* __ARCH_X86_KVM_XEN_H__ */
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index ca41220b40b8..00221fe56994 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -216,6 +216,20 @@ struct kvm_hyperv_exit {
} u;
};

+struct kvm_xen_exit {
+#define KVM_EXIT_XEN_HCALL 1
+ __u32 type;
+ union {
+ struct {
+ __u32 longmode;
+ __u32 cpl;
+ __u64 input;
+ __u64 result;
+ __u64 params[6];
+ } hcall;
+ } u;
+};
+
#define KVM_S390_GET_SKEYS_NONE 1
#define KVM_S390_SKEYS_MAX 1048576

@@ -250,6 +264,7 @@ struct kvm_hyperv_exit {
#define KVM_EXIT_ARM_NISV 28
#define KVM_EXIT_X86_RDMSR 29
#define KVM_EXIT_X86_WRMSR 30
+#define KVM_EXIT_XEN 31

/* For KVM_EXIT_INTERNAL_ERROR */
/* Emulate instruction failed. */
@@ -426,6 +441,8 @@ struct kvm_run {
__u32 index; /* kernel -> user */
__u64 data; /* kernel <-> user */
} msr;
+ /* KVM_EXIT_XEN */
+ struct kvm_xen_exit xen;
/* Fix the size of the union. */
char padding[256];
};
@@ -1126,6 +1143,8 @@ struct kvm_x86_mce {
#endif

#ifdef KVM_CAP_XEN_HVM
+#define KVM_XEN_HVM_CONFIG_INTERCEPT_HCALL (1 << 1)
+
struct kvm_xen_hvm_config {
__u32 flags;
__u32 msr;
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 3d14ef77755e..d94abec627e6 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -59,6 +59,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/xss_msr_test
TEST_GEN_PROGS_x86_64 += x86_64/debug_regs
TEST_GEN_PROGS_x86_64 += x86_64/tsc_msrs_test
TEST_GEN_PROGS_x86_64 += x86_64/user_msr_test
+TEST_GEN_PROGS_x86_64 += x86_64/xen_vmcall_test
TEST_GEN_PROGS_x86_64 += demand_paging_test
TEST_GEN_PROGS_x86_64 += dirty_log_test
TEST_GEN_PROGS_x86_64 += dirty_log_perf_test
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 126c6727a6b0..6e96ae47d28c 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -1654,6 +1654,7 @@ static struct exit_reason {
{KVM_EXIT_INTERNAL_ERROR, "INTERNAL_ERROR"},
{KVM_EXIT_OSI, "OSI"},
{KVM_EXIT_PAPR_HCALL, "PAPR_HCALL"},
+ {KVM_EXIT_XEN, "XEN"},
#ifdef KVM_EXIT_MEMORY_NOT_PRESENT
{KVM_EXIT_MEMORY_NOT_PRESENT, "MEMORY_NOT_PRESENT"},
#endif
diff --git a/tools/testing/selftests/kvm/x86_64/xen_vmcall_test.c b/tools/testing/selftests/kvm/x86_64/xen_vmcall_test.c
new file mode 100644
index 000000000000..3f1dd93626e5
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86_64/xen_vmcall_test.c
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * svm_vmcall_test
+ *
+ * Copyright © 2020 Amazon.com, Inc. or its affiliates.
+ *
+ * Userspace hypercall testing
+ */
+
+#include "test_util.h"
+#include "kvm_util.h"
+#include "processor.h"
+
+#define VCPU_ID 5
+
+#define HCALL_REGION_GPA 0xc0000000ULL
+#define HCALL_REGION_SLOT 10
+
+static struct kvm_vm *vm;
+
+#define INPUTVALUE 17
+#define ARGVALUE(x) (0xdeadbeef5a5a0000UL + x)
+#define RETVALUE 0xcafef00dfbfbffffUL
+
+#define XEN_HYPERCALL_MSR 0x40000000
+
+static void guest_code(void)
+{
+ unsigned long rax = INPUTVALUE;
+ unsigned long rdi = ARGVALUE(1);
+ unsigned long rsi = ARGVALUE(2);
+ unsigned long rdx = ARGVALUE(3);
+ register unsigned long r10 __asm__("r10") = ARGVALUE(4);
+ register unsigned long r8 __asm__("r8") = ARGVALUE(5);
+ register unsigned long r9 __asm__("r9") = ARGVALUE(6);
+
+ /* First a direct invocation of 'vmcall' */
+ __asm__ __volatile__("vmcall" :
+ "=a"(rax) :
+ "a"(rax), "D"(rdi), "S"(rsi), "d"(rdx),
+ "r"(r10), "r"(r8), "r"(r9));
+ GUEST_ASSERT(rax == RETVALUE);
+
+ /* Now fill in the hypercall page */
+ __asm__ __volatile__("wrmsr" : : "c" (XEN_HYPERCALL_MSR),
+ "a" (HCALL_REGION_GPA & 0xffffffff),
+ "d" (HCALL_REGION_GPA >> 32));
+
+ /* And invoke the same hypercall that way */
+ __asm__ __volatile__("call *%1" : "=a"(rax) :
+ "r"(HCALL_REGION_GPA + INPUTVALUE * 32),
+ "a"(rax), "D"(rdi), "S"(rsi), "d"(rdx),
+ "r"(r10), "r"(r8), "r"(r9));
+ GUEST_ASSERT(rax == RETVALUE);
+
+ GUEST_DONE();
+}
+
+int main(int argc, char *argv[])
+{
+ if (!(kvm_check_cap(KVM_CAP_XEN_HVM) &
+ KVM_XEN_HVM_CONFIG_INTERCEPT_HCALL) ) {
+ print_skip("KVM_XEN_HVM_CONFIG_INTERCEPT_HCALL not available");
+ exit(KSFT_SKIP);
+ }
+
+ vm = vm_create_default(VCPU_ID, 0, (void *) guest_code);
+ vcpu_set_cpuid(vm, VCPU_ID, kvm_get_supported_cpuid());
+
+ struct kvm_xen_hvm_config hvmc = {
+ .flags = KVM_XEN_HVM_CONFIG_INTERCEPT_HCALL,
+ .msr = XEN_HYPERCALL_MSR,
+ };
+ vm_ioctl(vm, KVM_XEN_HVM_CONFIG, &hvmc);
+
+ /* Map a region for the hypercall page */
+ vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS,
+ HCALL_REGION_GPA, HCALL_REGION_SLOT,
+ getpagesize(), 0);
+ virt_map(vm, HCALL_REGION_GPA, HCALL_REGION_GPA, 1, 0);
+
+ for (;;) {
+ volatile struct kvm_run *run = vcpu_state(vm, VCPU_ID);
+ struct ucall uc;
+
+ vcpu_run(vm, VCPU_ID);
+
+ if (run->exit_reason == KVM_EXIT_XEN) {
+ ASSERT_EQ(run->xen.type, KVM_EXIT_XEN_HCALL);
+ ASSERT_EQ(run->xen.u.hcall.cpl, 0);
+ ASSERT_EQ(run->xen.u.hcall.longmode, 1);
+ ASSERT_EQ(run->xen.u.hcall.input, INPUTVALUE);
+ ASSERT_EQ(run->xen.u.hcall.params[0], ARGVALUE(1));
+ ASSERT_EQ(run->xen.u.hcall.params[1], ARGVALUE(2));
+ ASSERT_EQ(run->xen.u.hcall.params[2], ARGVALUE(3));
+ ASSERT_EQ(run->xen.u.hcall.params[3], ARGVALUE(4));
+ ASSERT_EQ(run->xen.u.hcall.params[4], ARGVALUE(5));
+ ASSERT_EQ(run->xen.u.hcall.params[5], ARGVALUE(6));
+ run->xen.u.hcall.result = RETVALUE;
+ continue;
+ }
+
+ TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
+ "Got exit_reason other than KVM_EXIT_IO: %u (%s)\n",
+ run->exit_reason,
+ exit_reason_str(run->exit_reason));
+
+ switch (get_ucall(vm, VCPU_ID, &uc)) {
+ case UCALL_ABORT:
+ TEST_FAIL("%s", (const char *)uc.args[0]);
+ /* NOT REACHED */
+ case UCALL_SYNC:
+ break;
+ case UCALL_DONE:
+ goto done;
+ default:
+ TEST_FAIL("Unknown ucall 0x%lx.", uc.cmd);
+ }
+ }
+done:
+ kvm_vm_free(vm);
+ return 0;
+}
--
2.17.1

Attachments:

smime.p7s (5.05 kB)

2020-12-01 22:34:17

On Tue, 2020-12-01 at 16:40 -0800, Ankur Arora wrote:
> On 2020-12-01 5:07 a.m., David Woodhouse wrote:
> > On Wed, 2019-02-20 at 20:15 +0000, Joao Martins wrote:
> > > +static int kvm_xen_shared_info_init(struct kvm *kvm, gfn_t gfn)
> > > +{
> > > + struct shared_info *shared_info;
> > > + struct page *page;
> > > +
> > > + page = gfn_to_page(kvm, gfn);
> > > + if (is_error_page(page))
> > > + return -EINVAL;
> > > +
> > > + kvm->arch.xen.shinfo_addr = gfn;
> > > +
> > > + shared_info = page_to_virt(page);
> > > + memset(shared_info, 0, sizeof(struct shared_info));
> > > + kvm->arch.xen.shinfo = shared_info;
> > > + return 0;
> > > +}
> > > +
> >
> > Hm.
> >
> > How come we get to pin the page and directly dereference it every time,
> > while kvm_setup_pvclock_page() has to use kvm_write_guest_cached()
> > instead?
>
> So looking at my WIP trees from the time, this is something that
> we went back and forth on as well with using just a pinned page or a
> persistent kvm_vcpu_map().

OK, thanks.

> I remember distinguishing shared_info/vcpu_info from kvm_setup_pvclock_page()
> as shared_info is created early and is not expected to change during the
> lifetime of the guest which didn't seem true for MSR_KVM_SYSTEM_TIME (or
> MSR_KVM_STEAL_TIME) so that would either need to do a kvm_vcpu_map()
> kvm_vcpu_unmap() dance or do some kind of synchronization.
>
> That said, I don't think this code explicitly disallows any updates
> to shared_info.

It needs to allow updates as well as disabling the shared_info pages.
We're going to need that to implement SHUTDOWN_soft_reset for kexec.

> >
> > If that was allowed, wouldn't it have been a much simpler fix for
> > CVE-2019-3016? What am I missing?
>
> Agreed.
>
> Perhaps, Paolo can chime in with why KVM never uses pinned page
> and always prefers to do cached mappings instead?
>
> >
> > Should I rework these to use kvm_write_guest_cached()?
>
> kvm_vcpu_map() would be better. The event channel logic does RMW operations
> on shared_info->vcpu_info.

I've ported the shared_info/vcpu_info parts and made a test case, and
was going back through to make it use kvm_write_guest_cached(). But I
should probably plug on through the evtchn bits before I do that.

I also don't see much locking on the cache, and can't convince myself
that accessing the shared_info page from multiple CPUs with
kvm_write_guest_cached() or kvm_map_gfn() is sane unless they each have
their own cache.

What I have so far is at

https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/xenpv

I'll do the event channel support tomorrow and hook it up to my actual
VMM to give it some more serious testing.

Attachments:

smime.p7s (5.05 kB)

2020-12-02 05:23:26

by Ankur Arora

[permalink] [raw]

Subject: Re: [PATCH RFC 02/39] KVM: x86/xen: intercept xen hypercalls if enabled

On 2020-12-01 1:48 a.m., David Woodhouse wrote:
> On Wed, 2019-02-20 at 20:15 +0000, Joao Martins wrote:
>> Add a new exit reason for emulator to handle Xen hypercalls.
>> Albeit these are injected only if guest has initialized the Xen
>> hypercall page
>
> I've reworked this a little.
>
> I didn't like the inconsistency of allowing userspace to provide the
> hypercall pages even though the ABI is now defined by the kernel and it
> *has* to be VMCALL/VMMCALL.
>
> So I switched it to generate the hypercall page directly from the
> kernel, just like we do for the Hyper-V hypercall page.
>
> I introduced a new flag in the xen_hvm_config struct to enable this
> behaviour, and advertised it in the KVM_CAP_XEN_HVM return value.
>
> I also added the cpl and support for 6-argument hypercalls, and made it
> check the guest RIP when completing the call as discussed (although I
> still think that probably ought to be a generic thing).
>
> I adjusted the test case from my version of the patch, and added
> support for actually testing the hypercall page MSR.
>
> https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/xenpv
>
> I'll go through and rebase your patch series at least up to patch 16
> and collect them in that tree, then probably post them for real once
> I've got everything working locally.
>
>
> =======================
> From c037c329c8867b219afe2100e383c62e9db7b06d Mon Sep 17 00:00:00 2001
> From: Joao Martins <[email protected]>
> Date: Wed, 13 Jun 2018 09:55:44 -0400
> Subject: [PATCH] KVM: x86/xen: intercept xen hypercalls if enabled
>
> Add a new exit reason for emulator to handle Xen hypercalls.
>
> Since this means KVM owns the ABI, dispense with the facility for the
> VMM to provide its own copy of the hypercall pages; just fill them in
> directly using VMCALL/VMMCALL as we do for the Hyper-V hypercall page.
>
> This behaviour is enabled by a new INTERCEPT_HCALL flag in the
> KVM_XEN_HVM_CONFIG ioctl structure, and advertised by the same flag
> being returned from the KVM_CAP_XEN_HVM check.
>
> Add a test case and shift xen_hvm_config() to the nascent xen.c while
> we're at it.
>
> Signed-off-by: Joao Martins <[email protected]>
> Signed-off-by: David Woodhouse <[email protected]>
> ---
> arch/x86/include/asm/kvm_host.h | 6 +
> arch/x86/kvm/Makefile | 2 +-
> arch/x86/kvm/trace.h | 36 +++++
> arch/x86/kvm/x86.c | 46 +++---
> arch/x86/kvm/xen.c | 140 ++++++++++++++++++
> arch/x86/kvm/xen.h | 21 +++
> include/uapi/linux/kvm.h | 19 +++
> tools/testing/selftests/kvm/Makefile | 1 +
> tools/testing/selftests/kvm/lib/kvm_util.c | 1 +
> .../selftests/kvm/x86_64/xen_vmcall_test.c | 123 +++++++++++++++
> 10 files changed, 365 insertions(+), 30 deletions(-)
> create mode 100644 arch/x86/kvm/xen.c
> create mode 100644 arch/x86/kvm/xen.h
> create mode 100644 tools/testing/selftests/kvm/x86_64/xen_vmcall_test.c
>

> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> new file mode 100644
> index 000000000000..6400a4bc8480
> --- /dev/null
> +++ b/arch/x86/kvm/xen.c
> @@ -0,0 +1,140 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright © 2019 Oracle and/or its affiliates. All rights reserved.
> + * Copyright © 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved.
> + *
> + * KVM Xen emulation
> + */
> +
> +#include "x86.h"
> +#include "xen.h"
> +
> +#include <linux/kvm_host.h>
> +
> +#include <trace/events/kvm.h>
> +
> +#include "trace.h"
> +
> +int kvm_xen_hvm_config(struct kvm_vcpu *vcpu, u64 data)
> + {
> + struct kvm *kvm = vcpu->kvm;
> + u32 page_num = data & ~PAGE_MASK;
> + u64 page_addr = data & PAGE_MASK;
> +
> + /*
> + * If Xen hypercall intercept is enabled, fill the hypercall
> + * page with VMCALL/VMMCALL instructions since that's what
> + * we catch. Else the VMM has provided the hypercall pages
> + * with instructions of its own choosing, so use those.
> + */
> + if (kvm_xen_hypercall_enabled(kvm)) {
> + u8 instructions[32];
> + int i;
> +
> + if (page_num)
> + return 1;
> +
> + /* mov imm32, %eax */
> + instructions[0] = 0xb8;
> +
> + /* vmcall / vmmcall */
> + kvm_x86_ops.patch_hypercall(vcpu, instructions + 5);
> +
> + /* ret */
> + instructions[8] = 0xc3;
> +
> + /* int3 to pad */
> + memset(instructions + 9, 0xcc, sizeof(instructions) - 9);
> +
> + for (i = 0; i < PAGE_SIZE / sizeof(instructions); i++) {
> + *(u32 *)&instructions[1] = i;
> + if (kvm_vcpu_write_guest(vcpu,
> + page_addr + (i * sizeof(instructions)),
> + instructions, sizeof(instructions)))
> + return 1;
> + }

HYPERVISOR_iret isn't supported on 64bit so should be ud2 instead.

Ankur

> + } else {
> + int lm = is_long_mode(vcpu);
> + u8 *blob_addr = lm ? (u8 *)(long)kvm->arch.xen_hvm_config.blob_addr_64
> + : (u8 *)(long)kvm->arch.xen_hvm_config.blob_addr_32;
> + u8 blob_size = lm ? kvm->arch.xen_hvm_config.blob_size_64
> + : kvm->arch.xen_hvm_config.blob_size_32;
> + u8 *page;
> +
> + if (page_num >= blob_size)
> + return 1;
> +
> + page = memdup_user(blob_addr + (page_num * PAGE_SIZE), PAGE_SIZE);
> + if (IS_ERR(page))
> + return PTR_ERR(page);
> +
> + if (kvm_vcpu_write_guest(vcpu, page_addr, page, PAGE_SIZE)) {
> + kfree(page);
> + return 1;
> + }
> + }
> + return 0;
> +}
> +
> +static int kvm_xen_hypercall_set_result(struct kvm_vcpu *vcpu, u64 result)
> +{
> + kvm_rax_write(vcpu, result);
> + return kvm_skip_emulated_instruction(vcpu);
> +}
> +
> +static int kvm_xen_hypercall_complete_userspace(struct kvm_vcpu *vcpu)
> +{
> + struct kvm_run *run = vcpu->run;
> +
> + if (unlikely(!kvm_is_linear_rip(vcpu, vcpu->arch.xen.hypercall_rip)))
> + return 1;
> +
> + return kvm_xen_hypercall_set_result(vcpu, run->xen.u.hcall.result);
> +}
> +
> +int kvm_xen_hypercall(struct kvm_vcpu *vcpu)
> +{
> + bool longmode;
> + u64 input, params[6];
> +
> + input = (u64)kvm_register_read(vcpu, VCPU_REGS_RAX);
> +
> + longmode = is_64_bit_mode(vcpu);
> + if (!longmode) {
> + params[0] = (u32)kvm_rbx_read(vcpu);
> + params[1] = (u32)kvm_rcx_read(vcpu);
> + params[2] = (u32)kvm_rdx_read(vcpu);
> + params[3] = (u32)kvm_rsi_read(vcpu);
> + params[4] = (u32)kvm_rdi_read(vcpu);
> + params[5] = (u32)kvm_rbp_read(vcpu);
> + }
> +#ifdef CONFIG_X86_64
> + else {
> + params[0] = (u64)kvm_rdi_read(vcpu);
> + params[1] = (u64)kvm_rsi_read(vcpu);
> + params[2] = (u64)kvm_rdx_read(vcpu);
> + params[3] = (u64)kvm_r10_read(vcpu);
> + params[4] = (u64)kvm_r8_read(vcpu);
> + params[5] = (u64)kvm_r9_read(vcpu);
> + }
> +#endif
> + trace_kvm_xen_hypercall(input, params[0], params[1], params[2],
> + params[3], params[4], params[5]);
> +
> + vcpu->run->exit_reason = KVM_EXIT_XEN;
> + vcpu->run->xen.type = KVM_EXIT_XEN_HCALL;
> + vcpu->run->xen.u.hcall.longmode = longmode;
> + vcpu->run->xen.u.hcall.cpl = kvm_x86_ops.get_cpl(vcpu);
> + vcpu->run->xen.u.hcall.input = input;
> + vcpu->run->xen.u.hcall.params[0] = params[0];
> + vcpu->run->xen.u.hcall.params[1] = params[1];
> + vcpu->run->xen.u.hcall.params[2] = params[2];
> + vcpu->run->xen.u.hcall.params[3] = params[3];
> + vcpu->run->xen.u.hcall.params[4] = params[4];
> + vcpu->run->xen.u.hcall.params[5] = params[5];
> + vcpu->arch.xen.hypercall_rip = kvm_get_linear_rip(vcpu);
> + vcpu->arch.complete_userspace_io =
> + kvm_xen_hypercall_complete_userspace;
> +
> + return 0;
> +}
> diff --git a/arch/x86/kvm/xen.h b/arch/x86/kvm/xen.h
> new file mode 100644
> index 000000000000..81e12f716d2e
> --- /dev/null
> +++ b/arch/x86/kvm/xen.h
> @@ -0,0 +1,21 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright © 2019 Oracle and/or its affiliates. All rights reserved.
> + * Copyright © 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved.
> + *
> + * KVM Xen emulation
> + */
> +
> +#ifndef __ARCH_X86_KVM_XEN_H__
> +#define __ARCH_X86_KVM_XEN_H__
> +
> +int kvm_xen_hypercall(struct kvm_vcpu *vcpu);
> +int kvm_xen_hvm_config(struct kvm_vcpu *vcpu, u64 data);
> +
> +static inline bool kvm_xen_hypercall_enabled(struct kvm *kvm)
> +{
> + return kvm->arch.xen_hvm_config.flags &
> + KVM_XEN_HVM_CONFIG_INTERCEPT_HCALL;
> +}
> +
> +#endif /* __ARCH_X86_KVM_XEN_H__ */
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index ca41220b40b8..00221fe56994 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -216,6 +216,20 @@ struct kvm_hyperv_exit {
> } u;
> };
>
> +struct kvm_xen_exit {
> +#define KVM_EXIT_XEN_HCALL 1
> + __u32 type;
> + union {
> + struct {
> + __u32 longmode;
> + __u32 cpl;
> + __u64 input;
> + __u64 result;
> + __u64 params[6];
> + } hcall;
> + } u;
> +};
> +
> #define KVM_S390_GET_SKEYS_NONE 1
> #define KVM_S390_SKEYS_MAX 1048576
>
> @@ -250,6 +264,7 @@ struct kvm_hyperv_exit {
> #define KVM_EXIT_ARM_NISV 28
> #define KVM_EXIT_X86_RDMSR 29
> #define KVM_EXIT_X86_WRMSR 30
> +#define KVM_EXIT_XEN 31
>
> /* For KVM_EXIT_INTERNAL_ERROR */
> /* Emulate instruction failed. */
> @@ -426,6 +441,8 @@ struct kvm_run {
> __u32 index; /* kernel -> user */
> __u64 data; /* kernel <-> user */
> } msr;
> + /* KVM_EXIT_XEN */
> + struct kvm_xen_exit xen;
> /* Fix the size of the union. */
> char padding[256];
> };
> @@ -1126,6 +1143,8 @@ struct kvm_x86_mce {
> #endif
>
> #ifdef KVM_CAP_XEN_HVM
> +#define KVM_XEN_HVM_CONFIG_INTERCEPT_HCALL (1 << 1)
> +
> struct kvm_xen_hvm_config {
> __u32 flags;
> __u32 msr;
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index 3d14ef77755e..d94abec627e6 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -59,6 +59,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/xss_msr_test
> TEST_GEN_PROGS_x86_64 += x86_64/debug_regs
> TEST_GEN_PROGS_x86_64 += x86_64/tsc_msrs_test
> TEST_GEN_PROGS_x86_64 += x86_64/user_msr_test
> +TEST_GEN_PROGS_x86_64 += x86_64/xen_vmcall_test
> TEST_GEN_PROGS_x86_64 += demand_paging_test
> TEST_GEN_PROGS_x86_64 += dirty_log_test
> TEST_GEN_PROGS_x86_64 += dirty_log_perf_test
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 126c6727a6b0..6e96ae47d28c 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -1654,6 +1654,7 @@ static struct exit_reason {
> {KVM_EXIT_INTERNAL_ERROR, "INTERNAL_ERROR"},
> {KVM_EXIT_OSI, "OSI"},
> {KVM_EXIT_PAPR_HCALL, "PAPR_HCALL"},
> + {KVM_EXIT_XEN, "XEN"},
> #ifdef KVM_EXIT_MEMORY_NOT_PRESENT
> {KVM_EXIT_MEMORY_NOT_PRESENT, "MEMORY_NOT_PRESENT"},
> #endif
> diff --git a/tools/testing/selftests/kvm/x86_64/xen_vmcall_test.c b/tools/testing/selftests/kvm/x86_64/xen_vmcall_test.c
> new file mode 100644
> index 000000000000..3f1dd93626e5
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86_64/xen_vmcall_test.c
> @@ -0,0 +1,123 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * svm_vmcall_test
> + *
> + * Copyright © 2020 Amazon.com, Inc. or its affiliates.
> + *
> + * Userspace hypercall testing
> + */
> +
> +#include "test_util.h"
> +#include "kvm_util.h"
> +#include "processor.h"
> +
> +#define VCPU_ID 5
> +
> +#define HCALL_REGION_GPA 0xc0000000ULL
> +#define HCALL_REGION_SLOT 10
> +
> +static struct kvm_vm *vm;
> +
> +#define INPUTVALUE 17
> +#define ARGVALUE(x) (0xdeadbeef5a5a0000UL + x)
> +#define RETVALUE 0xcafef00dfbfbffffUL
> +
> +#define XEN_HYPERCALL_MSR 0x40000000
> +
> +static void guest_code(void)
> +{
> + unsigned long rax = INPUTVALUE;
> + unsigned long rdi = ARGVALUE(1);
> + unsigned long rsi = ARGVALUE(2);
> + unsigned long rdx = ARGVALUE(3);
> + register unsigned long r10 __asm__("r10") = ARGVALUE(4);
> + register unsigned long r8 __asm__("r8") = ARGVALUE(5);
> + register unsigned long r9 __asm__("r9") = ARGVALUE(6);
> +
> + /* First a direct invocation of 'vmcall' */
> + __asm__ __volatile__("vmcall" :
> + "=a"(rax) :
> + "a"(rax), "D"(rdi), "S"(rsi), "d"(rdx),
> + "r"(r10), "r"(r8), "r"(r9));
> + GUEST_ASSERT(rax == RETVALUE);
> +
> + /* Now fill in the hypercall page */
> + __asm__ __volatile__("wrmsr" : : "c" (XEN_HYPERCALL_MSR),
> + "a" (HCALL_REGION_GPA & 0xffffffff),
> + "d" (HCALL_REGION_GPA >> 32));
> +
> + /* And invoke the same hypercall that way */
> + __asm__ __volatile__("call *%1" : "=a"(rax) :
> + "r"(HCALL_REGION_GPA + INPUTVALUE * 32),
> + "a"(rax), "D"(rdi), "S"(rsi), "d"(rdx),
> + "r"(r10), "r"(r8), "r"(r9));
> + GUEST_ASSERT(rax == RETVALUE);
> +
> + GUEST_DONE();
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + if (!(kvm_check_cap(KVM_CAP_XEN_HVM) &
> + KVM_XEN_HVM_CONFIG_INTERCEPT_HCALL) ) {
> + print_skip("KVM_XEN_HVM_CONFIG_INTERCEPT_HCALL not available");
> + exit(KSFT_SKIP);
> + }
> +
> + vm = vm_create_default(VCPU_ID, 0, (void *) guest_code);
> + vcpu_set_cpuid(vm, VCPU_ID, kvm_get_supported_cpuid());
> +
> + struct kvm_xen_hvm_config hvmc = {
> + .flags = KVM_XEN_HVM_CONFIG_INTERCEPT_HCALL,
> + .msr = XEN_HYPERCALL_MSR,
> + };
> + vm_ioctl(vm, KVM_XEN_HVM_CONFIG, &hvmc);
> +
> + /* Map a region for the hypercall page */
> + vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS,
> + HCALL_REGION_GPA, HCALL_REGION_SLOT,
> + getpagesize(), 0);
> + virt_map(vm, HCALL_REGION_GPA, HCALL_REGION_GPA, 1, 0);
> +
> + for (;;) {
> + volatile struct kvm_run *run = vcpu_state(vm, VCPU_ID);
> + struct ucall uc;
> +
> + vcpu_run(vm, VCPU_ID);
> +
> + if (run->exit_reason == KVM_EXIT_XEN) {
> + ASSERT_EQ(run->xen.type, KVM_EXIT_XEN_HCALL);
> + ASSERT_EQ(run->xen.u.hcall.cpl, 0);
> + ASSERT_EQ(run->xen.u.hcall.longmode, 1);
> + ASSERT_EQ(run->xen.u.hcall.input, INPUTVALUE);
> + ASSERT_EQ(run->xen.u.hcall.params[0], ARGVALUE(1));
> + ASSERT_EQ(run->xen.u.hcall.params[1], ARGVALUE(2));
> + ASSERT_EQ(run->xen.u.hcall.params[2], ARGVALUE(3));
> + ASSERT_EQ(run->xen.u.hcall.params[3], ARGVALUE(4));
> + ASSERT_EQ(run->xen.u.hcall.params[4], ARGVALUE(5));
> + ASSERT_EQ(run->xen.u.hcall.params[5], ARGVALUE(6));
> + run->xen.u.hcall.result = RETVALUE;
> + continue;
> + }
> +
> + TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
> + "Got exit_reason other than KVM_EXIT_IO: %u (%s)\n",
> + run->exit_reason,
> + exit_reason_str(run->exit_reason));
> +
> + switch (get_ucall(vm, VCPU_ID, &uc)) {
> + case UCALL_ABORT:
> + TEST_FAIL("%s", (const char *)uc.args[0]);
> + /* NOT REACHED */
> + case UCALL_SYNC:
> + break;
> + case UCALL_DONE:
> + goto done;
> + default:
> + TEST_FAIL("Unknown ucall 0x%lx.", uc.cmd);
> + }
> + }
> +done:
> + kvm_vm_free(vm);
> + return 0;
> +}
>

2020-12-02 05:24:38

by Ankur Arora

[permalink] [raw]

Subject: Re: [PATCH RFC 03/39] KVM: x86/xen: register shared_info page

On 2020-12-01 5:26 p.m., David Woodhouse wrote
> On Tue, 2020-12-01 at 16:40 -0800, Ankur Arora wrote:
>> On 2020-12-01 5:07 a.m., David Woodhouse wrote:
>>> On Wed, 2019-02-20 at 20:15 +0000, Joao Martins wrote:
>>>> +static int kvm_xen_shared_info_init(struct kvm *kvm, gfn_t gfn)
>>>> +{
>>>> + struct shared_info *shared_info;
>>>> + struct page *page;
>>>> +
>>>> + page = gfn_to_page(kvm, gfn);
>>>> + if (is_error_page(page))
>>>> + return -EINVAL;
>>>> +
>>>> + kvm->arch.xen.shinfo_addr = gfn;
>>>> +
>>>> + shared_info = page_to_virt(page);
>>>> + memset(shared_info, 0, sizeof(struct shared_info));
>>>> + kvm->arch.xen.shinfo = shared_info;
>>>> + return 0;
>>>> +}
>>>> +
>>>
>>> Hm.
>>>
>>> How come we get to pin the page and directly dereference it every time,
>>> while kvm_setup_pvclock_page() has to use kvm_write_guest_cached()
>>> instead?
>>
>> So looking at my WIP trees from the time, this is something that
>> we went back and forth on as well with using just a pinned page or a
>> persistent kvm_vcpu_map().
>
> OK, thanks.
>
>> I remember distinguishing shared_info/vcpu_info from kvm_setup_pvclock_page()
>> as shared_info is created early and is not expected to change during the
>> lifetime of the guest which didn't seem true for MSR_KVM_SYSTEM_TIME (or
>> MSR_KVM_STEAL_TIME) so that would either need to do a kvm_vcpu_map()
>> kvm_vcpu_unmap() dance or do some kind of synchronization.
>>
>> That said, I don't think this code explicitly disallows any updates
>> to shared_info.
>
> It needs to allow updates as well as disabling the shared_info pages.
> We're going to need that to implement SHUTDOWN_soft_reset for kexec.
True.

>
>>>
>>> If that was allowed, wouldn't it have been a much simpler fix for
>>> CVE-2019-3016? What am I missing?
>>
>> Agreed.
>>
>> Perhaps, Paolo can chime in with why KVM never uses pinned page
>> and always prefers to do cached mappings instead?
>>
>>>
>>> Should I rework these to use kvm_write_guest_cached()?
>>
>> kvm_vcpu_map() would be better. The event channel logic does RMW operations
>> on shared_info->vcpu_info.
>
> I've ported the shared_info/vcpu_info parts and made a test case, and
> was going back through to make it use kvm_write_guest_cached(). But I
> should probably plug on through the evtchn bits before I do that.
>
> I also don't see much locking on the cache, and can't convince myself
> that accessing the shared_info page from multiple CPUs with
> kvm_write_guest_cached() or kvm_map_gfn() is sane unless they each have
> their own cache.

I think you could get a VCPU specific cache with kvm_vcpu_map().

>
> What I have so far is at
>
> https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/xenpv

Thanks. Will take a look there.

Ankur

>
> I'll do the event channel support tomorrow and hook it up to my actual
> VMM to give it some more serious testing.
>

2020-12-02 08:07:09

On Wed, 2019-02-20 at 20:15 +0000, Joao Martins wrote:
> @@ -176,6 +177,9 @@ int kvm_arch_set_irq_inatomic(struct kvm_kernel_irq_routing_entry *e,
> int r;
>
> switch (e->type) {
> + case KVM_IRQ_ROUTING_XEN_EVTCHN:
> + return kvm_xen_set_evtchn(e, kvm, irq_source_id, level,
> + line_status);
> case KVM_IRQ_ROUTING_HV_SINT:
> return kvm_hv_set_sint(e, kvm, irq_source_id, level,
> line_status);
> @@ -325,6 +329,13 @@ int kvm_set_routing_entry(struct kvm *kvm,
> e->hv_sint.vcpu = ue->u.hv_sint.vcpu;
> e->hv_sint.sint = ue->u.hv_sint.sint;
> break;
> + case KVM_IRQ_ROUTING_XEN_EVTCHN:
> + e->set = kvm_xen_set_evtchn;
> + e->evtchn.vcpu = ue->u.evtchn.vcpu;
> + e->evtchn.vector = ue->u.evtchn.vector;
> + e->evtchn.via = ue->u.evtchn.via;
> +
> + return kvm_xen_setup_evtchn(kvm, e);
> default:
> return -EINVAL;
> }

Hmm. I'm not sure I've have done it that way.

These IRQ routing entries aren't for individual event channel ports;
they don't map to kvm_xen_evtchn_send().

They actually represent the upcall to the given vCPU when any event
channel is signalled, and it's really per-vCPU configuration.

When the kernel raises (IPI, VIRQ) events on a given CPU, it doesn't
actually use these routing entries; it just uses the values in
vcpu_xen->cb.{via,vector} which were cached from the last of these IRQ
routing entries that happens to have been processed?

The VMM is *expected* to set up precisely one of these for each vCPU,
right? Would it not be better to do that via KVM_XEN_HVM_SET_ATTR?

The usage model for userspace is presumably that the VMM should set the
appropriate bit in evtchn_pending, check evtchn_mask and then call into
the kernel to do the set_irq() to inject the callback vector to the
guest?

I might be more inclined to go for a model where the kernel handles the
evtchn_pending/evtchn_mask for us. What would go into the irq routing
table is { vcpu, port# } which get passed to kvm_xen_evtchn_send().

Does that seem reasonable?

Either way, I do think we need a way for events raised in the kernel to
be signalled to userspace, if they are targeted at a vCPU which has
CALLBACK_VIA_INTX that the kernel can't do directly. So we probably
*do* need that eventfd I was objecting to earlier, except that it's not
a per-evtchn thing; it's per-vCPU.

Attachments:

smime.p7s (5.05 kB)

2020-12-02 11:22:38

by Joao Martins

[permalink] [raw]

Subject: Re: [PATCH RFC 02/39] KVM: x86/xen: intercept xen hypercalls if enabled

On 12/1/20 11:19 AM, David Woodhouse wrote:
> On Tue, 2020-12-01 at 09:48 +0000, David Woodhouse wrote:
>> So I switched it to generate the hypercall page directly from the
>> kernel, just like we do for the Hyper-V hypercall page.
>
> Speaking of Hyper-V... I'll post this one now so you can start heckling
> it.
>
Xen viridian mode is indeed one thing that needed fixing. And definitely a
separate patch as you do here. Both this one and the previous is looking good.

I suppose that because you switch to kvm_vcpu_write_guest() you no longer need
to validate that the hypercall page is correct neither marking as dirty. Probably
worth making that explicit in the commit message.

> I won't post the rest as I go; not much of the rest of the series when
> I eventually post it will be very new and exciting. It'll just be
> rebasing and tweaking your originals and perhaps adding some tests.
>

Awesome!!

2020-12-02 12:16:13

by David Woodhouse

[permalink] [raw]

Subject: Re: [PATCH RFC 02/39] KVM: x86/xen: intercept xen hypercalls if enabled

On Wed, 2020-12-02 at 11:17 +0000, Joao Martins wrote:
> Xen viridian mode is indeed one thing that needed fixing. And definitely a
> separate patch as you do here. Both this one and the previous is looking good.
>
> I suppose that because you switch to kvm_vcpu_write_guest() you no longer need
> to validate that the hypercall page is correct neither marking as dirty. Probably
> worth making that explicit in the commit message.

I had intended that to be covered by "open-coding it".

Surely the *point* in using a helper function and not open-coding it is
that we don't *have* to care about the details of what it does ;)

Attachments:

smime.p7s (5.05 kB)

2020-12-02 12:24:38

On Wed, 2020-12-02 at 13:12 +0000, Joao Martins wrote:
> On 12/2/20 11:17 AM, David Woodhouse wrote:
> > I might be more inclined to go for a model where the kernel handles the
> > evtchn_pending/evtchn_mask for us. What would go into the irq routing
> > table is { vcpu, port# } which get passed to kvm_xen_evtchn_send().
> >
>
> But passing port to the routing and handling the sending of events wouldn't it lead to
> unnecessary handling of event channels which aren't handled by the kernel, compared to
> just injecting caring about the upcall?

Well, I'm generally in favour of *not* doing things in the kernel that
don't need to be there.

But if the kernel is going to short-circuit the IPIs and VIRQs, then
it's already going to have to handle the evtchn_pending/evtchn_mask
bitmaps, and actually injecting interrupts.

Given that it has to have that functionality anyway, it seems saner to
let the kernel have full control over it and to just expose
'evtchn_send' to userspace.

The alternative is to have userspace trying to play along with the
atomic handling of those bitmasks too, and injecting events through
KVM_INTERRUPT/KVM_SIGNAL_MSI in parallel to the kernel doing so. That
seems like *more* complexity, not less.

> I wanted to mention the GSI callback method too, but wasn't enterily sure if eventfd was
> enough.

That actually works quite nicely even for userspace irqchip.

Forgetting Xen for the moment... my model for userspace I/OAPIC with
interrupt remapping is that during normal runtime, the irqfd is
assigned and things all work and we can even have IRQ posting for
eventfds which came from VFIO.

When the IOMMU invalidates an IRQ translation, all it does is
*deassign* the irqfd from the KVM IRQ. So the next time that eventfd
fires, it's caught in the userspace event loop instead. Userspace can
then retranslate through the IOMMU and reassign the irqfd for next
time.

So, back to Xen. As things stand with just the hypercall+shinfo patches
I've already rebased, we have enough to do fully functional Xen
hosting. The event channels are slow but it *can* be done entirely in
userspace — handling *all* the hypercalls, and delivering interrupts to
the guest in whatever mode is required.

Event channels are a very important optimisation though. For the VMM
API I think we should follow the Xen model, mixing the domain-wide and
per-vCPU configuration. It's the best way to faithfully model the
behaviour a true Xen guest would experience.

So KVM_XEN_ATTR_TYPE_CALLBACK_VIA can be used to set one of
• HVMIRQ_callback_vector, taking a vector#
• HVMIRQ_callback_gsi for the in-kernel irqchip, taking a GSI#

And *maybe* in a later patch it could also handle
• HVMIRQ_callback_gsi for split-irqchip, taking an eventfd
• HVMIRQ_callback_pci_intx, taking an eventfd (or a pair, for EOI?)

I don't know if the latter two really make sense. After all the
argument for handling IPI/VIRQ in kernel kind of falls over if we have
to bounce out to userspace anyway. So it *only* makes sense if those
eventfds actually end up wired *through* userspace to a KVM IRQFD as I
described for the IOMMU stuff.

In addition to that per-domain setup, we'd also have a per-vCPU
KVM_XEN_ATTR_TYPE_VCPU_CALLBACK_VECTOR which takes {vCPU, vector}.

Attachments:

smime.p7s (5.05 kB)

2020-12-02 18:23:43

by Ankur Arora

[permalink] [raw]

Subject: Re: [PATCH RFC 02/39] KVM: x86/xen: intercept xen hypercalls if enabled

On 2020-12-02 12:03 a.m., David Woodhouse wrote:
> On Tue, 2020-12-01 at 21:19 -0800, Ankur Arora wrote:
>>> + for (i = 0; i < PAGE_SIZE / sizeof(instructions); i++) {
>>> + *(u32 *)&instructions[1] = i;
>>> + if (kvm_vcpu_write_guest(vcpu,
>>> + page_addr + (i * sizeof(instructions)),
>>> + instructions, sizeof(instructions)))
>>> + return 1;
>>> + }
>>
>> HYPERVISOR_iret isn't supported on 64bit so should be ud2 instead.
>
> Yeah, I got part way through typing that part but concluded it probably
> wasn't a fast path that absolutely needed to be emulated in the kernel.
>
> The VMM can inject the UD# when it receives the hypercall.

That would work as well but if it's a straight ud2 on the hypercall
page, wouldn't the guest just execute it when/if it does a
HYPERVISOR_iret?

Ankur

>
> I appreciate it *is* a guest-visible difference, if we're being really
> pedantic, but I don't think we were even going to be able to 100% hide
> the fact that it's not actually Xen.
>

2020-12-02 18:37:07

by Joao Martins

[permalink] [raw]

Subject: Re: [PATCH RFC 10/39] KVM: x86/xen: support upcall vector

On 12/2/20 4:47 PM, David Woodhouse wrote:
> On Wed, 2020-12-02 at 13:12 +0000, Joao Martins wrote:
>> On 12/2/20 11:17 AM, David Woodhouse wrote:
>>> I might be more inclined to go for a model where the kernel handles the
>>> evtchn_pending/evtchn_mask for us. What would go into the irq routing
>>> table is { vcpu, port# } which get passed to kvm_xen_evtchn_send().
>>
>> But passing port to the routing and handling the sending of events wouldn't it lead to
>> unnecessary handling of event channels which aren't handled by the kernel, compared to
>> just injecting caring about the upcall?
>
> Well, I'm generally in favour of *not* doing things in the kernel that
> don't need to be there.
>
> But if the kernel is going to short-circuit the IPIs and VIRQs, then
> it's already going to have to handle the evtchn_pending/evtchn_mask
> bitmaps, and actually injecting interrupts.
>
Right. I was trying to point that out in the discussion we had
in next patch. But true be told, more about touting the idea of kernel
knowing if a given event channel is registered for userspace handling,
rather than fully handling the event channel.

I suppose we are able to provide both options to the VMM anyway
i.e. 1) letting them handle it enterily in userspace by intercepting
EVTCHNOP_send, or through the irq route if we want kernel to offload it.

> Given that it has to have that functionality anyway, it seems saner to
> let the kernel have full control over it and to just expose
> 'evtchn_send' to userspace.
>
> The alternative is to have userspace trying to play along with the
> atomic handling of those bitmasks too

That part is not particularly hard -- having it done already.

>, and injecting events through
> KVM_INTERRUPT/KVM_SIGNAL_MSI in parallel to the kernel doing so. That
> seems like *more* complexity, not less.
>
/me nods

>> I wanted to mention the GSI callback method too, but wasn't enterily sure if eventfd was
>> enough.
>
> That actually works quite nicely even for userspace irqchip.
>
> Forgetting Xen for the moment... my model for userspace I/OAPIC with
> interrupt remapping is that during normal runtime, the irqfd is
> assigned and things all work and we can even have IRQ posting for
> eventfds which came from VFIO.
>
> When the IOMMU invalidates an IRQ translation, all it does is
> *deassign* the irqfd from the KVM IRQ. So the next time that eventfd
> fires, it's caught in the userspace event loop instead. Userspace can
> then retranslate through the IOMMU and reassign the irqfd for next
> time.
>
> So, back to Xen. As things stand with just the hypercall+shinfo patches
> I've already rebased, we have enough to do fully functional Xen
> hosting.

Yes -- the rest become optimizations in performance sensitive paths.

TBH (and this is slightly offtopic) the somewhat hairy part is xenbus/xenstore.
And the alternative to playing nice with xenstored, is the VMM learning
to parse the xenbus messages and fake the xenstored content/transactions stuff
individually per per-VM .

> The event channels are slow but it *can* be done entirely in

While consulting my old notes, about twice as slow if done in userspace.

> userspace — handling *all* the hypercalls, and delivering interrupts to
> the guest in whatever mode is required.
>
> Event channels are a very important optimisation though.

/me nods

> For the VMM
> API I think we should follow the Xen model, mixing the domain-wide and
> per-vCPU configuration. It's the best way to faithfully model the
> behaviour a true Xen guest would experience.
>
> So KVM_XEN_ATTR_TYPE_CALLBACK_VIA can be used to set one of
> • HVMIRQ_callback_vector, taking a vector#
> • HVMIRQ_callback_gsi for the in-kernel irqchip, taking a GSI#
>
> And *maybe* in a later patch it could also handle
> • HVMIRQ_callback_gsi for split-irqchip, taking an eventfd
> • HVMIRQ_callback_pci_intx, taking an eventfd (or a pair, for EOI?)
>
Most of the Xen versions we were caring had callback_vector and
vcpu callback vector (despite Linux not using the latter). But if you're
dating back to 3.2 and 4.1 well (or certain Windows drivers), I suppose
gsi and pci-intx are must-haves.

> I don't know if the latter two really make sense. After all the
> argument for handling IPI/VIRQ in kernel kind of falls over if we have
> to bounce out to userspace anyway.

Might as well let userspace EVTCHNOP_send handle it, I wonder.

> So it *only* makes sense if those
> eventfds actually end up wired *through* userspace to a KVM IRQFD as I
> described for the IOMMU stuff.
>
We didn't implement the phy event channels, but for most old kernels we
tested (back to 2.6.XX) at the time, seemed to play along.

> In addition to that per-domain setup, we'd also have a per-vCPU
> KVM_XEN_ATTR_TYPE_VCPU_CALLBACK_VECTOR which takes {vCPU, vector}.
>
I feel we could just accommodate it as subtype in KVM_XEN_ATTR_TYPE_CALLBACK_VIA.
Don't see the adavantage in having another xen attr type.

But kinda have mixed feelings in having kernel handling all event channels ABI,
as opposed to only the ones userspace asked to offload. It looks a tad unncessary besides
the added gain to VMMs that don't need to care about how the internals of event channels.
But performance-wise it wouldn't bring anything better. But maybe, the former is reason
enough to consider it.

Joao

2020-12-02 19:06:14

by David Woodhouse

[permalink] [raw]

Subject: Re: [PATCH RFC 10/39] KVM: x86/xen: support upcall vector

On 9 December 2020 13:26:55 GMT, Joao Martins <[email protected]> wrote:
>On 12/9/20 11:39 AM, David Woodhouse wrote:
>> On Wed, 2020-12-09 at 10:51 +0000, Joao Martins wrote:
>>> Isn't this what the first half of this patch was doing initially
>(minus the
>>> irq routing) ? Looks really similar:
>>>
>>>
>https://lore.kernel.org/kvm/[email protected]/
>>
>> Absolutely! This thread is in reply to your original posting of
>> precisely that patch, and I've had your tree open in gitk to crib
>from
>> for most of the last week.
>>
>I forgot about this patch given all the discussion so far and I had to
>re-look given that
>it resembled me from your snippet. But I ended up being a little
>pedantic -- sorry about that.

Nah, pedantry is good :)

>> At most, we just need to make sure that kvm_xen_has_interrupt()
>returns
>> false if the per-vCPU LAPIC vector is configured. But I didn't do
>that
>> because I checked Xen and it doesn't do it either.
>>
>Oh! I have this strange recollection that it was, when we were looking
>at the Xen
>implementation.

Hm, maybe I missed it. Will stare at it harder, although looking at Xen code tends to make my brain hurt :)

>> As far as I can tell, Xen's hvm_vcpu_has_pending_irq() will still
>> return the domain-wide vector in preference to the one in the LAPIC,
>if
>> it actually gets invoked.
>
>Only if the callback installed is HVMIRQ_callback_vector IIUC.
>
>Otherwise the vector would be pending like any other LAPIC vector.

Ah, right.

For some reason I had it in my head that you could only set the per-vCPU lapic vector if the domain was set to HVMIRQ_callback_vector. If the domain is set to HVMIRQ_callback_none, that clearly makes more sense.

Still, my patch should do the same as Xen does in the case where a guest does set both, I think.

Faithful compatibility with odd Xen behaviour FTW :)

--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

2020-12-09 20:05:07

by Joao Martins

So... what we have in the kernel already is entirely sufficient to run
real Xen guests so they don't know they're running under KVM — with PV
netback and blkback, Xen timers, etc.

We can even run the PV Xen shim itself, to host PV guests.

There are some pieces we really want to move into the kernel though,
IPIs being top of the list — we really don't want them bouncing out to
the userspace VMM. In-kernel timers would be good, too.

I've finally got the basic 2l event channel delivery working in the
kernel, with a minor detour into fixing nesting UAF bugs. The first
simple use case for that is routing MSIs of assigned PCI devices to Xen
PIRQs. So I'm ready to stare at the patch in $SUBJECT again...

On Mon, 2020-11-30 at 15:08 +0000, Joao Martins wrote:
> On 11/30/20 12:55 PM, David Woodhouse wrote:
> > On Mon, 2020-11-30 at 12:17 +0000, Joao Martins wrote:
> > > But still, eventfd is probably unnecessary complexity when another @type
> > > (XEN_EVTCHN_TYPE_USER) would serve, and then just exiting to userspace
> > > and let it route its evtchn port handling to the its own I/O handling thread.
> >
> > Hmm... so the benefit of the eventfd is that we can wake the I/O thread
> > directly instead of bouncing out to userspace on the vCPU thread only
> > for it to send a signal and return to the guest? Did you ever use that,
> > and it is worth the additional in-kernel code?
> >
> This was my own preemptive optimization to the interface -- it's not worth
> the added code for vIRQ and IPI at this point which is what *for sure* the
> kernel will handle.
>
> > Is there any reason we'd want that for IPI or VIRQ event channels, or
> > can it be only for INTERDOM/UNBOUND event channels which come later?
> >
> /me nods.
>
> No reason to keep that for IPI/vIRQ.
>
> > I'm tempted to leave it out of the first patch, and *maybe* add it back
> > in a later patch, putting it in the union alongside .virq.type.
> >
> >
> > struct kvm_xen_eventfd {
> >
> > #define XEN_EVTCHN_TYPE_VIRQ 0
> > #define XEN_EVTCHN_TYPE_IPI 1
> > __u32 type;
> > __u32 port;
> > __u32 vcpu;
> > - __s32 fd;
> >
> > #define KVM_XEN_EVENTFD_DEASSIGN (1 << 0)
> > #define KVM_XEN_EVENTFD_UPDATE (1 << 1)
> > eentfd_ __u32 flags;
> > union {
> > struct {
> > __u8 type;
> > } virq;
> > + struct {
> > + __s32 eventfd;
> > + } interdom; /* including unbound */
> > __u32 padding[2];
> > };
> > } evtchn;
> >
> > Does that make sense to you?
> >
> Yeap! :)

It turns out there are actually two cases for interdom event channels.
As well as signalling "other domains" (i.e. userspace) they can also be
set up as loopback, so sending an event on one port actually raises an
event on another of that guest's same ports, a bit like an IPI.

So it isn't quite as simple as "if we handle interdom in the kernel it
has an eventfd".

It also means we need to provide both *source* and *target* ports.
Which brings me to the KVM_XEN_EVENTFD_DEASSIGN/KVM_XEN_EVENTFD_UPDATE
flags that you used. I'm not sure I like that API; given that we now
need separate source and target ports can we handle 'deassign' just by
setting the target to zero? (and eventfd to -1) ?

I think your original patch leaked refcounts on the eventfd in
kvm_xen_eventfd_update() because it would call eventfd_ctx_fdget() then
neither used nor released the result?

So I'm thinking of making it just a 'set'. And in fact I'm thinking of
modelling it on the irq routing table by having a single call to
set/update all of them at once?

Any objections?

Attachments:

smime.p7s (5.05 kB)

2022-02-09 10:19:02

On Thu, 2022-02-10 at 12:17 +0000, Joao Martins wrote:
> On 2/8/22 16:17, Woodhouse, David wrote:
> > And then we have the *outbound* events, which the guest can invoke with
> > the EVTCHNOP_send hypercall. Those are either:
> > • IPI, raising the same port# on the guest
> > • Interdomain looped back to a different port# on the guest
> > • Interdomain triggering an eventfd.
> >
>
> /me nods
>
> I am forgetting why you one do this on Xen:
>
> * Interdomain looped back to a different port# on the guest

It's one of the few things we had to fix up when we started running PV
guests in the 'shim' under KVM. I don't know that it actually sends
loopback events via the true Xen (or KVM) host, but it does at least
register them so that the port# is 'reserved' and the host won't
allocate that port for anything else. It does it at least for the
console port.

For the inbound vs. outbound thing.... I did ponder a really simple API
design in which outbound ports are *only* ever associated with an
eventfd, and for IPIs the VMM would be expected to bind those as IRQFD
to an inbound event on the same port#.

You pointed out that it was quite inefficient, but... we already have
complex hacks to bypass the eventfd for posted interrupts when the
source and destination "match", and perhaps we could do something
similar to allow EVTCHNOP_send to deliver directly to a local port#
without having to go through all the eventfd code?

But the implementation of that would end up being awful, *and* the
userspace API isn't even that nice despite being "simple", because it
would force userspace to allocate a whole bunch of eventfds and use
space in the IRQ routing table for them. So it didn't seem worth it.
Let's just let userspace tell us explicitly the vcpu/port/prio instead
of having to jump through hoops to magically work it out from matching
eventfds.

> > In the last case, that eventfd can be set up with IRQFD for direct
> > event channel delivery to a different KVM/Xen guest.
> >
> > I've used your implemention, with an idr for the outbound port# space
> > intercepting EVTCHNOP_send for known ports and only letting userspace
> > see the hypercall if it's for a port# the kernel doesn't know. Looks a
> > bit like
> > https://git.infradead.org/users/dwmw2/linux.git/commitdiff/b4fbc49218a
> >
> >
> >
> > But I *don't* want to do the VIRQ part shown above, "spotting" the VIRQ
> > in that outbound port# space and squirreling the information away into
> > the kvm_vcpu for when we need to deliver a timer event.
> >
> > The VIRQ isn't part of the *outbound* port# space; it isn't a port to
> > which a Xen guest can use EVTCHNOP_send to send an event.
>
> But it is still an event channel which port is unique regardless of port
> type/space hence (...)
>
> > If anything,
> > it would be part of the *inbound* port# space, in the KVM IRQ routing
> > table. So perhaps we could have a similar snippet in
> > kvm_xen_setup_evtchn() which spots a VIRQ and says "aha, now I know
> > where to deliver timer events for this vCPU".
> >
>
> (...) The thinking at the time was mainly simplicity so our way of saying
> 'offload the evtchn to KVM' was through the machinery that offloads the outbound
> part (using your terminology). I don't think even using XEN_EVENTFD as proposed
> here that that one could send an VIRQ via EVTCHNOP_send (I could be wrong as
> it has been a long time).

I confess I didn't test it but it *looked* like you could, while true
Xen wouldn't permit that.

> Regardless, I think you have a good point to split the semantics and (...)

> >
> > So I think I'm going to make the timer VIRQ (port#, priority) into an
> > explicit KVM_XEN_VCPU_ATTR_TYPE.
>
> (...) thus this makes sense. Do you particularly care about
> VIRQ_DEBUG?

Not really. Especially not as something to accelerate in KVM.

Our environment doesn't have any way to deliver that to guests,
although we *do* have an API call to deliver "diagnostic interrupt"
which maps to an NMI, and we *have* occasionally hacked the VMM to
deliver VIRQ_DEBUG to Xen guests instead of that NMI. Mostly back when
I was busy being confused about ->vcpu_id vs. ->vcpu_idx vs. the
Xen/ACPI CPU# and where the hell my interrupts were going.

> > Along with the *actual* timer expiry,
> > which we need to extract/restore for LU/LM too, don't we?
> > /me nods
>
> I haven't thought that one well for Live Update / Live Migration, but
> I wonder if wouldn't be better to be instead a general 'xen state'
> attr type should you need more than just pending timers expiry. Albeit
> considering that the VMM has everything it needs (?), perhaps for Xen PV
> timer look to be the oddball missing, and we donºt need to go that extent.

Yeah, the VMM knows most of this stuff already, as it *told* the kernel
in the first place. Userspace is still responsible for all the setup
and admin, and the kernel just handles the bare minimum of the fast
path.

So on live update/migrate we only really need to read out the runstate
data from the kernel... and now the current timer expiry.

On the *resume* side it's still lots of syscalls, and perhaps in the
end we might decide we want to do a KVM_XEN_HVM_SET_ATTR_MULTI which
takes an array of them? But I think that's a premature optimisation for
now.

Attachments:

smime.p7s (5.83 kB)

2022-02-11 01:39:35

by David Woodhouse

[permalink] [raw]

Subject: Re: [PATCH RFC 15/39] KVM: x86/xen: handle PV spinlocks slowpath

On Thu, 2022-02-10 at 12:17 +0000, Joao Martins wrote:
> On 2/8/22 12:36, David Woodhouse wrote:
> > > + if (!wait_pending_event(vcpu, sched_poll.nr_ports, ports))
> > > + wait_event_interruptible_timeout(
> > > + vcpu_xen->sched_waitq,
> > > + wait_pending_event(vcpu, sched_poll.nr_ports, ports),
> > > + sched_poll.timeout ?: KTIME_MAX);
> >
> > Hm, this doesn't wake on other interrupts, does it?
>
> Hmm, I don't think so? This was specifically polling on event channels,
> not sleeping or blocking.
>
> > I think it should.
> > Shouldn't it basically be like HLT, with an additional wakeup when the
> > listed ports are triggered even when they're masked?
> >
>
> I am actually not sure.
>
> Quickly glancing at the xen source, this hypercall doesn't appear to really
> block the vcpu, but rather just looking if the evtchn ports are pending and
> if a timeout is is specified it sets up a timer. And ofc, wake any evtchn
> pollers. But it doesn't appear to actually block the VCPU. It should be
> IIRC, the functional equivalent of KVM_HC_VAPIC_POLL_IRQ but for event
> channels.
>

It does block.

https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/common/sched/core.c;hb=RELEASE-4.14.4#l1385

It sets the _VPF_blocked bit in the vCPU's pause_flags, then the
raise_softirq(SCHEDULE_SOFTIRQ) causes it to deschedule this vCPU when
it gets back to the return-to-guest path. It's not Linux, so you don't
see a schedule() call :)

It'll remain blocked until either an (unmasked) event channel or HVM
IRQ is asserted, or one of the masked event channels listed in the poll
list is raised.

Which is why I tried to tie it into the kvm_vcpu_halt() code path when
I updated the patch.

Attachments:

smime.p7s (5.83 kB)