[PATCH 0/12] Get Xen PV shim running in qemu

[PATCH 0/12] Get Xen PV shim running in qemu

[David Woodhouse]

I hadn't got round to getting the PV shim running yet; I thought it would
need work on the multiboot loader. Turns out it doesn't. I *did* need to
fix a couple of brown-paper-bag bugs in the per-vCPU upcall vector support,
and implement Xen console support though. Now I can test PV guests:

 $ qemu-system-x86_64 --accel kvm,xen-version=0x40011,kernel-irqchip=split \
   -chardev stdio,mux=on,id=char0 -device xen-console,chardev=char0 \
   -drive file=${GUEST_IMAGE},if=xen -display none -m 1G \
   -kernel ~/git/xen/xen/xen -initrd ~/git/linux/arch/x86/boot/bzImage \
   -append "loglvl=all -- console=hvc0 root=/dev/xvda1"

 blockdev.c                                     |  15 +++-
 hw/block/xen-block.c                           |  26 +++++-
 hw/char/trace-events                           |   8 ++
 hw/char/xen_console.c                          | 522 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------
 hw/i386/kvm/meson.build                        |   1 +
 hw/i386/kvm/trace-events                       |   2 +
 hw/i386/kvm/xen-stubs.c                        |   5 ++
 hw/i386/kvm/xen_evtchn.c                       |   6 ++
 hw/i386/kvm/xen_gnttab.c                       |  32 ++++++-
 hw/i386/kvm/xen_primary_console.c              | 167 ++++++++++++++++++++++++++++++++++
 hw/i386/kvm/xen_primary_console.h              |  22 +++++
 hw/i386/kvm/xen_xenstore.c                     |  21 ++++-
 hw/xen/xen-backend.c                           |  81 +++++++++++++----
 hw/xen/xen-bus.c                               |  21 ++++-
 hw/xen/xen-legacy-backend.c                    |   1 -
 include/hw/xen/interface/arch-arm.h            |  37 ++++----
 include/hw/xen/interface/arch-x86/cpuid.h      |  31 +++----
 include/hw/xen/interface/arch-x86/xen-x86_32.h |  19 +---
 include/hw/xen/interface/arch-x86/xen-x86_64.h |  19 +---
 include/hw/xen/interface/arch-x86/xen.h        |  26 +-----
 include/hw/xen/interface/event_channel.h       |  19 +---
 include/hw/xen/interface/features.h            |  19 +---
 include/hw/xen/interface/grant_table.h         |  19 +---
 include/hw/xen/interface/hvm/hvm_op.h          |  19 +---
 include/hw/xen/interface/hvm/params.h          |  19 +---
 include/hw/xen/interface/io/blkif.h            |  27 ++----
 include/hw/xen/interface/io/console.h          |  19 +---
 include/hw/xen/interface/io/fbif.h             |  19 +---
 include/hw/xen/interface/io/kbdif.h            |  19 +---
 include/hw/xen/interface/io/netif.h            |  25 ++----
 include/hw/xen/interface/io/protocols.h        |  19 +---
 include/hw/xen/interface/io/ring.h             |  49 +++++-----
 include/hw/xen/interface/io/usbif.h            |  19 +---
 include/hw/xen/interface/io/xenbus.h           |  19 +---
 include/hw/xen/interface/io/xs_wire.h          |  36 ++++----
 include/hw/xen/interface/memory.h              |  30 +++----
 include/hw/xen/interface/physdev.h             |  23 +----
 include/hw/xen/interface/sched.h               |  19 +---
 include/hw/xen/interface/trace.h               |  19 +---
 include/hw/xen/interface/vcpu.h                |  19 +---
 include/hw/xen/interface/version.h             |  19 +---
 include/hw/xen/interface/xen-compat.h          |  19 +---
 include/hw/xen/interface/xen.h                 |  19 +---
 include/hw/xen/xen-backend.h                   |   4 +
 include/hw/xen/xen-bus.h                       |   2 +
 include/sysemu/kvm_xen.h                       |   1 +
 target/i386/kvm/kvm.c                          |   4 +
 target/i386/kvm/xen-emu.c                      |  35 ++++++--
 48 files changed, 941 insertions(+), 680 deletions(-)

[PATCH 01/12] i386/xen: fix per-vCPU upcall vector for Xen emulation

[David Woodhouse]

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

The per-vCPU upcall vector support had two problems. Firstly it was
using the wrong hypercall argument and would always return -EFAULT.
And secondly it was using the wrong ioctl() to pass the vector to
the kernel and thus the *kernel* would always return -EINVAL.

Linux doesn't (yet) use this mode so it went without decent testing
for a while.

Fixes: 105b47fdf2d0 ("i386/xen: implement HVMOP_set_evtchn_upcall_vector")
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 target/i386/kvm/xen-emu.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/target/i386/kvm/xen-emu.c b/target/i386/kvm/xen-emu.c
index c4fa84a982..b49a840438 100644
--- a/target/i386/kvm/xen-emu.c
+++ b/target/i386/kvm/xen-emu.c
@@ -306,7 +306,7 @@ static int kvm_xen_set_vcpu_callback_vector(CPUState *cs)
 
     trace_kvm_xen_set_vcpu_callback(cs->cpu_index, vector);
 
-    return kvm_vcpu_ioctl(cs, KVM_XEN_HVM_SET_ATTR, &xva);
+    return kvm_vcpu_ioctl(cs, KVM_XEN_VCPU_SET_ATTR, &xva);
 }
 
 static void do_set_vcpu_callback_vector(CPUState *cs, run_on_cpu_data data)
@@ -849,8 +849,7 @@ static bool kvm_xen_hcall_hvm_op(struct kvm_xen_exit *exit, X86CPU *cpu,
     int ret = -ENOSYS;
     switch (cmd) {
     case HVMOP_set_evtchn_upcall_vector:
-        ret = kvm_xen_hcall_evtchn_upcall_vector(exit, cpu,
-                                                 exit->u.hcall.params[0]);
+        ret = kvm_xen_hcall_evtchn_upcall_vector(exit, cpu, arg);
         break;
 
     case HVMOP_pagetable_dying:
-- 
2.40.1

Re: [PATCH 01/12] i386/xen: fix per-vCPU upcall vector for Xen emulation

[Paul Durrant]

On 16/10/2023 16:18, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> The per-vCPU upcall vector support had two problems. Firstly it was
> using the wrong hypercall argument and would always return -EFAULT.
> And secondly it was using the wrong ioctl() to pass the vector to
> the kernel and thus the *kernel* would always return -EINVAL.
> 
> Linux doesn't (yet) use this mode so it went without decent testing
> for a while.
> 
> Fixes: 105b47fdf2d0 ("i386/xen: implement HVMOP_set_evtchn_upcall_vector")
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>   target/i386/kvm/xen-emu.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)

Reviewed-by: Paul Durrant <paul@xen.org>

Re: [PATCH 01/12] i386/xen: fix per-vCPU upcall vector for Xen emulation

[David Woodhouse]

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

On Tue, 2023-10-24 at 13:16 +0100, Paul Durrant wrote:
> On 16/10/2023 16:18, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > The per-vCPU upcall vector support had two problems. Firstly it was
> > using the wrong hypercall argument and would always return -EFAULT.
> > And secondly it was using the wrong ioctl() to pass the vector to
> > the kernel and thus the *kernel* would always return -EINVAL.
> > 
> > Linux doesn't (yet) use this mode so it went without decent testing
> > for a while.
> > 
> > Fixes: 105b47fdf2d0 ("i386/xen: implement
> > HVMOP_set_evtchn_upcall_vector")
> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> > ---
> >   target/i386/kvm/xen-emu.c | 5 ++---
> >   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> Reviewed-by: Paul Durrant <paul@xen.org>

FWIW this patch gained a third "brown paper bag" fix this morning, when
I finally worked it out:

@@ -440,7 +440,8 @@ void kvm_xen_inject_vcpu_callback_vector(uint32_t
vcpu_id, int type)
          * deliver it as an MSI.
          */
         MSIMessage msg = {
-            .address = APIC_DEFAULT_ADDRESS | X86_CPU(cs)->apic_id,
+            .address = APIC_DEFAULT_ADDRESS |
+                       (X86_CPU(cs)->apic_id << MSI_ADDR_DEST_ID_SHIFT),
             .data = vector | (1UL << MSI_DATA_LEVEL_SHIFT),
         };
         kvm_irqchip_send_msi(kvm_state, msg);


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

[PATCH 02/12] hw/xen: select kernel mode for per-vCPU event channel upcall vector

[David Woodhouse]

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

A guest which has configured the per-vCPU upcall vector may set the
HVM_PARAM_CALLBACK_IRQ param to fairly much anything other than zero.

For example, Linux v6.0+ after commit b1c3497e604 ("x86/xen: Add support
for HVMOP_set_evtchn_upcall_vector") will just do this after setting the
vector:

       /* Trick toolstack to think we are enlightened. */
       if (!cpu)
               rc = xen_set_callback_via(1);

That's explicitly setting the delivery to GSI#, but it's supposed to be
overridden by the per-vCPU vector setting. This mostly works in QEMU
*except* for the logic to enable the in-kernel handling of event channels,
which falsely determines that the kernel cannot accelerate GSI delivery
in this case.

Add a kvm_xen_has_vcpu_callback_vector() to report whether vCPU#0 has
the vector set, and use that in xen_evtchn_set_callback_param() to
enable the kernel acceleration features even when the param *appears*
to be set to target a GSI.

Preserve the Xen behaviour that when HVM_PARAM_CALLBACK_IRQ is set to
*zero* the event channel delivery is disabled completely. (Which is
what that bizarre guest behaviour is working round in the first place.)

Fixes: 91cce756179 ("hw/xen: Add xen_evtchn device for event channel emulation")
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 hw/i386/kvm/xen_evtchn.c  | 6 ++++++
 include/sysemu/kvm_xen.h  | 1 +
 target/i386/kvm/xen-emu.c | 7 +++++++
 3 files changed, 14 insertions(+)

diff --git a/hw/i386/kvm/xen_evtchn.c b/hw/i386/kvm/xen_evtchn.c
index 4df973022c..d72dca6591 100644
--- a/hw/i386/kvm/xen_evtchn.c
+++ b/hw/i386/kvm/xen_evtchn.c
@@ -490,6 +490,12 @@ int xen_evtchn_set_callback_param(uint64_t param)
         break;
     }
 
+    /* If the guest has set a per-vCPU callback vector, prefer that. */
+    if (gsi && kvm_xen_has_vcpu_callback_vector()) {
+        in_kernel = kvm_xen_has_cap(EVTCHN_SEND);
+        gsi = 0;
+    }
+
     if (!ret) {
         /* If vector delivery was turned *off* then tell the kernel */
         if ((s->callback_param >> CALLBACK_VIA_TYPE_SHIFT) ==
diff --git a/include/sysemu/kvm_xen.h b/include/sysemu/kvm_xen.h
index 595abfbe40..961c702c4e 100644
--- a/include/sysemu/kvm_xen.h
+++ b/include/sysemu/kvm_xen.h
@@ -22,6 +22,7 @@
 int kvm_xen_soft_reset(void);
 uint32_t kvm_xen_get_caps(void);
 void *kvm_xen_get_vcpu_info_hva(uint32_t vcpu_id);
+bool kvm_xen_has_vcpu_callback_vector(void);
 void kvm_xen_inject_vcpu_callback_vector(uint32_t vcpu_id, int type);
 void kvm_xen_set_callback_asserted(void);
 int kvm_xen_set_vcpu_virq(uint32_t vcpu_id, uint16_t virq, uint16_t port);
diff --git a/target/i386/kvm/xen-emu.c b/target/i386/kvm/xen-emu.c
index b49a840438..477e93cd92 100644
--- a/target/i386/kvm/xen-emu.c
+++ b/target/i386/kvm/xen-emu.c
@@ -424,6 +424,13 @@ void kvm_xen_set_callback_asserted(void)
     }
 }
 
+bool kvm_xen_has_vcpu_callback_vector(void)
+{
+    CPUState *cs = qemu_get_cpu(0);
+
+    return cs && !!X86_CPU(cs)->env.xen_vcpu_callback_vector;
+}
+
 void kvm_xen_inject_vcpu_callback_vector(uint32_t vcpu_id, int type)
 {
     CPUState *cs = qemu_get_cpu(vcpu_id);
-- 
2.40.1

Re: [PATCH 02/12] hw/xen: select kernel mode for per-vCPU event channel upcall vector

[Paul Durrant]

On 16/10/2023 16:18, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> A guest which has configured the per-vCPU upcall vector may set the
> HVM_PARAM_CALLBACK_IRQ param to fairly much anything other than zero.
> 
> For example, Linux v6.0+ after commit b1c3497e604 ("x86/xen: Add support
> for HVMOP_set_evtchn_upcall_vector") will just do this after setting the
> vector:
> 
>         /* Trick toolstack to think we are enlightened. */
>         if (!cpu)
>                 rc = xen_set_callback_via(1);
> 
> That's explicitly setting the delivery to GSI#, but it's supposed to be
> overridden by the per-vCPU vector setting. This mostly works in QEMU
> *except* for the logic to enable the in-kernel handling of event channels,
> which falsely determines that the kernel cannot accelerate GSI delivery
> in this case.
> 
> Add a kvm_xen_has_vcpu_callback_vector() to report whether vCPU#0 has
> the vector set, and use that in xen_evtchn_set_callback_param() to
> enable the kernel acceleration features even when the param *appears*
> to be set to target a GSI.
> 
> Preserve the Xen behaviour that when HVM_PARAM_CALLBACK_IRQ is set to
> *zero* the event channel delivery is disabled completely. (Which is
> what that bizarre guest behaviour is working round in the first place.)
> 
> Fixes: 91cce756179 ("hw/xen: Add xen_evtchn device for event channel emulation")
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>   hw/i386/kvm/xen_evtchn.c  | 6 ++++++
>   include/sysemu/kvm_xen.h  | 1 +
>   target/i386/kvm/xen-emu.c | 7 +++++++
>   3 files changed, 14 insertions(+)
> 
> diff --git a/hw/i386/kvm/xen_evtchn.c b/hw/i386/kvm/xen_evtchn.c
> index 4df973022c..d72dca6591 100644
> --- a/hw/i386/kvm/xen_evtchn.c
> +++ b/hw/i386/kvm/xen_evtchn.c
> @@ -490,6 +490,12 @@ int xen_evtchn_set_callback_param(uint64_t param)
>           break;
>       }
>   
> +    /* If the guest has set a per-vCPU callback vector, prefer that. */
> +    if (gsi && kvm_xen_has_vcpu_callback_vector()) {
> +        in_kernel = kvm_xen_has_cap(EVTCHN_SEND);
> +        gsi = 0;
> +    }
> +

So this deals with setting the callback via after setting the upcall 
vector. What happens if the guest then disables the upcall vector (by 
setting it to zero)? Xen checks 'v->arch.hvm.evtchn_upcall_vector != 0' 
for every event delivery.

   Paul

Re: [PATCH 02/12] hw/xen: select kernel mode for per-vCPU event channel upcall vector

[David Woodhouse]

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

On Tue, 2023-10-24 at 13:29 +0100, Paul Durrant wrote:
> 
> > +    /* If the guest has set a per-vCPU callback vector, prefer that. */
> > +    if (gsi && kvm_xen_has_vcpu_callback_vector()) {
> > +        in_kernel = kvm_xen_has_cap(EVTCHN_SEND);
> > +        gsi = 0;
> > +    }
> > +
> 
> So this deals with setting the callback via after setting the upcall 
> vector. What happens if the guest then disables the upcall vector (by
> setting it to zero)? Xen checks 'v->arch.hvm.evtchn_upcall_vector != 0' 
> for every event delivery.

Qemu and KVM check before each delivery too. For a vCPU other than
vCPU0, if it isn't the per-vCPU lapic upcall vector, and it isn't the
system-wide vector, the interrupt isn't supposed to be delivered (the
GSI is tied to vCPU0).

However, we don't support dynamically disabling the kernel acceleration
(telling it to forget about the VIRQs, IPIs so that we can handle them
in userspace from now on). Not except for soft reset, when they're all
torn down anyway.

I don't really *want* to support that either. Better to make the kernel
mode unconditional, having it signal userspace via an eventfd when
vCPU0 has an upcall pending and it's GSI or INTx. But that's a *pair*
of eventfds, one for the resampler... and first I have to fix Qemu's
interrupt controller models to do the resampling properly on ack (qv).

Right now this works for all guests in practice and I have other yaks
to shave :)



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

[PATCH 03/12] include: update Xen public headers to Xen 4.17.2 release

[David Woodhouse]

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

... in order to advertise the XEN_HVM_CPUID_UPCALL_VECTOR feature,
which will come in a subsequent commit.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 hw/i386/kvm/xen_xenstore.c                    |  2 +-
 include/hw/xen/interface/arch-arm.h           | 37 +++++++-------
 include/hw/xen/interface/arch-x86/cpuid.h     | 31 +++++-------
 .../hw/xen/interface/arch-x86/xen-x86_32.h    | 19 +------
 .../hw/xen/interface/arch-x86/xen-x86_64.h    | 19 +------
 include/hw/xen/interface/arch-x86/xen.h       | 26 ++--------
 include/hw/xen/interface/event_channel.h      | 19 +------
 include/hw/xen/interface/features.h           | 19 +------
 include/hw/xen/interface/grant_table.h        | 19 +------
 include/hw/xen/interface/hvm/hvm_op.h         | 19 +------
 include/hw/xen/interface/hvm/params.h         | 19 +------
 include/hw/xen/interface/io/blkif.h           | 27 ++++------
 include/hw/xen/interface/io/console.h         | 19 +------
 include/hw/xen/interface/io/fbif.h            | 19 +------
 include/hw/xen/interface/io/kbdif.h           | 19 +------
 include/hw/xen/interface/io/netif.h           | 25 +++-------
 include/hw/xen/interface/io/protocols.h       | 19 +------
 include/hw/xen/interface/io/ring.h            | 49 ++++++++++---------
 include/hw/xen/interface/io/usbif.h           | 19 +------
 include/hw/xen/interface/io/xenbus.h          | 19 +------
 include/hw/xen/interface/io/xs_wire.h         | 36 ++++++--------
 include/hw/xen/interface/memory.h             | 30 +++++-------
 include/hw/xen/interface/physdev.h            | 23 ++-------
 include/hw/xen/interface/sched.h              | 19 +------
 include/hw/xen/interface/trace.h              | 19 +------
 include/hw/xen/interface/vcpu.h               | 19 +------
 include/hw/xen/interface/version.h            | 19 +------
 include/hw/xen/interface/xen-compat.h         | 19 +------
 include/hw/xen/interface/xen.h                | 19 +------
 29 files changed, 124 insertions(+), 523 deletions(-)

diff --git a/hw/i386/kvm/xen_xenstore.c b/hw/i386/kvm/xen_xenstore.c
index 660d0b72f9..d2b311109b 100644
--- a/hw/i386/kvm/xen_xenstore.c
+++ b/hw/i386/kvm/xen_xenstore.c
@@ -331,7 +331,7 @@ static void xs_error(XenXenstoreState *s, unsigned int id,
     const char *errstr = NULL;
 
     for (unsigned int i = 0; i < ARRAY_SIZE(xsd_errors); i++) {
-        struct xsd_errors *xsd_error = &xsd_errors[i];
+        const struct xsd_errors *xsd_error = &xsd_errors[i];
 
         if (xsd_error->errnum == errnum) {
             errstr = xsd_error->errstring;
diff --git a/include/hw/xen/interface/arch-arm.h b/include/hw/xen/interface/arch-arm.h
index 94b31511dd..1528ced509 100644
--- a/include/hw/xen/interface/arch-arm.h
+++ b/include/hw/xen/interface/arch-arm.h
@@ -1,26 +1,9 @@
+/* SPDX-License-Identifier: MIT */
 /******************************************************************************
  * arch-arm.h
  *
  * Guest OS interface to ARM Xen.
  *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
  * Copyright 2011 (C) Citrix Systems
  */
 
@@ -361,6 +344,7 @@ typedef uint64_t xen_callback_t;
 #define PSR_DBG_MASK    (1<<9)        /* arm64: Debug Exception mask */
 #define PSR_IT_MASK     (0x0600fc00)  /* Thumb If-Then Mask */
 #define PSR_JAZELLE     (1<<24)       /* Jazelle Mode */
+#define PSR_Z           (1<<30)       /* Zero condition flag */
 
 /* 32 bit modes */
 #define PSR_MODE_USR 0x10
@@ -383,7 +367,15 @@ typedef uint64_t xen_callback_t;
 #define PSR_MODE_EL1t 0x04
 #define PSR_MODE_EL0t 0x00
 
-#define PSR_GUEST32_INIT  (PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_SVC)
+/*
+ * We set PSR_Z to be able to boot Linux kernel versions with an invalid
+ * encoding of the first 8 NOP instructions. See commit a92882a4d270 in
+ * Linux.
+ *
+ * Note that PSR_Z is also set by U-Boot and QEMU -kernel when loading
+ * zImage kernels on aarch32.
+ */
+#define PSR_GUEST32_INIT (PSR_Z|PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_SVC)
 #define PSR_GUEST64_INIT (PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_EL1h)
 
 #define SCTLR_GUEST_INIT    xen_mk_ullong(0x00c50078)
@@ -398,6 +390,10 @@ typedef uint64_t xen_callback_t;
 
 /* Physical Address Space */
 
+/* Virtio MMIO mappings */
+#define GUEST_VIRTIO_MMIO_BASE   xen_mk_ullong(0x02000000)
+#define GUEST_VIRTIO_MMIO_SIZE   xen_mk_ullong(0x00100000)
+
 /*
  * vGIC mappings: Only one set of mapping is used by the guest.
  * Therefore they can overlap.
@@ -484,6 +480,9 @@ typedef uint64_t xen_callback_t;
 
 #define GUEST_VPL011_SPI        32
 
+#define GUEST_VIRTIO_MMIO_SPI_FIRST   33
+#define GUEST_VIRTIO_MMIO_SPI_LAST    43
+
 /* PSCI functions */
 #define PSCI_cpu_suspend 0
 #define PSCI_cpu_off     1
diff --git a/include/hw/xen/interface/arch-x86/cpuid.h b/include/hw/xen/interface/arch-x86/cpuid.h
index ce46305bee..7ecd16ae05 100644
--- a/include/hw/xen/interface/arch-x86/cpuid.h
+++ b/include/hw/xen/interface/arch-x86/cpuid.h
@@ -1,26 +1,9 @@
+/* SPDX-License-Identifier: MIT */
 /******************************************************************************
  * arch-x86/cpuid.h
  *
  * CPUID interface to Xen.
  *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
  * Copyright (c) 2007 Citrix Systems, Inc.
  *
  * Authors:
@@ -102,6 +85,18 @@
 #define XEN_HVM_CPUID_IOMMU_MAPPINGS   (1u << 2)
 #define XEN_HVM_CPUID_VCPU_ID_PRESENT  (1u << 3) /* vcpu id is present in EBX */
 #define XEN_HVM_CPUID_DOMID_PRESENT    (1u << 4) /* domid is present in ECX */
+/*
+ * With interrupt format set to 0 (non-remappable) bits 55:49 from the
+ * IO-APIC RTE and bits 11:5 from the MSI address can be used to store
+ * high bits for the Destination ID. This expands the Destination ID
+ * field from 8 to 15 bits, allowing to target APIC IDs up 32768.
+ */
+#define XEN_HVM_CPUID_EXT_DEST_ID      (1u << 5)
+/*
+ * Per-vCPU event channel upcalls work correctly with physical IRQs
+ * bound to event channels.
+ */
+#define XEN_HVM_CPUID_UPCALL_VECTOR    (1u << 6)
 
 /*
  * Leaf 6 (0x40000x05)
diff --git a/include/hw/xen/interface/arch-x86/xen-x86_32.h b/include/hw/xen/interface/arch-x86/xen-x86_32.h
index 19d7388633..139438e835 100644
--- a/include/hw/xen/interface/arch-x86/xen-x86_32.h
+++ b/include/hw/xen/interface/arch-x86/xen-x86_32.h
@@ -1,26 +1,9 @@
+/* SPDX-License-Identifier: MIT */
 /******************************************************************************
  * xen-x86_32.h
  *
  * Guest OS interface to x86 32-bit Xen.
  *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
  * Copyright (c) 2004-2007, K A Fraser
  */
 
diff --git a/include/hw/xen/interface/arch-x86/xen-x86_64.h b/include/hw/xen/interface/arch-x86/xen-x86_64.h
index 40aed14366..5d9035ed22 100644
--- a/include/hw/xen/interface/arch-x86/xen-x86_64.h
+++ b/include/hw/xen/interface/arch-x86/xen-x86_64.h
@@ -1,26 +1,9 @@
+/* SPDX-License-Identifier: MIT */
 /******************************************************************************
  * xen-x86_64.h
  *
  * Guest OS interface to x86 64-bit Xen.
  *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
  * Copyright (c) 2004-2006, K A Fraser
  */
 
diff --git a/include/hw/xen/interface/arch-x86/xen.h b/include/hw/xen/interface/arch-x86/xen.h
index 7acd94c8eb..c0f4551247 100644
--- a/include/hw/xen/interface/arch-x86/xen.h
+++ b/include/hw/xen/interface/arch-x86/xen.h
@@ -1,26 +1,9 @@
+/* SPDX-License-Identifier: MIT */
 /******************************************************************************
  * arch-x86/xen.h
  *
  * Guest OS interface to x86 Xen.
  *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
  * Copyright (c) 2004-2006, K A Fraser
  */
 
@@ -320,12 +303,9 @@ struct xen_arch_domainconfig {
     uint32_t misc_flags;
 };
 
-/* Location of online VCPU bitmap. */
-#define XEN_ACPI_CPU_MAP             0xaf00
-#define XEN_ACPI_CPU_MAP_LEN         ((HVM_MAX_VCPUS + 7) / 8)
+/* Max  XEN_X86_* constant. Used for ABI checking. */
+#define XEN_X86_MISC_FLAGS_MAX XEN_X86_MSR_RELAXED
 
-/* GPE0 bit set during CPU hotplug */
-#define XEN_ACPI_GPE0_CPUHP_BIT      2
 #endif
 
 /*
diff --git a/include/hw/xen/interface/event_channel.h b/include/hw/xen/interface/event_channel.h
index 73c9f38ce1..0d91a1c4af 100644
--- a/include/hw/xen/interface/event_channel.h
+++ b/include/hw/xen/interface/event_channel.h
@@ -1,26 +1,9 @@
+/* SPDX-License-Identifier: MIT */
 /******************************************************************************
  * event_channel.h
  *
  * Event channels between domains.
  *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
  * Copyright (c) 2003-2004, K A Fraser.
  */
 
diff --git a/include/hw/xen/interface/features.h b/include/hw/xen/interface/features.h
index 9ee2f760ef..d2a9175aae 100644
--- a/include/hw/xen/interface/features.h
+++ b/include/hw/xen/interface/features.h
@@ -1,26 +1,9 @@
+/* SPDX-License-Identifier: MIT */
 /******************************************************************************
  * features.h
  *
  * Feature flags, reported by XENVER_get_features.
  *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
  * Copyright (c) 2006, Keir Fraser <keir@xensource.com>
  */
 
diff --git a/include/hw/xen/interface/grant_table.h b/include/hw/xen/interface/grant_table.h
index 7934d7b718..1dfa17a6d0 100644
--- a/include/hw/xen/interface/grant_table.h
+++ b/include/hw/xen/interface/grant_table.h
@@ -1,27 +1,10 @@
+/* SPDX-License-Identifier: MIT */
 /******************************************************************************
  * grant_table.h
  *
  * Interface for granting foreign access to page frames, and receiving
  * page-ownership transfers.
  *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
  * Copyright (c) 2004, K A Fraser
  */
 
diff --git a/include/hw/xen/interface/hvm/hvm_op.h b/include/hw/xen/interface/hvm/hvm_op.h
index 870ec52060..e22adf0319 100644
--- a/include/hw/xen/interface/hvm/hvm_op.h
+++ b/include/hw/xen/interface/hvm/hvm_op.h
@@ -1,22 +1,5 @@
+/* SPDX-License-Identifier: MIT */
 /*
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
  * Copyright (c) 2007, Keir Fraser
  */
 
diff --git a/include/hw/xen/interface/hvm/params.h b/include/hw/xen/interface/hvm/params.h
index c9d6e70d7b..a22b4ed45d 100644
--- a/include/hw/xen/interface/hvm/params.h
+++ b/include/hw/xen/interface/hvm/params.h
@@ -1,22 +1,5 @@
+/* SPDX-License-Identifier: MIT */
 /*
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
  * Copyright (c) 2007, Keir Fraser
  */
 
diff --git a/include/hw/xen/interface/io/blkif.h b/include/hw/xen/interface/io/blkif.h
index 4cdba79aba..22f1eef0c0 100644
--- a/include/hw/xen/interface/io/blkif.h
+++ b/include/hw/xen/interface/io/blkif.h
@@ -1,26 +1,9 @@
+/* SPDX-License-Identifier: MIT */
 /******************************************************************************
  * blkif.h
  *
  * Unified block-device I/O interface for Xen guest OSes.
  *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
  * Copyright (c) 2003-2004, Keir Fraser
  * Copyright (c) 2012, Spectra Logic Corporation
  */
@@ -363,6 +346,14 @@
  *      that the frontend requires that the logical block size is 512 as it
  *      is hardcoded (which is the case in some frontend implementations).
  *
+ * trusted
+ *      Values:         0/1 (boolean)
+ *      Default value:  1
+ *
+ *      A value of "0" indicates that the frontend should not trust the
+ *      backend, and should deploy whatever measures available to protect from
+ *      a malicious backend on the other end.
+ *
  *------------------------- Virtual Device Properties -------------------------
  *
  * device-type
diff --git a/include/hw/xen/interface/io/console.h b/include/hw/xen/interface/io/console.h
index 4811f47220..4509b4b689 100644
--- a/include/hw/xen/interface/io/console.h
+++ b/include/hw/xen/interface/io/console.h
@@ -1,26 +1,9 @@
+/* SPDX-License-Identifier: MIT */
 /******************************************************************************
  * console.h
  *
  * Console I/O interface for Xen guest OSes.
  *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
  * Copyright (c) 2005, Keir Fraser
  */
 
diff --git a/include/hw/xen/interface/io/fbif.h b/include/hw/xen/interface/io/fbif.h
index cc25aab32e..93c73195d8 100644
--- a/include/hw/xen/interface/io/fbif.h
+++ b/include/hw/xen/interface/io/fbif.h
@@ -1,24 +1,7 @@
+/* SPDX-License-Identifier: MIT */
 /*
  * fbif.h -- Xen virtual frame buffer device
  *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
  * Copyright (C) 2005 Anthony Liguori <aliguori@us.ibm.com>
  * Copyright (C) 2006 Red Hat, Inc., Markus Armbruster <armbru@redhat.com>
  */
diff --git a/include/hw/xen/interface/io/kbdif.h b/include/hw/xen/interface/io/kbdif.h
index a6b01c52c7..4bde6b3821 100644
--- a/include/hw/xen/interface/io/kbdif.h
+++ b/include/hw/xen/interface/io/kbdif.h
@@ -1,24 +1,7 @@
+/* SPDX-License-Identifier: MIT */
 /*
  * kbdif.h -- Xen virtual keyboard/mouse
  *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
  * Copyright (C) 2005 Anthony Liguori <aliguori@us.ibm.com>
  * Copyright (C) 2006 Red Hat, Inc., Markus Armbruster <armbru@redhat.com>
  */
diff --git a/include/hw/xen/interface/io/netif.h b/include/hw/xen/interface/io/netif.h
index 00dd258712..c13b85061d 100644
--- a/include/hw/xen/interface/io/netif.h
+++ b/include/hw/xen/interface/io/netif.h
@@ -1,26 +1,9 @@
+/* SPDX-License-Identifier: MIT */
 /******************************************************************************
  * netif.h
  *
  * Unified network-device I/O interface for Xen guest OSes.
  *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
  * Copyright (c) 2003-2004, Keir Fraser
  */
 
@@ -160,6 +143,12 @@
  * be applied if it is set.
  */
 
+/*
+ * The setting of "trusted" node to "0" in the frontend path signals that the
+ * frontend should not trust the backend, and should deploy whatever measures
+ * available to protect from a malicious backend on the other end.
+ */
+
 /*
  * Control ring
  * ============
diff --git a/include/hw/xen/interface/io/protocols.h b/include/hw/xen/interface/io/protocols.h
index 52b4de0f81..7815e1ff0f 100644
--- a/include/hw/xen/interface/io/protocols.h
+++ b/include/hw/xen/interface/io/protocols.h
@@ -1,24 +1,7 @@
+/* SPDX-License-Identifier: MIT */
 /******************************************************************************
  * protocols.h
  *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
  * Copyright (c) 2008, Keir Fraser
  */
 
diff --git a/include/hw/xen/interface/io/ring.h b/include/hw/xen/interface/io/ring.h
index c486c457e0..025939278b 100644
--- a/include/hw/xen/interface/io/ring.h
+++ b/include/hw/xen/interface/io/ring.h
@@ -1,26 +1,9 @@
+/* SPDX-License-Identifier: MIT */
 /******************************************************************************
  * ring.h
  *
  * Shared producer-consumer ring macros.
  *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
  * Tim Deegan and Andrew Warfield November 2004.
  */
 
@@ -95,9 +78,8 @@ typedef unsigned int RING_IDX;
  * of the shared memory area (PAGE_SIZE, for instance). To initialise
  * the front half:
  *
- *     mytag_front_ring_t front_ring;
- *     SHARED_RING_INIT((mytag_sring_t *)shared_page);
- *     FRONT_RING_INIT(&front_ring, (mytag_sring_t *)shared_page, PAGE_SIZE);
+ *     mytag_front_ring_t ring;
+ *     XEN_FRONT_RING_INIT(&ring, (mytag_sring_t *)shared_page, PAGE_SIZE);
  *
  * Initializing the back follows similarly (note that only the front
  * initializes the shared ring):
@@ -184,6 +166,11 @@ typedef struct __name##_back_ring __name##_back_ring_t
 
 #define FRONT_RING_INIT(_r, _s, __size) FRONT_RING_ATTACH(_r, _s, 0, __size)
 
+#define XEN_FRONT_RING_INIT(r, s, size) do {                            \
+    SHARED_RING_INIT(s);                                                \
+    FRONT_RING_INIT(r, s, size);                                        \
+} while (0)
+
 #define BACK_RING_ATTACH(_r, _s, _i, __size) do {                       \
     (_r)->rsp_prod_pvt = (_i);                                          \
     (_r)->req_cons = (_i);                                              \
@@ -208,11 +195,11 @@ typedef struct __name##_back_ring __name##_back_ring_t
     (RING_FREE_REQUESTS(_r) == 0)
 
 /* Test if there are outstanding messages to be processed on a ring. */
-#define RING_HAS_UNCONSUMED_RESPONSES(_r)                               \
+#define XEN_RING_NR_UNCONSUMED_RESPONSES(_r)                            \
     ((_r)->sring->rsp_prod - (_r)->rsp_cons)
 
 #ifdef __GNUC__
-#define RING_HAS_UNCONSUMED_REQUESTS(_r) ({                             \
+#define XEN_RING_NR_UNCONSUMED_REQUESTS(_r) ({                          \
     unsigned int req = (_r)->sring->req_prod - (_r)->req_cons;          \
     unsigned int rsp = RING_SIZE(_r) -                                  \
         ((_r)->req_cons - (_r)->rsp_prod_pvt);                          \
@@ -220,13 +207,27 @@ typedef struct __name##_back_ring __name##_back_ring_t
 })
 #else
 /* Same as above, but without the nice GCC ({ ... }) syntax. */
-#define RING_HAS_UNCONSUMED_REQUESTS(_r)                                \
+#define XEN_RING_NR_UNCONSUMED_REQUESTS(_r)                             \
     ((((_r)->sring->req_prod - (_r)->req_cons) <                        \
       (RING_SIZE(_r) - ((_r)->req_cons - (_r)->rsp_prod_pvt))) ?        \
      ((_r)->sring->req_prod - (_r)->req_cons) :                         \
      (RING_SIZE(_r) - ((_r)->req_cons - (_r)->rsp_prod_pvt)))
 #endif
 
+#ifdef XEN_RING_HAS_UNCONSUMED_IS_BOOL
+/*
+ * These variants should only be used in case no caller is abusing them for
+ * obtaining the number of unconsumed responses/requests.
+ */
+#define RING_HAS_UNCONSUMED_RESPONSES(_r) \
+    (!!XEN_RING_NR_UNCONSUMED_RESPONSES(_r))
+#define RING_HAS_UNCONSUMED_REQUESTS(_r)  \
+    (!!XEN_RING_NR_UNCONSUMED_REQUESTS(_r))
+#else
+#define RING_HAS_UNCONSUMED_RESPONSES(_r) XEN_RING_NR_UNCONSUMED_RESPONSES(_r)
+#define RING_HAS_UNCONSUMED_REQUESTS(_r)  XEN_RING_NR_UNCONSUMED_REQUESTS(_r)
+#endif
+
 /* Direct access to individual ring elements, by index. */
 #define RING_GET_REQUEST(_r, _idx)                                      \
     (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].req))
