[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 00/30] PCI: Cleanup legacy casts in device la
From: |
Peter Crosthwaite |
Subject: |
Re: [Qemu-devel] [PATCH v2 00/30] PCI: Cleanup legacy casts in device land -- ANN: qom-next revived |
Date: |
Mon, 1 Jul 2013 14:33:35 +1000 |
Hi Andreas,
On Sun, Jun 30, 2013 at 8:44 PM, Andreas Färber <address@hidden> wrote:
> Hi Peter,
>
> Am 24.06.2013 08:49, schrieb address@hidden:
>> From: Peter Crosthwaite <address@hidden>
>>
>> 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
>
- Re: [Qemu-devel] [PATCH v2 00/30] PCI: Cleanup legacy casts in device land -- ANN: qom-next revived,
Peter Crosthwaite <=