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 <m...@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 <jus...@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 <jus...@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 <jus...@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.