s390x/zpci: some hotplug handler cleanups

I fail to see why this is useful as we require MSIX always and completely fail adding a device.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-pci-bus.c | 2 --
 hw/s390x/s390-pci-bus.h | 1 -
 2 files changed, 3 deletions(-)

hw/s390x/s390-pci-bus.c +0 -2

index e1b14b131b..1eaae3aca6 100644
@@ -745,7 +745,6 @@ static int s390_pci_msix_init(S390PCIBusDevice *pbdev)
 
     pos = pci_find_capability(pbdev->pdev, PCI_CAP_ID_MSIX);
     if (!pos) {
-        pbdev->msix.available = false;
         return -1;
     }
 

@@ -761,7 +760,6 @@ static int s390_pci_msix_init(S390PCIBusDevice *pbdev)
     pbdev->msix.pba_bar = pba & PCI_MSIX_FLAGS_BIRMASK;
     pbdev->msix.pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
     pbdev->msix.entries = (ctrl & PCI_MSIX_FLAGS_QSIZE) + 1;
-    pbdev->msix.available = true;
 
     name = g_strdup_printf("msix-s390-%04x", pbdev->uid);
     memory_region_init_io(&pbdev->msix_notify_mr, OBJECT(pbdev),

hw/s390x/s390-pci-bus.h +0 -0

index 1f7f9b5814..f47a0f2da5 100644

Quoted 34 lines · On Mon, 5 Nov 2018 12:03:10 +0100 David Hildenbrand <[email protected]> wrote:

I fail to see why this is useful as we require MSIX always and completely fail adding a device.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-pci-bus.c | 2 --
 hw/s390x/s390-pci-bus.h | 1 -
 2 files changed, 3 deletions(-)

hw/s390x/s390-pci-bus.c +0 -2

index e1b14b131b..1eaae3aca6 100644
@@ -745,7 +745,6 @@ static int s390_pci_msix_init(S390PCIBusDevice *pbdev)
 
     pos = pci_find_capability(pbdev->pdev, PCI_CAP_ID_MSIX);
     if (!pos) {
-        pbdev->msix.available = false;
         return -1;
     }
 

@@ -761,7 +760,6 @@ static int s390_pci_msix_init(S390PCIBusDevice *pbdev)
     pbdev->msix.pba_bar = pba & PCI_MSIX_FLAGS_BIRMASK;
     pbdev->msix.pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
     pbdev->msix.entries = (ctrl & PCI_MSIX_FLAGS_QSIZE) + 1;
-    pbdev->msix.available = true;
 
     name = g_strdup_printf("msix-s390-%04x", pbdev->uid);
     memory_region_init_io(&pbdev->msix_notify_mr, OBJECT(pbdev),

hw/s390x/s390-pci-bus.h +0 -0

index 1f7f9b5814..f47a0f2da5 100644

hw/s390x/s390-pci-bus.h typedef struct ChscSeiNt2Res { · line 257

Quoted 7 lines

hw/s390x/s390-pci-bus.h +0 -1

@@ -252,7 +252,6 @@ typedef struct ChscSeiNt2Res {
 } QEMU_PACKED ChscSeiNt2Res;
 
 typedef struct S390MsixInfo {
-    bool available;
     uint8_t table_bar;
     uint8_t pba_bar;
     uint16_t entries;

OK, so that was a write-only variable? :)

Question for the IBMers: is there any change we might have a different implementation not relying on msi-x in the future?

Quoted 47 lines · Cornelia Huck

Quoted 34 lines · On Mon, 5 Nov 2018 12:03:10 +0100 David Hildenbrand <[email protected]> wrote:

I fail to see why this is useful as we require MSIX always and completely fail adding a device.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-pci-bus.c | 2 --
 hw/s390x/s390-pci-bus.h | 1 -
 2 files changed, 3 deletions(-)

hw/s390x/s390-pci-bus.c +0 -2

index e1b14b131b..1eaae3aca6 100644
@@ -745,7 +745,6 @@ static int s390_pci_msix_init(S390PCIBusDevice *pbdev)
 
     pos = pci_find_capability(pbdev->pdev, PCI_CAP_ID_MSIX);
     if (!pos) {
-        pbdev->msix.available = false;
         return -1;
     }
 

@@ -761,7 +760,6 @@ static int s390_pci_msix_init(S390PCIBusDevice *pbdev)
     pbdev->msix.pba_bar = pba & PCI_MSIX_FLAGS_BIRMASK;
     pbdev->msix.pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
     pbdev->msix.entries = (ctrl & PCI_MSIX_FLAGS_QSIZE) + 1;
-    pbdev->msix.available = true;
 
     name = g_strdup_printf("msix-s390-%04x", pbdev->uid);
     memory_region_init_io(&pbdev->msix_notify_mr, OBJECT(pbdev),

hw/s390x/s390-pci-bus.h +0 -0

index 1f7f9b5814..f47a0f2da5 100644

hw/s390x/s390-pci-bus.h typedef struct ChscSeiNt2Res { · line 257

Quoted 7 lines

hw/s390x/s390-pci-bus.h +0 -1

@@ -252,7 +252,6 @@ typedef struct ChscSeiNt2Res {
 } QEMU_PACKED ChscSeiNt2Res;
 
 typedef struct S390MsixInfo {
-    bool available;
     uint8_t table_bar;
     uint8_t pba_bar;
     uint16_t entries;  

OK, so that was a write-only variable? :)

Quoted 3 lines

Question for the IBMers: is there any change we might have a different implementation not relying on msi-x in the future?

@Conny
Currently, the plan would be to stick with a hard requirement for MSIX unless someone strongly supports the legacy alternatives. I'm certainly open to discuss that. Maybe it would make sense to fallback to MSI for devices that don't support MSIX?

@David
Thanks for the cleanup.

Reviewed-by Collin Walling <[email protected]>

I fail to see why this is useful as we require MSIX always and completely fail adding a device.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-pci-bus.c | 2 --
 hw/s390x/s390-pci-bus.h | 1 -
 2 files changed, 3 deletions(-)

hw/s390x/s390-pci-bus.c +0 -2

index e1b14b131b..1eaae3aca6 100644
@@ -745,7 +745,6 @@ static int s390_pci_msix_init(S390PCIBusDevice *pbdev)
 
     pos = pci_find_capability(pbdev->pdev, PCI_CAP_ID_MSIX);
     if (!pos) {
-        pbdev->msix.available = false;
         return -1;
     }
 

@@ -761,7 +760,6 @@ static int s390_pci_msix_init(S390PCIBusDevice *pbdev)
     pbdev->msix.pba_bar = pba & PCI_MSIX_FLAGS_BIRMASK;
     pbdev->msix.pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
     pbdev->msix.entries = (ctrl & PCI_MSIX_FLAGS_QSIZE) + 1;
-    pbdev->msix.available = true;
 
     name = g_strdup_printf("msix-s390-%04x", pbdev->uid);
     memory_region_init_io(&pbdev->msix_notify_mr, OBJECT(pbdev),

hw/s390x/s390-pci-bus.h +0 -0

index 1f7f9b5814..f47a0f2da5 100644

I fail to see why this is useful as we require MSIX always and completely fail adding a device.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-pci-bus.c | 2 --
 hw/s390x/s390-pci-bus.h | 1 -
 2 files changed, 3 deletions(-)

We directly have it in our hands.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-pci-bus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

hw/s390x/s390-pci-bus.c +1 -1

index 1eaae3aca6..68660eac74 100644
@@ -814,9 +814,9 @@ static bool s390_pci_alloc_idx(S390pciState *s, S390PCIBusDevice *pbdev)
 static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                               Error **errp)
 {
+    S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
     PCIDevice *pdev = NULL;
     S390PCIBusDevice *pbdev = NULL;
-    S390pciState *s = s390_get_phb();
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
         BusState *bus;

@@ -924,11 +924,11 @@ static void s390_pcihost_timer_cb(void *opaque)
 static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
                                 Error **errp)

Quoted 33 lines · On Mon, 5 Nov 2018 12:03:11 +0100 David Hildenbrand <[email protected]> wrote:

We directly have it in our hands.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-pci-bus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

hw/s390x/s390-pci-bus.c +2 -2

index 1eaae3aca6..68660eac74 100644
@@ -814,9 +814,9 @@ static bool s390_pci_alloc_idx(S390pciState *s, S390PCIBusDevice *pbdev)
 static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                               Error **errp)
 {
+    S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
     PCIDevice *pdev = NULL;
     S390PCIBusDevice *pbdev = NULL;
-    S390pciState *s = s390_get_phb();
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
         BusState *bus;

@@ -924,11 +924,11 @@ static void s390_pcihost_timer_cb(void *opaque)
 static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
                                 Error **errp)
 {
+    S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
     PCIDevice *pci_dev = NULL;
     PCIBus *bus;
     int32_t devfn;
     S390PCIBusDevice *pbdev = NULL;
-    S390pciState *s = s390_get_phb();
 

hw/s390x/s390-pci-bus.c static void s390_pcihost_timer_cb(void *opaque) · line 934

Quoted 2 lines

hw/s390x/s390-pci-bus.c +0 -0

     if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
         error_setg(errp, "PCI bridge hot unplug currently not supported");

Not sure whether that is an improvement (s390_get_phb() caches the value, and is called from multiple other places as well.)

Quoted 36 lines · Cornelia Huck

Quoted 33 lines · On Mon, 5 Nov 2018 12:03:11 +0100 David Hildenbrand <[email protected]> wrote:

We directly have it in our hands.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-pci-bus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

hw/s390x/s390-pci-bus.c +2 -2

index 1eaae3aca6..68660eac74 100644
@@ -814,9 +814,9 @@ static bool s390_pci_alloc_idx(S390pciState *s, S390PCIBusDevice *pbdev)
 static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                               Error **errp)
 {
+    S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
     PCIDevice *pdev = NULL;
     S390PCIBusDevice *pbdev = NULL;
-    S390pciState *s = s390_get_phb();
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
         BusState *bus;

@@ -924,11 +924,11 @@ static void s390_pcihost_timer_cb(void *opaque)
 static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
                                 Error **errp)
 {
+    S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
     PCIDevice *pci_dev = NULL;
     PCIBus *bus;
     int32_t devfn;
     S390PCIBusDevice *pbdev = NULL;
-    S390pciState *s = s390_get_phb();
 

Quoted 6 lines

hw/s390x/s390-pci-bus.c static void s390_pcihost_timer_cb(void *opaque) · line 934

Quoted 2 lines

hw/s390x/s390-pci-bus.c +0 -0

     if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
         error_setg(errp, "PCI bridge hot unplug currently not supported");

Not sure whether that is an improvement (s390_get_phb() caches the value, and is called from multiple other places as well.)

Looking up a variable that is directly passed as an argument doesn't look clean to me.

Quoted 44 lines · David Hildenbrand

Quoted 42 lines · Cornelia Huck

Quoted 25 lines · On Mon, 5 Nov 2018 12:03:11 +0100 David Hildenbrand <[email protected]> wrote:

We directly have it in our hands.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-pci-bus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

hw/s390x/s390-pci-bus.c +1 -1

index 1eaae3aca6..68660eac74 100644
@@ -814,9 +814,9 @@ static bool s390_pci_alloc_idx(S390pciState *s, S390PCIBusDevice *pbdev)
 static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                               Error **errp)
 {
+    S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
     PCIDevice *pdev = NULL;
     S390PCIBusDevice *pbdev = NULL;
-    S390pciState *s = s390_get_phb();
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
         BusState *bus;

@@ -924,11 +924,11 @@ static void s390_pcihost_timer_cb(void *opaque)
 static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
                                 Error **errp)

hw/s390x/s390-pci-bus.c static void s390_pcihost_timer_cb(void *opaque) · line 934

Quoted 10 lines

hw/s390x/s390-pci-bus.c +1 -1

 {
+    S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
     PCIDevice *pci_dev = NULL;
     PCIBus *bus;
     int32_t devfn;
     S390PCIBusDevice *pbdev = NULL;
-    S390pciState *s = s390_get_phb();
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
         error_setg(errp, "PCI bridge hot unplug currently not supported");

Not sure whether that is an improvement (s390_get_phb() caches the value, and is called from multiple other places as well.)

Quoted 2 lines

Looking up a variable that is directly passed as an argument doesn't look clean to me.

I think there was a reason for this caching, namely that qom resolution can be quite expensive. For the hotplug case this obviously does not matter but for all the other cases it might. So do we really want to have different places use different methods?

Quoted 47 lines · Christian Borntraeger

Quoted 44 lines · David Hildenbrand

Quoted 42 lines · Cornelia Huck

Quoted 25 lines · On Mon, 5 Nov 2018 12:03:11 +0100 David Hildenbrand <[email protected]> wrote:

We directly have it in our hands.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-pci-bus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

hw/s390x/s390-pci-bus.c +1 -1

index 1eaae3aca6..68660eac74 100644
@@ -814,9 +814,9 @@ static bool s390_pci_alloc_idx(S390pciState *s, S390PCIBusDevice *pbdev)
 static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                               Error **errp)
 {
+    S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
     PCIDevice *pdev = NULL;
     S390PCIBusDevice *pbdev = NULL;
-    S390pciState *s = s390_get_phb();
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
         BusState *bus;

@@ -924,11 +924,11 @@ static void s390_pcihost_timer_cb(void *opaque)
 static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
                                 Error **errp)

hw/s390x/s390-pci-bus.c static void s390_pcihost_timer_cb(void *opaque) · line 934

Quoted 10 lines

hw/s390x/s390-pci-bus.c +1 -1

 {
+    S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
     PCIDevice *pci_dev = NULL;
     PCIBus *bus;
     int32_t devfn;
     S390PCIBusDevice *pbdev = NULL;
-    S390pciState *s = s390_get_phb();
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
         error_setg(errp, "PCI bridge hot unplug currently not supported");

Not sure whether that is an improvement (s390_get_phb() caches the value, and is called from multiple other places as well.)

Quoted 8 lines

Quoted 2 lines

Looking up a variable that is directly passed as an argument doesn't look clean to me.

I think there was a reason for this caching, namely that qom resolution can be quite expensive. For the hotplug case this obviously does not matter but for all the other cases it might. So do we really want to have different places use different methods?

Caching resolution is fine (as that is expensive), caching a downcast is as far as I remember not necessary. Especially, as you said, for hotplug handlers.

Anyhow, if there are strong feelings to this change, I can drop this patch. There are certainly more important things to do in zPCI hotplug code.

Quoted 57 lines · David Hildenbrand

Quoted 55 lines · Christian Borntraeger

Quoted 44 lines · David Hildenbrand

Quoted 36 lines · Cornelia Huck

Quoted 33 lines · On Mon, 5 Nov 2018 12:03:11 +0100 David Hildenbrand <[email protected]> wrote:

We directly have it in our hands.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-pci-bus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

hw/s390x/s390-pci-bus.c +2 -2

index 1eaae3aca6..68660eac74 100644
@@ -814,9 +814,9 @@ static bool s390_pci_alloc_idx(S390pciState *s, S390PCIBusDevice *pbdev)
 static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                               Error **errp)
 {
+    S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
     PCIDevice *pdev = NULL;
     S390PCIBusDevice *pbdev = NULL;
-    S390pciState *s = s390_get_phb();
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
         BusState *bus;

@@ -924,11 +924,11 @@ static void s390_pcihost_timer_cb(void *opaque)
 static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
                                 Error **errp)
 {
+    S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
     PCIDevice *pci_dev = NULL;
     PCIBus *bus;
     int32_t devfn;
     S390PCIBusDevice *pbdev = NULL;
-    S390pciState *s = s390_get_phb();
 

Quoted 6 lines

hw/s390x/s390-pci-bus.c static void s390_pcihost_timer_cb(void *opaque) · line 934

Quoted 2 lines

hw/s390x/s390-pci-bus.c +0 -0

     if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
         error_setg(errp, "PCI bridge hot unplug currently not supported");  

Not sure whether that is an improvement (s390_get_phb() caches the value, and is called from multiple other places as well.)

Quoted 2 lines

Looking up a variable that is directly passed as an argument doesn't look clean to me.

I think there was a reason for this caching, namely that qom resolution can be quite expensive. For the hotplug case this obviously does not matter but for all the other cases it might. So do we really want to have different places use different methods?

Quoted 3 lines

Caching resolution is fine (as that is expensive), caching a downcast is as far as I remember not necessary. Especially, as you said, for hotplug handlers.

Quoted 5 lines

Anyhow, if there are strong feelings to this change, I can drop this patch. There are certainly more important things to do in zPCI hotplug code.

Truthfully, I'm not in favor of one over the other. As long as the device handlers are consistent, I think either is fine.

However, it would be nice if at some point during plug we cache the PHB somewhere. That would be some sort of best-of-both-worlds approach.

Quoted 58 lines · On Wed, 7 Nov 2018 15:28:31 -0500 Collin Walling <[email protected]> wrote:

Quoted 57 lines · David Hildenbrand

Quoted 55 lines · Christian Borntraeger

Quoted 44 lines · David Hildenbrand

Quoted 36 lines · Cornelia Huck

Quoted 33 lines · On Mon, 5 Nov 2018 12:03:11 +0100 David Hildenbrand <[email protected]> wrote:

We directly have it in our hands.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-pci-bus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

hw/s390x/s390-pci-bus.c +2 -2

index 1eaae3aca6..68660eac74 100644
@@ -814,9 +814,9 @@ static bool s390_pci_alloc_idx(S390pciState *s, S390PCIBusDevice *pbdev)
 static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                               Error **errp)
 {
+    S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
     PCIDevice *pdev = NULL;
     S390PCIBusDevice *pbdev = NULL;
-    S390pciState *s = s390_get_phb();
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
         BusState *bus;

@@ -924,11 +924,11 @@ static void s390_pcihost_timer_cb(void *opaque)
 static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
                                 Error **errp)
 {
+    S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
     PCIDevice *pci_dev = NULL;
     PCIBus *bus;
     int32_t devfn;
     S390PCIBusDevice *pbdev = NULL;
-    S390pciState *s = s390_get_phb();
 

Quoted 6 lines

hw/s390x/s390-pci-bus.c static void s390_pcihost_timer_cb(void *opaque) · line 934

Quoted 2 lines

hw/s390x/s390-pci-bus.c +0 -0

     if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
         error_setg(errp, "PCI bridge hot unplug currently not supported");  

Not sure whether that is an improvement (s390_get_phb() caches the value, and is called from multiple other places as well.)

Quoted 2 lines

Looking up a variable that is directly passed as an argument doesn't look clean to me.

I think there was a reason for this caching, namely that qom resolution can be quite expensive. For the hotplug case this obviously does not matter but for all the other cases it might. So do we really want to have different places use different methods?

Quoted 3 lines

Quoted 3 lines

Caching resolution is fine (as that is expensive), caching a downcast is as far as I remember not necessary. Especially, as you said, for hotplug handlers.

Yes, the complete QOM cast was the expensive thing AFAIR.

Quoted 8 lines

Quoted 5 lines

Anyhow, if there are strong feelings to this change, I can drop this patch. There are certainly more important things to do in zPCI hotplug code.

Truthfully, I'm not in favor of one over the other. As long as the device handlers are consistent, I think either is fine.

I don't feel *that* strong about this change here, either :) Your call.

Quoted 3 lines

However, it would be nice if at some point during plug we cache the PHB somewhere. That would be some sort of best-of-both-worlds approach.

Not sure if caching-from-a-downcast would be conceptionally clean. I'd vote for either taking this patch or dropping it completely.

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 <[email protected]>

---
 hw/s390x/s390-pci-bus.c | 43 +++++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 17 deletions(-)

hw/s390x/s390-pci-bus.c +22 -12

index 68660eac74..1849f9d334 100644
@@ -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         */

hw/s390x/s390-pci-bus.c +3 -4

             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;

Quoted 77 lines · David Hildenbrand

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 <[email protected]>

---
 hw/s390x/s390-pci-bus.c | 43 +++++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 17 deletions(-)

hw/s390x/s390-pci-bus.c +22 -12

index 68660eac74..1849f9d334 100644
@@ -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         */

hw/s390x/s390-pci-bus.c static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev, · line 896

Quoted 5 lines

hw/s390x/s390-pci-bus.c +1 -1

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

Quoted 18 lines

hw/s390x/s390-pci-bus.c +2 -4

             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)
 

hw/s390x/s390-pci-bus.c static void s390_pcihost_class_init(ObjectClass *klass, void *data) · line 1044

Quoted 7 lines

hw/s390x/s390-pci-bus.c +1 -0

     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 ;)

Quoted 2 lines

The above concerns do not relate to any functionality of the code, so with them addressed, then:

Reviewed-by Collin Walling <[email protected]>

Quoted 4 lines

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 [email protected], so maybe easier to find for you).

Right now, errors during realize()/pre_plug/plug of the zPCI device would result in QEMU crashing instead of failing nicely when creating a zPCI device for a PCI device.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-pci-bus.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

hw/s390x/s390-pci-bus.c +18 -4

index 1849f9d334..4939490c7c 100644
@@ -778,17 +778,31 @@ static void s390_pci_msix_free(S390PCIBusDevice *pbdev)
 }
 
 static S390PCIBusDevice *s390_pci_device_new(S390pciState *s,
-                                             const char *target)
+                                             const char *target, Error **errp)
 {
-    DeviceState *dev = NULL;
+    Error *local_err = NULL;
+    DeviceState *dev;
 
     dev = qdev_try_create(BUS(s->bus), TYPE_S390_PCI_DEVICE);
     if (!dev) {
+        error_setg(errp, "zPCI device could not be created");
         return NULL;
     }
 
-    qdev_prop_set_string(dev, "target", target);
-    qdev_init_nofail(dev);
+    object_property_set_str(OBJECT(dev), "target", target, &local_err);
+    if (local_err) {
+        object_unparent(OBJECT(dev));
+        error_propagate_prepend(errp, local_err,
+                                "zPCI device could not be created: ");
+        return NULL;
+    }
+    object_property_set_bool(OBJECT(dev), true, "realized", &local_err);
+    if (local_err) {
+        object_unparent(OBJECT(dev));
+        error_propagate_prepend(errp, local_err,
+                                "zPCI device could not be created: ");
+        return NULL;
+    }
 
     return S390_PCI_DEVICE(dev);
 }

