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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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

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.

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

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

~Andrew

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