[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v1 06/22] vfio/common: Add a vfio device iterator
From: |
Eric Auger |
Subject: |
Re: [PATCH v1 06/22] vfio/common: Add a vfio device iterator |
Date: |
Wed, 20 Sep 2023 14:25:47 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 |
Hi Zhenzhong,
On 8/30/23 12:37, Zhenzhong Duan wrote:
> With a vfio device iterator added, we can make some migration and reset
> related functions group agnostic.
> E.x:
> vfio_mig_active
> vfio_migratable_device_num
> vfio_devices_all_dirty_tracking
> vfio_devices_all_device_dirty_tracking
> vfio_devices_all_running_and_mig_active
> vfio_devices_dma_logging_stop
> vfio_devices_dma_logging_start
> vfio_devices_query_dirty_bitmap
> vfio_reset_handler
>
> Or else we need to add container specific callback variants for above
> functions just because they iterate devices based on group.
>
> Move the reset handler registration/unregistration to a place that is not
> group specific, saying first vfio address space created instead of the
> first group.
I would move the reset handler registration/unregistration changes in a
separate patch.
besides, I don't catch what you mean by
"saying first vfio address space created instead of the first group."
>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
> hw/vfio/common.c | 224 ++++++++++++++++++++++++++---------------------
> 1 file changed, 122 insertions(+), 102 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 949ad6714a..51c6e7598e 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -84,6 +84,26 @@ static int vfio_ram_block_discard_disable(VFIOContainer
> *container, bool state)
> }
> }
>
I would add a comment:
iterate on all devices from all groups attached to a container
> +static VFIODevice *vfio_container_dev_iter_next(VFIOContainer *container,
> + VFIODevice *curr)
> +{
> + VFIOGroup *group;
> +
> + if (!curr) {
> + group = QLIST_FIRST(&container->group_list);
> + } else {
> + if (curr->next.le_next) {
> + return curr->next.le_next;
> + }
> + group = curr->group->container_next.le_next;
> + }
> +
> + if (!group) {
> + return NULL;
> + }
> + return QLIST_FIRST(&group->device_list);
> +}
> +
> /*
> * Device state interfaces
> */
> @@ -112,17 +132,22 @@ static int vfio_get_dirty_bitmap(VFIOContainer
> *container, uint64_t iova,
>
> bool vfio_mig_active(void)
> {
> - VFIOGroup *group;
> + VFIOAddressSpace *space;
> + VFIOContainer *container;
> VFIODevice *vbasedev;
>
> - if (QLIST_EMPTY(&vfio_group_list)) {
> + if (QLIST_EMPTY(&vfio_address_spaces)) {
> return false;
> }
>
> - QLIST_FOREACH(group, &vfio_group_list, next) {
> - QLIST_FOREACH(vbasedev, &group->device_list, next) {
> - if (vbasedev->migration_blocker) {
> - return false;
> + QLIST_FOREACH(space, &vfio_address_spaces, list) {
> + QLIST_FOREACH(container, &space->containers, next) {
> + vbasedev = NULL;
> + while ((vbasedev = vfio_container_dev_iter_next(container,
> + vbasedev))) {
Couldn't you use an extra define such as:
#define CONTAINER_FOREACH_DEV(container, vbasedev) \
vbasedev = NULL
while ((vbasedev = vfio_container_dev_iter_next(container, vbasedev)))
> + if (vbasedev->migration_blocker) {
> + return false;
> + }
> }
> }
> }
> @@ -133,14 +158,19 @@ static Error *multiple_devices_migration_blocker;
>
> static unsigned int vfio_migratable_device_num(void)
> {
> - VFIOGroup *group;
> + VFIOAddressSpace *space;
> + VFIOContainer *container;
> VFIODevice *vbasedev;
> unsigned int device_num = 0;
>
> - QLIST_FOREACH(group, &vfio_group_list, next) {
> - QLIST_FOREACH(vbasedev, &group->device_list, next) {
> - if (vbasedev->migration) {
> - device_num++;
> + QLIST_FOREACH(space, &vfio_address_spaces, list) {
> + QLIST_FOREACH(container, &space->containers, next) {
> + vbasedev = NULL;
> + while ((vbasedev = vfio_container_dev_iter_next(container,
> + vbasedev))) {
> + if (vbasedev->migration) {
> + device_num++;
> + }
> }
> }
> }
> @@ -207,8 +237,7 @@ static void vfio_set_migration_error(int err)
>
> static bool vfio_devices_all_dirty_tracking(VFIOContainer *container)
> {
> - VFIOGroup *group;
> - VFIODevice *vbasedev;
> + VFIODevice *vbasedev = NULL;
> MigrationState *ms = migrate_get_current();
>
> if (ms->state != MIGRATION_STATUS_ACTIVE &&
> @@ -216,19 +245,17 @@ static bool
> vfio_devices_all_dirty_tracking(VFIOContainer *container)
> return false;
> }
>
> - QLIST_FOREACH(group, &container->group_list, container_next) {
> - QLIST_FOREACH(vbasedev, &group->device_list, next) {
> - VFIOMigration *migration = vbasedev->migration;
> + while ((vbasedev = vfio_container_dev_iter_next(container, vbasedev))) {
> + VFIOMigration *migration = vbasedev->migration;
>
> - if (!migration) {
> - return false;
> - }
> + if (!migration) {
> + return false;
> + }
>
> - if (vbasedev->pre_copy_dirty_page_tracking == ON_OFF_AUTO_OFF &&
> - (migration->device_state == VFIO_DEVICE_STATE_RUNNING ||
> - migration->device_state == VFIO_DEVICE_STATE_PRE_COPY)) {
> - return false;
> - }
> + if (vbasedev->pre_copy_dirty_page_tracking == ON_OFF_AUTO_OFF &&
> + (migration->device_state == VFIO_DEVICE_STATE_RUNNING ||
> + migration->device_state == VFIO_DEVICE_STATE_PRE_COPY)) {
> + return false;
> }
> }
> return true;
> @@ -236,14 +263,11 @@ static bool
> vfio_devices_all_dirty_tracking(VFIOContainer *container)
>
> static bool vfio_devices_all_device_dirty_tracking(VFIOContainer *container)
> {
> - VFIOGroup *group;
> - VFIODevice *vbasedev;
> + VFIODevice *vbasedev = NULL;
>
> - QLIST_FOREACH(group, &container->group_list, container_next) {
> - QLIST_FOREACH(vbasedev, &group->device_list, next) {
> - if (!vbasedev->dirty_pages_supported) {
> - return false;
> - }
> + while ((vbasedev = vfio_container_dev_iter_next(container, vbasedev))) {
> + if (!vbasedev->dirty_pages_supported) {
> + return false;
> }
> }
>
> @@ -256,27 +280,24 @@ static bool
> vfio_devices_all_device_dirty_tracking(VFIOContainer *container)
> */
> static bool vfio_devices_all_running_and_mig_active(VFIOContainer *container)
> {
> - VFIOGroup *group;
> - VFIODevice *vbasedev;
> + VFIODevice *vbasedev = NULL;
>
> if (!migration_is_active(migrate_get_current())) {
> return false;
> }
>
> - QLIST_FOREACH(group, &container->group_list, container_next) {
> - QLIST_FOREACH(vbasedev, &group->device_list, next) {
> - VFIOMigration *migration = vbasedev->migration;
> + while ((vbasedev = vfio_container_dev_iter_next(container, vbasedev))) {
> + VFIOMigration *migration = vbasedev->migration;
>
> - if (!migration) {
> - return false;
> - }
> + if (!migration) {
> + return false;
> + }
>
> - if (migration->device_state == VFIO_DEVICE_STATE_RUNNING ||
> - migration->device_state == VFIO_DEVICE_STATE_PRE_COPY) {
> - continue;
> - } else {
> - return false;
> - }
> + if (migration->device_state == VFIO_DEVICE_STATE_RUNNING ||
> + migration->device_state == VFIO_DEVICE_STATE_PRE_COPY) {
> + continue;
> + } else {
> + return false;
> }
> }
> return true;
> @@ -1243,25 +1264,22 @@ static void
> vfio_devices_dma_logging_stop(VFIOContainer *container)
> uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature),
> sizeof(uint64_t))] = {};
> struct vfio_device_feature *feature = (struct vfio_device_feature *)buf;
> - VFIODevice *vbasedev;
> - VFIOGroup *group;
> + VFIODevice *vbasedev = NULL;
>
> feature->argsz = sizeof(buf);
> feature->flags = VFIO_DEVICE_FEATURE_SET |
> VFIO_DEVICE_FEATURE_DMA_LOGGING_STOP;
>
> - QLIST_FOREACH(group, &container->group_list, container_next) {
> - QLIST_FOREACH(vbasedev, &group->device_list, next) {
> - if (!vbasedev->dirty_tracking) {
> - continue;
> - }
> + while ((vbasedev = vfio_container_dev_iter_next(container, vbasedev))) {
> + if (!vbasedev->dirty_tracking) {
> + continue;
> + }
>
> - if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) {
> - warn_report("%s: Failed to stop DMA logging, err %d (%s)",
> - vbasedev->name, -errno, strerror(errno));
> - }
> - vbasedev->dirty_tracking = false;
> + if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) {
> + warn_report("%s: Failed to stop DMA logging, err %d (%s)",
> + vbasedev->name, -errno, strerror(errno));
> }
> + vbasedev->dirty_tracking = false;
> }
> }
>
> @@ -1336,8 +1354,7 @@ static int vfio_devices_dma_logging_start(VFIOContainer
> *container)
> {
> struct vfio_device_feature *feature;
> VFIODirtyRanges ranges;
> - VFIODevice *vbasedev;
> - VFIOGroup *group;
> + VFIODevice *vbasedev = NULL;
> int ret = 0;
>
> vfio_dirty_tracking_init(container, &ranges);
> @@ -1347,21 +1364,19 @@ static int
> vfio_devices_dma_logging_start(VFIOContainer *container)
> return -errno;
> }
>
> - QLIST_FOREACH(group, &container->group_list, container_next) {
> - QLIST_FOREACH(vbasedev, &group->device_list, next) {
> - if (vbasedev->dirty_tracking) {
> - continue;
> - }
> + while ((vbasedev = vfio_container_dev_iter_next(container, vbasedev))) {
> + if (vbasedev->dirty_tracking) {
> + continue;
> + }
>
> - ret = ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature);
> - if (ret) {
> - ret = -errno;
> - error_report("%s: Failed to start DMA logging, err %d (%s)",
> - vbasedev->name, ret, strerror(errno));
> - goto out;
> - }
> - vbasedev->dirty_tracking = true;
> + ret = ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature);
> + if (ret) {
> + ret = -errno;
> + error_report("%s: Failed to start DMA logging, err %d (%s)",
> + vbasedev->name, ret, strerror(errno));
> + goto out;
> }
> + vbasedev->dirty_tracking = true;
> }
>
> out:
> @@ -1440,22 +1455,19 @@ static int
> vfio_devices_query_dirty_bitmap(VFIOContainer *container,
> VFIOBitmap *vbmap, hwaddr iova,
> hwaddr size)
> {
> - VFIODevice *vbasedev;
> - VFIOGroup *group;
> + VFIODevice *vbasedev = NULL;
> int ret;
>
> - QLIST_FOREACH(group, &container->group_list, container_next) {
> - QLIST_FOREACH(vbasedev, &group->device_list, next) {
> - ret = vfio_device_dma_logging_report(vbasedev, iova, size,
> - vbmap->bitmap);
> - if (ret) {
> - error_report("%s: Failed to get DMA logging report, iova: "
> - "0x%" HWADDR_PRIx ", size: 0x%" HWADDR_PRIx
> - ", err: %d (%s)",
> - vbasedev->name, iova, size, ret,
> strerror(-ret));
> + while ((vbasedev = vfio_container_dev_iter_next(container, vbasedev))) {
> + ret = vfio_device_dma_logging_report(vbasedev, iova, size,
> + vbmap->bitmap);
> + if (ret) {
> + error_report("%s: Failed to get DMA logging report, iova: "
> + "0x%" HWADDR_PRIx ", size: 0x%" HWADDR_PRIx
> + ", err: %d (%s)",
> + vbasedev->name, iova, size, ret, strerror(-ret));
>
> - return ret;
> - }
> + return ret;
> }
> }
>
> @@ -1739,21 +1751,30 @@ bool vfio_get_info_dma_avail(struct
> vfio_iommu_type1_info *info,
>
> void vfio_reset_handler(void *opaque)
> {
> - VFIOGroup *group;
> + VFIOAddressSpace *space;
> + VFIOContainer *container;
> VFIODevice *vbasedev;
>
> - QLIST_FOREACH(group, &vfio_group_list, next) {
> - QLIST_FOREACH(vbasedev, &group->device_list, next) {
> - if (vbasedev->dev->realized) {
> - vbasedev->ops->vfio_compute_needs_reset(vbasedev);
> + QLIST_FOREACH(space, &vfio_address_spaces, list) {
> + QLIST_FOREACH(container, &space->containers, next) {
> + vbasedev = NULL;
> + while ((vbasedev = vfio_container_dev_iter_next(container,
> + vbasedev))) {
> + if (vbasedev->dev->realized) {
> + vbasedev->ops->vfio_compute_needs_reset(vbasedev);
> + }
> }
> }
> }
>
> - QLIST_FOREACH(group, &vfio_group_list, next) {
> - QLIST_FOREACH(vbasedev, &group->device_list, next) {
> - if (vbasedev->dev->realized && vbasedev->needs_reset) {
> - vbasedev->ops->vfio_hot_reset_multi(vbasedev);
> + QLIST_FOREACH(space, &vfio_address_spaces, list) {
> + QLIST_FOREACH(container, &space->containers, next) {
> + vbasedev = NULL;
> + while ((vbasedev = vfio_container_dev_iter_next(container,
> + vbasedev))) {
> + if (vbasedev->dev->realized && vbasedev->needs_reset) {
> + vbasedev->ops->vfio_hot_reset_multi(vbasedev);
> + }
> }
> }
> }
> @@ -1841,6 +1862,10 @@ static VFIOAddressSpace
> *vfio_get_address_space(AddressSpace *as)
> space->as = as;
> QLIST_INIT(&space->containers);
>
> + if (QLIST_EMPTY(&vfio_address_spaces)) {
> + qemu_register_reset(vfio_reset_handler, NULL);
> + }
> +
> QLIST_INSERT_HEAD(&vfio_address_spaces, space, list);
>
> return space;
> @@ -1852,6 +1877,9 @@ static void vfio_put_address_space(VFIOAddressSpace
> *space)
> QLIST_REMOVE(space, list);
> g_free(space);
> }
> + if (QLIST_EMPTY(&vfio_address_spaces)) {
> + qemu_unregister_reset(vfio_reset_handler, NULL);
> + }
> }
>
> /*
> @@ -2317,10 +2345,6 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace
> *as, Error **errp)
> goto close_fd_exit;
> }
>
> - if (QLIST_EMPTY(&vfio_group_list)) {
> - qemu_register_reset(vfio_reset_handler, NULL);
> - }
> -
> QLIST_INSERT_HEAD(&vfio_group_list, group, next);
>
> return group;
> @@ -2349,10 +2373,6 @@ void vfio_put_group(VFIOGroup *group)
> trace_vfio_put_group(group->fd);
> close(group->fd);
> g_free(group);
> -
> - if (QLIST_EMPTY(&vfio_group_list)) {
> - qemu_unregister_reset(vfio_reset_handler, NULL);
> - }
> }
>
> struct vfio_device_info *vfio_get_device_info(int fd)
Thanks
Eric
- Re: [PATCH v1 06/22] vfio/common: Add a vfio device iterator,
Eric Auger <=