Quoted 3 lines · David Hildenbrand

Right now, errors during realize()/pre_plug/plug of the zPCI device would result in QEMU crashing instead of failing nicely when creating a zPCI device for a PCI device.

Quoted 48 lines

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-pci-bus.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

hw/s390x/s390-pci-bus.c +18 -4

index 1849f9d334..4939490c7c 100644
@@ -778,17 +778,31 @@ static void s390_pci_msix_free(S390PCIBusDevice *pbdev)
 }
 
 static S390PCIBusDevice *s390_pci_device_new(S390pciState *s,
-                                             const char *target)
+                                             const char *target, Error **errp)
 {
-    DeviceState *dev = NULL;
+    Error *local_err = NULL;
+    DeviceState *dev;
 
     dev = qdev_try_create(BUS(s->bus), TYPE_S390_PCI_DEVICE);
     if (!dev) {
+        error_setg(errp, "zPCI device could not be created");
         return NULL;
     }
 
-    qdev_prop_set_string(dev, "target", target);
-    qdev_init_nofail(dev);
+    object_property_set_str(OBJECT(dev), "target", target, &local_err);
+    if (local_err) {
+        object_unparent(OBJECT(dev));
+        error_propagate_prepend(errp, local_err,
+                                "zPCI device could not be created: ");
+        return NULL;
+    }
+    object_property_set_bool(OBJECT(dev), true, "realized", &local_err);
+    if (local_err) {
+        object_unparent(OBJECT(dev));
+        error_propagate_prepend(errp, local_err,
+                                "zPCI device could not be created: ");
+        return NULL;
+    }
 
     return S390_PCI_DEVICE(dev);
 }

