Re: [PATCH] hw/pci/pcie: Forbid hot-plug via QMP if it's disabled on the slot

[Julia Suvorova]

On Mon, Apr 13, 2020 at 12:55 PM Michael S. Tsirkin <address@hidden> 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 <address@hidden> 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 <address@hidden>
> > > ---
> > >  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.