* Re: [Qemu-devel] [PATCH] hw/s390x: Add the possibility to specify the netboot image on the command line
2018-02-27 11:32 [Qemu-devel] [PATCH] hw/s390x: Add the possibility to specify the netboot image on the command line Thomas Huth
@ 2018-02-27 13:27 ` Viktor Mihajlovski
2018-02-27 19:04 ` Thomas Huth
2018-02-28 11:02 ` [Qemu-devel] [qemu-s390x] " David Hildenbrand
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Viktor Mihajlovski @ 2018-02-27 13:27 UTC (permalink / raw)
To: Thomas Huth, Cornelia Huck, qemu-s390x, qemu-devel
Cc: Christian Borntraeger, David Hildenbrand
On 27.02.2018 12:32, Thomas Huth wrote:
> The file name of the netboot binary is currently hard-coded to
> "s390-netboot.img", without a possibility for the user to select
> an alternative firmware image here. That's unfortunate, especially
> since the basics are already there: The filename is a property of
> the s390-ipl device. So we just have to add a check whether the user
> already provided the property and only set the default if the string
> is still empty. Now it is possible to select a different firmware
> image with "-global s390-ipl.netboot_fw=/path/to/s390-netboot.img".
> While I sympathize with the concept, it will be a bit hard to consume
since most of the QEMU instances will be started by libvirt and that has
no provisions for this kind of firmware replacement. We could craft some
s390-specific variety of the <loader> element.
Another thing to consider is that, while the current netboot firmware
doesn't rely on any special QEMU <-> loader interfaces, it might happen
that QEMU and the network firmware must match similar to QEMU and the
s390-ccw firmware image and I have no idea on how to ensure that.
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> hw/s390x/s390-virtio-ccw.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 4abbe89..7b3fb5f 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -254,8 +254,10 @@ static void s390_init_ipl_dev(const char *kernel_filename,
> }
> qdev_prop_set_string(dev, "cmdline", kernel_cmdline);
> qdev_prop_set_string(dev, "firmware", firmware);
Wouldn't it be consequent to allow firmware replacement? But then, see
above.
> - qdev_prop_set_string(dev, "netboot_fw", netboot_fw);
> qdev_prop_set_bit(dev, "enforce_bios", enforce_bios);
> + if (!strlen(object_property_get_str(new, "netboot_fw", &error_abort))) {
> + qdev_prop_set_string(dev, "netboot_fw", netboot_fw);
> + }
> object_property_add_child(qdev_get_machine(), TYPE_S390_IPL,
> new, NULL);
> object_unref(new);
>
--
Regards,
Viktor Mihajlovski
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] hw/s390x: Add the possibility to specify the netboot image on the command line
2018-02-27 13:27 ` Viktor Mihajlovski
@ 2018-02-27 19:04 ` Thomas Huth
0 siblings, 0 replies; 9+ messages in thread
From: Thomas Huth @ 2018-02-27 19:04 UTC (permalink / raw)
To: Viktor Mihajlovski, Cornelia Huck, qemu-s390x, qemu-devel
Cc: Christian Borntraeger, David Hildenbrand
On 27.02.2018 14:27, Viktor Mihajlovski wrote:
> On 27.02.2018 12:32, Thomas Huth wrote:
>> The file name of the netboot binary is currently hard-coded to
>> "s390-netboot.img", without a possibility for the user to select
>> an alternative firmware image here. That's unfortunate, especially
>> since the basics are already there: The filename is a property of
>> the s390-ipl device. So we just have to add a check whether the user
>> already provided the property and only set the default if the string
>> is still empty. Now it is possible to select a different firmware
>> image with "-global s390-ipl.netboot_fw=/path/to/s390-netboot.img".
>> While I sympathize with the concept, it will be a bit hard to consume
> since most of the QEMU instances will be started by libvirt and that has
> no provisions for this kind of firmware replacement. We could craft some
> s390-specific variety of the <loader> element.
> Another thing to consider is that, while the current netboot firmware
> doesn't rely on any special QEMU <-> loader interfaces, it might happen
> that QEMU and the network firmware must match similar to QEMU and the
> s390-ccw firmware image and I have no idea on how to ensure that.
Sure, the normal user likely does not (and should not have to) care
about this, and this patch also has not been written with libvirt in
mind. It's rather meant for QEMU developer convenience: When I'm
changing the code in pc-bios/s390-ccw/ and compiling new images there, I
want to use the binary pc-bios/s390-ccw/s390-netboot.img, and not the
pre-built image pc-bios/s390-netboot.img. So there must be a way to
specify my own images somehow...
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>> hw/s390x/s390-virtio-ccw.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 4abbe89..7b3fb5f 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -254,8 +254,10 @@ static void s390_init_ipl_dev(const char *kernel_filename,
>> }
>> qdev_prop_set_string(dev, "cmdline", kernel_cmdline);
>> qdev_prop_set_string(dev, "firmware", firmware);
> Wouldn't it be consequent to allow firmware replacement? But then, see
> above.
You can already override the main firmware file name with the "-bios"
parameter of QEMU, so there's no real need to change this here.
Thomas
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [qemu-s390x] [PATCH] hw/s390x: Add the possibility to specify the netboot image on the command line
2018-02-27 11:32 [Qemu-devel] [PATCH] hw/s390x: Add the possibility to specify the netboot image on the command line Thomas Huth
2018-02-27 13:27 ` Viktor Mihajlovski
@ 2018-02-28 11:02 ` David Hildenbrand
2018-02-28 12:24 ` Thomas Huth
2018-03-05 10:23 ` [Qemu-devel] " Cornelia Huck
2018-03-06 9:24 ` Cornelia Huck
3 siblings, 1 reply; 9+ messages in thread
From: David Hildenbrand @ 2018-02-28 11:02 UTC (permalink / raw)
To: Thomas Huth, Cornelia Huck, qemu-s390x, qemu-devel; +Cc: Christian Borntraeger
On 27.02.2018 12:32, Thomas Huth wrote:
> The file name of the netboot binary is currently hard-coded to
> "s390-netboot.img", without a possibility for the user to select
> an alternative firmware image here. That's unfortunate, especially
> since the basics are already there: The filename is a property of
> the s390-ipl device. So we just have to add a check whether the user
> already provided the property and only set the default if the string
> is still empty. Now it is possible to select a different firmware
> image with "-global s390-ipl.netboot_fw=/path/to/s390-netboot.img".
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> hw/s390x/s390-virtio-ccw.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 4abbe89..7b3fb5f 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -254,8 +254,10 @@ static void s390_init_ipl_dev(const char *kernel_filename,
> }
> qdev_prop_set_string(dev, "cmdline", kernel_cmdline);
> qdev_prop_set_string(dev, "firmware", firmware);
> - qdev_prop_set_string(dev, "netboot_fw", netboot_fw);
> qdev_prop_set_bit(dev, "enforce_bios", enforce_bios);
> + if (!strlen(object_property_get_str(new, "netboot_fw", &error_abort))) {
Isn't it the case that object_property_get_str() can return also NULL?
(looking at s390_ipl_set_loadparm())
> + qdev_prop_set_string(dev, "netboot_fw", netboot_fw);
> + }
> object_property_add_child(qdev_get_machine(), TYPE_S390_IPL,
> new, NULL);
> object_unref(new);
>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [qemu-s390x] [PATCH] hw/s390x: Add the possibility to specify the netboot image on the command line
2018-02-28 11:02 ` [Qemu-devel] [qemu-s390x] " David Hildenbrand
@ 2018-02-28 12:24 ` Thomas Huth
2018-03-05 10:21 ` Cornelia Huck
0 siblings, 1 reply; 9+ messages in thread
From: Thomas Huth @ 2018-02-28 12:24 UTC (permalink / raw)
To: David Hildenbrand, Cornelia Huck, qemu-s390x, qemu-devel
Cc: Christian Borntraeger
On 28.02.2018 12:02, David Hildenbrand wrote:
> On 27.02.2018 12:32, Thomas Huth wrote:
>> The file name of the netboot binary is currently hard-coded to
>> "s390-netboot.img", without a possibility for the user to select
>> an alternative firmware image here. That's unfortunate, especially
>> since the basics are already there: The filename is a property of
>> the s390-ipl device. So we just have to add a check whether the user
>> already provided the property and only set the default if the string
>> is still empty. Now it is possible to select a different firmware
>> image with "-global s390-ipl.netboot_fw=/path/to/s390-netboot.img".
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>> hw/s390x/s390-virtio-ccw.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 4abbe89..7b3fb5f 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -254,8 +254,10 @@ static void s390_init_ipl_dev(const char *kernel_filename,
>> }
>> qdev_prop_set_string(dev, "cmdline", kernel_cmdline);
>> qdev_prop_set_string(dev, "firmware", firmware);
>> - qdev_prop_set_string(dev, "netboot_fw", netboot_fw);
>> qdev_prop_set_bit(dev, "enforce_bios", enforce_bios);
>> + if (!strlen(object_property_get_str(new, "netboot_fw", &error_abort))) {
>
> Isn't it the case that object_property_get_str() can return also NULL?
>
> (looking at s390_ipl_set_loadparm())
It can return NULL in case of errors (e.g. the property is not a string
or not available at all). In this case, we know that the property is a
string and that it is available, so IMHO no need to check for NULL here.
Not sure why s390_ipl_set_loadparm() explicitely checks for this ...
maybe this was originally required to support the old s390-virtio
(non-ccw) machine, too?
Thomas
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [qemu-s390x] [PATCH] hw/s390x: Add the possibility to specify the netboot image on the command line
2018-02-28 12:24 ` Thomas Huth
@ 2018-03-05 10:21 ` Cornelia Huck
0 siblings, 0 replies; 9+ messages in thread
From: Cornelia Huck @ 2018-03-05 10:21 UTC (permalink / raw)
To: Thomas Huth
Cc: David Hildenbrand, qemu-s390x, qemu-devel, Christian Borntraeger
On Wed, 28 Feb 2018 13:24:35 +0100
Thomas Huth <thuth@redhat.com> wrote:
> On 28.02.2018 12:02, David Hildenbrand wrote:
> > On 27.02.2018 12:32, Thomas Huth wrote:
> >> The file name of the netboot binary is currently hard-coded to
> >> "s390-netboot.img", without a possibility for the user to select
> >> an alternative firmware image here. That's unfortunate, especially
> >> since the basics are already there: The filename is a property of
> >> the s390-ipl device. So we just have to add a check whether the user
> >> already provided the property and only set the default if the string
> >> is still empty. Now it is possible to select a different firmware
> >> image with "-global s390-ipl.netboot_fw=/path/to/s390-netboot.img".
> >>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >> ---
> >> hw/s390x/s390-virtio-ccw.c | 4 +++-
> >> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> >> index 4abbe89..7b3fb5f 100644
> >> --- a/hw/s390x/s390-virtio-ccw.c
> >> +++ b/hw/s390x/s390-virtio-ccw.c
> >> @@ -254,8 +254,10 @@ static void s390_init_ipl_dev(const char *kernel_filename,
> >> }
> >> qdev_prop_set_string(dev, "cmdline", kernel_cmdline);
> >> qdev_prop_set_string(dev, "firmware", firmware);
> >> - qdev_prop_set_string(dev, "netboot_fw", netboot_fw);
> >> qdev_prop_set_bit(dev, "enforce_bios", enforce_bios);
> >> + if (!strlen(object_property_get_str(new, "netboot_fw", &error_abort))) {
> >
> > Isn't it the case that object_property_get_str() can return also NULL?
> >
> > (looking at s390_ipl_set_loadparm())
>
> It can return NULL in case of errors (e.g. the property is not a string
> or not available at all). In this case, we know that the property is a
> string and that it is available, so IMHO no need to check for NULL here.
>
> Not sure why s390_ipl_set_loadparm() explicitely checks for this ...
> maybe this was originally required to support the old s390-virtio
> (non-ccw) machine, too?
Probably, IIRC the loadparm patches are quite old. Potential cleanup?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/s390x: Add the possibility to specify the netboot image on the command line
2018-02-27 11:32 [Qemu-devel] [PATCH] hw/s390x: Add the possibility to specify the netboot image on the command line Thomas Huth
2018-02-27 13:27 ` Viktor Mihajlovski
2018-02-28 11:02 ` [Qemu-devel] [qemu-s390x] " David Hildenbrand
@ 2018-03-05 10:23 ` Cornelia Huck
2018-03-05 11:19 ` Christian Borntraeger
2018-03-06 9:24 ` Cornelia Huck
3 siblings, 1 reply; 9+ messages in thread
From: Cornelia Huck @ 2018-03-05 10:23 UTC (permalink / raw)
To: Thomas Huth
Cc: qemu-s390x, qemu-devel, Christian Borntraeger, David Hildenbrand
On Tue, 27 Feb 2018 12:32:34 +0100
Thomas Huth <thuth@redhat.com> wrote:
> The file name of the netboot binary is currently hard-coded to
> "s390-netboot.img", without a possibility for the user to select
> an alternative firmware image here. That's unfortunate, especially
> since the basics are already there: The filename is a property of
> the s390-ipl device. So we just have to add a check whether the user
> already provided the property and only set the default if the string
> is still empty. Now it is possible to select a different firmware
> image with "-global s390-ipl.netboot_fw=/path/to/s390-netboot.img".
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> hw/s390x/s390-virtio-ccw.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 4abbe89..7b3fb5f 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -254,8 +254,10 @@ static void s390_init_ipl_dev(const char *kernel_filename,
> }
> qdev_prop_set_string(dev, "cmdline", kernel_cmdline);
> qdev_prop_set_string(dev, "firmware", firmware);
> - qdev_prop_set_string(dev, "netboot_fw", netboot_fw);
> qdev_prop_set_bit(dev, "enforce_bios", enforce_bios);
> + if (!strlen(object_property_get_str(new, "netboot_fw", &error_abort))) {
> + qdev_prop_set_string(dev, "netboot_fw", netboot_fw);
> + }
> object_property_add_child(qdev_get_machine(), TYPE_S390_IPL,
> new, NULL);
> object_unref(new);
So, any objections to me merging this? I think this makes sense for
Thomas' use case.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] hw/s390x: Add the possibility to specify the netboot image on the command line
2018-03-05 10:23 ` [Qemu-devel] " Cornelia Huck
@ 2018-03-05 11:19 ` Christian Borntraeger
0 siblings, 0 replies; 9+ messages in thread
From: Christian Borntraeger @ 2018-03-05 11:19 UTC (permalink / raw)
To: Cornelia Huck, Thomas Huth; +Cc: qemu-s390x, qemu-devel, David Hildenbrand
On 03/05/2018 11:23 AM, Cornelia Huck wrote:
> On Tue, 27 Feb 2018 12:32:34 +0100
> Thomas Huth <thuth@redhat.com> wrote:
>
>> The file name of the netboot binary is currently hard-coded to
>> "s390-netboot.img", without a possibility for the user to select
>> an alternative firmware image here. That's unfortunate, especially
>> since the basics are already there: The filename is a property of
>> the s390-ipl device. So we just have to add a check whether the user
>> already provided the property and only set the default if the string
>> is still empty. Now it is possible to select a different firmware
>> image with "-global s390-ipl.netboot_fw=/path/to/s390-netboot.img".
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>> hw/s390x/s390-virtio-ccw.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 4abbe89..7b3fb5f 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -254,8 +254,10 @@ static void s390_init_ipl_dev(const char *kernel_filename,
>> }
>> qdev_prop_set_string(dev, "cmdline", kernel_cmdline);
>> qdev_prop_set_string(dev, "firmware", firmware);
>> - qdev_prop_set_string(dev, "netboot_fw", netboot_fw);
>> qdev_prop_set_bit(dev, "enforce_bios", enforce_bios);
>> + if (!strlen(object_property_get_str(new, "netboot_fw", &error_abort))) {
>> + qdev_prop_set_string(dev, "netboot_fw", netboot_fw);
>> + }
>> object_property_add_child(qdev_get_machine(), TYPE_S390_IPL,
>> new, NULL);
>> object_unref(new);
>
> So, any objections to me merging this? I think this makes sense for
> Thomas' use case.
Fine with me.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/s390x: Add the possibility to specify the netboot image on the command line
2018-02-27 11:32 [Qemu-devel] [PATCH] hw/s390x: Add the possibility to specify the netboot image on the command line Thomas Huth
` (2 preceding siblings ...)
2018-03-05 10:23 ` [Qemu-devel] " Cornelia Huck
@ 2018-03-06 9:24 ` Cornelia Huck
3 siblings, 0 replies; 9+ messages in thread
From: Cornelia Huck @ 2018-03-06 9:24 UTC (permalink / raw)
To: Thomas Huth
Cc: qemu-s390x, qemu-devel, Christian Borntraeger, David Hildenbrand
On Tue, 27 Feb 2018 12:32:34 +0100
Thomas Huth <thuth@redhat.com> wrote:
> The file name of the netboot binary is currently hard-coded to
> "s390-netboot.img", without a possibility for the user to select
> an alternative firmware image here. That's unfortunate, especially
> since the basics are already there: The filename is a property of
> the s390-ipl device. So we just have to add a check whether the user
> already provided the property and only set the default if the string
> is still empty. Now it is possible to select a different firmware
> image with "-global s390-ipl.netboot_fw=/path/to/s390-netboot.img".
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> hw/s390x/s390-virtio-ccw.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 4abbe89..7b3fb5f 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -254,8 +254,10 @@ static void s390_init_ipl_dev(const char *kernel_filename,
> }
> qdev_prop_set_string(dev, "cmdline", kernel_cmdline);
> qdev_prop_set_string(dev, "firmware", firmware);
> - qdev_prop_set_string(dev, "netboot_fw", netboot_fw);
> qdev_prop_set_bit(dev, "enforce_bios", enforce_bios);
> + if (!strlen(object_property_get_str(new, "netboot_fw", &error_abort))) {
> + qdev_prop_set_string(dev, "netboot_fw", netboot_fw);
> + }
> object_property_add_child(qdev_get_machine(), TYPE_S390_IPL,
> new, NULL);
> object_unref(new);
Thanks, applied.
^ permalink raw reply [flat|nested] 9+ messages in thread