@@ -873,9 +887,8 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
 

hw/s390x/s390-pci-bus.c static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev, · line 894

Quoted 10 lines

hw/s390x/s390-pci-bus.c +1 -2

         pbdev = s390_pci_find_dev_by_target(s, dev->id);
         if (!pbdev) {
-            pbdev = s390_pci_device_new(s, dev->id);
+            pbdev = s390_pci_device_new(s, dev->id, errp);
             if (!pbdev) {
-                error_setg(errp, "create zpci device failed");
                 return;
             }
         }

Looks right to me, I think this is even suitable for v3.1.

Reviewed-by Thomas Huth <[email protected]>

Quoted 4 lines · On Mon, 5 Nov 2018 13:04:04 +0100 Thomas Huth <[email protected]> wrote:

Quoted 3 lines · David Hildenbrand

Right now, errors during realize()/pre_plug/plug of the zPCI device would result in QEMU crashing instead of failing nicely when creating a zPCI device for a PCI device.

Yeah, failing instead of crashing is better :)

Is there any way we can trigger this problem for testing?

Quoted 49 lines

Quoted 49 lines

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-pci-bus.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

hw/s390x/s390-pci-bus.c +18 -4

index 1849f9d334..4939490c7c 100644
@@ -778,17 +778,31 @@ static void s390_pci_msix_free(S390PCIBusDevice *pbdev)
 }
 
 static S390PCIBusDevice *s390_pci_device_new(S390pciState *s,
-                                             const char *target)
+                                             const char *target, Error **errp)
 {
-    DeviceState *dev = NULL;
+    Error *local_err = NULL;
+    DeviceState *dev;
 
     dev = qdev_try_create(BUS(s->bus), TYPE_S390_PCI_DEVICE);
     if (!dev) {
+        error_setg(errp, "zPCI device could not be created");
         return NULL;
     }
 
-    qdev_prop_set_string(dev, "target", target);
-    qdev_init_nofail(dev);
+    object_property_set_str(OBJECT(dev), "target", target, &local_err);
+    if (local_err) {
+        object_unparent(OBJECT(dev));
+        error_propagate_prepend(errp, local_err,
+                                "zPCI device could not be created: ");
+        return NULL;
+    }
+    object_property_set_bool(OBJECT(dev), true, "realized", &local_err);
+    if (local_err) {
+        object_unparent(OBJECT(dev));
+        error_propagate_prepend(errp, local_err,
+                                "zPCI device could not be created: ");
+        return NULL;
+    }
 
     return S390_PCI_DEVICE(dev);
 }

