Re: [Qemu-devel] [qemu-s390x] [PATCH v9 4/6] s390x/ap: base Adjunct Processor (AP) object model

[Tony Krowiak]

On 09/27/2018 08:29 AM, Thomas Huth wrote:
> On 2018-09-27 00:54, Tony Krowiak wrote:
> > From: Tony Krowiak <address@hidden>
> >
> > Introduces the base object model for virtualizing AP devices.
> >
> > Signed-off-by: Tony Krowiak <address@hidden>
> > ---
> [...]
> > 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 <address@hidden>
> > + *
> > + * 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 <address@hidden>
> > + *
> > + * 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 <address@hidden>
> > + *
> > + * 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