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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Acked-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> 
> 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?

> 
> > @@ -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.

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. And I don't see how switching to assert(3) help in
those cases, that ends up calling abort(3) anyway.

Wei.

> ~Andrew

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