Re: [PATCH v2 3/6] tools: remove support for running a guest with qemu-traditional

[Juergen Gross <jgross@xxxxxxxx>]

On 24.04.25 15:41, Anthony PERARD wrote:
> On Tue, Apr 08, 2025 at 02:35:21PM +0200, Juergen Gross wrote:
> > diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
> > index c3c61ca060..a00acdd45b 100644
> > --- a/tools/firmware/hvmloader/pci.c
> > +++ b/tools/firmware/hvmloader/pci.c
> > @@ -113,19 +113,9 @@ void pci_setup(void)
> >        * increase the size of the lowmem MMIO hole?  Defaulting to 1
> This say "default to 1", but the default is changing to 0 instead.
No, it explains the consequences in case the default would be 1.

> >        * here will mean that non-libxl toolstacks (including xend and
> >        * home-grown ones) means that those using qemu-xen will still
> > -     * experience the memory relocation bug described below; but it
> > -     * also means that those using qemu-traditional will *not*
> > -     * experience any change; and it also means that there is a
> > -     * work-around for those using qemu-xen, namely switching to
> > -     * qemu-traditional.
> > -     *
> > -     * If we defaulted to 0, and failing to resize the hole caused any
> > -     * problems with qemu-traditional, then there is no work-around.
> > -     *
> > -     * Since xend can only use qemu-traditional, I think this is the
> > -     * option that will have the least impact.
> > +     * experience the memory relocation bug described below.
> >        */
> > -    bool allow_memory_relocate = 1;
> > +    bool allow_memory_relocate = 0;
> I would keep this as 1. libxl does set it to 0 so there's no need to
> change the default here. I don't know if non-libxl toolstack will set
> the xenstore entry "hvmloader/allow-memory-relocate" with "1" if they
> need it.
Fair enough.

