Patchwork [for-1.2,v5,14/14] pci: Tidy up PCI host bridges

login
register
mail settings
Submitter Michael S. Tsirkin
Date Aug. 13, 2012, 1:16 p.m.
Message ID <20120813131659.GE16801@redhat.com>
Download mbox | patch
Permalink /patch/176937/
State New
Headers show

Comments

Michael S. Tsirkin - Aug. 13, 2012, 1:16 p.m.
On Thu, Aug 02, 2012 at 03:47:06AM +0200, Andreas Färber wrote:
> Uglify the parent field to enforce QOM-style access via casts.
> Don't just typedef PCIHostState, either use it directly or embed it.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>


IMHO only one chunk from this patch should be applied (below).
Below it is split out but needs to be rebased on top of patches
1-13.

-->

From: Andreas Färber <andreas.faerber@web.de>

piix: minor code simplification

There's no need to deal with qdev internals in piix - we get device
state from qdev_create so just use that.

Signed-off-by: Andreas Färber <andreas.faerber@web.de>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---
Anthony Liguori - Aug. 13, 2012, 2:16 p.m.
"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Thu, Aug 02, 2012 at 03:47:06AM +0200, Andreas Färber wrote:
>> Uglify the parent field to enforce QOM-style access via casts.
>> Don't just typedef PCIHostState, either use it directly or embed it.
>> 
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>
>
> IMHO only one chunk from this patch should be applied (below).
> Below it is split out but needs to be rebased on top of patches
> 1-13.

I understand what your objection is but it's unreasonable IMHO.  The
purpose of QOM is to bring consistency across large swaths of code in
QEMU that have historically done things there own way.

This means expressing concepts like inheritence and casting in the same
way across the board.  The common way (the QOM way) is to make the
parent type the first member of the struct (typically named parent or
parent_obj) and then to use cast macros to upcast and downcast.

This patch is 100% correct in that regard and I'm going to apply it once
Andreas makes the change I requested.

For my part, I'm long over due in writing up a device authoring style
guide that I promised a few weeks ago.  I'll write that up this
afternoon and send it out today.

We can debate the merits of this sort of thing in the style guide.

Regards,

Anthony Liguori

>
> -->
>
> From: Andreas Färber <andreas.faerber@web.de>
>
> piix: minor code simplification
>
> There's no need to deal with qdev internals in piix - we get device
> state from qdev_create so just use that.
>
> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> ---
>
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index c497a01..18554a6 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -274,7 +274,7 @@ static PCIBus *i440fx_common_init(const char *device_name,
>      dev = qdev_create(NULL, "i440FX-pcihost");
>      s = FROM_SYSBUS(I440FXState, sysbus_from_qdev(dev));
>      s->address_space = address_space_mem;
> -    b = pci_bus_new(&s->busdev.qdev, NULL, pci_address_space,
> +    b = pci_bus_new(&dev, NULL, pci_address_space,
>                      address_space_io, 0);
>      s->bus = b;
>      object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev), NULL);
Mark Cave-Ayland - Aug. 13, 2012, 6:57 p.m.
On 13/08/12 15:16, Anthony Liguori wrote:

> I understand what your objection is but it's unreasonable IMHO.  The
> purpose of QOM is to bring consistency across large swaths of code in
> QEMU that have historically done things there own way.
>
> This means expressing concepts like inheritence and casting in the same
> way across the board.  The common way (the QOM way) is to make the
> parent type the first member of the struct (typically named parent or
> parent_obj) and then to use cast macros to upcast and downcast.
>
> This patch is 100% correct in that regard and I'm going to apply it once
> Andreas makes the change I requested.
>
> For my part, I'm long over due in writing up a device authoring style
> guide that I promised a few weeks ago.  I'll write that up this
> afternoon and send it out today.
>
> We can debate the merits of this sort of thing in the style guide.

Yes please! I've spent quite a fair bit of time over the past couple of 
weeks in this area, and have found the lack of documentation related to 
trying to create a new hardware device fairly frustrating. The hardest 
part is not the coding, but figuring which of the following I should be 
using out of:

qom
qdev
qmp
hmp
sysbus
VMState

(I think that half of these are now obsolete interfaces, but a general 
document which explains how these things are related and which ones I 
should be using would be very welcome)


ATB,

Mark.

Patch

diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index c497a01..18554a6 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -274,7 +274,7 @@  static PCIBus *i440fx_common_init(const char *device_name,
     dev = qdev_create(NULL, "i440FX-pcihost");
     s = FROM_SYSBUS(I440FXState, sysbus_from_qdev(dev));
     s->address_space = address_space_mem;
-    b = pci_bus_new(&s->busdev.qdev, NULL, pci_address_space,
+    b = pci_bus_new(&dev, NULL, pci_address_space,
                     address_space_io, 0);
     s->bus = b;
     object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev), NULL);