[PATCH 0/1] RFC: pcie: parital fix for missing unplug events

[PATCH 0/1] RFC: pcie: parital fix for missing unplug events

[Maxim Levitsky]

As described in bugzilla #1854264 it is possible to plug
a pcie device and then unplug it before the guest notices
(has time to process the attention button press)

To partially fix this issue, detect and refuse the hotunplug event.

There are other ways to fix this, which is why I am sending this as RFC:

1. Queue/remember the unplug event and fire it (press the attention button again),
when the guest clears its pending status.
According to the spec this should cancel the hotplug event in the guest
if done within 5 seconds (and I think that guest actually should wait 5 seconds
after attention button is pressed before plugging in the device too to be strictly
up to the spec)

2. Detect that device isn't yet powered on by the guest (for example
checking the bus master bit) or check that attention button is still
pressed and in this case just unplug the device immediately.

Best regards,
	Maxim Levitsky

Maxim Levitsky (1):
  pci/pcie: refuse another hotplug/unplug event if attention button is
    pending

 hw/pci/pcie.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

-- 
2.26.2

[PATCH 1/1] pci/pcie: refuse another hotplug/unplug event if attention button is pending

[Maxim Levitsky]

Curently it is possible to hotplug a device and then immediatly
hotunplug it before the OS notices, and that will result
in missed unplug event since we can only send one attention button event.

Moreover the device will stuck in unplugging state forever.

Error out in such cases and rely on the caller (e.g libvirt) to retry
the unplug a bit later

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 hw/pci/pcie.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 5b48bae0f6..9e836cf2f4 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -402,6 +402,17 @@ static void pcie_cap_slot_plug_common(PCIDevice *hotplug_dev, DeviceState *dev,
          */
         error_setg_errno(errp, EBUSY, "slot is electromechanically locked");
     }
+
+    if (sltsta & PCI_EXP_SLTSTA_ABP) {
+        /*
+         * Attention button is pressed, thus we can't send another
+         * hotpplug event
+         */
+        error_setg_errno(errp, EBUSY,
+                         "attention button is already pressed, can't "
+                         "send another hotplug event");
+    }
+
 }
 
 void pcie_cap_slot_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
-- 
2.26.2

Re: [PATCH 1/1] pci/pcie: refuse another hotplug/unplug event if attention button is pending

[Maxim Levitsky]

On Wed, 2020-07-22 at 19:17 +0300, Maxim Levitsky wrote:
> Curently it is possible to hotplug a device and then immediatly
> hotunplug it before the OS notices, and that will result
> in missed unplug event since we can only send one attention button event.
> 
> Moreover the device will stuck in unplugging state forever.
> 
> Error out in such cases and rely on the caller (e.g libvirt) to retry
> the unplug a bit later
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  hw/pci/pcie.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 5b48bae0f6..9e836cf2f4 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -402,6 +402,17 @@ static void pcie_cap_slot_plug_common(PCIDevice *hotplug_dev, DeviceState *dev,
>           */
>          error_setg_errno(errp, EBUSY, "slot is electromechanically locked");
>      }
> +
> +    if (sltsta & PCI_EXP_SLTSTA_ABP) {
> +        /*
> +         * Attention button is pressed, thus we can't send another
> +         * hotpplug event
Typo here, forgot to refresh the commit.
> +         */
> +        error_setg_errno(errp, EBUSY,
> +                         "attention button is already pressed, can't "
> +                         "send another hotplug event");
> +    }
> +
>  }
>  
>  void pcie_cap_slot_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,

Re: [PATCH 1/1] pci/pcie: refuse another hotplug/unplug event if attention button is pending

[Maxim Levitsky]

On Wed, 2020-07-22 at 19:19 +0300, Maxim Levitsky wrote:
> On Wed, 2020-07-22 at 19:17 +0300, Maxim Levitsky wrote:
> > Curently it is possible to hotplug a device and then immediatly
> > hotunplug it before the OS notices, and that will result
> > in missed unplug event since we can only send one attention button event.
> > 
> > Moreover the device will stuck in unplugging state forever.
> > 
> > Error out in such cases and rely on the caller (e.g libvirt) to retry
> > the unplug a bit later
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  hw/pci/pcie.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index 5b48bae0f6..9e836cf2f4 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -402,6 +402,17 @@ static void pcie_cap_slot_plug_common(PCIDevice *hotplug_dev, DeviceState *dev,
> >           */
> >          error_setg_errno(errp, EBUSY, "slot is electromechanically locked");
> >      }
> > +
> > +    if (sltsta & PCI_EXP_SLTSTA_ABP) {
> > +        /*
> > +         * Attention button is pressed, thus we can't send another
> > +         * hotpplug event
> Typo here, forgot to refresh the commit.
> > +         */
> > +        error_setg_errno(errp, EBUSY,
> > +                         "attention button is already pressed, can't "
> > +                         "send another hotplug event");
> > +    }
> > +
> >  }
> >  
> >  void pcie_cap_slot_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
ping.


