[Qemu-devel] [PATCH] hw/s390x/ipl: Bail out if the network bootloader can not be found

[Qemu-devel] [PATCH] hw/s390x/ipl: Bail out if the network bootloader can not be found

[Thomas Huth]

If QEMU fails to load 's390-netboot.img', the guest firmware currently
loops forever and just floods the console with "Network boot device
detected" messages. The code in ipl.c apparently already tried to stop
the VM with vm_stop() in this case, but this is in vain since the run
state is later reset due to a call to vm_start() from vl.c again.
To avoid the ugly firmware loop, let's simply exit QEMU directly instead
since it just does not make sense to continue if the required firmware
image can not be loaded. While we're at it, also add the file name of
the netboot binary to the error message, so that the user has a better
hint about what is missing.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/s390x/ipl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 0d06fc1..ff8308e 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -322,7 +322,8 @@ static int load_netboot_image(Error **errp)
 
     netboot_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, ipl->netboot_fw);
     if (netboot_filename == NULL) {
-        error_setg(errp, "Could not find network bootloader");
+        error_setg(errp, "Could not find network bootloader '%s'",
+                   ipl->netboot_fw);
         goto unref_mr;
     }
 
@@ -416,7 +417,7 @@ void s390_ipl_prepare_cpu(S390CPU *cpu)
     if (ipl->netboot) {
         if (load_netboot_image(&err) < 0) {
             error_report_err(err);
-            vm_stop(RUN_STATE_INTERNAL_ERROR);
+            exit(1);
         }
         ipl->iplb.ccw.netboot_start_addr = cpu_to_be64(ipl->start_addr);
     }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] hw/s390x/ipl: Bail out if the network bootloader can not be found

[David Hildenbrand]

On 27.02.2018 11:05, Thomas Huth wrote:
> If QEMU fails to load 's390-netboot.img', the guest firmware currently
> loops forever and just floods the console with "Network boot device
> detected" messages. The code in ipl.c apparently already tried to stop
> the VM with vm_stop() in this case, but this is in vain since the run
> state is later reset due to a call to vm_start() from vl.c again.
> To avoid the ugly firmware loop, let's simply exit QEMU directly instead
> since it just does not make sense to continue if the required firmware
> image can not be loaded. While we're at it, also add the file name of
> the netboot binary to the error message, so that the user has a better
> hint about what is missing.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/s390x/ipl.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 0d06fc1..ff8308e 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -322,7 +322,8 @@ static int load_netboot_image(Error **errp)
>  
>      netboot_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, ipl->netboot_fw);
>      if (netboot_filename == NULL) {
> -        error_setg(errp, "Could not find network bootloader");
> +        error_setg(errp, "Could not find network bootloader '%s'",
> +                   ipl->netboot_fw);
>          goto unref_mr;
>      }
>  
> @@ -416,7 +417,7 @@ void s390_ipl_prepare_cpu(S390CPU *cpu)
>      if (ipl->netboot) {
>          if (load_netboot_image(&err) < 0) {
>              error_report_err(err);
> -            vm_stop(RUN_STATE_INTERNAL_ERROR);
> +            exit(1);
>          }
>          ipl->iplb.ccw.netboot_start_addr = cpu_to_be64(ipl->start_addr);
>      }
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH] hw/s390x/ipl: Bail out if the network bootloader can not be found

[Viktor Mihajlovski]

