On Tue, Dec 21, 2021 at 09:36:56AM -0700, Alex Williamson wrote:
> On Mon, 20 Dec 2021 18:03:56 -0500
> "Michael S. Tsirkin" <m...@redhat.com> wrote:
>
> > On Mon, Dec 20, 2021 at 11:26:59AM -0700, Alex Williamson wrote:
> > > The below referenced commit introduced a change where devices under a
> > > root port slot are reset in response to removing power to the slot.
> > > This improves emulation relative to bare metal when the slot is powered
> > > off, but introduces an unnecessary step when devices under that slot
> > > are slated for removal.
> > >
> > > In the case of an assigned device, there are mandatory delays
> > > associated with many device reset mechanisms which can stall the hot
> > > unplug operation. Also, in cases where the unplug request is triggered
> > > via a release operation of the host driver, internal device locking in
> > > the host kernel may result in a failure of the device reset mechanism,
> > > which generates unnecessary log warnings.
> > >
> > > Skip the reset for devices that are slated for unplug.
> > >
> > > Cc: qemu-sta...@nongnu.org
> > > Fixes: d5daff7d3126 ("pcie: implement slot power control for pcie root
> > > ports")
> > > Signed-off-by: Alex Williamson <alex.william...@redhat.com>
> >
> > I am not sure this is safe. IIUC pending_deleted_event
> > is normally set after host admin requested device removal,
> > while the reset could be triggered by guest for its own reasons
> > such as suspend or driver reload.
>
> Right, the case where I mention that we get the warning looks exactly
> like the admin doing a device eject, it calls qdev_unplug(). I'm not
> trying to prevent arbitrary guest resets of the device, in fact there
> are cases where the guest really should be able to reset the device,
> nested assignment in addition to the cases you mention. Gerd noted
> that this was an unintended side effect of the referenced patch to
> reset device that are imminently being removed.
>
> > Looking at this some more, I am not sure I understand the
> > issue completely.
> > We have:
> >
> > if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) &&
> > (val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF &&
> > (!(old_slt_ctl & PCI_EXP_SLTCTL_PCC) ||
> > (old_slt_ctl & PCI_EXP_SLTCTL_PIC_OFF) != PCI_EXP_SLTCTL_PIC_OFF)) {
> > pcie_cap_slot_do_unplug(dev);
> > }
> > pcie_cap_update_power(dev);
> >
> > so device unplug triggers first, reset follows and by that time
> > there should be no devices under the bus, if there are then
> > it's because guest did not clear the power indicator.
>
> Note that the unplug only triggers here if the Power Indicator Control
> is OFF, I see writes to SLTCTL in the following order:
>
> 01f1 - > 02f1 -> 06f1 -> 07f1
>
> So PIC changes to BLINK, then PCC changes the slot to OFF (this
> triggers the reset), then PIC changes to OFF triggering the unplug.
>
> The unnecessary reset that occurs here is universal. Should the unplug
> be occurring when:
>
> (val & PCI_EXP_SLTCTL_PIC_OFF) != PCI_EXP_SLTCTL_PIC_ON
>
> ?
well blinking generally means "do not remove yet".
> > So I am not sure how to fix the assignment issues as I'm not sure how do
> > they trigger, but here is a wild idea: maybe it should support an API
> > for starting reset asynchronously, then if the following access is
> > trying to reset again that second reset can just be skipped, while any
> > other access will stall.
>
> As above, there's not a concurrency problem, so I don't see how an
> async API buys us anything.
Well unplug resets the device again, right? Why is that reset not
problematic and this one is?
> It seems the ordering of the slot power
> induced reset versus device unplug is not as you expected. Can we fix
> that? Thanks,
>
> Alex
Oh I means on the PIC write. That triggers the unplug without triggering
a reset. I was under the impression you are saying the same guest
write triggers both reset and unplug.
Since in this case it's two writes, I don't see how we
can tie ourselves to guest doing things in a specific order.
It can always change the order of things.
>
> > > ---
> > > hw/pci/pci.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index e5993c1ef52b..f594da410797 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
> > > @@ -2869,7 +2869,7 @@ void pci_set_power(PCIDevice *d, bool state)
> > > memory_region_set_enabled(&d->bus_master_enable_region,
> > > (pci_get_word(d->config + PCI_COMMAND)
> > > & PCI_COMMAND_MASTER) && d->has_power);
> > > - if (!d->has_power) {
> > > + if (!d->has_power && !d->qdev.pending_deleted_event) {
> > > pci_device_reset(d);
> > > }
> > > }
> > >
> >
