|
From: | Collin Walling |
Subject: | Re: [Qemu-devel] [qemu-s390x] [PATCH v3 2/2] s390x/pci: Unplug remaining devices on pcihost reset |
Date: | Mon, 28 Jan 2019 19:09:54 -0500 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 |
On 1/28/19 6:28 AM, Cornelia Huck wrote:
On Wed, 23 Jan 2019 12:05:39 +0100 Cornelia Huck <address@hidden> wrote:On Mon, 21 Jan 2019 14:42:49 +0100 David Hildenbrand <address@hidden> wrote:When resetting the guest we should unplug and remove all devices that are still pending. Otherwise the fresh guest will see devices that will suddenly vanish. Can be triggered e.g. via (hmp) device_add virtio-mouse-pci,id=test (hmp) stop (hmp) device_del test (hmp) system_reset (hmp) c The device will vanish after roughly 5 minutes. With this patch, the device will vanish on reboot (S390_RESET_EXTERNAL and S390_RESET_REIPL, which reset the pcihost bridge via qemu_devices_reset()). If we want these devices to vanish directly on any reset (S390_RESET_MODIFIED_CLEAR and S390_RESET_LOAD_NORMAL), we have to modify s390_machine_reset(). But I have the feeling that this should not be done for all reset types. 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 (same thing could happen right now if the timer expires just after reset).I'm wondering what the architecture says regarding those events -- can someone with access to the documentation comment?Ping. Any comments from the IBM folks?
So the idea here is that if we have a PCI device that is the process of being deconfigured and we are also in the middle of a reset, then let's accelerate deconfiguring of the PCI device during the reset. Makes sense.
Note:The callback function will deconfigure the the device and put it into standby mode. However, a PCI device should only go into standby from the *disabled state* (which it could already be in due to the unplug sequence), or from a *permanent error state* (something we should hopefully never see -- this means something went seriously wrong with the device).
Two things I'm concerned about: 1)What I would suggest is adding a check for the pbdev->state for ZPCI_FS_DISABLED || ZPCI_FS_PERMANENT_ERROR. If it is in either of these states, then we're safe to deconfigure and put into standby. If the device is still in another state (such as enabled or blocked, etc) then we should allow the timer to resume and give the device some more time before forcing an unplug. It's also probably not a good idea to try and deconfigure a device that might already be deconfigured (e.g. if it's already in standby or reserved state). That might not happen though, but it's good to cover our bases.
A side thought: In addition to checking the states, what would happen if you forced the timer to 0? Would the callback get called? Would that just accelerate the already-in-progress unplug sequence?
and 2)I worry that the sclp might try to deconfigure a PCI device at the same time we force the callback in this patch. I noticed that the sclp_deconfigure function also checks on the release timer before unplugging. I think we should make sure the timer is properly stopped or canceled before the sclp tries to deconfigure and before this patch forces the callback, that way we hopefully won't try to do both at the same time.
something like if (release_timer) { stop timer unplug }Your adjustments to the pcihost_unplug function in patch #1 would of course handle freeing the release timer later on.
Signed-off-by: David Hildenbrand <address@hidden> --- hw/s390x/s390-pci-bus.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c index bc17a8cf65..b70ae25533 100644 --- a/hw/s390x/s390-pci-bus.c +++ b/hw/s390x/s390-pci-bus.c @@ -1102,6 +1102,14 @@ static void s390_pcihost_reset(DeviceState *dev) { S390pciState *s = S390_PCI_HOST_BRIDGE(dev); PCIBus *bus = s->parent_obj.bus; + S390PCIBusDevice *pbdev, *next; + + /* Unplug all pending devices that were requested to be released */ + QTAILQ_FOREACH_SAFE(pbdev, &s->zpci_devs, link, next) { + if (pbdev->release_timer) { + s390_pcihost_timer_cb(pbdev); + } + }s->bus_no = 0;pci_for_each_device(bus, pci_bus_num(bus), s390_pci_enumerate_bridge, s);
[Prev in Thread] | Current Thread | [Next in Thread] |