Patchwork [v2,08/30] ide/ich: QOM Upcast Sweep

login
register
mail settings
Submitter Peter Crosthwaite
Date June 24, 2013, 6:55 a.m.
Message ID <e2526694ced29042c7202d54f345d6a1229dcb09.1372055322.git.peter.crosthwaite@xilinx.com>
Download mbox | patch
Permalink /patch/253666/
State New
Headers show

Comments

Peter Crosthwaite - June 24, 2013, 6:55 a.m.
From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Define and use standard QOM cast macro. Remove usages of DO_UPCAST
and direct -> style upcasting.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/ide/ahci.h |  5 +++++
 hw/ide/ich.c  | 10 +++++-----
 2 files changed, 10 insertions(+), 5 deletions(-)
Andreas Färber - June 30, 2013, 8:21 a.m.
Am 24.06.2013 08:55, schrieb peter.crosthwaite@xilinx.com:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> 
> Define and use standard QOM cast macro. Remove usages of DO_UPCAST
> and direct -> style upcasting.
> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> 
>  hw/ide/ahci.h |  5 +++++
>  hw/ide/ich.c  | 10 +++++-----
>  2 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
> index 341a571..916bef0 100644
> --- a/hw/ide/ahci.h
> +++ b/hw/ide/ahci.h
> @@ -305,6 +305,11 @@ typedef struct AHCIPCIState {
>      AHCIState ahci;
>  } AHCIPCIState;
>  
> +#define TYPE_ICH_AHCI "ich9-ahci"

Let's be as precise as for the LSI SCSI HBA and name this ICH9. :)

> +
> +#define ICH_AHCI(obj) \
> +    OBJECT_CHECK(AHCIPCIState, (obj), TYPE_ICH_AHCI)

Wondering if this is specific to ICH(9)? Alex?
Leaving it as is for now, renaming is an easy follow-up.