@@ -873,9 +887,8 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
 

Quoted 12 lines

hw/s390x/s390-pci-bus.c static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev, · line 894

Quoted 10 lines

hw/s390x/s390-pci-bus.c +1 -2

         pbdev = s390_pci_find_dev_by_target(s, dev->id);
         if (!pbdev) {
-            pbdev = s390_pci_device_new(s, dev->id);
+            pbdev = s390_pci_device_new(s, dev->id, errp);
             if (!pbdev) {
-                error_setg(errp, "create zpci device failed");
                 return;
             }
         }

Looks right to me, I think this is even suitable for v3.1.

I can consider this for 3.1. Is this patch standalone?

Quoted 5 lines

Reviewed-by Thomas Huth <[email protected]>

Quoted 10 lines · Cornelia Huck

Quoted 4 lines · On Mon, 5 Nov 2018 13:04:04 +0100 Thomas Huth <[email protected]> wrote:

Quoted 3 lines · David Hildenbrand

Right now, errors during realize()/pre_plug/plug of the zPCI device would result in QEMU crashing instead of failing nicely when creating a zPCI device for a PCI device.

Yeah, failing instead of crashing is better :)

Quoted 1 line

Is there any way we can trigger this problem for testing?

I guess trying to add more PCI devices (with implicit zPCI devices getting created) than we have zPCI slots should be enough. So making e.g. s390_pci_alloc_idx() fail.

