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

[Tony Krowiak <akrowiak@linux.ibm.com>]

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.

> 
>   
>