| Message ID | 1519725913-24852-1-git-send-email-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/s390x/ipl: Bail out if the network bootloader can not be found | expand |
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>
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); > } >
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
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); >> } >> >
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
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>
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.
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); }
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(-)