Re: [RFC v3 11/18] vfio/ccw: Use vfio_[attach/detach]_device

From: Eric Auger <eric.auger@redhat.com>
To: Matthew Rosato <mjrosato@linux.ibm.com>,
	eric.auger.pro@gmail.com, yi.l.liu@intel.com, yi.y.sun@intel.com,
	alex.williamson@redhat.com, clg@redhat.com,
	qemu-devel@nongnu.org
Cc: david@gibson.dropbear.id.au, thuth@redhat.com,
	farman@linux.ibm.com, akrowiak@linux.ibm.com,
	pasic@linux.ibm.com, jjherne@linux.ibm.com, jasowang@redhat.com,
	kvm@vger.kernel.org, jgg@nvidia.com, nicolinc@nvidia.com,
	kevin.tian@intel.com, chao.p.peng@intel.com, peterx@redhat.com,
	shameerali.kolothum.thodi@huawei.com, zhangfei.gao@linaro.org,
	berrange@redhat.com, apopple@nvidia.com,
	suravee.suthikulpanit@amd.com
Subject: Re: [RFC v3 11/18] vfio/ccw: Use vfio_[attach/detach]_device
Date: Wed, 8 Mar 2023 11:56:33 +0100	[thread overview]
Message-ID: <ee9afb15-5bfd-b2fd-e3d8-ae585afbe87f@redhat.com> (raw)
In-Reply-To: <6e04ab8f-dc84-e9c2-deea-2b6b31678b53@linux.ibm.com>

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@redhat.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> 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