Re: [Xen-devel] [PATCH v8 05/13] libxl: Load guest BIOS from file
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
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
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
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
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: > >>> +abort(); > >> This is completely antisocial. Under no circumstances
Re: [Xen-devel] [PATCH v8 05/13] libxl: Load guest BIOS from file
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. The difference is some details of the problem going out on stderr. abort()
Re: [Xen-devel] [PATCH v8 05/13] libxl: Load guest BIOS from file
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? > > > @@ -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@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 05/13] libxl: Load guest BIOS from file
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