1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH v2 00/10] pci: hotplug handler reworks
  4. View patch

[Qemu-devel] [PATCH v2 04/10] pci/pcie: stop plug/unplug if the slot is locked

David Hildenbrand posted 10 patches 7 years ago
There is a newer version of this series
[Qemu-devel] [PATCH v2 04/10] pci/pcie: stop plug/unplug if the slot is locked
Posted by David Hildenbrand 7 years ago 
We better stop right away. While at it, properly move the check
to the pre_plug handler.

Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/pci/pcie.c         | 25 +++++++++++++++++--------
 hw/pci/pcie_port.c    |  1 +
 include/hw/pci/pcie.h |  2 ++
 3 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 44737cc1cd..ccba29269e 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -316,10 +316,10 @@ static void pcie_cap_slot_event(PCIDevice *dev, PCIExpressHotPlugEvent event)
 }
 
 static void pcie_cap_slot_plug_common(PCIDevice *hotplug_dev, DeviceState *dev,
-                                      uint8_t **exp_cap, Error **errp)
+                                      Error **errp)
 {
-    *exp_cap = hotplug_dev->config + hotplug_dev->exp.exp_cap;
-    uint16_t sltsta = pci_get_word(*exp_cap + PCI_EXP_SLTSTA);
+    uint8_t *exp_cap = hotplug_dev->config + hotplug_dev->exp.exp_cap;
+    uint16_t sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA);
 
     PCIE_DEV_PRINTF(PCI_DEVICE(dev), "hotplug state: 0x%x\n", sltsta);
     if (sltsta & PCI_EXP_SLTSTA_EIS) {
@@ -330,14 +330,19 @@ static void pcie_cap_slot_plug_common(PCIDevice *hotplug_dev, DeviceState *dev,
     }
 }
 
+void pcie_cap_slot_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
+                               Error **errp)
+{
+    pcie_cap_slot_plug_common(PCI_DEVICE(hotplug_dev), dev, errp);
+}
+
 void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
                            Error **errp)
 {
-    uint8_t *exp_cap;
+    PCIDevice *hotplug_pdev = PCI_DEVICE(hotplug_dev);
+    uint8_t *exp_cap = hotplug_pdev->config + hotplug_pdev->exp.exp_cap;
     PCIDevice *pci_dev = PCI_DEVICE(dev);
 
-    pcie_cap_slot_plug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp);
-
     /* Don't send event when device is enabled during qemu machine creation:
      * it is present on boot, no hotplug event is necessary. We do send an
      * event when the device is disabled later. */
