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

[Cornelia Huck]

On Thu, 27 Sep 2018 14:29:01 +0200
Thomas Huth <address@hidden> 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>
> > ---  

> > +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 <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 ?

Same here, I think.