diff --git a/include/hw/xen/interface/io/usbif.h b/include/hw/xen/interface/io/usbif.h
index c0a552e195..875af0dc7c 100644
--- a/include/hw/xen/interface/io/usbif.h
+++ b/include/hw/xen/interface/io/usbif.h
@@ -1,3 +1,4 @@
+/* SPDX-License-Identifier: MIT */
 /*
  * usbif.h
  *
@@ -5,24 +6,6 @@
  *
  * Copyright (C) 2009, FUJITSU LABORATORIES LTD.
  * Author: Noboru Iwamatsu <n_iwamatsu@jp.fujitsu.com>
- *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
  */
 
 #ifndef __XEN_PUBLIC_IO_USBIF_H__
diff --git a/include/hw/xen/interface/io/xenbus.h b/include/hw/xen/interface/io/xenbus.h
index 927f9db552..9cd0cd7c67 100644
--- a/include/hw/xen/interface/io/xenbus.h
+++ b/include/hw/xen/interface/io/xenbus.h
@@ -1,26 +1,9 @@
+/* SPDX-License-Identifier: MIT */
 /*****************************************************************************
  * xenbus.h
  *
  * Xenbus protocol details.
  *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
  * Copyright (C) 2005 XenSource Ltd.
  */
 
diff --git a/include/hw/xen/interface/io/xs_wire.h b/include/hw/xen/interface/io/xs_wire.h
index 4dd6632669..04e6849feb 100644
--- a/include/hw/xen/interface/io/xs_wire.h
+++ b/include/hw/xen/interface/io/xs_wire.h
@@ -1,25 +1,8 @@
+/* SPDX-License-Identifier: MIT */
 /*
  * Details of the "wire" protocol between Xen Store Daemon and client
  * library or guest kernel.
  *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
  * Copyright (C) 2005 Rusty Russell IBM Corporation
  */
 
@@ -71,11 +54,12 @@ struct xsd_errors
 #ifdef EINVAL
 #define XSD_ERROR(x) { x, #x }
 /* LINTED: static unused */
-static struct xsd_errors xsd_errors[]
+static const struct xsd_errors xsd_errors[]
 #if defined(__GNUC__)
 __attribute__((unused))
 #endif
     = {
+    /* /!\ New errors should be added at the end of the array. */
     XSD_ERROR(EINVAL),
     XSD_ERROR(EACCES),
     XSD_ERROR(EEXIST),
@@ -90,7 +74,8 @@ __attribute__((unused))
     XSD_ERROR(EBUSY),
     XSD_ERROR(EAGAIN),
     XSD_ERROR(EISCONN),
-    XSD_ERROR(E2BIG)
+    XSD_ERROR(E2BIG),
+    XSD_ERROR(EPERM),
 };
 #endif
 
@@ -124,6 +109,7 @@ struct xenstore_domain_interface {
     XENSTORE_RING_IDX rsp_cons, rsp_prod;
     uint32_t server_features; /* Bitmap of features supported by the server */
     uint32_t connection;
+    uint32_t error;
 };
 
 /* Violating this is very bad.  See docs/misc/xenstore.txt. */
@@ -135,10 +121,18 @@ struct xenstore_domain_interface {
 
 /* The ability to reconnect a ring */
 #define XENSTORE_SERVER_FEATURE_RECONNECTION 1
+/* The presence of the "error" field in the ring page */
+#define XENSTORE_SERVER_FEATURE_ERROR        2
 
 /* Valid values for the connection field */
 #define XENSTORE_CONNECTED 0 /* the steady-state */
-#define XENSTORE_RECONNECT 1 /* guest has initiated a reconnect */
+#define XENSTORE_RECONNECT 1 /* reconnect in progress */
+
+/* Valid values for the error field */
+#define XENSTORE_ERROR_NONE    0 /* No error */
+#define XENSTORE_ERROR_COMM    1 /* Communication problem */
+#define XENSTORE_ERROR_RINGIDX 2 /* Invalid ring index */
+#define XENSTORE_ERROR_PROTO   3 /* Protocol violation (payload too long) */
 
 #endif /* _XS_WIRE_H */
 
diff --git a/include/hw/xen/interface/memory.h b/include/hw/xen/interface/memory.h
index 383a9468c3..29cf5c8239 100644
--- a/include/hw/xen/interface/memory.h
+++ b/include/hw/xen/interface/memory.h
@@ -1,26 +1,9 @@
+/* SPDX-License-Identifier: MIT */
 /******************************************************************************
  * memory.h
  *
  * Memory reservation and information.
  *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
  * Copyright (c) 2005, Keir Fraser <keir@xensource.com>
  */
 
@@ -541,12 +524,14 @@ struct xen_mem_sharing_op {
                 uint32_t gref;     /* IN: gref to debug         */
             } u;
         } debug;