> >   
> >       BUILD_BUG_ON((typeof(*pci_devfn_decode_type))PCI_COMMAND_IO !=
> >                    PCI_COMMAND_IO);
> > @@ -347,10 +337,7 @@ void pci_setup(void)
> >       {
> >           /*
> >            * At the moment qemu-xen can't deal with relocated memory regions.
> > -         * It's too close to the release to make a proper fix; for now,
> > -         * only allow the MMIO hole to grow large enough to move guest memory
> > -         * if we're running qemu-traditional.  Items that don't fit will be
> > -         * relocated into the 64-bit address space.
> > +         * It's too close to the release to make a proper fix.
> I feel like this is removing too much information. Maybe instead of "if
> we are running qemu-traditional", we could have "if
> allow_memory_relocate is true".
Fine with me, but I think it is time to remove the reference to the release
being too close.

> >            *
> >            * This loop now does the following:
> >            * - If allow_memory_relocate, increase the MMIO hole until it's
> > diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
> > index 2d07ce1290..79c0e6bd4a 100644
> > --- a/tools/firmware/hvmloader/util.c
> > +++ b/tools/firmware/hvmloader/util.c
> > @@ -843,14 +843,7 @@ void hvmloader_acpi_build_tables(struct acpi_config 
> > *config,
> >   
> >       /* If the device model is specified switch to the corresponding tables */
> >       s = xenstore_read("platform/device-model", "");
> > -    if ( !strncmp(s, "qemu_xen_traditional", 21) )
> > -    {
> > -        config->dsdt_anycpu = dsdt_anycpu;
> > -        config->dsdt_anycpu_len = dsdt_anycpu_len;
> > -        config->dsdt_15cpu = dsdt_15cpu;
> > -        config->dsdt_15cpu_len = dsdt_15cpu_len;
> Those `dsdt_anycpu` and `dsdt_15cpu` blob are still built in. The are
> both associated with qemu-trad so should not be included in hvmloader
> anymore.
Okay, I'll remove them.

> > -    }
> > -    else if ( !strncmp(s, "qemu_xen", 9) )
> > +    if ( !strncmp(s, "qemu_xen", 9) )
> >       {
> >           config->dsdt_anycpu = dsdt_anycpu_qemu_xen;
> >           config->dsdt_anycpu_len = dsdt_anycpu_qemu_xen_len;
> > diff --git a/tools/golang/xenlight/types.gen.go 
> > b/tools/golang/xenlight/types.gen.go
> > index e7667f1ce3..02c4d8d123 100644
> > --- a/tools/golang/xenlight/types.gen.go
> > +++ b/tools/golang/xenlight/types.gen.go
> > @@ -70,7 +70,6 @@ ChannelConnectionSocket ChannelConnection = 2
> >   type DeviceModelVersion int
> >   const(
> >   DeviceModelVersionUnknown DeviceModelVersion = 0
> > -DeviceModelVersionQemuXenTraditional DeviceModelVersion = 1
> >   DeviceModelVersionQemuXen DeviceModelVersion = 2
> >   )
> >   
> > diff --git a/tools/libacpi/mk_dsdt.c b/tools/libacpi/mk_dsdt.c
> > index 34f6753f61..396e3e01e2 100644
> > --- a/tools/libacpi/mk_dsdt.c
> > +++ b/tools/libacpi/mk_dsdt.c
> > @@ -105,7 +80,7 @@ int main(int argc, char **argv)
> >   {
> >       unsigned int cpu, max_cpus;
> >   #if defined(CONFIG_X86)
> > -    dm_version dm_version = QEMU_XEN_TRADITIONAL;
> > +    dm_version dm_version = QEMU_XEN;
> This change means that we are going to have "dsdt_anycpu" (.asl and blob)
> been the same as "dsdt_anycpu_qemu_xen". Instead of changing the
> default, could you ensure that `--dm-version` is provided on the command
> line or return an error? No more default `dm_version`.
Okay.

> >       unsigned int slot, dev, intx, link;
> >   
> >       max_cpus = HVM_MAX_VCPUS;
> > diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
> > index e03599ea99..9c56c4c9e5 100644
> > --- a/tools/libs/light/libxl_create.c
> > +++ b/tools/libs/light/libxl_create.c
> > @@ -100,12 +100,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
> >   
> >       if (!b_info->device_model_version) {
> A lot more can be removed in this blocks. In all cases, we set
> device_model_version=qemu-xen. Which mean it's qemu-xen in the condition
> just after.
Right. I'll simplify that.

> >           if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
> > -            if (libxl_defbool_val(b_info->device_model_stubdomain)) {
> > -                b_info->device_model_version =
> > -                    LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
> > -            } else {
> > -                b_info->device_model_version = 
> > LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
> > -            }
> > +            b_info->device_model_version = LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
> >           } else {
> >               b_info->device_model_version =
> >                   LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
> > @@ -117,16 +112,8 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
> >               dm = libxl__domain_device_model(gc, b_info);
> >               rc = access(dm, X_OK);
> >               if (rc < 0) {
> > -                /* qemu-xen unavailable, use qemu-xen-traditional */
> > -                if (errno == ENOENT) {
> > -                    LOGE(INFO, "qemu-xen is unavailable"
> > -                         ", using qemu-xen-traditional instead");
> > -                    b_info->device_model_version =
> > -                        LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
> > -                } else {
> > -                    LOGE(ERROR, "qemu-xen access error");
> > -                    return ERROR_FAIL;
> > -                }
> > +                LOGE(ERROR, "qemu-xen access error");
> > +                return ERROR_FAIL;
> >               }
> >           }
> >       }
> > diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
> > index b193a5dc37..4146ee0ea1 100644
> > --- a/tools/libs/light/libxl_dm.c
> > +++ b/tools/libs/light/libxl_dm.c
> > @@ -2429,16 +2155,11 @@ void libxl__spawn_stub_dm(libxl__egc *egc, 
> > libxl__stub_dm_spawn_state *sdss)
> >                           "%s",
> >                           
> > libxl_bios_type_to_string(guest_config->b_info.u.hvm.bios));
> >       }
> > -    /* Disable relocating memory to make the MMIO hole larger
> > -     * unless we're running qemu-traditional and vNUMA is not
> > -     * configured. */
> > +    /* Disable relocating memory to make the MMIO hole larger. */
> I don't think this sentence as the right meaning :-). I feel like this
> read as " the following make the MMIO hole larger", but I think it is
> just an explanation of what "relocating memory" mean, and what is
> messing.
>
> We might want to rewrite the comment, "disable relocating memory, see
> comment in hvmloader about the meaning". Or "disable relocating memory,
> having a lager MMIO hole isn't implemented with qemu-xen". Or something
> like that.
I like the second variant better.

