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>
---
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);
- 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->dev = &vcdev->cdev.parent_obj.parent_obj;
/*
* All vfio-ccw devices are believed to operate in a way compatible with
@@ -609,80 +614,18 @@ static void vfio_ccw_get_device(VFIOGroup *group, VFIOCCWDevice *vcdev,
* 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, vcdev->cdev.mdevid, &vcdev->vdev, errp)) {
- goto out_err;
- }
+ vbasedev->ram_block_discard_allowed = true;
- vcdev->vdev.ops = &vfio_ccw_ops;
- vcdev->vdev.type = VFIO_DEVICE_TYPE_CCW;
- vcdev->vdev.name = name;
- vcdev->vdev.dev = &vcdev->cdev.parent_obj.parent_obj;
-
- 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;
- 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);
- Error *err = NULL;
-
- /* Call the class init function for subchannel. */
- if (cdc->realize) {
- cdc->realize(cdev, vcdev->vdev.sysfsdev, &err);
- if (err) {
- goto out_err_propagate;
- }
- }
-
- group = vfio_ccw_get_group(cdev, &err);
- if (!group) {
- goto out_group_err;
- }
-
- vfio_ccw_get_device(group, vcdev, &err);
- if (err) {
- goto out_device_err;
+ ret = vfio_attach_device(vbasedev, &address_space_memory, errp);
+ if (ret) {
+ g_free(vbasedev->name);
+ g_free(vbasedev->sysfsdev);
}
vfio_ccw_get_region(vcdev, &err);
if (err) {
- goto out_region_err;
+ goto out_get_dev_err;
}
vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX, &err);
@@ -714,11 +657,8 @@ out_irq_notifier_err:
vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX);
out_io_notifier_err:
vfio_ccw_put_region(vcdev);
-out_region_err:
- vfio_ccw_put_device(vcdev);
-out_device_err:
- vfio_put_group(group);
-out_group_err:
+out_get_dev_err:
+ vfio_detach_device(vbasedev);
if (cdc->unrealize) {
cdc->unrealize(cdev);
}
@@ -732,14 +672,12 @@ static void vfio_ccw_unrealize(DeviceState *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;
vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX);
vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_CRW_IRQ_INDEX);
vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX);
vfio_ccw_put_region(vcdev);
- vfio_ccw_put_device(vcdev);
- vfio_put_group(group);
+ vfio_detach_device(&vcdev->vdev);
if (cdc->unrealize) {
cdc->unrealize(cdev);
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 6e466ef037..b9ee56067c 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -993,7 +993,7 @@ static void vfio_disconnect_container(VFIOGroup *group)
}
}
-VFIOGroup *vfio_get_group(int groupid, AddressSpace *as, Error **errp)
+static VFIOGroup *vfio_get_group(int groupid, AddressSpace *as, Error **errp)
{
VFIOGroup *group;
VFIOContainer *bcontainer;
@@ -1062,7 +1062,7 @@ free_group_exit:
return NULL;
}
-void vfio_put_group(VFIOGroup *group)
+static void vfio_put_group(VFIOGroup *group)
{
if (!group || !QLIST_EMPTY(&group->device_list)) {
return;
@@ -1083,8 +1083,8 @@ void vfio_put_group(VFIOGroup *group)
}
}
-int vfio_get_device(VFIOGroup *group, const char *name,
- VFIODevice *vbasedev, Error **errp)
+static int vfio_get_device(VFIOGroup *group, const char *name,
+ VFIODevice *vbasedev, Error **errp)
{
struct vfio_device_info dev_info = { .argsz = sizeof(dev_info) };
int ret, fd;
--
2.37.3On 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;
/*
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
© 2016 - 2024 Red Hat, Inc.