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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4709 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]