(FH_MASK_INDEX = 0x0000ffff implies 65536 devices, which is not really easy to test ;) )

Right now, errors during realize()/pre_plug/plug of the zPCI device would result in QEMU crashing instead of failing nicely when creating a zPCI device for a PCI device.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-pci-bus.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

hw/s390x/s390-pci-bus.c +18 -4

index 1849f9d334..4939490c7c 100644
@@ -778,17 +778,31 @@ static void s390_pci_msix_free(S390PCIBusDevice *pbdev)
 }
 
 static S390PCIBusDevice *s390_pci_device_new(S390pciState *s,
-                                             const char *target)
+                                             const char *target, Error **errp)
 {
-    DeviceState *dev = NULL;
+    Error *local_err = NULL;
+    DeviceState *dev;
 
     dev = qdev_try_create(BUS(s->bus), TYPE_S390_PCI_DEVICE);
     if (!dev) {
+        error_setg(errp, "zPCI device could not be created");
         return NULL;
     }
 
-    qdev_prop_set_string(dev, "target", target);
-    qdev_init_nofail(dev);
+    object_property_set_str(OBJECT(dev), "target", target, &local_err);
+    if (local_err) {
+        object_unparent(OBJECT(dev));
+        error_propagate_prepend(errp, local_err,
+                                "zPCI device could not be created: ");
+        return NULL;
+    }
+    object_property_set_bool(OBJECT(dev), true, "realized", &local_err);
+    if (local_err) {
+        object_unparent(OBJECT(dev));
+        error_propagate_prepend(errp, local_err,
+                                "zPCI device could not be created: ");
+        return NULL;
+    }
 
     return S390_PCI_DEVICE(dev);
 }

