qemu-trivial
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Julia Suvorova
Subject: Re: [PATCH] hw/pci/pcie: Forbid hot-plug via QMP if it's disabled on the slot
Date: Fri, 17 Apr 2020 14:17:19 +0200

On Fri, Apr 17, 2020 at 1:11 PM Michael S. Tsirkin <address@hidden> 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 <address@hidden> 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 <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,
> > >
> > > How?
> > > I assumed this is what is used:
> > >
> > > commit 530a0963184e57e71a5b538e9161f115df533e96
> > > Author: Julia Suvorova <address@hidden>
> > > 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.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]