Re: [RFC PATCH v5 28/52] i386/xen: Add support for Xen event channel delivery to vCPU

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 6046 bytes --]

On Fri, 2022-12-30 at 12:12 +0000, David Woodhouse wrote:
> 
> +static void *gpa_to_hva(uint64_t gpa)
> +{
> +    MemoryRegionSection mrs;
> +
> +    mrs = memory_region_find(get_system_memory(), gpa, 1);
> +    return !mrs.mr ? NULL : qemu_map_ram_ptr(mrs.mr->ram_block,
> +                                             mrs.offset_within_region);
> +}
> +
> +void *kvm_xen_get_vcpu_info_hva(uint32_t vcpu_id)
> +{
> +    CPUState *cs = qemu_get_cpu(vcpu_id);
> +    CPUX86State *env;
> +    uint64_t gpa;
> +
> +    if (!cs) {
> +        return NULL;
> +    }
> +    env = &X86_CPU(cs)->env;
> +
> +    gpa = env->xen_vcpu_info_gpa;
> +    if (gpa == INVALID_GPA) {
> +        gpa = env->xen_vcpu_info_default_gpa;
> +    }
> +    if (gpa == INVALID_GPA) {
> +        return NULL;
> +    }
> +
> +    return gpa_to_hva(gpa);
> +}

Hm, no. That leaks a refcount on the MemoryRegion each time, doesn't
it?

Fixing it up something like this, by caching the HVA (and the
MemoryRegion so we can unref it later)... 

I'll send the incremental patch to allow for specific review without it
getting lost in the noise:

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 1a411e1b49..b579f0f0f8 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1791,6 +1791,8 @@ typedef struct CPUArchState {
 #endif
 #if defined(CONFIG_KVM)
     struct kvm_nested_state *nested_state;
+    MemoryRegion *xen_vcpu_info_mr;
+    void *xen_vcpu_info_hva;
     uint64_t xen_vcpu_info_gpa;
     uint64_t xen_vcpu_info_default_gpa;
     uint64_t xen_vcpu_time_info_gpa;
diff --git a/target/i386/kvm/xen-emu.c b/target/i386/kvm/xen-emu.c
index 6c0f180059..aa2c00e0b9 100644
--- a/target/i386/kvm/xen-emu.c
+++ b/target/i386/kvm/xen-emu.c
@@ -320,6 +320,35 @@ static void do_set_vcpu_callback_vector(CPUState *cs, run_on_cpu_data data)
     }
 }
 
+static int set_vcpu_info(CPUState *cs, uint64_t gpa)
+{
+    X86CPU *cpu = X86_CPU(cs);
+    CPUX86State *env = &cpu->env;
+    MemoryRegionSection mrs;
+    int ret;
+
+    ret = kvm_xen_set_vcpu_attr(cs, KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO, gpa);
+    if (ret || gpa == INVALID_GPA) {
+    fail:
+        if (env->xen_vcpu_info_mr) {
+            memory_region_unref(env->xen_vcpu_info_mr);
+            env->xen_vcpu_info_mr = NULL;
+        }
+        env->xen_vcpu_info_hva = NULL;
+        return ret;
+    }
+
+    mrs = memory_region_find(get_system_memory(), gpa, sizeof(struct vcpu_info));
+    if (!mrs.mr || !mrs.mr->ram_block || mrs.size < sizeof(struct vcpu_info) ||
+        !(env->xen_vcpu_info_hva = qemu_map_ram_ptr(mrs.mr->ram_block,
+                                                    mrs.offset_within_region))) {
+        ret = -EINVAL;
+        goto fail;
+    }
+    env->xen_vcpu_info_mr = mrs.mr;
+    return 0;
+}
+
 static void do_set_vcpu_info_default_gpa(CPUState *cs, run_on_cpu_data data)
 {
     X86CPU *cpu = X86_CPU(cs);
@@ -329,8 +358,7 @@ static void do_set_vcpu_info_default_gpa(CPUState *cs, run_on_cpu_data data)
 
     /* Changing the default does nothing if a vcpu_info was explicitly set. */
     if (env->xen_vcpu_info_gpa == INVALID_GPA) {
-        kvm_xen_set_vcpu_attr(cs, KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO,
-                              env->xen_vcpu_info_default_gpa);
+        set_vcpu_info(cs, env->xen_vcpu_info_default_gpa);
     }
 }
 
@@ -341,55 +369,30 @@ static void do_set_vcpu_info_gpa(CPUState *cs, run_on_cpu_data data)
 
     env->xen_vcpu_info_gpa = data.host_ulong;
 
-    kvm_xen_set_vcpu_attr(cs, KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO,
-                          env->xen_vcpu_info_gpa);
-}
-
-static void *gpa_to_hva(uint64_t gpa)
-{
-    MemoryRegionSection mrs;
-
-    mrs = memory_region_find(get_system_memory(), gpa, 1);
-    return !mrs.mr ? NULL : qemu_map_ram_ptr(mrs.mr->ram_block,
-                                             mrs.offset_within_region);
-}
-
-static void *vcpu_info_hva_from_cs(CPUState *cs)
-{
-    CPUX86State *env = &X86_CPU(cs)->env;
-    uint64_t gpa = env->xen_vcpu_info_gpa;
-
-    if (gpa == INVALID_GPA) {
-        gpa = env->xen_vcpu_info_default_gpa;
-    }
-    if (gpa == INVALID_GPA) {
-        return NULL;
-    }
-
-    return gpa_to_hva(gpa);
+    set_vcpu_info(cs, env->xen_vcpu_info_gpa);
 }
 
 void *kvm_xen_get_vcpu_info_hva(uint32_t vcpu_id)
 {
     CPUState *cs = qemu_get_cpu(vcpu_id);
-
     if (!cs) {
             return NULL;
     }
 
-    return vcpu_info_hva_from_cs(cs);
+    return X86_CPU(cs)->env.xen_vcpu_info_hva;
 }
 
 void kvm_xen_maybe_deassert_callback(CPUState *cs)
 {
-    struct vcpu_info *vi = vcpu_info_hva_from_cs(cs);
+    CPUX86State *env = &X86_CPU(cs)->env;
+    struct vcpu_info *vi = env->xen_vcpu_info_hva;
     if (!vi) {
             return;
     }
 
     /* If the evtchn_upcall_pending flag is cleared, turn the GSI off. */
     if (!vi->evtchn_upcall_pending) {
-        X86_CPU(cs)->env.xen_callback_asserted = false;
+        env->xen_callback_asserted = false;
         xen_evtchn_set_callback_level(0);
     }
 }
@@ -519,7 +522,7 @@ static void do_vcpu_soft_reset(CPUState *cs, run_on_cpu_data data)
     env->xen_singleshot_timer_ns = 0;
     memset(env->xen_virq, 0, sizeof(env->xen_virq));
 
-    kvm_xen_set_vcpu_attr(cs, KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO, INVALID_GPA);
+    set_vcpu_info(cs, INVALID_GPA);
     kvm_xen_set_vcpu_attr(cs, KVM_XEN_VCPU_ATTR_TYPE_VCPU_TIME_INFO,
                           INVALID_GPA);
     kvm_xen_set_vcpu_attr(cs, KVM_XEN_VCPU_ATTR_TYPE_RUNSTATE_ADDR,
@@ -1572,7 +1575,7 @@ int kvm_put_xen_state(CPUState *cs)
     }
 
     if (gpa != INVALID_GPA) {
-        ret = kvm_xen_set_vcpu_attr(cs, KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO, gpa);
+        ret = set_vcpu_info(cs, gpa);
         if (ret < 0) {
             return ret;
         }


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]