[Qemu-devel] [PATCH v1 0/4] s390x/zpci: some hotplug handler cleanups

[Qemu-devel] [PATCH v1 0/4] s390x/zpci: some hotplug handler cleanups

[David Hildenbrand]

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

-- 
2.17.2

[Qemu-devel] [PATCH v1 1/4] s390x/zpci: drop msix.available

[David Hildenbrand]

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

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-pci-bus.c | 2 --
 hw/s390x/s390-pci-bus.h | 1 -
 2 files changed, 3 deletions(-)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index e1b14b131b..1eaae3aca6 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -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),
diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
index 1f7f9b5814..f47a0f2da5 100644
--- a/hw/s390x/s390-pci-bus.h
+++ b/hw/s390x/s390-pci-bus.h
@@ -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;
-- 
2.17.2

[Qemu-devel] [PATCH v1 2/4] s390x/zpci: use hotplug_dev instead of looking up the host bridge

[David Hildenbrand]

We directly have it in our hands.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-pci-bus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 1eaae3aca6..68660eac74 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -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();
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
         error_setg(errp, "PCI bridge hot unplug currently not supported");
-- 
2.17.2

[Qemu-devel] [PATCH v1 3/4] s390x/zpci: move some hotplug checks to the pre_plug handler

[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 <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");
             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;
-- 
2.17.2

[Qemu-devel] [PATCH v1 4/4] s390x/zpci: properly fail if the zPCI device cannot be created

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

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-pci-bus.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 1849f9d334..4939490c7c 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -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;
             }
         }
-- 
2.17.2

Re: [Qemu-devel] [PATCH v1 1/4] s390x/zpci: drop msix.available

[Cornelia Huck]

On Mon,  5 Nov 2018 12:03:10 +0100
David Hildenbrand <david@redhat.com> 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 <david@redhat.com>
> ---
>  hw/s390x/s390-pci-bus.c | 2 --
>  hw/s390x/s390-pci-bus.h | 1 -
>  2 files changed, 3 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index e1b14b131b..1eaae3aca6 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -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),
> diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
> index 1f7f9b5814..f47a0f2da5 100644
> --- a/hw/s390x/s390-pci-bus.h
> +++ b/hw/s390x/s390-pci-bus.h
> @@ -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?

Re: [Qemu-devel] [PATCH v1 2/4] s390x/zpci: use hotplug_dev instead of looking up the host bridge

[Cornelia Huck]

On Mon,  5 Nov 2018 12:03:11 +0100
David Hildenbrand <david@redhat.com> wrote:

> We directly have it in our hands.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/s390-pci-bus.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 1eaae3aca6..68660eac74 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -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();
>  
>      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.)

Re: [Qemu-devel] [PATCH v1 1/4] s390x/zpci: drop msix.available

[Thomas Huth]

On 2018-11-05 12:03, David Hildenbrand 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 <david@redhat.com>
> ---
>  hw/s390x/s390-pci-bus.c | 2 --
>  hw/s390x/s390-pci-bus.h | 1 -
>  2 files changed, 3 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index e1b14b131b..1eaae3aca6 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -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),
> diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
> index 1f7f9b5814..f47a0f2da5 100644
> --- a/hw/s390x/s390-pci-bus.h
> +++ b/hw/s390x/s390-pci-bus.h
> @@ -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;
> 

Seems like the code that used this variable has been removed with commit
ID 4f6482bfe3da1e6b51ad4722a0c22f22f0d54a3b.

Fixes: 4f6482bfe3da1e6b51ad4722a0c22f22f0d54a3b
Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v1 2/4] s390x/zpci: use hotplug_dev instead of looking up the host bridge

[David Hildenbrand]

