From: David Woodhouse <dwmw@amazon.co.uk>

Include basic support for setting HVM_PARAM_CALLBACK_IRQ to the global
vector method HVM_PARAM_CALLBACK_TYPE_VECTOR, which is handled in-kernel
by raising the vector whenever the vCPU's vcpu_info->evtchn_upcall_pending
flag is set.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 hw/i386/kvm/meson.build   |   5 +-
 hw/i386/kvm/xen_evtchn.c  | 148 ++++++++++++++++++++++++++++++++++++++
 hw/i386/kvm/xen_evtchn.h  |  18 +++++
 hw/i386/pc.c              |   2 +
 target/i386/kvm/xen-emu.c |  10 +++
 5 files changed, 182 insertions(+), 1 deletion(-)
 create mode 100644 hw/i386/kvm/xen_evtchn.c
 create mode 100644 hw/i386/kvm/xen_evtchn.h

diff --git a/hw/i386/kvm/meson.build b/hw/i386/kvm/meson.build
index 6165cbf019..cab64df339 100644
--- a/hw/i386/kvm/meson.build
+++ b/hw/i386/kvm/meson.build
@@ -4,6 +4,9 @@ i386_kvm_ss.add(when: 'CONFIG_APIC', if_true: files('apic.c'))
 i386_kvm_ss.add(when: 'CONFIG_I8254', if_true: files('i8254.c'))
 i386_kvm_ss.add(when: 'CONFIG_I8259', if_true: files('i8259.c'))
 i386_kvm_ss.add(when: 'CONFIG_IOAPIC', if_true: files('ioapic.c'))
-i386_kvm_ss.add(when: 'CONFIG_XEN_EMU', if_true: files('xen_overlay.c'))
+i386_kvm_ss.add(when: 'CONFIG_XEN_EMU', if_true: files(
+  'xen_overlay.c',
+  'xen_evtchn.c',
+  ))
 
 i386_ss.add_all(when: 'CONFIG_KVM', if_true: i386_kvm_ss)
