| Message ID | 20190130155733.32742-7-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x/pci: remaining hot/un)plug patches | expand |
On 1/30/19 10:57 AM, David Hildenbrand wrote: > When resetting the guest we should unplug and remove all devices that > are still pending. > > With this patch, the requested device will be unpluged on reboot > (S390_RESET_EXTERNAL and S390_RESET_REIPL, which reset the pcihost bridge > via qemu_devices_reset()). > > This approach is similar to what's done for acpi PCI hotplug in > acpi_pcihp_reset() -> acpi_pcihp_update() -> > acpi_pcihp_update_hotplug_bus() -> acpi_pcihp_eject_slot(). > > s390_pci_generate_plug_event()'s will still be generated, I guess this > is not an issue. The same thing would happen right now when unplugging > a device just before starting the guest. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/s390x/s390-pci-bus.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c > index 867801ccf9..b9b0f44087 100644 > --- a/hw/s390x/s390-pci-bus.c > +++ b/hw/s390x/s390-pci-bus.c > @@ -1092,6 +1092,21 @@ static void s390_pcihost_reset(DeviceState *dev) > { > S390pciState *s = S390_PCI_HOST_BRIDGE(dev); > PCIBus *bus = s->parent_obj.bus; > + S390PCIBusDevice *pbdev, *next; > + > + /* Process all pending unplug requests */ > + QTAILQ_FOREACH_SAFE(pbdev, &s->zpci_devs, link, next) { > + if (pbdev->unplug_requested) { > + if (pbdev->summary_ind) { > + pci_dereg_irqs(pbdev); > + } > + if (pbdev->iommu->enabled) { > + pci_dereg_ioat(pbdev->iommu); > + } > + pbdev->state = ZPCI_FS_STANDBY; > + s390_pci_perform_unplug(pbdev); > + } > + } > > /* > * When resetting a PCI bridge, the assigned numbers are set to 0. So > Just "talking out loud" to reaffirm my understanding: The ..._unplug_request function will have already generated the deconfigure unplug event, and the unplug_requested flag will be set. Makes sense -- easy to follow. :) The callback that this code is essentially replacing used to generate a CONFIGURED_TO_STBRES plug event, but as I recall from a previous conversation regarding PCI states, we can skip this event because we're removing the device from the guest during a reset here (there are a few reasons why we'd do this, but no sense in listing them here). Also makes sense to me. Reviewed-by: Collin Walling <walling@linux.ibm.com>
On 31.01.19 21:26, Collin Walling wrote: > On 1/30/19 10:57 AM, David Hildenbrand wrote: >> When resetting the guest we should unplug and remove all devices that >> are still pending. >> >> With this patch, the requested device will be unpluged on reboot >> (S390_RESET_EXTERNAL and S390_RESET_REIPL, which reset the pcihost bridge >> via qemu_devices_reset()). >> >> This approach is similar to what's done for acpi PCI hotplug in >> acpi_pcihp_reset() -> acpi_pcihp_update() -> >> acpi_pcihp_update_hotplug_bus() -> acpi_pcihp_eject_slot(). >> >> s390_pci_generate_plug_event()'s will still be generated, I guess this >> is not an issue. The same thing would happen right now when unplugging >> a device just before starting the guest. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> hw/s390x/s390-pci-bus.c | 15 +++++++++++++++ >> 1 file changed, 15 insertions(+) >> >> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c >> index 867801ccf9..b9b0f44087 100644 >> --- a/hw/s390x/s390-pci-bus.c >> +++ b/hw/s390x/s390-pci-bus.c >> @@ -1092,6 +1092,21 @@ static void s390_pcihost_reset(DeviceState *dev) >> { >> S390pciState *s = S390_PCI_HOST_BRIDGE(dev); >> PCIBus *bus = s->parent_obj.bus; >> + S390PCIBusDevice *pbdev, *next; >> + >> + /* Process all pending unplug requests */ >> + QTAILQ_FOREACH_SAFE(pbdev, &s->zpci_devs, link, next) { >> + if (pbdev->unplug_requested) { >> + if (pbdev->summary_ind) { >> + pci_dereg_irqs(pbdev); >> + } >> + if (pbdev->iommu->enabled) { >> + pci_dereg_ioat(pbdev->iommu); >> + } >> + pbdev->state = ZPCI_FS_STANDBY; >> + s390_pci_perform_unplug(pbdev); >> + } >> + } >> >> /* >> * When resetting a PCI bridge, the assigned numbers are set to 0. So >> > > Just "talking out loud" to reaffirm my understanding: > > The ..._unplug_request function will have already generated the > deconfigure unplug event, and the unplug_requested flag will be set. > Makes sense -- easy to follow. :) Yes, that's how it should work :) > > The callback that this code is essentially replacing used to generate a > CONFIGURED_TO_STBRES plug event, but as I recall from a previous > conversation regarding PCI states, we can skip this event because we're > removing the device from the guest during a reset here (there are a few > reasons why we'd do this, but no sense in listing them here). Also makes > sense to me. Yes, it's pretty much to the guest like this device never existed. Thanks! > > Reviewed-by: Collin Walling <walling@linux.ibm.com> >
On Wed, 30 Jan 2019 16:57:33 +0100 David Hildenbrand <david@redhat.com> wrote: > When resetting the guest we should unplug and remove all devices that > are still pending. > > With this patch, the requested device will be unpluged on reboot > (S390_RESET_EXTERNAL and S390_RESET_REIPL, which reset the pcihost bridge > via qemu_devices_reset()). > > This approach is similar to what's done for acpi PCI hotplug in > acpi_pcihp_reset() -> acpi_pcihp_update() -> > acpi_pcihp_update_hotplug_bus() -> acpi_pcihp_eject_slot(). > > s390_pci_generate_plug_event()'s will still be generated, I guess this > is not an issue. The same thing would happen right now when unplugging > a device just before starting the guest. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/s390x/s390-pci-bus.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c > index 867801ccf9..b9b0f44087 100644 > --- a/hw/s390x/s390-pci-bus.c > +++ b/hw/s390x/s390-pci-bus.c > @@ -1092,6 +1092,21 @@ static void s390_pcihost_reset(DeviceState *dev) > { > S390pciState *s = S390_PCI_HOST_BRIDGE(dev); > PCIBus *bus = s->parent_obj.bus; > + S390PCIBusDevice *pbdev, *next; > + > + /* Process all pending unplug requests */ > + QTAILQ_FOREACH_SAFE(pbdev, &s->zpci_devs, link, next) { > + if (pbdev->unplug_requested) { > + if (pbdev->summary_ind) { > + pci_dereg_irqs(pbdev); > + } > + if (pbdev->iommu->enabled) { > + pci_dereg_ioat(pbdev->iommu); > + } > + pbdev->state = ZPCI_FS_STANDBY; > + s390_pci_perform_unplug(pbdev); > + } > + } > > /* > * When resetting a PCI bridge, the assigned numbers are set to 0. So I'm afraid this one doesn't quite apply without the first patches in the series, and I'm not sure how to fix it up. I'll either take a rebase on a codebase without patches 1-3, or this patch together with patches 1-3 after they get acks (whatever comes first :)
On 01.02.19 11:19, Cornelia Huck wrote: > On Wed, 30 Jan 2019 16:57:33 +0100 > David Hildenbrand <david@redhat.com> wrote: > >> When resetting the guest we should unplug and remove all devices that >> are still pending. >> >> With this patch, the requested device will be unpluged on reboot >> (S390_RESET_EXTERNAL and S390_RESET_REIPL, which reset the pcihost bridge >> via qemu_devices_reset()). >> >> This approach is similar to what's done for acpi PCI hotplug in >> acpi_pcihp_reset() -> acpi_pcihp_update() -> >> acpi_pcihp_update_hotplug_bus() -> acpi_pcihp_eject_slot(). >> >> s390_pci_generate_plug_event()'s will still be generated, I guess this >> is not an issue. The same thing would happen right now when unplugging >> a device just before starting the guest. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> hw/s390x/s390-pci-bus.c | 15 +++++++++++++++ >> 1 file changed, 15 insertions(+) >> >> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c >> index 867801ccf9..b9b0f44087 100644 >> --- a/hw/s390x/s390-pci-bus.c >> +++ b/hw/s390x/s390-pci-bus.c >> @@ -1092,6 +1092,21 @@ static void s390_pcihost_reset(DeviceState *dev) >> { >> S390pciState *s = S390_PCI_HOST_BRIDGE(dev); >> PCIBus *bus = s->parent_obj.bus; >> + S390PCIBusDevice *pbdev, *next; >> + >> + /* Process all pending unplug requests */ >> + QTAILQ_FOREACH_SAFE(pbdev, &s->zpci_devs, link, next) { >> + if (pbdev->unplug_requested) { >> + if (pbdev->summary_ind) { >> + pci_dereg_irqs(pbdev); >> + } >> + if (pbdev->iommu->enabled) { >> + pci_dereg_ioat(pbdev->iommu); >> + } >> + pbdev->state = ZPCI_FS_STANDBY; >> + s390_pci_perform_unplug(pbdev); >> + } >> + } >> >> /* >> * When resetting a PCI bridge, the assigned numbers are set to 0. So > > I'm afraid this one doesn't quite apply without the first patches in > the series, and I'm not sure how to fix it up. I'll either take a > rebase on a codebase without patches 1-3, or this patch together with > patches 1-3 after they get acks (whatever comes first :) > I guess it's only the comment that get's added in patch #1, so no major conflicts.
On Fri, 1 Feb 2019 16:06:43 +0100 David Hildenbrand <david@redhat.com> wrote: > On 01.02.19 11:19, Cornelia Huck wrote: > > On Wed, 30 Jan 2019 16:57:33 +0100 > > David Hildenbrand <david@redhat.com> wrote: > > > >> When resetting the guest we should unplug and remove all devices that > >> are still pending. > >> > >> With this patch, the requested device will be unpluged on reboot s/unpluged/unplugged/ > >> (S390_RESET_EXTERNAL and S390_RESET_REIPL, which reset the pcihost bridge > >> via qemu_devices_reset()). > >> > >> This approach is similar to what's done for acpi PCI hotplug in > >> acpi_pcihp_reset() -> acpi_pcihp_update() -> > >> acpi_pcihp_update_hotplug_bus() -> acpi_pcihp_eject_slot(). > >> > >> s390_pci_generate_plug_event()'s will still be generated, I guess this > >> is not an issue. The same thing would happen right now when unplugging > >> a device just before starting the guest. > >> > >> Signed-off-by: David Hildenbrand <david@redhat.com> > >> --- > >> hw/s390x/s390-pci-bus.c | 15 +++++++++++++++ > >> 1 file changed, 15 insertions(+) > >> > >> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c > >> index 867801ccf9..b9b0f44087 100644 > >> --- a/hw/s390x/s390-pci-bus.c > >> +++ b/hw/s390x/s390-pci-bus.c > >> @@ -1092,6 +1092,21 @@ static void s390_pcihost_reset(DeviceState *dev) > >> { > >> S390pciState *s = S390_PCI_HOST_BRIDGE(dev); > >> PCIBus *bus = s->parent_obj.bus; > >> + S390PCIBusDevice *pbdev, *next; > >> + > >> + /* Process all pending unplug requests */ > >> + QTAILQ_FOREACH_SAFE(pbdev, &s->zpci_devs, link, next) { > >> + if (pbdev->unplug_requested) { > >> + if (pbdev->summary_ind) { > >> + pci_dereg_irqs(pbdev); > >> + } > >> + if (pbdev->iommu->enabled) { > >> + pci_dereg_ioat(pbdev->iommu); > >> + } > >> + pbdev->state = ZPCI_FS_STANDBY; > >> + s390_pci_perform_unplug(pbdev); > >> + } > >> + } > >> > >> /* > >> * When resetting a PCI bridge, the assigned numbers are set to 0. So > > > > I'm afraid this one doesn't quite apply without the first patches in > > the series, and I'm not sure how to fix it up. I'll either take a > > rebase on a codebase without patches 1-3, or this patch together with > > patches 1-3 after they get acks (whatever comes first :) > > > > I guess it's only the comment that get's added in patch #1, so no major > conflicts. As I'm now picking 1-3, this obviously fits without conflicts again :)
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c index 867801ccf9..b9b0f44087 100644 --- a/hw/s390x/s390-pci-bus.c +++ b/hw/s390x/s390-pci-bus.c @@ -1092,6 +1092,21 @@ static void s390_pcihost_reset(DeviceState *dev) { S390pciState *s = S390_PCI_HOST_BRIDGE(dev); PCIBus *bus = s->parent_obj.bus; + S390PCIBusDevice *pbdev, *next; + + /* Process all pending unplug requests */ + QTAILQ_FOREACH_SAFE(pbdev, &s->zpci_devs, link, next) { + if (pbdev->unplug_requested) { + if (pbdev->summary_ind) { + pci_dereg_irqs(pbdev); + } + if (pbdev->iommu->enabled) { + pci_dereg_ioat(pbdev->iommu); + } + pbdev->state = ZPCI_FS_STANDBY; + s390_pci_perform_unplug(pbdev); + } + } /* * When resetting a PCI bridge, the assigned numbers are set to 0. So
When resetting the guest we should unplug and remove all devices that are still pending. With this patch, the requested device will be unpluged on reboot (S390_RESET_EXTERNAL and S390_RESET_REIPL, which reset the pcihost bridge via qemu_devices_reset()). This approach is similar to what's done for acpi PCI hotplug in acpi_pcihp_reset() -> acpi_pcihp_update() -> acpi_pcihp_update_hotplug_bus() -> acpi_pcihp_eject_slot(). s390_pci_generate_plug_event()'s will still be generated, I guess this is not an issue. The same thing would happen right now when unplugging a device just before starting the guest. Signed-off-by: David Hildenbrand <david@redhat.com> --- hw/s390x/s390-pci-bus.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)