On 09/27/2018 08:29 AM, Thomas Huth wrote:
On 2018-09-27 00:54, Tony Krowiak wrote:From: Tony Krowiak <akrow...@linux.ibm.com>Introduces the base object model for virtualizing AP devices. Signed-off-by: Tony Krowiak <akrow...@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 <pa...@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 <pa...@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 <akrow...@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
