Raise an error when trying to hot-plug/unplug a device through QMP to a device
with disabled hot-plug capability. This makes the device behaviour more
consistent and provides an explanation of the failure in the case of
asynchronous unplug.

Signed-off-by: Julia Suvorova <jusual@redhat.com>
---
 hw/pci/pcie.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 0eb3a2a5d2..e9798caa8a 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -415,6 +415,7 @@ void pcie_cap_slot_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);
     PCIDevice *pci_dev = PCI_DEVICE(dev);
 
     /* Don't send event when device is enabled during qemu machine creation:
@@ -430,6 +431,13 @@ void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
         return;
     }
 
+    /* Hot-plug is disabled on the slot */
+    if ((sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
+        error_setg(errp, "Device '%s' does not support hot-plug",
+                         DEVICE(hotplug_dev)->id);
+        return;
+    }
+
     /* To enable multifunction hot-plug, we just ensure the function
      * 0 added last. When function 0 is added, we set the sltsta and
      * inform OS via event notification.
@@ -464,14 +472,24 @@ static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
     object_unparent(OBJECT(dev));
 }
 
-void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
+void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_handler,
                                      DeviceState *dev, Error **errp)
 {
     Error *local_err = NULL;
     PCIDevice *pci_dev = PCI_DEVICE(dev);
     PCIBus *bus = pci_get_bus(pci_dev);
+    PCIDevice *hotplug_dev = PCI_DEVICE(hotplug_handler);
+    uint8_t *exp_cap = hotplug_dev->config + hotplug_dev->exp.exp_cap;
+    uint32_t sltcap = pci_get_word(exp_cap + PCI_EXP_SLTCAP);
+
+    /* Hot-unplug is disabled on the slot */
+    if ((sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
+        error_setg(errp, "Device '%s' does not support hot-unplug",
+                         DEVICE(hotplug_dev)->id);
+        return;
+    }
 
-    pcie_cap_slot_plug_common(PCI_DEVICE(hotplug_dev), dev, &local_err);
+    pcie_cap_slot_plug_common(hotplug_dev, dev, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
@@ -490,7 +508,7 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
         return;
     }
 
-    pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev));
+    pcie_cap_slot_push_attention_button(hotplug_dev);
 }
 
 /* pci express slot for pci express root/downstream port
-- 
2.24.1

On Tue,  7 Apr 2020 16:50:17 +0200
Julia Suvorova <jusual@redhat.com> wrote:

> Raise an error when trying to hot-plug/unplug a device through QMP to a device
> with disabled hot-plug capability. This makes the device behaviour more
> consistent and provides an explanation of the failure in the case of
> asynchronous unplug.

it applies to hotplug in general (i.e. not only QMP)

> 
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> ---
>  hw/pci/pcie.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 0eb3a2a5d2..e9798caa8a 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -415,6 +415,7 @@ void pcie_cap_slot_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);
>      PCIDevice *pci_dev = PCI_DEVICE(dev);
>  
>      /* Don't send event when device is enabled during qemu machine creation:
> @@ -430,6 +431,13 @@ void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>          return;
>      }
>  
> +    /* Hot-plug is disabled on the slot */
> +    if ((sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> +        error_setg(errp, "Device '%s' does not support hot-plug",
> +                         DEVICE(hotplug_dev)->id);
plug and unplug_req are synchronous. so one can skip on "Device '%s'",
user will get this error message as response to device_add/del command.

and more exactly it's concrete slot that does not support hotplug, how about
"slot doesn't support ..." or just "hotlpug is not supported"

> +        return;
> +    }
> +
>      /* To enable multifunction hot-plug, we just ensure the function
>       * 0 added last. When function 0 is added, we set the sltsta and
>       * inform OS via event notification.
> @@ -464,14 +472,24 @@ static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
>      object_unparent(OBJECT(dev));
>  }
>  
> -void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
> +void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_handler,
>                                       DeviceState *dev, Error **errp)
>  {
>      Error *local_err = NULL;
>      PCIDevice *pci_dev = PCI_DEVICE(dev);
>      PCIBus *bus = pci_get_bus(pci_dev);
> +    PCIDevice *hotplug_dev = PCI_DEVICE(hotplug_handler);
> +    uint8_t *exp_cap = hotplug_dev->config + hotplug_dev->exp.exp_cap;
> +    uint32_t sltcap = pci_get_word(exp_cap + PCI_EXP_SLTCAP);
> +
> +    /* Hot-unplug is disabled on the slot */
> +    if ((sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> +        error_setg(errp, "Device '%s' does not support hot-unplug",
> +                         DEVICE(hotplug_dev)->id);
> +        return;
> +    }
>  
> -    pcie_cap_slot_plug_common(PCI_DEVICE(hotplug_dev), dev, &local_err);
> +    pcie_cap_slot_plug_common(hotplug_dev, dev, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
> @@ -490,7 +508,7 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
>          return;
>      }
>  
> -    pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev));
> +    pcie_cap_slot_push_attention_button(hotplug_dev);
>  }
>  
>  /* pci express slot for pci express root/downstream port

On Wed, Apr 08, 2020 at 12:51:20PM +0200, Igor Mammedov wrote:
> On Tue,  7 Apr 2020 16:50:17 +0200
> Julia Suvorova <jusual@redhat.com> wrote:
> 
> > Raise an error when trying to hot-plug/unplug a device through QMP to a device
> > with disabled hot-plug capability. This makes the device behaviour more
> > consistent and provides an explanation of the failure in the case of
> > asynchronous unplug.
> 
> it applies to hotplug in general (i.e. not only QMP)
> 
> > 
> > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > ---
> >  hw/pci/pcie.c | 24 +++++++++++++++++++++---
> >  1 file changed, 21 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index 0eb3a2a5d2..e9798caa8a 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -415,6 +415,7 @@ void pcie_cap_slot_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);
> >      PCIDevice *pci_dev = PCI_DEVICE(dev);
> >  
> >      /* Don't send event when device is enabled during qemu machine creation:
> > @@ -430,6 +431,13 @@ void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> >          return;
> >      }
> >  
> > +    /* Hot-plug is disabled on the slot */
> > +    if ((sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> > +        error_setg(errp, "Device '%s' does not support hot-plug",
> > +                         DEVICE(hotplug_dev)->id);
> plug and unplug_req are synchronous. so one can skip on "Device '%s'",
> user will get this error message as response to device_add/del command.
> 
> and more exactly it's concrete slot that does not support hotplug, how about
> "slot doesn't support ..." or just "hotlpug is not supported"

Well device name is useful here, while these commands are synchronous
others aren't so log parsing might not be synchronous.

I do think we should mention slot since that's the reason
hotplug failed:
    "Device '%s' hot-plug failed: unsupported by slot"

> > +        return;
> > +    }
> > +
> >      /* To enable multifunction hot-plug, we just ensure the function
> >       * 0 added last. When function 0 is added, we set the sltsta and
> >       * inform OS via event notification.
> > @@ -464,14 +472,24 @@ static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
> >      object_unparent(OBJECT(dev));
> >  }
> >  
> > -void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
> > +void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_handler,
> >                                       DeviceState *dev, Error **errp)
> >  {
> >      Error *local_err = NULL;
> >      PCIDevice *pci_dev = PCI_DEVICE(dev);
> >      PCIBus *bus = pci_get_bus(pci_dev);
> > +    PCIDevice *hotplug_dev = PCI_DEVICE(hotplug_handler);
> > +    uint8_t *exp_cap = hotplug_dev->config + hotplug_dev->exp.exp_cap;
> > +    uint32_t sltcap = pci_get_word(exp_cap + PCI_EXP_SLTCAP);
> > +
> > +    /* Hot-unplug is disabled on the slot */
> > +    if ((sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> > +        error_setg(errp, "Device '%s' does not support hot-unplug",
> > +                         DEVICE(hotplug_dev)->id);
> > +        return;

Here too let's mention slot since that's the reason
hotplug failed:
    "Device '%s' hot-unplug failed: unsupported by slot"

?

> > +    }
> >  
> > -    pcie_cap_slot_plug_common(PCI_DEVICE(hotplug_dev), dev, &local_err);
> > +    pcie_cap_slot_plug_common(hotplug_dev, dev, &local_err);
> >      if (local_err) {
> >          error_propagate(errp, local_err);
> >          return;
> > @@ -490,7 +508,7 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
> >          return;
> >      }
> >  
> > -    pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev));
> > +    pcie_cap_slot_push_attention_button(hotplug_dev);
> >  }
> >  
> >  /* pci express slot for pci express root/downstream port

On Mon, Apr 13, 2020 at 12:55 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Apr 08, 2020 at 12:51:20PM +0200, Igor Mammedov wrote:
> > On Tue,  7 Apr 2020 16:50:17 +0200
> > Julia Suvorova <jusual@redhat.com> wrote:
> >
> > > Raise an error when trying to hot-plug/unplug a device through QMP to a device
> > > with disabled hot-plug capability. This makes the device behaviour more
> > > consistent and provides an explanation of the failure in the case of
> > > asynchronous unplug.
> >
> > it applies to hotplug in general (i.e. not only QMP)
> >
> > >
> > > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > > ---
> > >  hw/pci/pcie.c | 24 +++++++++++++++++++++---
> > >  1 file changed, 21 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > > index 0eb3a2a5d2..e9798caa8a 100644
> > > --- a/hw/pci/pcie.c
> > > +++ b/hw/pci/pcie.c
> > > @@ -415,6 +415,7 @@ void pcie_cap_slot_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);
> > >      PCIDevice *pci_dev = PCI_DEVICE(dev);
> > >
> > >      /* Don't send event when device is enabled during qemu machine creation:
> > > @@ -430,6 +431,13 @@ void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> > >          return;
> > >      }
> > >
> > > +    /* Hot-plug is disabled on the slot */
> > > +    if ((sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> > > +        error_setg(errp, "Device '%s' does not support hot-plug",
> > > +                         DEVICE(hotplug_dev)->id);
> > plug and unplug_req are synchronous. so one can skip on "Device '%s'",
> > user will get this error message as response to device_add/del command.
> >
> > and more exactly it's concrete slot that does not support hotplug, how about
> > "slot doesn't support ..." or just "hotlpug is not supported"
>
> Well device name is useful here, while these commands are synchronous
> others aren't so log parsing might not be synchronous.
>
> I do think we should mention slot since that's the reason
> hotplug failed:
>     "Device '%s' hot-plug failed: unsupported by slot"
>
> > > +        return;
> > > +    }
> > > +
> > >      /* To enable multifunction hot-plug, we just ensure the function
> > >       * 0 added last. When function 0 is added, we set the sltsta and
> > >       * inform OS via event notification.
> > > @@ -464,14 +472,24 @@ static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
> > >      object_unparent(OBJECT(dev));
> > >  }
> > >
> > > -void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
> > > +void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_handler,
> > >                                       DeviceState *dev, Error **errp)
> > >  {
> > >      Error *local_err = NULL;
> > >      PCIDevice *pci_dev = PCI_DEVICE(dev);
> > >      PCIBus *bus = pci_get_bus(pci_dev);
> > > +    PCIDevice *hotplug_dev = PCI_DEVICE(hotplug_handler);
> > > +    uint8_t *exp_cap = hotplug_dev->config + hotplug_dev->exp.exp_cap;
> > > +    uint32_t sltcap = pci_get_word(exp_cap + PCI_EXP_SLTCAP);
> > > +
> > > +    /* Hot-unplug is disabled on the slot */
> > > +    if ((sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> > > +        error_setg(errp, "Device '%s' does not support hot-unplug",
> > > +                         DEVICE(hotplug_dev)->id);
> > > +        return;
>
> Here too let's mention slot since that's the reason
> hotplug failed:
>     "Device '%s' hot-unplug failed: unsupported by slot"
>
> ?

Just to mention, for a user it's disabled on the device, and they
don't have to know how it's implemented. This is the reason for the
difference between the error text and comments in code.

Best regards, Julia Suvorova.

On Wed, Apr 15, 2020 at 10:20:33PM +0200, Julia Suvorova wrote:
> On Mon, Apr 13, 2020 at 12:55 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Wed, Apr 08, 2020 at 12:51:20PM +0200, Igor Mammedov wrote:
> > > On Tue,  7 Apr 2020 16:50:17 +0200
> > > Julia Suvorova <jusual@redhat.com> wrote:
> > >
> > > > Raise an error when trying to hot-plug/unplug a device through QMP to a device
> > > > with disabled hot-plug capability. This makes the device behaviour more
> > > > consistent and provides an explanation of the failure in the case of
> > > > asynchronous unplug.
> > >
> > > it applies to hotplug in general (i.e. not only QMP)
> > >
> > > >
> > > > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > > > ---
> > > >  hw/pci/pcie.c | 24 +++++++++++++++++++++---
> > > >  1 file changed, 21 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > > > index 0eb3a2a5d2..e9798caa8a 100644
> > > > --- a/hw/pci/pcie.c
> > > > +++ b/hw/pci/pcie.c
> > > > @@ -415,6 +415,7 @@ void pcie_cap_slot_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);
> > > >      PCIDevice *pci_dev = PCI_DEVICE(dev);
> > > >
> > > >      /* Don't send event when device is enabled during qemu machine creation:
> > > > @@ -430,6 +431,13 @@ void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > >          return;
> > > >      }
> > > >
> > > > +    /* Hot-plug is disabled on the slot */
> > > > +    if ((sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> > > > +        error_setg(errp, "Device '%s' does not support hot-plug",
> > > > +                         DEVICE(hotplug_dev)->id);
> > > plug and unplug_req are synchronous. so one can skip on "Device '%s'",
> > > user will get this error message as response to device_add/del command.
> > >
> > > and more exactly it's concrete slot that does not support hotplug, how about
> > > "slot doesn't support ..." or just "hotlpug is not supported"
> >
> > Well device name is useful here, while these commands are synchronous
> > others aren't so log parsing might not be synchronous.
> >
> > I do think we should mention slot since that's the reason
> > hotplug failed:
> >     "Device '%s' hot-plug failed: unsupported by slot"
> >
> > > > +        return;
> > > > +    }
> > > > +
> > > >      /* To enable multifunction hot-plug, we just ensure the function
> > > >       * 0 added last. When function 0 is added, we set the sltsta and
> > > >       * inform OS via event notification.
> > > > @@ -464,14 +472,24 @@ static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
> > > >      object_unparent(OBJECT(dev));
> > > >  }
> > > >
> > > > -void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
> > > > +void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_handler,
> > > >                                       DeviceState *dev, Error **errp)
> > > >  {
> > > >      Error *local_err = NULL;
> > > >      PCIDevice *pci_dev = PCI_DEVICE(dev);
> > > >      PCIBus *bus = pci_get_bus(pci_dev);
> > > > +    PCIDevice *hotplug_dev = PCI_DEVICE(hotplug_handler);
> > > > +    uint8_t *exp_cap = hotplug_dev->config + hotplug_dev->exp.exp_cap;
> > > > +    uint32_t sltcap = pci_get_word(exp_cap + PCI_EXP_SLTCAP);
> > > > +
> > > > +    /* Hot-unplug is disabled on the slot */
> > > > +    if ((sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> > > > +        error_setg(errp, "Device '%s' does not support hot-unplug",
> > > > +                         DEVICE(hotplug_dev)->id);
> > > > +        return;
> >
> > Here too let's mention slot since that's the reason
> > hotplug failed:
> >     "Device '%s' hot-unplug failed: unsupported by slot"
> >
> > ?
> 
> Just to mention, for a user it's disabled on the device,

How?
I assumed this is what is used:

commit 530a0963184e57e71a5b538e9161f115df533e96
Author: Julia Suvorova <jusual@redhat.com>
Date:   Wed Feb 26 18:46:07 2020 +0100

    pcie_root_port: Add hotplug disabling option
    
    Make hot-plug/hot-unplug on PCIe Root Ports optional to allow libvirt
    manage it and restrict unplug for the whole machine. This is going to
    prevent user-initiated unplug in guests (Windows mostly).
    Hotplug is enabled by default.
    Usage:
        -device pcie-root-port,hotplug=off,...


so it looks like user has to disable it on the slot.


> and they
> don't have to know how it's implemented. This is the reason for the
> difference between the error text and comments in code.
> 
> Best regards, Julia Suvorova.

On Fri, Apr 17, 2020 at 11:51 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Apr 15, 2020 at 10:20:33PM +0200, Julia Suvorova wrote:
> > On Mon, Apr 13, 2020 at 12:55 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Wed, Apr 08, 2020 at 12:51:20PM +0200, Igor Mammedov wrote:
> > > > On Tue,  7 Apr 2020 16:50:17 +0200
> > > > Julia Suvorova <jusual@redhat.com> wrote:
> > > >
> > > > > Raise an error when trying to hot-plug/unplug a device through QMP to a device
> > > > > with disabled hot-plug capability. This makes the device behaviour more
> > > > > consistent and provides an explanation of the failure in the case of
> > > > > asynchronous unplug.
> > > >
> > > > it applies to hotplug in general (i.e. not only QMP)
> > > >
> > > > >
> > > > > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > > > > ---
> > > > >  hw/pci/pcie.c | 24 +++++++++++++++++++++---
> > > > >  1 file changed, 21 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > > > > index 0eb3a2a5d2..e9798caa8a 100644
> > > > > --- a/hw/pci/pcie.c
> > > > > +++ b/hw/pci/pcie.c
> > > > > @@ -415,6 +415,7 @@ void pcie_cap_slot_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);
> > > > >      PCIDevice *pci_dev = PCI_DEVICE(dev);
> > > > >
> > > > >      /* Don't send event when device is enabled during qemu machine creation:
> > > > > @@ -430,6 +431,13 @@ void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > > >          return;
> > > > >      }
> > > > >
> > > > > +    /* Hot-plug is disabled on the slot */
> > > > > +    if ((sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> > > > > +        error_setg(errp, "Device '%s' does not support hot-plug",
> > > > > +                         DEVICE(hotplug_dev)->id);
> > > > plug and unplug_req are synchronous. so one can skip on "Device '%s'",
> > > > user will get this error message as response to device_add/del command.
> > > >
> > > > and more exactly it's concrete slot that does not support hotplug, how about
> > > > "slot doesn't support ..." or just "hotlpug is not supported"
> > >
> > > Well device name is useful here, while these commands are synchronous
> > > others aren't so log parsing might not be synchronous.
> > >
> > > I do think we should mention slot since that's the reason
> > > hotplug failed:
> > >     "Device '%s' hot-plug failed: unsupported by slot"
> > >
> > > > > +        return;
> > > > > +    }
> > > > > +
> > > > >      /* To enable multifunction hot-plug, we just ensure the function
> > > > >       * 0 added last. When function 0 is added, we set the sltsta and
> > > > >       * inform OS via event notification.
> > > > > @@ -464,14 +472,24 @@ static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
> > > > >      object_unparent(OBJECT(dev));
> > > > >  }
> > > > >
> > > > > -void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
> > > > > +void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_handler,
> > > > >                                       DeviceState *dev, Error **errp)
> > > > >  {
> > > > >      Error *local_err = NULL;
> > > > >      PCIDevice *pci_dev = PCI_DEVICE(dev);
> > > > >      PCIBus *bus = pci_get_bus(pci_dev);
> > > > > +    PCIDevice *hotplug_dev = PCI_DEVICE(hotplug_handler);
> > > > > +    uint8_t *exp_cap = hotplug_dev->config + hotplug_dev->exp.exp_cap;
> > > > > +    uint32_t sltcap = pci_get_word(exp_cap + PCI_EXP_SLTCAP);
> > > > > +
> > > > > +    /* Hot-unplug is disabled on the slot */
> > > > > +    if ((sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> > > > > +        error_setg(errp, "Device '%s' does not support hot-unplug",
> > > > > +                         DEVICE(hotplug_dev)->id);
> > > > > +        return;
> > >
> > > Here too let's mention slot since that's the reason
> > > hotplug failed:
> > >     "Device '%s' hot-unplug failed: unsupported by slot"
> > >
> > > ?
> >
> > Just to mention, for a user it's disabled on the device,
>
> How?
> I assumed this is what is used:
>
> commit 530a0963184e57e71a5b538e9161f115df533e96
> Author: Julia Suvorova <jusual@redhat.com>
> Date:   Wed Feb 26 18:46:07 2020 +0100
>
>     pcie_root_port: Add hotplug disabling option
>
>     Make hot-plug/hot-unplug on PCIe Root Ports optional to allow libvirt
>     manage it and restrict unplug for the whole machine. This is going to
>     prevent user-initiated unplug in guests (Windows mostly).
>     Hotplug is enabled by default.
>     Usage:
>         -device pcie-root-port,hotplug=off,...
>
>
> so it looks like user has to disable it on the slot.

By device I mean root port, and it's shown in the error message. If
you think it's not correct, I'll change it.

On Fri, Apr 17, 2020 at 01:03:01PM +0200, Julia Suvorova wrote:
> On Fri, Apr 17, 2020 at 11:51 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Wed, Apr 15, 2020 at 10:20:33PM +0200, Julia Suvorova wrote:
> > > On Mon, Apr 13, 2020 at 12:55 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Wed, Apr 08, 2020 at 12:51:20PM +0200, Igor Mammedov wrote:
> > > > > On Tue,  7 Apr 2020 16:50:17 +0200
> > > > > Julia Suvorova <jusual@redhat.com> wrote:
> > > > >
> > > > > > Raise an error when trying to hot-plug/unplug a device through QMP to a device
> > > > > > with disabled hot-plug capability. This makes the device behaviour more
> > > > > > consistent and provides an explanation of the failure in the case of
> > > > > > asynchronous unplug.
> > > > >
> > > > > it applies to hotplug in general (i.e. not only QMP)
> > > > >
> > > > > >
> > > > > > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > > > > > ---
> > > > > >  hw/pci/pcie.c | 24 +++++++++++++++++++++---
> > > > > >  1 file changed, 21 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > > > > > index 0eb3a2a5d2..e9798caa8a 100644
> > > > > > --- a/hw/pci/pcie.c
> > > > > > +++ b/hw/pci/pcie.c
> > > > > > @@ -415,6 +415,7 @@ void pcie_cap_slot_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);
> > > > > >      PCIDevice *pci_dev = PCI_DEVICE(dev);
> > > > > >
> > > > > >      /* Don't send event when device is enabled during qemu machine creation:
> > > > > > @@ -430,6 +431,13 @@ void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > > > >          return;
> > > > > >      }
> > > > > >
> > > > > > +    /* Hot-plug is disabled on the slot */
> > > > > > +    if ((sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> > > > > > +        error_setg(errp, "Device '%s' does not support hot-plug",
> > > > > > +                         DEVICE(hotplug_dev)->id);
> > > > > plug and unplug_req are synchronous. so one can skip on "Device '%s'",
> > > > > user will get this error message as response to device_add/del command.
> > > > >
> > > > > and more exactly it's concrete slot that does not support hotplug, how about
> > > > > "slot doesn't support ..." or just "hotlpug is not supported"
> > > >
> > > > Well device name is useful here, while these commands are synchronous
> > > > others aren't so log parsing might not be synchronous.
> > > >
> > > > I do think we should mention slot since that's the reason
> > > > hotplug failed:
> > > >     "Device '%s' hot-plug failed: unsupported by slot"
> > > >
> > > > > > +        return;
> > > > > > +    }
> > > > > > +
> > > > > >      /* To enable multifunction hot-plug, we just ensure the function
> > > > > >       * 0 added last. When function 0 is added, we set the sltsta and
> > > > > >       * inform OS via event notification.
> > > > > > @@ -464,14 +472,24 @@ static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
> > > > > >      object_unparent(OBJECT(dev));
> > > > > >  }
> > > > > >
> > > > > > -void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
> > > > > > +void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_handler,
> > > > > >                                       DeviceState *dev, Error **errp)
> > > > > >  {
> > > > > >      Error *local_err = NULL;
> > > > > >      PCIDevice *pci_dev = PCI_DEVICE(dev);
> > > > > >      PCIBus *bus = pci_get_bus(pci_dev);
> > > > > > +    PCIDevice *hotplug_dev = PCI_DEVICE(hotplug_handler);
> > > > > > +    uint8_t *exp_cap = hotplug_dev->config + hotplug_dev->exp.exp_cap;
> > > > > > +    uint32_t sltcap = pci_get_word(exp_cap + PCI_EXP_SLTCAP);
> > > > > > +
> > > > > > +    /* Hot-unplug is disabled on the slot */
> > > > > > +    if ((sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> > > > > > +        error_setg(errp, "Device '%s' does not support hot-unplug",
> > > > > > +                         DEVICE(hotplug_dev)->id);
> > > > > > +        return;
> > > >
> > > > Here too let's mention slot since that's the reason
> > > > hotplug failed:
> > > >     "Device '%s' hot-unplug failed: unsupported by slot"
> > > >
> > > > ?
> > >
> > > Just to mention, for a user it's disabled on the device,
> >
> > How?
> > I assumed this is what is used:
> >
> > commit 530a0963184e57e71a5b538e9161f115df533e96
> > Author: Julia Suvorova <jusual@redhat.com>
> > Date:   Wed Feb 26 18:46:07 2020 +0100
> >
> >     pcie_root_port: Add hotplug disabling option
> >
> >     Make hot-plug/hot-unplug on PCIe Root Ports optional to allow libvirt
> >     manage it and restrict unplug for the whole machine. This is going to
> >     prevent user-initiated unplug in guests (Windows mostly).
> >     Hotplug is enabled by default.
> >     Usage:
> >         -device pcie-root-port,hotplug=off,...
> >
> >
> > so it looks like user has to disable it on the slot.
> 
> By device I mean root port, and it's shown in the error message. If
> you think it's not correct, I'll change it.

Oh I see. It might be confusing for user: I add device A
and get a report "Device B does not support hot-plug".
How about:
	"Hot-plug failed: unsupported by port device %s"?

-- 
MST

On Fri, Apr 17, 2020 at 1:11 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Apr 17, 2020 at 01:03:01PM +0200, Julia Suvorova wrote:
> > On Fri, Apr 17, 2020 at 11:51 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Wed, Apr 15, 2020 at 10:20:33PM +0200, Julia Suvorova wrote:
> > > > On Mon, Apr 13, 2020 at 12:55 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > On Wed, Apr 08, 2020 at 12:51:20PM +0200, Igor Mammedov wrote:
> > > > > > On Tue,  7 Apr 2020 16:50:17 +0200
> > > > > > Julia Suvorova <jusual@redhat.com> wrote:
> > > > > >
> > > > > > > Raise an error when trying to hot-plug/unplug a device through QMP to a device
> > > > > > > with disabled hot-plug capability. This makes the device behaviour more
> > > > > > > consistent and provides an explanation of the failure in the case of
> > > > > > > asynchronous unplug.
> > > > > >
> > > > > > it applies to hotplug in general (i.e. not only QMP)
> > > > > >
> > > > > > >
> > > > > > > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > > > > > > ---
> > > > > > >  hw/pci/pcie.c | 24 +++++++++++++++++++++---
> > > > > > >  1 file changed, 21 insertions(+), 3 deletions(-)
> > > > > > >
> > > > > > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > > > > > > index 0eb3a2a5d2..e9798caa8a 100644
> > > > > > > --- a/hw/pci/pcie.c
> > > > > > > +++ b/hw/pci/pcie.c
> > > > > > > @@ -415,6 +415,7 @@ void pcie_cap_slot_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);
> > > > > > >      PCIDevice *pci_dev = PCI_DEVICE(dev);
> > > > > > >
> > > > > > >      /* Don't send event when device is enabled during qemu machine creation:
> > > > > > > @@ -430,6 +431,13 @@ void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > > > > >          return;
> > > > > > >      }
> > > > > > >
> > > > > > > +    /* Hot-plug is disabled on the slot */
> > > > > > > +    if ((sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> > > > > > > +        error_setg(errp, "Device '%s' does not support hot-plug",
> > > > > > > +                         DEVICE(hotplug_dev)->id);
> > > > > > plug and unplug_req are synchronous. so one can skip on "Device '%s'",
> > > > > > user will get this error message as response to device_add/del command.
> > > > > >
> > > > > > and more exactly it's concrete slot that does not support hotplug, how about
> > > > > > "slot doesn't support ..." or just "hotlpug is not supported"
> > > > >
> > > > > Well device name is useful here, while these commands are synchronous
> > > > > others aren't so log parsing might not be synchronous.
> > > > >
> > > > > I do think we should mention slot since that's the reason
> > > > > hotplug failed:
> > > > >     "Device '%s' hot-plug failed: unsupported by slot"
> > > > >
> > > > > > > +        return;
> > > > > > > +    }
> > > > > > > +
> > > > > > >      /* To enable multifunction hot-plug, we just ensure the function
> > > > > > >       * 0 added last. When function 0 is added, we set the sltsta and
> > > > > > >       * inform OS via event notification.
> > > > > > > @@ -464,14 +472,24 @@ static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
> > > > > > >      object_unparent(OBJECT(dev));
> > > > > > >  }
> > > > > > >
> > > > > > > -void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
> > > > > > > +void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_handler,
> > > > > > >                                       DeviceState *dev, Error **errp)
> > > > > > >  {
> > > > > > >      Error *local_err = NULL;
> > > > > > >      PCIDevice *pci_dev = PCI_DEVICE(dev);
> > > > > > >      PCIBus *bus = pci_get_bus(pci_dev);
> > > > > > > +    PCIDevice *hotplug_dev = PCI_DEVICE(hotplug_handler);
> > > > > > > +    uint8_t *exp_cap = hotplug_dev->config + hotplug_dev->exp.exp_cap;
> > > > > > > +    uint32_t sltcap = pci_get_word(exp_cap + PCI_EXP_SLTCAP);
> > > > > > > +
> > > > > > > +    /* Hot-unplug is disabled on the slot */
> > > > > > > +    if ((sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> > > > > > > +        error_setg(errp, "Device '%s' does not support hot-unplug",
> > > > > > > +                         DEVICE(hotplug_dev)->id);
> > > > > > > +        return;
> > > > >
> > > > > Here too let's mention slot since that's the reason
> > > > > hotplug failed:
> > > > >     "Device '%s' hot-unplug failed: unsupported by slot"
> > > > >
> > > > > ?
> > > >
> > > > Just to mention, for a user it's disabled on the device,
> > >
> > > How?
> > > I assumed this is what is used:
> > >
> > > commit 530a0963184e57e71a5b538e9161f115df533e96
> > > Author: Julia Suvorova <jusual@redhat.com>
> > > Date:   Wed Feb 26 18:46:07 2020 +0100
> > >
> > >     pcie_root_port: Add hotplug disabling option
> > >
> > >     Make hot-plug/hot-unplug on PCIe Root Ports optional to allow libvirt
> > >     manage it and restrict unplug for the whole machine. This is going to
> > >     prevent user-initiated unplug in guests (Windows mostly).
> > >     Hotplug is enabled by default.
> > >     Usage:
> > >         -device pcie-root-port,hotplug=off,...
> > >
> > >
> > > so it looks like user has to disable it on the slot.
> >
> > By device I mean root port, and it's shown in the error message. If
> > you think it's not correct, I'll change it.
>
> Oh I see. It might be confusing for user: I add device A
> and get a report "Device B does not support hot-plug".
> How about:
>         "Hot-plug failed: unsupported by port device %s"?

Looks good.

Best regards, Julia Suvorova.

Hi Julia,

On 4/7/20 5:50 PM, Julia Suvorova wrote:
> Raise an error when trying to hot-plug/unplug a device through QMP to a device
> with disabled hot-plug capability. This makes the device behaviour more
> consistent and provides an explanation of the failure in the case of
> asynchronous unplug.
>
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> ---
>   hw/pci/pcie.c | 24 +++++++++++++++++++++---
>   1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 0eb3a2a5d2..e9798caa8a 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -415,6 +415,7 @@ void pcie_cap_slot_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);
>       PCIDevice *pci_dev = PCI_DEVICE(dev);
>   
>       /* Don't send event when device is enabled during qemu machine creation:
> @@ -430,6 +431,13 @@ void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>           return;
>       }
>   
> +    /* Hot-plug is disabled on the slot */
> +    if ((sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> +        error_setg(errp, "Device '%s' does not support hot-plug",
> +                         DEVICE(hotplug_dev)->id);
> +        return;
> +    }
> +
>       /* To enable multifunction hot-plug, we just ensure the function
>        * 0 added last. When function 0 is added, we set the sltsta and
>        * inform OS via event notification.
> @@ -464,14 +472,24 @@ static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
>       object_unparent(OBJECT(dev));
>   }
>   
> -void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
> +void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_handler,
>                                        DeviceState *dev, Error **errp)
>   {
>       Error *local_err = NULL;
>       PCIDevice *pci_dev = PCI_DEVICE(dev);
>       PCIBus *bus = pci_get_bus(pci_dev);
> +    PCIDevice *hotplug_dev = PCI_DEVICE(hotplug_handler);
> +    uint8_t *exp_cap = hotplug_dev->config + hotplug_dev->exp.exp_cap;
> +    uint32_t sltcap = pci_get_word(exp_cap + PCI_EXP_SLTCAP);
> +
> +    /* Hot-unplug is disabled on the slot */
> +    if ((sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> +        error_setg(errp, "Device '%s' does not support hot-unplug",
> +                         DEVICE(hotplug_dev)->id);
> +        return;
> +    }

Since this chunk appears twice I would consider refactoring it into
a helper function. (I see the error message is different, but I suppose 
it can be tweaked)

>   
> -    pcie_cap_slot_plug_common(PCI_DEVICE(hotplug_dev), dev, &local_err);
> +    pcie_cap_slot_plug_common(hotplug_dev, dev, &local_err);

It doesn't seems related to this patch.

>       if (local_err) {
>           error_propagate(errp, local_err);
>           return;
> @@ -490,7 +508,7 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
>           return;
>       }
>   
> -    pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev));
> +    pcie_cap_slot_push_attention_button(hotplug_dev);

Same here, maybe you can split it in 2 patches.

Thanks,
Marcel

>   }
>   
>   /* pci express slot for pci express root/downstream port

On Tue, Apr 14, 2020 at 10:07 AM Marcel Apfelbaum
<marcel.apfelbaum@gmail.com> wrote:
>
> Hi Julia,
>
> On 4/7/20 5:50 PM, Julia Suvorova wrote:
> > Raise an error when trying to hot-plug/unplug a device through QMP to a device
> > with disabled hot-plug capability. This makes the device behaviour more
> > consistent and provides an explanation of the failure in the case of
> > asynchronous unplug.
> >
> > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > ---
> >   hw/pci/pcie.c | 24 +++++++++++++++++++++---
> >   1 file changed, 21 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index 0eb3a2a5d2..e9798caa8a 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -415,6 +415,7 @@ void pcie_cap_slot_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);
> >       PCIDevice *pci_dev = PCI_DEVICE(dev);
> >
> >       /* Don't send event when device is enabled during qemu machine creation:
> > @@ -430,6 +431,13 @@ void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> >           return;
> >       }
> >
> > +    /* Hot-plug is disabled on the slot */
> > +    if ((sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> > +        error_setg(errp, "Device '%s' does not support hot-plug",
> > +                         DEVICE(hotplug_dev)->id);
> > +        return;
> > +    }
> > +
> >       /* To enable multifunction hot-plug, we just ensure the function
> >        * 0 added last. When function 0 is added, we set the sltsta and
> >        * inform OS via event notification.
> > @@ -464,14 +472,24 @@ static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
> >       object_unparent(OBJECT(dev));
> >   }
> >
> > -void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
> > +void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_handler,
> >                                        DeviceState *dev, Error **errp)
> >   {
> >       Error *local_err = NULL;
> >       PCIDevice *pci_dev = PCI_DEVICE(dev);
> >       PCIBus *bus = pci_get_bus(pci_dev);
> > +    PCIDevice *hotplug_dev = PCI_DEVICE(hotplug_handler);
> > +    uint8_t *exp_cap = hotplug_dev->config + hotplug_dev->exp.exp_cap;
> > +    uint32_t sltcap = pci_get_word(exp_cap + PCI_EXP_SLTCAP);
> > +
> > +    /* Hot-unplug is disabled on the slot */
> > +    if ((sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> > +        error_setg(errp, "Device '%s' does not support hot-unplug",
> > +                         DEVICE(hotplug_dev)->id);
> > +        return;
> > +    }
>
> Since this chunk appears twice I would consider refactoring it into
> a helper function. (I see the error message is different, but I suppose
> it can be tweaked)

Okay.

> >
> > -    pcie_cap_slot_plug_common(PCI_DEVICE(hotplug_dev), dev, &local_err);
> > +    pcie_cap_slot_plug_common(hotplug_dev, dev, &local_err);
>
> It doesn't seems related to this patch.
>
> >       if (local_err) {
> >           error_propagate(errp, local_err);
> >           return;
> > @@ -490,7 +508,7 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
> >           return;
> >       }
> >
> > -    pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev));
> > +    pcie_cap_slot_push_attention_button(hotplug_dev);
>
> Same here, maybe you can split it in 2 patches.

Yes, but it doesn't make sense by itself. Just cleaning necessary with
a new variable introduced.

Best regards, Julia Suvorova.

On Tue, Apr 14, 2020 at 10:07 AM Marcel Apfelbaum
<marcel.apfelbaum@gmail.com> wrote:
>
> Hi Julia,
>
> On 4/7/20 5:50 PM, Julia Suvorova wrote:
> > Raise an error when trying to hot-plug/unplug a device through QMP to a device
> > with disabled hot-plug capability. This makes the device behaviour more
> > consistent and provides an explanation of the failure in the case of
> > asynchronous unplug.
> >
> > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > ---
> >   hw/pci/pcie.c | 24 +++++++++++++++++++++---
> >   1 file changed, 21 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index 0eb3a2a5d2..e9798caa8a 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -415,6 +415,7 @@ void pcie_cap_slot_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);
> >       PCIDevice *pci_dev = PCI_DEVICE(dev);
> >
> >       /* Don't send event when device is enabled during qemu machine creation:
> > @@ -430,6 +431,13 @@ void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> >           return;
> >       }
> >
> > +    /* Hot-plug is disabled on the slot */
> > +    if ((sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> > +        error_setg(errp, "Device '%s' does not support hot-plug",
> > +                         DEVICE(hotplug_dev)->id);
> > +        return;
> > +    }
> > +
> >       /* To enable multifunction hot-plug, we just ensure the function
> >        * 0 added last. When function 0 is added, we set the sltsta and
> >        * inform OS via event notification.
> > @@ -464,14 +472,24 @@ static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
> >       object_unparent(OBJECT(dev));
> >   }
> >
> > -void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
> > +void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_handler,
> >                                        DeviceState *dev, Error **errp)
> >   {
> >       Error *local_err = NULL;
> >       PCIDevice *pci_dev = PCI_DEVICE(dev);
> >       PCIBus *bus = pci_get_bus(pci_dev);
> > +    PCIDevice *hotplug_dev = PCI_DEVICE(hotplug_handler);
> > +    uint8_t *exp_cap = hotplug_dev->config + hotplug_dev->exp.exp_cap;
> > +    uint32_t sltcap = pci_get_word(exp_cap + PCI_EXP_SLTCAP);
> > +
> > +    /* Hot-unplug is disabled on the slot */
> > +    if ((sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> > +        error_setg(errp, "Device '%s' does not support hot-unplug",
> > +                         DEVICE(hotplug_dev)->id);
> > +        return;
> > +    }
>
> Since this chunk appears twice I would consider refactoring it into
> a helper function. (I see the error message is different, but I suppose
> it can be tweaked)

This only increases the amount of code, since you need to do
error_propagate() after the function call.