On 05.11.18 12:21, Cornelia Huck wrote:
> On Mon,  5 Nov 2018 12:03:11 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> We directly have it in our hands.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/s390x/s390-pci-bus.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>> index 1eaae3aca6..68660eac74 100644
>> --- a/hw/s390x/s390-pci-bus.c
>> +++ b/hw/s390x/s390-pci-bus.c
>> @@ -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();
>>  
>>      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.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 2/4] s390x/zpci: use hotplug_dev instead of looking up the host bridge

[Christian Borntraeger]

On 11/05/2018 12:37 PM, David Hildenbrand wrote:
> On 05.11.18 12:21, Cornelia Huck wrote:
>> On Mon,  5 Nov 2018 12:03:11 +0100
>> David Hildenbrand <david@redhat.com> wrote:
>>
>>> We directly have it in our hands.
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>  hw/s390x/s390-pci-bus.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>>> index 1eaae3aca6..68660eac74 100644
>>> --- a/hw/s390x/s390-pci-bus.c
>>> +++ b/hw/s390x/s390-pci-bus.c
>>> @@ -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();
>>>  
>>>      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.

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?

Re: [Qemu-devel] [PATCH v1 2/4] s390x/zpci: use hotplug_dev instead of looking up the host bridge

[David Hildenbrand]

On 05.11.18 12:40, Christian Borntraeger wrote:
> 
> 
> On 11/05/2018 12:37 PM, David Hildenbrand wrote:
>> On 05.11.18 12:21, Cornelia Huck wrote:
>>> On Mon,  5 Nov 2018 12:03:11 +0100
>>> David Hildenbrand <david@redhat.com> wrote:
>>>
>>>> We directly have it in our hands.
>>>>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>>  hw/s390x/s390-pci-bus.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>>>> index 1eaae3aca6..68660eac74 100644
>>>> --- a/hw/s390x/s390-pci-bus.c
>>>> +++ b/hw/s390x/s390-pci-bus.c
>>>> @@ -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();
>>>>  
>>>>      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.
> 
> 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.


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 3/4] s390x/zpci: move some hotplug checks to the pre_plug handler

[David Hildenbrand]

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


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 4/4] s390x/zpci: properly fail if the zPCI device cannot be created

[Thomas Huth]

On 2018-11-05 12:03, David Hildenbrand 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 <david@redhat.com>
> ---
>  hw/s390x/s390-pci-bus.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 1849f9d334..4939490c7c 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -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;
>              }
>          }
> 

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

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v1 4/4] s390x/zpci: properly fail if the zPCI device cannot be created

[Cornelia Huck]

On Mon, 5 Nov 2018 13:04:04 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 2018-11-05 12:03, David Hildenbrand 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.

Yeah, failing instead of crashing is better :)

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

> > 
> > Signed-off-by: David Hildenbrand <david@redhat.com>
> > ---
> >  hw/s390x/s390-pci-bus.c | 25 +++++++++++++++++++------
> >  1 file changed, 19 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> > index 1849f9d334..4939490c7c 100644
> > --- a/hw/s390x/s390-pci-bus.c
> > +++ b/hw/s390x/s390-pci-bus.c
> > @@ -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;
> >              }
> >          }
> >   
> 
> Looks right to me, I think this is even suitable for v3.1.

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

> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> 

Re: [Qemu-devel] [PATCH v1 4/4] s390x/zpci: properly fail if the zPCI device cannot be created

[David Hildenbrand]

On 05.11.18 13:41, Cornelia Huck wrote:
> On Mon, 5 Nov 2018 13:04:04 +0100
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 2018-11-05 12:03, David Hildenbrand 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.
> 
> Yeah, failing instead of crashing is better :)
> 
> 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 ;) )

Same would happen when running out of uids ... more unlikely to happen ;)

> 
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>  hw/s390x/s390-pci-bus.c | 25 +++++++++++++++++++------
>>>  1 file changed, 19 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>>> index 1849f9d334..4939490c7c 100644
>>> --- a/hw/s390x/s390-pci-bus.c
>>> +++ b/hw/s390x/s390-pci-bus.c
>>> @@ -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;
>>>              }
>>>          }
>>>   
>>
>> Looks right to me, I think this is even suitable for v3.1.
> 
> I can consider this for 3.1. Is this patch standalone?

