Re: [PATCH] pci: Refuse to hotplug PCI Devices when the Guest OS is not ready

[David Gibson <dgibson@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2445 bytes --]

On Thu, 22 Oct 2020 08:06:55 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Oct 22, 2020 at 02:40:26PM +0300, Marcel Apfelbaum wrote:
> > From: Marcel Apfelbaum <marcel@redhat.com>
> > 
> > During PCIe Root Port's transition from Power-Off to Power-ON (or vice-versa)
> > the "Slot Control Register" has the "Power Indicator Control"
> > set to "Blinking" expressing a "power transition" mode.
> > 
> > Any hotplug operation during the "power transition" mode is not permitted
> > or at least not expected by the Guest OS leading to strange failures.
> > 
> > Detect and refuse hotplug operations in such case.
> > 
> > Signed-off-by: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > ---
> >  hw/pci/pcie.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index 5b48bae0f6..2fe5c1473f 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -410,6 +410,7 @@ void pcie_cap_slot_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> >      PCIDevice *hotplug_pdev = PCI_DEVICE(hotplug_dev);
> >      uint8_t *exp_cap = hotplug_pdev->config + hotplug_pdev->exp.exp_cap;
> >      uint32_t sltcap = pci_get_word(exp_cap + PCI_EXP_SLTCAP);
> > +    uint32_t sltctl = pci_get_word(exp_cap + PCI_EXP_SLTCTL);
> >  
> >      /* Check if hot-plug is disabled on the slot */
> >      if (dev->hotplugged && (sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> > @@ -418,6 +419,12 @@ void pcie_cap_slot_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> >          return;
> >      }
> >  
> > +    if ((sltctl & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_BLINK) {
> > +        error_setg(errp, "Hot-plug failed: %s is in Power Transition",
> > +                   DEVICE(hotplug_pdev)->id);
> > +        return;
> > +    }
> > +
> >      pcie_cap_slot_plug_common(PCI_DEVICE(hotplug_dev), dev, errp);
> >  }  
> 
> Probably the only way to handle for existing machine types.
> For new ones, can't we queue it in host memory somewhere?

I'm not actually convinced we can't do that even for existing machine
types.  So I'm a bit hesitant to suggest going ahead with this without
looking a bit closer at whether we can implement a wait-for-ready in
qemu, rather than forcing every user of qemu (human or machine) to do
so.


-- 
David Gibson <dgibson@redhat.com>
Principal Software Engineer, Virtualization, Red Hat

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]