Re: [PATCH] pci: Skip power-off reset when pending unplug

[Michael S. Tsirkin]

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" <mst@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-stable@nongnu.org
> > > Fixes: d5daff7d3126 ("pcie: implement slot power control for pcie root 
> > > ports")
> > > Signed-off-by: Alex Williamson <alex.williamson@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);
> > >      }
> > >  }
> > >   
> >