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

[Wei Liu]

On Tue, Aug 23, 2016 at 04:00:24PM -0400, Doug Goldstein wrote:
> On 8/18/16 10:13 AM, Wei Liu wrote:
> 
> >  
> > +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;
> > +case LIBXL_BIOS_TYPE_ROMBIOS:
> > +default:
> > +abort();
> 
> Wei,
> 
> Please consider another solution. I've been trying to use libxl from
> Rust and the calls to abort() are just really hard to deal with. I know
> libxl has 50+ calls currently but let's work on reducing these as much
> as possible so that its possible to consume libxl in other languages.
> 
> abort() is just bad form for libraries because you don't give the caller
> the ability to catch the error and handle it appropriately (which could
> be as simple as displaying it to the user or potentially work around the
> issue.
> 
> I know you and Anthony have gone through 8 revisions but please consider
> changing this. I'm sorry for not speaking up sooner.

As said, the abort() in internal function marks an impossible situation
to get into -- much like BUG_ON in hypervisor. I would expect the error
to be handled in some other places in libxl. In this particular
instance, I haven't checked, but if there is no such check elsewhere, I
will either fix it here or somewhere else appropriate.

Furthermore, I'm not averse to systematically removing abort(), but I
would like to at least have a rough idea how upper layer would want to
handle that, and what is the implication on other parts of libxl.

Wei.

> -- 
> Doug Goldstein
> 




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

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

[Doug Goldstein]

On 8/18/16 10:13 AM, Wei Liu wrote:

>  
> +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;
> +case LIBXL_BIOS_TYPE_ROMBIOS:
> +default:
> +abort();

Wei,

Please consider another solution. I've been trying to use libxl from
Rust and the calls to abort() are just really hard to deal with. I know
libxl has 50+ calls currently but let's work on reducing these as much
as possible so that its possible to consume libxl in other languages.

abort() is just bad form for libraries because you don't give the caller
the ability to catch the error and handle it appropriately (which could
be as simple as displaying it to the user or potentially work around the
issue.

I know you and Anthony have gone through 8 revisions but please consider
changing this. I'm sorry for not speaking up sooner.
-- 
Doug Goldstein



signature.asc
Description: OpenPGP digital signature
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Wei Liu]

On Mon, Aug 22, 2016 at 04:09:07PM +0100, Andrew Cooper wrote:
> On 22/08/16 15:26, Wei Liu wrote:
> > 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 
> >
> > 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 
> > Acked-by: Wei Liu 
>  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.
> 
> HVMLoader works fine.  I am not sure how you got this idea.
> 

Huh? If you don't embed seabios or ovmf in hvmloader and then specify
bios=seabios or bios=ovmf in guest config file, I'm sure it will crash
when trying to load bios.

Wei.

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

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

[Andrew Cooper]

On 22/08/16 15:26, Wei Liu wrote:
> 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 
>
> 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 
> Acked-by: Wei Liu 
 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.

HVMLoader works fine.  I am not sure how you got this idea.


For XTF, we use firmware_override= to replace hvmloader with something
else.  As bios= is specific to hvmloader, it shouldn't be mandatory, and
should probably have an explicit none option.

~Andrew

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

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

[Wei Liu]

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 
> >>>
> >>> 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 
> >>> Acked-by: Wei Liu 
> >> 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:
> >>> + 

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

[Andrew Cooper]

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 
>>>
>>> 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 
>>> Acked-by: Wei Liu 
>> 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.

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

[Andrew Cooper]

On 18/08/16 15:13, Wei Liu wrote:
> From: Anthony PERARD 
>
> 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 
> Acked-by: Wei Liu 

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@lists.xen.org
https://lists.xen.org/xen-devel