> +
>  extern const VMStateDescription vmstate_ahci;
>  
>  #define VMSTATE_AHCI(_field, _state) {                               \
> diff --git a/hw/ide/ich.c b/hw/ide/ich.c
> index 6c0c0c2..c3cbf2a 100644
> --- a/hw/ide/ich.c
> +++ b/hw/ide/ich.c
> @@ -92,7 +92,7 @@ static const VMStateDescription vmstate_ich9_ahci = {
>  
>  static void pci_ich9_reset(DeviceState *dev)
>  {
> -    struct AHCIPCIState *d = DO_UPCAST(struct AHCIPCIState, card.qdev, dev);
> +    struct AHCIPCIState *d = ICH_AHCI(dev);

Let's drop the "struct" while touching the line.

Thanks, applied to qom-next:
https://github.com/afaerber/qemu-cpu/commits/qom-next

Andreas

>  
>      ahci_reset(&d->ahci);
>  }
> @@ -102,9 +102,9 @@ static int pci_ich9_ahci_init(PCIDevice *dev)
>      struct AHCIPCIState *d;
>      int sata_cap_offset;
>      uint8_t *sata_cap;
> -    d = DO_UPCAST(struct AHCIPCIState, card, dev);
> +    d = ICH_AHCI(dev);
>  
> -    ahci_init(&d->ahci, &dev->qdev, pci_get_address_space(dev), 6);
> +    ahci_init(&d->ahci, DEVICE(dev), pci_get_address_space(dev), 6);
>  
>      pci_config_set_prog_interface(d->card.config, AHCI_PROGMODE_MAJOR_REV_1);
>  
> @@ -141,7 +141,7 @@ static int pci_ich9_ahci_init(PCIDevice *dev)
>  static void pci_ich9_uninit(PCIDevice *dev)
>  {
>      struct AHCIPCIState *d;
> -    d = DO_UPCAST(struct AHCIPCIState, card, dev);
> +    d = ICH_AHCI(dev);
>  
>      msi_uninit(dev);
>      ahci_uninit(&d->ahci);
> @@ -163,7 +163,7 @@ static void ich_ahci_class_init(ObjectClass *klass, void *data)
>  }
>  
>  static const TypeInfo ich_ahci_info = {
> -    .name          = "ich9-ahci",
> +    .name          = TYPE_ICH_AHCI,
>      .parent        = TYPE_PCI_DEVICE,
>      .instance_size = sizeof(AHCIPCIState),
>      .class_init    = ich_ahci_class_init,
>
Alexander Graf - June 30, 2013, 11:36 p.m.
On 30.06.2013, at 10:21, Andreas Färber wrote:

> Am 24.06.2013 08:55, schrieb peter.crosthwaite@xilinx.com:
>> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> 
>> Define and use standard QOM cast macro. Remove usages of DO_UPCAST
>> and direct -> style upcasting.
>> 
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>> 
>> hw/ide/ahci.h |  5 +++++
>> hw/ide/ich.c  | 10 +++++-----
>> 2 files changed, 10 insertions(+), 5 deletions(-)
>> 
>> diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
>> index 341a571..916bef0 100644
>> --- a/hw/ide/ahci.h
>> +++ b/hw/ide/ahci.h
>> @@ -305,6 +305,11 @@ typedef struct AHCIPCIState {
>>     AHCIState ahci;
>> } AHCIPCIState;
>> 
>> +#define TYPE_ICH_AHCI "ich9-ahci"
> 
> Let's be as precise as for the LSI SCSI HBA and name this ICH9. :)

No, please. ICH9 is a controller hub. This device really is only about the AHCI part of it.

> 
>> +
>> +#define ICH_AHCI(obj) \
>> +    OBJECT_CHECK(AHCIPCIState, (obj), TYPE_ICH_AHCI)
> 
> Wondering if this is specific to ICH(9)? Alex?
> Leaving it as is for now, renaming is an easy follow-up.

This is not an ICH9 device. It's an ICH9-AHCI device. And for that the check looks sane from what I can tell.


Alex

> 
>> +
>> extern const VMStateDescription vmstate_ahci;
>> 
>> #define VMSTATE_AHCI(_field, _state) {                               \
>> diff --git a/hw/ide/ich.c b/hw/ide/ich.c
>> index 6c0c0c2..c3cbf2a 100644
>> --- a/hw/ide/ich.c
>> +++ b/hw/ide/ich.c
>> @@ -92,7 +92,7 @@ static const VMStateDescription vmstate_ich9_ahci = {
>> 
>> static void pci_ich9_reset(DeviceState *dev)
>> {
>> -    struct AHCIPCIState *d = DO_UPCAST(struct AHCIPCIState, card.qdev, dev);
>> +    struct AHCIPCIState *d = ICH_AHCI(dev);
> 
> Let's drop the "struct" while touching the line.
> 
> Thanks, applied to qom-next:
> https://github.com/afaerber/qemu-cpu/commits/qom-next
> 
> Andreas
> 
>> 
>>     ahci_reset(&d->ahci);
>> }
>> @@ -102,9 +102,9 @@ static int pci_ich9_ahci_init(PCIDevice *dev)
>>     struct AHCIPCIState *d;
>>     int sata_cap_offset;
>>     uint8_t *sata_cap;
>> -    d = DO_UPCAST(struct AHCIPCIState, card, dev);
>> +    d = ICH_AHCI(dev);
>> 
>> -    ahci_init(&d->ahci, &dev->qdev, pci_get_address_space(dev), 6);
>> +    ahci_init(&d->ahci, DEVICE(dev), pci_get_address_space(dev), 6);
>> 
>>     pci_config_set_prog_interface(d->card.config, AHCI_PROGMODE_MAJOR_REV_1);
>> 
>> @@ -141,7 +141,7 @@ static int pci_ich9_ahci_init(PCIDevice *dev)
>> static void pci_ich9_uninit(PCIDevice *dev)
>> {
>>     struct AHCIPCIState *d;
>> -    d = DO_UPCAST(struct AHCIPCIState, card, dev);
>> +    d = ICH_AHCI(dev);
>> 
>>     msi_uninit(dev);
>>     ahci_uninit(&d->ahci);
>> @@ -163,7 +163,7 @@ static void ich_ahci_class_init(ObjectClass *klass, void *data)
>> }
>> 
>> static const TypeInfo ich_ahci_info = {
>> -    .name          = "ich9-ahci",
>> +    .name          = TYPE_ICH_AHCI,
>>     .parent        = TYPE_PCI_DEVICE,
>>     .instance_size = sizeof(AHCIPCIState),
>>     .class_init    = ich_ahci_class_init,
>> 
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Andreas Färber - July 1, 2013, 10:03 a.m.
Am 01.07.2013 01:36, schrieb Alexander Graf:
> 
> On 30.06.2013, at 10:21, Andreas Färber wrote:
> 
>> Am 24.06.2013 08:55, schrieb peter.crosthwaite@xilinx.com:
>>> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>>
>>> Define and use standard QOM cast macro. Remove usages of DO_UPCAST
>>> and direct -> style upcasting.
>>>
>>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>> ---
>>>
>>> hw/ide/ahci.h |  5 +++++
>>> hw/ide/ich.c  | 10 +++++-----
>>> 2 files changed, 10 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
>>> index 341a571..916bef0 100644
>>> --- a/hw/ide/ahci.h
>>> +++ b/hw/ide/ahci.h
>>> @@ -305,6 +305,11 @@ typedef struct AHCIPCIState {
>>>     AHCIState ahci;
>>> } AHCIPCIState;
>>>
>>> +#define TYPE_ICH_AHCI "ich9-ahci"
>>
>> Let's be as precise as for the LSI SCSI HBA and name this ICH9. :)
> 
> No, please. ICH9 is a controller hub. This device really is only about the AHCI part of it.
> 
>>
>>> +
>>> +#define ICH_AHCI(obj) \
>>> +    OBJECT_CHECK(AHCIPCIState, (obj), TYPE_ICH_AHCI)
>>
>> Wondering if this is specific to ICH(9)? Alex?
>> Leaving it as is for now, renaming is an easy follow-up.
> 
> This is not an ICH9 device. It's an ICH9-AHCI device. And for that the check looks sane from what I can tell.

I don't see how either of your answers relate to my question? Maybe you
were just tired? ;)

So as you can see above, the type is called ich9-ahci, therefore I have
done s/TYPE_ICH_AHCI/TYPE_ICH9_AHCI/g, and I still don't see anything
wrong with it, Peter just ack'ed it. There's a link to the modified
patch below to verify.
What does a controller hub vs. AHCI have to do with ICH vs. ICH9?

The implied question really is, how specific is AHCIPCIState to
ich9-ahci (its sole user AFAICS)? Would an ich6-ahci or ich10-ahci use
the same struct or not? Is it even specific to ICH? If not, we might
want to name the cast macro after the struct PCI_AHCI() rather than
ICH_AHCI(). Compare EHCI where we have a base struct and one derived
class having its own extended state/class structs.

Andreas

>>> +
>>> extern const VMStateDescription vmstate_ahci;
>>>
>>> #define VMSTATE_AHCI(_field, _state) {                               \
>>> diff --git a/hw/ide/ich.c b/hw/ide/ich.c
>>> index 6c0c0c2..c3cbf2a 100644
>>> --- a/hw/ide/ich.c
>>> +++ b/hw/ide/ich.c
>>> @@ -92,7 +92,7 @@ static const VMStateDescription vmstate_ich9_ahci = {
>>>
>>> static void pci_ich9_reset(DeviceState *dev)
>>> {
>>> -    struct AHCIPCIState *d = DO_UPCAST(struct AHCIPCIState, card.qdev, dev);
>>> +    struct AHCIPCIState *d = ICH_AHCI(dev);
>>
>> Let's drop the "struct" while touching the line.
>>
>> Thanks, applied to qom-next:
>> https://github.com/afaerber/qemu-cpu/commits/qom-next
>>
>> Andreas
>>
>>>
>>>     ahci_reset(&d->ahci);
>>> }
>>> @@ -102,9 +102,9 @@ static int pci_ich9_ahci_init(PCIDevice *dev)
>>>     struct AHCIPCIState *d;
>>>     int sata_cap_offset;
>>>     uint8_t *sata_cap;
>>> -    d = DO_UPCAST(struct AHCIPCIState, card, dev);
>>> +    d = ICH_AHCI(dev);
>>>
>>> -    ahci_init(&d->ahci, &dev->qdev, pci_get_address_space(dev), 6);
>>> +    ahci_init(&d->ahci, DEVICE(dev), pci_get_address_space(dev), 6);
>>>
>>>     pci_config_set_prog_interface(d->card.config, AHCI_PROGMODE_MAJOR_REV_1);
>>>
>>> @@ -141,7 +141,7 @@ static int pci_ich9_ahci_init(PCIDevice *dev)
>>> static void pci_ich9_uninit(PCIDevice *dev)
>>> {
>>>     struct AHCIPCIState *d;
>>> -    d = DO_UPCAST(struct AHCIPCIState, card, dev);
>>> +    d = ICH_AHCI(dev);
>>>
>>>     msi_uninit(dev);
>>>     ahci_uninit(&d->ahci);
>>> @@ -163,7 +163,7 @@ static void ich_ahci_class_init(ObjectClass *klass, void *data)
>>> }
>>>
>>> static const TypeInfo ich_ahci_info = {
>>> -    .name          = "ich9-ahci",
>>> +    .name          = TYPE_ICH_AHCI,
>>>     .parent        = TYPE_PCI_DEVICE,
>>>     .instance_size = sizeof(AHCIPCIState),
>>>     .class_init    = ich_ahci_class_init,
>>>
>>
>>
>> -- 
>> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
>> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>
Alexander Graf - July 1, 2013, 10:10 a.m.
On 01.07.2013, at 12:03, Andreas Färber wrote:

> Am 01.07.2013 01:36, schrieb Alexander Graf:
>> 
>> On 30.06.2013, at 10:21, Andreas Färber wrote:
>> 
>>> Am 24.06.2013 08:55, schrieb peter.crosthwaite@xilinx.com:
>>>> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>>> 
>>>> Define and use standard QOM cast macro. Remove usages of DO_UPCAST
>>>> and direct -> style upcasting.
>>>> 
>>>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>>> ---
>>>> 
>>>> hw/ide/ahci.h |  5 +++++
>>>> hw/ide/ich.c  | 10 +++++-----
>>>> 2 files changed, 10 insertions(+), 5 deletions(-)
>>>> 
>>>> diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
>>>> index 341a571..916bef0 100644
>>>> --- a/hw/ide/ahci.h
>>>> +++ b/hw/ide/ahci.h
>>>> @@ -305,6 +305,11 @@ typedef struct AHCIPCIState {
>>>>    AHCIState ahci;
>>>> } AHCIPCIState;
>>>> 
>>>> +#define TYPE_ICH_AHCI "ich9-ahci"
>>> 
>>> Let's be as precise as for the LSI SCSI HBA and name this ICH9. :)
>> 
>> No, please. ICH9 is a controller hub. This device really is only about the AHCI part of it.
>> 
>>> 
>>>> +
>>>> +#define ICH_AHCI(obj) \
>>>> +    OBJECT_CHECK(AHCIPCIState, (obj), TYPE_ICH_AHCI)
>>> 
>>> Wondering if this is specific to ICH(9)? Alex?
>>> Leaving it as is for now, renaming is an easy follow-up.
>> 
>> This is not an ICH9 device. It's an ICH9-AHCI device. And for that the check looks sane from what I can tell.
> 
> I don't see how either of your answers relate to my question? Maybe you
> were just tired? ;)

Ah, sorry. I thought you wanted to just name it "ICH" instead of "ich9-ahci".

> 
> So as you can see above, the type is called ich9-ahci, therefore I have
> done s/TYPE_ICH_AHCI/TYPE_ICH9_AHCI/g, and I still don't see anything
> wrong with it, Peter just ack'ed it. There's a link to the modified
> patch below to verify.
> What does a controller hub vs. AHCI have to do with ICH vs. ICH9?
> 
> The implied question really is, how specific is AHCIPCIState to
> ich9-ahci (its sole user AFAICS)? Would an ich6-ahci or ich10-ahci use
> the same struct or not? Is it even specific to ICH? If not, we might
> want to name the cast macro after the struct PCI_AHCI() rather than
> ICH_AHCI(). Compare EHCI where we have a base struct and one derived
> class having its own extended state/class structs.

ich9-ahci is-a pci-ahci is-a ahci makes sense I would say. Different controllers can have different numbers of connectors, but apart from that they should look pretty similar.


Alex

Patch

diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index 341a571..916bef0 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -305,6 +305,11 @@  typedef struct AHCIPCIState {
     AHCIState ahci;
 } AHCIPCIState;
 
+#define TYPE_ICH_AHCI "ich9-ahci"
+
+#define ICH_AHCI(obj) \
+    OBJECT_CHECK(AHCIPCIState, (obj), TYPE_ICH_AHCI)
+
 extern const VMStateDescription vmstate_ahci;
 
 #define VMSTATE_AHCI(_field, _state) {                               \
diff --git a/hw/ide/ich.c b/hw/ide/ich.c
index 6c0c0c2..c3cbf2a 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -92,7 +92,7 @@  static const VMStateDescription vmstate_ich9_ahci = {
 
 static void pci_ich9_reset(DeviceState *dev)
 {
-    struct AHCIPCIState *d = DO_UPCAST(struct AHCIPCIState, card.qdev, dev);
+    struct AHCIPCIState *d = ICH_AHCI(dev);
 
     ahci_reset(&d->ahci);
 }
@@ -102,9 +102,9 @@  static int pci_ich9_ahci_init(PCIDevice *dev)
     struct AHCIPCIState *d;
     int sata_cap_offset;
     uint8_t *sata_cap;
-    d = DO_UPCAST(struct AHCIPCIState, card, dev);
+    d = ICH_AHCI(dev);
 
-    ahci_init(&d->ahci, &dev->qdev, pci_get_address_space(dev), 6);
+    ahci_init(&d->ahci, DEVICE(dev), pci_get_address_space(dev), 6);
 
     pci_config_set_prog_interface(d->card.config, AHCI_PROGMODE_MAJOR_REV_1);
 
@@ -141,7 +141,7 @@  static int pci_ich9_ahci_init(PCIDevice *dev)
 static void pci_ich9_uninit(PCIDevice *dev)
 {
     struct AHCIPCIState *d;
-    d = DO_UPCAST(struct AHCIPCIState, card, dev);
+    d = ICH_AHCI(dev);
 
     msi_uninit(dev);
     ahci_uninit(&d->ahci);
@@ -163,7 +163,7 @@  static void ich_ahci_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo ich_ahci_info = {
-    .name          = "ich9-ahci",
+    .name          = TYPE_ICH_AHCI,
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(AHCIPCIState),
     .class_init    = ich_ahci_class_init,