Right now, errors during realize()/pre_plug/plug of the zPCI device would result in QEMU crashing instead of failing nicely when creating a zPCI device for a PCI device.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-pci-bus.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

hw/s390x/s390-pci-bus.c +19 -6

index 1849f9d334..4939490c7c 100644
@@ -778,17 +778,31 @@ static void s390_pci_msix_free(S390PCIBusDevice *pbdev)
 }
 
 static S390PCIBusDevice *s390_pci_device_new(S390pciState *s,
-                                             const char *target)
+                                             const char *target, Error **errp)
 {
-    DeviceState *dev = NULL;
+    Error *local_err = NULL;
+    DeviceState *dev;
 
     dev = qdev_try_create(BUS(s->bus), TYPE_S390_PCI_DEVICE);
     if (!dev) {
+        error_setg(errp, "zPCI device could not be created");
         return NULL;
     }
 
-    qdev_prop_set_string(dev, "target", target);
-    qdev_init_nofail(dev);
+    object_property_set_str(OBJECT(dev), "target", target, &local_err);
+    if (local_err) {
+        object_unparent(OBJECT(dev));
+        error_propagate_prepend(errp, local_err,
+                                "zPCI device could not be created: ");
+        return NULL;
+    }
+    object_property_set_bool(OBJECT(dev), true, "realized", &local_err);
+    if (local_err) {
+        object_unparent(OBJECT(dev));
+        error_propagate_prepend(errp, local_err,
+                                "zPCI device could not be created: ");
+        return NULL;
+    }
 
     return S390_PCI_DEVICE(dev);
 }

