Re: [Qemu-devel] [PATCH v1 1/3] ahci: Separate the AHCI state structure into the header

[Peter Crosthwaite]

On Wed, Jul 29, 2015 at 5:12 PM, Alistair Francis
<address@hidden> wrote:
> On Wed, Jul 29, 2015 at 3:21 PM, John Snow <address@hidden> wrote:
>>
>>
>> On 07/27/2015 02:37 PM, Alistair Francis wrote:
>>> Pull the AHCI state structure out into the header. This allows
>>> other containers to access the struct. This is required to add
>>> the device to modern SoC containers.
>>>
>>
>> Is there a reason this structure needs to be directly inlined into
>> XlnxZynqMPState, instead of using e.g.
>> qdev_create/object_property_add_child and letting SysbusAHCIState be the
>> business of ahci.c?
>

This has come up a few times, and the conclusion was to do the
inlining by preference. I vaguely remember PMM started an effort to
build infrastructure to hide the internals from use by outsiders, but
the struct def is desirable for the inlining even if the internals are
private a different module. It has been the policy on ARM SoC for
quite some time. We have made a quite a few of these conversions for
an example of a big one, see:

[PATCH v4 00/24] arm: ARM11MPCore+A9MPCore+A15MPCore QOM'ification

> AFAIK there isn't a huge advantage, it ends up achieving basically the
> same thing. It just means that we don't need to use
> the old qdev_* functions (as much). It also follows with QOMificatiom and
> the device creation can be split up over the init and realise functions.
>

You can use object_new in place of object_initialize to achieve the
split without inlining as well.

Regards,
Peter

> This is also the way I did machine creation for the Netduino 2 and how
> it is done for the rest of the ZynqMP device.
>
> Thanks,
>
> Alistair
>
>>
>> Asking as a bit of a qdev outsider.
>>
>> --js
>>
>>> Signed-off-by: Alistair Francis <address@hidden>
>>> Reviewed-by: Sai Pavan Boddu <address@hidden>
>>> ---
>>>  hw/ide/ahci.c |   13 -------------
>>>  hw/ide/ahci.h |   14 ++++++++++++++
>>>  2 files changed, 14 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>>> index 48749c1..02d85fa 100644
>>> --- a/hw/ide/ahci.c
>>> +++ b/hw/ide/ahci.c
>>> @@ -25,7 +25,6 @@
>>>  #include <hw/pci/msi.h>
>>>  #include <hw/i386/pc.h>
>>>  #include <hw/pci/pci.h>
>>> -#include <hw/sysbus.h>
>>>
>>>  #include "qemu/error-report.h"
>>>  #include "sysemu/block-backend.h"
>>> @@ -1625,18 +1624,6 @@ const VMStateDescription vmstate_ahci = {
>>>      },
>>>  };
>>>
>>> -#define TYPE_SYSBUS_AHCI "sysbus-ahci"
>>> -#define SYSBUS_AHCI(obj) OBJECT_CHECK(SysbusAHCIState, (obj), 
>>> TYPE_SYSBUS_AHCI)
>>> -
>>> -typedef struct SysbusAHCIState {
>>> -    /*< private >*/
>>> -    SysBusDevice parent_obj;
>>> -    /*< public >*/
>>> -
>>> -    AHCIState ahci;
>>> -    uint32_t num_ports;
>>> -} SysbusAHCIState;
>>> -
>>>  static const VMStateDescription vmstate_sysbus_ahci = {
>>>      .name = "sysbus-ahci",
>>>      .fields = (VMStateField[]) {
>>> diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
>>> index 68d5074..5ab8ea4 100644
>>> --- a/hw/ide/ahci.h
>>> +++ b/hw/ide/ahci.h
>>> @@ -24,6 +24,8 @@
>>>  #ifndef HW_IDE_AHCI_H
>>>  #define HW_IDE_AHCI_H
>>>
>>> +#include <hw/sysbus.h>
>>> +
>>>  #define AHCI_MEM_BAR_SIZE         0x1000
>>>  #define AHCI_MAX_PORTS            32
>>>  #define AHCI_MAX_SG               168 /* hardware max is 64K */
>>> @@ -369,4 +371,16 @@ void ahci_reset(AHCIState *s);
>>>
>>>  void ahci_ide_create_devs(PCIDevice *dev, DriveInfo **hd);
>>>
>>> +#define TYPE_SYSBUS_AHCI "sysbus-ahci"
>>> +#define SYSBUS_AHCI(obj) OBJECT_CHECK(SysbusAHCIState, (obj), 
>>> TYPE_SYSBUS_AHCI)
>>> +
>>> +typedef struct SysbusAHCIState {
>>> +    /*< private >*/
>>> +    SysBusDevice parent_obj;
>>> +    /*< public >*/
>>> +
>>> +    AHCIState ahci;
>>> +    uint32_t num_ports;
>>> +} SysbusAHCIState;
>>> +
>>>  #endif /* HW_IDE_AHCI_H */
>>>
>>
>