Yes I think so.

> 
>>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>
>>
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 1/4] s390x/zpci: drop msix.available

[Collin Walling]

On 11/5/18 6:19 AM, Cornelia Huck wrote:
> On Mon,  5 Nov 2018 12:03:10 +0100
> David Hildenbrand <david@redhat.com> 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 <david@redhat.com>
>> ---
>>  hw/s390x/s390-pci-bus.c | 2 --
>>  hw/s390x/s390-pci-bus.h | 1 -
>>  2 files changed, 3 deletions(-)
>>
>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>> index e1b14b131b..1eaae3aca6 100644
>> --- a/hw/s390x/s390-pci-bus.c
>> +++ b/hw/s390x/s390-pci-bus.c
>> @@ -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),
>> diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
>> index 1f7f9b5814..f47a0f2da5 100644
>> --- a/hw/s390x/s390-pci-bus.h
>> +++ b/hw/s390x/s390-pci-bus.h
>> @@ -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?
> 

@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 <walling@linux.ibm.com>

-- 
Respectfully,
- Collin Walling

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 3/4] s390x/zpci: move some hotplug checks to the pre_plug handler

[Collin Walling]

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>

-- 
Respectfully,
- Collin Walling

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 3/4] s390x/zpci: move some hotplug checks to the pre_plug handler

[David Hildenbrand]

> 
> 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!

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 3/4] s390x/zpci: move some hotplug checks to the pre_plug handler

[Collin Walling]

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

-- 
Respectfully,
- Collin Walling

Re: [Qemu-devel] [PATCH v1 4/4] s390x/zpci: properly fail if the zPCI device cannot be created

[Collin Walling]

On 11/5/18 6:03 AM, David Hildenbrand 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 <david@redhat.com>
> ---
>  hw/s390x/s390-pci-bus.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 1849f9d334..4939490c7c 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -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;
>              }
>          }
> 

LGTM

Reviewed-by: Collin Walling <walling@linux.ibm.com>

-- 
Respectfully,
- Collin Walling

Re: [Qemu-devel] [PATCH v1 2/4] s390x/zpci: use hotplug_dev instead of looking up the host bridge

[Collin Walling]

On 11/5/18 6:50 AM, David Hildenbrand wrote:
> On 05.11.18 12:40, Christian Borntraeger wrote:
>>
>>
>> On 11/05/2018 12:37 PM, David Hildenbrand wrote:
>>> On 05.11.18 12:21, Cornelia Huck wrote:
>>>> On Mon,  5 Nov 2018 12:03:11 +0100
>>>> David Hildenbrand <david@redhat.com> wrote:
>>>>
>>>>> We directly have it in our hands.
>>>>>
>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>> ---
>>>>>  hw/s390x/s390-pci-bus.c | 4 ++--
>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>>>>> index 1eaae3aca6..68660eac74 100644
>>>>> --- a/hw/s390x/s390-pci-bus.c
>>>>> +++ b/hw/s390x/s390-pci-bus.c
>>>>> @@ -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();
>>>>>  
>>>>>      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.
>>
>> 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.
> 
> 

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.

-- 
Respectfully,
- Collin Walling

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 1/4] s390x/zpci: drop msix.available

[Cornelia Huck]

On Wed, 7 Nov 2018 11:26:50 -0500
Collin Walling <walling@linux.ibm.com> wrote:

