[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH v4 10/15] vfio/ccw: Use vfio_[attach/detach]_device
From: |
Duan, Zhenzhong |
Subject: |
RE: [PATCH v4 10/15] vfio/ccw: Use vfio_[attach/detach]_device |
Date: |
Sun, 8 Oct 2023 10:21:01 +0000 |
Hi Eric,
>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Sent: Wednesday, October 4, 2023 11:44 PM
>Subject: [PATCH v4 10/15] vfio/ccw: Use vfio_[attach/detach]_device
>
>Let the vfio-ccw device use vfio_attach_device() and
>vfio_detach_device(), hence hiding the details of the used
>IOMMU backend.
>
>Note that the migration reduces the following trace
>"vfio: subchannel %s has already been attached" (featuring
>cssid.ssid.devid) into "device is already attached"
>
>Also now all the devices have been migrated to use the new
>vfio_attach_device/vfio_detach_device API, let's turn the
>legacy functions into static functions, local to container.c.
>
>Signed-off-by: Eric Auger <eric.auger@redhat.com>
>Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
>
>---
>
>v3:
>- simplified vbasedev->dev setting
>
>v2 -> v3:
>- Hopefully fix confusion beteen vbasedev->name, mdevid and sysfsdev
> while keeping into account Matthew's comment
> https://lore.kernel.org/qemu-devel/6e04ab8f-dc84-e9c2-deea-
>2b6b31678b53@linux.ibm.com/
>---
> include/hw/vfio/vfio-common.h | 5 --
> hw/vfio/ccw.c | 122 +++++++++-------------------------
> hw/vfio/common.c | 10 +--
> 3 files changed, 37 insertions(+), 100 deletions(-)
>
>diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>index 12fbfbc37d..c486bdef2a 100644
>--- a/include/hw/vfio/vfio-common.h
>+++ b/include/hw/vfio/vfio-common.h
>@@ -202,7 +202,6 @@ typedef struct {
> hwaddr pages;
> } VFIOBitmap;
>
>-void vfio_put_base_device(VFIODevice *vbasedev);
> void vfio_disable_irqindex(VFIODevice *vbasedev, int index);
> void vfio_unmask_single_irqindex(VFIODevice *vbasedev, int index);
> void vfio_mask_single_irqindex(VFIODevice *vbasedev, int index);
>@@ -220,11 +219,7 @@ void vfio_region_unmap(VFIORegion *region);
> void vfio_region_exit(VFIORegion *region);
> void vfio_region_finalize(VFIORegion *region);
> void vfio_reset_handler(void *opaque);
>-VFIOGroup *vfio_get_group(int groupid, AddressSpace *as, Error **errp);
>-void vfio_put_group(VFIOGroup *group);
> struct vfio_device_info *vfio_get_device_info(int fd);
>-int vfio_get_device(VFIOGroup *group, const char *name,
>- VFIODevice *vbasedev, Error **errp);
> int vfio_attach_device(char *name, VFIODevice *vbasedev,
> AddressSpace *as, Error **errp);
> void vfio_detach_device(VFIODevice *vbasedev);
>diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
>index 1e2fce83b0..6ec35fedc9 100644
>--- a/hw/vfio/ccw.c
>+++ b/hw/vfio/ccw.c
>@@ -572,88 +572,15 @@ static void vfio_ccw_put_region(VFIOCCWDevice
>*vcdev)
> g_free(vcdev->io_region);
> }
>
>-static void vfio_ccw_put_device(VFIOCCWDevice *vcdev)
>-{
>- g_free(vcdev->vdev.name);
>- vfio_put_base_device(&vcdev->vdev);
>-}
>-
>-static void vfio_ccw_get_device(VFIOGroup *group, VFIOCCWDevice *vcdev,
>- Error **errp)
>-{
>- S390CCWDevice *cdev = S390_CCW_DEVICE(vcdev);
>- char *name = g_strdup_printf("%x.%x.%04x", cdev->hostid.cssid,
>- cdev->hostid.ssid,
>- cdev->hostid.devid);
>- VFIODevice *vbasedev;
>-
>- QLIST_FOREACH(vbasedev, &group->device_list, next) {
>- if (strcmp(vbasedev->name, name) == 0) {
>- error_setg(errp, "vfio: subchannel %s has already been attached",
>- name);
>- goto out_err;
>- }
>- }
>-
>- /*
>- * All vfio-ccw devices are believed to operate in a way compatible with
>- * discarding of memory in RAM blocks, ie. pages pinned in the host are
>- * in the current working set of the guest driver and therefore never
>- * overlap e.g., with pages available to the guest balloon driver. This
>- * needs to be set before vfio_get_device() for vfio common to handle
>- * ram_block_discard_disable().
>- */
>- vcdev->vdev.ram_block_discard_allowed = true;
>-
>- if (vfio_get_device(group, cdev->mdevid, &vcdev->vdev, errp)) {
>- goto out_err;
>- }
>-
>- vcdev->vdev.ops = &vfio_ccw_ops;
>- vcdev->vdev.type = VFIO_DEVICE_TYPE_CCW;
>- vcdev->vdev.name = name;
>- vcdev->vdev.dev = DEVICE(vcdev);
>-
>- return;
>-
>-out_err:
>- g_free(name);
>-}
>-
>-static VFIOGroup *vfio_ccw_get_group(S390CCWDevice *cdev, Error **errp)
>-{
>- char *tmp, group_path[PATH_MAX];
>- ssize_t len;
>- int groupid;
>-
>- tmp = g_strdup_printf("/sys/bus/css/devices/%x.%x.%04x/%s/iommu_group",
>- cdev->hostid.cssid, cdev->hostid.ssid,
>- cdev->hostid.devid, cdev->mdevid);
>- len = readlink(tmp, group_path, sizeof(group_path));
>- g_free(tmp);
>-
>- if (len <= 0 || len >= sizeof(group_path)) {
>- error_setg(errp, "vfio: no iommu_group found");
>- return NULL;
>- }
>-
>- group_path[len] = 0;
>-
>- if (sscanf(basename(group_path), "%d", &groupid) != 1) {
>- error_setg(errp, "vfio: failed to read %s", group_path);
>- return NULL;
>- }
>-
>- return vfio_get_group(groupid, &address_space_memory, errp);
>-}
>-
> static void vfio_ccw_realize(DeviceState *dev, Error **errp)
> {
>- VFIOGroup *group;
> S390CCWDevice *cdev = S390_CCW_DEVICE(dev);
> VFIOCCWDevice *vcdev = VFIO_CCW(cdev);
> S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev);
>+ VFIODevice *vbasedev = &vcdev->vdev;
> Error *err = NULL;
>+ char *name;
>+ int ret;
>
> /* Call the class init function for subchannel. */
> if (cdc->realize) {
>@@ -663,14 +590,31 @@ static void vfio_ccw_realize(DeviceState *dev, Error
>**errp)
> }
> }
>
>- group = vfio_ccw_get_group(cdev, &err);
>- if (!group) {
>- goto out_group_err;
>- }
>+ name = g_strdup_printf("%x.%x.%04x", vcdev->cdev.hostid.cssid,
>+ vcdev->cdev.hostid.ssid,
>+ vcdev->cdev.hostid.devid);
>+ vbasedev->sysfsdev = g_strdup_printf("/sys/bus/css/devices/%s/%s",
>+ name,
>+ cdev->mdevid);
Hoping not late for you to include this in v5.
I think no need to re-assign sysfsdev as it's a user property, we'd better to
keep the original user value. Also looks a memory leak here.
>+ vbasedev->ops = &vfio_ccw_ops;
>+ vbasedev->type = VFIO_DEVICE_TYPE_CCW;
>+ vbasedev->name = name;
There will be a potential failure when a second mdev device under
same cssid.ssid.devid attached. We can use cdev->mdevid as name.
Maybe you can use v2 of this patch, I remember these two issues are already
addressed in v2.
Thanks
Zhenzhong
- [PATCH v4 08/15] vfio/platform: Use vfio_[attach/detach]_device, (continued)
- [PATCH v4 08/15] vfio/platform: Use vfio_[attach/detach]_device, Eric Auger, 2023/10/04
- [PATCH v4 09/15] vfio/ap: Use vfio_[attach/detach]_device, Eric Auger, 2023/10/04
- [PATCH v4 11/15] vfio/common: Move VFIO reset handler registration to a group agnostic function, Eric Auger, 2023/10/04
- [PATCH v4 13/15] vfio/common: Store the parent container in VFIODevice, Eric Auger, 2023/10/04
- [PATCH v4 12/15] vfio/common: Introduce a per container device list, Eric Auger, 2023/10/04
- [PATCH v4 06/15] vfio/common: Extract out vfio_kvm_device_[add/del]_fd, Eric Auger, 2023/10/04
- [PATCH v4 14/15] vfio/common: Introduce a global VFIODevice list, Eric Auger, 2023/10/04
- [PATCH v4 10/15] vfio/ccw: Use vfio_[attach/detach]_device, Eric Auger, 2023/10/04
- RE: [PATCH v4 10/15] vfio/ccw: Use vfio_[attach/detach]_device,
Duan, Zhenzhong <=
- Re: [PATCH v4 10/15] vfio/ccw: Use vfio_[attach/detach]_device, Eric Auger, 2023/10/08
- RE: [PATCH v4 10/15] vfio/ccw: Use vfio_[attach/detach]_device, Duan, Zhenzhong, 2023/10/08
- Re: [PATCH v4 10/15] vfio/ccw: Use vfio_[attach/detach]_device, Eric Auger, 2023/10/09
- RE: [PATCH v4 10/15] vfio/ccw: Use vfio_[attach/detach]_device, Duan, Zhenzhong, 2023/10/09
[PATCH v4 15/15] vfio/common: Move legacy VFIO backend code into separate container.c, Eric Auger, 2023/10/04
Re: [PATCH v4 00/15] Prerequisite changes for IOMMUFD support, Cédric Le Goater, 2023/10/04