Hi Matthew,
On 3/3/23 18:30, Matthew Rosato wrote:
> On 1/31/23 3:52 PM, Eric Auger wrote:
>> Let the vfio-ccw device use vfio_attach_device() and
>> vfio_detach_device(), hence hiding the details of the used
>> IOMMU backend.
>>
>> 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@xxxxxxxxxx>
>> Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx>
> Hi Eric,
>
> While testing the cdev series on s390 I ran into a couple of issues with this patch, see below.
>
>> ---
>> include/hw/vfio/vfio-common.h | 4 --
>> hw/vfio/ccw.c | 118 ++++++++--------------------------
>> hw/vfio/container.c | 8 +--
>> 3 files changed, 32 insertions(+), 98 deletions(-)
>>
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>> index 9465c4b021..1580f9617c 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -176,10 +176,6 @@ 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);
>> -int vfio_get_device(VFIOGroup *group, const char *name,
>> - VFIODevice *vbasedev, Error **errp);
>> int vfio_attach_device(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 0354737666..6fde7849cc 100644
>> --- a/hw/vfio/ccw.c
>> +++ b/hw/vfio/ccw.c
>> @@ -579,27 +579,32 @@ 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)
>> +static void vfio_ccw_realize(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);
>> + VFIODevice *vbasedev = &vcdev->vdev;
>> + Error *err = NULL;
>> char *name = g_strdup_printf("%x.%x.%04x", vcdev->cdev.hostid.cssid,
>> vcdev->cdev.hostid.ssid,
>> vcdev->cdev.hostid.devid);
>
> We can't get these cssid, ssid, devid values quite yet, they are currently 0s. That has to happen after cdc->realize()
>
>
>> - VFIODevice *vbasedev;
>> + int ret;
>>
>> - 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;
>> + /* Call the class init function for subchannel. */
>> + if (cdc->realize) {
>> + cdc->realize(cdev, vcdev->vdev.sysfsdev, &err);
>> + if (err) {
>> + goto out_err_propagate;
>> }
>> }
>> + vbasedev->sysfsdev = g_strdup_printf("/sys/bus/css/devices/%s/%s",
>> + name, cdev->mdevid);
>> + vbasedev->ops = &vfio_ccw_ops;
>> + vbasedev->type = VFIO_DEVICE_TYPE_CCW;
>> + vbasedev->name = name;
> vbasedev->name is being set to the wrong value here, it needs to be the uuid.
>
> See below for a suggested diff on top of this patch that solves the issue for me.
>
> Thanks,
> Matt
>
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 6fde7849cc..394b73358f 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -587,9 +587,6 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
> S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev);
> VFIODevice *vbasedev = &vcdev->vdev;
> Error *err = NULL;
> - char *name = g_strdup_printf("%x.%x.%04x", vcdev->cdev.hostid.cssid,
> - vcdev->cdev.hostid.ssid,
> - vcdev->cdev.hostid.devid);
> int ret;
>
> /* Call the class init function for subchannel. */
> @@ -599,11 +596,14 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
> goto out_err_propagate;
> }
> }
> - vbasedev->sysfsdev = g_strdup_printf("/sys/bus/css/devices/%s/%s",
> - name, cdev->mdevid);
> + vbasedev->sysfsdev = g_strdup_printf("/sys/bus/css/devices/%x.%x.%04x/%s",
> + vcdev->cdev.hostid.cssid,
> + vcdev->cdev.hostid.ssid,
> + vcdev->cdev.hostid.devid,
> + cdev->mdevid);
> vbasedev->ops = &vfio_ccw_ops;
> vbasedev->type = VFIO_DEVICE_TYPE_CCW;
> - vbasedev->name = name;
> + vbasedev->name = g_strdup(cdev->mdevid);
> vbasedev->dev = &vcdev->cdev.parent_obj.parent_obj;
>
> /*
>
>
Thank you very much for your report.
This will be handled in the next version.
Eric