@@ -873,9 +887,8 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
 
         pbdev = s390_pci_find_dev_by_target(s, dev->id);
         if (!pbdev) {
-            pbdev = s390_pci_device_new(s, dev->id);
+            pbdev = s390_pci_device_new(s, dev->id, errp);
             if (!pbdev) {
-                error_setg(errp, "create zpci device failed");
                 return;
             }
         }

Quoted 8 lines · On Mon, 5 Nov 2018 12:03:13 +0100 David Hildenbrand <[email protected]> wrote:

Right now, errors during realize()/pre_plug/plug of the zPCI device would result in QEMU crashing instead of failing nicely when creating a zPCI device for a PCI device.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-pci-bus.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

Unfortunately, this patch applied as-is on top of master breaks zpci device autogeneration:

(quoted diff) +0 -2

-device zpci,target=pcidev -device virtio-net-pci,id=pcidev works
 
-device virtio-net-pci fails with

qemu-system-s390x: -device virtio-net-pci: zPCI device could not be created: Property '.auto_00:00.0' not found

Any idea?

[insert my usual rants about the zpci architecture here]

Quoted 52 lines

hw/s390x/s390-pci-bus.c +19 -6

index 1849f9d334..4939490c7c 100644
@@ -778,17 +778,31 @@ static void s390_pci_msix_free(S390PCIBusDevice *pbdev)
 }
 
 static S390PCIBusDevice *s390_pci_device_new(S390pciState *s,
-                                             const char *target)
+                                             const char *target, Error **errp)
 {
-    DeviceState *dev = NULL;
+    Error *local_err = NULL;
+    DeviceState *dev;
 
     dev = qdev_try_create(BUS(s->bus), TYPE_S390_PCI_DEVICE);
     if (!dev) {
+        error_setg(errp, "zPCI device could not be created");
         return NULL;
     }
 
-    qdev_prop_set_string(dev, "target", target);
-    qdev_init_nofail(dev);
+    object_property_set_str(OBJECT(dev), "target", target, &local_err);
+    if (local_err) {
+        object_unparent(OBJECT(dev));
+        error_propagate_prepend(errp, local_err,
+                                "zPCI device could not be created: ");
+        return NULL;
+    }
+    object_property_set_bool(OBJECT(dev), true, "realized", &local_err);
+    if (local_err) {
+        object_unparent(OBJECT(dev));
+        error_propagate_prepend(errp, local_err,
+                                "zPCI device could not be created: ");
+        return NULL;
+    }
 
     return S390_PCI_DEVICE(dev);
 }

@@ -873,9 +887,8 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
 
         pbdev = s390_pci_find_dev_by_target(s, dev->id);
         if (!pbdev) {
-            pbdev = s390_pci_device_new(s, dev->id);
+            pbdev = s390_pci_device_new(s, dev->id, errp);
             if (!pbdev) {
-                error_setg(errp, "create zpci device failed");
                 return;
             }
         }

