[v1] pcie: fix device hotplug failure at the meantime of VM boot

[Qemu-devel] [PATCH] pcie: fix device hotplug failure at the meantime of VM boot

Posted by Zhangbo (Oscar) 6 years, 6 months ago

If the PCI_EXP_LNKSTA_DLLLA capability is set by default, linux kernel will send 
PDC event to detect whether there is a device in pcie slot. If a device is pluged
in the pcie-root-port at the same time, hot-plug device will send ABP + PDC
events to the kernel. The VM kernel will wrongly unplug the device if two PDC
events get too close. Thus we'd better set the PCI_EXP_LNKSTA_DLLLA
capability only in hotplug callback.

By the way, we should clean up the PCI_EXP_LNKSTA_DLLLA capability during
unplug to avoid VM restart or migration failure which will enter the same
abnormal scenario as above.

Signed-off-by: limingwang@huawei.com
Signed-off-by: fangying1@huawei.com
Signed-off-by: oscar.zhangbo@huawei.com
---
 hw/pci/pcie.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index a6beb56..174f392 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -75,10 +75,6 @@ pcie_cap_v1_fill(PCIDevice *dev, uint8_t port, uint8_t type, uint8_t version)
                  QEMU_PCI_EXP_LNKSTA_NLW(QEMU_PCI_EXP_LNK_X1) |
                  QEMU_PCI_EXP_LNKSTA_CLS(QEMU_PCI_EXP_LNK_2_5GT));
 
