| Message ID | 20180926225440.6204-5-akrowiak@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: vfio-ap: guest dedicated crypto adapters | expand |
On 27/09/2018 00:54, Tony Krowiak wrote: > From: Tony Krowiak <akrowiak@linux.ibm.com> > > Introduces the base object model for virtualizing AP devices. > > Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> > --- > MAINTAINERS | 12 ++++++ > hw/s390x/Makefile.objs | 2 + > hw/s390x/ap-bridge.c | 81 ++++++++++++++++++++++++++++++++++++ > hw/s390x/ap-device.c | 39 +++++++++++++++++ > hw/s390x/s390-virtio-ccw.c | 4 ++ > include/hw/s390x/ap-bridge.h | 37 ++++++++++++++++ > include/hw/s390x/ap-device.h | 38 +++++++++++++++++ > 7 files changed, 213 insertions(+) > create mode 100644 hw/s390x/ap-bridge.c > create mode 100644 hw/s390x/ap-device.c > create mode 100644 include/hw/s390x/ap-bridge.h > create mode 100644 include/hw/s390x/ap-device.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index d12518c08f10..97e8ed808bc0 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1199,6 +1199,18 @@ F: include/hw/s390x/s390-ccw.h > T: git git://github.com/cohuck/qemu.git s390-next > L: qemu-s390x@nongnu.org > > +vfio-ap > +M: Christian Borntraeger <borntraeger@de.ibm.com> > +M: Tony Krowiak <akrowiak@linux.ibm.com> > +M: Halil Pasic <pasic@linux.ibm.com> > +M: Pierre Morel <pmorel@linux.ibm.com> > +S: Supported > +F: hw/s390x/ap-device.c > +F: hw/s390x/ap-bridge.c > +F: include/hw/s390x/ap-device.h > +F: include/hw/s390x/ap-bridge.h > +L: qemu-s390x@nongnu.org > + > vhost > M: Michael S. Tsirkin <mst@redhat.com> > S: Supported > diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs > index 93282f7c593c..add89b150d90 100644 > --- a/hw/s390x/Makefile.objs > +++ b/hw/s390x/Makefile.objs > @@ -20,3 +20,5 @@ obj-$(CONFIG_TCG) += tod-qemu.o > obj-$(CONFIG_KVM) += s390-skeys-kvm.o > obj-$(CONFIG_KVM) += s390-stattrib-kvm.o > obj-y += s390-ccw.o > +obj-y += ap-device.o > +obj-y += ap-bridge.o > diff --git a/hw/s390x/ap-bridge.c b/hw/s390x/ap-bridge.c > new file mode 100644 > index 000000000000..8564dfa96ee7 > --- /dev/null > +++ b/hw/s390x/ap-bridge.c > @@ -0,0 +1,81 @@ > +/* > + * ap bridge > + * > + * Copyright 2018 IBM Corp. > + * Author(s): Halil Pasic <pasic@linux.ibm.com> > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or (at > + * your option) any later version. See the COPYING file in the top-level > + * directory. > + */ > +#include "qemu/osdep.h" > +#include "qapi/error.h" > +#include "hw/sysbus.h" > +#include "qemu/bitops.h" > +#include "hw/s390x/ap-bridge.h" > +#include "cpu.h" > + > +static char *vfio_ap_bus_get_dev_path(DeviceState *dev) > +{ > + /* at most one */ > + return g_strdup_printf("/1"); > +} > + > +static void vfio_ap_bus_class_init(ObjectClass *klass, void *data) > +{ > + BusClass *k = BUS_CLASS(klass); > + > + k->get_dev_path = vfio_ap_bus_get_dev_path; > + /* More than one vfio-ap device does not make sense */ > + k->max_dev = 1; > +} > + > +static const TypeInfo vfio_ap_bus_info = { > + .name = TYPE_AP_BUS, > + .parent = TYPE_BUS, > + .instance_size = sizeof(APBus), > + .class_init = vfio_ap_bus_class_init, > +}; > + > +void s390_init_ap(void) > +{ > + DeviceState *dev; > + > + /* If no AP instructions then no need for AP bridge */ > + if (!s390_has_feat(S390_FEAT_AP)) { > + return; > + } > + > + /* Create bridge device */ > + dev = qdev_create(NULL, TYPE_AP_BRIDGE); > + object_property_add_child(qdev_get_machine(), TYPE_AP_BRIDGE, > + OBJECT(dev), NULL); > + qdev_init_nofail(dev); > + > + /* Create bus on bridge device */ > + qbus_create(TYPE_AP_BUS, dev, TYPE_AP_BUS); > + } > + > + > + > +static void ap_bridge_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(klass); > + > + set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); > +} > + > +static const TypeInfo ap_bridge_info = { > + .name = TYPE_AP_BRIDGE, > + .parent = TYPE_SYS_BUS_DEVICE, > + .instance_size = sizeof(APBridge), > + .class_init = ap_bridge_class_init, > +}; > + > +static void ap_register(void) > +{ > + type_register_static(&ap_bridge_info); > + type_register_static(&vfio_ap_bus_info); > +} > + > +type_init(ap_register) > diff --git a/hw/s390x/ap-device.c b/hw/s390x/ap-device.c > new file mode 100644 > index 000000000000..3cd4bae52591 > --- /dev/null > +++ b/hw/s390x/ap-device.c > @@ -0,0 +1,39 @@ > +/* > + * Adjunct Processor (AP) matrix device > + * > + * Copyright 2018 IBM Corp. > + * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com> > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or (at > + * your option) any later version. See the COPYING file in the top-level > + * directory. > + */ > +#include "qemu/osdep.h" > +#include "qemu/module.h" > +#include "qapi/error.h" > +#include "hw/qdev.h" > +#include "hw/s390x/ap-device.h" > + > +static void ap_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(klass); > + > + dc->desc = "AP device class"; > + dc->hotpluggable = false; > +} > + > +static const TypeInfo ap_device_info = { > + .name = AP_DEVICE_TYPE, > + .parent = TYPE_DEVICE, > + .instance_size = sizeof(APDevice), > + .class_size = sizeof(APDeviceClass), > + .class_init = ap_class_init, > + .abstract = true, > +}; > + > +static void ap_device_register(void) > +{ > + type_register_static(&ap_device_info); > +} > + > +type_init(ap_device_register) > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index f0f7fdcaddf2..3c100c24f3e8 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -32,6 +32,7 @@ > #include "ipl.h" > #include "hw/s390x/s390-virtio-ccw.h" > #include "hw/s390x/css-bridge.h" > +#include "hw/s390x/ap-bridge.h" > #include "migration/register.h" > #include "cpu_models.h" > #include "hw/nmi.h" > @@ -263,6 +264,9 @@ static void ccw_init(MachineState *machine) > /* init the SIGP facility */ > s390_init_sigp(); > > + /* create AP bridge and bus(es) */ > + s390_init_ap(); > + > /* get a BUS */ > css_bus = virtual_css_bus_init(); > s390_init_ipl_dev(machine->kernel_filename, machine->kernel_cmdline, > diff --git a/include/hw/s390x/ap-bridge.h b/include/hw/s390x/ap-bridge.h > new file mode 100644 > index 000000000000..b6ca6ae4ab17 > --- /dev/null > +++ b/include/hw/s390x/ap-bridge.h > @@ -0,0 +1,37 @@ > +/* > + * ap bridge > + * > + * Copyright 2018 IBM Corp. > + * Author(s): Halil Pasic <pasic@linux.ibm.com> > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or (at > + * your option) any later version. See the COPYING file in the top-level > + * directory. > + */ > + > +#ifndef HW_S390X_AP_BRIDGE_H > +#define HW_S390X_AP_BRIDGE_H > +#include "qom/object.h" > +#include "hw/qdev-core.h" > +#include "hw/sysbus.h" > + > +typedef struct APBridge { > + SysBusDevice sysbus_dev; > + bool css_dev_path; > +} APBridge; > + > +#define TYPE_AP_BRIDGE "ap-bridge" > +#define AP_BRIDGE(obj) \ > + OBJECT_CHECK(APBridge, (obj), TYPE_AP_BRIDGE) > + > +typedef struct APBus { > + BusState parent_obj; > +} APBus; > + > +#define TYPE_AP_BUS "ap-bus" > +#define AP_BUS(obj) \ > + OBJECT_CHECK(APBus, (obj), TYPE_AP_BUS) > + > +void s390_init_ap(void); > + > +#endif > diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h > new file mode 100644 > index 000000000000..693df90cc041 > --- /dev/null > +++ b/include/hw/s390x/ap-device.h > @@ -0,0 +1,38 @@ > +/* > + * Adjunct Processor (AP) matrix device interfaces > + * > + * Copyright 2018 IBM Corp. > + * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com> > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or (at > + * your option) any later version. See the COPYING file in the top-level > + * directory. > + */ > +#ifndef HW_S390X_AP_DEVICE_H > +#define HW_S390X_AP_DEVICE_H > + > +#define AP_DEVICE_TYPE "ap-device" > + > +typedef struct APDevice { > + DeviceState parent_obj; > +} APDevice; > + > +typedef struct APDeviceClass { > + DeviceClass parent_class; > +} APDeviceClass; > + > +static inline APDevice *to_ap_dev(DeviceState *dev) > +{ > + return container_of(dev, APDevice, parent_obj); > +} > + > +#define AP_DEVICE(obj) \ > + OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE) > + > +#define AP_DEVICE_GET_CLASS(obj) \ > + OBJECT_GET_CLASS(APDeviceClass, (obj), AP_DEVICE_TYPE) > + > +#define AP_DEVICE_CLASS(klass) \ > + OBJECT_CLASS_CHECK(APDeviceClass, (klass), AP_DEVICE_TYPE) > + > +#endif /* HW_S390X_AP_DEVICE_H */ > Having looked only and machine+cpu model specific stuff Acked-by: David Hildenbrand <david@redhat.com>
On 2018-09-27 00:54, Tony Krowiak wrote: > From: Tony Krowiak <akrowiak@linux.ibm.com> > > Introduces the base object model for virtualizing AP devices. > > Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> > --- [...] > diff --git a/hw/s390x/ap-bridge.c b/hw/s390x/ap-bridge.c > new file mode 100644 > index 000000000000..8564dfa96ee7 > --- /dev/null > +++ b/hw/s390x/ap-bridge.c > @@ -0,0 +1,81 @@ > +/* > + * ap bridge > + * > + * Copyright 2018 IBM Corp. > + * Author(s): Halil Pasic <pasic@linux.ibm.com> > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or (at > + * your option) any later version. See the COPYING file in the top-level > + * directory. > + */ > +#include "qemu/osdep.h" > +#include "qapi/error.h" > +#include "hw/sysbus.h" > +#include "qemu/bitops.h" > +#include "hw/s390x/ap-bridge.h" > +#include "cpu.h" > + > +static char *vfio_ap_bus_get_dev_path(DeviceState *dev) > +{ > + /* at most one */ > + return g_strdup_printf("/1"); > +} > + > +static void vfio_ap_bus_class_init(ObjectClass *klass, void *data) > +{ > + BusClass *k = BUS_CLASS(klass); I think calling the variable "oc" (or something similar) instead of "klass" is prefered nowadays. > + k->get_dev_path = vfio_ap_bus_get_dev_path; > + /* More than one vfio-ap device does not make sense */ > + k->max_dev = 1; Would it make sense to set a DEVICE_CATEGORY here, too? > +} > + > +static const TypeInfo vfio_ap_bus_info = { > + .name = TYPE_AP_BUS, > + .parent = TYPE_BUS, > + .instance_size = sizeof(APBus), > + .class_init = vfio_ap_bus_class_init, > +}; > + > +void s390_init_ap(void) > +{ > + DeviceState *dev; > + > + /* If no AP instructions then no need for AP bridge */ > + if (!s390_has_feat(S390_FEAT_AP)) { > + return; > + } > + > + /* Create bridge device */ > + dev = qdev_create(NULL, TYPE_AP_BRIDGE); > + object_property_add_child(qdev_get_machine(), TYPE_AP_BRIDGE, > + OBJECT(dev), NULL); > + qdev_init_nofail(dev); > + > + /* Create bus on bridge device */ > + qbus_create(TYPE_AP_BUS, dev, TYPE_AP_BUS); > + } > + > + > + One empty line should be enough? > +static void ap_bridge_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(klass); > + > + set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); > +} > + > +static const TypeInfo ap_bridge_info = { > + .name = TYPE_AP_BRIDGE, > + .parent = TYPE_SYS_BUS_DEVICE, > + .instance_size = sizeof(APBridge), > + .class_init = ap_bridge_class_init, > +}; > + > +static void ap_register(void) > +{ > + type_register_static(&ap_bridge_info); > + type_register_static(&vfio_ap_bus_info); > +} > + > +type_init(ap_register) [...] > diff --git a/include/hw/s390x/ap-bridge.h b/include/hw/s390x/ap-bridge.h > new file mode 100644 > index 000000000000..b6ca6ae4ab17 > --- /dev/null > +++ b/include/hw/s390x/ap-bridge.h > @@ -0,0 +1,37 @@ > +/* > + * ap bridge > + * > + * Copyright 2018 IBM Corp. > + * Author(s): Halil Pasic <pasic@linux.ibm.com> > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or (at > + * your option) any later version. See the COPYING file in the top-level > + * directory. > + */ > + > +#ifndef HW_S390X_AP_BRIDGE_H > +#define HW_S390X_AP_BRIDGE_H > +#include "qom/object.h" > +#include "hw/qdev-core.h" > +#include "hw/sysbus.h" > + > +typedef struct APBridge { > + SysBusDevice sysbus_dev; > + bool css_dev_path; What is this css_dev_path variable good for? I don't see it used in any of the other patches? If you don't need it, I think you could get rid of this struct completely? > +} APBridge; > + > +#define TYPE_AP_BRIDGE "ap-bridge" > +#define AP_BRIDGE(obj) \ > + OBJECT_CHECK(APBridge, (obj), TYPE_AP_BRIDGE) > + > +typedef struct APBus { > + BusState parent_obj; > +} APBus; > + > +#define TYPE_AP_BUS "ap-bus" > +#define AP_BUS(obj) \ > + OBJECT_CHECK(APBus, (obj), TYPE_AP_BUS) I think you could also get rid of AP_BRIDGE(), AP_BUS() and maybe even struct APBus. > +void s390_init_ap(void); > + > +#endif > diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h > new file mode 100644 > index 000000000000..693df90cc041 > --- /dev/null > +++ b/include/hw/s390x/ap-device.h > @@ -0,0 +1,38 @@ > +/* > + * Adjunct Processor (AP) matrix device interfaces > + * > + * Copyright 2018 IBM Corp. > + * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com> > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or (at > + * your option) any later version. See the COPYING file in the top-level > + * directory. > + */ > +#ifndef HW_S390X_AP_DEVICE_H > +#define HW_S390X_AP_DEVICE_H > + > +#define AP_DEVICE_TYPE "ap-device" > + > +typedef struct APDevice { > + DeviceState parent_obj; > +} APDevice; > + > +typedef struct APDeviceClass { > + DeviceClass parent_class; > +} APDeviceClass; > + > +static inline APDevice *to_ap_dev(DeviceState *dev) > +{ > + return container_of(dev, APDevice, parent_obj); > +} > + > +#define AP_DEVICE(obj) \ > + OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE) > + > +#define AP_DEVICE_GET_CLASS(obj) \ > + OBJECT_GET_CLASS(APDeviceClass, (obj), AP_DEVICE_TYPE) > + > +#define AP_DEVICE_CLASS(klass) \ > + OBJECT_CLASS_CHECK(APDeviceClass, (klass), AP_DEVICE_TYPE) Do you really need any of these definitions except AP_DEVICE_TYPE ? Thomas
On Thu, 27 Sep 2018 14:29:01 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 2018-09-27 00:54, Tony Krowiak wrote: > > From: Tony Krowiak <akrowiak@linux.ibm.com> > > > > Introduces the base object model for virtualizing AP devices. > > > > Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> > > --- > > +typedef struct APBridge { > > + SysBusDevice sysbus_dev; > > + bool css_dev_path; > > What is this css_dev_path variable good for? I don't see it used in any > of the other patches? > If you don't need it, I think you could get rid of this struct completely? Huh, now I remember complaining about it before. Looks like a copy-and-paste from the css bridge; that variable is used for compat handling there (and should be ditched here). > > > +} APBridge; > > + > > +#define TYPE_AP_BRIDGE "ap-bridge" > > +#define AP_BRIDGE(obj) \ > > + OBJECT_CHECK(APBridge, (obj), TYPE_AP_BRIDGE) > > + > > +typedef struct APBus { > > + BusState parent_obj; > > +} APBus; > > + > > +#define TYPE_AP_BUS "ap-bus" > > +#define AP_BUS(obj) \ > > + OBJECT_CHECK(APBus, (obj), TYPE_AP_BUS) > > I think you could also get rid of AP_BRIDGE(), AP_BUS() and maybe even > struct APBus. If there's nothing interesting to put in these inherited structures, probably yes. > > > +void s390_init_ap(void); > > + > > +#endif > > diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h > > new file mode 100644 > > index 000000000000..693df90cc041 > > --- /dev/null > > +++ b/include/hw/s390x/ap-device.h > > @@ -0,0 +1,38 @@ > > +/* > > + * Adjunct Processor (AP) matrix device interfaces > > + * > > + * Copyright 2018 IBM Corp. > > + * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com> > > + * > > + * This work is licensed under the terms of the GNU GPL, version 2 or (at > > + * your option) any later version. See the COPYING file in the top-level > > + * directory. > > + */ > > +#ifndef HW_S390X_AP_DEVICE_H > > +#define HW_S390X_AP_DEVICE_H > > + > > +#define AP_DEVICE_TYPE "ap-device" > > + > > +typedef struct APDevice { > > + DeviceState parent_obj; > > +} APDevice; > > + > > +typedef struct APDeviceClass { > > + DeviceClass parent_class; > > +} APDeviceClass; > > + > > +static inline APDevice *to_ap_dev(DeviceState *dev) > > +{ > > + return container_of(dev, APDevice, parent_obj); > > +} > > + > > +#define AP_DEVICE(obj) \ > > + OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE) > > + > > +#define AP_DEVICE_GET_CLASS(obj) \ > > + OBJECT_GET_CLASS(APDeviceClass, (obj), AP_DEVICE_TYPE) > > + > > +#define AP_DEVICE_CLASS(klass) \ > > + OBJECT_CLASS_CHECK(APDeviceClass, (klass), AP_DEVICE_TYPE) > > Do you really need any of these definitions except AP_DEVICE_TYPE ? Same here, I think.
On 09/27/2018 02:52 PM, Cornelia Huck wrote: > On Thu, 27 Sep 2018 14:29:01 +0200 > Thomas Huth <thuth@redhat.com> wrote: > >> On 2018-09-27 00:54, Tony Krowiak wrote: >>> From: Tony Krowiak <akrowiak@linux.ibm.com> >>> >>> Introduces the base object model for virtualizing AP devices. >>> >>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> >>> --- > >>> +typedef struct APBridge { >>> + SysBusDevice sysbus_dev; >>> + bool css_dev_path; >> >> What is this css_dev_path variable good for? I don't see it used in any >> of the other patches? >> If you don't need it, I think you could get rid of this struct completely? > > Huh, now I remember complaining about it before. Looks like a > copy-and-paste from the css bridge; that variable is used for compat > handling there (and should be ditched here). > Yes, we definitively discussed this before. And yes, it is a copy-paste error. >> >>> +} APBridge; >>> + >>> +#define TYPE_AP_BRIDGE "ap-bridge" >>> +#define AP_BRIDGE(obj) \ >>> + OBJECT_CHECK(APBridge, (obj), TYPE_AP_BRIDGE) >>> + >>> +typedef struct APBus { >>> + BusState parent_obj; >>> +} APBus; >>> + >>> +#define TYPE_AP_BUS "ap-bus" >>> +#define AP_BUS(obj) \ >>> + OBJECT_CHECK(APBus, (obj), TYPE_AP_BUS) >> >> I think you could also get rid of AP_BRIDGE(), AP_BUS() and maybe even >> struct APBus. > > If there's nothing interesting to put in these inherited structures, > probably yes. > I was under impression that this is how inheritance/subtyping is done in QEMU. Was probably added for the sake of interface completeness. >> >>> +void s390_init_ap(void); >>> + >>> +#endif >>> diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h >>> new file mode 100644 >>> index 000000000000..693df90cc041 >>> --- /dev/null >>> +++ b/include/hw/s390x/ap-device.h >>> @@ -0,0 +1,38 @@ >>> +/* >>> + * Adjunct Processor (AP) matrix device interfaces >>> + * >>> + * Copyright 2018 IBM Corp. >>> + * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com> >>> + * >>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at >>> + * your option) any later version. See the COPYING file in the top-level >>> + * directory. >>> + */ >>> +#ifndef HW_S390X_AP_DEVICE_H >>> +#define HW_S390X_AP_DEVICE_H >>> + >>> +#define AP_DEVICE_TYPE "ap-device" >>> + >>> +typedef struct APDevice { >>> + DeviceState parent_obj; >>> +} APDevice; >>> + >>> +typedef struct APDeviceClass { >>> + DeviceClass parent_class; >>> +} APDeviceClass; >>> + >>> +static inline APDevice *to_ap_dev(DeviceState *dev) >>> +{ >>> + return container_of(dev, APDevice, parent_obj); >>> +} >>> + >>> +#define AP_DEVICE(obj) \ >>> + OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE) >>> + >>> +#define AP_DEVICE_GET_CLASS(obj) \ >>> + OBJECT_GET_CLASS(APDeviceClass, (obj), AP_DEVICE_TYPE) >>> + >>> +#define AP_DEVICE_CLASS(klass) \ >>> + OBJECT_CLASS_CHECK(APDeviceClass, (klass), AP_DEVICE_TYPE) >> >> Do you really need any of these definitions except AP_DEVICE_TYPE ? > > Same here, I think. > I don't think the qom utility/casting macros will ever be used. I'm fine with removing what is not needed. Regards, Halil
On 09/27/2018 12:54 AM, Tony Krowiak wrote: > From: Tony Krowiak <akrowiak@linux.ibm.com> > > Introduces the base object model for virtualizing AP devices. > [..] > + > +static char *vfio_ap_bus_get_dev_path(DeviceState *dev) Cover letter states you want to remove vifo_ap references form the bus. Sane thing to do as this would also be the bus we would like to plug an emulated ap device into (if these get implemented). There are a couple of such references that survived around this comment. > +{ > + /* at most one */ > + return g_strdup_printf("/1"); > +} > + > +static void vfio_ap_bus_class_init(ObjectClass *klass, void *data) > +{ > + BusClass *k = BUS_CLASS(klass); > + > + k->get_dev_path = vfio_ap_bus_get_dev_path; > + /* More than one vfio-ap device does not make sense */ > + k->max_dev = 1; > +} > + > +static const TypeInfo vfio_ap_bus_info = { > + .name = TYPE_AP_BUS, > + .parent = TYPE_BUS, > + .instance_size = sizeof(APBus), > + .class_init = vfio_ap_bus_class_init, > +}; > + [..] > +static void ap_register(void) > +{ > + type_register_static(&ap_bridge_info); > + type_register_static(&vfio_ap_bus_info); > +} > + [..]
On 09/27/2018 02:29 PM, Thomas Huth wrote: >> +static void vfio_ap_bus_class_init(ObjectClass *klass, void *data) >> +{ >> + BusClass *k = BUS_CLASS(klass); > I think calling the variable "oc" (or something similar) instead of > "klass" is prefered nowadays. > >> + k->get_dev_path = vfio_ap_bus_get_dev_path; >> + /* More than one vfio-ap device does not make sense */ >> + k->max_dev = 1; > Would it make sense to set a DEVICE_CATEGORY here, too? > It would be set_bit(DEVICE_CATEGORY_MISC, dc->categories); I guess. People seem to do this, but I'm not sure what is the actual impact of having DEVICE_CATEGORY_MISC set or not set, except for vfio_ap being listed as misc or uncategorized device respectively. Currently we have loader and vfio-ap uncategorized. Should loader become MISC too? >> +}
On 09/28/2018 09:57 AM, Halil Pasic wrote: > > > On 09/27/2018 12:54 AM, Tony Krowiak wrote: >> From: Tony Krowiak <akrowiak@linux.ibm.com> >> >> Introduces the base object model for virtualizing AP devices. >> > > [..] > >> + >> +static char *vfio_ap_bus_get_dev_path(DeviceState *dev) > > Cover letter states you want to remove vifo_ap references form the bus. Sane > thing to do as this would also be the bus we would like to plug an emulated > ap device into (if these get implemented). > > There are a couple of such references that survived around this comment. I noticed that earlier today as I was reviewing these comments and already removed the vfio references. > >> +{ >> + /* at most one */ >> + return g_strdup_printf("/1"); >> +} >> + >> +static void vfio_ap_bus_class_init(ObjectClass *klass, void *data) >> +{ >> + BusClass *k = BUS_CLASS(klass); >> + >> + k->get_dev_path = vfio_ap_bus_get_dev_path; >> + /* More than one vfio-ap device does not make sense */ >> + k->max_dev = 1; >> +} >> + >> +static const TypeInfo vfio_ap_bus_info = { >> + .name = TYPE_AP_BUS, >> + .parent = TYPE_BUS, >> + .instance_size = sizeof(APBus), >> + .class_init = vfio_ap_bus_class_init, >> +}; >> + > > [..] > >> +static void ap_register(void) >> +{ >> + type_register_static(&ap_bridge_info); >> + type_register_static(&vfio_ap_bus_info); >> +} >> + > > [..] >
On Fri, 28 Sep 2018 16:22:12 +0200 Halil Pasic <pasic@linux.ibm.com> wrote: > On 09/27/2018 02:29 PM, Thomas Huth wrote: > >> +static void vfio_ap_bus_class_init(ObjectClass *klass, void *data) > >> +{ > >> + BusClass *k = BUS_CLASS(klass); > > I think calling the variable "oc" (or something similar) instead of > > "klass" is prefered nowadays. > > > >> + k->get_dev_path = vfio_ap_bus_get_dev_path; > >> + /* More than one vfio-ap device does not make sense */ > >> + k->max_dev = 1; > > Would it make sense to set a DEVICE_CATEGORY here, too? > > > > It would be > set_bit(DEVICE_CATEGORY_MISC, dc->categories); > I guess. > > People seem to do this, but I'm not sure what is the actual impact of having > DEVICE_CATEGORY_MISC set or not set, except for vfio_ap being listed as misc > or uncategorized device respectively. Currently we have loader and vfio-ap > uncategorized. Should loader become MISC too? I think MISC is the right category.
On 2018-09-28 16:22, Halil Pasic wrote: > > > On 09/27/2018 02:29 PM, Thomas Huth wrote: >>> +static void vfio_ap_bus_class_init(ObjectClass *klass, void *data) >>> +{ >>> + BusClass *k = BUS_CLASS(klass); >> I think calling the variable "oc" (or something similar) instead of >> "klass" is prefered nowadays. >> >>> + k->get_dev_path = vfio_ap_bus_get_dev_path; >>> + /* More than one vfio-ap device does not make sense */ >>> + k->max_dev = 1; >> Would it make sense to set a DEVICE_CATEGORY here, too? >> > > It would be > set_bit(DEVICE_CATEGORY_MISC, dc->categories); > I guess. > > People seem to do this, but I'm not sure what is the actual impact of having > DEVICE_CATEGORY_MISC set or not set, except for vfio_ap being listed as misc > or uncategorized device respectively. Currently we have loader and vfio-ap > uncategorized. Should loader become MISC too? All devices that are visible in the output of "-device help" should have a category. If no other category fits, it should be put into MISC. So yes, loader should become MISC, too. Thomas
On 09/27/2018 08:29 AM, Thomas Huth wrote: > On 2018-09-27 00:54, Tony Krowiak wrote: >> From: Tony Krowiak <akrowiak@linux.ibm.com> >> >> Introduces the base object model for virtualizing AP devices. >> >> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> >> --- > [...] >> diff --git a/hw/s390x/ap-bridge.c b/hw/s390x/ap-bridge.c >> new file mode 100644 >> index 000000000000..8564dfa96ee7 >> --- /dev/null >> +++ b/hw/s390x/ap-bridge.c >> @@ -0,0 +1,81 @@ >> +/* >> + * ap bridge >> + * >> + * Copyright 2018 IBM Corp. >> + * Author(s): Halil Pasic <pasic@linux.ibm.com> >> + * >> + * This work is licensed under the terms of the GNU GPL, version 2 or (at >> + * your option) any later version. See the COPYING file in the top-level >> + * directory. >> + */ >> +#include "qemu/osdep.h" >> +#include "qapi/error.h" >> +#include "hw/sysbus.h" >> +#include "qemu/bitops.h" >> +#include "hw/s390x/ap-bridge.h" >> +#include "cpu.h" >> + >> +static char *vfio_ap_bus_get_dev_path(DeviceState *dev) >> +{ >> + /* at most one */ >> + return g_strdup_printf("/1"); >> +} >> + >> +static void vfio_ap_bus_class_init(ObjectClass *klass, void *data) >> +{ >> + BusClass *k = BUS_CLASS(klass); > > I think calling the variable "oc" (or something similar) instead of > "klass" is prefered nowadays. I did a search of the entire qemu project and did not find one case where the parameter to the BUS_CLASS macro was anything but klass. On the other hand, I also did a search for "bus_class_init(" and was also unable to to find a single instance of the use of anything other than klass or class. On the other hand, a search of "class_init(" found that most instances do us 'oc' for the ObjectClass parameter name. So, I'll assume that most busses have been around for a long time and that you are correct. Besides, I don't have a strong opinion about either name, so I will replace all 'ObjectClass *klass' with 'ObjectClass *oc' in the rest of this file too. > >> + k->get_dev_path = vfio_ap_bus_get_dev_path; >> + /* More than one vfio-ap device does not make sense */ >> + k->max_dev = 1; > > Would it make sense to set a DEVICE_CATEGORY here, too? It does not make sense set a DEVICE_CATEGORY here because categories do not exist for struct BusClass. Were you referring to the bridge device? The bridge class category is set to DEVICE_CATEGORY_BRIDGE in the ap_bridge_class_init() function below. If not, then to what are you referring here? > >> +} >> + >> +static const TypeInfo vfio_ap_bus_info = { >> + .name = TYPE_AP_BUS, >> + .parent = TYPE_BUS, >> + .instance_size = sizeof(APBus), >> + .class_init = vfio_ap_bus_class_init, >> +}; >> + >> +void s390_init_ap(void) >> +{ >> + DeviceState *dev; >> + >> + /* If no AP instructions then no need for AP bridge */ >> + if (!s390_has_feat(S390_FEAT_AP)) { >> + return; >> + } >> + >> + /* Create bridge device */ >> + dev = qdev_create(NULL, TYPE_AP_BRIDGE); >> + object_property_add_child(qdev_get_machine(), TYPE_AP_BRIDGE, >> + OBJECT(dev), NULL); >> + qdev_init_nofail(dev); >> + >> + /* Create bus on bridge device */ >> + qbus_create(TYPE_AP_BUS, dev, TYPE_AP_BUS); >> + } >> + >> + >> + > > One empty line should be enough? I'll fix that > >> +static void ap_bridge_class_init(ObjectClass *klass, void *data) >> +{ >> + DeviceClass *dc = DEVICE_CLASS(klass); >> + >> + set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); >> +} >> + >> +static const TypeInfo ap_bridge_info = { >> + .name = TYPE_AP_BRIDGE, >> + .parent = TYPE_SYS_BUS_DEVICE, >> + .instance_size = sizeof(APBridge), >> + .class_init = ap_bridge_class_init, >> +}; >> + >> +static void ap_register(void) >> +{ >> + type_register_static(&ap_bridge_info); >> + type_register_static(&vfio_ap_bus_info); >> +} >> + >> +type_init(ap_register) > [...] >> diff --git a/include/hw/s390x/ap-bridge.h b/include/hw/s390x/ap-bridge.h >> new file mode 100644 >> index 000000000000..b6ca6ae4ab17 >> --- /dev/null >> +++ b/include/hw/s390x/ap-bridge.h >> @@ -0,0 +1,37 @@ >> +/* >> + * ap bridge >> + * >> + * Copyright 2018 IBM Corp. >> + * Author(s): Halil Pasic <pasic@linux.ibm.com> >> + * >> + * This work is licensed under the terms of the GNU GPL, version 2 or (at >> + * your option) any later version. See the COPYING file in the top-level >> + * directory. >> + */ >> + >> +#ifndef HW_S390X_AP_BRIDGE_H >> +#define HW_S390X_AP_BRIDGE_H >> +#include "qom/object.h" >> +#include "hw/qdev-core.h" >> +#include "hw/sysbus.h" >> + >> +typedef struct APBridge { >> + SysBusDevice sysbus_dev; >> + bool css_dev_path; > > What is this css_dev_path variable good for? I don't see it used in any > of the other patches? > If you don't need it, I think you could get rid of this struct completely? > >> +} APBridge; >> + >> +#define TYPE_AP_BRIDGE "ap-bridge" >> +#define AP_BRIDGE(obj) \DEVICE_CATEGORY_BRIDGE >> + OBJECT_CHECK(APBridge, (obj), TYPE_AP_BRIDGE) >> + >> +typedef struct APBus { >> + BusState parent_obj; >> +} APBus; >> + >> +#define TYPE_AP_BUS "ap-bus" >> +#define AP_BUS(obj) \ >> + OBJECT_CHECK(APBus, (obj), TYPE_AP_BUS) > > I think you could also get rid of AP_BRIDGE(), AP_BUS() and maybe even > struct APBus. > >> +void s390_init_ap(void); >> + >> +#endif >> diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h >> new file mode 100644 >> index 000000000000..693df90cc041 >> --- /dev/null >> +++ b/include/hw/s390x/ap-device.h >> @@ -0,0 +1,38 @@ >> +/* >> + * Adjunct Processor (AP) matrix device interfaces >> + * >> + * Copyright 2018 IBM Corp. >> + * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com> >> + * >> + * This work is licensed under the terms of the GNU GPL, version 2 or (at >> + * your option) any later version. See the COPYING file in the top-level >> + * directory. >> + */ >> +#ifndef HW_S390X_AP_DEVICE_H >> +#define HW_S390X_AP_DEVICE_H >> + >> +#define AP_DEVICE_TYPE "ap-device" >> + >> +typedef struct APDevice { >> + DeviceState parent_obj; >> +} APDevice; >> + >> +typedef struct APDeviceClass { >> + DeviceClass parent_class; >> +} APDeviceClass; >> + >> +static inline APDevice *to_ap_dev(DeviceState *dev) >> +{ >> + return container_of(dev, APDevice, parent_obj); >> +} >> + >> +#define AP_DEVICE(obj) \ >> + OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE) >> + >> +#define AP_DEVICE_GET_CLASS(obj) \ >> + OBJECT_GET_CLASS(APDeviceClass, (obj), AP_DEVICE_TYPE) >> + >> +#define AP_DEVICE_CLASS(klass) \ >> + OBJECT_CLASS_CHECK(APDeviceClass, (klass), AP_DEVICE_TYPE) > > Do you really need any of these definitions except AP_DEVICE_TYPE ? > > Thomas >
On 09/28/2018 08:51 AM, Halil Pasic wrote: > > > On 09/27/2018 02:52 PM, Cornelia Huck wrote: >> On Thu, 27 Sep 2018 14:29:01 +0200 >> Thomas Huth <thuth@redhat.com> wrote: >> >>> On 2018-09-27 00:54, Tony Krowiak wrote: >>>> From: Tony Krowiak <akrowiak@linux.ibm.com> >>>> >>>> Introduces the base object model for virtualizing AP devices. >>>> >>>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> >>>> --- >> >>>> +typedef struct APBridge { >>>> + SysBusDevice sysbus_dev; >>>> + bool css_dev_path; >>> >>> What is this css_dev_path variable good for? I don't see it used in any >>> of the other patches? >>> If you don't need it, I think you could get rid of this struct completely? >> >> Huh, now I remember complaining about it before. Looks like a >> copy-and-paste from the css bridge; that variable is used for compat >> handling there (and should be ditched here). >> > > Yes, we definitively discussed this before. And yes, it is a copy-paste > error. > > >>> >>>> +} APBridge; >>>> + >>>> +#define TYPE_AP_BRIDGE "ap-bridge" >>>> +#define AP_BRIDGE(obj) \ >>>> + OBJECT_CHECK(APBridge, (obj), TYPE_AP_BRIDGE) >>>> + >>>> +typedef struct APBus { >>>> + BusState parent_obj; >>>> +} APBus; >>>> + >>>> +#define TYPE_AP_BUS "ap-bus" >>>> +#define AP_BUS(obj) \ >>>> + OBJECT_CHECK(APBus, (obj), TYPE_AP_BUS) >>> >>> I think you could also get rid of AP_BRIDGE(), AP_BUS() and maybe even >>> struct APBus. >> >> If there's nothing interesting to put in these inherited structures, >> probably yes. >> > > I was under impression that this is how inheritance/subtyping is done in > QEMU. Was probably added for the sake of interface completeness. > >>> >>>> +void s390_init_ap(void); >>>> + >>>> +#endif >>>> diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h >>>> new file mode 100644 >>>> index 000000000000..693df90cc041 >>>> --- /dev/null >>>> +++ b/include/hw/s390x/ap-device.h >>>> @@ -0,0 +1,38 @@ >>>> +/* >>>> + * Adjunct Processor (AP) matrix device interfaces >>>> + * >>>> + * Copyright 2018 IBM Corp. >>>> + * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com> >>>> + * >>>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at >>>> + * your option) any later version. See the COPYING file in the top-level >>>> + * directory. >>>> + */ >>>> +#ifndef HW_S390X_AP_DEVICE_H >>>> +#define HW_S390X_AP_DEVICE_H >>>> + >>>> +#define AP_DEVICE_TYPE "ap-device" >>>> + >>>> +typedef struct APDevice { >>>> + DeviceState parent_obj; >>>> +} APDevice; >>>> + >>>> +typedef struct APDeviceClass { >>>> + DeviceClass parent_class; >>>> +} APDeviceClass; >>>> + >>>> +static inline APDevice *to_ap_dev(DeviceState *dev) >>>> +{ >>>> + return container_of(dev, APDevice, parent_obj); >>>> +} >>>> + >>>> +#define AP_DEVICE(obj) \ >>>> + OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE) >>>> + >>>> +#define AP_DEVICE_GET_CLASS(obj) \ >>>> + OBJECT_GET_CLASS(APDeviceClass, (obj), AP_DEVICE_TYPE) >>>> + >>>> +#define AP_DEVICE_CLASS(klass) \ >>>> + OBJECT_CLASS_CHECK(APDeviceClass, (klass), AP_DEVICE_TYPE) >>> >>> Do you really need any of these definitions except AP_DEVICE_TYPE ? >> >> Same here, I think. >> > > I don't think the qom utility/casting macros will ever be used. I'm > fine with removing what is not needed. > > Regards, > Halil Per Thomas's suggestions and Connie's concurrence, I am going to remove the macros as well as the structures they reference. The ap_bridge.h will be relegated to defining the bridge and bus types and serve as a container for AP bridge and bus definitions that might be needed in the future. > > >
On 27/09/2018 00:54, Tony Krowiak wrote: > From: Tony Krowiak <akrowiak@linux.ibm.com> > > Introduces the base object model for virtualizing AP devices. > > Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> > --- > MAINTAINERS | 12 ++++++ > hw/s390x/Makefile.objs | 2 + > hw/s390x/ap-bridge.c | 81 ++++++++++++++++++++++++++++++++++++ > hw/s390x/ap-device.c | 39 +++++++++++++++++ > hw/s390x/s390-virtio-ccw.c | 4 ++ > include/hw/s390x/ap-bridge.h | 37 ++++++++++++++++ > include/hw/s390x/ap-device.h | 38 +++++++++++++++++ > 7 files changed, 213 insertions(+) > create mode 100644 hw/s390x/ap-bridge.c > create mode 100644 hw/s390x/ap-device.c > create mode 100644 include/hw/s390x/ap-bridge.h > create mode 100644 include/hw/s390x/ap-device.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index d12518c08f10..97e8ed808bc0 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1199,6 +1199,18 @@ F: include/hw/s390x/s390-ccw.h > T: git git://github.com/cohuck/qemu.git s390-next > L: qemu-s390x@nongnu.org > > +vfio-ap > +M: Christian Borntraeger <borntraeger@de.ibm.com> > +M: Tony Krowiak <akrowiak@linux.ibm.com> > +M: Halil Pasic <pasic@linux.ibm.com> > +M: Pierre Morel <pmorel@linux.ibm.com> > +S: Supported > +F: hw/s390x/ap-device.c > +F: hw/s390x/ap-bridge.c > +F: include/hw/s390x/ap-device.h > +F: include/hw/s390x/ap-bridge.h > +L: qemu-s390x@nongnu.org > + > vhost > M: Michael S. Tsirkin <mst@redhat.com> > S: Supported > diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs > index 93282f7c593c..add89b150d90 100644 > --- a/hw/s390x/Makefile.objs > +++ b/hw/s390x/Makefile.objs > @@ -20,3 +20,5 @@ obj-$(CONFIG_TCG) += tod-qemu.o > obj-$(CONFIG_KVM) += s390-skeys-kvm.o > obj-$(CONFIG_KVM) += s390-stattrib-kvm.o > obj-y += s390-ccw.o > +obj-y += ap-device.o > +obj-y += ap-bridge.o > diff --git a/hw/s390x/ap-bridge.c b/hw/s390x/ap-bridge.c > new file mode 100644 > index 000000000000..8564dfa96ee7 > --- /dev/null > +++ b/hw/s390x/ap-bridge.c > @@ -0,0 +1,81 @@ > +/* > + * ap bridge > + * > + * Copyright 2018 IBM Corp. > + * Author(s): Halil Pasic <pasic@linux.ibm.com> > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or (at > + * your option) any later version. See the COPYING file in the top-level > + * directory. > + */ > +#include "qemu/osdep.h" > +#include "qapi/error.h" > +#include "hw/sysbus.h" > +#include "qemu/bitops.h" > +#include "hw/s390x/ap-bridge.h" > +#include "cpu.h" > + > +static char *vfio_ap_bus_get_dev_path(DeviceState *dev) > +{ > + /* at most one */ > + return g_strdup_printf("/1"); > +} > + > +static void vfio_ap_bus_class_init(ObjectClass *klass, void *data) > +{ > + BusClass *k = BUS_CLASS(klass); > + > + k->get_dev_path = vfio_ap_bus_get_dev_path; > + /* More than one vfio-ap device does not make sense */ > + k->max_dev = 1; > +} > + > +static const TypeInfo vfio_ap_bus_info = { > + .name = TYPE_AP_BUS, > + .parent = TYPE_BUS, > + .instance_size = sizeof(APBus), > + .class_init = vfio_ap_bus_class_init, > +}; > + > +void s390_init_ap(void) > +{ > + DeviceState *dev; > + > + /* If no AP instructions then no need for AP bridge */ > + if (!s390_has_feat(S390_FEAT_AP)) { > + return; > + } > + > + /* Create bridge device */ > + dev = qdev_create(NULL, TYPE_AP_BRIDGE); > + object_property_add_child(qdev_get_machine(), TYPE_AP_BRIDGE, > + OBJECT(dev), NULL); > + qdev_init_nofail(dev); > + > + /* Create bus on bridge device */ > + qbus_create(TYPE_AP_BUS, dev, TYPE_AP_BUS); > + } > + > + > + > +static void ap_bridge_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(klass); > + > + set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); > +} > + > +static const TypeInfo ap_bridge_info = { > + .name = TYPE_AP_BRIDGE, > + .parent = TYPE_SYS_BUS_DEVICE, > + .instance_size = sizeof(APBridge), > + .class_init = ap_bridge_class_init, > +}; > + > +static void ap_register(void) > +{ > + type_register_static(&ap_bridge_info); > + type_register_static(&vfio_ap_bus_info); > +} > + > +type_init(ap_register) > diff --git a/hw/s390x/ap-device.c b/hw/s390x/ap-device.c > new file mode 100644 > index 000000000000..3cd4bae52591 > --- /dev/null > +++ b/hw/s390x/ap-device.c > @@ -0,0 +1,39 @@ > +/* > + * Adjunct Processor (AP) matrix device > + * > + * Copyright 2018 IBM Corp. > + * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com> > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or (at > + * your option) any later version. See the COPYING file in the top-level > + * directory. > + */ > +#include "qemu/osdep.h" > +#include "qemu/module.h" > +#include "qapi/error.h" > +#include "hw/qdev.h" > +#include "hw/s390x/ap-device.h" > + > +static void ap_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(klass); > + > + dc->desc = "AP device class"; > + dc->hotpluggable = false; > +} > + > +static const TypeInfo ap_device_info = { > + .name = AP_DEVICE_TYPE, > + .parent = TYPE_DEVICE, > + .instance_size = sizeof(APDevice), > + .class_size = sizeof(APDeviceClass), > + .class_init = ap_class_init, > + .abstract = true, > +}; > + > +static void ap_device_register(void) > +{ > + type_register_static(&ap_device_info); > +} > + > +type_init(ap_device_register) > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index f0f7fdcaddf2..3c100c24f3e8 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -32,6 +32,7 @@ > #include "ipl.h" > #include "hw/s390x/s390-virtio-ccw.h" > #include "hw/s390x/css-bridge.h" > +#include "hw/s390x/ap-bridge.h" > #include "migration/register.h" > #include "cpu_models.h" > #include "hw/nmi.h" > @@ -263,6 +264,9 @@ static void ccw_init(MachineState *machine) > /* init the SIGP facility */ > s390_init_sigp(); > > + /* create AP bridge and bus(es) */ > + s390_init_ap(); > + > /* get a BUS */ > css_bus = virtual_css_bus_init(); > s390_init_ipl_dev(machine->kernel_filename, machine->kernel_cmdline, > diff --git a/include/hw/s390x/ap-bridge.h b/include/hw/s390x/ap-bridge.h > new file mode 100644 > index 000000000000..b6ca6ae4ab17 > --- /dev/null > +++ b/include/hw/s390x/ap-bridge.h > @@ -0,0 +1,37 @@ > +/* > + * ap bridge > + * > + * Copyright 2018 IBM Corp. > + * Author(s): Halil Pasic <pasic@linux.ibm.com> > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or (at > + * your option) any later version. See the COPYING file in the top-level > + * directory. > + */ > + > +#ifndef HW_S390X_AP_BRIDGE_H > +#define HW_S390X_AP_BRIDGE_H > +#include "qom/object.h" > +#include "hw/qdev-core.h" > +#include "hw/sysbus.h" > + > +typedef struct APBridge { > + SysBusDevice sysbus_dev; > + bool css_dev_path; > +} APBridge; > + > +#define TYPE_AP_BRIDGE "ap-bridge" > +#define AP_BRIDGE(obj) \ > + OBJECT_CHECK(APBridge, (obj), TYPE_AP_BRIDGE) > + > +typedef struct APBus { > + BusState parent_obj; > +} APBus; > + > +#define TYPE_AP_BUS "ap-bus" > +#define AP_BUS(obj) \ > + OBJECT_CHECK(APBus, (obj), TYPE_AP_BUS) > + > +void s390_init_ap(void); > + > +#endif > diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h > new file mode 100644 > index 000000000000..693df90cc041 > --- /dev/null > +++ b/include/hw/s390x/ap-device.h > @@ -0,0 +1,38 @@ > +/* > + * Adjunct Processor (AP) matrix device interfaces > + * > + * Copyright 2018 IBM Corp. > + * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com> > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or (at > + * your option) any later version. See the COPYING file in the top-level > + * directory. > + */ > +#ifndef HW_S390X_AP_DEVICE_H > +#define HW_S390X_AP_DEVICE_H > + > +#define AP_DEVICE_TYPE "ap-device" > + > +typedef struct APDevice { > + DeviceState parent_obj; > +} APDevice; > + > +typedef struct APDeviceClass { > + DeviceClass parent_class; > +} APDeviceClass; > + > +static inline APDevice *to_ap_dev(DeviceState *dev) > +{ > + return container_of(dev, APDevice, parent_obj); > +} > + > +#define AP_DEVICE(obj) \ > + OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE) > + > +#define AP_DEVICE_GET_CLASS(obj) \ > + OBJECT_GET_CLASS(APDeviceClass, (obj), AP_DEVICE_TYPE) > + > +#define AP_DEVICE_CLASS(klass) \ > + OBJECT_CLASS_CHECK(APDeviceClass, (klass), AP_DEVICE_TYPE) > + > +#endif /* HW_S390X_AP_DEVICE_H */ > Appart of the current discussions on the implementation: Bus and devices appear correctly on the qtree. Tested-by: Pierre Morel<pmorel@linux.ibm.com>
On 09/27/2018 08:52 AM, Cornelia Huck wrote: > On Thu, 27 Sep 2018 14:29:01 +0200 > Thomas Huth <thuth@redhat.com> wrote: > >> On 2018-09-27 00:54, Tony Krowiak wrote: >>> From: Tony Krowiak <akrowiak@linux.ibm.com> >>> >>> Introduces the base object model for virtualizing AP devices. >>> >>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> >>> --- > >>> +typedef struct APBridge { >>> + SysBusDevice sysbus_dev; >>> + bool css_dev_path; >> >> What is this css_dev_path variable good for? I don't see it used in any >> of the other patches? >> If you don't need it, I think you could get rid of this struct completely? > > Huh, now I remember complaining about it before. Looks like a > copy-and-paste from the css bridge; that variable is used for compat > handling there (and should be ditched here). > >> >>> +} APBridge; >>> + >>> +#define TYPE_AP_BRIDGE "ap-bridge" >>> +#define AP_BRIDGE(obj) \ >>> + OBJECT_CHECK(APBridge, (obj), TYPE_AP_BRIDGE) >>> + >>> +typedef struct APBus { >>> + BusState parent_obj; >>> +} APBus; >>> + >>> +#define TYPE_AP_BUS "ap-bus" >>> +#define AP_BUS(obj) \ >>> + OBJECT_CHECK(APBus, (obj), TYPE_AP_BUS) >> >> I think you could also get rid of AP_BRIDGE(), AP_BUS() and maybe even >> struct APBus. > > If there's nothing interesting to put in these inherited structures, > probably yes. > >> >>> +void s390_init_ap(void); >>> + >>> +#endif >>> diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h >>> new file mode 100644 >>> index 000000000000..693df90cc041 >>> --- /dev/null >>> +++ b/include/hw/s390x/ap-device.h >>> @@ -0,0 +1,38 @@ >>> +/* >>> + * Adjunct Processor (AP) matrix device interfaces >>> + * >>> + * Copyright 2018 IBM Corp. >>> + * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com> >>> + * >>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at >>> + * your option) any later version. See the COPYING file in the top-level >>> + * directory. >>> + */ >>> +#ifndef HW_S390X_AP_DEVICE_H >>> +#define HW_S390X_AP_DEVICE_H >>> + >>> +#define AP_DEVICE_TYPE "ap-device" >>> + >>> +typedef struct APDevice { >>> + DeviceState parent_obj; >>> +} APDevice; >>> + >>> +typedef struct APDeviceClass { >>> + DeviceClass parent_class; >>> +} APDeviceClass; >>> + >>> +static inline APDevice *to_ap_dev(DeviceState *dev) >>> +{ >>> + return container_of(dev, APDevice, parent_obj); >>> +} >>> + >>> +#define AP_DEVICE(obj) \ >>> + OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE) >>> + >>> +#define AP_DEVICE_GET_CLASS(obj) \ >>> + OBJECT_GET_CLASS(APDeviceClass, (obj), AP_DEVICE_TYPE) >>> + >>> +#define AP_DEVICE_CLASS(klass) \ >>> + OBJECT_CLASS_CHECK(APDeviceClass, (klass), AP_DEVICE_TYPE) >> >> Do you really need any of these definitions except AP_DEVICE_TYPE ? Yes, we need AP_DEVICE(obj) and struct APDevice; they are both used in patch 5/6. We can probably get rid of AP_DEVICE_GET_CLASS(obj) and AP_DEVICE_CLASS(klass), but aren't those typically included in all QOM definitions? > > Same here, I think. >
On 08/10/2018 16:20, Tony Krowiak wrote: > On 09/27/2018 08:52 AM, Cornelia Huck wrote: >> On Thu, 27 Sep 2018 14:29:01 +0200 >> Thomas Huth <thuth@redhat.com> wrote: >> >>> On 2018-09-27 00:54, Tony Krowiak wrote: >>>> From: Tony Krowiak <akrowiak@linux.ibm.com> >>>> >>>> Introduces the base object model for virtualizing AP devices. >>>> >>>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> >>>> --- >> >>>> +typedef struct APBridge { >>>> + SysBusDevice sysbus_dev; >>>> + bool css_dev_path; >>> >>> What is this css_dev_path variable good for? I don't see it used in any >>> of the other patches? >>> If you don't need it, I think you could get rid of this struct completely? >> >> Huh, now I remember complaining about it before. Looks like a >> copy-and-paste from the css bridge; that variable is used for compat >> handling there (and should be ditched here). >> >>> >>>> +} APBridge; >>>> + >>>> +#define TYPE_AP_BRIDGE "ap-bridge" >>>> +#define AP_BRIDGE(obj) \ >>>> + OBJECT_CHECK(APBridge, (obj), TYPE_AP_BRIDGE) >>>> + >>>> +typedef struct APBus { >>>> + BusState parent_obj; >>>> +} APBus; >>>> + >>>> +#define TYPE_AP_BUS "ap-bus" >>>> +#define AP_BUS(obj) \ >>>> + OBJECT_CHECK(APBus, (obj), TYPE_AP_BUS) >>> >>> I think you could also get rid of AP_BRIDGE(), AP_BUS() and maybe even >>> struct APBus. >> >> If there's nothing interesting to put in these inherited structures, >> probably yes. >> >>> >>>> +void s390_init_ap(void); >>>> + >>>> +#endif >>>> diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h >>>> new file mode 100644 >>>> index 000000000000..693df90cc041 >>>> --- /dev/null >>>> +++ b/include/hw/s390x/ap-device.h >>>> @@ -0,0 +1,38 @@ >>>> +/* >>>> + * Adjunct Processor (AP) matrix device interfaces >>>> + * >>>> + * Copyright 2018 IBM Corp. >>>> + * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com> >>>> + * >>>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at >>>> + * your option) any later version. See the COPYING file in the top-level >>>> + * directory. >>>> + */ >>>> +#ifndef HW_S390X_AP_DEVICE_H >>>> +#define HW_S390X_AP_DEVICE_H >>>> + >>>> +#define AP_DEVICE_TYPE "ap-device" >>>> + >>>> +typedef struct APDevice { >>>> + DeviceState parent_obj; >>>> +} APDevice; >>>> + >>>> +typedef struct APDeviceClass { >>>> + DeviceClass parent_class; >>>> +} APDeviceClass; >>>> + >>>> +static inline APDevice *to_ap_dev(DeviceState *dev) >>>> +{ >>>> + return container_of(dev, APDevice, parent_obj); >>>> +} >>>> + >>>> +#define AP_DEVICE(obj) \ >>>> + OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE) >>>> + >>>> +#define AP_DEVICE_GET_CLASS(obj) \ >>>> + OBJECT_GET_CLASS(APDeviceClass, (obj), AP_DEVICE_TYPE) >>>> + >>>> +#define AP_DEVICE_CLASS(klass) \ >>>> + OBJECT_CLASS_CHECK(APDeviceClass, (klass), AP_DEVICE_TYPE) >>> >>> Do you really need any of these definitions except AP_DEVICE_TYPE ? > > Yes, we need AP_DEVICE(obj) and struct APDevice; they are both used in > patch 5/6. We can probably get rid of AP_DEVICE_GET_CLASS(obj) and > AP_DEVICE_CLASS(klass), but aren't those typically included in all > QOM definitions? Yes, we usually add all of them although only some might actually be used. (adding a new device usually looks like filling out a template)
On Mon, 8 Oct 2018 16:22:27 +0200 David Hildenbrand <david@redhat.com> wrote: > On 08/10/2018 16:20, Tony Krowiak wrote: > > On 09/27/2018 08:52 AM, Cornelia Huck wrote: > >> On Thu, 27 Sep 2018 14:29:01 +0200 > >> Thomas Huth <thuth@redhat.com> wrote: > >> > >>> On 2018-09-27 00:54, Tony Krowiak wrote: > >>>> From: Tony Krowiak <akrowiak@linux.ibm.com> > >>>> > >>>> Introduces the base object model for virtualizing AP devices. > >>>> > >>>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> > >>>> --- > >> > >>>> +typedef struct APBridge { > >>>> + SysBusDevice sysbus_dev; > >>>> + bool css_dev_path; > >>> > >>> What is this css_dev_path variable good for? I don't see it used in any > >>> of the other patches? > >>> If you don't need it, I think you could get rid of this struct completely? > >> > >> Huh, now I remember complaining about it before. Looks like a > >> copy-and-paste from the css bridge; that variable is used for compat > >> handling there (and should be ditched here). > >> > >>> > >>>> +} APBridge; > >>>> + > >>>> +#define TYPE_AP_BRIDGE "ap-bridge" > >>>> +#define AP_BRIDGE(obj) \ > >>>> + OBJECT_CHECK(APBridge, (obj), TYPE_AP_BRIDGE) > >>>> + > >>>> +typedef struct APBus { > >>>> + BusState parent_obj; > >>>> +} APBus; > >>>> + > >>>> +#define TYPE_AP_BUS "ap-bus" > >>>> +#define AP_BUS(obj) \ > >>>> + OBJECT_CHECK(APBus, (obj), TYPE_AP_BUS) > >>> > >>> I think you could also get rid of AP_BRIDGE(), AP_BUS() and maybe even > >>> struct APBus. > >> > >> If there's nothing interesting to put in these inherited structures, > >> probably yes. > >> > >>> > >>>> +void s390_init_ap(void); > >>>> + > >>>> +#endif > >>>> diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h > >>>> new file mode 100644 > >>>> index 000000000000..693df90cc041 > >>>> --- /dev/null > >>>> +++ b/include/hw/s390x/ap-device.h > >>>> @@ -0,0 +1,38 @@ > >>>> +/* > >>>> + * Adjunct Processor (AP) matrix device interfaces > >>>> + * > >>>> + * Copyright 2018 IBM Corp. > >>>> + * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com> > >>>> + * > >>>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at > >>>> + * your option) any later version. See the COPYING file in the top-level > >>>> + * directory. > >>>> + */ > >>>> +#ifndef HW_S390X_AP_DEVICE_H > >>>> +#define HW_S390X_AP_DEVICE_H > >>>> + > >>>> +#define AP_DEVICE_TYPE "ap-device" > >>>> + > >>>> +typedef struct APDevice { > >>>> + DeviceState parent_obj; > >>>> +} APDevice; > >>>> + > >>>> +typedef struct APDeviceClass { > >>>> + DeviceClass parent_class; > >>>> +} APDeviceClass; > >>>> + > >>>> +static inline APDevice *to_ap_dev(DeviceState *dev) > >>>> +{ > >>>> + return container_of(dev, APDevice, parent_obj); > >>>> +} > >>>> + > >>>> +#define AP_DEVICE(obj) \ > >>>> + OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE) > >>>> + > >>>> +#define AP_DEVICE_GET_CLASS(obj) \ > >>>> + OBJECT_GET_CLASS(APDeviceClass, (obj), AP_DEVICE_TYPE) > >>>> + > >>>> +#define AP_DEVICE_CLASS(klass) \ > >>>> + OBJECT_CLASS_CHECK(APDeviceClass, (klass), AP_DEVICE_TYPE) > >>> > >>> Do you really need any of these definitions except AP_DEVICE_TYPE ? > > > > Yes, we need AP_DEVICE(obj) and struct APDevice; they are both used in > > patch 5/6. We can probably get rid of AP_DEVICE_GET_CLASS(obj) and > > AP_DEVICE_CLASS(klass), but aren't those typically included in all > > QOM definitions? > > Yes, we usually add all of them although only some might actually be > used. (adding a new device usually looks like filling out a template) Much of this seems to be boilerplate in this case, and I'm not sure how much sense it makes. On the plus side, however, it looks like everything else :) So, I would merge both a complete version or a stripped-down-to-the-needed version, unless someone else has a strong argument.
On 2018-10-08 16:20, Tony Krowiak wrote: > On 09/27/2018 08:52 AM, Cornelia Huck wrote: >> On Thu, 27 Sep 2018 14:29:01 +0200 >> Thomas Huth <thuth@redhat.com> wrote: >> >>> On 2018-09-27 00:54, Tony Krowiak wrote: >>>> From: Tony Krowiak <akrowiak@linux.ibm.com> >>>> >>>> Introduces the base object model for virtualizing AP devices. >>>> >>>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> >>>> --- >> >>>> +typedef struct APBridge { >>>> + SysBusDevice sysbus_dev; >>>> + bool css_dev_path; >>> >>> What is this css_dev_path variable good for? I don't see it used in any >>> of the other patches? >>> If you don't need it, I think you could get rid of this struct >>> completely? >> >> Huh, now I remember complaining about it before. Looks like a >> copy-and-paste from the css bridge; that variable is used for compat >> handling there (and should be ditched here). >> >>> >>>> +} APBridge; >>>> + >>>> +#define TYPE_AP_BRIDGE "ap-bridge" >>>> +#define AP_BRIDGE(obj) \ >>>> + OBJECT_CHECK(APBridge, (obj), TYPE_AP_BRIDGE) >>>> + >>>> +typedef struct APBus { >>>> + BusState parent_obj; >>>> +} APBus; >>>> + >>>> +#define TYPE_AP_BUS "ap-bus" >>>> +#define AP_BUS(obj) \ >>>> + OBJECT_CHECK(APBus, (obj), TYPE_AP_BUS) >>> >>> I think you could also get rid of AP_BRIDGE(), AP_BUS() and maybe even >>> struct APBus. >> >> If there's nothing interesting to put in these inherited structures, >> probably yes. >> >>> >>>> +void s390_init_ap(void); >>>> + >>>> +#endif >>>> diff --git a/include/hw/s390x/ap-device.h >>>> b/include/hw/s390x/ap-device.h >>>> new file mode 100644 >>>> index 000000000000..693df90cc041 >>>> --- /dev/null >>>> +++ b/include/hw/s390x/ap-device.h >>>> @@ -0,0 +1,38 @@ >>>> +/* >>>> + * Adjunct Processor (AP) matrix device interfaces >>>> + * >>>> + * Copyright 2018 IBM Corp. >>>> + * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com> >>>> + * >>>> + * This work is licensed under the terms of the GNU GPL, version 2 >>>> or (at >>>> + * your option) any later version. See the COPYING file in the >>>> top-level >>>> + * directory. >>>> + */ >>>> +#ifndef HW_S390X_AP_DEVICE_H >>>> +#define HW_S390X_AP_DEVICE_H >>>> + >>>> +#define AP_DEVICE_TYPE "ap-device" >>>> + >>>> +typedef struct APDevice { >>>> + DeviceState parent_obj; >>>> +} APDevice; >>>> + >>>> +typedef struct APDeviceClass { >>>> + DeviceClass parent_class; >>>> +} APDeviceClass; >>>> + >>>> +static inline APDevice *to_ap_dev(DeviceState *dev) >>>> +{ >>>> + return container_of(dev, APDevice, parent_obj); >>>> +} >>>> + >>>> +#define AP_DEVICE(obj) \ >>>> + OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE) >>>> + >>>> +#define AP_DEVICE_GET_CLASS(obj) \ >>>> + OBJECT_GET_CLASS(APDeviceClass, (obj), AP_DEVICE_TYPE) >>>> + >>>> +#define AP_DEVICE_CLASS(klass) \ >>>> + OBJECT_CLASS_CHECK(APDeviceClass, (klass), AP_DEVICE_TYPE) >>> >>> Do you really need any of these definitions except AP_DEVICE_TYPE ? > > Yes, we need AP_DEVICE(obj) and struct APDevice; they are both used in > patch 5/6. Fine for me, if you replace the DO_UPCAST in patch 5 with AP_DEVICE(). > We can probably get rid of AP_DEVICE_GET_CLASS(obj) and > AP_DEVICE_CLASS(klass), but aren't those typically included in all > QOM definitions? As long as you don't really need them, I'd simply remove them. They can be added back when some code really needs them. Thomas
On 10/08/2018 10:35 AM, Cornelia Huck wrote: > On Mon, 8 Oct 2018 16:22:27 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> On 08/10/2018 16:20, Tony Krowiak wrote: >>> On 09/27/2018 08:52 AM, Cornelia Huck wrote: >>>> On Thu, 27 Sep 2018 14:29:01 +0200 >>>> Thomas Huth <thuth@redhat.com> wrote: >>>> >>>>> On 2018-09-27 00:54, Tony Krowiak wrote: >>>>>> From: Tony Krowiak <akrowiak@linux.ibm.com> >>>>>> >>>>>> Introduces the base object model for virtualizing AP devices. >>>>>> >>>>>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> >>>>>> --- >>>> >>>>>> +typedef struct APBridge { >>>>>> + SysBusDevice sysbus_dev; >>>>>> + bool css_dev_path; >>>>> >>>>> What is this css_dev_path variable good for? I don't see it used in any >>>>> of the other patches? >>>>> If you don't need it, I think you could get rid of this struct completely? >>>> >>>> Huh, now I remember complaining about it before. Looks like a >>>> copy-and-paste from the css bridge; that variable is used for compat >>>> handling there (and should be ditched here). >>>> >>>>> >>>>>> +} APBridge; >>>>>> + >>>>>> +#define TYPE_AP_BRIDGE "ap-bridge" >>>>>> +#define AP_BRIDGE(obj) \ >>>>>> + OBJECT_CHECK(APBridge, (obj), TYPE_AP_BRIDGE) >>>>>> + >>>>>> +typedef struct APBus { >>>>>> + BusState parent_obj; >>>>>> +} APBus; >>>>>> + >>>>>> +#define TYPE_AP_BUS "ap-bus" >>>>>> +#define AP_BUS(obj) \ >>>>>> + OBJECT_CHECK(APBus, (obj), TYPE_AP_BUS) >>>>> >>>>> I think you could also get rid of AP_BRIDGE(), AP_BUS() and maybe even >>>>> struct APBus. >>>> >>>> If there's nothing interesting to put in these inherited structures, >>>> probably yes. >>>> >>>>> >>>>>> +void s390_init_ap(void); >>>>>> + >>>>>> +#endif >>>>>> diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h >>>>>> new file mode 100644 >>>>>> index 000000000000..693df90cc041 >>>>>> --- /dev/null >>>>>> +++ b/include/hw/s390x/ap-device.h >>>>>> @@ -0,0 +1,38 @@ >>>>>> +/* >>>>>> + * Adjunct Processor (AP) matrix device interfaces >>>>>> + * >>>>>> + * Copyright 2018 IBM Corp. >>>>>> + * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com> >>>>>> + * >>>>>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at >>>>>> + * your option) any later version. See the COPYING file in the top-level >>>>>> + * directory. >>>>>> + */ >>>>>> +#ifndef HW_S390X_AP_DEVICE_H >>>>>> +#define HW_S390X_AP_DEVICE_H >>>>>> + >>>>>> +#define AP_DEVICE_TYPE "ap-device" >>>>>> + >>>>>> +typedef struct APDevice { >>>>>> + DeviceState parent_obj; >>>>>> +} APDevice; >>>>>> + >>>>>> +typedef struct APDeviceClass { >>>>>> + DeviceClass parent_class; >>>>>> +} APDeviceClass; >>>>>> + >>>>>> +static inline APDevice *to_ap_dev(DeviceState *dev) >>>>>> +{ >>>>>> + return container_of(dev, APDevice, parent_obj); >>>>>> +} >>>>>> + >>>>>> +#define AP_DEVICE(obj) \ >>>>>> + OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE) >>>>>> + >>>>>> +#define AP_DEVICE_GET_CLASS(obj) \ >>>>>> + OBJECT_GET_CLASS(APDeviceClass, (obj), AP_DEVICE_TYPE) >>>>>> + >>>>>> +#define AP_DEVICE_CLASS(klass) \ >>>>>> + OBJECT_CLASS_CHECK(APDeviceClass, (klass), AP_DEVICE_TYPE) >>>>> >>>>> Do you really need any of these definitions except AP_DEVICE_TYPE ? >>> >>> Yes, we need AP_DEVICE(obj) and struct APDevice; they are both used in >>> patch 5/6. We can probably get rid of AP_DEVICE_GET_CLASS(obj) and >>> AP_DEVICE_CLASS(klass), but aren't those typically included in all >>> QOM definitions? >> >> Yes, we usually add all of them although only some might actually be >> used. (adding a new device usually looks like filling out a template) > > Much of this seems to be boilerplate in this case, and I'm not sure how > much sense it makes. On the plus side, however, it looks like > everything else :) > > So, I would merge both a complete version or a > stripped-down-to-the-needed version, unless someone else has a strong > argument. The 'I would merge both' implies you are asking for two versions, but the 'or' implies you are asking for one or the other; I'm going to assume you are asking for one or the other. I'll provide a stripped down version in v10 which I am planning on posting today. >
On 10/08/2018 10:44 AM, Thomas Huth wrote: > On 2018-10-08 16:20, Tony Krowiak wrote: >> On 09/27/2018 08:52 AM, Cornelia Huck wrote: >>> On Thu, 27 Sep 2018 14:29:01 +0200 >>> Thomas Huth <thuth@redhat.com> wrote: >>> >>>> On 2018-09-27 00:54, Tony Krowiak wrote: >>>>> From: Tony Krowiak <akrowiak@linux.ibm.com> >>>>> >>>>> Introduces the base object model for virtualizing AP devices. >>>>> >>>>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> >>>>> --- >>> >>>>> +typedef struct APBridge { >>>>> + SysBusDevice sysbus_dev; >>>>> + bool css_dev_path; >>>> >>>> What is this css_dev_path variable good for? I don't see it used in any >>>> of the other patches? >>>> If you don't need it, I think you could get rid of this struct >>>> completely? >>> >>> Huh, now I remember complaining about it before. Looks like a >>> copy-and-paste from the css bridge; that variable is used for compat >>> handling there (and should be ditched here). >>> >>>> >>>>> +} APBridge; >>>>> + >>>>> +#define TYPE_AP_BRIDGE "ap-bridge" >>>>> +#define AP_BRIDGE(obj) \ >>>>> + OBJECT_CHECK(APBridge, (obj), TYPE_AP_BRIDGE) >>>>> + >>>>> +typedef struct APBus { >>>>> + BusState parent_obj; >>>>> +} APBus; >>>>> + >>>>> +#define TYPE_AP_BUS "ap-bus" >>>>> +#define AP_BUS(obj) \ >>>>> + OBJECT_CHECK(APBus, (obj), TYPE_AP_BUS) >>>> >>>> I think you could also get rid of AP_BRIDGE(), AP_BUS() and maybe even >>>> struct APBus. >>> >>> If there's nothing interesting to put in these inherited structures, >>> probably yes. >>> >>>> >>>>> +void s390_init_ap(void); >>>>> + >>>>> +#endif >>>>> diff --git a/include/hw/s390x/ap-device.h >>>>> b/include/hw/s390x/ap-device.h >>>>> new file mode 100644 >>>>> index 000000000000..693df90cc041 >>>>> --- /dev/null >>>>> +++ b/include/hw/s390x/ap-device.h >>>>> @@ -0,0 +1,38 @@ >>>>> +/* >>>>> + * Adjunct Processor (AP) matrix device interfaces >>>>> + * >>>>> + * Copyright 2018 IBM Corp. >>>>> + * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com> >>>>> + * >>>>> + * This work is licensed under the terms of the GNU GPL, version 2 >>>>> or (at >>>>> + * your option) any later version. See the COPYING file in the >>>>> top-level >>>>> + * directory. >>>>> + */ >>>>> +#ifndef HW_S390X_AP_DEVICE_H >>>>> +#define HW_S390X_AP_DEVICE_H >>>>> + >>>>> +#define AP_DEVICE_TYPE "ap-device" >>>>> + >>>>> +typedef struct APDevice { >>>>> + DeviceState parent_obj; >>>>> +} APDevice; >>>>> + >>>>> +typedef struct APDeviceClass { >>>>> + DeviceClass parent_class; >>>>> +} APDeviceClass; >>>>> + >>>>> +static inline APDevice *to_ap_dev(DeviceState *dev) >>>>> +{ >>>>> + return container_of(dev, APDevice, parent_obj); >>>>> +} >>>>> + >>>>> +#define AP_DEVICE(obj) \ >>>>> + OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE) >>>>> + >>>>> +#define AP_DEVICE_GET_CLASS(obj) \ >>>>> + OBJECT_GET_CLASS(APDeviceClass, (obj), AP_DEVICE_TYPE) >>>>> + >>>>> +#define AP_DEVICE_CLASS(klass) \ >>>>> + OBJECT_CLASS_CHECK(APDeviceClass, (klass), AP_DEVICE_TYPE) >>>> >>>> Do you really need any of these definitions except AP_DEVICE_TYPE ? >> >> Yes, we need AP_DEVICE(obj) and struct APDevice; they are both used in >> patch 5/6. > > Fine for me, if you replace the DO_UPCAST in patch 5 with AP_DEVICE(). > >> We can probably get rid of AP_DEVICE_GET_CLASS(obj) and >> AP_DEVICE_CLASS(klass), but aren't those typically included in all >> QOM definitions? > > As long as you don't really need them, I'd simply remove them. They can > be added back when some code really needs them. That is the plan > > Thomas >
diff --git a/MAINTAINERS b/MAINTAINERS index d12518c08f10..97e8ed808bc0 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1199,6 +1199,18 @@ F: include/hw/s390x/s390-ccw.h T: git git://github.com/cohuck/qemu.git s390-next L: qemu-s390x@nongnu.org +vfio-ap +M: Christian Borntraeger <borntraeger@de.ibm.com> +M: Tony Krowiak <akrowiak@linux.ibm.com> +M: Halil Pasic <pasic@linux.ibm.com> +M: Pierre Morel <pmorel@linux.ibm.com> +S: Supported +F: hw/s390x/ap-device.c +F: hw/s390x/ap-bridge.c +F: include/hw/s390x/ap-device.h +F: include/hw/s390x/ap-bridge.h +L: qemu-s390x@nongnu.org + vhost M: Michael S. Tsirkin <mst@redhat.com> S: Supported diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs index 93282f7c593c..add89b150d90 100644 --- a/hw/s390x/Makefile.objs +++ b/hw/s390x/Makefile.objs @@ -20,3 +20,5 @@ obj-$(CONFIG_TCG) += tod-qemu.o obj-$(CONFIG_KVM) += s390-skeys-kvm.o obj-$(CONFIG_KVM) += s390-stattrib-kvm.o obj-y += s390-ccw.o +obj-y += ap-device.o +obj-y += ap-bridge.o diff --git a/hw/s390x/ap-bridge.c b/hw/s390x/ap-bridge.c new file mode 100644 index 000000000000..8564dfa96ee7 --- /dev/null +++ b/hw/s390x/ap-bridge.c @@ -0,0 +1,81 @@ +/* + * ap bridge + * + * Copyright 2018 IBM Corp. + * Author(s): Halil Pasic <pasic@linux.ibm.com> + * + * This work is licensed under the terms of the GNU GPL, version 2 or (at + * your option) any later version. See the COPYING file in the top-level + * directory. + */ +#include "qemu/osdep.h" +#include "qapi/error.h" +#include "hw/sysbus.h" +#include "qemu/bitops.h" +#include "hw/s390x/ap-bridge.h" +#include "cpu.h" + +static char *vfio_ap_bus_get_dev_path(DeviceState *dev) +{ + /* at most one */ + return g_strdup_printf("/1"); +} + +static void vfio_ap_bus_class_init(ObjectClass *klass, void *data) +{ + BusClass *k = BUS_CLASS(klass); + + k->get_dev_path = vfio_ap_bus_get_dev_path; + /* More than one vfio-ap device does not make sense */ + k->max_dev = 1; +} + +static const TypeInfo vfio_ap_bus_info = { + .name = TYPE_AP_BUS, + .parent = TYPE_BUS, + .instance_size = sizeof(APBus), + .class_init = vfio_ap_bus_class_init, +}; + +void s390_init_ap(void) +{ + DeviceState *dev; + + /* If no AP instructions then no need for AP bridge */ + if (!s390_has_feat(S390_FEAT_AP)) { + return; + } + + /* Create bridge device */ + dev = qdev_create(NULL, TYPE_AP_BRIDGE); + object_property_add_child(qdev_get_machine(), TYPE_AP_BRIDGE, + OBJECT(dev), NULL); + qdev_init_nofail(dev); + + /* Create bus on bridge device */ + qbus_create(TYPE_AP_BUS, dev, TYPE_AP_BUS); + } + + + +static void ap_bridge_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + + set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); +} + +static const TypeInfo ap_bridge_info = { + .name = TYPE_AP_BRIDGE, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(APBridge), + .class_init = ap_bridge_class_init, +}; + +static void ap_register(void) +{ + type_register_static(&ap_bridge_info); + type_register_static(&vfio_ap_bus_info); +} + +type_init(ap_register) diff --git a/hw/s390x/ap-device.c b/hw/s390x/ap-device.c new file mode 100644 index 000000000000..3cd4bae52591 --- /dev/null +++ b/hw/s390x/ap-device.c @@ -0,0 +1,39 @@ +/* + * Adjunct Processor (AP) matrix device + * + * Copyright 2018 IBM Corp. + * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com> + * + * This work is licensed under the terms of the GNU GPL, version 2 or (at + * your option) any later version. See the COPYING file in the top-level + * directory. + */ +#include "qemu/osdep.h" +#include "qemu/module.h" +#include "qapi/error.h" +#include "hw/qdev.h" +#include "hw/s390x/ap-device.h" + +static void ap_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + + dc->desc = "AP device class"; + dc->hotpluggable = false; +} + +static const TypeInfo ap_device_info = { + .name = AP_DEVICE_TYPE, + .parent = TYPE_DEVICE, + .instance_size = sizeof(APDevice), + .class_size = sizeof(APDeviceClass), + .class_init = ap_class_init, + .abstract = true, +}; + +static void ap_device_register(void) +{ + type_register_static(&ap_device_info); +} + +type_init(ap_device_register) diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index f0f7fdcaddf2..3c100c24f3e8 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -32,6 +32,7 @@ #include "ipl.h" #include "hw/s390x/s390-virtio-ccw.h" #include "hw/s390x/css-bridge.h" +#include "hw/s390x/ap-bridge.h" #include "migration/register.h" #include "cpu_models.h" #include "hw/nmi.h" @@ -263,6 +264,9 @@ static void ccw_init(MachineState *machine) /* init the SIGP facility */ s390_init_sigp(); + /* create AP bridge and bus(es) */ + s390_init_ap(); + /* get a BUS */ css_bus = virtual_css_bus_init(); s390_init_ipl_dev(machine->kernel_filename, machine->kernel_cmdline, diff --git a/include/hw/s390x/ap-bridge.h b/include/hw/s390x/ap-bridge.h new file mode 100644 index 000000000000..b6ca6ae4ab17 --- /dev/null +++ b/include/hw/s390x/ap-bridge.h @@ -0,0 +1,37 @@ +/* + * ap bridge + * + * Copyright 2018 IBM Corp. + * Author(s): Halil Pasic <pasic@linux.ibm.com> + * + * This work is licensed under the terms of the GNU GPL, version 2 or (at + * your option) any later version. See the COPYING file in the top-level + * directory. + */ + +#ifndef HW_S390X_AP_BRIDGE_H +#define HW_S390X_AP_BRIDGE_H +#include "qom/object.h" +#include "hw/qdev-core.h" +#include "hw/sysbus.h" + +typedef struct APBridge { + SysBusDevice sysbus_dev; + bool css_dev_path; +} APBridge; + +#define TYPE_AP_BRIDGE "ap-bridge" +#define AP_BRIDGE(obj) \ + OBJECT_CHECK(APBridge, (obj), TYPE_AP_BRIDGE) + +typedef struct APBus { + BusState parent_obj; +} APBus; + +#define TYPE_AP_BUS "ap-bus" +#define AP_BUS(obj) \ + OBJECT_CHECK(APBus, (obj), TYPE_AP_BUS) + +void s390_init_ap(void); + +#endif diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h new file mode 100644 index 000000000000..693df90cc041 --- /dev/null +++ b/include/hw/s390x/ap-device.h @@ -0,0 +1,38 @@ +/* + * Adjunct Processor (AP) matrix device interfaces + * + * Copyright 2018 IBM Corp. + * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com> + * + * This work is licensed under the terms of the GNU GPL, version 2 or (at + * your option) any later version. See the COPYING file in the top-level + * directory. + */ +#ifndef HW_S390X_AP_DEVICE_H +#define HW_S390X_AP_DEVICE_H + +#define AP_DEVICE_TYPE "ap-device" + +typedef struct APDevice { + DeviceState parent_obj; +} APDevice; + +typedef struct APDeviceClass { + DeviceClass parent_class; +} APDeviceClass; + +static inline APDevice *to_ap_dev(DeviceState *dev) +{ + return container_of(dev, APDevice, parent_obj); +} + +#define AP_DEVICE(obj) \ + OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE) + +#define AP_DEVICE_GET_CLASS(obj) \ + OBJECT_GET_CLASS(APDeviceClass, (obj), AP_DEVICE_TYPE) + +#define AP_DEVICE_CLASS(klass) \ + OBJECT_CLASS_CHECK(APDeviceClass, (klass), AP_DEVICE_TYPE) + +#endif /* HW_S390X_AP_DEVICE_H */