| Message ID | 20181105110313.29312-4-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x/zpci: some hotplug handler cleanups | expand |
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 <david@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; >
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 <david@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 <walling@linux.ibm.com>
> > 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 ;) > Mentioned it in the cover letter, part of another series (also CC'ed to qemu-s390x@nongnu.org, so maybe easier to find for you). >> >> > > The above concerns do not relate to any functionality of the code, so with them addressed, then: > > Reviewed-by: Collin Walling <walling@linux.ibm.com> > Thanks!
On 11/7/18 2:36 PM, David Hildenbrand wrote: >> >> 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 ;) >> > > Mentioned it in the cover letter, part of another series > (also CC'ed to qemu-s390x@nongnu.org, so maybe easier to find for you). Ah, I see that now. Thanks for pointing that out (again).
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"); 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;
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 <david@redhat.com> --- hw/s390x/s390-pci-bus.c | 43 +++++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 17 deletions(-)