@@ -367,11 +372,15 @@ static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
 void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
                                      DeviceState *dev, Error **errp)
 {
-    uint8_t *exp_cap;
+    Error *local_err = NULL;
     PCIDevice *pci_dev = PCI_DEVICE(dev);
     PCIBus *bus = pci_get_bus(pci_dev);
 
-    pcie_cap_slot_plug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp);
+    pcie_cap_slot_plug_common(PCI_DEVICE(hotplug_dev), dev, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
 
     /* In case user cancel the operation of multi-function hot-add,
      * remove the function that is unexposed to guest individually,
diff --git a/hw/pci/pcie_port.c b/hw/pci/pcie_port.c
index 73e81e5847..7982a87880 100644
--- a/hw/pci/pcie_port.c
+++ b/hw/pci/pcie_port.c
@@ -154,6 +154,7 @@ static void pcie_slot_class_init(ObjectClass *oc, void *data)
     HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
 
     dc->props = pcie_slot_props;
+    hc->pre_plug = pcie_cap_slot_pre_plug_cb;
     hc->plug = pcie_cap_slot_plug_cb;
     hc->unplug_request = pcie_cap_slot_unplug_request_cb;
 }
diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index 735f8e8154..d9fbcf4a4a 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -131,6 +131,8 @@ void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn);
 void pcie_dev_ser_num_init(PCIDevice *dev, uint16_t offset, uint64_t ser_num);
 void pcie_ats_init(PCIDevice *dev, uint16_t offset);
 
+void pcie_cap_slot_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
+                               Error **errp);
 void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
                            Error **errp);
 void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
-- 
2.17.2
Re: [Qemu-devel] [PATCH v2 04/10] pci/pcie: stop plug/unplug if the slot is locked
Posted by David Gibson 6 years, 12 months ago 
On Mon, Nov 05, 2018 at 11:20:38AM +0100, David Hildenbrand wrote:
> We better stop right away. While at it, properly move the check
> to the pre_plug handler.
> 
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Although I'm not sure the commit message gives a terribly clear
picture of what's going on here.

> ---
>  hw/pci/pcie.c         | 25 +++++++++++++++++--------
>  hw/pci/pcie_port.c    |  1 +
>  include/hw/pci/pcie.h |  2 ++
>  3 files changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 44737cc1cd..ccba29269e 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -316,10 +316,10 @@ static void pcie_cap_slot_event(PCIDevice *dev, PCIExpressHotPlugEvent event)
>  }
>  
>  static void pcie_cap_slot_plug_common(PCIDevice *hotplug_dev, DeviceState *dev,
> -                                      uint8_t **exp_cap, Error **errp)
> +                                      Error **errp)
>  {
> -    *exp_cap = hotplug_dev->config + hotplug_dev->exp.exp_cap;
> -    uint16_t sltsta = pci_get_word(*exp_cap + PCI_EXP_SLTSTA);
> +    uint8_t *exp_cap = hotplug_dev->config + hotplug_dev->exp.exp_cap;
> +    uint16_t sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA);
>  
>      PCIE_DEV_PRINTF(PCI_DEVICE(dev), "hotplug state: 0x%x\n", sltsta);
>      if (sltsta & PCI_EXP_SLTSTA_EIS) {
> @@ -330,14 +330,19 @@ static void pcie_cap_slot_plug_common(PCIDevice *hotplug_dev, DeviceState *dev,
>      }
>  }
>  
> +void pcie_cap_slot_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                               Error **errp)
> +{
> +    pcie_cap_slot_plug_common(PCI_DEVICE(hotplug_dev), dev, errp);
> +}
> +
>  void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>                             Error **errp)
>  {
> -    uint8_t *exp_cap;
> +    PCIDevice *hotplug_pdev = PCI_DEVICE(hotplug_dev);
> +    uint8_t *exp_cap = hotplug_pdev->config + hotplug_pdev->exp.exp_cap;
>      PCIDevice *pci_dev = PCI_DEVICE(dev);
>  
> -    pcie_cap_slot_plug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp);
> -
>      /* Don't send event when device is enabled during qemu machine creation:
>       * it is present on boot, no hotplug event is necessary. We do send an
>       * event when the device is disabled later. */
> @@ -367,11 +372,15 @@ static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
>  void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
>                                       DeviceState *dev, Error **errp)
>  {
> -    uint8_t *exp_cap;
> +    Error *local_err = NULL;
>      PCIDevice *pci_dev = PCI_DEVICE(dev);
>      PCIBus *bus = pci_get_bus(pci_dev);
>  
> -    pcie_cap_slot_plug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp);
> +    pcie_cap_slot_plug_common(PCI_DEVICE(hotplug_dev), dev, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      /* In case user cancel the operation of multi-function hot-add,
>       * remove the function that is unexposed to guest individually,
> diff --git a/hw/pci/pcie_port.c b/hw/pci/pcie_port.c
> index 73e81e5847..7982a87880 100644
> --- a/hw/pci/pcie_port.c
> +++ b/hw/pci/pcie_port.c
> @@ -154,6 +154,7 @@ static void pcie_slot_class_init(ObjectClass *oc, void *data)
>      HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
>  
>      dc->props = pcie_slot_props;
> +    hc->pre_plug = pcie_cap_slot_pre_plug_cb;
>      hc->plug = pcie_cap_slot_plug_cb;
>      hc->unplug_request = pcie_cap_slot_unplug_request_cb;
>  }
> diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> index 735f8e8154..d9fbcf4a4a 100644
> --- a/include/hw/pci/pcie.h
> +++ b/include/hw/pci/pcie.h
> @@ -131,6 +131,8 @@ void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn);
>  void pcie_dev_ser_num_init(PCIDevice *dev, uint16_t offset, uint64_t ser_num);
>  void pcie_ats_init(PCIDevice *dev, uint16_t offset);
>  
> +void pcie_cap_slot_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                               Error **errp);
>  void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>                             Error **errp);
>  void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
Re: [Qemu-devel] [PATCH v2 04/10] pci/pcie: stop plug/unplug if the slot is locked
Posted by David Hildenbrand 6 years, 12 months ago 
On 07.11.18 00:10, David Gibson wrote:
> On Mon, Nov 05, 2018 at 11:20:38AM +0100, David Hildenbrand wrote:
>> We better stop right away. While at it, properly move the check
>> to the pre_plug handler.
>>
>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Although I'm not sure the commit message gives a terribly clear
> picture of what's going on here.

Thanks, I'll change it to

"We better stop right away. For now, errors would be partially ignored
(so the guest might get informed or the device might get unplugged),
although actual plug/unplug will be reported as failed to the user.

While at it, properly move the check to the pre_plug handler for the
plug case, as we can test the slot state before the device will be
realized."

-- 

Thanks,

David / dhildenb

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.