qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [qemu-s390x] [PATCH v9 4/6] s390x/ap: base Adjunct Proc


From: Cornelia Huck
Subject: Re: [Qemu-devel] [qemu-s390x] [PATCH v9 4/6] s390x/ap: base Adjunct Processor (AP) object model
Date: Mon, 8 Oct 2018 16:35:03 +0200

On Mon, 8 Oct 2018 16:22:27 +0200
David Hildenbrand <address@hidden> wrote:

> On 08/10/2018 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 <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 ?  
> > 
> > Yes, we need AP_DEVICE(obj) and struct APDevice; they are both used in
> > patch 5/6. 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?  
> 
> Yes, we usually add all of them although only some might actually be
> used. (adding a new device usually looks like filling out a template)

Much of this seems to be boilerplate in this case, and I'm not sure how
much sense it makes. On the plus side, however, it looks like
everything else :)

So, I would merge both a complete version or a
stripped-down-to-the-needed version, unless someone else has a strong
argument.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]