The hotplug code needs more love, but let's do some obvious cleanups first. In the future, we want to propery make use of unplug_request() + unplug(), instead of routing everything (especially two separate but linked) devices via a single unplug call. Also, we want to move all errors in plug() into the pre_plug() handler, but this will require general PCI refactorings (moving stuff from realize() to the pre_plug/plug handler).

This series is based on "[PATCH v2 00/10] pci: hotplug handler reworks", which contains one cleanup for s390x.

David Hildenbrand (4):
s390x/zpci: drop msix.available

s390x/zpci: use hotplug_dev instead of looking up the host bridge

s390x/zpci: move some hotplug checks to the pre_plug handler

s390x/zpci: properly fail if the zPCI device cannot be created

 hw/s390x/s390-pci-bus.c | 74 ++++++++++++++++++++++++++---------------
 hw/s390x/s390-pci-bus.h |  1 -
 2 files changed, 47 insertions(+), 28 deletions(-)

Quoted 8 lines · On Mon, 5 Nov 2018 12:03:09 +0100 David Hildenbrand <[email protected]> wrote:

The hotplug code needs more love, but let's do some obvious cleanups first. In the future, we want to propery make use of unplug_request() + unplug(), instead of routing everything (especially two separate but linked) devices via a single unplug call. Also, we want to move all errors in plug() into the pre_plug() handler, but this will require general PCI refactorings (moving stuff from realize() to the pre_plug/plug handler).

Quoted 5 lines

This series is based on "[PATCH v2 00/10] pci: hotplug handler reworks", which contains one cleanup for s390x.

David Hildenbrand (4):
s390x/zpci: drop msix.available

queued to s390-next

Quoted 1 line

s390x/zpci: use hotplug_dev instead of looking up the host bridge

Do we have consensus on that one yet? I can take it or leave it :)

Quoted 1 line

s390x/zpci: move some hotplug checks to the pre_plug handler

depends on the handler rework

Quoted 2 lines

s390x/zpci: properly fail if the zPCI device cannot be created

Waiting for a fixed patch... can queue to s390-fixes if it arrives soon(tm).

Quoted 21 lines · Cornelia Huck

Quoted 8 lines · On Mon, 5 Nov 2018 12:03:09 +0100 David Hildenbrand <[email protected]> wrote:

The hotplug code needs more love, but let's do some obvious cleanups first. In the future, we want to propery make use of unplug_request() + unplug(), instead of routing everything (especially two separate but linked) devices via a single unplug call. Also, we want to move all errors in plug() into the pre_plug() handler, but this will require general PCI refactorings (moving stuff from realize() to the pre_plug/plug handler).

Quoted 5 lines

This series is based on "[PATCH v2 00/10] pci: hotplug handler reworks", which contains one cleanup for s390x.

David Hildenbrand (4):
s390x/zpci: drop msix.available

queued to s390-next

Quoted 1 line

s390x/zpci: use hotplug_dev instead of looking up the host bridge

Quoted 5 lines

Do we have consensus on that one yet? I can take it or leave it :)

Quoted 1 line

s390x/zpci: move some hotplug checks to the pre_plug handler

depends on the handler rework

I can pull that one out from the general handler rework (still need review either way so it could take a while).

Quoted 6 lines

Quoted 3 lines

s390x/zpci: properly fail if the zPCI device cannot be created

Waiting for a fixed patch... can queue to s390-fixes if it arrives soon(tm).

Thanks!

Shall I resend all or only this one?

Quoted 22 lines · On Mon, 12 Nov 2018 18:34:34 +0100 David Hildenbrand <[email protected]> wrote:

Quoted 21 lines · Cornelia Huck

Quoted 8 lines · On Mon, 5 Nov 2018 12:03:09 +0100 David Hildenbrand <[email protected]> wrote:

The hotplug code needs more love, but let's do some obvious cleanups first. In the future, we want to propery make use of unplug_request() + unplug(), instead of routing everything (especially two separate but linked) devices via a single unplug call. Also, we want to move all errors in plug() into the pre_plug() handler, but this will require general PCI refactorings (moving stuff from realize() to the pre_plug/plug handler).

Quoted 5 lines

This series is based on "[PATCH v2 00/10] pci: hotplug handler reworks", which contains one cleanup for s390x.

David Hildenbrand (4):
s390x/zpci: drop msix.available

queued to s390-next

Quoted 1 line

s390x/zpci: use hotplug_dev instead of looking up the host bridge

Quoted 8 lines

Quoted 5 lines

Do we have consensus on that one yet? I can take it or leave it :)

Quoted 1 line

s390x/zpci: move some hotplug checks to the pre_plug handler

depends on the handler rework

I can pull that one out from the general handler rework (still need review either way so it could take a while).

It's 4.0 material anyway, so no need to hurry.

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

s390x/zpci: properly fail if the zPCI device cannot be created

Quoted 6 lines

Quoted 2 lines

Waiting for a fixed patch... can queue to s390-fixes if it arrives soon(tm).

Thanks!

Shall I resend all or only this one?

The last one would be great, as I think it's still 3.1 material.