On 27.02.2018 11:05, Thomas Huth wrote:
> If QEMU fails to load 's390-netboot.img', the guest firmware currently
> loops forever and just floods the console with "Network boot device
> detected" messages. The code in ipl.c apparently already tried to stop
> the VM with vm_stop() in this case, but this is in vain since the run
> state is later reset due to a call to vm_start() from vl.c again.
> To avoid the ugly firmware loop, let's simply exit QEMU directly instead
> since it just does not make sense to continue if the required firmware
> image can not be loaded. While we're at it, also add the file name of
> the netboot binary to the error message, so that the user has a better
> hint about what is missing.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/s390x/ipl.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 0d06fc1..ff8308e 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -322,7 +322,8 @@ static int load_netboot_image(Error **errp)
> 
>      netboot_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, ipl->netboot_fw);
>      if (netboot_filename == NULL) {
> -        error_setg(errp, "Could not find network bootloader");
> +        error_setg(errp, "Could not find network bootloader '%s'",> +                   ipl->netboot_fw);
>          goto unref_mr;
>      }
> 
> @@ -416,7 +417,7 @@ void s390_ipl_prepare_cpu(S390CPU *cpu)
>      if (ipl->netboot) {
>          if (load_netboot_image(&err) < 0) {
>              error_report_err(err);
> -            vm_stop(RUN_STATE_INTERNAL_ERROR);
Should we print something like 'exiting' or 'terminating' here, to make
clear that the situation is terminal? Sometimes errors are reported and
processing continues nonetheless.
> +            exit(1);
>          }
>          ipl->iplb.ccw.netboot_start_addr = cpu_to_be64(ipl->start_addr);
>      }
> 

-- 
Regards,
 Viktor Mihajlovski

Re: [Qemu-devel] [PATCH] hw/s390x/ipl: Bail out if the network bootloader can not be found

[Thomas Huth]