Best regards,
	Maxim Levitsky

Re: [PATCH 1/1] pci/pcie: refuse another hotplug/unplug event if attention button is pending

[Igor Mammedov]

On Wed, 29 Jul 2020 08:09:37 +0300
Maxim Levitsky <mlevitsk@redhat.com> wrote:

> On Wed, 2020-07-22 at 19:19 +0300, Maxim Levitsky wrote:
> > On Wed, 2020-07-22 at 19:17 +0300, Maxim Levitsky wrote:  
> > > Curently it is possible to hotplug a device and then immediatly
> > > hotunplug it before the OS notices, and that will result
> > > in missed unplug event since we can only send one attention button event.
> > > 
> > > Moreover the device will stuck in unplugging state forever.
> > > 
> > > Error out in such cases and rely on the caller (e.g libvirt) to retry
> > > the unplug a bit later
> > > 
> > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > > ---
> > >  hw/pci/pcie.c | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > > 
> > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > > index 5b48bae0f6..9e836cf2f4 100644
> > > --- a/hw/pci/pcie.c
> > > +++ b/hw/pci/pcie.c
> > > @@ -402,6 +402,17 @@ static void pcie_cap_slot_plug_common(PCIDevice *hotplug_dev, DeviceState *dev,
> > >           */
> > >          error_setg_errno(errp, EBUSY, "slot is electromechanically locked");
> > >      }
> > > +
> > > +    if (sltsta & PCI_EXP_SLTSTA_ABP) {
> > > +        /*
> > > +         * Attention button is pressed, thus we can't send another
> > > +         * hotpplug event  
> > Typo here, forgot to refresh the commit.  
> > > +         */
> > > +        error_setg_errno(errp, EBUSY,
> > > +                         "attention button is already pressed, can't "
> > > +                         "send another hotplug event");
> > > +    }
> > > +
> > >  }
> > >  
> > >  void pcie_cap_slot_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,  
> ping.

CCing Julia since she was looking into PCI hotplug/unplug code recently.

> 
> Best regards,
> 	Maxim Levitsky
> 
> 

Re: [PATCH 1/1] pci/pcie: refuse another hotplug/unplug event if attention button is pending

[Michael S. Tsirkin]

On Wed, Jul 22, 2020 at 07:17:22PM +0300, Maxim Levitsky wrote:
> Curently it is possible to hotplug a device and then immediatly
> hotunplug it before the OS notices, and that will result
> in missed unplug event since we can only send one attention button event.
> 
> Moreover the device will stuck in unplugging state forever.
> 
> Error out in such cases and rely on the caller (e.g libvirt) to retry
> the unplug a bit later
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  hw/pci/pcie.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 5b48bae0f6..9e836cf2f4 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -402,6 +402,17 @@ static void pcie_cap_slot_plug_common(PCIDevice *hotplug_dev, DeviceState *dev,
>           */
>          error_setg_errno(errp, EBUSY, "slot is electromechanically locked");
>      }
> +
> +    if (sltsta & PCI_EXP_SLTSTA_ABP) {
> +        /*
> +         * Attention button is pressed, thus we can't send another
> +         * hotpplug event

typo

> +         */
> +        error_setg_errno(errp, EBUSY,
> +                         "attention button is already pressed, can't "
> +                         "send another hotplug event");
> +    }
> +
>  }

It would be neater if we could queue the event up
in qemu. Alternatively - can we clean up the unhandled
event so guest does not even notice the device
briefly appeared?

>  
>  void pcie_cap_slot_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> -- 
> 2.26.2

Re: [PATCH 0/1] RFC: pcie: parital fix for missing unplug events

[Michael S. Tsirkin]

On Wed, Jul 22, 2020 at 07:17:21PM +0300, Maxim Levitsky wrote:
> As described in bugzilla #1854264 it is possible to plug
> a pcie device and then unplug it before the guest notices
> (has time to process the attention button press)
> 
> To partially fix this issue, detect and refuse the hotunplug event.
> 
> There are other ways to fix this, which is why I am sending this as RFC:
> 
> 1. Queue/remember the unplug event and fire it (press the attention button again),
> when the guest clears its pending status.
> According to the spec this should cancel the hotplug event in the guest
> if done within 5 seconds (and I think that guest actually should wait 5 seconds
> after attention button is pressed before plugging in the device too to be strictly
> up to the spec)
> 
> 2. Detect that device isn't yet powered on by the guest (for example
> checking the bus master bit) or check that attention button is still
> pressed and in this case just unplug the device immediately.

I think 2 or failing that 1 would be preferable.


> Best regards,
> 	Maxim Levitsky
> 
> Maxim Levitsky (1):
>   pci/pcie: refuse another hotplug/unplug event if attention button is
>     pending
> 
>  hw/pci/pcie.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> -- 
> 2.26.2
>