Re: [PATCH v8 05/13] libxl: Load guest BIOS from file

[Wei Liu <wei.liu2@citrix.com>]

On Mon, Aug 22, 2016 at 02:26:11PM +0100, Andrew Cooper wrote:
> On 22/08/16 14:13, Wei Liu wrote:
> > On Fri, Aug 19, 2016 at 03:43:00PM +0100, Andrew Cooper wrote:
> >> On 18/08/16 15:13, Wei Liu wrote:
> >>> From: Anthony PERARD <anthony.perard@citrix.com>
> >>>
> >>> The path to the BIOS blob can be overriden by the xl's
> >>> bios_path_override option, or provided by u.hvm.bios_firmware in the
> >>> domain_build_info struct by other libxl user.
> >>>
> >>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> >>> Acked-by: Wei Liu <wei.liu2@citrix.com>
> >> This introduces a regression, but I am not sure how best to fix it.
> >>
> >> [root@xrtuk-09-12 xen-test-framework]# xl -vvv create -p
> >> tests/selftest/test-hvm32-selftest.cfg
> >> Parsing config from tests/selftest/test-hvm32-selftest.cfg
> >> libxl: debug: libxl_create.c:1603:do_domain_create: ao 0xa6b9f0: create:
> >> how=(nil) callback=(nil) poller=0xa6c120
> >> libxl: debug: libxl_create.c:959:initiate_domain_create: running bootloader
> >> libxl: debug: libxl_bootloader.c:324:libxl__bootloader_run: not a PV
> >> domain, skipping bootloader
> >> libxl: debug: libxl_event.c:686:libxl__ev_xswatch_deregister: watch
> >> w=0xa6cc30: deregister unregistered
> >> libxl: debug: libxl_numa.c:483:libxl__get_numa_candidate: New best NUMA
> >> placement candidate found: nr_nodes=1, nr_cpus=12, nr_vcpus=17,
> >> free_memkb=30611
> >> libxl: detail: libxl_dom.c:182:numa_place_domain: NUMA placement
> >> candidate with 1 nodes, 12 cpus and 30611 KB free selected
> >> domainbuilder: detail: xc_dom_allocate: cmdline="(null)", features="(null)"
> >> domainbuilder: detail: xc_dom_kernel_file:
> >> filename="/opt/xen-test-framework/tests/selftest/test-hvm32-selftest"
> >> domainbuilder: detail: xc_dom_malloc_filemap    : 1090 kB
> >> libxl: debug: libxl_dom.c:874:libxl__load_hvm_firmware_module: Loading
> >> BIOS: /usr/libexec/xen/boot/bios.bin
> >> libxl: error: libxl_dom.c:882:libxl__load_hvm_firmware_module: failed to
> >> read BIOS file: No such file or directory
> >>
> >> In this case, tools have been built with ./configure --disable-seabios
> >>
> >> As a result, /usr/libexec/xen/boot/bios.bin (name altered a patch sent
> >> separately) isn't built or installed.
> >>
> >> I think libxl needs to logic to determine which firmware to use based on
> >> the available CONFIG_* options it was built with.
> > I don' quite follow here.
> >
> > Do you mean if user hasn't specified any bios= option, (s)he gets
> > whatever available?
> >
> > I think we should stick with the seabios-default behaviour to avoid
> > surprising breakage.
> >
> > If you don't want any bois, maybe we should provide a bios=none option?
> 
> XenServer is built with ./configure --disable-seabios because we don't
> use it (yet).  This means that SeaBIOS is not built, installed, or
> available.
> 
> Because of this change, libxl unconditionally assumes the presence of
> SeaBIOS, and tries to use the installed seabios binary.

Right, this needs fixing.

We can restore the behaviour in libxl -- if you specify a not available
bios, libxl won't complain, hvmloader will crash in the same way as
before.

> 
> >
> >>> @@ -914,6 +951,30 @@ static int libxl__domain_firmware(libxl__gc *gc,
> >>>          goto out;
> >>>      }
> >>>  
> >>> +    if (info->device_model_version == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
> >>> +        if (info->u.hvm.system_firmware) {
> >>> +            bios_filename = info->u.hvm.system_firmware;
> >>> +        } else {
> >>> +            switch (info->u.hvm.bios) {
> >>> +            case LIBXL_BIOS_TYPE_SEABIOS:
> >>> +                bios_filename = libxl__seabios_path();
> >>> +                break;
> >>> +            case LIBXL_BIOS_TYPE_OVMF:
> >>> +                bios_filename = libxl__ovmf_path();
> >>> +                break;
> >> At the very least, these need to be guarded by #ifdef
> >> CONFIG_{SEABIOS,OVMF}, as it is explicitly permitted for them not to be
> >> present in a build.
> >>
> >>> +            case LIBXL_BIOS_TYPE_ROMBIOS:
> >> ROMBIOS certainly does function correctly with QEMU_XEN, and is how
> >> XenServer is planning to start the migration from a qemu-trad world to a
> >> qemu-upstream world.  Even if libxl doesn't want to formally support
> >> such a configuration, it shouldn't be excluded.
> >>
> > There is no written statement, but I would rather not support this
> > configuration.
> 
> Rightly or wrongly, it is already available, usable and working (until
> this changeset), via supported configuration options.
> 

Ian, do you have more insight on whether that is supported?

> > I expect this is an impossible situation to get into, since verification
> > should have been done a few steps before -- hence the abort(3) here is
> > justified. But I would need to double-check if that's not the case and
> > will do something about it either here or at the place I see
> > appropriate.
> >
> >>> +            default:
> >>> +                abort();
> >> This is completely antisocial.  Under no circumstances is it ok for a
> >> library to call abort(); fail an assertion if necessary, but this is a
> >> configuration error and should pass an error back to its caller, not
> >> take the entire process with it.
> >>
> > In general it is ok to call abort(3) in an internal function that only
> > expects valid input.
> 
> No.  It very much is not.
> 
> >  And I don't see how switching to assert(3) help in
> > those cases, that ends up calling abort(3) anyway.
> 
> The difference is some details of the problem going out on stderr. 
> abort() causes the process to cease to exist without any trace.
> 

I forgot to mention that assert(3) will generate no code in non-debug
build. There is no point to continue the program, really.

Anyway, libxl is already strewn with abort(3). If you're interested in a
assert(3) vs abort(3) debate, please start a new thread.

Wei.

> ~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel