Re: [Qemu-devel] [PATCH v2 00/30] PCI: Cleanup legacy casts in device land -- ANN: qom-next revived

[Peter Crosthwaite <peter.crosthwaite@xilinx.com>]

Hi Andreas,

On Sun, Jun 30, 2013 at 8:44 PM, Andreas Färber <afaerber@suse.de> wrote:
> Hi Peter,
>
> Am 24.06.2013 08:49, schrieb peter.crosthwaite@xilinx.com:
>> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>
>> There are a number of different cast implementations from various
>> stages of QEMU development out in device model land. This series cleans
>> up the ones involving TYPE_PCI_DEVICE to consistently use proper QOM
>> casts for both up and down casts.
>>
>> Some were easy, some needed QOM cast macros which are added as
>> appropriate.
>>
>> Following the recent discussion RE performance consequences of QOM
>> casts, im interested in any reports of possible performance regressions
>> here, although I am hoping that Anthony current efforts to improve
>> QOM casting efficiency make this a non-issue.
>
> While I did not run extensive benchmarks, state of the discussion
> between Paolo, Anthony and me, I believe, was that it can be considered
> okay to use QOM casts "everywhere" consistently now, but we should not
> use casts where they are unnecessary (i.e., only where we change type).
> E.g., http://patchwork.ozlabs.org/patch/255367/
>
> I have therefore dropped some opaque casts where the type on both sides
> of void* matched (for an up-/downcast I do prefer the cast for safety).
>
> Anyway, if we get this merged early, then there is still time for more
> benchmarking/optimizations during Soft Freeze IMO.
> Maybe our staging tree will facilitate testing, too. ;)
>
>> Changed since V1:
>> Removed hunks which macroified VMSD names
>> Dropped virtio/virtio.pci patch
>> Rebased
>>
>>
>> Peter Crosthwaite (30):
>>   net/e1000: QOM Upcast Sweep
>>   net/rtl8139: QOM Upcast Sweep
>>   net/pcnet-pci: QOM Upcast Sweep
>>   usb/hcd-xhci: QOM Upcast Sweep
>>   scsi/lsi53c895a: QOM Upcast Sweep
>>   scsi/megasas: QOM Upcast Sweep
>>   scsi/esp-pci: QOM Upcast Sweep
>>   ide/ich: QOM Upcast Sweep
>>   ide/piix: QOM casting sweep
>>   acpi/piix4: QOM Upcast Sweep
>>   misc/pci-testdev: QOM Upcast Sweep
>>   virtio/vmware_vga: QOM casting sweep
>>   misc/ivshmem: QOM Upcast Sweep
>>   xen/xen_platform: QOM casting sweep
>
> As requested, I've started picking up QOM type/cast/realize patches on:
>
> git://github.com/afaerber/qemu-cpu.git qom-next

Perhaps this is becoming the "qom-devices" queue?

> https://github.com/afaerber/qemu-cpu/commits/qom-next
>
> (Not to be confused with my qom-cpu / qom-cpu-next CPU trees.)
>
> If anyone wishes to contribute patches against that tree, please
> indicate so with --subject-prefix="PATCH qom-next ...".
>
> As a matter of personal taste and consistency, I've used the gtk-doc
> notation DO_UPCAST() wherever I stumbled over it in commit messages.
>
> I've queued all patches above except for ide/piix (09/30) and had
> comments and/or minor changes for some of them. Noticing some
> incompleteness, I will reiterate over them.
>

I think the incompleteness comes from the fact that my series is only
worrying about type PCI_DEVICE. I have left alone QOM cast issues for
casts that are not to/from or through TYPE_PCI_DEVICE, so there are
instances where a QOM cast macro may be introduces but not used.

> Whether I send a pull when we're all happy with it or whether we let
> submaintainers pick/pull by subsystem at some point doesn't matter to
> me, as long as we can join efforts to make QOM realize reality soon. :)
>

I think getting it all into one giant queue then we can centralise
testing a little better.