diff --git a/hw/i386/kvm/xen_evtchn.c b/hw/i386/kvm/xen_evtchn.c
new file mode 100644
index 0000000000..018f4ef4da
--- /dev/null
+++ b/hw/i386/kvm/xen_evtchn.c
@@ -0,0 +1,148 @@
+/*
+ * QEMU Xen emulation: Event channel support
+ *
+ * Copyright © 2022 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * Authors: David Woodhouse <dwmw2@infradead.org>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/host-utils.h"
+#include "qemu/module.h"
+#include "qemu/main-loop.h"
+#include "qapi/error.h"
+#include "qom/object.h"
+#include "exec/target_page.h"
+#include "exec/address-spaces.h"
+#include "migration/vmstate.h"
+
+#include "hw/sysbus.h"
+#include "hw/xen/xen.h"
+#include "xen_evtchn.h"
+
+#include "sysemu/kvm.h"
+#include "sysemu/kvm_xen.h"
+#include <linux/kvm.h>
+
+#include "standard-headers/xen/memory.h"
+#include "standard-headers/xen/hvm/params.h"
+
+#define TYPE_XEN_EVTCHN "xen-evtchn"
+OBJECT_DECLARE_SIMPLE_TYPE(XenEvtchnState, XEN_EVTCHN)
+
+struct XenEvtchnState {
+    /*< private >*/
+    SysBusDevice busdev;
+    /*< public >*/
+
+    uint64_t callback_param;
+    bool evtchn_in_kernel;
+
+    QemuMutex port_lock;
+};
+
+struct XenEvtchnState *xen_evtchn_singleton;
+
+/* Top bits of callback_param are the type (HVM_PARAM_CALLBACK_TYPE_xxx) */
+#define CALLBACK_VIA_TYPE_SHIFT 56
+
+static int xen_evtchn_post_load(void *opaque, int version_id)
+{
+    XenEvtchnState *s = opaque;
+
+    if (s->callback_param) {
+        xen_evtchn_set_callback_param(s->callback_param);
+    }
+
+    return 0;
+}
+
+static bool xen_evtchn_is_needed(void *opaque)
+{
+    return xen_mode == XEN_EMULATE;
+}
+
+static const VMStateDescription xen_evtchn_vmstate = {
+    .name = "xen_evtchn",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = xen_evtchn_is_needed,
+    .post_load = xen_evtchn_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(callback_param, XenEvtchnState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void xen_evtchn_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->vmsd = &xen_evtchn_vmstate;
+}
+
+static const TypeInfo xen_evtchn_info = {
+    .name          = TYPE_XEN_EVTCHN,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(XenEvtchnState),
+    .class_init    = xen_evtchn_class_init,
+};
+
+void xen_evtchn_create(void)
+{
+    XenEvtchnState *s = XEN_EVTCHN(sysbus_create_simple(TYPE_XEN_EVTCHN,
+                                                        -1, NULL));
+    xen_evtchn_singleton = s;
+
+    qemu_mutex_init(&s->port_lock);
+}
+
+static void xen_evtchn_register_types(void)
+{
+    type_register_static(&xen_evtchn_info);
+}
+
+type_init(xen_evtchn_register_types)
+
+int xen_evtchn_set_callback_param(uint64_t param)
+{
+    XenEvtchnState *s = xen_evtchn_singleton;
+    bool in_kernel = false;
+    int ret;
+
+    if (!s) {
+        return -ENOTSUP;
+    }
+
+    qemu_mutex_lock(&s->port_lock);
+
+    switch (param >> CALLBACK_VIA_TYPE_SHIFT) {
+    case HVM_PARAM_CALLBACK_TYPE_VECTOR: {
+        struct kvm_xen_hvm_attr xa = {
+            .type = KVM_XEN_ATTR_TYPE_UPCALL_VECTOR,
+            .u.vector = (uint8_t)param,
+        };
+
+        ret = kvm_vm_ioctl(kvm_state, KVM_XEN_HVM_SET_ATTR, &xa);
+        if (!ret && kvm_xen_has_cap(EVTCHN_SEND)) {
+            in_kernel = true;
+        }
+        break;
+    }
+    default:
+        ret = -ENOSYS;
+        break;
+    }
+
+    if (!ret) {
+        s->callback_param = param;
+        s->evtchn_in_kernel = in_kernel;
+    }
+
+    qemu_mutex_unlock(&s->port_lock);
+
+    return ret;
+}
diff --git a/hw/i386/kvm/xen_evtchn.h b/hw/i386/kvm/xen_evtchn.h
new file mode 100644
index 0000000000..c9b7f9d11f
--- /dev/null
+++ b/hw/i386/kvm/xen_evtchn.h
@@ -0,0 +1,18 @@
+/*
+ * QEMU Xen emulation: Event channel support
+ *
+ * Copyright © 2022 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * Authors: David Woodhouse <dwmw2@infradead.org>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef QEMU_XEN_EVTCHN_H
+#define QEMU_XEN_EVTCHN_H
+
+void xen_evtchn_create(void);
+int xen_evtchn_set_callback_param(uint64_t param);
+
+#endif /* QEMU_XEN_EVTCHN_H */
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 0ddae2f6ad..8f668a5138 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -90,6 +90,7 @@
 #include "hw/virtio/virtio-pmem-pci.h"
 #include "hw/virtio/virtio-mem-pci.h"
 #include "hw/i386/kvm/xen_overlay.h"
+#include "hw/i386/kvm/xen_evtchn.h"
 #include "hw/mem/memory-device.h"
 #include "sysemu/replay.h"
 #include "target/i386/cpu.h"
@@ -1850,6 +1851,7 @@ int pc_machine_kvm_type(MachineState *machine, const char *kvm_type)
 #ifdef CONFIG_XEN_EMU
     if (xen_mode == XEN_EMULATE) {
         xen_overlay_create();
+        xen_evtchn_create();
     }
 #endif
     return 0;
diff --git a/target/i386/kvm/xen-emu.c b/target/i386/kvm/xen-emu.c
index 3e05ef836d..b0e7620b16 100644
--- a/target/i386/kvm/xen-emu.c
+++ b/target/i386/kvm/xen-emu.c
@@ -21,6 +21,7 @@
 #include "sysemu/runstate.h"
 
 #include "hw/i386/kvm/xen_overlay.h"
+#include "hw/i386/kvm/xen_evtchn.h"
 
 #include "standard-headers/xen/version.h"
 #include "standard-headers/xen/sched.h"
@@ -507,6 +508,10 @@ static bool handle_set_param(struct kvm_xen_exit *exit, X86CPU *cpu,
     }
 
     switch (hp.index) {
+    case HVM_PARAM_CALLBACK_IRQ:
+        err = xen_evtchn_set_callback_param(hp.value);
+        xen_set_long_mode(exit->u.hcall.longmode);
+        break;
     default:
         return false;
     }
@@ -712,6 +717,11 @@ static int kvm_xen_soft_reset(void)
     CPUState *cpu;
     int err;
 
+    err = xen_evtchn_set_callback_param(0);
+    if (err) {
+        return err;
+    }
+
     CPU_FOREACH(cpu) {
         async_run_on_cpu(cpu, do_vcpu_soft_reset, RUN_ON_CPU_NULL);
     }
-- 
2.39.0

On 16/01/2023 21:57, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> Include basic support for setting HVM_PARAM_CALLBACK_IRQ to the global
> vector method HVM_PARAM_CALLBACK_TYPE_VECTOR, which is handled in-kernel
> by raising the vector whenever the vCPU's vcpu_info->evtchn_upcall_pending
> flag is set.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>   hw/i386/kvm/meson.build   |   5 +-
>   hw/i386/kvm/xen_evtchn.c  | 148 ++++++++++++++++++++++++++++++++++++++
>   hw/i386/kvm/xen_evtchn.h  |  18 +++++
>   hw/i386/pc.c              |   2 +
>   target/i386/kvm/xen-emu.c |  10 +++
>   5 files changed, 182 insertions(+), 1 deletion(-)
>   create mode 100644 hw/i386/kvm/xen_evtchn.c
>   create mode 100644 hw/i386/kvm/xen_evtchn.h
> 
> diff --git a/hw/i386/kvm/meson.build b/hw/i386/kvm/meson.build
> index 6165cbf019..cab64df339 100644
> --- a/hw/i386/kvm/meson.build
> +++ b/hw/i386/kvm/meson.build
> @@ -4,6 +4,9 @@ i386_kvm_ss.add(when: 'CONFIG_APIC', if_true: files('apic.c'))
>   i386_kvm_ss.add(when: 'CONFIG_I8254', if_true: files('i8254.c'))
>   i386_kvm_ss.add(when: 'CONFIG_I8259', if_true: files('i8259.c'))
>   i386_kvm_ss.add(when: 'CONFIG_IOAPIC', if_true: files('ioapic.c'))
> -i386_kvm_ss.add(when: 'CONFIG_XEN_EMU', if_true: files('xen_overlay.c'))
> +i386_kvm_ss.add(when: 'CONFIG_XEN_EMU', if_true: files(
> +  'xen_overlay.c',
> +  'xen_evtchn.c',
> +  ))
>   
>   i386_ss.add_all(when: 'CONFIG_KVM', if_true: i386_kvm_ss)
> diff --git a/hw/i386/kvm/xen_evtchn.c b/hw/i386/kvm/xen_evtchn.c
> new file mode 100644
> index 0000000000..018f4ef4da
> --- /dev/null
> +++ b/hw/i386/kvm/xen_evtchn.c
> @@ -0,0 +1,148 @@
> +/*
> + * QEMU Xen emulation: Event channel support
> + *
> + * Copyright © 2022 Amazon.com, Inc. or its affiliates. All Rights Reserved.
> + *
> + * Authors: David Woodhouse <dwmw2@infradead.org>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/host-utils.h"
> +#include "qemu/module.h"
> +#include "qemu/main-loop.h"
> +#include "qapi/error.h"
> +#include "qom/object.h"
> +#include "exec/target_page.h"
> +#include "exec/address-spaces.h"
> +#include "migration/vmstate.h"
> +
> +#include "hw/sysbus.h"
> +#include "hw/xen/xen.h"
> +#include "xen_evtchn.h"
> +
> +#include "sysemu/kvm.h"
> +#include "sysemu/kvm_xen.h"
> +#include <linux/kvm.h>
> +
> +#include "standard-headers/xen/memory.h"
> +#include "standard-headers/xen/hvm/params.h"
> +
> +#define TYPE_XEN_EVTCHN "xen-evtchn"
> +OBJECT_DECLARE_SIMPLE_TYPE(XenEvtchnState, XEN_EVTCHN)
> +
> +struct XenEvtchnState {
> +    /*< private >*/
> +    SysBusDevice busdev;
> +    /*< public >*/
> +
> +    uint64_t callback_param;
> +    bool evtchn_in_kernel;
> +
> +    QemuMutex port_lock;
> +};
> +
> +struct XenEvtchnState *xen_evtchn_singleton;
> +
> +/* Top bits of callback_param are the type (HVM_PARAM_CALLBACK_TYPE_xxx) */
> +#define CALLBACK_VIA_TYPE_SHIFT 56
> +
> +static int xen_evtchn_post_load(void *opaque, int version_id)
> +{
> +    XenEvtchnState *s = opaque;
> +
> +    if (s->callback_param) {
> +        xen_evtchn_set_callback_param(s->callback_param);
> +    }
> +
> +    return 0;
> +}
> +
> +static bool xen_evtchn_is_needed(void *opaque)
> +{
> +    return xen_mode == XEN_EMULATE;
> +}
> +
> +static const VMStateDescription xen_evtchn_vmstate = {
> +    .name = "xen_evtchn",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = xen_evtchn_is_needed,
> +    .post_load = xen_evtchn_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT64(callback_param, XenEvtchnState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void xen_evtchn_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->vmsd = &xen_evtchn_vmstate;
> +}
> +
> +static const TypeInfo xen_evtchn_info = {
> +    .name          = TYPE_XEN_EVTCHN,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(XenEvtchnState),
> +    .class_init    = xen_evtchn_class_init,
> +};
> +
> +void xen_evtchn_create(void)
> +{
> +    XenEvtchnState *s = XEN_EVTCHN(sysbus_create_simple(TYPE_XEN_EVTCHN,
> +                                                        -1, NULL));
> +    xen_evtchn_singleton = s;
> +
> +    qemu_mutex_init(&s->port_lock);
> +}
> +
> +static void xen_evtchn_register_types(void)
> +{
> +    type_register_static(&xen_evtchn_info);
> +}
> +
> +type_init(xen_evtchn_register_types)
> +
> +int xen_evtchn_set_callback_param(uint64_t param)
> +{
> +    XenEvtchnState *s = xen_evtchn_singleton;
> +    bool in_kernel = false;
> +    int ret;
> +
> +    if (!s) {
> +        return -ENOTSUP;
> +    }
> +
> +    qemu_mutex_lock(&s->port_lock);
> +
> +    switch (param >> CALLBACK_VIA_TYPE_SHIFT) {
> +    case HVM_PARAM_CALLBACK_TYPE_VECTOR: {
> +        struct kvm_xen_hvm_attr xa = {
> +            .type = KVM_XEN_ATTR_TYPE_UPCALL_VECTOR,
> +            .u.vector = (uint8_t)param,
> +        };
> +
> +        ret = kvm_vm_ioctl(kvm_state, KVM_XEN_HVM_SET_ATTR, &xa);
> +        if (!ret && kvm_xen_has_cap(EVTCHN_SEND)) {
> +            in_kernel = true;
> +        }
> +        break;
> +    }
> +    default:
> +        ret = -ENOSYS;
> +        break;
> +    }
> +
> +    if (!ret) {
> +        s->callback_param = param;
> +        s->evtchn_in_kernel = in_kernel;
> +    }
> +
> +    qemu_mutex_unlock(&s->port_lock);
> +
> +    return ret;
> +}
> diff --git a/hw/i386/kvm/xen_evtchn.h b/hw/i386/kvm/xen_evtchn.h
> new file mode 100644
> index 0000000000..c9b7f9d11f
> --- /dev/null
> +++ b/hw/i386/kvm/xen_evtchn.h
> @@ -0,0 +1,18 @@
> +/*
> + * QEMU Xen emulation: Event channel support
> + *
> + * Copyright © 2022 Amazon.com, Inc. or its affiliates. All Rights Reserved.
> + *
> + * Authors: David Woodhouse <dwmw2@infradead.org>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef QEMU_XEN_EVTCHN_H
> +#define QEMU_XEN_EVTCHN_H
> +
> +void xen_evtchn_create(void);
> +int xen_evtchn_set_callback_param(uint64_t param);
> +
> +#endif /* QEMU_XEN_EVTCHN_H */
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 0ddae2f6ad..8f668a5138 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -90,6 +90,7 @@
>   #include "hw/virtio/virtio-pmem-pci.h"
>   #include "hw/virtio/virtio-mem-pci.h"
>   #include "hw/i386/kvm/xen_overlay.h"
> +#include "hw/i386/kvm/xen_evtchn.h"
>   #include "hw/mem/memory-device.h"
>   #include "sysemu/replay.h"
>   #include "target/i386/cpu.h"
> @@ -1850,6 +1851,7 @@ int pc_machine_kvm_type(MachineState *machine, const char *kvm_type)
>   #ifdef CONFIG_XEN_EMU
>       if (xen_mode == XEN_EMULATE) {
>           xen_overlay_create();
> +        xen_evtchn_create();
>       }
>   #endif
>       return 0;
> diff --git a/target/i386/kvm/xen-emu.c b/target/i386/kvm/xen-emu.c
> index 3e05ef836d..b0e7620b16 100644
> --- a/target/i386/kvm/xen-emu.c
> +++ b/target/i386/kvm/xen-emu.c
> @@ -21,6 +21,7 @@
>   #include "sysemu/runstate.h"
>   
>   #include "hw/i386/kvm/xen_overlay.h"
> +#include "hw/i386/kvm/xen_evtchn.h"
>   
>   #include "standard-headers/xen/version.h"
>   #include "standard-headers/xen/sched.h"
> @@ -507,6 +508,10 @@ static bool handle_set_param(struct kvm_xen_exit *exit, X86CPU *cpu,
>       }
>   
>       switch (hp.index) {
> +    case HVM_PARAM_CALLBACK_IRQ:
> +        err = xen_evtchn_set_callback_param(hp.value);
> +        xen_set_long_mode(exit->u.hcall.longmode);
> +        break;
>       default:
>           return false;
>       }
> @@ -712,6 +717,11 @@ static int kvm_xen_soft_reset(void)
>       CPUState *cpu;
>       int err;
>   
> +    err = xen_evtchn_set_callback_param(0);

Doesn't this always result in -ENOSYS?

> +    if (err) {
> +        return err;
> +    }
> +
>       CPU_FOREACH(cpu) {
>           async_run_on_cpu(cpu, do_vcpu_soft_reset, RUN_ON_CPU_NULL);
>       }

On Tue, 2023-01-17 at 10:00 +0000, Paul Durrant wrote:
> 
> > @@ -712,6 +717,11 @@ static int kvm_xen_soft_reset(void)
> >        CPUState *cpu;
> >        int err;
> >    
> > +    err = xen_evtchn_set_callback_param(0);
> 
> Doesn't this always result in -ENOSYS?

Hm?

Even at this point in the series, HVM_PARAM_CALLBACK_TYPE_VECTOR works
and doesn't result in -ENOSYS.

But even if xen_evtchn_set_callback_param() *was* a stub that just
returned -ENOSYS at this point, that would be OK, surely? We add the
(other) HVM_PARAM_CALLBACK_TYPE_* support later, which warrants
separate review because of the GSI and iothread lock fun.

On 17/01/2023 10:23, David Woodhouse wrote:
> On Tue, 2023-01-17 at 10:00 +0000, Paul Durrant wrote:
>>
>>> @@ -712,6 +717,11 @@ static int kvm_xen_soft_reset(void)
>>>         CPUState *cpu;
>>>         int err;
>>>     
>>> +    err = xen_evtchn_set_callback_param(0);
>>
>> Doesn't this always result in -ENOSYS?
> 
> Hm?
> 
> Even at this point in the series, HVM_PARAM_CALLBACK_TYPE_VECTOR works
> and doesn't result in -ENOSYS.
> 
> But even if xen_evtchn_set_callback_param() *was* a stub that just
> returned -ENOSYS at this point, that would be OK, surely? We add the
> (other) HVM_PARAM_CALLBACK_TYPE_* support later, which warrants
> separate review because of the GSI and iothread lock fun.

I'm just having a hard time seeing why passing 0 to 
xen_evtchn_set_callback_param() does anything useful...

+    switch (param >> CALLBACK_VIA_TYPE_SHIFT) {
+    case HVM_PARAM_CALLBACK_TYPE_VECTOR: {
+        struct kvm_xen_hvm_attr xa = {
+            .type = KVM_XEN_ATTR_TYPE_UPCALL_VECTOR,
+            .u.vector = (uint8_t)param,
+        };

HVM_PARAM_CALLBACK_TYPE_VECTOR is 2 AFAICT, so it won't hit that case. 
Also, you appear to be passing the unshifted param to kernel anyway.

What is the call trying to achieve?

   Paul

On Tue, 2023-01-17 at 10:56 +0000, Paul Durrant wrote:
> 
> I'm just having a hard time seeing why passing 0 to 
> xen_evtchn_set_callback_param() does anything useful...
> 
> +    switch (param >> CALLBACK_VIA_TYPE_SHIFT) {
> +    case HVM_PARAM_CALLBACK_TYPE_VECTOR: {
> +        struct kvm_xen_hvm_attr xa = {
> +            .type = KVM_XEN_ATTR_TYPE_UPCALL_VECTOR,
> +            .u.vector = (uint8_t)param,
> +        };
> 
> HVM_PARAM_CALLBACK_TYPE_VECTOR is 2 AFAICT, so it won't hit that case. 
> Also, you appear to be passing the unshifted param to kernel anyway.
> 
> What is the call trying to achieve?

Zero is HVM_PARAM_CALLBACK_TYPE_GSI, with GSI==0. It's basically
disabling event channel delivery for the new kernel.

On 17/01/2023 11:02, David Woodhouse wrote:
> On Tue, 2023-01-17 at 10:56 +0000, Paul Durrant wrote:
>>
>> I'm just having a hard time seeing why passing 0 to
>> xen_evtchn_set_callback_param() does anything useful...
>>
>> +    switch (param >> CALLBACK_VIA_TYPE_SHIFT) {
>> +    case HVM_PARAM_CALLBACK_TYPE_VECTOR: {
>> +        struct kvm_xen_hvm_attr xa = {
>> +            .type = KVM_XEN_ATTR_TYPE_UPCALL_VECTOR,
>> +            .u.vector = (uint8_t)param,
>> +        };
>>
>> HVM_PARAM_CALLBACK_TYPE_VECTOR is 2 AFAICT, so it won't hit that case.
>> Also, you appear to be passing the unshifted param to kernel anyway.
>>
>> What is the call trying to achieve?
> 
> Zero is HVM_PARAM_CALLBACK_TYPE_GSI, with GSI==0. It's basically
> disabling event channel delivery for the new kernel.
> 

AFAICT it is doing nothing at this point. Unless I am going insane it 
results in this codepath:

+    default:
+        ret = -ENOSYS;
+        break;
+    }
+
+    if (!ret) {
+        s->callback_param = param;
+        s->evtchn_in_kernel = in_kernel;
+    }

So it doesn't result in any cleanup. What am I missing?

   Paul

On Tue, 2023-01-17 at 11:06 +0000, Paul Durrant wrote:
> On 17/01/2023 11:02, David Woodhouse wrote:
> > On Tue, 2023-01-17 at 10:56 +0000, Paul Durrant wrote:
> > > 
> > > I'm just having a hard time seeing why passing 0 to
> > > xen_evtchn_set_callback_param() does anything useful...
> > > 
> > > +    switch (param >> CALLBACK_VIA_TYPE_SHIFT) {
> > > +    case HVM_PARAM_CALLBACK_TYPE_VECTOR: {
> > > +        struct kvm_xen_hvm_attr xa = {
> > > +            .type = KVM_XEN_ATTR_TYPE_UPCALL_VECTOR,
> > > +            .u.vector = (uint8_t)param,
> > > +        };
> > > 
> > > HVM_PARAM_CALLBACK_TYPE_VECTOR is 2 AFAICT, so it won't hit that
> > > case.
> > > Also, you appear to be passing the unshifted param to kernel
> > > anyway.
> > > 
> > > What is the call trying to achieve?
> > 
> > Zero is HVM_PARAM_CALLBACK_TYPE_GSI, with GSI==0. It's basically
> > disabling event channel delivery for the new kernel.
> > 
> 
> AFAICT it is doing nothing at this point. Unless I am going insane it
> results in this codepath:
> 
> +    default:
> +        ret = -ENOSYS;
> +        break;
> +    }
> +
> +    if (!ret) {
> +        s->callback_param = param;
> +        s->evtchn_in_kernel = in_kernel;
> +    }
> 
> So it doesn't result in any cleanup. What am I missing?

Indeed, it doesn't result in any cleanup at *this* point in the series
because HVM_PARAM_CALLBACK_TYPE_GSI hasn't been implemented yet; it's
in a later patch.

The series is broken up into sensible individual patches for review,
not so people can actually *run* with some partial subset of them.

Otherwise I'd have to implement vmstate migration versioning for every
change in the series, and that way lies madness :)

I *will* add a comment in the code explaining what zero means though,
and note in the commit message that it doesn't actually have any effect
will, but will do in a few patches' time.

On 17/01/2023 11:24, David Woodhouse wrote:
> On Tue, 2023-01-17 at 11:06 +0000, Paul Durrant wrote:
>> On 17/01/2023 11:02, David Woodhouse wrote:
>>> On Tue, 2023-01-17 at 10:56 +0000, Paul Durrant wrote:
>>>>
>>>> I'm just having a hard time seeing why passing 0 to
>>>> xen_evtchn_set_callback_param() does anything useful...
>>>>
>>>> +    switch (param >> CALLBACK_VIA_TYPE_SHIFT) {
>>>> +    case HVM_PARAM_CALLBACK_TYPE_VECTOR: {
>>>> +        struct kvm_xen_hvm_attr xa = {
>>>> +            .type = KVM_XEN_ATTR_TYPE_UPCALL_VECTOR,
>>>> +            .u.vector = (uint8_t)param,
>>>> +        };
>>>>
>>>> HVM_PARAM_CALLBACK_TYPE_VECTOR is 2 AFAICT, so it won't hit that
>>>> case.
>>>> Also, you appear to be passing the unshifted param to kernel
>>>> anyway.
>>>>
>>>> What is the call trying to achieve?
>>>
>>> Zero is HVM_PARAM_CALLBACK_TYPE_GSI, with GSI==0. It's basically
>>> disabling event channel delivery for the new kernel.
>>>
>>
>> AFAICT it is doing nothing at this point. Unless I am going insane it
>> results in this codepath:
>>
>> +    default:
>> +        ret = -ENOSYS;
>> +        break;
>> +    }
>> +
>> +    if (!ret) {
>> +        s->callback_param = param;
>> +        s->evtchn_in_kernel = in_kernel;
>> +    }
>>
>> So it doesn't result in any cleanup. What am I missing?
> 
> Indeed, it doesn't result in any cleanup at *this* point in the series
> because HVM_PARAM_CALLBACK_TYPE_GSI hasn't been implemented yet; it's
> in a later patch.
> 
> The series is broken up into sensible individual patches for review,
> not so people can actually *run* with some partial subset of them.
> 

That's fine. It's just confusing for a reviewer to know whether you are 
intentionally introducing code that has no effect, or whether that is a bug.

> Otherwise I'd have to implement vmstate migration versioning for every
> change in the series, and that way lies madness :)
> 
> I *will* add a comment in the code explaining what zero means though,
> and note in the commit message that it doesn't actually have any effect
> will, but will do in a few patches' time.

That would certainly help, thanks.

On Tue, 2023-01-17 at 11:53 +0000, Paul Durrant wrote:
> On 17/01/2023 11:24, David Woodhouse wrote:
> > On Tue, 2023-01-17 at 11:06 +0000, Paul Durrant wrote:
> > > On 17/01/2023 11:02, David Woodhouse wrote:
> > > > On Tue, 2023-01-17 at 10:56 +0000, Paul Durrant wrote:
> > > > > 
> > > > > I'm just having a hard time seeing why passing 0 to
> > > > > xen_evtchn_set_callback_param() does anything useful...
> > > > > 
> > > > > +    switch (param >> CALLBACK_VIA_TYPE_SHIFT) {
> > > > > +    case HVM_PARAM_CALLBACK_TYPE_VECTOR: {
> > > > > +        struct kvm_xen_hvm_attr xa = {
> > > > > +            .type = KVM_XEN_ATTR_TYPE_UPCALL_VECTOR,
> > > > > +            .u.vector = (uint8_t)param,
> > > > > +        };
> > > > > 
> > > > > HVM_PARAM_CALLBACK_TYPE_VECTOR is 2 AFAICT, so it won't hit that
> > > > > case.
> > > > > Also, you appear to be passing the unshifted param to kernel
> > > > > anyway.
> > > > > 
> > > > > What is the call trying to achieve?
> > > > 
> > > > Zero is HVM_PARAM_CALLBACK_TYPE_GSI, with GSI==0. It's basically
> > > > disabling event channel delivery for the new kernel.
> > > > 
> > > 
> > > AFAICT it is doing nothing at this point. Unless I am going insane it
> > > results in this codepath:
> > > 
> > > +    default:
> > > +        ret = -ENOSYS;
> > > +        break;
> > > +    }
> > > +
> > > +    if (!ret) {
> > > +        s->callback_param = param;
> > > +        s->evtchn_in_kernel = in_kernel;
> > > +    }
> > > 
> > > So it doesn't result in any cleanup. What am I missing?
> > 
> > Indeed, it doesn't result in any cleanup at *this* point in the series
> > because HVM_PARAM_CALLBACK_TYPE_GSI hasn't been implemented yet; it's
> > in a later patch.
> > 
> > The series is broken up into sensible individual patches for review,
> > not so people can actually *run* with some partial subset of them.
> > 
> 
> That's fine. It's just confusing for a reviewer to know whether you are 
> intentionally introducing code that has no effect, or whether that is a bug.

Actually... it *is* a bug. Even when GSI support is added later,
nothing ever tells the kernel to *stop* delivering via the upcall
vector. I'll fix. Thanks for drawing attention to it.

From: David Woodhouse <dwmw@amazon.co.uk>

Include basic support for setting HVM_PARAM_CALLBACK_IRQ to the global
vector method HVM_PARAM_CALLBACK_TYPE_VECTOR, which is handled in-kernel
by raising the vector whenever the vCPU's vcpu_info->evtchn_upcall_pending
flag is set.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 hw/i386/kvm/meson.build   |   5 +-
 hw/i386/kvm/xen_evtchn.c  | 154 ++++++++++++++++++++++++++++++++++++++
 hw/i386/kvm/xen_evtchn.h  |  18 +++++
 hw/i386/pc.c              |   2 +
 target/i386/kvm/xen-emu.c |  15 ++++
 5 files changed, 193 insertions(+), 1 deletion(-)
 create mode 100644 hw/i386/kvm/xen_evtchn.c
 create mode 100644 hw/i386/kvm/xen_evtchn.h

diff --git a/hw/i386/kvm/meson.build b/hw/i386/kvm/meson.build
index 6165cbf019..cab64df339 100644
--- a/hw/i386/kvm/meson.build
+++ b/hw/i386/kvm/meson.build
@@ -4,6 +4,9 @@ i386_kvm_ss.add(when: 'CONFIG_APIC', if_true: files('apic.c'))
 i386_kvm_ss.add(when: 'CONFIG_I8254', if_true: files('i8254.c'))
 i386_kvm_ss.add(when: 'CONFIG_I8259', if_true: files('i8259.c'))
 i386_kvm_ss.add(when: 'CONFIG_IOAPIC', if_true: files('ioapic.c'))
-i386_kvm_ss.add(when: 'CONFIG_XEN_EMU', if_true: files('xen_overlay.c'))
+i386_kvm_ss.add(when: 'CONFIG_XEN_EMU', if_true: files(
+  'xen_overlay.c',
+  'xen_evtchn.c',
+  ))
 
 i386_ss.add_all(when: 'CONFIG_KVM', if_true: i386_kvm_ss)
diff --git a/hw/i386/kvm/xen_evtchn.c b/hw/i386/kvm/xen_evtchn.c
new file mode 100644
index 0000000000..57aa72a3ab
--- /dev/null
+++ b/hw/i386/kvm/xen_evtchn.c
@@ -0,0 +1,154 @@
+/*
+ * QEMU Xen emulation: Event channel support
+ *
+ * Copyright © 2022 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * Authors: David Woodhouse <dwmw2@infradead.org>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/host-utils.h"
+#include "qemu/module.h"
+#include "qemu/main-loop.h"
+#include "qapi/error.h"
+#include "qom/object.h"
+#include "exec/target_page.h"
+#include "exec/address-spaces.h"
+#include "migration/vmstate.h"
+
+#include "hw/sysbus.h"
+#include "hw/xen/xen.h"
+#include "xen_evtchn.h"
+
+#include "sysemu/kvm.h"
+#include "sysemu/kvm_xen.h"
+#include <linux/kvm.h>
+
+#include "standard-headers/xen/memory.h"
+#include "standard-headers/xen/hvm/params.h"
+
+#define TYPE_XEN_EVTCHN "xen-evtchn"
+OBJECT_DECLARE_SIMPLE_TYPE(XenEvtchnState, XEN_EVTCHN)
+
+struct XenEvtchnState {
+    /*< private >*/
+    SysBusDevice busdev;
+    /*< public >*/
+
+    uint64_t callback_param;
+    bool evtchn_in_kernel;
+
+    QemuMutex port_lock;
+};
+
+struct XenEvtchnState *xen_evtchn_singleton;
+
+/* Top bits of callback_param are the type (HVM_PARAM_CALLBACK_TYPE_xxx) */
+#define CALLBACK_VIA_TYPE_SHIFT 56
+
+static int xen_evtchn_post_load(void *opaque, int version_id)
+{
+    XenEvtchnState *s = opaque;
+
+    if (s->callback_param) {
+        xen_evtchn_set_callback_param(s->callback_param);
+    }
+
+    return 0;
+}
+
+static bool xen_evtchn_is_needed(void *opaque)
+{
+    return xen_mode == XEN_EMULATE;
+}
+
+static const VMStateDescription xen_evtchn_vmstate = {
+    .name = "xen_evtchn",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = xen_evtchn_is_needed,
+    .post_load = xen_evtchn_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(callback_param, XenEvtchnState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void xen_evtchn_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->vmsd = &xen_evtchn_vmstate;
+}
+
+static const TypeInfo xen_evtchn_info = {
+    .name          = TYPE_XEN_EVTCHN,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(XenEvtchnState),
+    .class_init    = xen_evtchn_class_init,
+};
+
+void xen_evtchn_create(void)
+{
+    XenEvtchnState *s = XEN_EVTCHN(sysbus_create_simple(TYPE_XEN_EVTCHN,
+                                                        -1, NULL));
+    xen_evtchn_singleton = s;
+
+    qemu_mutex_init(&s->port_lock);
+}
+
+static void xen_evtchn_register_types(void)
+{
+    type_register_static(&xen_evtchn_info);
+}
+
+type_init(xen_evtchn_register_types)
+
+int xen_evtchn_set_callback_param(uint64_t param)
+{
+    XenEvtchnState *s = xen_evtchn_singleton;
+    struct kvm_xen_hvm_attr xa = {
+        .type = KVM_XEN_ATTR_TYPE_UPCALL_VECTOR,
+        .u.vector = 0,
+    };
+    bool in_kernel = false;
+    int ret;
+
+    if (!s) {
+        return -ENOTSUP;
+    }
+
+    qemu_mutex_lock(&s->port_lock);
+
+    switch (param >> CALLBACK_VIA_TYPE_SHIFT) {
+    case HVM_PARAM_CALLBACK_TYPE_VECTOR: {
+        xa.u.vector = (uint8_t)param,
+
+        ret = kvm_vm_ioctl(kvm_state, KVM_XEN_HVM_SET_ATTR, &xa);
+        if (!ret && kvm_xen_has_cap(EVTCHN_SEND)) {
+            in_kernel = true;
+        }
+        break;
+    }
+    default:
+        ret = -ENOSYS;
+        break;
+    }
+
+    if (!ret) {
+        /* If vector delivery was turned *off* then tell the kernel */
+        if ((s->callback_param >> CALLBACK_VIA_TYPE_SHIFT) ==
+            HVM_PARAM_CALLBACK_TYPE_VECTOR && !xa.u.vector) {
+            kvm_vm_ioctl(kvm_state, KVM_XEN_HVM_SET_ATTR, &xa);
+        }
+        s->callback_param = param;
+        s->evtchn_in_kernel = in_kernel;
+    }
+
+    qemu_mutex_unlock(&s->port_lock);
+
+    return ret;
+}
diff --git a/hw/i386/kvm/xen_evtchn.h b/hw/i386/kvm/xen_evtchn.h
new file mode 100644
index 0000000000..c9b7f9d11f
--- /dev/null
+++ b/hw/i386/kvm/xen_evtchn.h
@@ -0,0 +1,18 @@
+/*
+ * QEMU Xen emulation: Event channel support
+ *
+ * Copyright © 2022 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * Authors: David Woodhouse <dwmw2@infradead.org>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef QEMU_XEN_EVTCHN_H
+#define QEMU_XEN_EVTCHN_H
+
+void xen_evtchn_create(void);
+int xen_evtchn_set_callback_param(uint64_t param);
+
+#endif /* QEMU_XEN_EVTCHN_H */
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 0ddae2f6ad..8f668a5138 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -90,6 +90,7 @@
 #include "hw/virtio/virtio-pmem-pci.h"
 #include "hw/virtio/virtio-mem-pci.h"
 #include "hw/i386/kvm/xen_overlay.h"
+#include "hw/i386/kvm/xen_evtchn.h"
 #include "hw/mem/memory-device.h"
 #include "sysemu/replay.h"
 #include "target/i386/cpu.h"
@@ -1850,6 +1851,7 @@ int pc_machine_kvm_type(MachineState *machine, const char *kvm_type)
 #ifdef CONFIG_XEN_EMU
     if (xen_mode == XEN_EMULATE) {
         xen_overlay_create();
+        xen_evtchn_create();
     }
 #endif
     return 0;
diff --git a/target/i386/kvm/xen-emu.c b/target/i386/kvm/xen-emu.c
index c302304b3e..6a134bd854 100644
--- a/target/i386/kvm/xen-emu.c
+++ b/target/i386/kvm/xen-emu.c
@@ -21,6 +21,7 @@
 #include "sysemu/runstate.h"
 
 #include "hw/i386/kvm/xen_overlay.h"
+#include "hw/i386/kvm/xen_evtchn.h"
 
 #include "standard-headers/xen/version.h"
 #include "standard-headers/xen/sched.h"
@@ -507,6 +508,10 @@ static bool handle_set_param(struct kvm_xen_exit *exit, X86CPU *cpu,
     }
 
     switch (hp.index) {
+    case HVM_PARAM_CALLBACK_IRQ:
+        err = xen_evtchn_set_callback_param(hp.value);
+        xen_set_long_mode(exit->u.hcall.longmode);
+        break;
     default:
         return false;
     }
@@ -712,6 +717,16 @@ static int kvm_xen_soft_reset(void)
     CPUState *cpu;
     int err;
 
+    /*
+     * Zero is the reset/startup state for HVM_PARAM_CALLBACK_IRQ. Strictly,
+     * it maps to HVM_PARAM_CALLBACK_TYPE_GSI with GSI#0, but Xen refuses to
+     * to deliver to the timer interrupt and treats that as 'disabled'.
+     */
+    err = xen_evtchn_set_callback_param(0);
+    if (err) {
+        return err;
+    }
+
     CPU_FOREACH(cpu) {
         async_run_on_cpu(cpu, do_vcpu_soft_reset, RUN_ON_CPU_NULL);
     }
-- 
2.34.1


>