Re: [PATCH 1/5] KVM: x86/xen: Restrict hypercall MSR to unofficial synthetic range

From: David Woodhouse
Date: Thu Feb 06 2025 - 04:19:06 EST

On Wed, 2025-02-05 at 07:51 -0800, Sean Christopherson wrote:
>
> > Those two happen in reverse chronological order, don't they? And in the
> > lower one the comment tells you that hyperv_enabled() doesn't work yet.
> > When the higher one is called later, it calls kvm_xen_init() *again* to
> > put the MSR in the right place.
> >
> > It could be prettier, but I don't think it's broken, is it?
>
> Gah, -ENOCOFFEE.

I trust this version would require less coffee to parse...

From a90c2df0bd9589609085dd42f94b61de1bf48eb7 Mon Sep 17 00:00:00 2001
From: David Woodhouse <dwmw@xxxxxxxxxxxx>
Date: Thu, 6 Feb 2025 08:52:52 +0000
Subject: [PATCH] i386/xen: Move KVM_XEN_HVM_CONFIG ioctl to
kvm_xen_init_vcpu()

At the time kvm_xen_init() is called, hyperv_enabled() doesn't yet work, so
the correct MSR index to use for the hypercall page isn't known.

Rather than setting it to the default and then shifting it later for the
Hyper-V case with a confusing second call to kvm_init_xen(), just do it
once in kvm_xen_init_vcpu().

Signed-off-by: David Woodhouse <dwmw@xxxxxxxxxxxx>
---
target/i386/kvm/kvm.c | 16 +++++---------
target/i386/kvm/xen-emu.c | 44 ++++++++++++++++++++-------------------
target/i386/kvm/xen-emu.h | 4 ++--
3 files changed, 30 insertions(+), 34 deletions(-)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 6c749d4ee8..b4deec6255 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -2129,6 +2129,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
if (cs->kvm_state->xen_version) {
#ifdef CONFIG_XEN_EMU
struct kvm_cpuid_entry2 *xen_max_leaf;
+ uint32_t hypercall_msr =
+ hyperv_enabled(cpu) ? XEN_HYPERCALL_MSR_HYPERV : XEN_HYPERCALL_MSR;

memcpy(signature, "XenVMMXenVMM", 12);

@@ -2150,13 +2152,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
c->function = kvm_base + XEN_CPUID_HVM_MSR;
/* Number of hypercall-transfer pages */
c->eax = 1;
- /* Hypercall MSR base address */
- if (hyperv_enabled(cpu)) {
- c->ebx = XEN_HYPERCALL_MSR_HYPERV;
- kvm_xen_init(cs->kvm_state, c->ebx);
- } else {
- c->ebx = XEN_HYPERCALL_MSR;
- }
+ c->ebx = hypercall_msr;
c->ecx = 0;
c->edx = 0;

@@ -2194,7 +2190,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
}
}

- r = kvm_xen_init_vcpu(cs);
+ r = kvm_xen_init_vcpu(cs, hypercall_msr);
if (r) {
return r;
}
@@ -3245,9 +3241,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
error_report("kvm: Xen support only available in PC machine");
return -ENOTSUP;
}
- /* hyperv_enabled() doesn't work yet. */
- uint32_t msr = XEN_HYPERCALL_MSR;
- ret = kvm_xen_init(s, msr);
+ ret = kvm_xen_init(s);
if (ret < 0) {
return ret;
}
diff --git a/target/i386/kvm/xen-emu.c b/target/i386/kvm/xen-emu.c
index e81a245881..1144a6efcd 100644
--- a/target/i386/kvm/xen-emu.c
+++ b/target/i386/kvm/xen-emu.c
@@ -108,15 +108,11 @@ static inline int kvm_copy_to_gva(CPUState *cs, uint64_t gva, void *buf,
return kvm_gva_rw(cs, gva, buf, sz, true);
}

