| Message ID | 20191018150630.31099-7-damien.hedde@greensocs.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Multi-phase reset mechanism | expand |
On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@greensocs.com> wrote: > > In qdev_set_parent_bus(), when changing the parent bus of a > realized device, if the source and destination buses are not in the > same reset state, some adaptation are required. This patch adds "adaptations" > needed call to resettable_change_parent() to make sure a device reset > state stays coherent with its parent bus. > > The addition is a no-op if: > 1. the device being parented is not realized. > 2. the device is realized, but both buses are not under reset. > > Case 2 means that as long as qdev_set_parent_bus() is called > during the machine realization procedure (which is before the > machine reset so nothing is in reset), it is a no op. > > There are 49 call sites of qdev_set_parent_bus(). All but one fall > into the no-op case: > + 28 calls related to virtio (in hw/{s390x,display,virtio}/ > {vhost,virtio}-xxx.c) to set a _vdev_/_vgpu_ composing device > parent bus just before realizing the _vdev_/_vgpu_. > + hw/qdev.c: when creating a device in qdev_try_create() > + hw/sysbus.c: when initializing a device in the sysbus > + hw/display/virtio-gpu-pci.c: before realizing VirtIOGPUPCIBase/vgpu > + hw/display/virtio-vga.c: before realizing VirtIOVGABase/vgpu > + hw/i386/amd_iommu.c: before realizing AMDVIState/pci > + hw/misc/auxbus.c: when creating an AUXBus > + hw/misc/auxbus.c: when creating an AUXBus child > + hw/misc/macio/macio.c: when initializing a MACIOState child > + hw/misc/macio/macio.c: before realizing NewWorldMacIOState/pmu > + hw/misc/macio/macio.c: before realizing NewWorldMacIOState/cuda > + hw/pci-host/designware.c: before realizing DesignwarePCIEHost/root > + hw/pci-host/gpex.c: before realizing GPEXHost/root > + hw/pci-host/prep.c: when initializaing PREPPCIState/pci_dev > + hw/pci-host/q35.c: before realizing Q35PCIHost/mch > + hw/pci-host/versatile.c: when initializing PCIVPBState/pci_dev > + hw/pci-host/xilinx-pcie.c: before realizing XilinxPCIEHost/root > + hw/s390x/event-facility.c: when creating SCLPEventFacility/ > TYPE_SCLP_QUIESCE > + hw/s390x/event-facility.c: ditto with SCLPEventFacility/ > TYPE_SCLP_CPU_HOTPLUG > + hw/s390x/sclp.c: Not trivial because it is called on a SLCPDevice > just after realizing it. Ok because at this point the destination > bus (sysbus) is not in reset; the realize step is before the > machine reset. > + hw/sd/core.c: Not OK. Used in sdbus_reparent_card(). See below. > + hw/ssi/ssi.c: Used to put spi slave on spi bus and connect the cs > line in ssi_auto_connect_slave(). Ok because this function is only > used in realize step in hw/ssi/aspeed_smc.ci, hw/ssi/imx_spi.c, > hw/ssi/mss-spi.c, hw/ssi/xilinx_spi.c and hw/ssi/xilinx_spips.c. > + hw/xen/xen-legacy-backend.c: when creating a XenLegacyDevice device > + qdev-monitor.c: in device hotplug creation procedure before realize This is a really useful analysis to have in the commit message; thanks! (Side note: I wonder if the sclp.c case could be reordered so it realizes the device after parenting it? Anyway, not something to worry about now.) > Note that this commit alone will have no effect, right now there is no > use of resettable API to reset anything. So a bus will never be tagged > as in-reset by this same API. > > The one place where side-effect will occurs is in hw/sd/core.c in > sdbus_reparent_card(). This function is only used in the raspi machines, > including during the sysbus reset procedure. This case will be fixed by > a following commit before globally enabling resettable API for sysbus > reset. > > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
On Fri, 29 Nov 2019 18:41:26 +0000 Peter Maydell <peter.maydell@linaro.org> wrote: > On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@greensocs.com> wrote: > > > > In qdev_set_parent_bus(), when changing the parent bus of a > > realized device, if the source and destination buses are not in the > > same reset state, some adaptation are required. This patch adds > > "adaptations" > > > needed call to resettable_change_parent() to make sure a device reset > > state stays coherent with its parent bus. > > > > The addition is a no-op if: > > 1. the device being parented is not realized. > > 2. the device is realized, but both buses are not under reset. > > > > Case 2 means that as long as qdev_set_parent_bus() is called > > during the machine realization procedure (which is before the > > machine reset so nothing is in reset), it is a no op. > > > > There are 49 call sites of qdev_set_parent_bus(). All but one fall > > into the no-op case: > > + 28 calls related to virtio (in hw/{s390x,display,virtio}/ > > {vhost,virtio}-xxx.c) to set a _vdev_/_vgpu_ composing device > > parent bus just before realizing the _vdev_/_vgpu_. > > + hw/qdev.c: when creating a device in qdev_try_create() > > + hw/sysbus.c: when initializing a device in the sysbus > > + hw/display/virtio-gpu-pci.c: before realizing VirtIOGPUPCIBase/vgpu > > + hw/display/virtio-vga.c: before realizing VirtIOVGABase/vgpu > > + hw/i386/amd_iommu.c: before realizing AMDVIState/pci > > + hw/misc/auxbus.c: when creating an AUXBus > > + hw/misc/auxbus.c: when creating an AUXBus child > > + hw/misc/macio/macio.c: when initializing a MACIOState child > > + hw/misc/macio/macio.c: before realizing NewWorldMacIOState/pmu > > + hw/misc/macio/macio.c: before realizing NewWorldMacIOState/cuda > > + hw/pci-host/designware.c: before realizing DesignwarePCIEHost/root > > + hw/pci-host/gpex.c: before realizing GPEXHost/root > > + hw/pci-host/prep.c: when initializaing PREPPCIState/pci_dev > > + hw/pci-host/q35.c: before realizing Q35PCIHost/mch > > + hw/pci-host/versatile.c: when initializing PCIVPBState/pci_dev > > + hw/pci-host/xilinx-pcie.c: before realizing XilinxPCIEHost/root > > + hw/s390x/event-facility.c: when creating SCLPEventFacility/ > > TYPE_SCLP_QUIESCE > > + hw/s390x/event-facility.c: ditto with SCLPEventFacility/ > > TYPE_SCLP_CPU_HOTPLUG > > + hw/s390x/sclp.c: Not trivial because it is called on a SLCPDevice > > just after realizing it. Ok because at this point the destination > > bus (sysbus) is not in reset; the realize step is before the > > machine reset. > > + hw/sd/core.c: Not OK. Used in sdbus_reparent_card(). See below. > > + hw/ssi/ssi.c: Used to put spi slave on spi bus and connect the cs > > line in ssi_auto_connect_slave(). Ok because this function is only > > used in realize step in hw/ssi/aspeed_smc.ci, hw/ssi/imx_spi.c, > > hw/ssi/mss-spi.c, hw/ssi/xilinx_spi.c and hw/ssi/xilinx_spips.c. > > + hw/xen/xen-legacy-backend.c: when creating a XenLegacyDevice device > > + qdev-monitor.c: in device hotplug creation procedure before realize > > This is a really useful analysis to have in the commit message; > thanks! > > (Side note: I wonder if the sclp.c case could be reordered so > it realizes the device after parenting it? Anyway, not something > to worry about now.) As far as I can see, that should work. This code is a bit weird anyway; the problem is that we need the sysbus somewhere in there... I'm wondering if that can be handled in a different way. But agreed, that is something we can revisit later. > > > Note that this commit alone will have no effect, right now there is no > > use of resettable API to reset anything. So a bus will never be tagged > > as in-reset by this same API. > > > > The one place where side-effect will occurs is in hw/sd/core.c in > > sdbus_reparent_card(). This function is only used in the raspi machines, > > including during the sysbus reset procedure. This case will be fixed by > > a following commit before globally enabling resettable API for sysbus > > reset. > > > > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > > thanks > -- PMM >
diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 81784c73fb..3933f62d0c 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -96,23 +96,29 @@ static void bus_add_child(BusState *bus, DeviceState *child) void qdev_set_parent_bus(DeviceState *dev, BusState *bus) { - bool replugging = dev->parent_bus != NULL; + BusState *old_parent_bus = dev->parent_bus; - if (replugging) { + if (old_parent_bus) { trace_qdev_update_parent_bus(dev, dev->parent_bus, bus); /* * Keep a reference to the device while it's not plugged into * any bus, to avoid it potentially evaporating when it is * dereffed in bus_remove_child(). + * Also keep the ref of the parent bus until the end, so that + * we can safely call resettable_change_parent() below. */ object_ref(OBJECT(dev)); bus_remove_child(dev->parent_bus, dev); - object_unref(OBJECT(dev->parent_bus)); } dev->parent_bus = bus; object_ref(OBJECT(bus)); bus_add_child(bus, dev); - if (replugging) { + if (dev->realized) { + resettable_change_parent(OBJECT(dev), OBJECT(bus), + OBJECT(old_parent_bus)); + } + if (old_parent_bus) { + object_unref(OBJECT(old_parent_bus)); object_unref(OBJECT(dev)); } }
In qdev_set_parent_bus(), when changing the parent bus of a realized device, if the source and destination buses are not in the same reset state, some adaptation are required. This patch adds needed call to resettable_change_parent() to make sure a device reset state stays coherent with its parent bus. The addition is a no-op if: 1. the device being parented is not realized. 2. the device is realized, but both buses are not under reset. Case 2 means that as long as qdev_set_parent_bus() is called during the machine realization procedure (which is before the machine reset so nothing is in reset), it is a no op. There are 49 call sites of qdev_set_parent_bus(). All but one fall into the no-op case: + 28 calls related to virtio (in hw/{s390x,display,virtio}/ {vhost,virtio}-xxx.c) to set a _vdev_/_vgpu_ composing device parent bus just before realizing the _vdev_/_vgpu_. + hw/qdev.c: when creating a device in qdev_try_create() + hw/sysbus.c: when initializing a device in the sysbus + hw/display/virtio-gpu-pci.c: before realizing VirtIOGPUPCIBase/vgpu + hw/display/virtio-vga.c: before realizing VirtIOVGABase/vgpu + hw/i386/amd_iommu.c: before realizing AMDVIState/pci + hw/misc/auxbus.c: when creating an AUXBus + hw/misc/auxbus.c: when creating an AUXBus child + hw/misc/macio/macio.c: when initializing a MACIOState child + hw/misc/macio/macio.c: before realizing NewWorldMacIOState/pmu + hw/misc/macio/macio.c: before realizing NewWorldMacIOState/cuda + hw/pci-host/designware.c: before realizing DesignwarePCIEHost/root + hw/pci-host/gpex.c: before realizing GPEXHost/root + hw/pci-host/prep.c: when initializaing PREPPCIState/pci_dev + hw/pci-host/q35.c: before realizing Q35PCIHost/mch + hw/pci-host/versatile.c: when initializing PCIVPBState/pci_dev + hw/pci-host/xilinx-pcie.c: before realizing XilinxPCIEHost/root + hw/s390x/event-facility.c: when creating SCLPEventFacility/ TYPE_SCLP_QUIESCE + hw/s390x/event-facility.c: ditto with SCLPEventFacility/ TYPE_SCLP_CPU_HOTPLUG + hw/s390x/sclp.c: Not trivial because it is called on a SLCPDevice just after realizing it. Ok because at this point the destination bus (sysbus) is not in reset; the realize step is before the machine reset. + hw/sd/core.c: Not OK. Used in sdbus_reparent_card(). See below. + hw/ssi/ssi.c: Used to put spi slave on spi bus and connect the cs line in ssi_auto_connect_slave(). Ok because this function is only used in realize step in hw/ssi/aspeed_smc.ci, hw/ssi/imx_spi.c, hw/ssi/mss-spi.c, hw/ssi/xilinx_spi.c and hw/ssi/xilinx_spips.c. + hw/xen/xen-legacy-backend.c: when creating a XenLegacyDevice device + qdev-monitor.c: in device hotplug creation procedure before realize Note that this commit alone will have no effect, right now there is no use of resettable API to reset anything. So a bus will never be tagged as in-reset by this same API. The one place where side-effect will occurs is in hw/sd/core.c in sdbus_reparent_card(). This function is only used in the raspi machines, including during the sysbus reset procedure. This case will be fixed by a following commit before globally enabling resettable API for sysbus reset. Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> --- Exhaustive list of the 28 virtio caller to qdev_set_parent_bus(): + hw/display/virtio-gpu-pci.c -> VirtIOGPUPCIBase/vgpu realize + hw/display_virtio-vga.c -> VirtIOVGABase/vgpu realize + hw/s390x/vhost-vsock-ccw.c -> VHostVSockCCWState/vdev realize + hw/s390x/virtio-ccw-9p.c -> V9fsCCWState/vdev realize + hw/s390x/virtio-ccw-balloon.c -> VirtIOBalloonCcw/vdev realize + hw/s390x/virtio-ccw-blk.c -> VirtIOBlkCcw/vdev realize + hw/s390x/virtio-ccw-crypto.c -> VirtIOCryptoCcw/vdev realize + hw/s390x/virtio-ccw-gpu.c -> VirtIOGPUCcw/vdev realize + hw/s390x/virtio-ccw-input.c -> VirtIOInputCcw/vdev realize + hw/s390x/virtio-ccw-net.c -> VirtIONetCcw/vdev realize + hw/s390x/virtio-ccw-rng.c -> VirtIORNGCcw/vdev realize + hw/s390x/virtio-ccw-scsi.c -> VirtIOSCSICcw/vdev realize + hw/s390x/virtio-ccw-scsi.c -> VHostSCSICcw/vdev realize + hw/s390x/virtio-ccw-serial.c -> VirtioSerialCcw/vdev realize + hw/virtio/vhost-scsi-pci.c -> VHostSCSIPCI/vdev realize + hw/virtio/vhost-user-blk-pci.c -> VHostUserBlkPCI/vdev realize + hw/virtio/vhost-user-scsi-pci.c -> VHostUserSCSIPCI/vdev realize + hw/virtio/vhost-vsock-pci.c -> VHostVSockPCI/vdev realize + hw/virtio/virtio-9p-pci.c -> V9fsPCIState/vdev realize + hw/virtio/virtio-balloon-pci.c -> VirtIOBalloonPCI/vdev realize + hw/virtio/virtio-blk-pci.c -> VirtIOBlkPCI/vdev realize + hw/virtio/virtio-crypto-pci.c -> VirtIOCryptoPCI/vdev realize + hw/virtio/virtio-input-pci.c -> VirtIOInputPCI/vdev realize + hw/virtio/virtio-net-pci.c -> VirtIONetPCI/vdev realize + hw/virtio/virtio-pmem-pci.c -> VirtIOPMEMPCI/vdev realize + hw/virtio/virtio-rng-pci.c -> VirtIORngPCI/vdev realize + hw/virtio/virtio-scsi-pci.c -> VirtIOSCSIPCI/vdev realize + hw/virtio/virtio-serial-pci.c -> VirtIOSerialPCI/vdev realize --- hw/core/qdev.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)