* [PATCH] hw/pci/pcie: Forbid hot-plug via QMP if it's disabled on the slot @ 2020-04-07 14:50 Julia Suvorova 2020-04-08 10:51 ` Igor Mammedov 2020-04-14 8:07 ` Marcel Apfelbaum 0 siblings, 2 replies; 11+ messages in thread From: Julia Suvorova @ 2020-04-07 14:50 UTC (permalink / raw) To: qemu-devel Cc: Daniel P. Berrangé, Laine Stump, Michael S. Tsirkin, qemu-trivial, Julia Suvorova 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] hw/pci/pcie: Forbid hot-plug via QMP if it's disabled on the slot 2020-04-07 14:50 [PATCH] hw/pci/pcie: Forbid hot-plug via QMP if it's disabled on the slot Julia Suvorova @ 2020-04-08 10:51 ` Igor Mammedov 2020-04-13 10:54 ` Michael S. Tsirkin 2020-04-14 8:07 ` Marcel Apfelbaum 1 sibling, 1 reply; 11+ messages in thread From: Igor Mammedov @ 2020-04-08 10:51 UTC (permalink / raw) To: Julia Suvorova Cc: qemu-trivial, Daniel P. Berrangé, qemu-devel, Laine Stump, Michael S. Tsirkin 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] hw/pci/pcie: Forbid hot-plug via QMP if it's disabled on the slot 2020-04-08 10:51 ` Igor Mammedov @ 2020-04-13 10:54 ` Michael S. Tsirkin 2020-04-15 20:20 ` Julia Suvorova 0 siblings, 1 reply; 11+ messages in thread From: Michael S. Tsirkin @ 2020-04-13 10:54 UTC (permalink / raw) To: Igor Mammedov Cc: qemu-trivial, Daniel P. Berrangé, Julia Suvorova, qemu-devel, Laine Stump 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] hw/pci/pcie: Forbid hot-plug via QMP if it's disabled on the slot 2020-04-13 10:54 ` Michael S. Tsirkin @ 2020-04-15 20:20 ` Julia Suvorova 2020-04-17 9:51 ` Michael S. Tsirkin 0 siblings, 1 reply; 11+ messages in thread From: Julia Suvorova @ 2020-04-15 20:20 UTC (permalink / raw) To: Michael S. Tsirkin Cc: qemu-trivial, Igor Mammedov, Daniel P. Berrangé, QEMU Developers, Laine Stump 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] hw/pci/pcie: Forbid hot-plug via QMP if it's disabled on the slot 2020-04-15 20:20 ` Julia Suvorova @ 2020-04-17 9:51 ` Michael S. Tsirkin 2020-04-17 11:03 ` Julia Suvorova 0 siblings, 1 reply; 11+ messages in thread From: Michael S. Tsirkin @ 2020-04-17 9:51 UTC (permalink / raw) To: Julia Suvorova Cc: qemu-trivial, Igor Mammedov, Daniel P. Berrangé, QEMU Developers, Laine Stump 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] hw/pci/pcie: Forbid hot-plug via QMP if it's disabled on the slot 2020-04-17 9:51 ` Michael S. Tsirkin @ 2020-04-17 11:03 ` Julia Suvorova 2020-04-17 11:10 ` Michael S. Tsirkin 0 siblings, 1 reply; 11+ messages in thread From: Julia Suvorova @ 2020-04-17 11:03 UTC (permalink / raw) To: Michael S. Tsirkin Cc: qemu-trivial, Igor Mammedov, Daniel P. Berrangé, QEMU Developers, Laine Stump 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] hw/pci/pcie: Forbid hot-plug via QMP if it's disabled on the slot 2020-04-17 11:03 ` Julia Suvorova @ 2020-04-17 11:10 ` Michael S. Tsirkin 2020-04-17 12:17 ` Julia Suvorova 0 siblings, 1 reply; 11+ messages in thread From: Michael S. Tsirkin @ 2020-04-17 11:10 UTC (permalink / raw) To: Julia Suvorova Cc: qemu-trivial, Igor Mammedov, Daniel P. Berrangé, QEMU Developers, Laine Stump 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] hw/pci/pcie: Forbid hot-plug via QMP if it's disabled on the slot 2020-04-17 11:10 ` Michael S. Tsirkin @ 2020-04-17 12:17 ` Julia Suvorova 0 siblings, 0 replies; 11+ messages in thread From: Julia Suvorova @ 2020-04-17 12:17 UTC (permalink / raw) To: Michael S. Tsirkin Cc: qemu-trivial, Igor Mammedov, Daniel P. Berrangé, QEMU Developers, Laine Stump 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] hw/pci/pcie: Forbid hot-plug via QMP if it's disabled on the slot 2020-04-07 14:50 [PATCH] hw/pci/pcie: Forbid hot-plug via QMP if it's disabled on the slot Julia Suvorova 2020-04-08 10:51 ` Igor Mammedov @ 2020-04-14 8:07 ` Marcel Apfelbaum 2020-04-15 20:13 ` Julia Suvorova 2020-04-20 21:41 ` Julia Suvorova 1 sibling, 2 replies; 11+ messages in thread From: Marcel Apfelbaum @ 2020-04-14 8:07 UTC (permalink / raw) To: Julia Suvorova, qemu-devel Cc: qemu-trivial, Daniel P. Berrangé, Laine Stump, Michael S. Tsirkin 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] hw/pci/pcie: Forbid hot-plug via QMP if it's disabled on the slot 2020-04-14 8:07 ` Marcel Apfelbaum @ 2020-04-15 20:13 ` Julia Suvorova 2020-04-20 21:41 ` Julia Suvorova 1 sibling, 0 replies; 11+ messages in thread From: Julia Suvorova @ 2020-04-15 20:13 UTC (permalink / raw) To: Marcel Apfelbaum Cc: qemu-trivial, Daniel P. Berrangé, QEMU Developers, Laine Stump, Michael S. Tsirkin 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] hw/pci/pcie: Forbid hot-plug via QMP if it's disabled on the slot 2020-04-14 8:07 ` Marcel Apfelbaum 2020-04-15 20:13 ` Julia Suvorova @ 2020-04-20 21:41 ` Julia Suvorova 1 sibling, 0 replies; 11+ messages in thread From: Julia Suvorova @ 2020-04-20 21:41 UTC (permalink / raw) To: Marcel Apfelbaum Cc: Daniel P. Berrangé, QEMU Developers, Laine Stump, Michael S. Tsirkin 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-04-20 21:42 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-04-07 14:50 [PATCH] hw/pci/pcie: Forbid hot-plug via QMP if it's disabled on the slot Julia Suvorova 2020-04-08 10:51 ` Igor Mammedov 2020-04-13 10:54 ` Michael S. Tsirkin 2020-04-15 20:20 ` Julia Suvorova 2020-04-17 9:51 ` Michael S. Tsirkin 2020-04-17 11:03 ` Julia Suvorova 2020-04-17 11:10 ` Michael S. Tsirkin 2020-04-17 12:17 ` Julia Suvorova 2020-04-14 8:07 ` Marcel Apfelbaum 2020-04-15 20:13 ` Julia Suvorova 2020-04-20 21:41 ` Julia Suvorova
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).