-        struct mem_sharing_op_fork {      /* OP_FORK */
+        struct mem_sharing_op_fork {      /* OP_FORK{,_RESET} */
             domid_t parent_domain;        /* IN: parent's domain id */
 /* Only makes sense for short-lived forks */
 #define XENMEM_FORK_WITH_IOMMU_ALLOWED (1u << 0)
 /* Only makes sense for short-lived forks */
 #define XENMEM_FORK_BLOCK_INTERRUPTS   (1u << 1)
+#define XENMEM_FORK_RESET_STATE        (1u << 2)
+#define XENMEM_FORK_RESET_MEMORY       (1u << 3)
             uint16_t flags;               /* IN: optional settings */
             uint32_t pad;                 /* Must be set to 0 */
         } fork;
@@ -662,6 +647,13 @@ struct xen_mem_acquire_resource {
      * two calls.
      */
     uint32_t nr_frames;
+    /*
+     * Padding field, must be zero on input.
+     * In a previous version this was an output field with the lowest bit
+     * named XENMEM_rsrc_acq_caller_owned. Future versions of this interface
+     * will not reuse this bit as an output with the field being zero on
+     * input.
+     */
     uint32_t pad;
     /*
      * IN - the index of the initial frame to be mapped. This parameter
diff --git a/include/hw/xen/interface/physdev.h b/include/hw/xen/interface/physdev.h
index d271766ad0..f0c0d4727c 100644
--- a/include/hw/xen/interface/physdev.h
+++ b/include/hw/xen/interface/physdev.h
@@ -1,22 +1,5 @@
+/* SPDX-License-Identifier: MIT */
 /*
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
  * Copyright (c) 2006, Keir Fraser
  */
 
@@ -211,8 +194,8 @@ struct physdev_manage_pci_ext {
     /* IN */
     uint8_t bus;
     uint8_t devfn;
-    unsigned is_extfn;
-    unsigned is_virtfn;
+    uint32_t is_extfn;
+    uint32_t is_virtfn;
     struct {
         uint8_t bus;
         uint8_t devfn;
diff --git a/include/hw/xen/interface/sched.h b/include/hw/xen/interface/sched.h
index 811bd87c82..b4362c6a1d 100644
--- a/include/hw/xen/interface/sched.h
+++ b/include/hw/xen/interface/sched.h
@@ -1,26 +1,9 @@
+/* SPDX-License-Identifier: MIT */
 /******************************************************************************
  * sched.h
  *
  * Scheduler state interactions
  *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
  * Copyright (c) 2005, Keir Fraser <keir@xensource.com>
  */
 
diff --git a/include/hw/xen/interface/trace.h b/include/hw/xen/interface/trace.h
index d5fa4aea8d..62a179971d 100644
--- a/include/hw/xen/interface/trace.h
+++ b/include/hw/xen/interface/trace.h
@@ -1,24 +1,7 @@
+/* SPDX-License-Identifier: MIT */
 /******************************************************************************
  * include/public/trace.h
  *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
  * Mark Williamson, (C) 2004 Intel Research Cambridge
  * Copyright (C) 2005 Bin Ren
  */
diff --git a/include/hw/xen/interface/vcpu.h b/include/hw/xen/interface/vcpu.h
index 3623af932f..81a3b3a743 100644
--- a/include/hw/xen/interface/vcpu.h
+++ b/include/hw/xen/interface/vcpu.h
@@ -1,26 +1,9 @@
+/* SPDX-License-Identifier: MIT */
 /******************************************************************************
  * vcpu.h
  *
  * VCPU initialisation, query, and hotplug.
  *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
  * Copyright (c) 2005, Keir Fraser <keir@xensource.com>
  */
 
diff --git a/include/hw/xen/interface/version.h b/include/hw/xen/interface/version.h
index 17a81e23cd..9c78b4f3b6 100644
--- a/include/hw/xen/interface/version.h
+++ b/include/hw/xen/interface/version.h
@@ -1,26 +1,9 @@
+/* SPDX-License-Identifier: MIT */
 /******************************************************************************
  * version.h
  *
  * Xen version, type, and compile information.
  *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
  * Copyright (c) 2005, Nguyen Anh Quynh <aquynh@gmail.com>
  * Copyright (c) 2005, Keir Fraser <keir@xensource.com>
  */
diff --git a/include/hw/xen/interface/xen-compat.h b/include/hw/xen/interface/xen-compat.h
index e1c027a95c..97fe698498 100644
--- a/include/hw/xen/interface/xen-compat.h
+++ b/include/hw/xen/interface/xen-compat.h
@@ -1,26 +1,9 @@
+/* SPDX-License-Identifier: MIT */
 /******************************************************************************
  * xen-compat.h
  *
  * Guest OS interface to Xen.  Compatibility layer.
  *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
  * Copyright (c) 2006, Christian Limpach
  */
 
diff --git a/include/hw/xen/interface/xen.h b/include/hw/xen/interface/xen.h
index e373592c33..920567e006 100644
--- a/include/hw/xen/interface/xen.h
+++ b/include/hw/xen/interface/xen.h
@@ -1,26 +1,9 @@
+/* SPDX-License-Identifier: MIT */
 /******************************************************************************
  * xen.h
  *
  * Guest OS interface to Xen.
  *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
  * Copyright (c) 2004, K A Fraser
  */
 
-- 
2.40.1

Re: [PATCH 03/12] include: update Xen public headers to Xen 4.17.2 release

[Paul Durrant]

On 16/10/2023 16:19, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> ... in order to advertise the XEN_HVM_CPUID_UPCALL_VECTOR feature,
> which will come in a subsequent commit.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>   hw/i386/kvm/xen_xenstore.c                    |  2 +-
>   include/hw/xen/interface/arch-arm.h           | 37 +++++++-------
>   include/hw/xen/interface/arch-x86/cpuid.h     | 31 +++++-------
>   .../hw/xen/interface/arch-x86/xen-x86_32.h    | 19 +------
>   .../hw/xen/interface/arch-x86/xen-x86_64.h    | 19 +------
>   include/hw/xen/interface/arch-x86/xen.h       | 26 ++--------
>   include/hw/xen/interface/event_channel.h      | 19 +------
>   include/hw/xen/interface/features.h           | 19 +------
>   include/hw/xen/interface/grant_table.h        | 19 +------
>   include/hw/xen/interface/hvm/hvm_op.h         | 19 +------
>   include/hw/xen/interface/hvm/params.h         | 19 +------
>   include/hw/xen/interface/io/blkif.h           | 27 ++++------
>   include/hw/xen/interface/io/console.h         | 19 +------
>   include/hw/xen/interface/io/fbif.h            | 19 +------
>   include/hw/xen/interface/io/kbdif.h           | 19 +------
>   include/hw/xen/interface/io/netif.h           | 25 +++-------
>   include/hw/xen/interface/io/protocols.h       | 19 +------
>   include/hw/xen/interface/io/ring.h            | 49 ++++++++++---------
>   include/hw/xen/interface/io/usbif.h           | 19 +------
>   include/hw/xen/interface/io/xenbus.h          | 19 +------
>   include/hw/xen/interface/io/xs_wire.h         | 36 ++++++--------
>   include/hw/xen/interface/memory.h             | 30 +++++-------
>   include/hw/xen/interface/physdev.h            | 23 ++-------
>   include/hw/xen/interface/sched.h              | 19 +------
>   include/hw/xen/interface/trace.h              | 19 +------
>   include/hw/xen/interface/vcpu.h               | 19 +------
>   include/hw/xen/interface/version.h            | 19 +------
>   include/hw/xen/interface/xen-compat.h         | 19 +------
>   include/hw/xen/interface/xen.h                | 19 +------
>   29 files changed, 124 insertions(+), 523 deletions(-)
> 

Acked-by: Paul Durrant <paul@xen.org>

[PATCH 04/12] i386/xen: advertise XEN_HVM_CPUID_UPCALL_VECTOR in CPUID

[David Woodhouse]

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

This will allow Linux guests (since v6.0) to use the per-vCPU upcall
vector delivered as MSI through the local APIC.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 target/i386/kvm/kvm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index f6c7f7e268..8bdbdcdffe 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -1887,6 +1887,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
                 c->eax |= XEN_HVM_CPUID_VCPU_ID_PRESENT;
                 c->ebx = cs->cpu_index;
             }
+
+            if (cs->kvm_state->xen_version >= XEN_VERSION(4, 17)) {
+                c->eax |= XEN_HVM_CPUID_UPCALL_VECTOR;
+            }
         }
 
         r = kvm_xen_init_vcpu(cs);
-- 
2.40.1

Re: [PATCH 04/12] i386/xen: advertise XEN_HVM_CPUID_UPCALL_VECTOR in CPUID

[Paul Durrant]

On 16/10/2023 16:19, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> This will allow Linux guests (since v6.0) to use the per-vCPU upcall
> vector delivered as MSI through the local APIC.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>   target/i386/kvm/kvm.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 

Reviewed-by: Paul Durrant <paul@xen.org>

[PATCH 05/12] hw/xen: populate store frontend nodes with XenStore PFN/port

[David Woodhouse]

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

This is kind of redundant since without being able to get these through
ome other method (HVMOP_get_param) the guest wouldn't be able to access
XenStore in order to find them. But Xen populates them, and it does
allow guests to *rebind* to the event channel port after a reset.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 hw/i386/kvm/xen_xenstore.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/i386/kvm/xen_xenstore.c b/hw/i386/kvm/xen_xenstore.c
index d2b311109b..3300e0614a 100644
--- a/hw/i386/kvm/xen_xenstore.c
+++ b/hw/i386/kvm/xen_xenstore.c
@@ -1432,6 +1432,7 @@ static void alloc_guest_port(XenXenstoreState *s)
 int xen_xenstore_reset(void)
 {
     XenXenstoreState *s = xen_xenstore_singleton;
+    GList *perms;
     int err;
 
     if (!s) {
@@ -1459,6 +1460,15 @@ int xen_xenstore_reset(void)
     }
     s->be_port = err;
 
+    /* Create frontend store nodes */
+    perms = g_list_append(NULL, xs_perm_as_string(XS_PERM_NONE, DOMID_QEMU));
+    perms = g_list_append(perms, xs_perm_as_string(XS_PERM_READ, xen_domid));
+
+    relpath_printf(s, perms, "store/ring-ref", "%lu", XEN_SPECIAL_PFN(XENSTORE));
+    relpath_printf(s, perms, "store/port", "%u", s->be_port);
+
+    g_list_free_full(perms, g_free);
+
     /*
      * We don't actually access the guest's page through the grant, because
      * this isn't real Xen, and we can just use the page we gave it in the
-- 
2.40.1

Re: [PATCH 05/12] hw/xen: populate store frontend nodes with XenStore PFN/port

[Paul Durrant]

On 16/10/2023 16:19, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> This is kind of redundant since without being able to get these through
> ome other method (HVMOP_get_param) the guest wouldn't be able to access

^ typo

> XenStore in order to find them. But Xen populates them, and it does
> allow guests to *rebind* to the event channel port after a reset.
> 

... although this can also be done by querying the remote end of the 
port before reset.

> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>   hw/i386/kvm/xen_xenstore.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 

Reviewed-by: Paul Durrant <paul@xen.org>

Re: [PATCH 05/12] hw/xen: populate store frontend nodes with XenStore PFN/port

[David Woodhouse]

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

On Tue, 2023-10-24 at 13:35 +0100, Paul Durrant wrote:
> On 16/10/2023 16:19, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > This is kind of redundant since without being able to get these
> > through
> > ome other method (HVMOP_get_param) the guest wouldn't be able to
> > access
> 
> ^ typo
> 
> > XenStore in order to find them. But Xen populates them, and it does
> > allow guests to *rebind* to the event channel port after a reset.
> > 
> 
> ... although this can also be done by querying the remote end of the 
> port before reset.
> 
I think you had to do that anyway; I don't think I was supposed to be
putting s->be_port in there anyway; it was supposed to be the
*frontend* port, and I've changed that in my tree.

I'll drop this whole sentence (and fix the typo).

> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> > ---
> >   hw/i386/kvm/xen_xenstore.c | 10 ++++++++++
> >   1 file changed, 10 insertions(+)
> > 
> 
> Reviewed-by: Paul Durrant <paul@xen.org>
> 


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

[PATCH 06/12] hw/xen: add get_frontend_path() method to XenDeviceClass

[David Woodhouse]

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

The primary Xen console is special. The guest's side is set up for it by
the toolstack automatically and not by the standard PV init sequence.

Accordingly, its *frontend* doesn't appear in …/device/console/0 either;
instead it appears under …/console in the guest's XenStore node.

To allow the Xen console driver to override the frontend path for the
primary console, add a method to the XenDeviceClass which can be used
instead of the standard xen_device_get_frontend_path()

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 hw/xen/xen-bus.c         | 10 +++++++++-
 include/hw/xen/xen-bus.h |  2 ++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index ece8ec40cd..cc524ed92c 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -711,8 +711,16 @@ static void xen_device_frontend_create(XenDevice *xendev, Error **errp)
 {
     ERRP_GUARD();
     XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
+    XenDeviceClass *xendev_class = XEN_DEVICE_GET_CLASS(xendev);
 
-    xendev->frontend_path = xen_device_get_frontend_path(xendev);
+    if (xendev_class->get_frontend_path) {
+        xendev->frontend_path = xendev_class->get_frontend_path(xendev, errp);
+        if (!xendev->frontend_path) {
+            return;
+        }
+    } else {
+        xendev->frontend_path = xen_device_get_frontend_path(xendev);
+    }
 
     /*
      * The frontend area may have already been created by a legacy
diff --git a/include/hw/xen/xen-bus.h b/include/hw/xen/xen-bus.h
index f435898164..eb440880b5 100644
--- a/include/hw/xen/xen-bus.h
+++ b/include/hw/xen/xen-bus.h
@@ -33,6 +33,7 @@ struct XenDevice {
 };
 typedef struct XenDevice XenDevice;
 
+typedef char *(*XenDeviceGetFrontendPath)(XenDevice *xendev, Error **errp);
 typedef char *(*XenDeviceGetName)(XenDevice *xendev, Error **errp);
 typedef void (*XenDeviceRealize)(XenDevice *xendev, Error **errp);
 typedef void (*XenDeviceFrontendChanged)(XenDevice *xendev,
@@ -46,6 +47,7 @@ struct XenDeviceClass {
     /*< public >*/
     const char *backend;
     const char *device;
+    XenDeviceGetFrontendPath get_frontend_path;
     XenDeviceGetName get_name;
     XenDeviceRealize realize;
     XenDeviceFrontendChanged frontend_changed;
-- 
2.40.1

Re: [PATCH 06/12] hw/xen: add get_frontend_path() method to XenDeviceClass

[Paul Durrant]

On 16/10/2023 16:19, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> The primary Xen console is special. The guest's side is set up for it by
> the toolstack automatically and not by the standard PV init sequence.
> 
> Accordingly, its *frontend* doesn't appear in …/device/console/0 either;
> instead it appears under …/console in the guest's XenStore node.
> 
> To allow the Xen console driver to override the frontend path for the
> primary console, add a method to the XenDeviceClass which can be used
> instead of the standard xen_device_get_frontend_path()
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>   hw/xen/xen-bus.c         | 10 +++++++++-
>   include/hw/xen/xen-bus.h |  2 ++
>   2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
> index ece8ec40cd..cc524ed92c 100644
> --- a/hw/xen/xen-bus.c
> +++ b/hw/xen/xen-bus.c
> @@ -711,8 +711,16 @@ static void xen_device_frontend_create(XenDevice *xendev, Error **errp)
>   {
>       ERRP_GUARD();
>       XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
> +    XenDeviceClass *xendev_class = XEN_DEVICE_GET_CLASS(xendev);
>   
> -    xendev->frontend_path = xen_device_get_frontend_path(xendev);
> +    if (xendev_class->get_frontend_path) {
> +        xendev->frontend_path = xendev_class->get_frontend_path(xendev, errp);
> +        if (!xendev->frontend_path) {
> +            return;

I think you need to update errp here to note that you are failing to 
create the frontend.

   Paul

Re: [PATCH 06/12] hw/xen: add get_frontend_path() method to XenDeviceClass

[David Woodhouse]

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

On Tue, 2023-10-24 at 13:42 +0100, Paul Durrant wrote:
> 
> > --- a/hw/xen/xen-bus.c
> > +++ b/hw/xen/xen-bus.c
> > @@ -711,8 +711,16 @@ static void xen_device_frontend_create(XenDevice *xendev, Error **errp)
> >    {
> >        ERRP_GUARD();
> >        XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
> > +    XenDeviceClass *xendev_class = XEN_DEVICE_GET_CLASS(xendev);
> >    
> > -    xendev->frontend_path = xen_device_get_frontend_path(xendev);
> > +    if (xendev_class->get_frontend_path) {
> > +        xendev->frontend_path = xendev_class->get_frontend_path(xendev, errp);
> > +        if (!xendev->frontend_path) {
> > +            return;
> 
> I think you need to update errp here to note that you are failing to 
> create the frontend.

If xendev_class->get_frontend_path returned NULL it will have filled in errp.

As a general rule (I'll be doing a bombing run on xen-bus once I get my
patch queue down into single digits) we should never check 'if (*errp)'
to check if a function had an error. It should *also* return a success
or failure indication, and we should cope with errp being NULL.



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

Re: [PATCH 06/12] hw/xen: add get_frontend_path() method to XenDeviceClass

[Paul Durrant]

On 24/10/2023 13:56, David Woodhouse wrote:
> On Tue, 2023-10-24 at 13:42 +0100, Paul Durrant wrote:
>>
>>> --- a/hw/xen/xen-bus.c
>>> +++ b/hw/xen/xen-bus.c
>>> @@ -711,8 +711,16 @@ static void xen_device_frontend_create(XenDevice *xendev, Error **errp)
>>>     {
>>>         ERRP_GUARD();
>>>         XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
>>> +    XenDeviceClass *xendev_class = XEN_DEVICE_GET_CLASS(xendev);
>>>     
>>> -    xendev->frontend_path = xen_device_get_frontend_path(xendev);
>>> +    if (xendev_class->get_frontend_path) {
>>> +        xendev->frontend_path = xendev_class->get_frontend_path(xendev, errp);
>>> +        if (!xendev->frontend_path) {
>>> +            return;
>>
>> I think you need to update errp here to note that you are failing to
>> create the frontend.
> 
> If xendev_class->get_frontend_path returned NULL it will have filled in errp.
> 

Ok, but a prepend to say that a lack of path there means we skip 
frontend creation seems reasonable?

> As a general rule (I'll be doing a bombing run on xen-bus once I get my
> patch queue down into single digits) we should never check 'if (*errp)'
> to check if a function had an error. It should *also* return a success
> or failure indication, and we should cope with errp being NULL.
> 

I'm pretty sure someone told me the exact opposite a few years back.

   Paul

Re: [PATCH 06/12] hw/xen: add get_frontend_path() method to XenDeviceClass

[David Woodhouse]

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

On Tue, 2023-10-24 at 13:59 +0100, Paul Durrant wrote:
> On 24/10/2023 13:56, David Woodhouse wrote:
> > On Tue, 2023-10-24 at 13:42 +0100, Paul Durrant wrote:
> > > 
> > > > --- a/hw/xen/xen-bus.c
> > > > +++ b/hw/xen/xen-bus.c
> > > > @@ -711,8 +711,16 @@ static void xen_device_frontend_create(XenDevice *xendev, Error **errp)
> > > >     {
> > > >         ERRP_GUARD();
> > > >         XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
> > > > +    XenDeviceClass *xendev_class = XEN_DEVICE_GET_CLASS(xendev);
> > > >     
> > > > -    xendev->frontend_path = xen_device_get_frontend_path(xendev);
> > > > +    if (xendev_class->get_frontend_path) {
> > > > +        xendev->frontend_path = xendev_class->get_frontend_path(xendev, errp);
> > > > +        if (!xendev->frontend_path) {
> > > > +            return;
> > > 
> > > I think you need to update errp here to note that you are failing to
> > > create the frontend.
> > 
> > If xendev_class->get_frontend_path returned NULL it will have filled in errp.
> > 
> 
> Ok, but a prepend to say that a lack of path there means we skip 
> frontend creation seems reasonable?

No, it *is* returning an error. Perhaps I can make it

    if (!xendev->frontend_path) {
        /*
         * If the ->get_frontend_path() method returned NULL, it will
         * already have set *errp accordingly. Return the error.
         */
        return /* false */;
    }


> > As a general rule (I'll be doing a bombing run on xen-bus once I get my
> > patch queue down into single digits) we should never check 'if (*errp)'
> > to check if a function had an error. It should *also* return a success
> > or failure indication, and we should cope with errp being NULL.
> > 
> 
> I'm pretty sure someone told me the exact opposite a few years back.

Then they were wrong :)

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

Re: [PATCH 06/12] hw/xen: add get_frontend_path() method to XenDeviceClass

[Paul Durrant]

On 24/10/2023 14:29, David Woodhouse wrote:
> On Tue, 2023-10-24 at 13:59 +0100, Paul Durrant wrote:
>> On 24/10/2023 13:56, David Woodhouse wrote:
>>> On Tue, 2023-10-24 at 13:42 +0100, Paul Durrant wrote:
>>>>
>>>>> --- a/hw/xen/xen-bus.c
>>>>> +++ b/hw/xen/xen-bus.c
>>>>> @@ -711,8 +711,16 @@ static void xen_device_frontend_create(XenDevice *xendev, Error **errp)
>>>>>      {
>>>>>          ERRP_GUARD();
>>>>>          XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
>>>>> +    XenDeviceClass *xendev_class = XEN_DEVICE_GET_CLASS(xendev);
>>>>>      
>>>>> -    xendev->frontend_path = xen_device_get_frontend_path(xendev);
>>>>> +    if (xendev_class->get_frontend_path) {
>>>>> +        xendev->frontend_path = xendev_class->get_frontend_path(xendev, errp);
>>>>> +        if (!xendev->frontend_path) {
>>>>> +            return;
>>>>
>>>> I think you need to update errp here to note that you are failing to
>>>> create the frontend.
>>>
>>> If xendev_class->get_frontend_path returned NULL it will have filled in errp.
>>>
>>
>> Ok, but a prepend to say that a lack of path there means we skip
>> frontend creation seems reasonable?
> 
> No, it *is* returning an error. Perhaps I can make it
> 

I understand it is returning an error. I thought the point of the 
cascading error handling was to prepend text at each (meaningful) layer 
such that the eventual message conveyed what failed and also what the 
consequences of that failure were.

   Paul

>      if (!xendev->frontend_path) {
>          /*
>           * If the ->get_frontend_path() method returned NULL, it will
>           * already have set *errp accordingly. Return the error.
>           */
>          return /* false */;
>      }
> 
> 
>>> As a general rule (I'll be doing a bombing run on xen-bus once I get my
>>> patch queue down into single digits) we should never check 'if (*errp)'
>>> to check if a function had an error. It should *also* return a success
>>> or failure indication, and we should cope with errp being NULL.
>>>
>>
>> I'm pretty sure someone told me the exact opposite a few years back.
> 
> Then they were wrong :)

Re: [PATCH 06/12] hw/xen: add get_frontend_path() method to XenDeviceClass

[David Woodhouse]

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

On Tue, 2023-10-24 at 14:37 +0100, Paul Durrant wrote:
> 
> > > Ok, but a prepend to say that a lack of path there means we skip
> > > frontend creation seems reasonable?
> > 
> > No, it *is* returning an error. Perhaps I can make it
> > 
> 
> I understand it is returning an error. I thought the point of the 
> cascading error handling was to prepend text at each (meaningful) layer 
> such that the eventual message conveyed what failed and also what the
> consequences of that failure were.

Oh, I see. Added; thanks.

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

Re: [PATCH 06/12] hw/xen: add get_frontend_path() method to XenDeviceClass

[David Woodhouse]

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

On Tue, 2023-10-24 at 14:29 +0100, David Woodhouse wrote:
> 
> > > As a general rule (I'll be doing a bombing run on xen-bus once I get my
> > > patch queue down into single digits) we should never check 'if (*errp)'
> > > to check if a function had an error. It should *also* return a success
> > > or failure indication, and we should cope with errp being NULL.
> > > 
> > 
> > I'm pretty sure someone told me the exact opposite a few years back.
> 
> Then they were wrong :)


cf. https://gitlab.com/qemu-project/qemu/-/commit/e3fe3988d7

"Whenever practical, also return a value that indicates success / failure"

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

[PATCH 07/12] hw/xen: update Xen console to XenDevice model

[David Woodhouse]

This allows (non-primary) console devices to be created on the command
line.

Signed-off-by: David Woodhouse <dwmw2@infradead.org>
---
 hw/char/trace-events        |   8 +
 hw/char/xen_console.c       | 502 +++++++++++++++++++++++++++---------
 hw/xen/xen-legacy-backend.c |   1 -
 3 files changed, 381 insertions(+), 130 deletions(-)

diff --git a/hw/char/trace-events b/hw/char/trace-events
index babf4d35ea..7a398c82a5 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -105,3 +105,11 @@ cadence_uart_baudrate(unsigned baudrate) "baudrate %u"
 # sh_serial.c
 sh_serial_read(char *id, unsigned size, uint64_t offs, uint64_t val) " %s size %d offs 0x%02" PRIx64 " -> 0x%02" PRIx64
 sh_serial_write(char *id, unsigned size, uint64_t offs, uint64_t val) "%s size %d offs 0x%02" PRIx64 " <- 0x%02" PRIx64
+
+# xen_console.c
+xen_console_connect(unsigned int idx, unsigned int ring_ref, unsigned int port, unsigned int limit) "idx %u ring_ref %u port %u limit %u"
+xen_console_disconnect(unsigned int idx) "idx %u"
+xen_console_unrealize(unsigned int idx) "idx %u"
+xen_console_realize(unsigned int idx, const char *chrdev) "idx %u chrdev %s"
+xen_console_device_create(unsigned int idx) "idx %u"
+xen_console_device_destroy(unsigned int idx) "idx %u"
diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index 810dae3f44..bd20be116c 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -20,15 +20,19 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/cutils.h"
 #include <sys/select.h>
 #include <termios.h>
 
 #include "qapi/error.h"
 #include "sysemu/sysemu.h"
 #include "chardev/char-fe.h"
-#include "hw/xen/xen-legacy-backend.h"
-
+#include "hw/xen/xen-backend.h"
+#include "hw/xen/xen-bus-helper.h"
+#include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
 #include "hw/xen/interface/io/console.h"
+#include "trace.h"
 
 struct buffer {
     uint8_t *data;
@@ -39,16 +43,22 @@ struct buffer {
 };
 
 struct XenConsole {
-    struct XenLegacyDevice  xendev;  /* must be first */
+    struct XenDevice  xendev;  /* must be first */
+    XenEventChannel   *event_channel;
+    int               dev;
     struct buffer     buffer;
-    char              console[XEN_BUFSIZE];
-    int               ring_ref;
+    char              *fe_path;
+    unsigned int      ring_ref;
     void              *sring;
     CharBackend       chr;
     int               backlog;
 };
+typedef struct XenConsole XenConsole;
+
+#define TYPE_XEN_CONSOLE_DEVICE "xen-console"
+OBJECT_DECLARE_SIMPLE_TYPE(XenConsole, XEN_CONSOLE_DEVICE)
 
-static void buffer_append(struct XenConsole *con)
+static bool buffer_append(XenConsole *con)
 {
     struct buffer *buffer = &con->buffer;
     XENCONS_RING_IDX cons, prod, size;
@@ -60,7 +70,7 @@ static void buffer_append(struct XenConsole *con)
 
     size = prod - cons;
     if ((size == 0) || (size > sizeof(intf->out)))
-        return;
+        return false;
 
     if ((buffer->capacity - buffer->size) < size) {
         buffer->capacity += (size + 1024);
@@ -73,7 +83,7 @@ static void buffer_append(struct XenConsole *con)
 
     xen_mb();
     intf->out_cons = cons;
-    xen_pv_send_notify(&con->xendev);
+    xen_device_notify_event_channel(XEN_DEVICE(con), con->event_channel, NULL);
 
     if (buffer->max_capacity &&
         buffer->size > buffer->max_capacity) {
@@ -89,6 +99,7 @@ static void buffer_append(struct XenConsole *con)
         if (buffer->consumed > buffer->max_capacity - over)
             buffer->consumed = buffer->max_capacity - over;
     }
+    return true;
 }
 
 static void buffer_advance(struct buffer *buffer, size_t len)
@@ -100,7 +111,7 @@ static void buffer_advance(struct buffer *buffer, size_t len)
     }
 }
 
-static int ring_free_bytes(struct XenConsole *con)
+static int ring_free_bytes(XenConsole *con)
 {
     struct xencons_interface *intf = con->sring;
     XENCONS_RING_IDX cons, prod, space;
@@ -118,13 +129,13 @@ static int ring_free_bytes(struct XenConsole *con)
 
 static int xencons_can_receive(void *opaque)
 {
-    struct XenConsole *con = opaque;
+    XenConsole *con = opaque;
     return ring_free_bytes(con);
 }
 
 static void xencons_receive(void *opaque, const uint8_t *buf, int len)
 {
-    struct XenConsole *con = opaque;
+    XenConsole *con = opaque;
     struct xencons_interface *intf = con->sring;
     XENCONS_RING_IDX prod;
     int i, max;
@@ -141,10 +152,10 @@ static void xencons_receive(void *opaque, const uint8_t *buf, int len)
     }
     xen_wmb();
     intf->in_prod = prod;
-    xen_pv_send_notify(&con->xendev);
+    xen_device_notify_event_channel(XEN_DEVICE(con), con->event_channel, NULL);
 }
 
-static void xencons_send(struct XenConsole *con)
+static bool xencons_send(XenConsole *con)
 {
     ssize_t len, size;
 
@@ -159,174 +170,407 @@ static void xencons_send(struct XenConsole *con)
     if (len < 1) {
         if (!con->backlog) {
             con->backlog = 1;
-            xen_pv_printf(&con->xendev, 1,
-                          "backlog piling up, nobody listening?\n");
         }
     } else {
         buffer_advance(&con->buffer, len);
         if (con->backlog && len == size) {
             con->backlog = 0;
-            xen_pv_printf(&con->xendev, 1, "backlog is gone\n");
         }
     }
+    return len > 0;
 }
 
 /* -------------------------------------------------------------------- */
 
-static int store_con_info(struct XenConsole *con)
+static bool con_event(void *_xendev)
 {
-    Chardev *cs = qemu_chr_fe_get_driver(&con->chr);
-    char *pts = NULL;
-    char *dom_path;
-    g_autoptr(GString) path = NULL;
+    XenConsole *con = XEN_CONSOLE_DEVICE(_xendev);
+    bool done_something;
 
-    /* Only continue if we're talking to a pty. */
-    if (!CHARDEV_IS_PTY(cs)) {
-        return 0;
-    }
-    pts = cs->filename + 4;
+    done_something = buffer_append(con);
 
-    dom_path = qemu_xen_xs_get_domain_path(xenstore, xen_domid);
-    if (!dom_path) {
-        return 0;
+    if (con->buffer.size - con->buffer.consumed) {
+        done_something |= xencons_send(con);
     }
+    return done_something;
+}
 
-    path = g_string_new(dom_path);
-    free(dom_path);
+/* -------------------------------------------------------------------- */
 
-    if (con->xendev.dev) {
-        g_string_append_printf(path, "/device/console/%d", con->xendev.dev);
-    } else {
-        g_string_append(path, "/console");
+static void xen_console_disconnect(XenDevice *xendev, Error **errp)
+{
+    XenConsole *con = XEN_CONSOLE_DEVICE(xendev);
+
+    qemu_chr_fe_set_handlers(&con->chr, NULL, NULL, NULL, NULL,
+                             con, NULL, true);
+
+
+    trace_xen_console_disconnect(con->dev);
+
+    if (con->event_channel) {
+        xen_device_unbind_event_channel(xendev, con->event_channel,
+                                        errp);
     }
-    g_string_append(path, "/tty");
 
-    if (xenstore_write_str(con->console, path->str, pts)) {
-        fprintf(stderr, "xenstore_write_str for '%s' fail", path->str);
-        return -1;
+    if (!con->sring) {
+        return;
+    }
+
+    if (!con->dev) {
+        qemu_xen_foreignmem_unmap(con->sring, 1);
+    } else {
+        xen_device_unmap_grant_refs(xendev, con->sring,
+                                    &con->ring_ref, 1, errp);
     }
-    return 0;
 }
 
-static int con_init(struct XenLegacyDevice *xendev)
+static void xen_console_connect(XenDevice *xendev, Error **errp)
 {
-    struct XenConsole *con = container_of(xendev, struct XenConsole, xendev);
-    char *type, *dom, label[32];
-    int ret = 0;
-    const char *output;
-
-    /* setup */
-    dom = qemu_xen_xs_get_domain_path(xenstore, con->xendev.dom);
-    if (!xendev->dev) {
-        snprintf(con->console, sizeof(con->console), "%s/console", dom);
-    } else {
-        snprintf(con->console, sizeof(con->console), "%s/device/console/%d", dom, xendev->dev);
+    XenConsole *con = XEN_CONSOLE_DEVICE(xendev);
+    unsigned int port, limit;
+
+    if (xen_device_frontend_scanf(xendev, "ring-ref", "%u",
+                                  &con->ring_ref) != 1) {
+        error_setg(errp, "failed to read ring-ref");
+        return;
     }
-    free(dom);
 
-    type = xenstore_read_str(con->console, "type");
-    if (!type || strcmp(type, "ioemu") != 0) {
-        xen_pv_printf(xendev, 1, "not for me (type=%s)\n", type);
-        ret = -1;
-        goto out;
+    if (xen_device_frontend_scanf(xendev, "port", "%u", &port) != 1) {
+        error_setg(errp, "failed to read remote port");
+        return;
     }
 
-    output = xenstore_read_str(con->console, "output");
+    if (xen_device_frontend_scanf(xendev, "limit", "%u", &limit) == 1) {
+        con->buffer.max_capacity = limit;
+    }
 
-    /* no Xen override, use qemu output device */
-    if (output == NULL) {
-        if (con->xendev.dev) {
-            qemu_chr_fe_init(&con->chr, serial_hd(con->xendev.dev),
-                             &error_abort);
+    if (!con->dev) {
+        xen_pfn_t mfn = (xen_pfn_t)con->ring_ref;
+        con->sring = qemu_xen_foreignmem_map(xendev->frontend_id, NULL,
+                                             PROT_READ | PROT_WRITE,
+                                             1, &mfn, NULL);
+        if (!con->sring) {
+            error_setg(errp, "failed to map console page");
+            return;
         }
     } else {
-        snprintf(label, sizeof(label), "xencons%d", con->xendev.dev);
-        qemu_chr_fe_init(&con->chr,
-                         /*
-                          * FIXME: sure we want to support implicit
-                          * muxed monitors here?
-                          */
-                         qemu_chr_new_mux_mon(label, output, NULL),
-                         &error_abort);
+        con->sring = xen_device_map_grant_refs(xendev,
+                                               &con->ring_ref, 1,
+                                               PROT_READ | PROT_WRITE,
+                                               errp);
+        if (!con->sring) {
+            error_prepend(errp, "failed to map grant ref: ");
+            return;
+        }
     }
 
-    store_con_info(con);
+    con->event_channel = xen_device_bind_event_channel(xendev, port,
+                                                       con_event,
+                                                       con,
+                                                       errp);
+    if (!con->event_channel) {
+        xen_console_disconnect(xendev, NULL);
+        return;
+    }
 
-out:
-    g_free(type);
-    return ret;
+    trace_xen_console_connect(con->dev, con->ring_ref, port,
+                              con->buffer.max_capacity);
+
+    qemu_chr_fe_set_handlers(&con->chr, xencons_can_receive,
+                             xencons_receive, NULL, NULL, con, NULL,
+                             true);
 }
 
-static int con_initialise(struct XenLegacyDevice *xendev)
+
+static void xen_console_frontend_changed(XenDevice *xendev,
+                                       enum xenbus_state frontend_state,
+                                       Error **errp)
 {
-    struct XenConsole *con = container_of(xendev, struct XenConsole, xendev);
-    int limit;
-
-    if (xenstore_read_int(con->console, "ring-ref", &con->ring_ref) == -1)
-        return -1;
-    if (xenstore_read_int(con->console, "port", &con->xendev.remote_port) == -1)
-        return -1;
-    if (xenstore_read_int(con->console, "limit", &limit) == 0)
-        con->buffer.max_capacity = limit;
+    ERRP_GUARD();
+    enum xenbus_state backend_state = xen_device_backend_get_state(xendev);
+
+    switch (frontend_state) {
+    case XenbusStateInitialised:
+    case XenbusStateConnected:
+        if (backend_state == XenbusStateConnected) {
+            break;
+        }
 
-    if (!xendev->dev) {
-        xen_pfn_t mfn = con->ring_ref;
-        con->sring = qemu_xen_foreignmem_map(con->xendev.dom, NULL,
-                                             PROT_READ | PROT_WRITE,
-                                             1, &mfn, NULL);
-    } else {
-        con->sring = xen_be_map_grant_ref(xendev, con->ring_ref,
-                                          PROT_READ | PROT_WRITE);
+        xen_console_disconnect(xendev, errp);
+        if (*errp) {
+            break;
+        }
+
+        xen_console_connect(xendev, errp);
+        if (*errp) {
+            break;
+        }
+
+        xen_device_backend_set_state(xendev, XenbusStateConnected);
+        break;
+
+    case XenbusStateClosing:
+        xen_device_backend_set_state(xendev, XenbusStateClosing);
+        break;
+
+    case XenbusStateClosed:
+    case XenbusStateUnknown:
+        xen_console_disconnect(xendev, errp);
+        if (*errp) {
+            break;
+        }
+
+        xen_device_backend_set_state(xendev, XenbusStateClosed);
+        break;
+
+    default:
+        break;
     }
-    if (!con->sring)
-        return -1;
+}
 
-    xen_be_bind_evtchn(&con->xendev);
-    qemu_chr_fe_set_handlers(&con->chr, xencons_can_receive,
-                             xencons_receive, NULL, NULL, con, NULL, true);
-
-    xen_pv_printf(xendev, 1,
-                  "ring mfn %d, remote port %d, local port %d, limit %zd\n",
-                  con->ring_ref,
-                  con->xendev.remote_port,
-                  con->xendev.local_port,
-                  con->buffer.max_capacity);
-    return 0;
+
+static char *xen_console_get_name(XenDevice *xendev, Error **errp)
+{
+    XenConsole *con = XEN_CONSOLE_DEVICE(xendev);
+
+    return g_strdup_printf("%u", con->dev);
 }
 
-static void con_disconnect(struct XenLegacyDevice *xendev)
+static void xen_console_unrealize(XenDevice *xendev)
 {
-    struct XenConsole *con = container_of(xendev, struct XenConsole, xendev);
+    XenConsole *con = XEN_CONSOLE_DEVICE(xendev);
+
+    trace_xen_console_unrealize(con->dev);
+
+    /* Disconnect from the frontend in case this has not already happened */
+    xen_console_disconnect(xendev, NULL);
 
     qemu_chr_fe_deinit(&con->chr, false);
-    xen_pv_unbind_evtchn(&con->xendev);
+}
+
+static void xen_console_realize(XenDevice *xendev, Error **errp)
+{
+    ERRP_GUARD();
+    XenConsole *con = XEN_CONSOLE_DEVICE(xendev);
+    Chardev *cs = qemu_chr_fe_get_driver(&con->chr);
+    unsigned int u;
+
+    if (!cs) {
+        error_setg(errp, "no backing character device");
+        return;
+    }
+
+    if (con->dev == -1) {
+        error_setg(errp, "no device index provided");
+        return;
+    }
 
-    if (con->sring) {
-        if (!xendev->dev) {
-            qemu_xen_foreignmem_unmap(con->sring, 1);
-        } else {
-            xen_be_unmap_grant_ref(xendev, con->sring, con->ring_ref);
+    /*
+     * The Xen primary console is special. The ring-ref is actually a GFN to
+     * be mapped directly as foreignmem (not a grant ref), and the guest port
+     * was allocated *for* the guest by the toolstack. The guest gets these
+     * through HVMOP_get_param and can use the console long before it's got
+     * XenStore up and running. We cannot create those for a Xen guest.
+     */
+    if (!con->dev) {
+        if (xen_device_frontend_scanf(xendev, "ring-ref", "%u", &u) != 1 ||
+            xen_device_frontend_scanf(xendev, "port", "%u", &u) != 1) {
+            error_setg(errp, "cannot create primary Xen console");
+            return;
         }
-        con->sring = NULL;
+    }
+
+    trace_xen_console_realize(con->dev, object_get_typename(OBJECT(cs)));
+
+    if (CHARDEV_IS_PTY(cs)) {
+        /* Strip the leading 'pty:' */
+        xen_device_frontend_printf(xendev, "tty", "%s", cs->filename + 4);
+    }
+
+    /* No normal PV driver initialization for the primary console */
+    if (!con->dev) {
+        xen_console_connect(xendev, errp);
+    }
+}
+
+static char *console_frontend_path(struct qemu_xs_handle *xenstore,
+                                   unsigned int dom_id, unsigned int dev)
+{
+    if (!dev) {
+        return g_strdup_printf("/local/domain/%u/console", dom_id);
+    } else {
+        return g_strdup_printf("/local/domain/%u/device/console/%u", dom_id, dev);
     }
 }
 
-static void con_event(struct XenLegacyDevice *xendev)
+static char *xen_console_get_frontend_path(XenDevice *xendev, Error **errp)
 {
-    struct XenConsole *con = container_of(xendev, struct XenConsole, xendev);
+    XenConsole *con = XEN_CONSOLE_DEVICE(xendev);
+    XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
+    char *ret = console_frontend_path(xenbus->xsh, xendev->frontend_id,
+                                      con->dev);
 
-    buffer_append(con);
-    if (con->buffer.size - con->buffer.consumed)
-        xencons_send(con);
+    if (!ret) {
+        error_setg(errp, "failed to create frontend path");
+    }
+    return ret;
 }
 
-/* -------------------------------------------------------------------- */
 
-struct XenDevOps xen_console_ops = {
-    .size       = sizeof(struct XenConsole),
-    .flags      = DEVOPS_FLAG_IGNORE_STATE|DEVOPS_FLAG_NEED_GNTDEV,
-    .init       = con_init,
-    .initialise = con_initialise,
-    .event      = con_event,
-    .disconnect = con_disconnect,
+static Property xen_console_properties[] = {
+    DEFINE_PROP_CHR("chardev", XenConsole, chr),
+    DEFINE_PROP_INT32("idx", XenConsole, dev, -1),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void xen_console_class_init(ObjectClass *class, void *data)
+{
+    DeviceClass *dev_class = DEVICE_CLASS(class);
+    XenDeviceClass *xendev_class = XEN_DEVICE_CLASS(class);
+
+    xendev_class->backend = "console";
+    xendev_class->device = "console";
+    xendev_class->get_name = xen_console_get_name;
+    xendev_class->realize = xen_console_realize;
+    xendev_class->frontend_changed = xen_console_frontend_changed;
+    xendev_class->unrealize = xen_console_unrealize;
+    xendev_class->get_frontend_path = xen_console_get_frontend_path;
+
+    device_class_set_props(dev_class, xen_console_properties);
+}
+
+static const TypeInfo xen_console_type_info = {
+    .name = TYPE_XEN_CONSOLE_DEVICE,
+    .parent = TYPE_XEN_DEVICE,
+    .instance_size = sizeof(XenConsole),
+    .class_init = xen_console_class_init,
+};
+
+static void xen_console_register_types(void)
+{
+    type_register_static(&xen_console_type_info);
+}
+
+type_init(xen_console_register_types)
+
+/* Called to instantiate a XenConsole when the backend is detected. */
+static void xen_console_device_create(XenBackendInstance *backend,
+                                      QDict *opts, Error **errp)
+{
+    ERRP_GUARD();
+    XenBus *xenbus = xen_backend_get_bus(backend);
+    const char *name = xen_backend_get_name(backend);
+    unsigned long number;
+    char *fe_path = NULL, *type = NULL, *output = NULL;
+    char label[32];
+    XenDevice *xendev = NULL;
+    XenConsole *con;
+    Chardev *cd = NULL;
+    struct qemu_xs_handle *xsh = xenbus->xsh;
+
+    if (qemu_strtoul(name, NULL, 10, &number)) {
+        error_setg(errp, "failed to parse name '%s'", name);
+        goto fail;
+    }
+
+    trace_xen_console_device_create(number);
+
+    fe_path = console_frontend_path(xsh, xen_domid, number);
+    if (fe_path == NULL) {
+        error_setg(errp, "failed to generate frontend path");
+        goto fail;
+    }
+
+    if (xs_node_scanf(xsh, XBT_NULL, fe_path, "type", errp, "%ms", &type) != 1) {
+        error_prepend(errp, "failed to read console device type: ");
+        goto fail;
+    }
+
+    if (strcmp(type, "ioemu")) {
+        error_setg(errp, "declining to handle console type '%s'",
+                   type);
+        goto fail;
+    }
+
+    xendev = XEN_DEVICE(qdev_new(TYPE_XEN_CONSOLE_DEVICE));
+    con = XEN_CONSOLE_DEVICE(xendev);
+
+    con->dev = number;
+
+    snprintf(label, sizeof(label), "xencons%ld", number);
+
+    if (xs_node_scanf(xsh, XBT_NULL, fe_path, "output", NULL, "%ms",
+                      &output) == 1) {
+        /*
+         * FIXME: sure we want to support implicit
+         * muxed monitors here?
+         */
+        cd = qemu_chr_new_mux_mon(label, output, NULL);
+        if (!cd) {
+            error_setg(errp, "console: No valid chardev found at '%s': ",
+                       output);
+            goto fail;
+        }
+    } else if (number) {
+        cd = serial_hd(number);
+        if (!cd) {
+            error_prepend(errp, "console: No serial device #%ld found: ",
+                          number);
+            goto fail;
+        }
+    } else {
+        /* No 'output' node on primary console: use null. */
+        cd = qemu_chr_new(label, "null", NULL);
+        if (!cd) {
+            error_setg(errp, "console: failed to create null device");
+            goto fail;
+        }
+    }
+
+    if (!qemu_chr_fe_init(&con->chr, cd, errp)) {
+        error_prepend(errp, "console: failed to initialize backing chardev: ");
+        goto fail;
+    }
+
+    if (qdev_realize_and_unref(DEVICE(xendev), BUS(xenbus), errp)) {
+        xendev = NULL;
+    } else {
+        error_prepend(errp, "realization of console device %lu failed: ",
+                      number);
+        goto fail;
+    }
+
+ fail:
+    if (xendev) {
+        object_unparent(OBJECT(xendev));
+    }
+
+    g_free(fe_path);
+    free(type);
+    free(output);
+}
+
+static void xen_console_device_destroy(XenBackendInstance *backend,
+                                       Error **errp)
+{
+    ERRP_GUARD();
+    XenDevice *xendev = xen_backend_get_device(backend);
+    XenConsole *con = XEN_CONSOLE_DEVICE(xendev);
+
+    trace_xen_console_device_destroy(con->dev);
+
+    object_unparent(OBJECT(xendev));
+}
+
+static const XenBackendInfo xen_console_backend_info  = {
+    .type = "console",
+    .create = xen_console_device_create,
+    .destroy = xen_console_device_destroy,
 };
+
+static void xen_console_register_backend(void)
+{
+    xen_backend_register(&xen_console_backend_info);
+}
+
+xen_backend_init(xen_console_register_backend);
diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
index 4ded3cec23..124dd5f3d6 100644
--- a/hw/xen/xen-legacy-backend.c
+++ b/hw/xen/xen-legacy-backend.c
@@ -623,7 +623,6 @@ void xen_be_init(void)
 
     xen_set_dynamic_sysbus();
 
-    xen_be_register("console", &xen_console_ops);
     xen_be_register("vkbd", &xen_kbdmouse_ops);
 #ifdef CONFIG_VIRTFS
     xen_be_register("9pfs", &xen_9pfs_ops);
-- 
2.40.1

Re: [PATCH 07/12] hw/xen: update Xen console to XenDevice model

[Paul Durrant]

On 16/10/2023 16:19, David Woodhouse wrote:
> This allows (non-primary) console devices to be created on the command
> line.
> 
> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> ---
>   hw/char/trace-events        |   8 +
>   hw/char/xen_console.c       | 502 +++++++++++++++++++++++++++---------
>   hw/xen/xen-legacy-backend.c |   1 -
>   3 files changed, 381 insertions(+), 130 deletions(-)
> 
> diff --git a/hw/char/trace-events b/hw/char/trace-events
> index babf4d35ea..7a398c82a5 100644
> --- a/hw/char/trace-events
> +++ b/hw/char/trace-events
> @@ -105,3 +105,11 @@ cadence_uart_baudrate(unsigned baudrate) "baudrate %u"
>   # sh_serial.c
>   sh_serial_read(char *id, unsigned size, uint64_t offs, uint64_t val) " %s size %d offs 0x%02" PRIx64 " -> 0x%02" PRIx64
>   sh_serial_write(char *id, unsigned size, uint64_t offs, uint64_t val) "%s size %d offs 0x%02" PRIx64 " <- 0x%02" PRIx64
> +
> +# xen_console.c
> +xen_console_connect(unsigned int idx, unsigned int ring_ref, unsigned int port, unsigned int limit) "idx %u ring_ref %u port %u limit %u"
> +xen_console_disconnect(unsigned int idx) "idx %u"
> +xen_console_unrealize(unsigned int idx) "idx %u"
> +xen_console_realize(unsigned int idx, const char *chrdev) "idx %u chrdev %s"
> +xen_console_device_create(unsigned int idx) "idx %u"
> +xen_console_device_destroy(unsigned int idx) "idx %u"
> diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
> index 810dae3f44..bd20be116c 100644
> --- a/hw/char/xen_console.c
> +++ b/hw/char/xen_console.c
> @@ -20,15 +20,19 @@
>    */
>   
>   #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
>   #include <sys/select.h>
>   #include <termios.h>
>   
>   #include "qapi/error.h"
>   #include "sysemu/sysemu.h"
>   #include "chardev/char-fe.h"
> -#include "hw/xen/xen-legacy-backend.h"
> -
> +#include "hw/xen/xen-backend.h"
> +#include "hw/xen/xen-bus-helper.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/qdev-properties-system.h"
>   #include "hw/xen/interface/io/console.h"
> +#include "trace.h"
>   
>   struct buffer {
>       uint8_t *data;
> @@ -39,16 +43,22 @@ struct buffer {
>   };
>   
>   struct XenConsole {
> -    struct XenLegacyDevice  xendev;  /* must be first */
> +    struct XenDevice  xendev;  /* must be first */
> +    XenEventChannel   *event_channel;
> +    int               dev;
>       struct buffer     buffer;
> -    char              console[XEN_BUFSIZE];
> -    int               ring_ref;
> +    char              *fe_path;
> +    unsigned int      ring_ref;
>       void              *sring;
>       CharBackend       chr;
>       int               backlog;
>   };
> +typedef struct XenConsole XenConsole;
> +
> +#define TYPE_XEN_CONSOLE_DEVICE "xen-console"
> +OBJECT_DECLARE_SIMPLE_TYPE(XenConsole, XEN_CONSOLE_DEVICE)
>   
> -static void buffer_append(struct XenConsole *con)
> +static bool buffer_append(XenConsole *con)
>   {
>       struct buffer *buffer = &con->buffer;
>       XENCONS_RING_IDX cons, prod, size;
> @@ -60,7 +70,7 @@ static void buffer_append(struct XenConsole *con)
>   
>       size = prod - cons;
>       if ((size == 0) || (size > sizeof(intf->out)))
> -        return;
> +        return false;
>   
>       if ((buffer->capacity - buffer->size) < size) {
>           buffer->capacity += (size + 1024);
> @@ -73,7 +83,7 @@ static void buffer_append(struct XenConsole *con)
>   
>       xen_mb();
>       intf->out_cons = cons;
> -    xen_pv_send_notify(&con->xendev);
> +    xen_device_notify_event_channel(XEN_DEVICE(con), con->event_channel, NULL);
>   
>       if (buffer->max_capacity &&
>           buffer->size > buffer->max_capacity) {
> @@ -89,6 +99,7 @@ static void buffer_append(struct XenConsole *con)
>           if (buffer->consumed > buffer->max_capacity - over)
>               buffer->consumed = buffer->max_capacity - over;
>       }
> +    return true;
>   }
>   
>   static void buffer_advance(struct buffer *buffer, size_t len)
> @@ -100,7 +111,7 @@ static void buffer_advance(struct buffer *buffer, size_t len)
>       }
>   }
>   
> -static int ring_free_bytes(struct XenConsole *con)
> +static int ring_free_bytes(XenConsole *con)
>   {
>       struct xencons_interface *intf = con->sring;
>       XENCONS_RING_IDX cons, prod, space;
> @@ -118,13 +129,13 @@ static int ring_free_bytes(struct XenConsole *con)
>   
>   static int xencons_can_receive(void *opaque)
>   {
> -    struct XenConsole *con = opaque;
> +    XenConsole *con = opaque;
>       return ring_free_bytes(con);
>   }
>   
>   static void xencons_receive(void *opaque, const uint8_t *buf, int len)
>   {
> -    struct XenConsole *con = opaque;
> +    XenConsole *con = opaque;
>       struct xencons_interface *intf = con->sring;
>       XENCONS_RING_IDX prod;
>       int i, max;
> @@ -141,10 +152,10 @@ static void xencons_receive(void *opaque, const uint8_t *buf, int len)
>       }
>       xen_wmb();
>       intf->in_prod = prod;
> -    xen_pv_send_notify(&con->xendev);
> +    xen_device_notify_event_channel(XEN_DEVICE(con), con->event_channel, NULL);
>   }
>   
> -static void xencons_send(struct XenConsole *con)
> +static bool xencons_send(XenConsole *con)
>   {
>       ssize_t len, size;
>   
> @@ -159,174 +170,407 @@ static void xencons_send(struct XenConsole *con)
>       if (len < 1) {
>           if (!con->backlog) {
>               con->backlog = 1;
> -            xen_pv_printf(&con->xendev, 1,
> -                          "backlog piling up, nobody listening?\n");
>           }
>       } else {
>           buffer_advance(&con->buffer, len);
>           if (con->backlog && len == size) {
>               con->backlog = 0;
> -            xen_pv_printf(&con->xendev, 1, "backlog is gone\n");
>           }
>       }
> +    return len > 0;
>   }
>   
>   /* -------------------------------------------------------------------- */
>   
> -static int store_con_info(struct XenConsole *con)
> +static bool con_event(void *_xendev)
>   {
> -    Chardev *cs = qemu_chr_fe_get_driver(&con->chr);
> -    char *pts = NULL;
> -    char *dom_path;
> -    g_autoptr(GString) path = NULL;
> +    XenConsole *con = XEN_CONSOLE_DEVICE(_xendev);
> +    bool done_something;
>   
> -    /* Only continue if we're talking to a pty. */
> -    if (!CHARDEV_IS_PTY(cs)) {
> -        return 0;
> -    }
> -    pts = cs->filename + 4;
> +    done_something = buffer_append(con);
>   
> -    dom_path = qemu_xen_xs_get_domain_path(xenstore, xen_domid);
> -    if (!dom_path) {
> -        return 0;
> +    if (con->buffer.size - con->buffer.consumed) {
> +        done_something |= xencons_send(con);
>       }
> +    return done_something;
> +}
>   
> -    path = g_string_new(dom_path);
> -    free(dom_path);
> +/* -------------------------------------------------------------------- */
>   
> -    if (con->xendev.dev) {
> -        g_string_append_printf(path, "/device/console/%d", con->xendev.dev);
> -    } else {
> -        g_string_append(path, "/console");
> +static void xen_console_disconnect(XenDevice *xendev, Error **errp)
> +{
> +    XenConsole *con = XEN_CONSOLE_DEVICE(xendev);
> +
> +    qemu_chr_fe_set_handlers(&con->chr, NULL, NULL, NULL, NULL,
> +                             con, NULL, true);
> +

nit: extraneous blank line by the looks of it.

With that fixed...

Reviewed-by: Paul Durrant <paul@xen.org>

[PATCH 08/12] hw/xen: do not repeatedly try to create a failing backend device

[David Woodhouse]

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

If xen_backend_device_create() fails to instantiate a device, the XenBus
code will just keep trying over and over again each time the bus is
re-enumerated, as long as the backend appears online and in
XenbusStateInitialising.

The only thing which prevents the XenBus code from recreating duplicates
of devices which already exist, is the fact that xen_device_realize()
sets the backend state to XenbusStateInitWait. If the attempt to create
the device doesn't get *that* far, that's when it will keep getting
retried.

My first thought was to handle errors by setting the backend state to
XenbusStateClosed, but that doesn't work for XenConsole which wants to
*ignore* any device of type != "ioemu" completely.

So, make xen_backend_device_create() *keep* the XenBackendInstance for a
failed device, and provide a new xen_backend_exists() function to allow
xen_bus_type_enumerate() to check whether one already exists before
creating a new one.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 hw/xen/xen-backend.c         | 27 +++++++++++++++++++++------
 hw/xen/xen-bus.c             |  3 ++-
 include/hw/xen/xen-backend.h |  1 +
 3 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/hw/xen/xen-backend.c b/hw/xen/xen-backend.c
index 5b0fb76eae..b9bf70a9f5 100644
--- a/hw/xen/xen-backend.c
+++ b/hw/xen/xen-backend.c
@@ -101,6 +101,24 @@ static XenBackendInstance *xen_backend_list_find(XenDevice *xendev)
     return NULL;
 }
 
+bool xen_backend_exists(const char *type, const char *name)
+{
+    const XenBackendImpl *impl = xen_backend_table_lookup(type);
+    XenBackendInstance *backend;
+
+    if (!impl) {
+        return false;
+    }
+
+    QLIST_FOREACH(backend, &backend_list, entry) {
+        if (backend->impl == impl && !strcmp(backend->name, name)) {
+            return true;
+        }
+    }
+
+    return false;
+}
+
 static void xen_backend_list_remove(XenBackendInstance *backend)
 {
     QLIST_REMOVE(backend, entry);
@@ -122,11 +140,6 @@ void xen_backend_device_create(XenBus *xenbus, const char *type,
     backend->name = g_strdup(name);
 
     impl->create(backend, opts, errp);
-    if (*errp) {
-        g_free(backend->name);
-        g_free(backend);
-        return;
-    }
 
     backend->impl = impl;
     xen_backend_list_add(backend);
@@ -165,7 +178,9 @@ bool xen_backend_try_device_destroy(XenDevice *xendev, Error **errp)
     }
 
     impl = backend->impl;
-    impl->destroy(backend, errp);
+    if (backend->xendev) {
+        impl->destroy(backend, errp);
+    }
 
     xen_backend_list_remove(backend);
     g_free(backend->name);
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index cc524ed92c..0da2aa219a 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -209,7 +209,8 @@ static void xen_bus_type_enumerate(XenBus *xenbus, const char *type)
                           NULL, "%u", &online) != 1)
             online = 0;
 
-        if (online && state == XenbusStateInitialising) {
+        if (online && state == XenbusStateInitialising &&
+            !xen_backend_exists(type, backend[i])) {
             Error *local_err = NULL;
 
             xen_bus_backend_create(xenbus, type, backend[i], backend_path,
diff --git a/include/hw/xen/xen-backend.h b/include/hw/xen/xen-backend.h
index aac2fd454d..0f01631ae7 100644
--- a/include/hw/xen/xen-backend.h
+++ b/include/hw/xen/xen-backend.h
@@ -33,6 +33,7 @@ XenDevice *xen_backend_get_device(XenBackendInstance *backend);
 void xen_backend_register(const XenBackendInfo *info);
 const char **xen_backend_get_types(unsigned int *nr);
 
+bool xen_backend_exists(const char *type, const char *name);
 void xen_backend_device_create(XenBus *xenbus, const char *type,
                                const char *name, QDict *opts, Error **errp);
 bool xen_backend_try_device_destroy(XenDevice *xendev, Error **errp);
-- 
2.40.1

Re: [PATCH 08/12] hw/xen: do not repeatedly try to create a failing backend device

[Paul Durrant]

On 16/10/2023 16:19, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> If xen_backend_device_create() fails to instantiate a device, the XenBus
> code will just keep trying over and over again each time the bus is
> re-enumerated, as long as the backend appears online and in
> XenbusStateInitialising.
> 
> The only thing which prevents the XenBus code from recreating duplicates
> of devices which already exist, is the fact that xen_device_realize()
> sets the backend state to XenbusStateInitWait. If the attempt to create
> the device doesn't get *that* far, that's when it will keep getting
> retried.
> 
> My first thought was to handle errors by setting the backend state to
> XenbusStateClosed, but that doesn't work for XenConsole which wants to
> *ignore* any device of type != "ioemu" completely.
> 
> So, make xen_backend_device_create() *keep* the XenBackendInstance for a
> failed device, and provide a new xen_backend_exists() function to allow
> xen_bus_type_enumerate() to check whether one already exists before
> creating a new one.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>   hw/xen/xen-backend.c         | 27 +++++++++++++++++++++------
>   hw/xen/xen-bus.c             |  3 ++-
>   include/hw/xen/xen-backend.h |  1 +
>   3 files changed, 24 insertions(+), 7 deletions(-)
> 

Reviewed-by: Paul Durrant <paul@xen.org>

[PATCH 09/12] hw/xen: prevent duplicate device registrations

[David Woodhouse]

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

Ensure that we have a XenBackendInstance for every device regardless
of whether it was "discovered" in XenStore or created directly in QEMU.

This allows the backend_list to be a source of truth about whether a
given backend exists, and allows us to reject duplicates.

This also cleans up the fact that backend drivers were calling
xen_backend_set_device() with a XenDevice immediately after calling
qdev_realize_and_unref() on it, when it wasn't theirs to play with any
more.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 hw/block/xen-block.c         |  1 -
 hw/char/xen_console.c        |  2 +-
 hw/xen/xen-backend.c         | 78 ++++++++++++++++++++++++++----------
 hw/xen/xen-bus.c             |  8 ++++
 include/hw/xen/xen-backend.h |  3 ++
 5 files changed, 69 insertions(+), 23 deletions(-)

diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index a07cd7eb5d..9262338535 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -975,7 +975,6 @@ static void xen_block_device_create(XenBackendInstance *backend,
         goto fail;
     }
 
-    xen_backend_set_device(backend, xendev);
     return;
 
 fail:
diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index bd20be116c..2825b8c511 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -468,7 +468,7 @@ static void xen_console_device_create(XenBackendInstance *backend,
     Chardev *cd = NULL;
     struct qemu_xs_handle *xsh = xenbus->xsh;
 
-    if (qemu_strtoul(name, NULL, 10, &number)) {
+    if (qemu_strtoul(name, NULL, 10, &number) || number >= INT_MAX) {
         error_setg(errp, "failed to parse name '%s'", name);
         goto fail;
     }
diff --git a/hw/xen/xen-backend.c b/hw/xen/xen-backend.c
index b9bf70a9f5..dcb4329258 100644
--- a/hw/xen/xen-backend.c
+++ b/hw/xen/xen-backend.c
@@ -101,22 +101,28 @@ static XenBackendInstance *xen_backend_list_find(XenDevice *xendev)
     return NULL;
 }
 
-bool xen_backend_exists(const char *type, const char *name)
+static XenBackendInstance *xen_backend_lookup(const XenBackendImpl *impl, const char *name)
 {
-    const XenBackendImpl *impl = xen_backend_table_lookup(type);
     XenBackendInstance *backend;
 
-    if (!impl) {
-        return false;
-    }
-
     QLIST_FOREACH(backend, &backend_list, entry) {
         if (backend->impl == impl && !strcmp(backend->name, name)) {
-            return true;
+            return backend;
         }
     }
 
-    return false;
+    return NULL;
+}
+
+bool xen_backend_exists(const char *type, const char *name)
+{
+    const XenBackendImpl *impl = xen_backend_table_lookup(type);
+
+    if (!impl) {
+        return false;
+    }
+
+    return !!xen_backend_lookup(impl, name);
 }
 
 static void xen_backend_list_remove(XenBackendInstance *backend)
@@ -138,11 +144,10 @@ void xen_backend_device_create(XenBus *xenbus, const char *type,
     backend = g_new0(XenBackendInstance, 1);
     backend->xenbus = xenbus;
     backend->name = g_strdup(name);
-
-    impl->create(backend, opts, errp);
-
     backend->impl = impl;
     xen_backend_list_add(backend);
+
+    impl->create(backend, opts, errp);
 }
 
 XenBus *xen_backend_get_bus(XenBackendInstance *backend)
@@ -155,13 +160,6 @@ const char *xen_backend_get_name(XenBackendInstance *backend)
     return backend->name;
 }
 
-void xen_backend_set_device(XenBackendInstance *backend,
-                            XenDevice *xendev)
-{
-    g_assert(!backend->xendev);
-    backend->xendev = xendev;
-}
-
 XenDevice *xen_backend_get_device(XenBackendInstance *backend)
 {
     return backend->xendev;
@@ -178,9 +176,7 @@ bool xen_backend_try_device_destroy(XenDevice *xendev, Error **errp)
     }
 
     impl = backend->impl;
-    if (backend->xendev) {
-        impl->destroy(backend, errp);
-    }
+    impl->destroy(backend, errp);
 
     xen_backend_list_remove(backend);
     g_free(backend->name);
@@ -188,3 +184,43 @@ bool xen_backend_try_device_destroy(XenDevice *xendev, Error **errp)
 
     return true;
 }
+
+bool xen_backend_device_realized(XenDevice *xendev, Error **errp)
+{
+    XenDeviceClass *xendev_class = XEN_DEVICE_GET_CLASS(xendev);
+    const char *type = xendev_class->backend ? : object_get_typename(OBJECT(xendev));
+    const XenBackendImpl *impl = xen_backend_table_lookup(type);
+    XenBackendInstance *backend;
+
+    if (!impl) {
+        return false;
+    }
+
+    backend = xen_backend_lookup(impl, xendev->name);
+    if (backend) {
+        if (backend->xendev && backend->xendev != xendev) {
+            error_setg(errp, "device %s/%s already exists", type, xendev->name);
+            return false;
+        }
+        backend->xendev = xendev;
+        return true;
+    }
+
+    backend = g_new0(XenBackendInstance, 1);
+    backend->xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
+    backend->xendev = xendev;
+    backend->name = g_strdup(xendev->name);
+    backend->impl = impl;
+
+    xen_backend_list_add(backend);
+    return true;
+}
+
+void xen_backend_device_unrealized(XenDevice *xendev)
+{
+    XenBackendInstance *backend = xen_backend_list_find(xendev);
+
+    if (backend) {
+        backend->xendev = NULL;
+    }
+}
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index 0da2aa219a..0b232d1f94 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -359,6 +359,8 @@ static void xen_bus_realize(BusState *bus, Error **errp)
 
     g_free(type);
     g_free(key);
+
+    xen_bus_enumerate(xenbus);
     return;
 
 fail:
@@ -958,6 +960,8 @@ static void xen_device_unrealize(DeviceState *dev)
 
     trace_xen_device_unrealize(type, xendev->name);
 
+    xen_backend_device_unrealized(xendev);
+
     if (xendev->exit.notify) {
         qemu_remove_exit_notifier(&xendev->exit);
         xendev->exit.notify = NULL;
@@ -1024,6 +1028,10 @@ static void xen_device_realize(DeviceState *dev, Error **errp)
         goto unrealize;
     }
 
+    if (!xen_backend_device_realized(xendev, errp)) {
+        goto unrealize;
+    }
+
     trace_xen_device_realize(type, xendev->name);
 
     xendev->xsh = qemu_xen_xs_open();
diff --git a/include/hw/xen/xen-backend.h b/include/hw/xen/xen-backend.h
index 0f01631ae7..3f1e764c51 100644
--- a/include/hw/xen/xen-backend.h
+++ b/include/hw/xen/xen-backend.h
@@ -38,4 +38,7 @@ void xen_backend_device_create(XenBus *xenbus, const char *type,
                                const char *name, QDict *opts, Error **errp);
 bool xen_backend_try_device_destroy(XenDevice *xendev, Error **errp);
 
+bool xen_backend_device_realized(XenDevice *xendev, Error **errp);
+void xen_backend_device_unrealized(XenDevice *xendev);
+
 #endif /* HW_XEN_BACKEND_H */
-- 
2.40.1

Re: [PATCH 09/12] hw/xen: prevent duplicate device registrations

[Paul Durrant]

On 16/10/2023 16:19, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> Ensure that we have a XenBackendInstance for every device regardless
> of whether it was "discovered" in XenStore or created directly in QEMU.
> 
> This allows the backend_list to be a source of truth about whether a
> given backend exists, and allows us to reject duplicates.
> 
> This also cleans up the fact that backend drivers were calling
> xen_backend_set_device() with a XenDevice immediately after calling
> qdev_realize_and_unref() on it, when it wasn't theirs to play with any
> more.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>   hw/block/xen-block.c         |  1 -
>   hw/char/xen_console.c        |  2 +-
>   hw/xen/xen-backend.c         | 78 ++++++++++++++++++++++++++----------
>   hw/xen/xen-bus.c             |  8 ++++
>   include/hw/xen/xen-backend.h |  3 ++
>   5 files changed, 69 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> index a07cd7eb5d..9262338535 100644
> --- a/hw/block/xen-block.c
> +++ b/hw/block/xen-block.c
> @@ -975,7 +975,6 @@ static void xen_block_device_create(XenBackendInstance *backend,
>           goto fail;
>       }
>   
> -    xen_backend_set_device(backend, xendev);
>       return;
>   
>   fail:
> diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
> index bd20be116c..2825b8c511 100644
> --- a/hw/char/xen_console.c
> +++ b/hw/char/xen_console.c
> @@ -468,7 +468,7 @@ static void xen_console_device_create(XenBackendInstance *backend,
>       Chardev *cd = NULL;
>       struct qemu_xs_handle *xsh = xenbus->xsh;
>   
> -    if (qemu_strtoul(name, NULL, 10, &number)) {
> +    if (qemu_strtoul(name, NULL, 10, &number) || number >= INT_MAX) {
>           error_setg(errp, "failed to parse name '%s'", name);
>           goto fail;
>       }
I don't think this hunk belongs here, does it? Seems like it should be 
in patch 7.

> diff --git a/hw/xen/xen-backend.c b/hw/xen/xen-backend.c
> index b9bf70a9f5..dcb4329258 100644
> --- a/hw/xen/xen-backend.c
> +++ b/hw/xen/xen-backend.c
> @@ -101,22 +101,28 @@ static XenBackendInstance *xen_backend_list_find(XenDevice *xendev)
>       return NULL;
>   }
>   
> -bool xen_backend_exists(const char *type, const char *name)
> +static XenBackendInstance *xen_backend_lookup(const XenBackendImpl *impl, const char *name)

This name is a little close to xen_backend_table_lookup()... perhaps 
that one should be renamed xen_backend_impl_lookup() for clarity.

>   {
> -    const XenBackendImpl *impl = xen_backend_table_lookup(type);
>       XenBackendInstance *backend;
>   
> -    if (!impl) {
> -        return false;
> -    }
> -
>       QLIST_FOREACH(backend, &backend_list, entry) {
>           if (backend->impl == impl && !strcmp(backend->name, name)) {
> -            return true;
> +            return backend;
>           }
>       }
>   
> -    return false;
> +    return NULL;
> +}
> +
> +bool xen_backend_exists(const char *type, const char *name)
> +{
> +    const XenBackendImpl *impl = xen_backend_table_lookup(type);
> +
> +    if (!impl) {
> +        return false;
> +    }
> +
> +    return !!xen_backend_lookup(impl, name);
>   }
>   
>   static void xen_backend_list_remove(XenBackendInstance *backend)
> @@ -138,11 +144,10 @@ void xen_backend_device_create(XenBus *xenbus, const char *type,
>       backend = g_new0(XenBackendInstance, 1);
>       backend->xenbus = xenbus;
>       backend->name = g_strdup(name);
> -
> -    impl->create(backend, opts, errp);
> -
>       backend->impl = impl;
>       xen_backend_list_add(backend);
> +
> +    impl->create(backend, opts, errp);
>   }
>   
>   XenBus *xen_backend_get_bus(XenBackendInstance *backend)
> @@ -155,13 +160,6 @@ const char *xen_backend_get_name(XenBackendInstance *backend)
>       return backend->name;
>   }
>   
> -void xen_backend_set_device(XenBackendInstance *backend,
> -                            XenDevice *xendev)
> -{
> -    g_assert(!backend->xendev);
> -    backend->xendev = xendev;
> -}
> -

The declaration also needs removing from xen-backend.h presumably.

>   XenDevice *xen_backend_get_device(XenBackendInstance *backend)
>   {
>       return backend->xendev;
> @@ -178,9 +176,7 @@ bool xen_backend_try_device_destroy(XenDevice *xendev, Error **errp)
>       }
>   
>       impl = backend->impl;
> -    if (backend->xendev) {
> -        impl->destroy(backend, errp);
> -    }
> +    impl->destroy(backend, errp);
>   
>       xen_backend_list_remove(backend);
>       g_free(backend->name);
> @@ -188,3 +184,43 @@ bool xen_backend_try_device_destroy(XenDevice *xendev, Error **errp)
>   
>       return true;
>   }
> +
> +bool xen_backend_device_realized(XenDevice *xendev, Error **errp)
> +{
> +    XenDeviceClass *xendev_class = XEN_DEVICE_GET_CLASS(xendev);
> +    const char *type = xendev_class->backend ? : object_get_typename(OBJECT(xendev));
> +    const XenBackendImpl *impl = xen_backend_table_lookup(type);
> +    XenBackendInstance *backend;
> +
> +    if (!impl) {
> +        return false;
> +    }
> +
> +    backend = xen_backend_lookup(impl, xendev->name);
> +    if (backend) {
> +        if (backend->xendev && backend->xendev != xendev) {
> +            error_setg(errp, "device %s/%s already exists", type, xendev->name);
> +            return false;
> +        }
> +        backend->xendev = xendev;
> +        return true;
> +    }
> +
> +    backend = g_new0(XenBackendInstance, 1);
> +    backend->xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
> +    backend->xendev = xendev;
> +    backend->name = g_strdup(xendev->name);
> +    backend->impl = impl;
> +
> +    xen_backend_list_add(backend);
> +    return true;
> +}

Not sure I'm getting my head around this. How does this play with the 
legacy backend code? The point about having the impl list was so that 
the newer xenbus code didn't conflict with the legacy backend code, 
which thinks it has carte blanche ownership of a class.

   Paul

> +
> +void xen_backend_device_unrealized(XenDevice *xendev)
> +{
> +    XenBackendInstance *backend = xen_backend_list_find(xendev);
> +
> +    if (backend) {
> +        backend->xendev = NULL;
> +    }
> +}
> diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
> index 0da2aa219a..0b232d1f94 100644
> --- a/hw/xen/xen-bus.c
> +++ b/hw/xen/xen-bus.c
> @@ -359,6 +359,8 @@ static void xen_bus_realize(BusState *bus, Error **errp)
>   
>       g_free(type);
>       g_free(key);
> +
> +    xen_bus_enumerate(xenbus);
>       return;
>   
>   fail:
> @@ -958,6 +960,8 @@ static void xen_device_unrealize(DeviceState *dev)
>   
>       trace_xen_device_unrealize(type, xendev->name);
>   
> +    xen_backend_device_unrealized(xendev);
> +
>       if (xendev->exit.notify) {
>           qemu_remove_exit_notifier(&xendev->exit);
>           xendev->exit.notify = NULL;
> @@ -1024,6 +1028,10 @@ static void xen_device_realize(DeviceState *dev, Error **errp)
>           goto unrealize;
>       }
>   
> +    if (!xen_backend_device_realized(xendev, errp)) {
> +        goto unrealize;
> +    }
> +
>       trace_xen_device_realize(type, xendev->name);
>   
>       xendev->xsh = qemu_xen_xs_open();
> diff --git a/include/hw/xen/xen-backend.h b/include/hw/xen/xen-backend.h
> index 0f01631ae7..3f1e764c51 100644
> --- a/include/hw/xen/xen-backend.h
> +++ b/include/hw/xen/xen-backend.h
> @@ -38,4 +38,7 @@ void xen_backend_device_create(XenBus *xenbus, const char *type,
>                                  const char *name, QDict *opts, Error **errp);
>   bool xen_backend_try_device_destroy(XenDevice *xendev, Error **errp);
>   
> +bool xen_backend_device_realized(XenDevice *xendev, Error **errp);
> +void xen_backend_device_unrealized(XenDevice *xendev);
> +
>   #endif /* HW_XEN_BACKEND_H */

Re: [PATCH 09/12] hw/xen: prevent duplicate device registrations

[David Woodhouse]

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

On Tue, 2023-10-24 at 15:10 +0100, Paul Durrant wrote:
> On 16/10/2023 16:19, David Woodhouse wrote:
> > --- a/hw/char/xen_console.c
> > +++ b/hw/char/xen_console.c
> > @@ -468,7 +468,7 @@ static void xen_console_device_create(XenBackendInstance *backend,
> >       Chardev *cd = NULL;
> >       struct qemu_xs_handle *xsh = xenbus->xsh;
> >   
> > -    if (qemu_strtoul(name, NULL, 10, &number)) {
> > +    if (qemu_strtoul(name, NULL, 10, &number) || number >= INT_MAX) {
> >           error_setg(errp, "failed to parse name '%s'", name);
> >           goto fail;
> >       }
> I don't think this hunk belongs here, does it? Seems like it should be 
> in patch 7.

Well, console#4294967295 *did* actually work before this patch started
using -1 to mean something different. But yes, I've already moved that
into the previous patch.

In fact I've just completely dropped this patch now, as the
dedeuplication needs to happen on the *frontend* nodes, since a given
frontend can be powered by a backend of different types, or in
different driver domains.


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

[PATCH 10/12] hw/xen: automatically assign device index to console devices

[David Woodhouse]

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

Now that we can reliably tell whether a given device already exists, we
can allow the user to add console devices on the command line with just
'-device xen-console,chardev=foo'.

Start at 1, because we can't add the *primary* console; that's special
because the toolstack has to set up the guest end of that.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 hw/char/xen_console.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index 2825b8c511..1a0f5ed3e1 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -333,6 +333,22 @@ static char *xen_console_get_name(XenDevice *xendev, Error **errp)
 {
     XenConsole *con = XEN_CONSOLE_DEVICE(xendev);
 
+    if (con->dev == -1) {
+        char name[11];
+        int idx = 1;
+
+        /* Theoretically we could go up to INT_MAX here but that's overkill */
+        while (idx < 100) {
+            snprintf(name, sizeof(name), "%u", idx);
+            if (!xen_backend_exists("console", name)) {
+                con->dev = idx;
+                return g_strdup(name);
+            }
+            idx++;
+        }
+        error_setg(errp, "cannot find device index for console device");
+        return NULL;
+    }
     return g_strdup_printf("%u", con->dev);
 }
 
-- 
2.40.1

[PATCH 11/12] hw/xen: automatically assign device index to block devices

[David Woodhouse]

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

There's no need to force the user to assign a vdev. We can automatically
assign one, starting at xvda and searching until we find the first disk
name that's unused.

This means we can now allow '-drive if=xen,file=xxx' to work without an
explicit separate -driver argument, just like if=virtio.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 blockdev.c           | 15 ++++++++++++---
 hw/block/xen-block.c | 25 +++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 325b7a3bef..9dec4c9c74 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -255,13 +255,13 @@ void drive_check_orphaned(void)
          * Ignore default drives, because we create certain default
          * drives unconditionally, then leave them unclaimed.  Not the
          * users fault.
-         * Ignore IF_VIRTIO, because it gets desugared into -device,
-         * so we can leave failing to -device.
+         * Ignore IF_VIRTIO or IF_XEN, because it gets desugared into
+         * -device, so we can leave failing to -device.
          * Ignore IF_NONE, because leaving unclaimed IF_NONE remains
          * available for device_add is a feature.
          */
         if (dinfo->is_default || dinfo->type == IF_VIRTIO
-            || dinfo->type == IF_NONE) {
+            || dinfo->type == IF_XEN || dinfo->type == IF_NONE) {
             continue;
         }
         if (!blk_get_attached_dev(blk)) {
@@ -977,6 +977,15 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type,
         qemu_opt_set(devopts, "driver", "virtio-blk", &error_abort);
         qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"),
                      &error_abort);
+    } else if (type == IF_XEN) {
+        QemuOpts *devopts;
+        devopts = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
+                                   &error_abort);
+        qemu_opt_set(devopts, "driver",
+                     (media == MEDIA_CDROM) ? "xen-cdrom" : "xen-disk",
+                     &error_abort);
+        qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"),
+                     &error_abort);
     }
 
     filename = qemu_opt_get(legacy_opts, "file");
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 9262338535..20fa783cbe 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -34,6 +34,31 @@ static char *xen_block_get_name(XenDevice *xendev, Error **errp)
     XenBlockDevice *blockdev = XEN_BLOCK_DEVICE(xendev);
     XenBlockVdev *vdev = &blockdev->props.vdev;
 
+    if (blockdev->props.vdev.type == XEN_BLOCK_VDEV_TYPE_INVALID) {
+        char name[11];
+        int disk = 0;
+        unsigned long idx;
+
+        /* Find an unoccupied device name */
+        while (disk < (1 << 20)) {
+            if (disk < (1 << 4)) {
+                idx = (202 << 8) | (disk << 4);
+            } else {
+                idx = (1 << 28) | (disk << 8);
+            }
+            snprintf(name, sizeof(name), "%lu", idx);
+            if (!xen_backend_exists("qdisk", name)) {
+                vdev->type = XEN_BLOCK_VDEV_TYPE_XVD;
+                vdev->partition = 0;
+                vdev->disk = disk;
+                vdev->number = idx;
+                return g_strdup(name);
+            }
+            disk++;
+        }
+        error_setg(errp, "cannot find device vdev for block device");
+        return NULL;
+    }
     return g_strdup_printf("%lu", vdev->number);
 }
 
-- 
2.40.1

Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices

[Kevin Wolf]

Am 16.10.2023 um 17:19 hat David Woodhouse geschrieben:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> There's no need to force the user to assign a vdev. We can automatically
> assign one, starting at xvda and searching until we find the first disk
> name that's unused.
> 
> This means we can now allow '-drive if=xen,file=xxx' to work without an
> explicit separate -driver argument, just like if=virtio.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>

> @@ -34,6 +34,31 @@ static char *xen_block_get_name(XenDevice *xendev, Error **errp)
>      XenBlockDevice *blockdev = XEN_BLOCK_DEVICE(xendev);
>      XenBlockVdev *vdev = &blockdev->props.vdev;
>  
> +    if (blockdev->props.vdev.type == XEN_BLOCK_VDEV_TYPE_INVALID) {
> +        char name[11];
> +        int disk = 0;
> +        unsigned long idx;
> +
> +        /* Find an unoccupied device name */
> +        while (disk < (1 << 20)) {

I like your optimism that we can handle a million disks. :-)

I haven't reviewed the Xen part in detail, but the patch looks fine on
the block layer side.

Acked-by: Kevin Wolf <kwolf@redhat.com>

Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices

[David Woodhouse]

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

On Tue, 2023-10-17 at 12:21 +0200, Kevin Wolf wrote:
> Am 16.10.2023 um 17:19 hat David Woodhouse geschrieben:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > There's no need to force the user to assign a vdev. We can automatically
> > assign one, starting at xvda and searching until we find the first disk
> > name that's unused.
> > 
> > This means we can now allow '-drive if=xen,file=xxx' to work without an
> > explicit separate -driver argument, just like if=virtio.
> > 
> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> 
> > @@ -34,6 +34,31 @@ static char *xen_block_get_name(XenDevice *xendev, Error **errp)
> >      XenBlockDevice *blockdev = XEN_BLOCK_DEVICE(xendev);
> >      XenBlockVdev *vdev = &blockdev->props.vdev;
> >  
> > +    if (blockdev->props.vdev.type == XEN_BLOCK_VDEV_TYPE_INVALID) {
> > +        char name[11];
> > +        int disk = 0;
> > +        unsigned long idx;
> > +
> > +        /* Find an unoccupied device name */
> > +        while (disk < (1 << 20)) {
> 
> I like your optimism that we can handle a million disks. :-)

Heh, yeah. Given that there *is* a limit, setting it lower seemed a bit
arbitrary.

For consoles I picked 100 instead of letting it go all the way to
INT_MAX, and in a patch set soon to be posted I'll do the same for the
xen-net-device as I convert it.

Even with a limit of 100, having that many devices *WITHOUT SPECIFYING
WHICH ONE IS WHICH* seems a bit many!

FWIW I've changed it to check for the existence of the *frontend* nodes
(the ones which are visible to the guest). Currently it checks for the
backend nodes, but those might be in different places.

> I haven't reviewed the Xen part in detail, but the patch looks fine on
> the block layer side.
> 
> Acked-by: Kevin Wolf <kwolf@redhat.com>

Thanks.

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

Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices

[Igor Mammedov]

On Mon, 16 Oct 2023 16:19:08 +0100
David Woodhouse <dwmw2@infradead.org> wrote:

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

is this index a user (guest) visible?

> There's no need to force the user to assign a vdev. We can automatically
> assign one, starting at xvda and searching until we find the first disk
> name that's unused.
> 
> This means we can now allow '-drive if=xen,file=xxx' to work without an
> explicit separate -driver argument, just like if=virtio.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>  blockdev.c           | 15 ++++++++++++---
>  hw/block/xen-block.c | 25 +++++++++++++++++++++++++
>  2 files changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 325b7a3bef..9dec4c9c74 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -255,13 +255,13 @@ void drive_check_orphaned(void)
>           * Ignore default drives, because we create certain default
>           * drives unconditionally, then leave them unclaimed.  Not the
>           * users fault.
> -         * Ignore IF_VIRTIO, because it gets desugared into -device,
> -         * so we can leave failing to -device.
> +         * Ignore IF_VIRTIO or IF_XEN, because it gets desugared into
> +         * -device, so we can leave failing to -device.
>           * Ignore IF_NONE, because leaving unclaimed IF_NONE remains
>           * available for device_add is a feature.
>           */
>          if (dinfo->is_default || dinfo->type == IF_VIRTIO
> -            || dinfo->type == IF_NONE) {
> +            || dinfo->type == IF_XEN || dinfo->type == IF_NONE) {
>              continue;
>          }
>          if (!blk_get_attached_dev(blk)) {
> @@ -977,6 +977,15 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type,
>          qemu_opt_set(devopts, "driver", "virtio-blk", &error_abort);
>          qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"),
>                       &error_abort);
> +    } else if (type == IF_XEN) {
> +        QemuOpts *devopts;
> +        devopts = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
> +                                   &error_abort);
> +        qemu_opt_set(devopts, "driver",
> +                     (media == MEDIA_CDROM) ? "xen-cdrom" : "xen-disk",
> +                     &error_abort);
> +        qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"),
> +                     &error_abort);
>      }
>  
>      filename = qemu_opt_get(legacy_opts, "file");
> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> index 9262338535..20fa783cbe 100644
> --- a/hw/block/xen-block.c
> +++ b/hw/block/xen-block.c
> @@ -34,6 +34,31 @@ static char *xen_block_get_name(XenDevice *xendev, Error **errp)
>      XenBlockDevice *blockdev = XEN_BLOCK_DEVICE(xendev);
>      XenBlockVdev *vdev = &blockdev->props.vdev;
>  
> +    if (blockdev->props.vdev.type == XEN_BLOCK_VDEV_TYPE_INVALID) {
> +        char name[11];
> +        int disk = 0;
> +        unsigned long idx;
> +
> +        /* Find an unoccupied device name */
> +        while (disk < (1 << 20)) {
> +            if (disk < (1 << 4)) {
> +                idx = (202 << 8) | (disk << 4);
> +            } else {
> +                idx = (1 << 28) | (disk << 8);
> +            }
> +            snprintf(name, sizeof(name), "%lu", idx);
> +            if (!xen_backend_exists("qdisk", name)) {
> +                vdev->type = XEN_BLOCK_VDEV_TYPE_XVD;
> +                vdev->partition = 0;
> +                vdev->disk = disk;
> +                vdev->number = idx;
> +                return g_strdup(name);
> +            }
> +            disk++;
> +        }
> +        error_setg(errp, "cannot find device vdev for block device");
> +        return NULL;
> +    }
>      return g_strdup_printf("%lu", vdev->number);
>  }
>  

Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices

[David Woodhouse]

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

On Wed, 2023-10-18 at 09:32 +0200, Igor Mammedov wrote:
> On Mon, 16 Oct 2023 16:19:08 +0100
> David Woodhouse <dwmw2@infradead.org> wrote:
> 
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> 
> is this index a user (guest) visible?

Yes. It defines what block device (e.g. /dev/xvda) the disk appears as
in the guest. In the common case, it literally encodes the Linux
major/minor numbers. So xvda (major 202) is 0xca00, xvdb is 0xca10 etc.

Previously we had to explicitly set it for each disk in Qemu:

  -drive file=disk1.img,id=drv1 -device xen-disk,drive=drv1,vdev=xvda
  -drive file=disk2.img,id=drv2 -device xen-disk,drive=drv2,vdev=xvdb

Now we can just do

  -drive file=disk1.img,if=xen -drive file-disk2.img,if=xen

(We could go further and make if=xen the default for Xen guests too,
but that doesn't work right now because xen-block will barf on the
default provided CD-ROM when it's empty. It doesn't handle empty
devices. And if I work around that, then `-hda disk1.img` would work on
the command line... but would make it appear as /dev/xvda instead of
/dev/hda, and I don't know how I feel about that.)

[root@localhost ~]# xenstore-ls  -f device/vbd
device/vbd = ""
device/vbd/51712 = ""
device/vbd/51712/backend = "/local/domain/0/backend/qdisk/1/51712"
device/vbd/51712/backend-id = "0"
device/vbd/51712/device-type = "disk"
device/vbd/51712/event-channel = "8"
device/vbd/51712/feature-persistent = "1"
device/vbd/51712/protocol = "x86_64-abi"
device/vbd/51712/ring-ref = "8"
device/vbd/51712/state = "4"
device/vbd/51712/virtual-device = "51712"

> 
> > There's no need to force the user to assign a vdev. We can automatically
> > assign one, starting at xvda and searching until we find the first disk
> > name that's unused.
> > 
> > This means we can now allow '-drive if=xen,file=xxx' to work without an
> > explicit separate -driver argument, just like if=virtio.
> > 
> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> > ---
> >  blockdev.c           | 15 ++++++++++++---
> >  hw/block/xen-block.c | 25 +++++++++++++++++++++++++
> >  2 files changed, 37 insertions(+), 3 deletions(-)
> > 
> > diff --git a/blockdev.c b/blockdev.c
> > index 325b7a3bef..9dec4c9c74 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -255,13 +255,13 @@ void drive_check_orphaned(void)
> >           * Ignore default drives, because we create certain default
> >           * drives unconditionally, then leave them unclaimed.  Not the
> >           * users fault.
> > -         * Ignore IF_VIRTIO, because it gets desugared into -device,
> > -         * so we can leave failing to -device.
> > +         * Ignore IF_VIRTIO or IF_XEN, because it gets desugared into
> > +         * -device, so we can leave failing to -device.
> >           * Ignore IF_NONE, because leaving unclaimed IF_NONE remains
> >           * available for device_add is a feature.
> >           */
> >          if (dinfo->is_default || dinfo->type == IF_VIRTIO
> > -            || dinfo->type == IF_NONE) {
> > +            || dinfo->type == IF_XEN || dinfo->type == IF_NONE) {
> >              continue;
> >          }
> >          if (!blk_get_attached_dev(blk)) {
> > @@ -977,6 +977,15 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type,
> >          qemu_opt_set(devopts, "driver", "virtio-blk", &error_abort);
> >          qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"),
> >                       &error_abort);
> > +    } else if (type == IF_XEN) {
> > +        QemuOpts *devopts;
> > +        devopts = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
> > +                                   &error_abort);
> > +        qemu_opt_set(devopts, "driver",
> > +                     (media == MEDIA_CDROM) ? "xen-cdrom" : "xen-disk",
> > +                     &error_abort);
> > +        qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"),
> > +                     &error_abort);
> >      }
> >  
> >      filename = qemu_opt_get(legacy_opts, "file");
> > diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> > index 9262338535..20fa783cbe 100644
> > --- a/hw/block/xen-block.c
> > +++ b/hw/block/xen-block.c
> > @@ -34,6 +34,31 @@ static char *xen_block_get_name(XenDevice *xendev, Error **errp)
> >      XenBlockDevice *blockdev = XEN_BLOCK_DEVICE(xendev);
> >      XenBlockVdev *vdev = &blockdev->props.vdev;
> >  
> > +    if (blockdev->props.vdev.type == XEN_BLOCK_VDEV_TYPE_INVALID) {
> > +        char name[11];
> > +        int disk = 0;
> > +        unsigned long idx;
> > +
> > +        /* Find an unoccupied device name */
> > +        while (disk < (1 << 20)) {
> > +            if (disk < (1 << 4)) {
> > +                idx = (202 << 8) | (disk << 4);
> > +            } else {
> > +                idx = (1 << 28) | (disk << 8);
> > +            }
> > +            snprintf(name, sizeof(name), "%lu", idx);
> > +            if (!xen_backend_exists("qdisk", name)) {
> > +                vdev->type = XEN_BLOCK_VDEV_TYPE_XVD;
> > +                vdev->partition = 0;
> > +                vdev->disk = disk;
> > +                vdev->number = idx;
> > +                return g_strdup(name);
> > +            }
> > +            disk++;
> > +        }
> > +        error_setg(errp, "cannot find device vdev for block device");
> > +        return NULL;
> > +    }
> >      return g_strdup_printf("%lu", vdev->number);
> >  }
> >  
> 
> 


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

Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices

[Igor Mammedov]

On Wed, 18 Oct 2023 09:32:47 +0100
David Woodhouse <dwmw2@infradead.org> wrote:

> On Wed, 2023-10-18 at 09:32 +0200, Igor Mammedov wrote:
> > On Mon, 16 Oct 2023 16:19:08 +0100
> > David Woodhouse <dwmw2@infradead.org> wrote:
> >   
> > > From: David Woodhouse <dwmw@amazon.co.uk>
> > >   
> > 
> > is this index a user (guest) visible?  
> 
> Yes. It defines what block device (e.g. /dev/xvda) the disk appears as
> in the guest. In the common case, it literally encodes the Linux
> major/minor numbers. So xvda (major 202) is 0xca00, xvdb is 0xca10 etc.

that makes 'index' an implicit ABI and a subject to versioning
when the way it's assigned changes (i.e. one has to use versioned
machine types to keep older versions working the they used to).

From what I remember it's discouraged to make QEMU invent
various IDs that are part of ABI (guest or mgmt side).
Instead it's preferred for mgmt side/user to provide that explicitly.

Basically you are trading off manageability/simplicity at QEMU
level with CLI usability for human user.
I don't care much as long as it is hidden within xen code base,
but maybe libvirt does.

> Previously we had to explicitly set it for each disk in Qemu:
> 
>   -drive file=disk1.img,id=drv1 -device xen-disk,drive=drv1,vdev=xvda
>   -drive file=disk2.img,id=drv2 -device xen-disk,drive=drv2,vdev=xvdb
> 
> Now we can just do
> 
>   -drive file=disk1.img,if=xen -drive file-disk2.img,if=xen
>
> (We could go further and make if=xen the default for Xen guests too,
> but that doesn't work right now because xen-block will barf on the
> default provided CD-ROM when it's empty. It doesn't handle empty
> devices. And if I work around that, then `-hda disk1.img` would work on
> the command line... but would make it appear as /dev/xvda instead of
> /dev/hda, and I don't know how I feel about that.)
>
> [root@localhost ~]# xenstore-ls  -f device/vbd
> device/vbd = ""
> device/vbd/51712 = ""
> device/vbd/51712/backend = "/local/domain/0/backend/qdisk/1/51712"
> device/vbd/51712/backend-id = "0"
> device/vbd/51712/device-type = "disk"
> device/vbd/51712/event-channel = "8"
> device/vbd/51712/feature-persistent = "1"
> device/vbd/51712/protocol = "x86_64-abi"
> device/vbd/51712/ring-ref = "8"
> device/vbd/51712/state = "4"
> device/vbd/51712/virtual-device = "51712"
> 
> >   
> > > There's no need to force the user to assign a vdev. We can automatically
> > > assign one, starting at xvda and searching until we find the first disk
> > > name that's unused.
> > > 
> > > This means we can now allow '-drive if=xen,file=xxx' to work without an
> > > explicit separate -driver argument, just like if=virtio.
> > > 
> > > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> > > ---
> > >  blockdev.c           | 15 ++++++++++++---
> > >  hw/block/xen-block.c | 25 +++++++++++++++++++++++++
> > >  2 files changed, 37 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/blockdev.c b/blockdev.c
> > > index 325b7a3bef..9dec4c9c74 100644
> > > --- a/blockdev.c
> > > +++ b/blockdev.c
> > > @@ -255,13 +255,13 @@ void drive_check_orphaned(void)
> > >           * Ignore default drives, because we create certain default
> > >           * drives unconditionally, then leave them unclaimed.  Not the
> > >           * users fault.
> > > -         * Ignore IF_VIRTIO, because it gets desugared into -device,
> > > -         * so we can leave failing to -device.
> > > +         * Ignore IF_VIRTIO or IF_XEN, because it gets desugared into
> > > +         * -device, so we can leave failing to -device.
> > >           * Ignore IF_NONE, because leaving unclaimed IF_NONE remains
> > >           * available for device_add is a feature.
> > >           */
> > >          if (dinfo->is_default || dinfo->type == IF_VIRTIO
> > > -            || dinfo->type == IF_NONE) {
> > > +            || dinfo->type == IF_XEN || dinfo->type == IF_NONE) {
> > >              continue;
> > >          }
> > >          if (!blk_get_attached_dev(blk)) {
> > > @@ -977,6 +977,15 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type,
> > >          qemu_opt_set(devopts, "driver", "virtio-blk", &error_abort);
> > >          qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"),
> > >                       &error_abort);
> > > +    } else if (type == IF_XEN) {
> > > +        QemuOpts *devopts;
> > > +        devopts = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
> > > +                                   &error_abort);
> > > +        qemu_opt_set(devopts, "driver",
> > > +                     (media == MEDIA_CDROM) ? "xen-cdrom" : "xen-disk",
> > > +                     &error_abort);
> > > +        qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"),
> > > +                     &error_abort);
> > >      }
> > >  
> > >      filename = qemu_opt_get(legacy_opts, "file");
> > > diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> > > index 9262338535..20fa783cbe 100644
> > > --- a/hw/block/xen-block.c
> > > +++ b/hw/block/xen-block.c
> > > @@ -34,6 +34,31 @@ static char *xen_block_get_name(XenDevice *xendev, Error **errp)
> > >      XenBlockDevice *blockdev = XEN_BLOCK_DEVICE(xendev);
> > >      XenBlockVdev *vdev = &blockdev->props.vdev;
> > >  
> > > +    if (blockdev->props.vdev.type == XEN_BLOCK_VDEV_TYPE_INVALID) {
> > > +        char name[11];
> > > +        int disk = 0;
> > > +        unsigned long idx;
> > > +
> > > +        /* Find an unoccupied device name */
> > > +        while (disk < (1 << 20)) {
> > > +            if (disk < (1 << 4)) {
> > > +                idx = (202 << 8) | (disk << 4);
> > > +            } else {
> > > +                idx = (1 << 28) | (disk << 8);
> > > +            }
> > > +            snprintf(name, sizeof(name), "%lu", idx);
> > > +            if (!xen_backend_exists("qdisk", name)) {
> > > +                vdev->type = XEN_BLOCK_VDEV_TYPE_XVD;
> > > +                vdev->partition = 0;
> > > +                vdev->disk = disk;
> > > +                vdev->number = idx;
> > > +                return g_strdup(name);
> > > +            }
> > > +            disk++;
> > > +        }
> > > +        error_setg(errp, "cannot find device vdev for block device");
> > > +        return NULL;
> > > +    }
> > >      return g_strdup_printf("%lu", vdev->number);
> > >  }
> > >    
> > 
> >   
> 

Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices

[David Woodhouse]

On 23 October 2023 10:30:02 BST, Igor Mammedov <imammedo@redhat.com> wrote:
>On Wed, 18 Oct 2023 09:32:47 +0100
>David Woodhouse <dwmw2@infradead.org> wrote:
>
>> On Wed, 2023-10-18 at 09:32 +0200, Igor Mammedov wrote:
>> > On Mon, 16 Oct 2023 16:19:08 +0100
>> > David Woodhouse <dwmw2@infradead.org> wrote:
>> >   
>> > > From: David Woodhouse <dwmw@amazon.co.uk>
>> > >   
>> > 
>> > is this index a user (guest) visible?  
>> 
>> Yes. It defines what block device (e.g. /dev/xvda) the disk appears as
>> in the guest. In the common case, it literally encodes the Linux
>> major/minor numbers. So xvda (major 202) is 0xca00, xvdb is 0xca10 etc.
>
>that makes 'index' an implicit ABI and a subject to versioning
>when the way it's assigned changes (i.e. one has to use versioned
>machine types to keep older versions working the they used to).
>
>From what I remember it's discouraged to make QEMU invent
>various IDs that are part of ABI (guest or mgmt side).
>Instead it's preferred for mgmt side/user to provide that explicitly.
>
>Basically you are trading off manageability/simplicity at QEMU
>level with CLI usability for human user.
>I don't care much as long as it is hidden within xen code base,
>but maybe libvirt does.

Well, it can still be set explicitly. So not so much a "trade-off" as adding the option for the user to choose the simple way.

Yes, in a way it's an ABI, just like the dynamic assignment of PCI devfn for network devices added with "-nic". And I think also for virtio block devices too? And for the ISA ne2000.

But it seems unlikely that we'll ever really want to change "the first one is xvda, the second is xvdb...."

Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices

[David Woodhouse]

On 23 October 2023 10:30:02 BST, Igor Mammedov <imammedo@redhat.com> wrote:
>On Wed, 18 Oct 2023 09:32:47 +0100
>David Woodhouse <dwmw2@infradead.org> wrote:
>
>> On Wed, 2023-10-18 at 09:32 +0200, Igor Mammedov wrote:
>> > On Mon, 16 Oct 2023 16:19:08 +0100
>> > David Woodhouse <dwmw2@infradead.org> wrote:
>> >   
>> > > From: David Woodhouse <dwmw@amazon.co.uk>
>> > >   
>> > 
>> > is this index a user (guest) visible?  
>> 
>> Yes. It defines what block device (e.g. /dev/xvda) the disk appears as
>> in the guest. In the common case, it literally encodes the Linux
>> major/minor numbers. So xvda (major 202) is 0xca00, xvdb is 0xca10 etc.
>
>that makes 'index' an implicit ABI and a subject to versioning
>when the way it's assigned changes (i.e. one has to use versioned
>machine types to keep older versions working the they used to).
>
From what I remember it's discouraged to make QEMU invent
>various IDs that are part of ABI (guest or mgmt side).
>Instead it's preferred for mgmt side/user to provide that explicitly.
>
>Basically you are trading off manageability/simplicity at QEMU
>level with CLI usability for human user.
>I don't care much as long as it is hidden within xen code base,
>but maybe libvirt does.

Well, it can still be set explicitly. So not so much a "trade-off" as adding the option for the user to choose the simple way.

Yes, in a way it's an ABI, just like the dynamic assignment of PCI devfn for network devices added with "-nic". And I think also for virtio block devices too? And for the ISA ne2000.

But it seems unlikely that we'll ever really want to change "the first one is xvda, the second is xvdb...."

Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices

[Kevin Wolf]

Am 23.10.2023 um 11:30 hat Igor Mammedov geschrieben:
> On Wed, 18 Oct 2023 09:32:47 +0100
> David Woodhouse <dwmw2@infradead.org> wrote:
> 
> > On Wed, 2023-10-18 at 09:32 +0200, Igor Mammedov wrote:
> > > On Mon, 16 Oct 2023 16:19:08 +0100
> > > David Woodhouse <dwmw2@infradead.org> wrote:
> > >   
> > > > From: David Woodhouse <dwmw@amazon.co.uk>
> > > >   
> > > 
> > > is this index a user (guest) visible?  
> > 
> > Yes. It defines what block device (e.g. /dev/xvda) the disk appears as
> > in the guest. In the common case, it literally encodes the Linux
> > major/minor numbers. So xvda (major 202) is 0xca00, xvdb is 0xca10 etc.
> 
> that makes 'index' an implicit ABI and a subject to versioning
> when the way it's assigned changes (i.e. one has to use versioned
> machine types to keep older versions working the they used to).
> 
> From what I remember it's discouraged to make QEMU invent
> various IDs that are part of ABI (guest or mgmt side).
> Instead it's preferred for mgmt side/user to provide that explicitly.
> 
> Basically you are trading off manageability/simplicity at QEMU
> level with CLI usability for human user.
> I don't care much as long as it is hidden within xen code base,
> but maybe libvirt does.

-drive is mostly a convenience option for human users anyway. Management
tools should use a combination of -blockdev and -device.

Kevin

Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices

[Kevin Wolf]

Am 16.10.2023 um 17:19 hat David Woodhouse geschrieben:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> There's no need to force the user to assign a vdev. We can automatically
> assign one, starting at xvda and searching until we find the first disk
> name that's unused.
> 
> This means we can now allow '-drive if=xen,file=xxx' to work without an
> explicit separate -driver argument, just like if=virtio.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>

Actually, how does this play together with xen_config_dev_blk()? This
looks like it tried to implement a very similar thing (which is IF_XEN
even already existed).

Are we now trying to attach each if=xen disk twice in the 'xenpv'
machine? Or if something prevents this, is it dead code?

I think in both cases, we would want to delete that function and remove
the loop that calls it in xen_init_pv()?

Kevin

Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices

[David Woodhouse]

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

On Wed, 2023-10-18 at 10:52 +0200, Kevin Wolf wrote:
> Am 16.10.2023 um 17:19 hat David Woodhouse geschrieben:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > There's no need to force the user to assign a vdev. We can automatically
> > assign one, starting at xvda and searching until we find the first disk
> > name that's unused.
> > 
> > This means we can now allow '-drive if=xen,file=xxx' to work without an
> > explicit separate -driver argument, just like if=virtio.
> > 
> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> 
> Actually, how does this play together with xen_config_dev_blk()? This
> looks like it tried to implement a very similar thing (which is IF_XEN
> even already existed).

Ah yes, thanks for spotting that! I hadn't been looking at the xenfv

> Are we now trying to attach each if=xen disk twice in the 'xenpv'
> machine? Or if something prevents this, is it dead code.

I suspect we end up creating them twice (and probably thus failing to
lock the backing file).

The old Xen PV device drivers in Qemu, before Paul's XenDevice
conversion, were purely reactive. If they see nodes in XenStore
describing a backend which they should implement, they'd do so.

The code in xen_config_dev_blk() is *creating* those nodes for the
frontend (guest) and backend (host/qemu) to see, which prompts the
xen-block and xen-net drivers into action.

In the new XenDevice model, we can just instantiate a device (e.g.
qdev_new(TYPE_XEN_DISK_DEVICE) and its realize() method will create the
corresponding XenStore nodes (with some help from the generic XenBus
code for the common ones).

The new XenDevice/XenBus model will *also* react to nodes which it
discovers in XenStore, and instantiate the corresponding devices.

So I suspect what'll happen is xen_config_dev_blk() will create the
nodes starting at xvda (int vdev = 202 * 256 + 16 * disk->unit), and
later my new code will create a new set starting from xvdb or wherever
the next free one is.

> I think in both cases, we would want to delete that function and remove
> the loop that calls it in xen_init_pv()?

Yes, I think so. The xen_config_dev_blk() one can just die, as can
xen_config_dev_console() which isn't called from anywhere anyway.

The vkbd/vfb ones can stay until/unless those drivers are converted to
the new XenDevice model.

And xen_config_dev_nic() probably just needs to loop doing the same as
I did in pc_init_nic() in
https://lore.kernel.org/qemu-devel/20231017182545.97973-5-dwmw2@infradead.org/T/#u

+        if (xen_bus && (!nd->model || g_str_equal(model, "xen-net-device"))) {
+            DeviceState *dev = qdev_new("xen-net-device");
+            qdev_set_nic_properties(dev, nd);
+            qdev_realize_and_unref(dev, xen_bus, &error_fatal);


... but this just reinforces what I said there about "if
qmp_device_add() can find the damn bus and do this right, why do we
have to litter it through platform code?"

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

Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices

[Kevin Wolf]

Am 18.10.2023 um 12:52 hat David Woodhouse geschrieben:
> > Actually, how does this play together with xen_config_dev_blk()? This
> > looks like it tried to implement a very similar thing (which is IF_XEN
> > even already existed).
> 
> Ah yes, thanks for spotting that! I hadn't been looking at the xenfv
> 
> > Are we now trying to attach each if=xen disk twice in the 'xenpv'
> > machine? Or if something prevents this, is it dead code.
> 
> I suspect we end up creating them twice (and probably thus failing to
> lock the backing file).
>
> [...]
>
> ... but this just reinforces what I said there about "if
> qmp_device_add() can find the damn bus and do this right, why do we
> have to litter it through platform code?"

Indeed, if you can do -device, it's always the best option. For block
devices not the least because it gives you a way to use -blockdev with
it. I'm happy whenever I see a drive_get() call go away.

Kevin

Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices

[David Woodhouse]

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

On Wed, 2023-10-18 at 11:52 +0100, David Woodhouse wrote:
> 
> And xen_config_dev_nic() probably just needs to loop doing the same
> as
> I did in pc_init_nic() in
> https://lore.kernel.org/qemu-devel/20231017182545.97973-5-dwmw2@infradead.org/T/#u
> 
> +        if (xen_bus && (!nd->model || g_str_equal(model, "xen-net-
> device"))) {
> +            DeviceState *dev = qdev_new("xen-net-device");
> +            qdev_set_nic_properties(dev, nd);
> +            qdev_realize_and_unref(dev, xen_bus, &error_fatal);
> 
> 
> ... but this just reinforces what I said there about "if
> qmp_device_add() can find the damn bus and do this right, why do we
> have to litter it through platform code?"

I had a look through the network setup.

There are a bunch of platforms adding specific devices to their own
internal system bus, which often use nd_table[] directly. Sometimes
they do so whether it's been set up or now.

They can mostly be divided into two camps. Some of them will create
their NIC anyway, and will use a matching -nic configuration if it
exists. Others will only create their NIC if a corresponding -nic
configuration does exist.

This is fairly random, and perhaps platforms should be more consistent,
but it's best to avoid user-visible changes as much as possible while
doing the cleanup, so I've kept them as they were.

I've created qemu_configure_nic_device() and qemu_create_nic_device()
functions for those two use cases respectively, and most code which
directly accesses nd_table[] can be converted to those fairly simply:
https://git.infradead.org/users/dwmw2/qemu.git/commitdiff/7b4fb6fc10a4

It means I can throw away the horrid parts of -nic support for the Xen
network device, which were in the pc and xenfv platforms, and replace
it with a trivial loop in xenbus_init():

+    /* This is a bus-generic "configure all NICs on this bus type" */
+    while ((qnic = qemu_create_nic_device("xen-net-device", true, "xen"))) {
+            qdev_realize_and_unref(qnic, bus, &error_fatal);

Other than that one (which is cheating because there's only one type of
network device that can be instantiated on the XenBus), the only
remaining case is PCI. Most platforms just iterate over the -nic
configurations adding devices to a PCI bus of the platform's choice.
In some cases there's a special case, the first one goes at a specific
devfn and/or on a different bus.

There was a little more variation here... for example fuloong2e would
put the first nic (nd_table[0]) in slot 7 only if it's an rtl8139 (if
the model is unspecified). But PReP would put the first PCI NIC in slot
3 *regardless* of whether it's the expected PCNET or not.

I didn't faithfully preserve the behaviour there, because I don't think
it matters. They mostly look like this now (e.g. hw/sh4/r2d):

+    nd = qemu_find_nic_info(mc->default_nic, true, NULL);
+    if (nd) {
+        pci_nic_init_nofail(nd, pci_bus, mc->default_nic, "2");
+    }
+    pci_init_nic_devices(pci_bus, mc->default_nic);

So they'll take the first NIC configuration which is of the expected
model (again, or unspecified model) and place that in the special slot,
and then put the rest of the devices wherever they land.

For the change in behaviour to *matter*, the user would have to
explicitly specify a NIC of the *non-default* type first, and then a
NIC of the default type. My new code will put the default-type NIC in
the "right place", while the old code mostly wouldn't. I think that's
an OK change to make.

My plan is to *remember* which NIC models are used for lookups during
the board in, so that qemu_show_nic_devices() can print them all at the
end.

Current WIP if anyone wants to take a quick look at
https://git.infradead.org/users/dwmw2/qemu.git/shortlog/refs/heads/xenfv-net
but the weekend arrived quicker than I'd hoped, so I haven't quite got
it into shape to post yet. Hopefully it makes sense as an approach
though?



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

Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices

[David Woodhouse]

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

On Wed, 2023-10-18 at 10:52 +0200, Kevin Wolf wrote:
> Am 16.10.2023 um 17:19 hat David Woodhouse geschrieben:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > There's no need to force the user to assign a vdev. We can automatically
> > assign one, starting at xvda and searching until we find the first disk
> > name that's unused.
> > 
> > This means we can now allow '-drive if=xen,file=xxx' to work without an
> > explicit separate -driver argument, just like if=virtio.
> > 
> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> 
> Actually, how does this play together with xen_config_dev_blk()? This
> looks like it tried to implement a very similar thing (which is IF_XEN
> even already existed).
> 
> Are we now trying to attach each if=xen disk twice in the 'xenpv'
> machine? Or if something prevents this, is it dead code?
> 
> I think in both cases, we would want to delete that function and remove
> the loop that calls it in xen_init_pv()?

I tested this with an xl config which looks a bit like this...

disk = [ "backendtype=qdisk,format=qcow2,vdev=xvda,access=rw,target=/var/lib/libvirt/images/fedora28.qcow2" ]
device_model_override = "/home/dwmw2/git/qemu/build/qemu-system-x86_64"
device_model_version = "qemu-xen"
device_model_args = [ "-trace","xen*","-chardev","pty,id=mon","-mon","mon","-drive","file=/var/lib/libvirt/images/solaris11.qcow2,format=qcow2,if=xen","-nic","user,model=xen" ]

The code in xen_config_dev_blk() scribbles over the disk that the
toolstack has configured for me, and adds that qcow2 file from the
'-drive' option on the command line, but in *raw* mode.

Then the new xen-disk support kicks in and finds a free vdev, and adds
the -drive properly /dev/xvdb (as qcow2).

So v2 of this patch will just rip out xen_config_dev_blk() as you
suggest.


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

[PATCH 12/12] hw/xen: add support for Xen primary console in emulated mode

[David Woodhouse]

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

The primary console is special because the toolstack maps a page at a
fixed GFN and also allocates the guest-side event channel. Add support
for that in emulated mode, so that we can have a primary console.

Add a *very* rudimentary stub of foriegnmem ops for emulated mode, which
supports literally nothing except a single-page mapping of the console
page. This might as well have been a hack in the xen_console driver, but
this way at least the special-casing is kept within the Xen emulation
code, and it gives us a hook for a more complete implementation if/when
we ever do need one.

Now at last we can boot the Xen PV shim and run PV kernels in QEMU.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 hw/char/xen_console.c             |  12 ++-
 hw/i386/kvm/meson.build           |   1 +
 hw/i386/kvm/trace-events          |   2 +
 hw/i386/kvm/xen-stubs.c           |   5 +
 hw/i386/kvm/xen_gnttab.c          |  32 +++++-
 hw/i386/kvm/xen_primary_console.c | 167 ++++++++++++++++++++++++++++++
 hw/i386/kvm/xen_primary_console.h |  22 ++++
 hw/i386/kvm/xen_xenstore.c        |   9 ++
 target/i386/kvm/xen-emu.c         |  23 +++-
 9 files changed, 266 insertions(+), 7 deletions(-)
 create mode 100644 hw/i386/kvm/xen_primary_console.c
 create mode 100644 hw/i386/kvm/xen_primary_console.h

diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index 1a0f5ed3e1..dfc4be0aa1 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -32,6 +32,7 @@
 #include "hw/qdev-properties.h"
 #include "hw/qdev-properties-system.h"
 #include "hw/xen/interface/io/console.h"
+#include "hw/i386/kvm/xen_primary_console.h"
 #include "trace.h"
 
 struct buffer {
@@ -334,8 +335,8 @@ static char *xen_console_get_name(XenDevice *xendev, Error **errp)
     XenConsole *con = XEN_CONSOLE_DEVICE(xendev);
 
     if (con->dev == -1) {
+        int idx = (xen_mode == XEN_EMULATE) ? 0 : 1;
         char name[11];
-        int idx = 1;
 
         /* Theoretically we could go up to INT_MAX here but that's overkill */
         while (idx < 100) {
@@ -386,10 +387,13 @@ static void xen_console_realize(XenDevice *xendev, Error **errp)
      * be mapped directly as foreignmem (not a grant ref), and the guest port
      * was allocated *for* the guest by the toolstack. The guest gets these
      * through HVMOP_get_param and can use the console long before it's got
-     * XenStore up and running. We cannot create those for a Xen guest.
+     * XenStore up and running. We cannot create those for a true Xen guest,
+     * but we can for Xen emulation.
      */
     if (!con->dev) {
-        if (xen_device_frontend_scanf(xendev, "ring-ref", "%u", &u) != 1 ||
+        if (xen_mode == XEN_EMULATE) {
+            xen_primary_console_create();
+        } else if (xen_device_frontend_scanf(xendev, "ring-ref", "%u", &u) != 1 ||
             xen_device_frontend_scanf(xendev, "port", "%u", &u) != 1) {
             error_setg(errp, "cannot create primary Xen console");
             return;
@@ -404,7 +408,7 @@ static void xen_console_realize(XenDevice *xendev, Error **errp)
     }
 
     /* No normal PV driver initialization for the primary console */
-    if (!con->dev) {
+    if (!con->dev && xen_mode != XEN_EMULATE) {
         xen_console_connect(xendev, errp);
     }
 }
diff --git a/hw/i386/kvm/meson.build b/hw/i386/kvm/meson.build
index ab143d6474..a4a2e23c06 100644
--- a/hw/i386/kvm/meson.build
+++ b/hw/i386/kvm/meson.build
@@ -9,6 +9,7 @@ i386_kvm_ss.add(when: 'CONFIG_XEN_EMU', if_true: files(
   'xen_evtchn.c',
   'xen_gnttab.c',
   'xen_xenstore.c',
+  'xen_primary_console.c',
   'xenstore_impl.c',
   ))
 
diff --git a/hw/i386/kvm/trace-events b/hw/i386/kvm/trace-events
index e4c82de6f3..67bf7f174e 100644
--- a/hw/i386/kvm/trace-events
+++ b/hw/i386/kvm/trace-events
@@ -18,3 +18,5 @@ xenstore_watch(const char *path, const char *token) "path %s token %s"
 xenstore_unwatch(const char *path, const char *token) "path %s token %s"
 xenstore_reset_watches(void) ""
 xenstore_watch_event(const char *path, const char *token) "path %s token %s"
+xen_primary_console_create(void) ""
+xen_primary_console_reset(int port) "port %u"
diff --git a/hw/i386/kvm/xen-stubs.c b/hw/i386/kvm/xen-stubs.c
index ae406e0b02..10068970fe 100644
--- a/hw/i386/kvm/xen-stubs.c
+++ b/hw/i386/kvm/xen-stubs.c
@@ -15,6 +15,7 @@
 #include "qapi/qapi-commands-misc-target.h"
 
 #include "xen_evtchn.h"
+#include "xen_primary_console.h"
 
 void xen_evtchn_snoop_msi(PCIDevice *dev, bool is_msix, unsigned int vector,
                           uint64_t addr, uint32_t data, bool is_masked)
@@ -30,6 +31,10 @@ bool xen_evtchn_deliver_pirq_msi(uint64_t address, uint32_t data)
     return false;
 }
 
+void xen_primary_console_create(void)
+{
+}
+
 #ifdef TARGET_I386
 EvtchnInfoList *qmp_xen_event_list(Error **errp)
 {
diff --git a/hw/i386/kvm/xen_gnttab.c b/hw/i386/kvm/xen_gnttab.c
index 21c30e3659..ea201cd582 100644
--- a/hw/i386/kvm/xen_gnttab.c
+++ b/hw/i386/kvm/xen_gnttab.c
@@ -25,6 +25,7 @@
 #include "hw/xen/xen_backend_ops.h"
 #include "xen_overlay.h"
 #include "xen_gnttab.h"
+#include "xen_primary_console.h"
 
 #include "sysemu/kvm.h"
 #include "sysemu/kvm_xen.h"
@@ -38,6 +39,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(XenGnttabState, XEN_GNTTAB)
 #define ENTRIES_PER_FRAME_V1 (XEN_PAGE_SIZE / sizeof(grant_entry_v1_t))
 
 static struct gnttab_backend_ops emu_gnttab_backend_ops;
+static struct foreignmem_backend_ops emu_foreignmem_backend_ops;
 
 struct XenGnttabState {
     /*< private >*/
@@ -100,6 +102,7 @@ static void xen_gnttab_realize(DeviceState *dev, Error **errp)
     s->map_track = g_new0(uint8_t, s->max_frames * ENTRIES_PER_FRAME_V1);
 
     xen_gnttab_ops = &emu_gnttab_backend_ops;
+    xen_foreignmem_ops = &emu_foreignmem_backend_ops;
 }
 
 static int xen_gnttab_post_load(void *opaque, int version_id)
@@ -524,6 +527,29 @@ static struct gnttab_backend_ops emu_gnttab_backend_ops = {
     .unmap = xen_be_gnttab_unmap,
 };
 
+/* Dummy implementation of foriegnmem; just enough for console */
+static void *xen_be_foreignmem_map(uint32_t dom, void *addr, int prot,
+                                   size_t pages, xen_pfn_t *pfns,
+                                   int *errs)
+{
+    if (dom == xen_domid && !addr && pages == 1 &&
+        pfns[0] == xen_primary_console_get_pfn()) {
+        return xen_primary_console_get_map();
+    }
+
+    return NULL;
+}
+
+static int xen_be_foreignmem_unmap(void *addr, size_t pages)
+{
+    return 0;
+}
+
+static struct foreignmem_backend_ops emu_foreignmem_backend_ops = {
+    .map = xen_be_foreignmem_map,
+    .unmap = xen_be_foreignmem_unmap,
+};
+
 int xen_gnttab_reset(void)
 {
     XenGnttabState *s = xen_gnttab_singleton;
@@ -537,10 +563,14 @@ int xen_gnttab_reset(void)
     s->nr_frames = 0;
 
     memset(s->entries.v1, 0, XEN_PAGE_SIZE * s->max_frames);
-
     s->entries.v1[GNTTAB_RESERVED_XENSTORE].flags = GTF_permit_access;
     s->entries.v1[GNTTAB_RESERVED_XENSTORE].frame = XEN_SPECIAL_PFN(XENSTORE);
 
+    if (xen_primary_console_get_pfn()) {
+        s->entries.v1[GNTTAB_RESERVED_CONSOLE].flags = GTF_permit_access;
+        s->entries.v1[GNTTAB_RESERVED_CONSOLE].frame = XEN_SPECIAL_PFN(CONSOLE);
+    }
+
     memset(s->map_track, 0, s->max_frames * ENTRIES_PER_FRAME_V1);
 
     return 0;
diff --git a/hw/i386/kvm/xen_primary_console.c b/hw/i386/kvm/xen_primary_console.c
new file mode 100644
index 0000000000..0aa1c16ad6
--- /dev/null
+++ b/hw/i386/kvm/xen_primary_console.c
@@ -0,0 +1,167 @@
+/*
+ * QEMU Xen emulation: Primary console support
+ *
+ * Copyright © 2023 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 "qapi/error.h"
+
+#include "hw/sysbus.h"
+#include "hw/xen/xen.h"
+#include "hw/xen/xen_backend_ops.h"
+#include "xen_evtchn.h"
+#include "xen_overlay.h"
+#include "xen_primary_console.h"
+
+#include "sysemu/kvm.h"
+#include "sysemu/kvm_xen.h"
+
+#include "trace.h"
+
+#include "hw/xen/interface/event_channel.h"
+#include "hw/xen/interface/grant_table.h"
+
+#define TYPE_XEN_PRIMARY_CONSOLE "xen-primary-console"
+OBJECT_DECLARE_SIMPLE_TYPE(XenPrimaryConsoleState, XEN_PRIMARY_CONSOLE)
+
+struct XenPrimaryConsoleState {
+    /*< private >*/
+    SysBusDevice busdev;
+    /*< public >*/
+
+    MemoryRegion console_page;
+    void *cp;
+
+    evtchn_port_t guest_port;
+    evtchn_port_t be_port;
+
+    struct xengntdev_handle *gt;
+    void *granted_xs;
+};
+
+struct XenPrimaryConsoleState *xen_primary_console_singleton;
+
+static void xen_primary_console_realize(DeviceState *dev, Error **errp)
+{
+    XenPrimaryConsoleState *s = XEN_PRIMARY_CONSOLE(dev);
+
+    if (xen_mode != XEN_EMULATE) {
+        error_setg(errp, "Xen primary console support is for Xen emulation");
+        return;
+    }
+
+    memory_region_init_ram(&s->console_page, OBJECT(dev), "xen:console_page",
+                           XEN_PAGE_SIZE, &error_abort);
+    memory_region_set_enabled(&s->console_page, true);
+    s->cp = memory_region_get_ram_ptr(&s->console_page);
+    memset(s->cp, 0, XEN_PAGE_SIZE);
+
+    /* We can't map it this early as KVM isn't ready */
+    xen_primary_console_singleton = s;
+}
+
+static void xen_primary_console_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = xen_primary_console_realize;
+}
+
+static const TypeInfo xen_primary_console_info = {
+    .name          = TYPE_XEN_PRIMARY_CONSOLE,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(XenPrimaryConsoleState),
+    .class_init    = xen_primary_console_class_init,
+};
+
+
+void xen_primary_console_create(void)
+{
+    DeviceState *dev = sysbus_create_simple(TYPE_XEN_PRIMARY_CONSOLE, -1, NULL);
+
+    trace_xen_primary_console_create();
+
+    xen_primary_console_singleton = XEN_PRIMARY_CONSOLE(dev);
+
+    /*
+     * Defer the init (xen_primary_console_reset()) until KVM is set up and the
+     * overlay page can be mapped.
+     */
+}
+
+static void xen_primary_console_register_types(void)
+{
+    type_register_static(&xen_primary_console_info);
+}
+
+type_init(xen_primary_console_register_types)
+
+uint16_t xen_primary_console_get_port(void)
+{
+    XenPrimaryConsoleState *s = xen_primary_console_singleton;
+    if (!s) {
+        return 0;
+    }
+    return s->guest_port;
+}
+
+uint64_t xen_primary_console_get_pfn(void)
+{
+    XenPrimaryConsoleState *s = xen_primary_console_singleton;
+    if (!s) {
+        return 0;
+    }
+    return XEN_SPECIAL_PFN(CONSOLE);
+}
+
+void *xen_primary_console_get_map(void)
+{
+    XenPrimaryConsoleState *s = xen_primary_console_singleton;
+    if (!s) {
+        return 0;
+    }
+    return s->cp;
+}
+
+static void alloc_guest_port(XenPrimaryConsoleState *s)
+{
+    struct evtchn_alloc_unbound alloc = {
+        .dom = DOMID_SELF,
+        .remote_dom = DOMID_QEMU,
+    };
+
+    if (!xen_evtchn_alloc_unbound_op(&alloc)) {
+        s->guest_port = alloc.port;
+    }
+}
+
+int xen_primary_console_reset(void)
+{
+    XenPrimaryConsoleState *s = xen_primary_console_singleton;
+    if (!s) {
+        return 0;
+    }
+
+    if (!memory_region_is_mapped(&s->console_page)) {
+        uint64_t gpa = XEN_SPECIAL_PFN(CONSOLE) << TARGET_PAGE_BITS;
+        xen_overlay_do_map_page(&s->console_page, gpa);
+    }
+
+    alloc_guest_port(s);
+
+    trace_xen_primary_console_reset(s->guest_port);
+
+    s->gt = qemu_xen_gnttab_open();
+    uint32_t xs_gntref = GNTTAB_RESERVED_CONSOLE;
+    s->granted_xs = qemu_xen_gnttab_map_refs(s->gt, 1, xen_domid, &xs_gntref,
+                                             PROT_READ | PROT_WRITE);
+
+    return 0;
+}
diff --git a/hw/i386/kvm/xen_primary_console.h b/hw/i386/kvm/xen_primary_console.h
new file mode 100644
index 0000000000..dd4922f3f4
--- /dev/null
+++ b/hw/i386/kvm/xen_primary_console.h
@@ -0,0 +1,22 @@
+/*
+ * QEMU Xen emulation: Primary console support
+ *
+ * Copyright © 2023 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_PRIMARY_CONSOLE_H
+#define QEMU_XEN_PRIMARY_CONSOLE_H
+
+void xen_primary_console_create(void);
+int xen_primary_console_reset(void);
+
+uint16_t xen_primary_console_get_port(void);
+uint64_t xen_primary_console_get_pfn(void);
+void *xen_primary_console_get_map(void);
+
+#endif /* QEMU_XEN_PRIMARY_CONSOLE_H */
diff --git a/hw/i386/kvm/xen_xenstore.c b/hw/i386/kvm/xen_xenstore.c
index 3300e0614a..9f8946e0ce 100644
--- a/hw/i386/kvm/xen_xenstore.c
+++ b/hw/i386/kvm/xen_xenstore.c
@@ -25,6 +25,7 @@
 #include "hw/xen/xen_backend_ops.h"
 #include "xen_overlay.h"
 #include "xen_evtchn.h"
+#include "xen_primary_console.h"
 #include "xen_xenstore.h"
 
 #include "sysemu/kvm.h"
@@ -1432,6 +1433,7 @@ static void alloc_guest_port(XenXenstoreState *s)
 int xen_xenstore_reset(void)
 {
     XenXenstoreState *s = xen_xenstore_singleton;
+    int console_port;
     GList *perms;
     int err;
 
@@ -1467,6 +1469,13 @@ int xen_xenstore_reset(void)
     relpath_printf(s, perms, "store/ring-ref", "%lu", XEN_SPECIAL_PFN(XENSTORE));
     relpath_printf(s, perms, "store/port", "%u", s->be_port);
 
+    console_port = xen_primary_console_get_port();
+    if (console_port) {
+        relpath_printf(s, perms, "console/ring-ref", "%lu", XEN_SPECIAL_PFN(CONSOLE));
+        relpath_printf(s, perms, "console/port", "%u", console_port);
+        relpath_printf(s, perms, "console/state", "%u", XenbusStateInitialised);
+    }
+
     g_list_free_full(perms, g_free);
 
     /*
diff --git a/target/i386/kvm/xen-emu.c b/target/i386/kvm/xen-emu.c
index 477e93cd92..9f57786e95 100644
--- a/target/i386/kvm/xen-emu.c
+++ b/target/i386/kvm/xen-emu.c
@@ -28,6 +28,7 @@
 #include "hw/i386/kvm/xen_overlay.h"
 #include "hw/i386/kvm/xen_evtchn.h"
 #include "hw/i386/kvm/xen_gnttab.h"
+#include "hw/i386/kvm/xen_primary_console.h"
 #include "hw/i386/kvm/xen_xenstore.h"
 
 #include "hw/xen/interface/version.h"
@@ -182,7 +183,8 @@ int kvm_xen_init(KVMState *s, uint32_t hypercall_msr)
         return ret;
     }
 
-    /* The page couldn't be overlaid until KVM was initialized */
+    /* The pages couldn't be overlaid until KVM was initialized */
+    xen_primary_console_reset();
     xen_xenstore_reset();
 
     return 0;
@@ -811,11 +813,23 @@ static bool handle_get_param(struct kvm_xen_exit *exit, X86CPU *cpu,
     case HVM_PARAM_STORE_EVTCHN:
         hp.value = xen_xenstore_get_port();
         break;
+    case HVM_PARAM_CONSOLE_PFN:
+        hp.value = xen_primary_console_get_pfn();
+        if (!hp.value) {
+            err = -EINVAL;
+        }
+        break;
+    case HVM_PARAM_CONSOLE_EVTCHN:
+        hp.value = xen_primary_console_get_port();
+        if (!hp.value) {
+            err = -EINVAL;
+        }
+        break;
     default:
         return false;
     }
 
-    if (kvm_copy_to_gva(cs, arg, &hp, sizeof(hp))) {
+    if (!err && kvm_copy_to_gva(cs, arg, &hp, sizeof(hp))) {
         err = -EFAULT;
     }
 out:
@@ -1426,6 +1440,11 @@ int kvm_xen_soft_reset(void)
         return err;
     }
 
+    err = xen_primary_console_reset();
+    if (err) {
+        return err;
+    }
+
     err = xen_xenstore_reset();
     if (err) {
         return err;
-- 
2.40.1

Re: [PATCH 12/12] hw/xen: add support for Xen primary console in emulated mode

[Paul Durrant]

On 16/10/2023 16:19, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> The primary console is special because the toolstack maps a page at a
> fixed GFN and also allocates the guest-side event channel. Add support
> for that in emulated mode, so that we can have a primary console.
> 
> Add a *very* rudimentary stub of foriegnmem ops for emulated mode, which
> supports literally nothing except a single-page mapping of the console
> page. This might as well have been a hack in the xen_console driver, but
> this way at least the special-casing is kept within the Xen emulation
> code, and it gives us a hook for a more complete implementation if/when
> we ever do need one.
> 
Why can't you map the console page via the grant table like the xenstore 
page?

   Paul

Re: [PATCH 12/12] hw/xen: add support for Xen primary console in emulated mode

[David Woodhouse]

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

On Tue, 2023-10-24 at 15:20 +0100, Paul Durrant wrote:
> On 16/10/2023 16:19, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > The primary console is special because the toolstack maps a page at a
> > fixed GFN and also allocates the guest-side event channel. Add support
> > for that in emulated mode, so that we can have a primary console.
> > 
> > Add a *very* rudimentary stub of foriegnmem ops for emulated mode, which
> > supports literally nothing except a single-page mapping of the console
> > page. This might as well have been a hack in the xen_console driver, but
> > this way at least the special-casing is kept within the Xen emulation
> > code, and it gives us a hook for a more complete implementation if/when
> > we ever do need one.
> > 
> Why can't you map the console page via the grant table like the xenstore 
> page?

I suppose we could, but I didn't really want the generic xen-console
device code having any more of a special case for 'Xen emulation' than
it does already by having to call xen_primary_console_create().



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

Re: [PATCH 12/12] hw/xen: add support for Xen primary console in emulated mode

[Paul Durrant]

On 24/10/2023 16:37, David Woodhouse wrote:
> On Tue, 2023-10-24 at 15:20 +0100, Paul Durrant wrote:
>> On 16/10/2023 16:19, David Woodhouse wrote:
>>> From: David Woodhouse <dwmw@amazon.co.uk>
>>>
>>> The primary console is special because the toolstack maps a page at a
>>> fixed GFN and also allocates the guest-side event channel. Add support
>>> for that in emulated mode, so that we can have a primary console.
>>>
>>> Add a *very* rudimentary stub of foriegnmem ops for emulated mode, which
>>> supports literally nothing except a single-page mapping of the console
>>> page. This might as well have been a hack in the xen_console driver, but
>>> this way at least the special-casing is kept within the Xen emulation
>>> code, and it gives us a hook for a more complete implementation if/when
>>> we ever do need one.
>>>
>> Why can't you map the console page via the grant table like the xenstore
>> page?
> 
> I suppose we could, but I didn't really want the generic xen-console
> device code having any more of a special case for 'Xen emulation' than
> it does already by having to call xen_primary_console_create().
> 

But doesn't is save you the whole foreignmem thing? You can use the 
grant table for primary and secondary consoles.

   Paul

Re: [PATCH 12/12] hw/xen: add support for Xen primary console in emulated mode

[David Woodhouse]

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

On Tue, 2023-10-24 at 16:39 +0100, Paul Durrant wrote:
> On 24/10/2023 16:37, David Woodhouse wrote:
> > On Tue, 2023-10-24 at 15:20 +0100, Paul Durrant wrote:
> > > On 16/10/2023 16:19, David Woodhouse wrote:
> > > > From: David Woodhouse <dwmw@amazon.co.uk>
> > > > 
> > > > The primary console is special because the toolstack maps a page at a
> > > > fixed GFN and also allocates the guest-side event channel. Add support
> > > > for that in emulated mode, so that we can have a primary console.
> > > > 
> > > > Add a *very* rudimentary stub of foriegnmem ops for emulated mode, which
> > > > supports literally nothing except a single-page mapping of the console
> > > > page. This might as well have been a hack in the xen_console driver, but
> > > > this way at least the special-casing is kept within the Xen emulation
> > > > code, and it gives us a hook for a more complete implementation if/when
> > > > we ever do need one.
> > > > 
> > > Why can't you map the console page via the grant table like the xenstore
> > > page?
> > 
> > I suppose we could, but I didn't really want the generic xen-console
> > device code having any more of a special case for 'Xen emulation' than
> > it does already by having to call xen_primary_console_create().
> > 
> 
> But doesn't is save you the whole foreignmem thing? You can use the 
> grant table for primary and secondary consoles.

Yes. And I could leave the existing foreignmem thing just for the case
of primary console under true Xen. It's probably not that awful a
special case, in the end.

Then again, I was surprised I didn't *already* have a foreignmem ops
for the emulated case, and we're probably going to want to continue
fleshing it out later, so I don't really mind adding it.


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

Re: [PATCH 12/12] hw/xen: add support for Xen primary console in emulated mode

[Paul Durrant]

On 24/10/2023 16:49, David Woodhouse wrote:
> On Tue, 2023-10-24 at 16:39 +0100, Paul Durrant wrote:
>> On 24/10/2023 16:37, David Woodhouse wrote:
>>> On Tue, 2023-10-24 at 15:20 +0100, Paul Durrant wrote:
>>>> On 16/10/2023 16:19, David Woodhouse wrote:
>>>>> From: David Woodhouse <dwmw@amazon.co.uk>
>>>>>
>>>>> The primary console is special because the toolstack maps a page at a
>>>>> fixed GFN and also allocates the guest-side event channel. Add support
>>>>> for that in emulated mode, so that we can have a primary console.
>>>>>
>>>>> Add a *very* rudimentary stub of foriegnmem ops for emulated mode, which
>>>>> supports literally nothing except a single-page mapping of the console
>>>>> page. This might as well have been a hack in the xen_console driver, but
>>>>> this way at least the special-casing is kept within the Xen emulation
>>>>> code, and it gives us a hook for a more complete implementation if/when
>>>>> we ever do need one.
>>>>>
>>>> Why can't you map the console page via the grant table like the xenstore
>>>> page?
>>>
>>> I suppose we could, but I didn't really want the generic xen-console
>>> device code having any more of a special case for 'Xen emulation' than
>>> it does already by having to call xen_primary_console_create().
>>>
>>
>> But doesn't is save you the whole foreignmem thing? You can use the
>> grant table for primary and secondary consoles.
> 
> Yes. And I could leave the existing foreignmem thing just for the case
> of primary console under true Xen. It's probably not that awful a
> special case, in the end.
> 
> Then again, I was surprised I didn't *already* have a foreignmem ops
> for the emulated case, and we're probably going to want to continue
> fleshing it out later, so I don't really mind adding it.
> 

True. We'll need it for some of the other more fun protocols like vkbd 
or fb. Still, I think it'd be nicer to align the xenstore and primary 
console code to look similar and punt the work until then :-)

   Paul

Re: [PATCH 12/12] hw/xen: add support for Xen primary console in emulated mode

[David Woodhouse]

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

On Tue, 2023-10-24 at 17:25 +0100, Paul Durrant wrote:
> On 24/10/2023 16:49, David Woodhouse wrote:
> > On Tue, 2023-10-24 at 16:39 +0100, Paul Durrant wrote:
> > > On 24/10/2023 16:37, David Woodhouse wrote:
> > > > On Tue, 2023-10-24 at 15:20 +0100, Paul Durrant wrote:
> > > > > On 16/10/2023 16:19, David Woodhouse wrote:
> > > > > > From: David Woodhouse <dwmw@amazon.co.uk>
> > > > > > 
> > > > > > The primary console is special because the toolstack maps a page at a
> > > > > > fixed GFN and also allocates the guest-side event channel. Add support
> > > > > > for that in emulated mode, so that we can have a primary console.
> > > > > > 
> > > > > > Add a *very* rudimentary stub of foriegnmem ops for emulated mode, which
> > > > > > supports literally nothing except a single-page mapping of the console
> > > > > > page. This might as well have been a hack in the xen_console driver, but
> > > > > > this way at least the special-casing is kept within the Xen emulation
> > > > > > code, and it gives us a hook for a more complete implementation if/when
> > > > > > we ever do need one.
> > > > > > 
> > > > > Why can't you map the console page via the grant table like the xenstore
> > > > > page?
> > > > 
> > > > I suppose we could, but I didn't really want the generic xen-console
> > > > device code having any more of a special case for 'Xen emulation' than
> > > > it does already by having to call xen_primary_console_create().
> > > > 
> > > 
> > > But doesn't is save you the whole foreignmem thing? You can use the
> > > grant table for primary and secondary consoles.
> > 
> > Yes. And I could leave the existing foreignmem thing just for the case
> > of primary console under true Xen. It's probably not that awful a
> > special case, in the end.
> > 
> > Then again, I was surprised I didn't *already* have a foreignmem ops
> > for the emulated case, and we're probably going to want to continue
> > fleshing it out later, so I don't really mind adding it.
> > 
> 
> True. We'll need it for some of the other more fun protocols like vkbd 
> or fb. Still, I think it'd be nicer to align the xenstore and primary
> console code to look similar and punt the work until then :-)

I don't think it ends up looking like xenstore either way, does it?
Xenstore is special because it gets to use the original pointer to its
own page.

I don't think I want to hack the xen_console code to explicitly call a
xen_console_give_me_your_page() function. If not foreignmem, I think
you were suggesting that we actually call the grant mapping code to get
a pointer to the underlying page, right? 

I could kind of live with that... except that Xen has this ugly
convention that the "ring-ref" frontend node for the primary console
actually has the *MFN* not a grant ref. Which I don't understand since
the toolstack *does* populate the grant table for it (just as it does
for the xenstore page). But we'd have to add a special case exception
to that special case, so that in the emu case it's an actual grant ref
again. I think I prefer just having a stub of foreignmem, TBH.

(I didn't yet manage to get Xen to actually create a primary console of
type iomem, FWIW)



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

Re: [PATCH 12/12] hw/xen: add support for Xen primary console in emulated mode

[Paul Durrant]

On 24/10/2023 17:34, David Woodhouse wrote:
> On Tue, 2023-10-24 at 17:25 +0100, Paul Durrant wrote:
>> On 24/10/2023 16:49, David Woodhouse wrote:
>>> On Tue, 2023-10-24 at 16:39 +0100, Paul Durrant wrote:
>>>> On 24/10/2023 16:37, David Woodhouse wrote:
>>>>> On Tue, 2023-10-24 at 15:20 +0100, Paul Durrant wrote:
>>>>>> On 16/10/2023 16:19, David Woodhouse wrote:
>>>>>>> From: David Woodhouse <dwmw@amazon.co.uk>
>>>>>>>
>>>>>>> The primary console is special because the toolstack maps a page at a
>>>>>>> fixed GFN and also allocates the guest-side event channel. Add support
>>>>>>> for that in emulated mode, so that we can have a primary console.
>>>>>>>
>>>>>>> Add a *very* rudimentary stub of foriegnmem ops for emulated mode, which
>>>>>>> supports literally nothing except a single-page mapping of the console
>>>>>>> page. This might as well have been a hack in the xen_console driver, but
>>>>>>> this way at least the special-casing is kept within the Xen emulation
>>>>>>> code, and it gives us a hook for a more complete implementation if/when
>>>>>>> we ever do need one.
>>>>>>>
>>>>>> Why can't you map the console page via the grant table like the xenstore
>>>>>> page?
>>>>>
>>>>> I suppose we could, but I didn't really want the generic xen-console
>>>>> device code having any more of a special case for 'Xen emulation' than
>>>>> it does already by having to call xen_primary_console_create().
>>>>>
>>>>
>>>> But doesn't is save you the whole foreignmem thing? You can use the
>>>> grant table for primary and secondary consoles.
>>>
>>> Yes. And I could leave the existing foreignmem thing just for the case
>>> of primary console under true Xen. It's probably not that awful a
>>> special case, in the end.
>>>
>>> Then again, I was surprised I didn't *already* have a foreignmem ops
>>> for the emulated case, and we're probably going to want to continue
>>> fleshing it out later, so I don't really mind adding it.
>>>
>>
>> True. We'll need it for some of the other more fun protocols like vkbd
>> or fb. Still, I think it'd be nicer to align the xenstore and primary
>> console code to look similar and punt the work until then :-)
> 
> I don't think it ends up looking like xenstore either way, does it?
> Xenstore is special because it gets to use the original pointer to its
> own page.
> 

Not sure what you mean there? A guest can query the PFN for either 
xenstore or console using HVM params, or it can find them in its own 
grant table entries 0 or 1.

> I don't think I want to hack the xen_console code to explicitly call a
> xen_console_give_me_your_page() function. If not foreignmem, I think
> you were suggesting that we actually call the grant mapping code to get
> a pointer to the underlying page, right?

I'm suggesting that the page be mapped in the same way that the xenstore 
backend does:

1462    /* 

1463     * We don't actually access the guest's page through the grant, 
because
1464     * this isn't real Xen, and we can just use the page we gave it 
in the
1465     * first place. Map the grant anyway, mostly for cosmetic 
purposes so
1466     * it *looks* like it's in use in the guest-visible grant table. 

1467     */
1468    s->gt = qemu_xen_gnttab_open();
1469    uint32_t xs_gntref = GNTTAB_RESERVED_XENSTORE;
1470    s->granted_xs = qemu_xen_gnttab_map_refs(s->gt, 1, xen_domid, 
&xs_gntref,
1471                                             PROT_READ | PROT_WRITE);


> 
> I could kind of live with that... except that Xen has this ugly
> convention that the "ring-ref" frontend node for the primary console
> actually has the *MFN* not a grant ref. Which I don't understand since
> the toolstack *does* populate the grant table for it (just as it does
> for the xenstore page). But we'd have to add a special case exception
> to that special case, so that in the emu case it's an actual grant ref
> again. I think I prefer just having a stub of foreignmem, TBH.
> 

You're worried about the guest changing the page it uses for the primary 
console and putting a new one in xenstore? I'd be amazed if that even 
works on Xen unless the guest is careful to write it into 
GNTTAB_RESERVED_CONSOLE.

> (I didn't yet manage to get Xen to actually create a primary console of
> type iomem, FWIW)
> 

No, that doesn't entirely surprise me.

   Paul

Re: [PATCH 12/12] hw/xen: add support for Xen primary console in emulated mode

[David Woodhouse]

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

On Wed, 2023-10-25 at 09:31 +0100, Paul Durrant wrote:
> On 24/10/2023 17:34, David Woodhouse wrote:
> > On Tue, 2023-10-24 at 17:25 +0100, Paul Durrant wrote:
> > > On 24/10/2023 16:49, David Woodhouse wrote:
> > > > On Tue, 2023-10-24 at 16:39 +0100, Paul Durrant wrote:
> > > > > On 24/10/2023 16:37, David Woodhouse wrote:
> > > > > > On Tue, 2023-10-24 at 15:20 +0100, Paul Durrant wrote:
> > > > > > > On 16/10/2023 16:19, David Woodhouse wrote:
> > > > > > > > From: David Woodhouse <dwmw@amazon.co.uk>
> > > > > > > > 
> > > > > > > > The primary console is special because the toolstack maps a page at a
> > > > > > > > fixed GFN and also allocates the guest-side event channel. Add support
> > > > > > > > for that in emulated mode, so that we can have a primary console.
> > > > > > > > 
> > > > > > > > Add a *very* rudimentary stub of foriegnmem ops for emulated mode, which
> > > > > > > > supports literally nothing except a single-page mapping of the console
> > > > > > > > page. This might as well have been a hack in the xen_console driver, but
> > > > > > > > this way at least the special-casing is kept within the Xen emulation
> > > > > > > > code, and it gives us a hook for a more complete implementation if/when
> > > > > > > > we ever do need one.
> > > > > > > > 
> > > > > > > Why can't you map the console page via the grant table like the xenstore
> > > > > > > page?
> > > > > > 
> > > > > > I suppose we could, but I didn't really want the generic xen-console
> > > > > > device code having any more of a special case for 'Xen emulation' than
> > > > > > it does already by having to call xen_primary_console_create().
> > > > > > 
> > > > > 
> > > > > But doesn't is save you the whole foreignmem thing? You can use the
> > > > > grant table for primary and secondary consoles.
> > > > 
> > > > Yes. And I could leave the existing foreignmem thing just for the case
> > > > of primary console under true Xen. It's probably not that awful a
> > > > special case, in the end.
> > > > 
> > > > Then again, I was surprised I didn't *already* have a foreignmem ops
> > > > for the emulated case, and we're probably going to want to continue
> > > > fleshing it out later, so I don't really mind adding it.
> > > > 
> > > 
> > > True. We'll need it for some of the other more fun protocols like vkbd
> > > or fb. Still, I think it'd be nicer to align the xenstore and primary
> > > console code to look similar and punt the work until then :-)
> > 
> > I don't think it ends up looking like xenstore either way, does it?
> > Xenstore is special because it gets to use the original pointer to its
> > own page.
> > 
> 
> Not sure what you mean there? A guest can query the PFN for either 
> xenstore or console using HVM params, or it can find them in its own 
> grant table entries 0 or 1.

The code in our xen_xenstore.c uses its *own* pointer (s->xs) to the
MemoryRegion that it created (s->xenstore_page). It is its own backend,
as well as doing the "magic" to create the guest-side mapping and event
channel.

The difference for the console code is that we actually have a
*separation* between the standard backend code in xen_console.c, and
the magic frontend parts for the emulated mode.


> 
> > I don't think I want to hack the xen_console code to explicitly call a
> > xen_console_give_me_your_page() function. If not foreignmem, I think
> > you were suggesting that we actually call the grant mapping code to get
> > a pointer to the underlying page, right?
> 
> I'm suggesting that the page be mapped in the same way that the xenstore 
> backend does:
> 
> 1462    /* 
> 
> 1463     * We don't actually access the guest's page through the grant, because
> 1464     * this isn't real Xen, and we can just use the page we gave it in the
> 1465     * first place. Map the grant anyway, mostly for cosmetic purposes so
> 1466     * it *looks* like it's in use in the guest-visible grant table. 
> 1467     */
> 1468    s->gt = qemu_xen_gnttab_open();
> 1469    uint32_t xs_gntref = GNTTAB_RESERVED_XENSTORE;
> 1470    s->granted_xs = qemu_xen_gnttab_map_refs(s->gt, 1, xen_domid, &xs_gntref,
> 1471                                             PROT_READ | PROT_WRITE);

It already *is*. But as with xen_xenstore.c, nothing ever *uses* the
s->granted_xs pointer. It's just cosmetic to make the grant table look
right.

But that doesn't help the *backend* code. The backend doesn't even know
the grant ref#, because the convention we inherited from Xen is that
the `ring-ref` in XenStore for the primary console is actually the MFN,
to be mapped as foreignmem.

Of course, we *do* know the grant-ref for the primary console, as it's
always GNTTAB_RESERVED_CONSOLE. So I suppose we could put a hack into
the xen_console backend to map *that* in the case of primary console
under emu? In fact that would probably do the right thing even under
Xen if we could persuade Xen to make an ioemu primary console?





> > 
> > I could kind of live with that... except that Xen has this ugly
> > convention that the "ring-ref" frontend node for the primary console
> > actually has the *MFN* not a grant ref. Which I don't understand since
> > the toolstack *does* populate the grant table for it (just as it does
> > for the xenstore page). But we'd have to add a special case exception
> > to that special case, so that in the emu case it's an actual grant ref
> > again. I think I prefer just having a stub of foreignmem, TBH.
> > 
> 
> You're worried about the guest changing the page it uses for the primary 
> console and putting a new one in xenstore? I'd be amazed if that even
> works on Xen unless the guest is careful to write it into 
> GNTTAB_RESERVED_CONSOLE.

Not worried about the guest changing it. I was mostly just concerned
about the xen-console having to have another special case and magically
"know" it. But I suppose I can live with it being hard-coded to
GNTTAB_RESERVED_CONSOLE. I'll knock that up and see how it makes me
feel.

I'm reworking some of that connect/disconnect code anyway, to have the
backend tell the primary_console code directly what the backend port#
is, so I can remove the soft-reset hacks in xen_evtchn.c entirely.

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

Re: [PATCH 12/12] hw/xen: add support for Xen primary console in emulated mode

[Paul Durrant]

On 25/10/2023 10:00, David Woodhouse wrote:
> On Wed, 2023-10-25 at 09:31 +0100, Paul Durrant wrote:
>> On 24/10/2023 17:34, David Woodhouse wrote:
>>> On Tue, 2023-10-24 at 17:25 +0100, Paul Durrant wrote:
>>>> On 24/10/2023 16:49, David Woodhouse wrote:
>>>>> On Tue, 2023-10-24 at 16:39 +0100, Paul Durrant wrote:
>>>>>> On 24/10/2023 16:37, David Woodhouse wrote:
>>>>>>> On Tue, 2023-10-24 at 15:20 +0100, Paul Durrant wrote:
>>>>>>>> On 16/10/2023 16:19, David Woodhouse wrote:
>>>>>>>>> From: David Woodhouse <dwmw@amazon.co.uk>
>>>>>>>>>
>>>>>>>>> The primary console is special because the toolstack maps a page at a
>>>>>>>>> fixed GFN and also allocates the guest-side event channel. Add support
>>>>>>>>> for that in emulated mode, so that we can have a primary console.
>>>>>>>>>
>>>>>>>>> Add a *very* rudimentary stub of foriegnmem ops for emulated mode, which
>>>>>>>>> supports literally nothing except a single-page mapping of the console
>>>>>>>>> page. This might as well have been a hack in the xen_console driver, but
>>>>>>>>> this way at least the special-casing is kept within the Xen emulation
>>>>>>>>> code, and it gives us a hook for a more complete implementation if/when
>>>>>>>>> we ever do need one.
>>>>>>>>>
>>>>>>>> Why can't you map the console page via the grant table like the xenstore
>>>>>>>> page?
>>>>>>>
>>>>>>> I suppose we could, but I didn't really want the generic xen-console
>>>>>>> device code having any more of a special case for 'Xen emulation' than
>>>>>>> it does already by having to call xen_primary_console_create().
>>>>>>>
>>>>>>
>>>>>> But doesn't is save you the whole foreignmem thing? You can use the
>>>>>> grant table for primary and secondary consoles.
>>>>>
>>>>> Yes. And I could leave the existing foreignmem thing just for the case
>>>>> of primary console under true Xen. It's probably not that awful a
>>>>> special case, in the end.
>>>>>
>>>>> Then again, I was surprised I didn't *already* have a foreignmem ops
>>>>> for the emulated case, and we're probably going to want to continue
>>>>> fleshing it out later, so I don't really mind adding it.
>>>>>
>>>>
>>>> True. We'll need it for some of the other more fun protocols like vkbd
>>>> or fb. Still, I think it'd be nicer to align the xenstore and primary
>>>> console code to look similar and punt the work until then :-)
>>>
>>> I don't think it ends up looking like xenstore either way, does it?
>>> Xenstore is special because it gets to use the original pointer to its
>>> own page.
>>>
>>
>> Not sure what you mean there? A guest can query the PFN for either
>> xenstore or console using HVM params, or it can find them in its own
>> grant table entries 0 or 1.
> 
> The code in our xen_xenstore.c uses its *own* pointer (s->xs) to the
> MemoryRegion that it created (s->xenstore_page). It is its own backend,
> as well as doing the "magic" to create the guest-side mapping and event
> channel.
> 
> The difference for the console code is that we actually have a
> *separation* between the standard backend code in xen_console.c, and
> the magic frontend parts for the emulated mode.
> 
> 
>>
>>> I don't think I want to hack the xen_console code to explicitly call a
>>> xen_console_give_me_your_page() function. If not foreignmem, I think
>>> you were suggesting that we actually call the grant mapping code to get
>>> a pointer to the underlying page, right?
>>
>> I'm suggesting that the page be mapped in the same way that the xenstore
>> backend does:
>>
>> 1462    /*
>>
>> 1463     * We don't actually access the guest's page through the grant, because
>> 1464     * this isn't real Xen, and we can just use the page we gave it in the
>> 1465     * first place. Map the grant anyway, mostly for cosmetic purposes so
>> 1466     * it *looks* like it's in use in the guest-visible grant table.
>> 1467     */
>> 1468    s->gt = qemu_xen_gnttab_open();
>> 1469    uint32_t xs_gntref = GNTTAB_RESERVED_XENSTORE;
>> 1470    s->granted_xs = qemu_xen_gnttab_map_refs(s->gt, 1, xen_domid, &xs_gntref,
>> 1471                                             PROT_READ | PROT_WRITE);
> 
> It already *is*. But as with xen_xenstore.c, nothing ever *uses* the
> s->granted_xs pointer. It's just cosmetic to make the grant table look
> right.
> 
> But that doesn't help the *backend* code. The backend doesn't even know
> the grant ref#, because the convention we inherited from Xen is that
> the `ring-ref` in XenStore for the primary console is actually the MFN,
> to be mapped as foreignmem.
> 
> Of course, we *do* know the grant-ref for the primary console, as it's
> always GNTTAB_RESERVED_CONSOLE. So I suppose we could put a hack into
> the xen_console backend to map *that* in the case of primary console
> under emu? In fact that would probably do the right thing even under
> Xen if we could persuade Xen to make an ioemu primary console?
> 

That's exactly what I am getting at :-) I don't think we need care about 
the ring-ref in xenstore for the primary console.

   Paul

> 
> 
> 
> 
>>>
>>> I could kind of live with that... except that Xen has this ugly
>>> convention that the "ring-ref" frontend node for the primary console
>>> actually has the *MFN* not a grant ref. Which I don't understand since
>>> the toolstack *does* populate the grant table for it (just as it does
>>> for the xenstore page). But we'd have to add a special case exception
>>> to that special case, so that in the emu case it's an actual grant ref
>>> again. I think I prefer just having a stub of foreignmem, TBH.
>>>
>>
>> You're worried about the guest changing the page it uses for the primary
>> console and putting a new one in xenstore? I'd be amazed if that even
>> works on Xen unless the guest is careful to write it into
>> GNTTAB_RESERVED_CONSOLE.
> 
> Not worried about the guest changing it. I was mostly just concerned
> about the xen-console having to have another special case and magically
> "know" it. But I suppose I can live with it being hard-coded to
> GNTTAB_RESERVED_CONSOLE. I'll knock that up and see how it makes me
> feel.
> 
> I'm reworking some of that connect/disconnect code anyway, to have the
> backend tell the primary_console code directly what the backend port#
> is, so I can remove the soft-reset hacks in xen_evtchn.c entirely.

Re: [PATCH 0/12] Get Xen PV shim running in qemu

[Alex Bennée]

David Woodhouse <dwmw2@infradead.org> writes:

> I hadn't got round to getting the PV shim running yet; I thought it would
> need work on the multiboot loader. Turns out it doesn't. I *did* need to
> fix a couple of brown-paper-bag bugs in the per-vCPU upcall vector support,
> and implement Xen console support though. Now I can test PV guests:
>
>  $ qemu-system-x86_64 --accel kvm,xen-version=0x40011,kernel-irqchip=split \
>    -chardev stdio,mux=on,id=char0 -device xen-console,chardev=char0 \
>    -drive file=${GUEST_IMAGE},if=xen -display none -m 1G \
>    -kernel ~/git/xen/xen/xen -initrd ~/git/linux/arch/x86/boot/bzImage
>  \

So this is a KVM guest running the Xen hypervisor (via -kernel) and a
Dom0 Linux guest (via -initrd)?

Should this work for any Xen architecture or is this x86 specific? Does
the -M machine model matter?

Would it be possible to have some sort of overview document in our
manual for how Xen guests are supported under KVM?

>    -append "loglvl=all -- console=hvc0 root=/dev/xvda1"
>
<snip>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 0/12] Get Xen PV shim running in qemu

[David Woodhouse]

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

On Tue, 2023-10-24 at 16:24 +0100, Alex Bennée wrote:
> 
> David Woodhouse <dwmw2@infradead.org> writes:
> 
> > I hadn't got round to getting the PV shim running yet; I thought it would
> > need work on the multiboot loader. Turns out it doesn't. I *did* need to
> > fix a couple of brown-paper-bag bugs in the per-vCPU upcall vector support,
> > and implement Xen console support though. Now I can test PV guests:
> > 
> >  $ qemu-system-x86_64 --accel kvm,xen-version=0x40011,kernel-irqchip=split \
> >    -chardev stdio,mux=on,id=char0 -device xen-console,chardev=char0 \
> >    -drive file=${GUEST_IMAGE},if=xen -display none -m 1G \
> >    -kernel ~/git/xen/xen/xen -initrd ~/git/linux/arch/x86/boot/bzImage
> >  \
> 

(Reordering your questions so the answers flow better)

> Would it be possible to have some sort of overview document in our
> manual for how Xen guests are supported under KVM?

https://qemu-project.gitlab.io/qemu/system/i386/xen.html covers running
Xen HVM guests under Qemu/KVM.

What I'm adding here is the facility to support Xen PV guests. There is
a corresponding update to the documentation in my working tree at 
https://git.infradead.org/users/dwmw2/qemu.git/shortlog/refs/heads/xenfv

https://git.infradead.org/users/dwmw2/qemu.git/commitdiff/af693bf51141

PV mode is the old mode which predates proper virtualization support in
the CPUs, where a guest kernel *knows* it doesn't have access to real
(or indeed virtualized) hardware. It runs in ring 1 (or ring 3 on
x86_64) and makes hypercalls to Xen to ask it to do all the MMU
management.

When Spectre/Meltdown happened, running actual PV guests directly under
Xen became kind of insane, so we hacked a version of Xen to work as a
"shim", running inside a proper HVM guest, and just providing those MMU
management services to its guest. Its *only* guest. This shim doesn't
even do any of the PV disk/block stuff; that's passed through directly
to the real hypervisor.

So you have a real Xen hypervisor, then a "PV shim" Xen running inside
that hardware virtual machine, and a guest kernel hosted by that PV
shim.

Now, since Qemu/KVM can now pretend to be Xen and host guests that
think they're running as Xen HVM guests... Qemu/KVM can host that PV
shim too. As noted, I just had to realise that we could use '-initrd'
to trick Qemu's multiboot loader into doing it... and fix a few brown
paper bag bugs.

> So this is a KVM guest running the Xen hypervisor (via -kernel) and a
> Dom0 Linux guest (via -initrd)?

Fairly much. It's a KVM guest running that "shim" version of the Xen
hypervisor via -kernel, and a Linux guest via -initrd.

Although I wouldn't call that a "Dom0 Linux guest" because we tend to
use "Dom0" to mean the control domain, which can launch other (DomU)
guests... and that isn't what's happening here. It's more of a "Dom1".
The one and only unprivileged guest.

In particular, there's no nested virtualization here. Because in that
sense, what "Xen" does to host a PV guest isn't really virtualization.

> Should this work for any Xen architecture or is this x86 specific? Does
> the -M machine model matter?

It's currently x86-specific and KVM-specific. You can use the pc or q35
models as you see fit, although see the doc linked above for discussion
of the IDE 'unplug' mechanism. And recent patches on the list to fix it
for q35.

It would be interesting to make it work on other platforms, and even
with TCG. I've tried to keep it as portable as possible up to a point,
but without too much gratuitous overengineering to chase that goal.

Making it work with TCG would require dealing with all the struct
layouts where alignment/padding differs on different host
architectures, so we probably wouldn't be able to use the Xen public
header files directly. And we would need to implement some of the basic
event channel delivery and shared info page handling that we rely on
KVM to do for us. The latter probably isn't that hard; the former is
what stopped me even bothering.

Making it work for e.g. Arm would require porting some of the KVM
support to Arm in the kernel (it's currently x86-specific). And/or
making it work for TCG.... but the parts that *are* accelerated in the
kernel (timers, IPIs, etc) are there for a reason though. If we do make
it work for TCG by implementing those in userspace, I wouldn't
necessarily want a *KVM* guest to have to rely on those in userspace.

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