Re: [PATCH v1 09/10] hw/i386/xen: Add a Xen PVH x86 machine

[Stefano Stabellini]

On Wed, 14 Aug 2024, Edgar E. Iglesias wrote:
> On Mon, Aug 12, 2024 at 06:48:52PM -0700, Stefano Stabellini wrote:
> > On Mon, 12 Aug 2024, Edgar E. Iglesias wrote:
> > > From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
> > > 
> > > This adds a Xen PVH x86 machine based on the PVH Common
> > > module used by the ARM PVH machine.
> > > 
> > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
> > > ---
> > >  hw/i386/xen/meson.build |   1 +
> > >  hw/i386/xen/xen-pvh.c   | 196 ++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 197 insertions(+)
> > >  create mode 100644 hw/i386/xen/xen-pvh.c
> > > 
> > > diff --git a/hw/i386/xen/meson.build b/hw/i386/xen/meson.build
> > > index 3f0df8bc07..c73c62b8e3 100644
> > > --- a/hw/i386/xen/meson.build
> > > +++ b/hw/i386/xen/meson.build
> > > @@ -4,6 +4,7 @@ i386_ss.add(when: 'CONFIG_XEN', if_true: files(
> > >  ))
> > >  i386_ss.add(when: ['CONFIG_XEN', xen], if_true: files(
> > >    'xen-hvm.c',
> > > +  'xen-pvh.c',
> > >  ))
> > >  
> > >  i386_ss.add(when: 'CONFIG_XEN_BUS', if_true: files(
> > > diff --git a/hw/i386/xen/xen-pvh.c b/hw/i386/xen/xen-pvh.c
> > > new file mode 100644
> > > index 0000000000..9c3d3fc58d
> > > --- /dev/null
> > > +++ b/hw/i386/xen/xen-pvh.c
> > > @@ -0,0 +1,196 @@
> > > +/*
> > > + * QEMU Xen PVH x86 Machine
> > > + *
> > > + * Copyright (c) 2024 Advanced Micro Devices, Inc.
> > > + * Written by Edgar E. Iglesias <edgar.iglesias@amd.com>
> > > + *
> > > + * SPDX-License-Identifier: GPL-2.0-or-later
> > > + */
> > > +
> > > +#include "qemu/osdep.h"
> > > +#include "qapi/error.h"
> > > +#include "qapi/visitor.h"
> > > +#include "qemu/error-report.h"
> > > +#include "hw/boards.h"
> > > +#include "sysemu/sysemu.h"
> > > +#include "hw/xen/arch_hvm.h"
> > > +#include "hw/xen/xen.h"
> > > +#include "hw/xen/xen-pvh-common.h"
> > > +
> > > +#define TYPE_XEN_PVH_X86  MACHINE_TYPE_NAME("xenpvh")
> > > +OBJECT_DECLARE_SIMPLE_TYPE(XenPVHx86State, XEN_PVH_X86)
> > > +
> > > +#define PVH_MAX_CPUS 128
> > > +
> > > +struct XenPVHx86State {
> > > +    /*< private >*/
> > > +    MachineState parent;
> > > +
> > > +    DeviceState *cpu[PVH_MAX_CPUS];
> > > +    XenPVHCommonState pvh;
> > > +
> > > +    /*
> > > +     * We provide these properties to allow Xen to move things to other
> > > +     * addresses for example when users need to accomodate the memory-map
> > > +     * for 1:1 mapped devices/memory.
> > > +     */
> > > +    struct {
> > > +        MemMapEntry ram_low, ram_high;
> > > +        MemMapEntry pci_ecam, pci_mmio, pci_mmio_high;
> > 
> > Can we use the same properties already present under XenPVHCommonState?
> > 
> > 
> > 
> > > +    } cfg;
> > > +};
> > > +
> > > +static void xenpvh_cpu_new(MachineState *ms,
> > > +                           XenPVHx86State *xp,
> > > +                           int cpu_idx,
> > > +                           int64_t apic_id)
> > > +{
> > > +    Object *cpu = object_new(ms->cpu_type);
> > > +
> > > +    object_property_add_child(OBJECT(ms), "cpu[*]", cpu);
> > > +    object_property_set_uint(cpu, "apic-id", apic_id, &error_fatal);
> > > +    qdev_realize(DEVICE(cpu), NULL, &error_fatal);
> > > +    object_unref(cpu);
> > > +
> > > +    xp->cpu[cpu_idx] = DEVICE(cpu);
> > 
> > 
> > Why do we need to create CPU devices in QEMU given that we are only
> > doing device emulation? I guess it is because ....
> > 
> > 
> > 
> > > +}
> > > +
> > > +static void xenpvh_init(MachineState *ms)
> > > +{
> > > +    XenPVHx86State *xp = XEN_PVH_X86(ms);
> > > +    const struct {
> > > +        const char *name;
> > > +        MemMapEntry *map;
> > > +    } map[] = {
> > > +        { "ram-low", &xp->cfg.ram_low },
> > > +        { "ram-high", &xp->cfg.ram_high },
> > > +        { "pci-ecam", &xp->cfg.pci_ecam },
> > > +        { "pci-mmio", &xp->cfg.pci_mmio },
> > > +        { "pci-mmio-high", &xp->cfg.pci_mmio_high },
> > > +    };
> > > +    int i;
> > > +
> > > +    object_initialize_child(OBJECT(ms), "pvh", &xp->pvh, 
> > > TYPE_XEN_PVH_COMMON);
> > > +    object_property_set_int(OBJECT(&xp->pvh), "max-cpus", 
> > > ms->smp.max_cpus,
> > > +                            &error_abort);
> > > +    object_property_set_int(OBJECT(&xp->pvh), "ram-size", ms->ram_size,
> > > +                            &error_abort);
> > > +
> > > +    for (i = 0; i < ARRAY_SIZE(map); i++) {
> > > +        g_autofree char *base_name = g_strdup_printf("%s-base", 
> > > map[i].name);
> > > +        g_autofree char *size_name = g_strdup_printf("%s-size", 
> > > map[i].name);
> > > +
> > > +        object_property_set_int(OBJECT(&xp->pvh), base_name, 
> > > map[i].map->base,
> > > +                                 &error_abort);
> > > +        object_property_set_int(OBJECT(&xp->pvh), size_name, 
> > > map[i].map->size,
> > > +                                 &error_abort);
> > > +    }
> > > +
> > > +    /* GSI's 16 - 20 are used for legacy PCIe INTX IRQs.  */
> > > +    object_property_set_int(OBJECT(&xp->pvh), "pci-intx-irq-base", 16,
> > > +                            &error_abort);
> > > +
> > > +    sysbus_realize(SYS_BUS_DEVICE(&xp->pvh), &error_abort);
> > > +
> > > +    /* Create dummy cores. This will indirectly create the APIC MSI 
> > > window.  */
> > 
> > ... of the APIC MSI window ?
> 
> 
> Yes, exactly. I did have a first version without the CPUs that only had
> the MSI window but there was a bit of disentanglement needed and some
> other issue that I forgot. Note that with TCG disabled, this doesn't
> have the any CPU emulation so it doesn't affect our small PVH config
> very much. I could look into the MSI windows again though.

no, I think this is OK, especially if the number of CPUs is accurate