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

[Tony Krowiak]

On 11/30/18 4:31 AM, Pierre Morel wrote:
> On 29/11/2018 21:42, Tony Krowiak wrote:
> > 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 <address@hidden>
> > > ---
> > >   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)?
>
> Yes, keep same conventions.
>
> > > +{
> > > +    static DeviceState *apb;
> >
> > Why static if you call s390_get_ap_bridge()
> > to retrieve it without first checking for NULL?
>
> static are initialized as NULL.

Yes, but down below, you call s390_get_ap_bridge() to set apb
regardless of whether apb is NULL or not. It makes no sense to
declare is as static here. It is also redundant because the 
s390_get_ap_bridge() function already caches it in a static
variable.

> > > +    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
> > <address@hidden>
> > that DO_UPCAST should be avoided in new code. You should
> > create an AP_DEVICE macro for this in ap-device.h
>
> Thanks I will do.
>
> Regards,
> Pierre