> On 11/5/18 6:19 AM, Cornelia Huck wrote:
> > On Mon,  5 Nov 2018 12:03:10 +0100
> > David Hildenbrand <david@redhat.com> 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 <david@redhat.com>
> >> ---
> >>  hw/s390x/s390-pci-bus.c | 2 --
> >>  hw/s390x/s390-pci-bus.h | 1 -
> >>  2 files changed, 3 deletions(-)
> >>
> >> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> >> index e1b14b131b..1eaae3aca6 100644
> >> --- a/hw/s390x/s390-pci-bus.c
> >> +++ b/hw/s390x/s390-pci-bus.c
> >> @@ -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),
> >> diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
> >> index 1f7f9b5814..f47a0f2da5 100644
> >> --- a/hw/s390x/s390-pci-bus.h
> >> +++ b/hw/s390x/s390-pci-bus.h
> >> @@ -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?
> >   
> 
> @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?

I'm not sure whether that would be very useful. My question was more
along the lines of "ok, so now that available flag is gone for good?" :)

> 
> @David
> Thanks for the cleanup.
> 
> Reviewed-by: Collin Walling <walling@linux.ibm.com>
> 

Re: [Qemu-devel] [PATCH v1 2/4] s390x/zpci: use hotplug_dev instead of looking up the host bridge

[Cornelia Huck]

On Wed, 7 Nov 2018 15:28:31 -0500
Collin Walling <walling@linux.ibm.com> wrote:

> On 11/5/18 6:50 AM, David Hildenbrand wrote:
> > On 05.11.18 12:40, Christian Borntraeger wrote:  
> >>
> >>
> >> On 11/05/2018 12:37 PM, David Hildenbrand wrote:  
> >>> On 05.11.18 12:21, Cornelia Huck wrote:  
> >>>> On Mon,  5 Nov 2018 12:03:11 +0100
> >>>> David Hildenbrand <david@redhat.com> wrote:
> >>>>  
> >>>>> We directly have it in our hands.
> >>>>>
> >>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
> >>>>> ---
> >>>>>  hw/s390x/s390-pci-bus.c | 4 ++--
> >>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> >>>>> index 1eaae3aca6..68660eac74 100644
> >>>>> --- a/hw/s390x/s390-pci-bus.c
> >>>>> +++ b/hw/s390x/s390-pci-bus.c
> >>>>> @@ -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();
> >>>>>  
> >>>>>      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.  
> >>
> >> 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.

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

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

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

Re: [Qemu-devel] [PATCH v1 4/4] s390x/zpci: properly fail if the zPCI device cannot be created

[Cornelia Huck]

On Mon, 5 Nov 2018 13:46:10 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 05.11.18 13:41, Cornelia Huck wrote:
> > On Mon, 5 Nov 2018 13:04:04 +0100
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> >> On 2018-11-05 12:03, David Hildenbrand 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.  
> > 
> > Yeah, failing instead of crashing is better :)
> > 
> > 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 ;) )
> 

I was hoping for an easy test. Oh well :)

Re: [Qemu-devel] [PATCH v1 2/4] s390x/zpci: use hotplug_dev instead of looking up the host bridge

[David Hildenbrand]

On 08.11.18 12:07, Cornelia Huck wrote:
> On Wed, 7 Nov 2018 15:28:31 -0500
> Collin Walling <walling@linux.ibm.com> wrote:
> 
>> On 11/5/18 6:50 AM, David Hildenbrand wrote:
>>> On 05.11.18 12:40, Christian Borntraeger wrote:  
>>>>
>>>>
>>>> On 11/05/2018 12:37 PM, David Hildenbrand wrote:  
>>>>> On 05.11.18 12:21, Cornelia Huck wrote:  
>>>>>> On Mon,  5 Nov 2018 12:03:11 +0100
>>>>>> David Hildenbrand <david@redhat.com> wrote:
>>>>>>  
>>>>>>> We directly have it in our hands.
>>>>>>>
>>>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>>>> ---
>>>>>>>  hw/s390x/s390-pci-bus.c | 4 ++--
>>>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>>>>>>> index 1eaae3aca6..68660eac74 100644
>>>>>>> --- a/hw/s390x/s390-pci-bus.c
>>>>>>> +++ b/hw/s390x/s390-pci-bus.c
>>>>>>> @@ -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();
>>>>>>>  
>>>>>>>      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.  
>>>>
>>>> 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.
> 
> Yes, the complete QOM cast was the expensive thing AFAIR.
> 
>>>
>>> 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.
> 
>>
>> 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.
> 

