[Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize
QOM splits the destruction of a device in two phases: - unrealize, also known as "exit" from qdev times, should isolate the device from the guest. After unrealize returns, the guest should not be able to issue new requests. - instance_finalize will reclaim the memory. This is only called after all requests terminate and drop the references on the device. This is important even now, but overlooked. It will become much more important for devices that will be able to access memory out of the big QEMU lock. This series changes all PCI devices (the sole to support hotplug _and_ use MemoryRegions) to do memory_region_del_subregion at unrealize time, and memory_region_destroy at instance_finalize time. This is mostly a PCI patch, and independent from the others, so I believe it should go through mst's tree. Paolo Paolo Bonzini (39): scsi: keep device alive while it has requests dma: keep a device alive while it has SGLists pci: split exit and finalize ac97: use instance_finalize instead of exit es1370: use instance_finalize instead of exit hda: use instance_finalize instead of exit serial: use instance_finalize instead of exit tpci200: use instance_finalize instead of exit pci-assign: use instance_finalize instead of exit ahci: use instance_finalize instead of exit msix: split msix_free from msix_uninit cmd646: use instance_finalize instead of exit ide/piix: use instance_finalize instead of exit ide/via: use instance_finalize instead of exit ivshmem: use instance_finalize instead of exit pci-testdev: use instance_finalize instead of exit vfio: use instance_finalize instead of exit e1000: use instance_finalize instead of exit eepro100: use instance_finalize instead of exit ne2000: use instance_finalize instead of exit pcnet: use instance_finalize instead of exit rtl8139: use instance_finalize instead of exit vmxnet3: use instance_finalize instead of exit shpc: use instance_finalize instead of exit pci_bridge: split pci_bridge_exitfn from pci_bridge_free pcie_aer: pcie_aer_exit really frees stuff pci_bridge: use instance_finalize instead of exit ioh4320: use instance_finalize instead of exit xio3130-downstream: use instance_finalize instead of exit xio3130-upstream: use instance_finalize instead of exit pcie: do not recreate mmcfg I/O region, use an alias instead esp: use instance_finalize instead of exit lsi: use instance_finalize instead of exit pvscsi: use instance_finalize instead of exit usb-uhci: use instance_finalize instead of exit virtio-pci: use instance_finalize instead of exit wdt_i6300esb: use instance_finalize instead of exit xen_pt: use instance_finalize instead of exit tpm: move add/del_subregion to realize/unrealize dma-helpers.c | 6 - hw/audio/ac97.c| 5 ++-- hw/audio/es1370.c | 5 ++-- hw/audio/intel-hda.c | 8 ++ hw/char/serial-pci.c | 24 ++ hw/char/tpci200.c | 5 ++-- hw/i386/kvm/pci-assign.c | 8 ++ hw/ide/ahci.c | 5 ++-- hw/ide/ahci.h | 2 +- hw/ide/cmd646.c| 5 ++-- hw/ide/ich.c | 13 +++--- hw/ide/macio.c | 4 +-- hw/ide/piix.c | 8 +++--- hw/ide/via.c | 5 ++-- hw/misc/ivshmem.c | 10 +++- hw/misc/pci-testdev.c | 5 ++-- hw/misc/vfio.c | 52 +++--- hw/net/e1000.c | 5 ++-- hw/net/eepro100.c | 5 ++-- hw/net/ne2000.c| 5 ++-- hw/net/pcnet-pci.c | 5 ++-- hw/net/rtl8139.c | 5 ++-- hw/net/vmxnet3.c | 14 -- hw/pci-bridge/i82801b11.c | 1 + hw/pci-bridge/ioh3420.c| 11 +++- hw/pci-bridge/pci_bridge_dev.c | 14 +- hw/pci-bridge/xio3130_downstream.c | 11 +++- hw/pci-bridge/xio3130_upstream.c | 11 +++- hw/pci/msix.c | 26 ++- hw/pci/pci.c | 15 --- hw/pci/pci_bridge.c| 5 hw/pci/pcie_aer.c | 3 ++- hw/pci/pcie_host.c | 22 hw/pci/shpc.c | 8 +- hw/scsi/esp-pci.c | 5 ++-- hw/scsi/lsi53c895a.c | 5 ++-- hw/scsi/megasas.c | 4 +-- hw/scsi/scsi-bus.c | 4 +++ hw/scsi/virtio-scsi.c | 10 +--- hw/scsi/vmw_pvscsi.c | 12 - hw/tpm/tpm_tis.c | 14 +++--- hw/usb/hcd-ehci.c | 4 +-- hw/usb/hcd-uhci.c | 5 ++-- hw/virtio/virtio-pci.c | 10 +++- hw/watchdog/wdt_i6300esb.c | 5 ++-- hw/xen/xen_pt.c
Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize
Am 04.06.2013 20:51, schrieb Paolo Bonzini: > This series changes all PCI devices (the sole to support hotplug > _and_ use MemoryRegions) to do memory_region_del_subregion at > unrealize time, and memory_region_destroy at instance_finalize > time. The general idea looks good. Could you please follow-up with a patch that switches from exit to unrealize? Also I notice some patches are accessing parent fields directly - please use BUS(), PCI_DEVICE() etc. to hide this. Andreas -- 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 00/39] Delay destruction of memory regions to instance_finalize
On Wed, Jun 05, 2013 at 11:50:52AM +0200, Andreas Färber wrote: > Am 04.06.2013 20:51, schrieb Paolo Bonzini: > > This series changes all PCI devices (the sole to support hotplug > > _and_ use MemoryRegions) to do memory_region_del_subregion at > > unrealize time, and memory_region_destroy at instance_finalize > > time. > > The general idea looks good. > > Could you please follow-up with a patch that switches from exit to > unrealize? What do you guys think about changing the name to something else e.g. "free" or "destroy"? unrealize is not a word in english: http://dictionary.cambridge.org/spellcheck/american-english/?q=unrealize I can do it easily if people agree. > use BUS(), PCI_DEVICE() etc. to hide this. > > Andreas > > -- > 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 00/39] Delay destruction of memory regions to instance_finalize
Am 05.06.2013 13:10, schrieb Michael S. Tsirkin: > On Wed, Jun 05, 2013 at 11:50:52AM +0200, Andreas Färber wrote: >> Am 04.06.2013 20:51, schrieb Paolo Bonzini: >>> This series changes all PCI devices (the sole to support hotplug >>> _and_ use MemoryRegions) to do memory_region_del_subregion at >>> unrealize time, and memory_region_destroy at instance_finalize >>> time. >> >> The general idea looks good. >> >> Could you please follow-up with a patch that switches from exit to >> unrealize? > > What do you guys think about changing the name to something > else e.g. "free" or "destroy"? I'm not generally opposed to renaming things, but current unrealize is a pair with realize, and destroy or free doesn't really fit it's purpose - that's instance_finalize. Let's CC Anthony. Andreas > > unrealize is not a word in english: > http://dictionary.cambridge.org/spellcheck/american-english/?q=unrealize > > I can do it easily if people agree. > >> use BUS(), PCI_DEVICE() etc. to hide this. >> >> Andreas >> >> -- >> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany >> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg -- 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 00/39] Delay destruction of memory regions to instance_finalize
On 5 June 2013 12:10, Michael S. Tsirkin wrote: > unrealize is not a word in english: The OED says: # unˈrealize, v. # trans. To make unreal; to deprive of reality. with the earliest citation from 1804. so if it seems like the best term (and it does make clear the pairing with realize, which I think is a strong argument) we should go ahead and use it. thanks -- PMM
Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize
On Wed, Jun 05, 2013 at 12:38:35PM +0100, Peter Maydell wrote: > On 5 June 2013 12:10, Michael S. Tsirkin wrote: > > unrealize is not a word in english: > > The OED says: > # unˈrealize, v. > # trans. To make unreal; to deprive of reality. > > with the earliest citation from 1804. > > so if it seems like the best term (and it does make > clear the pairing with realize, which I think is > a strong argument) we should go ahead and use it. > > thanks > -- PMM realize is a bad name too. what does it mean? make real? It's still all virtual ... If we want it to mean hide from guest/expose to guest, then why not call it like this? expose_to_guest unexpose_to_guest finalize is even more ambigous, and not pairing with anything as far as I could see. -- MST
Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize
On Wed, Jun 05, 2013 at 01:32:17PM +0200, Andreas Färber wrote: > Am 05.06.2013 13:10, schrieb Michael S. Tsirkin: > > On Wed, Jun 05, 2013 at 11:50:52AM +0200, Andreas Färber wrote: > >> Am 04.06.2013 20:51, schrieb Paolo Bonzini: > >>> This series changes all PCI devices (the sole to support hotplug > >>> _and_ use MemoryRegions) to do memory_region_del_subregion at > >>> unrealize time, and memory_region_destroy at instance_finalize > >>> time. > >> > >> The general idea looks good. > >> > >> Could you please follow-up with a patch that switches from exit to > >> unrealize? > > > > What do you guys think about changing the name to something > > else e.g. "free" or "destroy"? > > I'm not generally opposed to renaming things, but current unrealize is a > pair with realize, and destroy or free doesn't really fit it's purpose - > that's instance_finalize. Let's CC Anthony. > > Andreas So @instance_init -> instance_alloc instance_finalize -> @instance_free? > > > > unrealize is not a word in english: > > http://dictionary.cambridge.org/spellcheck/american-english/?q=unrealize > > > > I can do it easily if people agree. > > > >> use BUS(), PCI_DEVICE() etc. to hide this. > >> > >> Andreas > >> > >> -- > >> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > >> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg > > > -- > 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 00/39] Delay destruction of memory regions to instance_finalize
On Wed, Jun 05, 2013 at 12:38:35PM +0100, Peter Maydell wrote: > On 5 June 2013 12:10, Michael S. Tsirkin wrote: > > unrealize is not a word in english: > > The OED says: > # unˈrealize, v. > # trans. To make unreal; to deprive of reality. > > with the earliest citation from 1804. So someone somewhere uses it like this once. It's still a bad idea to use uncommon words, it won't be in a dictionary of non-native english speakers and attempts to look it up in online dictionaries fail to return useful info. Documentation also talks about Realization as a process of making real. You are going to say someone used it like that in the 19th century? It does not change the fact that realize means "understand" in the most common meaning of this word. include/hw/qdev-core.h also uses the term Realization. Again for most people Realization means becoming aware of http://oxforddictionaries.com/definition/english/realization?q=Realization So at least, this is ambigous. Can we use terms which are less ambigous? > so if it seems like the best term (and it does make > clear the pairing with realize, which I think is > a strong argument) we should go ahead and use it. > > thanks > -- PMM
Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize
Am 05.06.2013 14:06, schrieb Michael S. Tsirkin: > On Wed, Jun 05, 2013 at 01:32:17PM +0200, Andreas Färber wrote: >> Am 05.06.2013 13:10, schrieb Michael S. Tsirkin: >>> On Wed, Jun 05, 2013 at 11:50:52AM +0200, Andreas Färber wrote: Am 04.06.2013 20:51, schrieb Paolo Bonzini: > This series changes all PCI devices (the sole to support hotplug > _and_ use MemoryRegions) to do memory_region_del_subregion at > unrealize time, and memory_region_destroy at instance_finalize > time. The general idea looks good. Could you please follow-up with a patch that switches from exit to unrealize? >>> >>> What do you guys think about changing the name to something >>> else e.g. "free" or "destroy"? >> >> I'm not generally opposed to renaming things, but current unrealize is a >> pair with realize, and destroy or free doesn't really fit it's purpose - >> that's instance_finalize. Let's CC Anthony. > > So @instance_init -> instance_alloc No, allocation happens before instance_init, it only initializes fields of the instance, so that name seems good to me. My ISA realize patches (need to respin after Paolo enabled gus) worked towards resolving the DeviceClass::init vs. instance_init ambiguity, so once completed only instance_init and class_init would remain as "init"s. PCI is a bit more involved, and would collide with this series; Jesse's virtio-net config size issue is calling for converting VirtioDevice, which might be quicker. > instance_finalize -> @instance_free? /me misunderstandable, sorry. It doesn't free the instance either, and Java uses "finalize" too and so does .NET iirc. Anyway, my point was, when moving stuff out of exit, we should also change the signature to the new one - DeviceState* and (unused) Error**. Then we're getting closer to removing the old exit field, and at that point renaming individual hooks - if desired - becomes a trivial patch. Andreas -- 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 00/39] Delay destruction of memory regions to instance_finalize
On Wed, Jun 05, 2013 at 02:23:03PM +0200, Andreas Färber wrote: > Am 05.06.2013 14:06, schrieb Michael S. Tsirkin: > > On Wed, Jun 05, 2013 at 01:32:17PM +0200, Andreas Färber wrote: > >> Am 05.06.2013 13:10, schrieb Michael S. Tsirkin: > >>> On Wed, Jun 05, 2013 at 11:50:52AM +0200, Andreas Färber wrote: > Am 04.06.2013 20:51, schrieb Paolo Bonzini: > > This series changes all PCI devices (the sole to support hotplug > > _and_ use MemoryRegions) to do memory_region_del_subregion at > > unrealize time, and memory_region_destroy at instance_finalize > > time. > > The general idea looks good. > > Could you please follow-up with a patch that switches from exit to > unrealize? > >>> > >>> What do you guys think about changing the name to something > >>> else e.g. "free" or "destroy"? > >> > >> I'm not generally opposed to renaming things, but current unrealize is a > >> pair with realize, and destroy or free doesn't really fit it's purpose - > >> that's instance_finalize. Let's CC Anthony. > > > > So @instance_init -> instance_alloc > > No, allocation happens before instance_init, it only initializes fields > of the instance, so that name seems good to me. > > My ISA realize patches (need to respin after Paolo enabled gus) worked > towards resolving the DeviceClass::init vs. instance_init ambiguity, so > once completed only instance_init and class_init would remain as > "init"s. PCI is a bit more involved, and would collide with this series; > Jesse's virtio-net config size issue is calling for converting > VirtioDevice, which might be quicker. > > > instance_finalize -> @instance_free? > > /me misunderstandable, sorry. It doesn't free the instance either, and > Java uses "finalize" too and so does .NET iirc. Well the do not have initialize though, so if someone comes from .NET background that person will *still* be confused. I think we should use names that pair well and are not ambiguous: alloc/free create/destroy init/cleanup (some people do init/uninit) get/put ... These are all standard C things with no ambiguity. > Anyway, my point was, when moving stuff out of exit, we should also > change the signature to the new one - DeviceState* and (unused) Error**. > Then we're getting closer to removing the old exit field, and at that > point renaming individual hooks - if desired - becomes a trivial patch. > > Andreas Why is renaming new hooks related to getting rid of old ones? > -- > 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 00/39] Delay destruction of memory regions to instance_finalize
Am 05.06.2013 14:36, schrieb Michael S. Tsirkin: >> Anyway, my point was, when moving stuff out of exit, we should also >> change the signature to the new one - DeviceState* and (unused) Error**. >> Then we're getting closer to removing the old exit field, and at that >> point renaming individual hooks - if desired - becomes a trivial patch. > > Why is renaming new hooks related to getting rid of old ones? * less ambiguity and more names to choose from * introducing new callbacks as done here for instance_finalize requires care for variable names (PCIDevice *dev vs. DeviceState *dev is the classic) whereas renaming a hook once used is a trivial one-line change * renaming hooks now adds to the already existing confusion of a half-done conversion Andreas -- 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 00/39] Delay destruction of memory regions to instance_finalize
"Michael S. Tsirkin" writes: > On Wed, Jun 05, 2013 at 11:50:52AM +0200, Andreas Färber wrote: >> Am 04.06.2013 20:51, schrieb Paolo Bonzini: >> > This series changes all PCI devices (the sole to support hotplug >> > _and_ use MemoryRegions) to do memory_region_del_subregion at >> > unrealize time, and memory_region_destroy at instance_finalize >> > time. >> >> The general idea looks good. >> >> Could you please follow-up with a patch that switches from exit to >> unrealize? > > What do you guys think about changing the name to something > else e.g. "free" or "destroy"? exit/unrealize != free/destroy. You don't actually free anything. See 00/39 in this series for a precise description. > unrealize is not a word in english: > http://dictionary.cambridge.org/spellcheck/american-english/?q=unrealize English is a fluid language. I wouldn't worry too much about that. Regards, Anthony Liguori > I can do it easily if people agree. > >> use BUS(), PCI_DEVICE() etc. to hide this. >> >> Andreas >> >> -- >> 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 00/39] Delay destruction of memory regions to instance_finalize
On Wed, Jun 05, 2013 at 07:53:05AM -0500, Anthony Liguori wrote: > "Michael S. Tsirkin" writes: > > > On Wed, Jun 05, 2013 at 11:50:52AM +0200, Andreas Färber wrote: > >> Am 04.06.2013 20:51, schrieb Paolo Bonzini: > >> > This series changes all PCI devices (the sole to support hotplug > >> > _and_ use MemoryRegions) to do memory_region_del_subregion at > >> > unrealize time, and memory_region_destroy at instance_finalize > >> > time. > >> > >> The general idea looks good. > >> > >> Could you please follow-up with a patch that switches from exit to > >> unrealize? > > > > What do you guys think about changing the name to something > > else e.g. "free" or "destroy"? > > exit/unrealize != free/destroy. > > You don't actually free anything. See 00/39 in this series for a > precise description. That's where I got this. It says: "instance_finalize will reclaim the memory" > > http://dictionary.cambridge.org/spellcheck/american-english/?q=unrealize > > English is a fluid language. I wouldn't worry too much about that. > > Regards, > > Anthony Liguori Well I am not worried about English at all. I'm just confused by the function naming, and I think it can be improved. Can we have names actually say what a function is doing? There's no need to use ambiguous terms and then document what they mean. > > I can do it easily if people agree. > > > >> use BUS(), PCI_DEVICE() etc. to hide this. > >> > >> Andreas > >> > >> -- > >> 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 00/39] Delay destruction of memory regions to instance_finalize
Paolo Bonzini writes: > QOM splits the destruction of a device in two phases: > > - unrealize, also known as "exit" from qdev times, should isolate > the device from the guest. After unrealize returns, the guest > should not be able to issue new requests. > > - instance_finalize will reclaim the memory. This is only called > after all requests terminate and drop the references on the > device. Does it make sense to QOM-ify the memory regions? If they were QOM objects, they could be added as children and then the reaping would be handled automatically. Could be a follow up. Regards, Anthony Liguori > > This is important even now, but overlooked. It will become much > more important for devices that will be able to access memory > out of the big QEMU lock. > > This series changes all PCI devices (the sole to support hotplug > _and_ use MemoryRegions) to do memory_region_del_subregion at > unrealize time, and memory_region_destroy at instance_finalize > time. > > This is mostly a PCI patch, and independent from the others, > so I believe it should go through mst's tree. > > Paolo > > > Paolo Bonzini (39): > scsi: keep device alive while it has requests > dma: keep a device alive while it has SGLists > pci: split exit and finalize > ac97: use instance_finalize instead of exit > es1370: use instance_finalize instead of exit > hda: use instance_finalize instead of exit > serial: use instance_finalize instead of exit > tpci200: use instance_finalize instead of exit > pci-assign: use instance_finalize instead of exit > ahci: use instance_finalize instead of exit > msix: split msix_free from msix_uninit > cmd646: use instance_finalize instead of exit > ide/piix: use instance_finalize instead of exit > ide/via: use instance_finalize instead of exit > ivshmem: use instance_finalize instead of exit > pci-testdev: use instance_finalize instead of exit > vfio: use instance_finalize instead of exit > e1000: use instance_finalize instead of exit > eepro100: use instance_finalize instead of exit > ne2000: use instance_finalize instead of exit > pcnet: use instance_finalize instead of exit > rtl8139: use instance_finalize instead of exit > vmxnet3: use instance_finalize instead of exit > shpc: use instance_finalize instead of exit > pci_bridge: split pci_bridge_exitfn from pci_bridge_free > pcie_aer: pcie_aer_exit really frees stuff > pci_bridge: use instance_finalize instead of exit > ioh4320: use instance_finalize instead of exit > xio3130-downstream: use instance_finalize instead of exit > xio3130-upstream: use instance_finalize instead of exit > pcie: do not recreate mmcfg I/O region, use an alias instead > esp: use instance_finalize instead of exit > lsi: use instance_finalize instead of exit > pvscsi: use instance_finalize instead of exit > usb-uhci: use instance_finalize instead of exit > virtio-pci: use instance_finalize instead of exit > wdt_i6300esb: use instance_finalize instead of exit > xen_pt: use instance_finalize instead of exit > tpm: move add/del_subregion to realize/unrealize > > dma-helpers.c | 6 - > hw/audio/ac97.c| 5 ++-- > hw/audio/es1370.c | 5 ++-- > hw/audio/intel-hda.c | 8 ++ > hw/char/serial-pci.c | 24 ++ > hw/char/tpci200.c | 5 ++-- > hw/i386/kvm/pci-assign.c | 8 ++ > hw/ide/ahci.c | 5 ++-- > hw/ide/ahci.h | 2 +- > hw/ide/cmd646.c| 5 ++-- > hw/ide/ich.c | 13 +++--- > hw/ide/macio.c | 4 +-- > hw/ide/piix.c | 8 +++--- > hw/ide/via.c | 5 ++-- > hw/misc/ivshmem.c | 10 +++- > hw/misc/pci-testdev.c | 5 ++-- > hw/misc/vfio.c | 52 > +++--- > hw/net/e1000.c | 5 ++-- > hw/net/eepro100.c | 5 ++-- > hw/net/ne2000.c| 5 ++-- > hw/net/pcnet-pci.c | 5 ++-- > hw/net/rtl8139.c | 5 ++-- > hw/net/vmxnet3.c | 14 -- > hw/pci-bridge/i82801b11.c | 1 + > hw/pci-bridge/ioh3420.c| 11 +++- > hw/pci-bridge/pci_bridge_dev.c | 14 +- > hw/pci-bridge/xio3130_downstream.c | 11 +++- > hw/pci-bridge/xio3130_upstream.c | 11 +++- > hw/pci/msix.c | 26 ++- > hw/pci/pci.c | 15 --- > hw/pci/pci_bridge.c| 5 > hw/pci/pcie_aer.c | 3 ++- > hw/pci/pcie_host.c | 22 > hw/pci/shpc.c | 8 +- > hw/scsi/esp-pci.c | 5 ++-- > hw/scsi/lsi53c895a.c | 5 ++-- > hw/scsi/megasas.
Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize
On Wed, Jun 05, 2013 at 10:33:05AM -0500, Anthony Liguori wrote: > "Michael S. Tsirkin" writes: > > > On Wed, Jun 05, 2013 at 07:53:05AM -0500, Anthony Liguori wrote: > >> "Michael S. Tsirkin" writes: > >> > >> > On Wed, Jun 05, 2013 at 11:50:52AM +0200, Andreas Färber wrote: > >> >> Am 04.06.2013 20:51, schrieb Paolo Bonzini: > >> >> > This series changes all PCI devices (the sole to support hotplug > >> >> > _and_ use MemoryRegions) to do memory_region_del_subregion at > >> >> > unrealize time, and memory_region_destroy at instance_finalize > >> >> > time. > >> >> > >> >> The general idea looks good. > >> >> > >> >> Could you please follow-up with a patch that switches from exit to > >> >> unrealize? > >> > > >> > What do you guys think about changing the name to something > >> > else e.g. "free" or "destroy"? > >> > >> exit/unrealize != free/destroy. > >> > >> You don't actually free anything. See 00/39 in this series for a > >> precise description. > > > > That's where I got this. It says: > > "instance_finalize will reclaim the memory" > > I'm not sure what you're talking about. > > There are two callbacks: exit and instance_finalize. > > exit() is called to disconnect the device to the guest. Andreas is > proposing renaming it to unrealize. > > Ideally, reset() would be implemented as a combination of calls to > unrealize() + realize() (or exit() + init() as they are named today). > > Nothing should be freed in exit. > > instance_finalize is only called once, before the object is freed(). It > should be used to release resources associated with the object. > > Paolo is using this to destroy memory regions. Normally this callback > is never used because child objects are automatically handled by QOM. > > "finalize" is the standard name of the hook that gets called before > garbage collection as Andreas previously pointed out. I am simply asking for function names to tell us what they do, not when they are called. > It's this way in > Java, Python, C#, etc. Well bringing in idioms from more languages just serves to confuse readers even more. > Regards, > > Anthony Liguori > > > > >> > http://dictionary.cambridge.org/spellcheck/american-english/?q=unrealize > >> > >> English is a fluid language. I wouldn't worry too much about that. > >> > >> Regards, > >> > >> Anthony Liguori > > > > Well I am not worried about English at all. > > I'm just confused by the function naming, and > > I think it can be improved. > > > > Can we have names actually say what a function is > > doing? There's no need to use ambiguous terms and then > > document what they mean. > > > > > >> > I can do it easily if people agree. > >> > > >> >> use BUS(), PCI_DEVICE() etc. to hide this. > >> >> > >> >> Andreas > >> >> > >> >> -- > >> >> 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 00/39] Delay destruction of memory regions to instance_finalize
"Michael S. Tsirkin" writes: > On Wed, Jun 05, 2013 at 07:53:05AM -0500, Anthony Liguori wrote: >> "Michael S. Tsirkin" writes: >> >> > On Wed, Jun 05, 2013 at 11:50:52AM +0200, Andreas Färber wrote: >> >> Am 04.06.2013 20:51, schrieb Paolo Bonzini: >> >> > This series changes all PCI devices (the sole to support hotplug >> >> > _and_ use MemoryRegions) to do memory_region_del_subregion at >> >> > unrealize time, and memory_region_destroy at instance_finalize >> >> > time. >> >> >> >> The general idea looks good. >> >> >> >> Could you please follow-up with a patch that switches from exit to >> >> unrealize? >> > >> > What do you guys think about changing the name to something >> > else e.g. "free" or "destroy"? >> >> exit/unrealize != free/destroy. >> >> You don't actually free anything. See 00/39 in this series for a >> precise description. > > That's where I got this. It says: > "instance_finalize will reclaim the memory" I'm not sure what you're talking about. There are two callbacks: exit and instance_finalize. exit() is called to disconnect the device to the guest. Andreas is proposing renaming it to unrealize. Ideally, reset() would be implemented as a combination of calls to unrealize() + realize() (or exit() + init() as they are named today). Nothing should be freed in exit. instance_finalize is only called once, before the object is freed(). It should be used to release resources associated with the object. Paolo is using this to destroy memory regions. Normally this callback is never used because child objects are automatically handled by QOM. "finalize" is the standard name of the hook that gets called before garbage collection as Andreas previously pointed out. It's this way in Java, Python, C#, etc. Regards, Anthony Liguori > >> > http://dictionary.cambridge.org/spellcheck/american-english/?q=unrealize >> >> English is a fluid language. I wouldn't worry too much about that. >> >> Regards, >> >> Anthony Liguori > > Well I am not worried about English at all. > I'm just confused by the function naming, and > I think it can be improved. > > Can we have names actually say what a function is > doing? There's no need to use ambiguous terms and then > document what they mean. > > >> > I can do it easily if people agree. >> > >> >> use BUS(), PCI_DEVICE() etc. to hide this. >> >> >> >> Andreas >> >> >> >> -- >> >> 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 00/39] Delay destruction of memory regions to instance_finalize
On 5 June 2013 16:44, Michael S. Tsirkin wrote: > On Wed, Jun 05, 2013 at 10:33:05AM -0500, Anthony Liguori wrote: >> "finalize" is the standard name of the hook that gets called before >> garbage collection as Andreas previously pointed out. > > I am simply asking for function names to tell us > what they do, not when they are called. These functions are object lifecycle ones -- "when they are called" is exactly the key information about what they do. -- PMM
Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize
Il 05/06/2013 05:50, Andreas Färber ha scritto:
> Am 04.06.2013 20:51, schrieb Paolo Bonzini:
>> This series changes all PCI devices (the sole to support hotplug
>> _and_ use MemoryRegions) to do memory_region_del_subregion at
>> unrealize time, and memory_region_destroy at instance_finalize
>> time.
>
> The general idea looks good.
>
> Could you please follow-up with a patch that switches from exit to
> unrealize?
I can add it to the queue, but I have at least 4 pending series.
> Also I notice some patches are accessing parent fields directly - please
> use BUS(), PCI_DEVICE() etc. to hide this.
I'm always using them. For example:
+static void intel_hda_instance_finalize(Object *obj)
+{
+PCIDevice *pci = PCI_DEVICE(obj);
+IntelHDAState *d = DO_UPCAST(IntelHDAState, pci, pci);
What I'm not doing, is adding new cast macros---one thing at a time.
Paolo
Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize
Am 07.06.2013 03:03, schrieb Paolo Bonzini:
> Il 05/06/2013 05:50, Andreas Färber ha scritto:
>> Also I notice some patches are accessing parent fields directly - please
>> use BUS(), PCI_DEVICE() etc. to hide this.
>
> I'm always using them. For example:
>
> +static void intel_hda_instance_finalize(Object *obj)
> +{
> +PCIDevice *pci = PCI_DEVICE(obj);
> +IntelHDAState *d = DO_UPCAST(IntelHDAState, pci, pci);
I'll comment inline then. :)
Andreas
--
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 00/39] Delay destruction of memory regions to instance_finalize
Hi,
On Jun 7, 2013 11:04 AM, "Paolo Bonzini" wrote:
>
> Il 05/06/2013 05:50, Andreas Färber ha scritto:
> > Am 04.06.2013 20:51, schrieb Paolo Bonzini:
> >> This series changes all PCI devices (the sole to support hotplug
> >> _and_ use MemoryRegions) to do memory_region_del_subregion at
> >> unrealize time, and memory_region_destroy at instance_finalize
> >> time.
> >
> > The general idea looks good.
> >
> > Could you please follow-up with a patch that switches from exit to
> > unrealize?
>
> I can add it to the queue, but I have at least 4 pending series.
>
> > Also I notice some patches are accessing parent fields directly - please
> > use BUS(), PCI_DEVICE() etc. to hide this.
>
> I'm always using them. For example:
>
> +static void intel_hda_instance_finalize(Object *obj)
> +{
> +PCIDevice *pci = PCI_DEVICE(obj);
> +IntelHDAState *d = DO_UPCAST(IntelHDAState, pci, pci);
>
> What I'm not doing, is adding new cast macros---one thing at a time.
>
I have a series that fixes all qom cast macros for all PCI devices tree
wide. Can post. Qom cast macros added as needed.
How are you regression testing this series? If you have a pc/PCI regression
suite I could use it for my series.
Regards
Peter
> Paolo
>
On Jun 7, 2013 11:04 AM, "Paolo Bonzini" wrote:
> Il 05/06/2013 05:50, Andreas Färber ha scritto:
> > Am 04.06.2013 20:51, schrieb Paolo Bonzini:
> >> This series changes all PCI devices (the sole to support hotplug
> >> _and_ use MemoryRegions) to do memory_region_del_subregion at
> >> unrealize time, and memory_region_destroy at instance_finalize
> >> time.
> >
> > The general idea looks good.
> >
> > Could you please follow-up with a patch that switches from exit to
> > unrealize?
>
> I can add it to the queue, but I have at least 4 pending series.
>
> > Also I notice some patches are accessing parent fields directly - please
> > use BUS(), PCI_DEVICE() etc. to hide this.
>
> I'm always using them. For example:
>
> +static void intel_hda_instance_finalize(Object *obj)
> +{
> +PCIDevice *pci = PCI_DEVICE(obj);
> +IntelHDAState *d = DO_UPCAST(IntelHDAState, pci, pci);
>
> What I'm not doing, is adding new cast macros---one thing at a time.
>
> Paolo
>
>
Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize
Il 07/06/2013 03:45, Andreas Färber ha scritto:
> Am 07.06.2013 03:03, schrieb Paolo Bonzini:
>> Il 05/06/2013 05:50, Andreas Färber ha scritto:
>>> Also I notice some patches are accessing parent fields directly - please
>>> use BUS(), PCI_DEVICE() etc. to hide this.
>>
>> I'm always using them. For example:
>>
>> +static void intel_hda_instance_finalize(Object *obj)
>> +{
>> +PCIDevice *pci = PCI_DEVICE(obj);
>> +IntelHDAState *d = DO_UPCAST(IntelHDAState, pci, pci);
>
> I'll comment inline then. :)
Ah right, in those cases I was mostly doing what the
adjacent/pre-existing code did. But I can definitely fix them up!
Paolo
Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize
Hi Peter, Am 07.06.2013 10:41, schrieb Peter Crosthwaite: > I have a series that fixes all qom cast macros for all PCI devices tree > wide. Can post. Qom cast macros added as needed. Sounds promising! I just CC'ed you on my ISA series v2, which touches on PCI_BUS() in the final patch, dropping FROM_QBUS(). > How are you regression testing this series? If you have a pc/PCI > regression suite I could use it for my series. I rely on `make check` to find the most obvious QOM errors. libqos PCI support is still very new, so we don't have full device coverage there yet. Usually I run an openSUSE and/or SLES guest under KVM (pick your favorite), which will exercise the relevant init, realize and reset code paths at least. And finally I re-review things in gitk before sending - not the nicest tool but convenient for fixing up things and verifying quickly. HTH, Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
