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

[Halil Pasic]

On 09/27/2018 02:52 PM, Cornelia Huck wrote:
> 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).
>

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 <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.
> 

I don't think the qom utility/casting macros will ever be used. I'm
fine with removing what is not needed.

Regards,
Halil