Yes, caching at that point, I would consider premature optimization.

I'll drag this patch along and let the maintainers decide what to do :)

(later we should forward the actual value to e.g. event injection code,
so we don't have to look it up multiple times during one call)

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 4/4] s390x/zpci: properly fail if the zPCI device cannot be created

[Cornelia Huck]

On Mon,  5 Nov 2018 12:03:13 +0100
David Hildenbrand <david@redhat.com> 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 <david@redhat.com>
> ---
>  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:

-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]

> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 1849f9d334..4939490c7c 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -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;
>              }
>          }

Re: [Qemu-devel] [PATCH v1 4/4] s390x/zpci: properly fail if the zPCI device cannot be created

[David Hildenbrand]

On 08.11.18 14:35, Cornelia Huck wrote:
> On Mon,  5 Nov 2018 12:03:13 +0100
> David Hildenbrand <david@redhat.com> 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 <david@redhat.com>
>> ---
>>  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:
> 
> -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?

Yes, the "target" and target have to be inverted.

Thanks for testing. Will resend.

> 
> [insert my usual rants about the zpci architecture here]
> 
>>
>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>> index 1849f9d334..4939490c7c 100644
>> --- a/hw/s390x/s390-pci-bus.c
>> +++ b/hw/s390x/s390-pci-bus.c
>> @@ -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;
>>              }
>>          }
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 1/4] s390x/zpci: drop msix.available

[Cornelia Huck]

On Mon,  5 Nov 2018 12:03:10 +0100
David Hildenbrand <david@redhat.com> 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 <david@redhat.com>
> ---
>  hw/s390x/s390-pci-bus.c | 2 --
>  hw/s390x/s390-pci-bus.h | 1 -
>  2 files changed, 3 deletions(-)

Thanks, queued to s390-next.

Re: [Qemu-devel] [PATCH v1 0/4] s390x/zpci: some hotplug handler cleanups

[Cornelia Huck]

On Mon,  5 Nov 2018 12:03:09 +0100
David Hildenbrand <david@redhat.com> 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).
> 
> 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

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

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

depends on the handler rework

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

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

Re: [Qemu-devel] [PATCH v1 0/4] s390x/zpci: some hotplug handler cleanups

[David Hildenbrand]

On 12.11.18 18:14, Cornelia Huck wrote:
> On Mon,  5 Nov 2018 12:03:09 +0100
> David Hildenbrand <david@redhat.com> 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).
>>
>> 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
> 
>>   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 :)
> 
>>   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).

> 
>>   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?


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 0/4] s390x/zpci: some hotplug handler cleanups

[Cornelia Huck]

On Mon, 12 Nov 2018 18:34:34 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 12.11.18 18:14, Cornelia Huck wrote:
> > On Mon,  5 Nov 2018 12:03:09 +0100
> > David Hildenbrand <david@redhat.com> 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).
> >>
> >> 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
> >   
> >>   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 :)
> >   
> >>   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.

> 
> >   
> >>   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?

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

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 0/4] s390x/zpci: some hotplug handler cleanups

[David Hildenbrand]

On 13.11.18 10:03, Cornelia Huck wrote:
> On Mon, 12 Nov 2018 18:34:34 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 12.11.18 18:14, Cornelia Huck wrote:
>>> On Mon,  5 Nov 2018 12:03:09 +0100
>>> David Hildenbrand <david@redhat.com> 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).
>>>>
>>>> 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
>>>   
>>>>   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 :)
>>>   
>>>>   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.
> 
>>
>>>   
>>>>   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?
> 
> The last one would be great, as I think it's still 3.1 material.
> 

Alrighty, so I'll resend (after testing this time ;) ) the last patch.

-- 

Thanks,

David / dhildenb