| Message ID | 1542904555-1136-3-git-send-email-pmorel@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x/vfio: VFIO-AP interrupt control interception | expand |
On Thu, 22 Nov 2018 17:35:51 +0100 Pierre Morel <pmorel@linux.ibm.com> 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) > +{ > + static DeviceState *apb; > + 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 */ So, there'll never a mixed environment? Or would that have a 'hybrid' ap device? > + 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); > + 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 { > VFIODevice vdev; > } VFIOAPDevice; > > -#define VFIO_AP_DEVICE(obj) \ > - OBJECT_CHECK(VFIOAPDevice, (obj), VFIO_AP_DEVICE_TYPE) Hm? > - > static void vfio_ap_compute_needs_reset(VFIODevice *vdev) > { > vdev->needs_reset = false;
On 29/11/2018 12:45, Cornelia Huck wrote: > On Thu, 22 Nov 2018 17:35:51 +0100 > Pierre Morel <pmorel@linux.ibm.com> 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) >> +{ >> + static DeviceState *apb; >> + 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 */ > > So, there'll never a mixed environment? Or would that have a 'hybrid' > ap device? Not for now, we only support VFIOAPDevice and we can only have one single VFIO device per guest. > >> + 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); >> + 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 { >> VFIODevice vdev; >> } VFIOAPDevice; >> >> -#define VFIO_AP_DEVICE(obj) \ >> - OBJECT_CHECK(VFIOAPDevice, (obj), VFIO_AP_DEVICE_TYPE) > > Hm? We do not need this anymore. We used to plug directly the VFIOAPDevice on the BUS but the APDevice is now plugged on the APBUS. The VFIOAPDevice is plugged on the APBus through its APDevice. This will allow to have different APDevice childs using APDevice features. Usable for interception or emulation. > >> - >> static void vfio_ap_compute_needs_reset(VFIODevice *vdev) >> { >> vdev->needs_reset = false; >
On 11/29/18 6:45 AM, Cornelia Huck wrote: > On Thu, 22 Nov 2018 17:35:51 +0100 > Pierre Morel <pmorel@linux.ibm.com> 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) >> +{ >> + static DeviceState *apb; >> + 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 */ > > So, there'll never a mixed environment? Or would that have a 'hybrid' > ap device? It is not possible to have interpretation and interception. I suppose one could mix emulated and virtual AP devices, but that seems highly unlikely; in fact, I think it is highly unlikely that emulation is ever implemented. > >> + 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); >> + 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 { >> VFIODevice vdev; >> } VFIOAPDevice; >> >> -#define VFIO_AP_DEVICE(obj) \ >> - OBJECT_CHECK(VFIOAPDevice, (obj), VFIO_AP_DEVICE_TYPE) > > Hm? 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. This macro should probably be restored and an AP_DEVICE() macro added. > >> - >> static void vfio_ap_compute_needs_reset(VFIODevice *vdev) >> { >> vdev->needs_reset = false; >
On Thu, 29 Nov 2018 10:02:24 -0500 Tony Krowiak <akrowiak@linux.ibm.com> wrote: > On 11/29/18 6:45 AM, Cornelia Huck wrote: > > On Thu, 22 Nov 2018 17:35:51 +0100 > > Pierre Morel <pmorel@linux.ibm.com> wrote: > >> 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 { > >> VFIODevice vdev; > >> } VFIOAPDevice; > >> > >> -#define VFIO_AP_DEVICE(obj) \ > >> - OBJECT_CHECK(VFIOAPDevice, (obj), VFIO_AP_DEVICE_TYPE) > > > > Hm? > > 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. This macro > should probably be restored and an AP_DEVICE() macro added. Yes, that makes sense.
On 29/11/2018 16:11, Cornelia Huck wrote: > On Thu, 29 Nov 2018 10:02:24 -0500 > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >> On 11/29/18 6:45 AM, Cornelia Huck wrote: >>> On Thu, 22 Nov 2018 17:35:51 +0100 >>> Pierre Morel <pmorel@linux.ibm.com> wrote: > >>>> 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 { >>>> VFIODevice vdev; >>>> } VFIOAPDevice; >>>> >>>> -#define VFIO_AP_DEVICE(obj) \ >>>> - OBJECT_CHECK(VFIOAPDevice, (obj), VFIO_AP_DEVICE_TYPE) >>> >>> Hm? >> >> 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. This macro >> should probably be restored and an AP_DEVICE() macro added. > > Yes, that makes sense. > Yes, Thanks Pierre
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 */ >
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 <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)? 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. > >> + 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 > Thanks I will do. Regards, Pierre
On 30/11/2018 10:31, 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 <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)? > > Yes, keep same conventions. Apropos convention, this function is exported. So I think the s390 prefix is important. Pierre
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 <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)? > > 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 >> <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 >> > > Thanks I will do. > > Regards, > Pierre > >
On 30/11/2018 16:58, Tony Krowiak wrote: > 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 <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)? >> >> 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. OK thanks, yes we do not need the static for apb here.
On Thu, 22 Nov 2018 17:35:51 +0100 Pierre Morel <pmorel@linux.ibm.com> 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) > +{ > + static DeviceState *apb; > + 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); > + 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 { > 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); > + VFIOAPDevice *vapdev = DO_UPCAST(VFIOAPDevice, apdev, apdev); > > 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); > 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); > + 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; Inheriting this to the parent class as opposed to specifying it here makes sense. > } > > 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); > + > #endif /* HW_S390X_AP_DEVICE_H */
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) +{ + static DeviceState *apb; + 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); + 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 { 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); + VFIOAPDevice *vapdev = DO_UPCAST(VFIOAPDevice, apdev, apdev); 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); 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); + 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); + #endif /* HW_S390X_AP_DEVICE_H */
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(-)