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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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

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

~Andrew

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