>>   isa/*: QOM casting sweep
>>   pci/*: QOM casting sweep
>>   pci-bridge/pci_bridge_dev: Don't use DO_UPCAST
>>   pci-bridge/*: substitute ->qdev casts with DEVICE()
>>   pci/pci_bridge: substitute ->qdev casts with DEVICE()
>>   misc/vfio: substitute ->qdev casts with DEVICE()
>>   net/eepro100: substitute ->qdev casts with DEVICE()
>>   net/ne2000: substitute ->qdev casts with DEVICE()
>>   usb/*: substitute ->qdev casts with DEVICE()
>>   watchdog/wdt_i6300esb: substitute ->qdev casts with DEVICE()
>>   scsi/vmw_pvscsi: substitute ->qdev casts with DEVICE()
>>   i2c/smbus_ich9: substitute ->qdev casts with DEVICE()
>>   ide/cmd646: substitute ->qdev casts with DEVICE()
>>   ide/via: substitute ->qdev casts with DEVICE()
>>   pci-host/*: substitute ->qdev casts with DEVICE()
>>   i386/*: substitute ->qdev casts with DEVICE()
>
> These patches seem more "sloppy" while not reaching a clear goal such as
> dropping a macro or renaming PCIDevice::qdev, so I'd prefer to get open
> issues sorted out before rushing ahead with half-done conversions.
> Functionally everything I've seen so far looked fine though.
>
> But maybe I'm missing something? What exactly was the motivation behind
> the series? Do you have a follow-up?
>

I have an experimental series out-of-tree (change the parent type of
TYPE_PCI_DEVICE to TYPE_SYSBUS) to remove the need to coreify devices
for use as both sysbus and PCI (e.g. EHCI). These casts get in the way
of that series.

I really want to make it clear though, that this series is legitimate
cleanup without that goal, so I didn't want to tangle this series in
that much-longer discussion.

So this series specifically is a compile bug chase on:

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 6ef1f97..5f607b3 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -217,7 +217,6 @@ typedef void (*MSIVectorPollNotifier)(PCIDevice *dev,
                                       unsigned int vector_end);

 struct PCIDevice {
-    DeviceState qdev;

     /* PCI config space */
     uint8_t *config;

FWIW, one could actually remove all parent_obj fields from QOM structs
(just like this) and the tree should compile, good way to catch all
legacy casts.

Regards,
Peter

> Regards,
> Andreas
>
>>
>>  hw/acpi/piix4.c                    | 31 +++++++++++++++++--------------
>>  hw/display/vmware_vga.c            | 13 ++++++++-----
>>  hw/i2c/smbus_ich9.c                |  2 +-
>>  hw/i386/kvm/pci-assign.c           | 21 ++++++++++++---------
>>  hw/i386/pc.c                       |  3 ++-
>>  hw/i386/pc_piix.c                  |  4 ++--
>>  hw/i386/pc_q35.c                   |  4 ++--
>>  hw/ide/ahci.h                      |  5 +++++
>>  hw/ide/cmd646.c                    |  8 ++++----
>>  hw/ide/ich.c                       | 10 +++++-----
>>  hw/ide/piix.c                      |  8 ++++----
>>  hw/ide/via.c                       |  4 ++--
>>  hw/isa/i82378.c                    |  8 ++++----
>>  hw/isa/lpc_ich9.c                  |  6 +++---
>>  hw/misc/ivshmem.c                  | 18 +++++++++++-------
>>  hw/misc/pci-testdev.c              | 11 ++++++++---
>>  hw/misc/vfio.c                     |  4 ++--
>>  hw/net/e1000.c                     | 18 ++++++++++++------
>>  hw/net/eepro100.c                  | 14 ++++++++------
>>  hw/net/ne2000.c                    |  6 ++++--
>>  hw/net/pcnet-pci.c                 | 14 +++++++++-----
>>  hw/net/rtl8139.c                   | 26 ++++++++++++++++++--------
>>  hw/pci-bridge/dec.c                |  2 +-
>>  hw/pci-bridge/i82801b11.c          |  2 +-
>>  hw/pci-bridge/ioh3420.c            |  2 +-
>>  hw/pci-bridge/pci_bridge_dev.c     |  2 +-
>>  hw/pci-bridge/xio3130_downstream.c |  2 +-
>>  hw/pci-bridge/xio3130_upstream.c   |  2 +-
>>  hw/pci-host/apb.c                  |  4 ++--
>>  hw/pci-host/q35.c                  |  4 ++--
>>  hw/pci/pci-hotplug.c               | 18 ++++++++++--------
>>  hw/pci/pci.c                       | 17 +++++++++--------
>>  hw/pci/pci_bridge.c                |  7 ++++---
>>  hw/pci/pcie.c                      |  4 ++--
>>  hw/pci/shpc.c                      |  8 ++++----
>>  hw/scsi/esp-pci.c                  | 14 +++++++++-----
>>  hw/scsi/lsi53c895a.c               | 26 ++++++++++++++++----------
>>  hw/scsi/megasas.c                  | 15 ++++++++++-----
>>  hw/scsi/vmw_pvscsi.c               |  2 +-
>>  hw/usb/hcd-ehci-pci.c              | 13 ++++++++-----
>>  hw/usb/hcd-ohci.c                  |  2 +-
>>  hw/usb/hcd-uhci.c                  |  2 +-
>>  hw/usb/hcd-xhci.c                  | 19 +++++++++++++------
>>  hw/watchdog/wdt_i6300esb.c         |  2 +-
>>  hw/xen/xen_platform.c              | 28 ++++++++++++++++------------
>>  45 files changed, 258 insertions(+), 177 deletions(-)
>>
>
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>