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
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>
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>
>
>
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
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 :)
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
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;
> }
> }
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
© 2016 - 2025 Red Hat, Inc.