Re: [PATCH 1/4] conf: add loader type 'none'

On 3/22/23 06:25, Daniel P. Berrangé wrote: > On Wed, Mar 22, 2023 at 06:10:18AM -0300, Daniel Henrique Barboza wrote: >> Today, trying to boot a RISC-V Fedora Rawhide image in a RISC-V QEMU domain >> results in the following error: >> >> ==== >> error: Failed to start domain 'riscv-fedora' >> error: internal error: process exited while connecting to monitor: >> 2023-03-20T17:31:02.650862Z qemu-system-riscv64: Some ROM regions are overlapping >> These ROM regions might have been loaded by direct user request or by default. >> They could be BIOS/firmware images, a guest kernel, initrd or some other file loaded >> into guest memory. >> Check whether you intended to load all this guest code, and whether it has been built >> to load to the correct addresses. >> ==== >> >> This happens because the default RISC-V QEMU firmware, OpenSBI, is >> always loaded unless "-bios none" is passed in the command line, and the >> Fedora Rawhide guest kernel has its own ROM. Other machines such as >> PPC64 'pseries' shows the same behavior: the default firmware is always >> loaded unless specified otherwise with the '-bios' option. >> >> At this moment we don't have XML support for '-bios none'. Using >> "<qemu:commandline>" works but it will leave the domain in a tainted >> state. It'll also have unpredictable consequences with the autoselect >> firmware feature libvirt has. >> >> Add a new loader type 'none' that, if no path is specified and we're not >> use firmware autoselection,  will tell QEMU that no default firmware >> should be used: >> >> <os> >>    <loader type='none'/> >>     (...) >> </os> >> >> Signed-off-by: Daniel Henrique Barboza <dbarboza(a)ventanamicro.com&gt; >> --- >>   src/conf/domain_conf.c            | 5 +++-- >>   src/conf/domain_validate.c        | 2 +- >>   src/conf/schemas/domaincommon.rng | 1 + >>   3 files changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >> index 9ef50c818b..d7a5cd094b 100644 >> --- a/src/conf/domain_conf.c >> +++ b/src/conf/domain_conf.c >> @@ -16837,7 +16837,7 @@ virDomainLoaderDefParseXMLLoader(virDomainLoaderDef *loader, >>           return -1; >>       if (virXMLPropEnum(loaderNode, "type", virDomainLoaderTypeFromString, >> -                       VIR_XML_PROP_NONZERO, &loader->type) < 0) >> +                       VIR_XML_PROP_NONE, &loader->type) < 0) >>           return -1; >>       if (!(loader->path = virXMLNodeContentString(loaderNode))) >> @@ -26259,7 +26259,8 @@ virDomainLoaderDefFormat(virBuffer *buf, >>           virBufferAsprintf(&loaderAttrBuf, " secure='%s'", >>                             virTristateBoolTypeToString(loader->secure)); >> -    if (loader->type != VIR_DOMAIN_LOADER_TYPE_NONE) >> +    if (loader->type != VIR_DOMAIN_LOADER_TYPE_NONE || >> +        (loader->type == VIR_DOMAIN_LOADER_TYPE_NONE && !loader->path)) >>           virBufferAsprintf(&loaderAttrBuf, " type='%s'", >>                             virDomainLoaderTypeToString(loader->type)); >> > > VIR_DOMAIN_LOADER_TYPE_NONE is a constant we're already using internally > to track when the user has not given any <loader> element at all. > > It is not a good idea to overload for this for the user explicitly > requesting a config. Makes sense. Any name suggestions for this new constant? VIR_DOMAIN_LOADER_TYPE_SKIP is the first thing that comes to mind. If we want to keep it consistent we should also change this new type to <loader type='skip'/> as well.

Thanks, Daniel > > > With regards, > Daniel