-int kvm_xen_init(KVMState *s, uint32_t hypercall_msr)
+int kvm_xen_init(KVMState *s)
{
const int required_caps = KVM_XEN_HVM_CONFIG_HYPERCALL_MSR |
KVM_XEN_HVM_CONFIG_INTERCEPT_HCALL | KVM_XEN_HVM_CONFIG_SHARED_INFO;
- struct kvm_xen_hvm_config cfg = {
- .msr = hypercall_msr,
- .flags = KVM_XEN_HVM_CONFIG_INTERCEPT_HCALL,
- };
- int xen_caps, ret;
+ int xen_caps;

xen_caps = kvm_check_extension(s, KVM_CAP_XEN_HVM);
if (required_caps & ~xen_caps) {
@@ -130,20 +126,6 @@ int kvm_xen_init(KVMState *s, uint32_t hypercall_msr)
.u.xen_version = s->xen_version,
};
(void)kvm_vm_ioctl(s, KVM_XEN_HVM_SET_ATTR, &ha);
-
- cfg.flags |= KVM_XEN_HVM_CONFIG_EVTCHN_SEND;
- }
-
- ret = kvm_vm_ioctl(s, KVM_XEN_HVM_CONFIG, &cfg);
- if (ret < 0) {
- error_report("kvm: Failed to enable Xen HVM support: %s",
- strerror(-ret));
- return ret;
- }
-
- /* If called a second time, don't repeat the rest of the setup. */
- if (s->xen_caps) {
- return 0;
}

/*
@@ -185,10 +167,14 @@ int kvm_xen_init(KVMState *s, uint32_t hypercall_msr)
return 0;
}

-int kvm_xen_init_vcpu(CPUState *cs)
+int kvm_xen_init_vcpu(CPUState *cs, uint32_t hypercall_msr)
{
X86CPU *cpu = X86_CPU(cs);
CPUX86State *env = &cpu->env;
+ struct kvm_xen_hvm_config cfg = {
+ .msr = hypercall_msr,
+ .flags = KVM_XEN_HVM_CONFIG_INTERCEPT_HCALL,
+ };
int err;

/*
@@ -210,6 +196,22 @@ int kvm_xen_init_vcpu(CPUState *cs)
strerror(-err));
return err;
}
+
+ cfg.flags |= KVM_XEN_HVM_CONFIG_EVTCHN_SEND;
+ }
+
+ /*
+ * This is a per-KVM setting, but hyperv_enabled() can't be used
+ * when kvm_xen_init() is called from kvm_arch_init(), so do it
+ * when the BSP is initialized.
+ */
+ if (cs->cpu_index == 0) {
+ err = kvm_vm_ioctl(cs->kvm_state, KVM_XEN_HVM_CONFIG, &cfg);
+ if (err) {
+ error_report("kvm: Failed to enable Xen HVM support: %s",
+ strerror(-err));
+ return err;
+ }
}

env->xen_vcpu_info_gpa = INVALID_GPA;
diff --git a/target/i386/kvm/xen-emu.h b/target/i386/kvm/xen-emu.h
index fe85e0b195..7a7c72eee5 100644
--- a/target/i386/kvm/xen-emu.h
+++ b/target/i386/kvm/xen-emu.h
@@ -23,8 +23,8 @@

#define XEN_VERSION(maj, min) ((maj) << 16 | (min))

-int kvm_xen_init(KVMState *s, uint32_t hypercall_msr);
-int kvm_xen_init_vcpu(CPUState *cs);
+int kvm_xen_init(KVMState *s);
+int kvm_xen_init_vcpu(CPUState *cs, uint32_t hypercall_msr);
int kvm_xen_handle_exit(X86CPU *cpu, struct kvm_xen_exit *exit);
int kvm_put_xen_state(CPUState *cs);
int kvm_get_xen_state(CPUState *cs);
--
2.48.1


Attachment: smime.p7s
Description: S/MIME cryptographic signature