-    if (dev->cap_present & QEMU_PCIE_LNKSTA_DLLLA) {
-        pci_word_test_and_set_mask(exp_cap + PCI_EXP_LNKSTA,
-                                   PCI_EXP_LNKSTA_DLLLA);
-    }
 
     /* We changed link status bits over time, and changing them across
      * migrations is generally fine as hardware changes them too.
@@ -484,6 +480,11 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
         return;
     }
 
+    if (pci_dev->cap_present & QEMU_PCIE_LNKSTA_DLLLA) {
+        pci_word_test_and_clear_mask(exp_cap + PCI_EXP_LNKSTA,
+                                     PCI_EXP_LNKSTA_DLLLA);
+    }
+
     pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev));
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] pcie: fix device hotplug failure at the meantime of VM boot

Posted by Philippe Mathieu-Daudé 6 years, 6 months ago

Hi Oscar,

On 7/23/19 9:47 AM, Zhangbo (Oscar) wrote:
> If the PCI_EXP_LNKSTA_DLLLA capability is set by default, linux kernel will send 
> PDC event to detect whether there is a device in pcie slot. If a device is pluged
> in the pcie-root-port at the same time, hot-plug device will send ABP + PDC
> events to the kernel. The VM kernel will wrongly unplug the device if two PDC
> events get too close. Thus we'd better set the PCI_EXP_LNKSTA_DLLLA
> capability only in hotplug callback.
> 
> By the way, we should clean up the PCI_EXP_LNKSTA_DLLLA capability during
> unplug to avoid VM restart or migration failure which will enter the same
> abnormal scenario as above.
> 
> Signed-off-by: limingwang@huawei.com
> Signed-off-by: fangying1@huawei.com

QEMU contribution guideline says:

  Please use your real name to sign a patch (not an alias or acronym).

See:
https://wiki.qemu.org/Contribute/SubmitAPatch#Patch_emails_must_include_a_Signed-off-by:_line

We can infer limingwang@ real name is Liming Wang, oscar.zhangbo@ is
Oscar Zhangbo, but for fangying1@ it is too hard.
Can you provide his real name?

Thanks,

Phil.

> Signed-off-by: oscar.zhangbo@huawei.com
> ---
>  hw/pci/pcie.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index a6beb56..174f392 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -75,10 +75,6 @@ pcie_cap_v1_fill(PCIDevice *dev, uint8_t port, uint8_t type, uint8_t version)
>                   QEMU_PCI_EXP_LNKSTA_NLW(QEMU_PCI_EXP_LNK_X1) |
>                   QEMU_PCI_EXP_LNKSTA_CLS(QEMU_PCI_EXP_LNK_2_5GT));
>  
> -    if (dev->cap_present & QEMU_PCIE_LNKSTA_DLLLA) {
> -        pci_word_test_and_set_mask(exp_cap + PCI_EXP_LNKSTA,
> -                                   PCI_EXP_LNKSTA_DLLLA);
> -    }
>  
>      /* We changed link status bits over time, and changing them across
>       * migrations is generally fine as hardware changes them too.
> @@ -484,6 +480,11 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
>          return;
>      }
>  
> +    if (pci_dev->cap_present & QEMU_PCIE_LNKSTA_DLLLA) {
> +        pci_word_test_and_clear_mask(exp_cap + PCI_EXP_LNKSTA,
> +                                     PCI_EXP_LNKSTA_DLLLA);
> +    }
> +
>      pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev));
>  }
>  
>

Re: [Qemu-devel] [PATCH] pcie: fix device hotplug failure at the meantime of VM boot

Posted by Zhangbo (Oscar) 6 years, 6 months ago


>-----Original Message-----
>From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com]
>Sent: Tuesday, July 23, 2019 5:18 PM
>To: Zhangbo (Oscar) <oscar.zhangbo@huawei.com>; qemu-devel@nongnu.org
>Cc: fangying <fangying1@huawei.com>; dengkai (A) <dengkai1@huawei.com>;
>limingwang (A) <limingwang@huawei.com>; mst@redhat.com
>Subject: Re: [Qemu-devel] [PATCH] pcie: fix device hotplug failure at the
>meantime of VM boot
>
>Hi Oscar,
>
>On 7/23/19 9:47 AM, Zhangbo (Oscar) wrote:
>> If the PCI_EXP_LNKSTA_DLLLA capability is set by default, linux kernel will send
>> PDC event to detect whether there is a device in pcie slot. If a device is pluged
>> in the pcie-root-port at the same time, hot-plug device will send ABP + PDC
>> events to the kernel. The VM kernel will wrongly unplug the device if two PDC
>> events get too close. Thus we'd better set the PCI_EXP_LNKSTA_DLLLA
>> capability only in hotplug callback.
>>
>> By the way, we should clean up the PCI_EXP_LNKSTA_DLLLA capability during
>> unplug to avoid VM restart or migration failure which will enter the same
>> abnormal scenario as above.
>>
>> Signed-off-by: limingwang@huawei.com
>> Signed-off-by: fangying1@huawei.com
>
>QEMU contribution guideline says:
>
>  Please use your real name to sign a patch (not an alias or acronym).
>
Thanks for reminding, I've sent a patch V2 just now, plz check there.

>See:
>https://wiki.qemu.org/Contribute/SubmitAPatch#Patch_emails_must_include_a
>_Signed-off-by:_line
>
>We can infer limingwang@ real name is Liming Wang, oscar.zhangbo@ is
>Oscar Zhangbo, but for fangying1@ it is too hard.
>Can you provide his real name?
>
>Thanks,
>
>Phil.
>
>> Signed-off-by: oscar.zhangbo@huawei.com
>> ---
>>  hw/pci/pcie.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
>> index a6beb56..174f392 100644
>> --- a/hw/pci/pcie.c
>> +++ b/hw/pci/pcie.c
>> @@ -75,10 +75,6 @@ pcie_cap_v1_fill(PCIDevice *dev, uint8_t port, uint8_t
>type, uint8_t version)
>>
>QEMU_PCI_EXP_LNKSTA_NLW(QEMU_PCI_EXP_LNK_X1) |
>>
>QEMU_PCI_EXP_LNKSTA_CLS(QEMU_PCI_EXP_LNK_2_5GT));
>>
>> -    if (dev->cap_present & QEMU_PCIE_LNKSTA_DLLLA) {
>> -        pci_word_test_and_set_mask(exp_cap + PCI_EXP_LNKSTA,
>> -                                   PCI_EXP_LNKSTA_DLLLA);
>> -    }
>>
>>      /* We changed link status bits over time, and changing them across
>>       * migrations is generally fine as hardware changes them too.
>> @@ -484,6 +480,11 @@ void
>pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
>>          return;
>>      }
>>
>> +    if (pci_dev->cap_present & QEMU_PCIE_LNKSTA_DLLLA) {
>> +        pci_word_test_and_clear_mask(exp_cap + PCI_EXP_LNKSTA,
>> +                                     PCI_EXP_LNKSTA_DLLLA);
>> +    }
>> +
>>      pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev));
>>  }
>>
>>

Re: [Qemu-devel] [PATCH] pcie: fix device hotplug failure at the meantime of VM boot

Posted by Michael S. Tsirkin 6 years, 6 months ago

On Tue, Jul 23, 2019 at 07:47:51AM +0000, Zhangbo (Oscar) wrote:
> If the PCI_EXP_LNKSTA_DLLLA capability is set by default, linux kernel will send 
> PDC event to detect whether there is a device in pcie slot. If a device is pluged
> in the pcie-root-port at the same time, hot-plug device will send ABP + PDC
> events to the kernel. The VM kernel will wrongly unplug the device if two PDC
> events get too close. Thus we'd better set the PCI_EXP_LNKSTA_DLLLA
> capability only in hotplug callback.

Could you please describe a reproducer in a bit more detail?

> 
> By the way, we should clean up the PCI_EXP_LNKSTA_DLLLA capability during
> unplug to avoid VM restart or migration failure which will enter the same
> abnormal scenario as above.
> 
> Signed-off-by: limingwang@huawei.com
> Signed-off-by: fangying1@huawei.com
> Signed-off-by: oscar.zhangbo@huawei.com

So looking at linux I see:

 * pciehp_card_present_or_link_active() - whether given slot is occupied
 * @ctrl: PCIe hotplug controller
 *
 * Unlike pciehp_card_present(), which determines presence solely from the
 * Presence Detect State bit, this helper also returns true if the Link Active
 * bit is set.  This is a concession to broken hotplug ports which hardwire
 * Presence Detect State to zero, such as Wilocity's [1ae9:0200].

so it looks like linux actually looks at presence detect state,
but we have a bug just like Wilocity's and keeping
link active up fixes that. Can't we fix the bug instead?

> ---
>  hw/pci/pcie.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index a6beb56..174f392 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -75,10 +75,6 @@ pcie_cap_v1_fill(PCIDevice *dev, uint8_t port, uint8_t type, uint8_t version)
>                   QEMU_PCI_EXP_LNKSTA_NLW(QEMU_PCI_EXP_LNK_X1) |
>                   QEMU_PCI_EXP_LNKSTA_CLS(QEMU_PCI_EXP_LNK_2_5GT));
>  
> -    if (dev->cap_present & QEMU_PCIE_LNKSTA_DLLLA) {
> -        pci_word_test_and_set_mask(exp_cap + PCI_EXP_LNKSTA,
> -                                   PCI_EXP_LNKSTA_DLLLA);
> -    }
>  
>      /* We changed link status bits over time, and changing them across
>       * migrations is generally fine as hardware changes them too.

Does this actually change anything?

I don't know why do we bother setting it here but we do
set it later in pcie_cap_slot_plug_cb, correct?

I'd like to understand whether this is part of fix or
just a cleanup.

> @@ -484,6 +480,11 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
>          return;
>      }
>  
> +    if (pci_dev->cap_present & QEMU_PCIE_LNKSTA_DLLLA) {
> +        pci_word_test_and_clear_mask(exp_cap + PCI_EXP_LNKSTA,
> +                                     PCI_EXP_LNKSTA_DLLLA);
> +    }
> +
>      pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev));
>  }

So this reports data link inactive immediately after
unplug request. Seems a bit questionable: guest did not
respond yet. I'd like to see a comment explaining why
this makes sense.

> -- 
> 1.8.3.1