Re: [PATCH V3 06/42] vfio/container: register container for cpr

[Cédric Le Goater]

On 5/15/25 21:06, Steven Sistare wrote:
> On 5/15/2025 3:54 AM, Cédric Le Goater wrote:
> > On 5/12/25 17:32, Steve Sistare wrote:
> > > Register a legacy container for cpr-transfer, replacing the generic CPR
> > > register call with a more specific legacy container register call.  Add a
> > > blocker if the kernel does not support VFIO_UPDATE_VADDR or VFIO_UNMAP_ALL.
> > >
> > > This is mostly boiler plate.  The fields to to saved and restored are added
> > > in subsequent patches.
> > >
> > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> > > ---
> > >   hw/vfio/container.c              |  6 ++--
> > >   hw/vfio/cpr-legacy.c             | 70 ++++++++++++++++++++++++++++++++++++++++
> > >   hw/vfio/cpr.c                    |  5 ++-
> > >   hw/vfio/meson.build              |  1 +
> > >   include/hw/vfio/vfio-container.h |  2 ++
> > >   include/hw/vfio/vfio-cpr.h       | 14 ++++++++
> > >   6 files changed, 92 insertions(+), 6 deletions(-)
> > >   create mode 100644 hw/vfio/cpr-legacy.c
> > >
> > > diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> > > index eb56f00..85c76da 100644
> > > --- a/hw/vfio/container.c
> > > +++ b/hw/vfio/container.c
> > > @@ -642,7 +642,7 @@ static bool vfio_container_connect(VFIOGroup *group, 
> > > AddressSpace *as,
> > >       new_container = true;
> > >       bcontainer = &container->bcontainer;
> > > -    if (!vfio_cpr_register_container(bcontainer, errp)) {
> > > +    if (!vfio_legacy_cpr_register_container(container, errp)) {
> > >           goto fail;
> > >       }
> > > @@ -678,7 +678,7 @@ fail:
> > >           vioc->release(bcontainer);
> > >       }
> > >       if (new_container) {
> > > -        vfio_cpr_unregister_container(bcontainer);
> > > +        vfio_legacy_cpr_unregister_container(container);
> > >           object_unref(container);
> > >       }
> > >       if (fd >= 0) {
> > > @@ -719,7 +719,7 @@ static void vfio_container_disconnect(VFIOGroup *group)
> > >           VFIOAddressSpace *space = bcontainer->space;
> > >           trace_vfio_container_disconnect(container->fd);
> > > -        vfio_cpr_unregister_container(bcontainer);
> > > +        vfio_legacy_cpr_unregister_container(container);
> > >           close(container->fd);
> > >           object_unref(container);
> > > diff --git a/hw/vfio/cpr-legacy.c b/hw/vfio/cpr-legacy.c
> > > new file mode 100644
> > > index 0000000..fac323c
> > > --- /dev/null
> > > +++ b/hw/vfio/cpr-legacy.c
> > > @@ -0,0 +1,70 @@
> > > +/*
> > > + * Copyright (c) 2021-2025 Oracle and/or its affiliates.
> > > + *
> > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > + * See the COPYING file in the top-level directory.
> >
> > Please add a SPDX-License-Identifier tag.
>
> Sure.  I'll do the same for my other new files.

and remove the License boiler plate too please.

A newer version of checkpatch will complain with :

  ERROR: New file 'hw/vfio/cpr-legacy.c' requires 'SPDX-License-Identifier'
  ERROR: New file 'hw/vfio/cpr-legacy.c' must not have license boilerplate 
header text unless this file is copied from existing code with such  text 
already present.
  WARNING: added, moved or deleted file(s):

    hw/vfio/cpr-legacy.c

  Does MAINTAINERS need updating?

  total: 2 errors, 1 warnings, 152 lines checked


Thanks,

C.


> > > + */
> > > +
> > > +#include <sys/ioctl.h>
> > > +#include <linux/vfio.h>
> > > +#include "qemu/osdep.h"
> > > +#include "hw/vfio/vfio-container.h"
> > > +#include "hw/vfio/vfio-cpr.h"
> > > +#include "migration/blocker.h"
> > > +#include "migration/cpr.h"
> > > +#include "migration/migration.h"
> > > +#include "migration/vmstate.h"
> > > +#include "qapi/error.h"
> > > +
> > > +static bool vfio_cpr_supported(VFIOContainer *container, Error **errp)
> > > +{
> > > +    if (!ioctl(container->fd, VFIO_CHECK_EXTENSION, VFIO_UPDATE_VADDR)) {
> > > +        error_setg(errp, "VFIO container does not support VFIO_UPDATE_VADDR");
> > > +        return false;
> > > +
> > > +    } else if (!ioctl(container->fd, VFIO_CHECK_EXTENSION, VFIO_UNMAP_ALL)) {
> > > +        error_setg(errp, "VFIO container does not support VFIO_UNMAP_ALL");
> > > +        return false;
> > > +
> > > +    } else {
> > > +        return true;
> > > +    }
> > > +}
> > > +
> > > +static const VMStateDescription vfio_container_vmstate = {
> > > +    .name = "vfio-container",
> > > +    .version_id = 0,
> > > +    .minimum_version_id = 0,
> > > +    .needed = cpr_needed_for_reuse,
> > > +    .fields = (VMStateField[]) {
> > > +        VMSTATE_END_OF_LIST()
> > > +    }
> > > +};
> > > +
> > > +bool vfio_legacy_cpr_register_container(VFIOContainer *container, Error **errp)
> > > +{
> > > +    VFIOContainerBase *bcontainer = &container->bcontainer;
> > > +    Error **cpr_blocker = &container->cpr.blocker;
> > > +
> > > +    migration_add_notifier_mode(&bcontainer->cpr_reboot_notifier,
> > > +                                vfio_cpr_reboot_notifier,
> > > +                                MIG_MODE_CPR_REBOOT);
> > > +
> > > +    if (!vfio_cpr_supported(container, cpr_blocker)) {
> > > +        return migrate_add_blocker_modes(cpr_blocker, errp,
> > > +                                         MIG_MODE_CPR_TRANSFER, -1) == 0;
> > > +    }
> > > +
> > > +    vmstate_register(NULL, -1, &vfio_container_vmstate, container);
> > > +
> > > +    return true;
> > > +}
> > > +
> > > +void vfio_legacy_cpr_unregister_container(VFIOContainer *container)
> > > +{
> > > +    VFIOContainerBase *bcontainer = &container->bcontainer;
> > > +
> > > +    migration_remove_notifier(&bcontainer->cpr_reboot_notifier);
> > > +    migrate_del_blocker(&container->cpr.blocker);
> > > +    vmstate_unregister(NULL, &vfio_container_vmstate, container);
> > > +}
> > > diff --git a/hw/vfio/cpr.c b/hw/vfio/cpr.c
> > > index 0210e76..0e59612 100644
> > > --- a/hw/vfio/cpr.c
> > > +++ b/hw/vfio/cpr.c
> > > @@ -7,13 +7,12 @@
> > >   #include "qemu/osdep.h"
> > >   #include "hw/vfio/vfio-device.h"
> > > -#include "migration/misc.h"
> > >   #include "hw/vfio/vfio-cpr.h"
> > >   #include "qapi/error.h"
> > >   #include "system/runstate.h"
> > > -static int vfio_cpr_reboot_notifier(NotifierWithReturn *notifier,
> > > -                                    MigrationEvent *e, Error **errp)
> > > +int vfio_cpr_reboot_notifier(NotifierWithReturn *notifier,
> > > +                             MigrationEvent *e, Error **errp)
> > >   {
> > >       if (e->type == MIG_EVENT_PRECOPY_SETUP &&
> > >           !runstate_check(RUN_STATE_SUSPENDED) && !vm_get_suspended()) {
> > > diff --git a/hw/vfio/meson.build b/hw/vfio/meson.build
> > > index bccb050..73d29f9 100644
> > > --- a/hw/vfio/meson.build
> > > +++ b/hw/vfio/meson.build
> > > @@ -21,6 +21,7 @@ system_ss.add(when: 'CONFIG_VFIO_XGMAC', if_true: 
> > > files('calxeda-xgmac.c'))
> > >   system_ss.add(when: 'CONFIG_VFIO_AMD_XGBE', if_true: files('amd-xgbe.c'))
> > >   system_ss.add(when: 'CONFIG_VFIO', if_true: files(
> > >     'cpr.c',
> > > +  'cpr-legacy.c',
> > >     'device.c',
> > >     'migration.c',
> > >     'migration-multifd.c',
> > > diff --git a/include/hw/vfio/vfio-container.h b/include/hw/vfio/vfio-container.h
> > > index afc498d..21e5807 100644
> > > --- a/include/hw/vfio/vfio-container.h
> > > +++ b/include/hw/vfio/vfio-container.h
> > > @@ -10,6 +10,7 @@
> > >   #define HW_VFIO_CONTAINER_H
> > >   #include "hw/vfio/vfio-container-base.h"
> > > +#include "hw/vfio/vfio-cpr.h"
> > >   typedef struct VFIOContainer VFIOContainer;
> > >   typedef struct VFIODevice VFIODevice;
> > > @@ -29,6 +30,7 @@ typedef struct VFIOContainer {
> > >       int fd; /* /dev/vfio/vfio, empowered by the attached groups */
> > >       unsigned iommu_type;
> > >       QLIST_HEAD(, VFIOGroup) group_list;
> > > +    VFIOContainerCPR cpr;
> > >   } VFIOContainer;
> > >   OBJECT_DECLARE_SIMPLE_TYPE(VFIOContainer, VFIO_IOMMU_LEGACY);
> > > diff --git a/include/hw/vfio/vfio-cpr.h b/include/hw/vfio/vfio-cpr.h
> > > index 750ea5b..f864547 100644
> > > --- a/include/hw/vfio/vfio-cpr.h
> > > +++ b/include/hw/vfio/vfio-cpr.h
> > > @@ -9,8 +9,22 @@
> > >   #ifndef HW_VFIO_VFIO_CPR_H
> > >   #define HW_VFIO_VFIO_CPR_H
> > > +#include "migration/misc.h"
> > > +
> > > +typedef struct VFIOContainerCPR {
> > > +    Error *blocker;
> > > +} VFIOContainerCPR;
> > > +
> > > +struct VFIOContainer;
> > >   struct VFIOContainerBase;
> > > +bool vfio_legacy_cpr_register_container(struct VFIOContainer *container,
> > > +                                        Error **errp);
> > > +void vfio_legacy_cpr_unregister_container(struct VFIOContainer *container);
> > > +
> > > +int vfio_cpr_reboot_notifier(NotifierWithReturn *notifier, MigrationEvent *e,
> > > +                             Error **errp);
> > > +
> > >   bool vfio_cpr_register_container(struct VFIOContainerBase *bcontainer,
> > >                                    Error **errp);
> > >   void vfio_cpr_unregister_container(struct VFIOContainerBase *bcontainer);
> >
> > what about vfio_cpr_un/register_container ? Shouldn't we remove them ?
>
> At this patch in the series, those are still used by iommufd containers.
> Those uses are removed in "vfio/iommufd: register container for cpr", and
> vfio_cpr_un/register_container are deleted by the last patch in the series.
>
> - Steve