Re: [Qemu-devel] [PATCH v2 2/6] s390x/vfio: ap: Use the APdevice as a child of the APBus

[Tony Krowiak <akrowiak@linux.ibm.com>]

On 11/22/18 11:35 AM, Pierre Morel wrote:
> Two good reasons to use the base device as a child of the
> AP BUS:
> - We can easily find the device without traversing the qtree.
> - In case we have different APdevice instantiation, VFIO with
>    interception or emulation, we will need the APDevice as
>    a parent device.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   hw/s390x/ap-device.c         | 22 ++++++++++++++++++++++
>   hw/vfio/ap.c                 | 16 ++++++----------
>   include/hw/s390x/ap-device.h |  2 ++
>   3 files changed, 30 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/s390x/ap-device.c b/hw/s390x/ap-device.c
> index f5ac8db..554d5aa 100644
> --- a/hw/s390x/ap-device.c
> +++ b/hw/s390x/ap-device.c
> @@ -11,13 +11,35 @@
>   #include "qemu/module.h"
>   #include "qapi/error.h"
>   #include "hw/qdev.h"
> +#include "hw/s390x/ap-bridge.h"
>   #include "hw/s390x/ap-device.h"
>   
> +APDevice *s390_get_ap(void)

How about ap_get_device(void)?

> +{
> +    static DeviceState *apb;

Why static if you call s390_get_ap_bridge()
to retrieve it without first checking for NULL?

> +    BusState *bus;
> +    BusChild *child;
> +    static APDevice *ap;
> +
> +    if (ap) {
> +        return ap;
> +    }
> +
> +    apb = s390_get_ap_bridge();
> +    /* We have only a single child on the BUS */
> +    bus = qdev_get_child_bus(apb, TYPE_AP_BUS
> +    child = QTAILQ_FIRST(&bus->children);
> +    assert(child != NULL);
> +    ap = DO_UPCAST(APDevice, parent_obj, child->child);

I received a comment from Thomas Huth in Message ID
<2291104a-4cbf-e4fd-3496-fa0910beb96a@redhat.com>
that DO_UPCAST should be avoided in new code. You should
create an AP_DEVICE macro for this in ap-device.h

> +    return ap;
> +}
> +
>   static void ap_class_init(ObjectClass *klass, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
>   
>       dc->desc = "AP device class";
> +    dc->bus_type = TYPE_AP_BUS;
>       dc->hotpluggable = false;
>   }
>   
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 65de952..94e5a1a 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -35,9 +35,6 @@ typedef struct VFIOAPDevice {See my comment above about DO_UPCAST
>       VFIODevice vdev;
>   } VFIOAPDevice;
>   
> -#define VFIO_AP_DEVICE(obj) \
> -        OBJECT_CHECK(VFIOAPDevice, (obj), VFIO_AP_DEVICE_TYPE)
> -
>   static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
>   {
>       vdev->needs_reset = false;
> @@ -90,8 +87,8 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
>       char *mdevid;
>       Error *local_err = NULL;
>       VFIOGroup *vfio_group;
> -    APDevice *apdev = AP_DEVICE(dev);
> -    VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev);
> +    APDevice *apdev = DO_UPCAST(APDevice, parent_obj, dev);See my comment above about DO_UPCAST
> +    VFIOAPDevice *vapdev = DO_UPCAST(VFIOAPDevice, apdev, apdev);

See my comment above about DO_UPCAST.

>   
>       vfio_group = vfio_ap_get_group(vapdev, &local_err);
>       if (!vfio_group) {
> @@ -120,8 +117,8 @@ out_err:
>   
>   static void vfio_ap_unrealize(DeviceState *dev, Error **errp)
>   {
> -    APDevice *apdev = AP_DEVICE(dev);
> -    VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev);
> +    APDevice *apdev = DO_UPCAST(APDevice, parent_obj, dev);
> +    VFIOAPDevice *vapdev = DO_UPCAST(VFIOAPDevice, apdev, apdev);

See my comment above about DO_UPCAST

>       VFIOGroup *group = vapdev->vdev.group;
>   
>       vfio_ap_put_device(vapdev);
> @@ -136,8 +133,8 @@ static Property vfio_ap_properties[] = {
>   static void vfio_ap_reset(DeviceState *dev)
>   {
>       int ret;
> -    APDevice *apdev = AP_DEVICE(dev);
> -    VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev);

FYI, these macros were created in response to Thomas Huth's
comments about DO_UPCAST.

> +    APDevice *apdev = DO_UPCAST(APDevice, parent_obj, dev);
> +    VFIOAPDevice *vapdev = DO_UPCAST(VFIOAPDevice, apdev, apdev);
>   
>       ret = ioctl(vapdev->vdev.fd, VFIO_DEVICE_RESET);
>       if (ret) {
> @@ -163,7 +160,6 @@ static void vfio_ap_class_init(ObjectClass *klass, void *data)
>       dc->unrealize = vfio_ap_unrealize;
>       dc->hotpluggable = false;
>       dc->reset = vfio_ap_reset;
> -    dc->bus_type = TYPE_AP_BUS;
>   }
>   
>   static const TypeInfo vfio_ap_info = {
> diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h
> index 765e908..5f3c840 100644
> --- a/include/hw/s390x/ap-device.h
> +++ b/include/hw/s390x/ap-device.h
> @@ -19,4 +19,6 @@ typedef struct APDevice {
>   #define AP_DEVICE(obj) \
>       OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE)
>   
> +APDevice *s390_get_ap(void);

How about APDevice *ap_get_device(void)?

> +
>   #endif /* HW_S390X_AP_DEVICE_H */
>