On Tue, Apr 14, 2020 at 10:07 AM Marcel Apfelbaum <marcel.apfelb...@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 <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); > > + 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.