* Alex Williamson <alex.william...@redhat.com> [2017-04-24 16:56:28 -0600]:
[...] > > > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c > > > new file mode 100644 > > > index 0000000..c491bee > > > --- /dev/null > > > +++ b/hw/vfio/ccw.c > > > @@ -0,0 +1,207 @@ > > > +/* > > > + * vfio based subchannel assignment support > > > + * > > > + * Copyright 2017 IBM Corp. > > > + * Author(s): Dong Jia Shi <bjsdj...@linux.vnet.ibm.com> > > > + * Xiao Feng Ren <renxi...@linux.vnet.ibm.com> > > > + * Pierre Morel <pmo...@linux.vnet.ibm.com> > > > + * > > > + * This work is licensed under the terms of the GNU GPL, version 2 or(at > > > + * your option) any version. See the COPYING file in the top-level > > > + * directory. > > > + */ > > > + > > > +#include <linux/vfio.h> > > > +#include <sys/ioctl.h> > > > + > > > +#include "qemu/osdep.h" > > > +#include "qapi/error.h" > > > +#include "hw/sysbus.h" > > > +#include "hw/vfio/vfio.h" > > > +#include "hw/vfio/vfio-common.h" > > > +#include "hw/s390x/s390-ccw.h" > > > +#include "hw/s390x/ccw-device.h" > > > + > > > +#define TYPE_VFIO_CCW "vfio-ccw" > > > +typedef struct VFIOCCWDevice { > > > + S390CCWDevice cdev; > > > + VFIODevice vdev; > > > +} VFIOCCWDevice; > > > + > > > +static void vfio_ccw_compute_needs_reset(VFIODevice *vdev) > > > +{ > > > + vdev->needs_reset = false; > > > +} > > > + > > > +/* > > > + * We don't need vfio_hot_reset_multi and vfio_eoi operationis for > > One more: > > s/operationis/operations/ > Ok. > > > + * vfio_ccw device now. > > > + */ > > > +struct VFIODeviceOps vfio_ccw_ops = { > > > + .vfio_compute_needs_reset = vfio_ccw_compute_needs_reset, > > > +}; > > > + > > > +static void vfio_ccw_reset(DeviceState *dev) > > > +{ > > > + CcwDevice *ccw_dev = DO_UPCAST(CcwDevice, parent_obj, dev); > > > + S390CCWDevice *cdev = DO_UPCAST(S390CCWDevice, parent_obj, ccw_dev); > > > + VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev); > > > + > > > + ioctl(vcdev->vdev.fd, VFIO_DEVICE_RESET); > > > +} > > > + > > > +static void vfio_put_device(VFIOCCWDevice *vcdev) > > > +{ > > > + g_free(vcdev->vdev.name); > > > + vfio_put_base_device(&vcdev->vdev); > > > +} > > > + > > > +static VFIOGroup *vfio_ccw_get_group(S390CCWDevice *cdev, char **path, > > > + Error **errp) > > > +{ > > > + struct stat st; > > > + int groupid; > > > + GError *gerror = NULL; > > > + > > > + /* Check that host subchannel exists. */ > > > + path[0] = g_strdup_printf("/sys/bus/css/devices/%x.%x.%04x", > > > + cdev->hostid.cssid, > > > + cdev->hostid.ssid, > > > + cdev->hostid.devid); > > > + if (stat(path[0], &st) < 0) { > > > + error_setg(errp, "vfio: no such host subchannel %s", path[0]); > > > + return NULL; > > > + } > > > + > > > + /* Check that mediated device exists. */ > > > + path[1] = g_strdup_printf("%s/%s", path[0], cdev->mdevid); > > > + if (stat(path[0], &st) < 0) { > > > + error_setg(errp, "vfio: no such mediated device %s", path[1]); > > > + return NULL; > > > + } > > > > Isn't this all a bit circular since we build the S390CCWDevice based on > > the sysfsdev mdev path? > > Right! We don't need to verify the existance of the path here again, since we already did that during the realization of the S390CCWDevice, which is triggered by calling cdc->realize before vfio_ccw_get_group in vfio_ccw_realize. > > > + > > > + /* Get the iommu_group patch as the interim variable. */ > > > + path[2] = g_strconcat(path[1], "/iommu_group", NULL); > > > + > > > + /* Get the link file path of the device iommu_group. */ > > > + path[3] = g_file_read_link(path[2], &gerror); > > > + if (!path[3]) { > > > + error_setg(errp, "vfio: error no iommu_group for subchannel"); > > > + return NULL; > > > + } > > > + > > > + /* Get the device groupid. */ > > > + if (sscanf(basename(path[3]), "%d", &groupid) != 1) { > > > + error_setg(errp, "vfio: error reading %s:%m", path[3]); > > > + return NULL; > > > + } > > > + > > > + return vfio_get_group(groupid, &address_space_memory, errp); > > > +} > > > + > > > +static void vfio_ccw_put_group(VFIOGroup *group, char **path) > > > +{ > > > + g_free(path); > > > + vfio_put_group(group); > > > +} > > > + > > > +static void vfio_ccw_realize(DeviceState *dev, Error **errp) > > > +{ > > > + VFIODevice *vbasedev; > > > + VFIOGroup *group; > > > + CcwDevice *ccw_dev = DO_UPCAST(CcwDevice, parent_obj, dev); > > > + S390CCWDevice *cdev = DO_UPCAST(S390CCWDevice, parent_obj, ccw_dev); > > > + VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev); > > > + S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev); > > > + char *path[4] = {NULL, NULL, NULL, NULL}; > > > > I don't understand what's happening with 'path' throughout this > > function. vfio_ccw_get_group() allocates strings into this array, we > > only seem to use one in an error path below, it's only freed on an error > > path and I don't see it getting linked to the VFIOCCWDevice, so it > > seems to be leaked, and even the g_free() above looks quite a bit > > suspicious. This @path, together with the operations on it, must be a leftover of a previous implementation. I will rewrite these stuff. Thanks for the catch! > > > > > + > > > + /* Call the class init function for subchannel. */ > > > + if (cdc->realize) { > > > + cdc->realize(cdev, vcdev->vdev.sysfsdev, errp); > > > + if (*errp) { > > > + return; > > > + } > > > + } > > > + > > > + group = vfio_ccw_get_group(cdev, path, errp); > > > + if (!group) { > > > + goto out_group_err; > > > + } > > > + > > > + vcdev->vdev.ops = &vfio_ccw_ops; > > > + vcdev->vdev.type = VFIO_DEVICE_TYPE_CCW; > > > + vcdev->vdev.name = g_strdup_printf("%x.%x.%04x", cdev->hostid.cssid, > > > + cdev->hostid.ssid, > > > cdev->hostid.devid); > > > + QLIST_FOREACH(vbasedev, &group->device_list, next) { > > > + if (strcmp(vbasedev->name, vcdev->vdev.name) == 0) { > > > + error_setg(errp, "vfio: subchannel %s has already been > > > attached", > > > + basename(path[0])); > > > + goto out_device_err; > > > + } > > > + } > > > + > > > + if (vfio_get_device(group, cdev->mdevid, &vcdev->vdev, errp)) { > > > + goto out_device_err; > > > + } > > > + > > > + return; > > > + > > > +out_device_err: > > > + vfio_ccw_put_group(group, path); > > > +out_group_err: > > > + if (cdc->unrealize) { > > > + cdc->unrealize(cdev, errp); > > > + } > > > +} > > > + > > > +static void vfio_ccw_unrealize(DeviceState *dev, Error **errp) > > > +{ > > > + CcwDevice *ccw_dev = DO_UPCAST(CcwDevice, parent_obj, dev); > > > + S390CCWDevice *cdev = DO_UPCAST(S390CCWDevice, parent_obj, ccw_dev); > > > + VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev); > > > + S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev); > > > + VFIOGroup *group = vcdev->vdev.group; > > > + > > > + if (cdc->unrealize) { > > > + cdc->unrealize(cdev, errp); > > > + } > > > + > > > + vfio_put_device(vcdev); > > > + vfio_put_group(group); > > > +} > > > > Doesn't seem to matter here, but I would have expected the > > cdc->unrealize to occur at the end to unwind the order of the realize > > function. Ok! Will do. > > > > > + [...] -- Dong Jia Shi