> There's the same comment in the non-stubdom code path
> (device_model_launch) just below..
>
> >       libxl__xs_printf(gc, XBT_NULL,
> >                        libxl__sprintf(gc, "%s/hvmloader/allow-memory-relocate",
> >                                       libxl__xs_get_dompath(gc, guest_domid)),
> > -                     "%d",
> > -                     guest_config->b_info.device_model_version
> > -                        == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL &&
> > -                     !libxl__vnuma_configured(&guest_config->b_info));
> > +                     "0");
> >       ret = xc_domain_set_target(ctx->xch, dm_domid, guest_domid);
> >       if (ret<0) {
> >           LOGED(ERROR, guest_domid, "setting target domain %d -> %d",
> > @@ -3162,26 +2879,16 @@ static void device_model_launch(libxl__egc *egc,
> >           libxl__xs_printf(gc, XBT_NULL,
> >                            GCSPRINTF("%s/hvmloader/bios", path),
> >                            "%s", libxl_bios_type_to_string(b_info->u.hvm.bios));
> > -        /* Disable relocating memory to make the MMIO hole larger
> > -         * unless we're running qemu-traditional and vNUMA is not
> > -         * configured. */
> > +        /* Disable relocating memory to make the MMIO hole larger. */
> >           libxl__xs_printf(gc, XBT_NULL,
> >                            GCSPRINTF("%s/hvmloader/allow-memory-relocate", 
> > path),
> > -                         "%d",
> > -                         
> > b_info->device_model_version==LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL &&
> > -                         !libxl__vnuma_configured(b_info));
> > +                         "0");
> >           free(path);
> >       }
> >   
> > diff --git a/tools/libs/light/libxl_dom_suspend.c b/tools/libs/light/libxl_dom_suspend.c
> > index 6091a5f3f6..f0a74fc82c 100644
> > --- a/tools/libs/light/libxl_dom_suspend.c
> > +++ b/tools/libs/light/libxl_dom_suspend.c
> > @@ -85,15 +85,8 @@ void libxl__domain_suspend_device_model(libxl__egc *egc,
> >       STATE_AO_GC(dsps->ao);
> >       int rc = 0;
> >       uint32_t const domid = dsps->domid;
> > -    const char *const filename = dsps->dm_savefile;
> >   
> >       switch (libxl__device_model_version_running(gc, domid)) {
> > -    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL: {
> > -        LOGD(DEBUG, domid, "Saving device model state to %s", filename);
> > -        libxl__qemu_traditional_cmd(gc, domid, "save");
> > -        libxl__wait_for_device_model_deprecated(gc, domid, "paused", NULL, 
> > NULL, NULL);
> That function isn't used anymore, and can be removed from libxl. Same
> for libxl__xenstore_child_wait_deprecated() which is used by it.
I'll remove those.

> > -        break;
> > -    }
> >       case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> >           /* calls dsps->callback_device_model_done when done */
> >           libxl__qmp_suspend_save(egc, dsps); /* must be last */
> > diff --git a/tools/libs/light/libxl_stream_write.c 
> > b/tools/libs/light/libxl_stream_write.c
> > index 634f3240d1..98d44597a7 100644
> > --- a/tools/libs/light/libxl_stream_write.c
> > +++ b/tools/libs/light/libxl_stream_write.c
> > @@ -252,10 +252,6 @@ void libxl__stream_write_start(libxl__egc *egc,
> >           stream->device_model_version =
> >               libxl__device_model_version_running(gc, dss->domid);
> >           switch (stream->device_model_version) {
> > -        case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
> > -            stream->emu_sub_hdr.id = EMULATOR_QEMU_TRADITIONAL;
> EMULATOR_QEMU_TRADITIONAL is still defined in libxl_sr_stream_format.h,
> we probably what to comment it out.
Wouldn't it be better to add a comment like "no longer supported from Xen 4.21
onwards"?

> > -            break;
> > -
> >           case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> >               stream->emu_sub_hdr.id = EMULATOR_QEMU_UPSTREAM;
> >               break;
> > diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> > index 9bb2969931..246bb3ddd2 100644
> > --- a/tools/libs/light/libxl_types.idl
> > +++ b/tools/libs/light/libxl_types.idl
> > @@ -104,7 +104,6 @@ libxl_channel_connection = 
> > Enumeration("channel_connection", [
> >   
> >   libxl_device_model_version = Enumeration("device_model_version", [
> >       (0, "UNKNOWN"),
> > -    (1, "QEMU_XEN_TRADITIONAL"), # Historical qemu-xen device model (qemu-dm)
> I think we need to keep that, so application using libxl and using this
> value don't need to be changed. At least, that's a promise made in
> libxl.h about the API.
In that I cas I'll add "(no longer supported)".

> >       (2, "QEMU_XEN"),             # Upstream based qemu-xen device model
> >       ])
> >   
> > diff --git a/tools/python/xen/migration/libxl.py b/tools/python/xen/migration/libxl.py
> > index 5dcb50fe02..dc5c7ac355 100644
> > --- a/tools/python/xen/migration/libxl.py
> > +++ b/tools/python/xen/migration/libxl.py
> > @@ -51,12 +51,10 @@ rec_type_to_str = {
> >   EMULATOR_HEADER_FORMAT = "II"
> >   
> >   EMULATOR_ID_unknown       = 0x00000000
> > -EMULATOR_ID_qemu_trad     = 0x00000001
> Should we keep this value as a comment? Or maybe the information in
> "docs/specs/libxl-migration-stream.pandoc" is enough.
I don't think keeping it will have any real advantage.

> >   EMULATOR_ID_qemu_upstream = 0x00000002
> >   
> >   emulator_id_to_str = {
> >       EMULATOR_ID_unknown       : "Unknown",
> > -    EMULATOR_ID_qemu_trad     : "Qemu Traditional",
> >       EMULATOR_ID_qemu_upstream : "Qemu Upstream",
> >   }

Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature