| Message ID | 20230213070820.76881-17-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw: Use QOM macros and remove DO_UPCAST() uses | expand |
On Mon, 2023-02-13 at 08:08 +0100, Philippe Mathieu-Daudé wrote: > Use the VFIO_CCW() QOM type-checking macro to avoid DO_UPCAST(). > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > hw/vfio/ccw.c | 35 ++++++++++++++++------------------- > 1 file changed, 16 insertions(+), 19 deletions(-) > > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c > index 0354737666..a8aa5b48c4 100644 > --- a/hw/vfio/ccw.c > +++ b/hw/vfio/ccw.c ...snip... > @@ -252,8 +248,8 @@ again: > static void vfio_ccw_reset(DeviceState *dev) > { > CcwDevice *ccw_dev = DO_UPCAST(CcwDevice, parent_obj, dev); If I'm not mistaken, I believe that this (and (un)realize below) could be changed to: CcwDevice *ccw_dev = CCW_DEVICE(dev); > - S390CCWDevice *cdev = DO_UPCAST(S390CCWDevice, parent_obj, > ccw_dev); > - VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev); > + S390CCWDevice *cdev = S390_CCW_DEVICE(ccw_dev); > + VFIOCCWDevice *vcdev = VFIO_CCW(cdev); > > ioctl(vcdev->vdev.fd, VFIO_DEVICE_RESET); > } ...snip... > @@ -657,9 +654,9 @@ 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); > + S390CCWDevice *cdev = S390_CCW_DEVICE(ccw_dev); > S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev); > + VFIOCCWDevice *vcdev = VFIO_CCW(cdev); > Error *err = NULL; > > /* Call the class init function for subchannel. */ > @@ -729,9 +726,9 @@ out_err_propagate: > static void vfio_ccw_unrealize(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); > + S390CCWDevice *cdev = S390_CCW_DEVICE(ccw_dev); > S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev); > + VFIOCCWDevice *vcdev = VFIO_CCW(cdev); > VFIOGroup *group = vcdev->vdev.group; > > vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX);
On 13/2/23 16:29, Eric Farman wrote: > On Mon, 2023-02-13 at 08:08 +0100, Philippe Mathieu-Daudé wrote: >> Use the VFIO_CCW() QOM type-checking macro to avoid DO_UPCAST(). >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> hw/vfio/ccw.c | 35 ++++++++++++++++------------------- >> 1 file changed, 16 insertions(+), 19 deletions(-) >> >> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c >> index 0354737666..a8aa5b48c4 100644 >> --- a/hw/vfio/ccw.c >> +++ b/hw/vfio/ccw.c > > ...snip... > >> @@ -252,8 +248,8 @@ again: >> static void vfio_ccw_reset(DeviceState *dev) >> { >> CcwDevice *ccw_dev = DO_UPCAST(CcwDevice, parent_obj, dev); > > If I'm not mistaken, I believe that this (and (un)realize below) could > be changed to: > > CcwDevice *ccw_dev = CCW_DEVICE(dev); Even ... >> - S390CCWDevice *cdev = DO_UPCAST(S390CCWDevice, parent_obj, >> ccw_dev); >> - VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev); >> + S390CCWDevice *cdev = S390_CCW_DEVICE(ccw_dev); >> + VFIOCCWDevice *vcdev = VFIO_CCW(cdev); VFIOCCWDevice *vcdev = VFIO_CCW(dev); But I somehow got scared to of removing too many casts... Are these paths covered by a "make check-qtest" on a s390x host? >> ioctl(vcdev->vdev.fd, VFIO_DEVICE_RESET); >> } > > ...snip... > >> @@ -657,9 +654,9 @@ 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); >> + S390CCWDevice *cdev = S390_CCW_DEVICE(ccw_dev); >> S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev); >> + VFIOCCWDevice *vcdev = VFIO_CCW(cdev); >> Error *err = NULL; >> >> /* Call the class init function for subchannel. */ >> @@ -729,9 +726,9 @@ out_err_propagate: >> static void vfio_ccw_unrealize(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); >> + S390CCWDevice *cdev = S390_CCW_DEVICE(ccw_dev); >> S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev); >> + VFIOCCWDevice *vcdev = VFIO_CCW(cdev); >> VFIOGroup *group = vcdev->vdev.group; >> >> vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX); >
On 13/2/23 16:51, Philippe Mathieu-Daudé wrote: > On 13/2/23 16:29, Eric Farman wrote: >> On Mon, 2023-02-13 at 08:08 +0100, Philippe Mathieu-Daudé wrote: >>> Use the VFIO_CCW() QOM type-checking macro to avoid DO_UPCAST(). >>> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> --- >>> hw/vfio/ccw.c | 35 ++++++++++++++++------------------- >>> 1 file changed, 16 insertions(+), 19 deletions(-) >>> >>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c >>> index 0354737666..a8aa5b48c4 100644 >>> --- a/hw/vfio/ccw.c >>> +++ b/hw/vfio/ccw.c >> >> ...snip... >> >>> @@ -252,8 +248,8 @@ again: >>> static void vfio_ccw_reset(DeviceState *dev) >>> { >>> CcwDevice *ccw_dev = DO_UPCAST(CcwDevice, parent_obj, dev); >> >> If I'm not mistaken, I believe that this (and (un)realize below) could >> be changed to: >> >> CcwDevice *ccw_dev = CCW_DEVICE(dev); > > Even ... > >>> - S390CCWDevice *cdev = DO_UPCAST(S390CCWDevice, parent_obj, >>> ccw_dev); >>> - VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev); >>> + S390CCWDevice *cdev = S390_CCW_DEVICE(ccw_dev); >>> + VFIOCCWDevice *vcdev = VFIO_CCW(cdev); > > VFIOCCWDevice *vcdev = VFIO_CCW(dev); > > But I somehow got scared to of removing too many casts... > > Are these paths covered by a "make check-qtest" on a s390x host? They are covered by the Avocado tests :) $ avocado --show=app,console run -t arch:s390x tests/avocado
On Mon, 2023-02-13 at 17:10 +0100, Philippe Mathieu-Daudé wrote: > On 13/2/23 16:51, Philippe Mathieu-Daudé wrote: > > On 13/2/23 16:29, Eric Farman wrote: > > > On Mon, 2023-02-13 at 08:08 +0100, Philippe Mathieu-Daudé wrote: > > > > Use the VFIO_CCW() QOM type-checking macro to avoid > > > > DO_UPCAST(). > > > > > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > > > --- > > > > hw/vfio/ccw.c | 35 ++++++++++++++++------------------- > > > > 1 file changed, 16 insertions(+), 19 deletions(-) > > > > > > > > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c > > > > index 0354737666..a8aa5b48c4 100644 > > > > --- a/hw/vfio/ccw.c > > > > +++ b/hw/vfio/ccw.c > > > > > > ...snip... > > > > > > > @@ -252,8 +248,8 @@ again: > > > > static void vfio_ccw_reset(DeviceState *dev) > > > > { > > > > CcwDevice *ccw_dev = DO_UPCAST(CcwDevice, parent_obj, > > > > dev); > > > > > > If I'm not mistaken, I believe that this (and (un)realize below) > > > could > > > be changed to: > > > > > > CcwDevice *ccw_dev = CCW_DEVICE(dev); > > > > Even ... > > > > > > - S390CCWDevice *cdev = DO_UPCAST(S390CCWDevice, parent_obj, > > > > ccw_dev); > > > > - VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, > > > > cdev); > > > > + S390CCWDevice *cdev = S390_CCW_DEVICE(ccw_dev); > > > > + VFIOCCWDevice *vcdev = VFIO_CCW(cdev); > > > > VFIOCCWDevice *vcdev = VFIO_CCW(dev); Ha, I didn't look to see if we cared about the intermediary ones, but this is true here. (Realize cares a bit, but that's easy enough.) > > > > But I somehow got scared to of removing too many casts... > > > > Are these paths covered by a "make check-qtest" on a s390x host? > > They are covered by the Avocado tests :) > > $ avocado --show=app,console run -t arch:s390x tests/avocado > Woo! Then I'm happy with the big squash then. Reviewed-by: Eric Farman <farman@linux.ibm.com>
On 13/2/23 17:24, Eric Farman wrote: > On Mon, 2023-02-13 at 17:10 +0100, Philippe Mathieu-Daudé wrote: >> On 13/2/23 16:51, Philippe Mathieu-Daudé wrote: >>> On 13/2/23 16:29, Eric Farman wrote: >>>> On Mon, 2023-02-13 at 08:08 +0100, Philippe Mathieu-Daudé wrote: >>>>> Use the VFIO_CCW() QOM type-checking macro to avoid >>>>> DO_UPCAST(). >>>>> >>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>>>> --- >>>>> hw/vfio/ccw.c | 35 ++++++++++++++++------------------- >>>>> 1 file changed, 16 insertions(+), 19 deletions(-) >>>>> CcwDevice *ccw_dev = DO_UPCAST(CcwDevice, parent_obj, >>>>> dev); >>>> >>>> If I'm not mistaken, I believe that this (and (un)realize below) >>>> could >>>> be changed to: >>>> >>>> CcwDevice *ccw_dev = CCW_DEVICE(dev); >>> >>> Even ... >>> VFIOCCWDevice *vcdev = VFIO_CCW(dev); > > Ha, I didn't look to see if we cared about the intermediary ones, but > this is true here. (Realize cares a bit, but that's easy enough.) > >>> >>> But I somehow got scared to of removing too many casts... >>> >>> Are these paths covered by a "make check-qtest" on a s390x host? >> >> They are covered by the Avocado tests :) >> >> $ avocado --show=app,console run -t arch:s390x tests/avocado >> > > Woo! Then I'm happy with the big squash then. > > Reviewed-by: Eric Farman <farman@linux.ibm.com> Thanks! Posted cleaned v3 here: https://lore.kernel.org/qemu-devel/20230213170145.45666-1-philmd@linaro.org/
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c index 0354737666..a8aa5b48c4 100644 --- a/hw/vfio/ccw.c +++ b/hw/vfio/ccw.c @@ -76,8 +76,7 @@ struct VFIODeviceOps vfio_ccw_ops = { static IOInstEnding vfio_ccw_handle_request(SubchDev *sch) { - S390CCWDevice *cdev = sch->driver_data; - VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev); + VFIOCCWDevice *vcdev = VFIO_CCW(sch->driver_data); struct ccw_io_region *region = vcdev->io_region; int ret; @@ -125,8 +124,7 @@ again: static IOInstEnding vfio_ccw_handle_store(SubchDev *sch) { - S390CCWDevice *cdev = sch->driver_data; - VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev); + VFIOCCWDevice *vcdev = VFIO_CCW(sch->driver_data); SCHIB *schib = &sch->curr_status; struct ccw_schib_region *region = vcdev->schib_region; SCHIB *s; @@ -170,8 +168,7 @@ static IOInstEnding vfio_ccw_handle_store(SubchDev *sch) static int vfio_ccw_handle_clear(SubchDev *sch) { - S390CCWDevice *cdev = sch->driver_data; - VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev); + VFIOCCWDevice *vcdev = VFIO_CCW(sch->driver_data); struct ccw_cmd_region *region = vcdev->async_cmd_region; int ret; @@ -210,8 +207,7 @@ again: static int vfio_ccw_handle_halt(SubchDev *sch) { - S390CCWDevice *cdev = sch->driver_data; - VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev); + VFIOCCWDevice *vcdev = VFIO_CCW(sch->driver_data); struct ccw_cmd_region *region = vcdev->async_cmd_region; int ret; @@ -252,8 +248,8 @@ again: 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); + S390CCWDevice *cdev = S390_CCW_DEVICE(ccw_dev); + VFIOCCWDevice *vcdev = VFIO_CCW(cdev); ioctl(vcdev->vdev.fd, VFIO_DEVICE_RESET); } @@ -588,9 +584,10 @@ static void vfio_ccw_put_device(VFIOCCWDevice *vcdev) static void vfio_ccw_get_device(VFIOGroup *group, VFIOCCWDevice *vcdev, Error **errp) { - char *name = g_strdup_printf("%x.%x.%04x", vcdev->cdev.hostid.cssid, - vcdev->cdev.hostid.ssid, - vcdev->cdev.hostid.devid); + 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) { @@ -611,14 +608,14 @@ static void vfio_ccw_get_device(VFIOGroup *group, VFIOCCWDevice *vcdev, */ vcdev->vdev.ram_block_discard_allowed = true; - if (vfio_get_device(group, vcdev->cdev.mdevid, &vcdev->vdev, errp)) { + 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 = &vcdev->cdev.parent_obj.parent_obj; + vcdev->vdev.dev = &cdev->parent_obj.parent_obj; return; @@ -657,9 +654,9 @@ 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); + S390CCWDevice *cdev = S390_CCW_DEVICE(ccw_dev); S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev); + VFIOCCWDevice *vcdev = VFIO_CCW(cdev); Error *err = NULL; /* Call the class init function for subchannel. */ @@ -729,9 +726,9 @@ out_err_propagate: static void vfio_ccw_unrealize(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); + S390CCWDevice *cdev = S390_CCW_DEVICE(ccw_dev); S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev); + VFIOCCWDevice *vcdev = VFIO_CCW(cdev); VFIOGroup *group = vcdev->vdev.group; vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX);
Use the VFIO_CCW() QOM type-checking macro to avoid DO_UPCAST(). Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/vfio/ccw.c | 35 ++++++++++++++++------------------- 1 file changed, 16 insertions(+), 19 deletions(-)