On 11/5/18 6:50 AM, David Hildenbrand wrote: > On 05.11.18 12:03, David Hildenbrand wrote: >> Let's move most of the checks to the new pre_plug handler. As a PCI >> bridge is just a PCI device, we can simplify the code. >> >> Notes: We cannot yet move the MSIX check or device ID creation + >> zPCI device creation to the pre_plug handler as both parts are not >> fixed before actual device realization (and therefore after pre_plug and >> before plug). Once that part is factored out, we can move these parts to >> the pre_plug handler, too and therefore remove all possible errors from >> the plug handler. >> >> Signed-off-by: David Hildenbrand <da...@redhat.com> >> --- >> hw/s390x/s390-pci-bus.c | 43 +++++++++++++++++++++++++---------------- >> 1 file changed, 26 insertions(+), 17 deletions(-) >> >> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c >> index 68660eac74..1849f9d334 100644 >> --- a/hw/s390x/s390-pci-bus.c >> +++ b/hw/s390x/s390-pci-bus.c >> @@ -806,11 +806,31 @@ static bool s390_pci_alloc_idx(S390pciState *s, >> S390PCIBusDevice *pbdev) >> } >> >> pbdev->idx = idx; >> - s->next_idx = (idx + 1) & FH_MASK_INDEX; >> - >> return true; >> } >> >> +static void s390_pcihost_pre_plug(HotplugHandler *hotplug_dev, DeviceState >> *dev, >> + Error **errp) >> +{ >> + S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev); >> + >> + if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) { >> + PCIDevice *pdev = PCI_DEVICE(dev); >> + >> + if (pdev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) { >> + error_setg(errp, "multifunction not supported in s390"); >> + return; >> + } >> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) { >> + S390PCIBusDevice *pbdev = S390_PCI_DEVICE(dev); >> + >> + if (!s390_pci_alloc_idx(s, pbdev)) { >> + error_setg(errp, "no slot for plugging zpci device"); >> + return; >> + } >> + } >> +} >> + >> static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev, >> Error **errp) >> { >> @@ -823,11 +843,6 @@ static void s390_pcihost_plug(HotplugHandler >> *hotplug_dev, DeviceState *dev,>> PCIBridge *pb = PCI_BRIDGE(dev); >> PCIDevice *pdev = PCI_DEVICE(dev); >> >> - if (pdev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) { >> - error_setg(errp, "multifunction not supported in s390"); >> - return; >> - } >> - >> pci_bridge_map_irq(pb, dev->id, s390_pci_map_irq); >> pci_setup_iommu(&pb->sec_bus, s390_pci_dma_iommu, s); >> >> @@ -847,11 +862,6 @@ static void s390_pcihost_plug(HotplugHandler >> *hotplug_dev, DeviceState *dev, >> } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) { >> pdev = PCI_DEVICE(dev); >> >> - if (pdev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) { >> - error_setg(errp, "multifunction not supported in s390"); >> - return; >> - } >> - >> if (!dev->id) { >> /* In the case the PCI device does not define an id */ >> /* we generate one based on the PCI address */ >> @@ -883,7 +893,7 @@ static void s390_pcihost_plug(HotplugHandler >> *hotplug_dev, DeviceState *dev, >> >> if (s390_pci_msix_init(pbdev)) { >> error_setg(errp, "MSI-X support is mandatory " >> - "in the S390 architecture"); >> + "in the S390 architecture"); > > I will drop this unrelated change. > >> return; >> } >> >> @@ -894,10 +904,8 @@ static void s390_pcihost_plug(HotplugHandler >> *hotplug_dev, DeviceState *dev, >> } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) { >> pbdev = S390_PCI_DEVICE(dev); >> >> - if (!s390_pci_alloc_idx(s, pbdev)) { >> - error_setg(errp, "no slot for plugging zpci device"); >> - return; >> - } >> + /* the allocated idx is actually getting used */ >> + s->next_idx = (pbdev->idx + 1) & FH_MASK_INDEX; >> pbdev->fh = pbdev->idx; >> QTAILQ_INSERT_TAIL(&s->zpci_devs, pbdev, link); >> g_hash_table_insert(s->zpci_table, &pbdev->idx, pbdev); >> @@ -1030,6 +1038,7 @@ static void s390_pcihost_class_init(ObjectClass >> *klass, void *data) >> >> dc->reset = s390_pcihost_reset; >> dc->realize = s390_pcihost_realize; >> + hc->pre_plug = s390_pcihost_pre_plug; >> hc->plug = s390_pcihost_plug; >> hc->unplug = s390_pcihost_unplug; >> msi_nonbroken = true; >>
When did these function names drop the *_hot_plug postfix? Latest master does not reflect this change for me. Just figured I'd mention it now in case merging becomes a pain later ;) > > The above concerns do not relate to any functionality of the code, so with them addressed, then: Reviewed-by: Collin Walling <wall...@linux.ibm.com> -- Respectfully, - Collin Walling