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

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

On 10/08/2018 10:44 AM, Thomas Huth wrote:
> On 2018-10-08 16:20, Tony Krowiak wrote:
>> On 09/27/2018 08:52 AM, 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).
>>>
>>>>
>>>>> +} 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 <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 ?
>>
>> Yes, we need AP_DEVICE(obj) and struct APDevice; they are both used in
>> patch 5/6.
> 
> Fine for me, if you replace the DO_UPCAST in patch 5 with AP_DEVICE().
> 
>> We can probably get rid of AP_DEVICE_GET_CLASS(obj) and
>> AP_DEVICE_CLASS(klass), but aren't those typically included in all
>> QOM definitions?
> 
> As long as you don't really need them, I'd simply remove them. They can
> be added back when some code really needs them.

That is the plan

> 
>   Thomas
>