Hi David, Michael,
On Thu, Oct 22, 2020 at 3:56 PM David Gibson <dgib...@redhat.com> wrote:
> On Thu, 22 Oct 2020 08:06:55 -0400
> "Michael S. Tsirkin" <m...@redhat.com> wrote:
>
> > On Thu, Oct 22, 2020 at 02:40:26PM +0300, Marcel Apfelbaum wrote:
> > > From: Marcel Apfelbaum <mar...@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.apfelb...@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.
>
I agree
> > For new ones, can't we queue it in host memory somewhere?
>
>
I am not sure I understand what will be the flow.
- The user asks for a hotplug operation.
- QEMU deferred operation.
After that the operation may still fail, how would the user know if the
operation
succeeded or not?
> I'm not actually convinced we can't do that even for existing machine
> types.
Is a Guest visible change, I don't think we can do it.
> 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.
>
While I agree it is a pain from the usability point of view, hotplug
operations
are allowed to fail. This is not more than a corner case, ensuring the right
response (gracefully erroring out) may be enough.
Thanks,
Marcel
>
>
> --
> David Gibson <dgib...@redhat.com>
> Principal Software Engineer, Virtualization, Red Hat
>
