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

[Daniel Henrique Barboza]

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 at ventanamicro.com>
>> ---
>>   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