On 27.02.2018 11:16, Viktor Mihajlovski wrote:
> On 27.02.2018 11:05, Thomas Huth wrote:
>> If QEMU fails to load 's390-netboot.img', the guest firmware currently
>> loops forever and just floods the console with "Network boot device
>> detected" messages. The code in ipl.c apparently already tried to stop
>> the VM with vm_stop() in this case, but this is in vain since the run
>> state is later reset due to a call to vm_start() from vl.c again.
>> To avoid the ugly firmware loop, let's simply exit QEMU directly instead
>> since it just does not make sense to continue if the required firmware
>> image can not be loaded. While we're at it, also add the file name of
>> the netboot binary to the error message, so that the user has a better
>> hint about what is missing.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  hw/s390x/ipl.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index 0d06fc1..ff8308e 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -322,7 +322,8 @@ static int load_netboot_image(Error **errp)
>>
>>      netboot_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, ipl->netboot_fw);
>>      if (netboot_filename == NULL) {
>> -        error_setg(errp, "Could not find network bootloader");
>> +        error_setg(errp, "Could not find network bootloader '%s'",> +                   ipl->netboot_fw);
>>          goto unref_mr;
>>      }
>>
>> @@ -416,7 +417,7 @@ void s390_ipl_prepare_cpu(S390CPU *cpu)
>>      if (ipl->netboot) {
>>          if (load_netboot_image(&err) < 0) {
>>              error_report_err(err);
>> -            vm_stop(RUN_STATE_INTERNAL_ERROR);
> Should we print something like 'exiting' or 'terminating' here, to make
> clear that the situation is terminal?
No, we normally don't print out "exiting" or "terminating" in such a
case. For example:

hw/alpha/dp264.c:    error_report("could not load palcode '%s'", palcode_filename);
hw/alpha/dp264.c:    exit(1);

hw/arm/boot.c:       error_report("failed to load \"%s\"", image_name);
hw/arm/boot.c:       exit(1);

hw/mips/mips_jazz.c: error_report("Could not load MIPS bios '%s'", bios_name);
hw/mips/mips_jazz.c: exit(1);

hw/ppc/spapr.c:      error_report("Could not load LPAR rtas '%s'", filename);
hw/ppc/spapr.c:      exit(1);

etc.

 Thomas

Re: [Qemu-devel] [PATCH] hw/s390x/ipl: Bail out if the network bootloader can not be found

[Farhan Ali]

On 02/27/2018 05:16 AM, Viktor Mihajlovski wrote:
> On 27.02.2018 11:05, Thomas Huth wrote:
>> If QEMU fails to load 's390-netboot.img', the guest firmware currently
>> loops forever and just floods the console with "Network boot device
>> detected" messages. The code in ipl.c apparently already tried to stop
>> the VM with vm_stop() in this case, but this is in vain since the run
>> state is later reset due to a call to vm_start() from vl.c again.
>> To avoid the ugly firmware loop, let's simply exit QEMU directly instead
>> since it just does not make sense to continue if the required firmware
>> image can not be loaded. While we're at it, also add the file name of
>> the netboot binary to the error message, so that the user has a better
>> hint about what is missing.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   hw/s390x/ipl.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index 0d06fc1..ff8308e 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -322,7 +322,8 @@ static int load_netboot_image(Error **errp)
>>
>>       netboot_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, ipl->netboot_fw);
>>       if (netboot_filename == NULL) {
>> -        error_setg(errp, "Could not find network bootloader");
>> +        error_setg(errp, "Could not find network bootloader '%s'",> +                   ipl->netboot_fw);
>>           goto unref_mr;
>>       }
>>
>> @@ -416,7 +417,7 @@ void s390_ipl_prepare_cpu(S390CPU *cpu)
>>       if (ipl->netboot) {
>>           if (load_netboot_image(&err) < 0) {
>>               error_report_err(err);
>> -            vm_stop(RUN_STATE_INTERNAL_ERROR);
> Should we print something like 'exiting' or 'terminating' here, to make
> clear that the situation is terminal? Sometimes errors are reported and
> processing continues nonetheless.


I had to go through my old notes to see why I didn't just exit when I 
wrote it, and the reason was so we could put the guest in wait state so 
we can do some diagnostics....

Do we want to change this behavior?

>> +            exit(1);
>>           }
>>           ipl->iplb.ccw.netboot_start_addr = cpu_to_be64(ipl->start_addr);
>>       }
>>
> 

Re: [Qemu-devel] [PATCH] hw/s390x/ipl: Bail out if the network bootloader can not be found

[Thomas Huth]

On 27.02.2018 18:26, Farhan Ali wrote:
> 
> 
> On 02/27/2018 05:16 AM, Viktor Mihajlovski wrote:
>> On 27.02.2018 11:05, Thomas Huth wrote:
>>> If QEMU fails to load 's390-netboot.img', the guest firmware currently
>>> loops forever and just floods the console with "Network boot device
>>> detected" messages. The code in ipl.c apparently already tried to stop
>>> the VM with vm_stop() in this case, but this is in vain since the run
>>> state is later reset due to a call to vm_start() from vl.c again.
>>> To avoid the ugly firmware loop, let's simply exit QEMU directly instead
>>> since it just does not make sense to continue if the required firmware
>>> image can not be loaded. While we're at it, also add the file name of
>>> the netboot binary to the error message, so that the user has a better
>>> hint about what is missing.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   hw/s390x/ipl.c | 5 +++--
>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>>> index 0d06fc1..ff8308e 100644
>>> --- a/hw/s390x/ipl.c
>>> +++ b/hw/s390x/ipl.c
>>> @@ -322,7 +322,8 @@ static int load_netboot_image(Error **errp)
>>>
>>>       netboot_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS,
>>> ipl->netboot_fw);
>>>       if (netboot_filename == NULL) {
>>> -        error_setg(errp, "Could not find network bootloader");
>>> +        error_setg(errp, "Could not find network bootloader '%s'",>
>>> +                   ipl->netboot_fw);
>>>           goto unref_mr;
>>>       }
>>>
>>> @@ -416,7 +417,7 @@ void s390_ipl_prepare_cpu(S390CPU *cpu)
>>>       if (ipl->netboot) {
>>>           if (load_netboot_image(&err) < 0) {
>>>               error_report_err(err);
>>> -            vm_stop(RUN_STATE_INTERNAL_ERROR);
>> Should we print something like 'exiting' or 'terminating' here, to make
>> clear that the situation is terminal? Sometimes errors are reported and
>> processing continues nonetheless.
> 
> 
> I had to go through my old notes to see why I didn't just exit when I
> wrote it, and the reason was so we could put the guest in wait state so
> we can do some diagnostics....
> 
> Do we want to change this behavior?

That intended behavior obviously does not work, since the guest is
started anyway here.

And in this case, it does not make much sense to put the guest into wait
state, since the problem is simply that a firmware image could not be
loaded by QEMU, i.e. it's a problem in the host, not in the guest. Or
which kind of guest diagnostics would you expect here? ... Putting the
guest into stopped state only makes sense if the guest crashed /
panicked, so you could analyze the reason for the crash.

 Thomas

Re: [Qemu-devel] [PATCH] hw/s390x/ipl: Bail out if the network bootloader can not be found

[Farhan Ali]

On 02/27/2018 02:11 PM, Thomas Huth wrote:
> On 27.02.2018 18:26, Farhan Ali wrote:
>>
>>
>> On 02/27/2018 05:16 AM, Viktor Mihajlovski wrote:
>>> On 27.02.2018 11:05, Thomas Huth wrote:
>>>> If QEMU fails to load 's390-netboot.img', the guest firmware currently
>>>> loops forever and just floods the console with "Network boot device
>>>> detected" messages. The code in ipl.c apparently already tried to stop
>>>> the VM with vm_stop() in this case, but this is in vain since the run
>>>> state is later reset due to a call to vm_start() from vl.c again.
>>>> To avoid the ugly firmware loop, let's simply exit QEMU directly instead
>>>> since it just does not make sense to continue if the required firmware
>>>> image can not be loaded. While we're at it, also add the file name of
>>>> the netboot binary to the error message, so that the user has a better
>>>> hint about what is missing.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>    hw/s390x/ipl.c | 5 +++--
>>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>>>> index 0d06fc1..ff8308e 100644
>>>> --- a/hw/s390x/ipl.c
>>>> +++ b/hw/s390x/ipl.c
>>>> @@ -322,7 +322,8 @@ static int load_netboot_image(Error **errp)
>>>>
>>>>        netboot_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS,
>>>> ipl->netboot_fw);
>>>>        if (netboot_filename == NULL) {
>>>> -        error_setg(errp, "Could not find network bootloader");
>>>> +        error_setg(errp, "Could not find network bootloader '%s'",>
>>>> +                   ipl->netboot_fw);
>>>>            goto unref_mr;
>>>>        }
>>>>
>>>> @@ -416,7 +417,7 @@ void s390_ipl_prepare_cpu(S390CPU *cpu)
>>>>        if (ipl->netboot) {
>>>>            if (load_netboot_image(&err) < 0) {
>>>>                error_report_err(err);
>>>> -            vm_stop(RUN_STATE_INTERNAL_ERROR);
>>> Should we print something like 'exiting' or 'terminating' here, to make
>>> clear that the situation is terminal? Sometimes errors are reported and
>>> processing continues nonetheless.
>>
>>
>> I had to go through my old notes to see why I didn't just exit when I
>> wrote it, and the reason was so we could put the guest in wait state so
>> we can do some diagnostics....
>>
>> Do we want to change this behavior?
> 
> That intended behavior obviously does not work, since the guest is
> started anyway here.
> 
> And in this case, it does not make much sense to put the guest into wait
> state, since the problem is simply that a firmware image could not be
> loaded by QEMU, i.e. it's a problem in the host, not in the guest. Or
> which kind of guest diagnostics would you expect here? ... Putting the
> guest into stopped state only makes sense if the guest crashed /
> panicked, so you could analyze the reason for the crash.
> 
>   Thomas
> 

You are right and this would be the right thing to do.

Reviewed-by: Farhan Ali <alifm@linux.vnet.ibm.com>

Re: [Qemu-devel] [PATCH] hw/s390x/ipl: Bail out if the network bootloader can not be found

[Cornelia Huck]

On Tue, 27 Feb 2018 11:05:13 +0100
Thomas Huth <thuth@redhat.com> wrote:

> If QEMU fails to load 's390-netboot.img', the guest firmware currently
> loops forever and just floods the console with "Network boot device
> detected" messages. The code in ipl.c apparently already tried to stop
> the VM with vm_stop() in this case, but this is in vain since the run
> state is later reset due to a call to vm_start() from vl.c again.
> To avoid the ugly firmware loop, let's simply exit QEMU directly instead
> since it just does not make sense to continue if the required firmware
> image can not be loaded. While we're at it, also add the file name of
> the netboot binary to the error message, so that the user has a better
> hint about what is missing.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/s390x/ipl.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Thanks, applied.