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

[Daniel Henrique Barboza]

On 3/22/23 06:42, Daniel Henrique Barboza wrote:
> 
> 
> 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.

VIR_DOMAIN_LOADER_TYPE_OTHER seems more appropriate to indicate that the firmware
will be loaded not by QEMU, but other means (e.g. kernel).

I'll do that for v2.


Daniel

> 
> 
> Thanks,
> 
> Daniel
> 
>>
>>
>> With regards,
>> Daniel