Hi Michael,
On Thu, Oct 22, 2020 at 6:01 PM Michael S. Tsirkin <m...@redhat.com> wrote:
> On Thu, Oct 22, 2020 at 05:50:51PM +0300, Marcel Apfelbaum wrote:
> >
> >
> > On Thu, Oct 22, 2020 at 5:33 PM Michael S. Tsirkin <m...@redhat.com>
> wrote:
> >
> > On Thu, Oct 22, 2020 at 05:10:43PM +0300, Marcel Apfelbaum wrote:
> > >
> > >
> > > On Thu, Oct 22, 2020 at 5:01 PM Michael S. Tsirkin <m...@redhat.com
> >
> > wrote:
> > >
> > > On Thu, Oct 22, 2020 at 04:55:10PM +0300, Marcel Apfelbaum
> wrote:
> > > > 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?
> > >
> > >
> > > How can it fail? It's just a button press ...
> > >
> > >
> > >
> > > Currently we have "Hotplug unsupported."
> > > With this change we have "Guest/System not ready"
> >
> >
> > Hotplug unsupported is not an error that can trigger with
> > a well behaved management such as libvirt.
> >
> >
> > >
> > >
> > > >
> > > >
> > > > 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
> > > >
> > >
> > >
> > > I don't think they ever failed in the past so management is
> unlikely
> > > to handle the failure by retrying ...
> > >
> > >
> > > That would require some management handling, yes.
> > > But even without a "retry", failing is better than strange OS
> behavior.
> > >
> > > Trying a better alternative like deferring the operation for new
> machines
> > > would make sense, however is out of the scope of this patch
> >
> > Expand the scope please. The scope should be "solve a problem xx" not
> > "solve a problem xx by doing abc".
> >
> >
> >
> > The scope is detecting a hotplug error early instead
> > passing to the Guest OS a hotplug operation that we know it will fail.
> >
>
> Right. After detecting just failing unconditionally it a bit too
> simplistic IMHO.
>
>
Simplistic does not mean wrong or incorrect.
I fail to see why it is not enough.
What QEMU can do better? Wait an unbounded time for the blinking to finish?
What if we have a buggy guest with a kernel stuck in blinking?
Is QEMU's responsibility to emulate the operator itself? Because the
operator
is the one who is supposed to wait.
Thanks,
Marcel
[...]
