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 <address@hidden>
> > > > ---
> > > >    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 <address@hidden>