[libvirt] [PATCH 2/2] qemu_firmware: Try to autofill for old style UEFI specification

Michal Privoznik mprivozn at redhat.com
Mon Jan 6 16:23:31 UTC 2020


On 12/18/19 11:50 PM, Cole Robinson wrote:
> On 12/17/19 12:25 PM, Michal Privoznik wrote:
>> While we discourage people to use the old style of specifying
>> UEFI for their domains (the old style is putting path to the FW
>> image under /domain/os/loader/ whilst the new one is using
>> /domain/os/@firmware), some applications might have not adopted
>> yet. They still rely on libvirt autofilling NVRAM path and
>> figuring out NVRAM template when using the old way (notably
>> virt-install does this). And in a way they are right. However,
>> since we really want distro maintainers to leave
>> --with-loader-nvram configure option and rely on JSON
>> descriptors, we need to implement autofilling of NVRAM template
>> for the old way too.
>>
>> Fedora: https://bugzilla.redhat.com/show_bug.cgi?id=1782778
>> RHEL: https://bugzilla.redhat.com/show_bug.cgi?id=1776949
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
> 
> So this uses firmware.json to info to turn this input XML
> 
>    <loader readonly="yes" type="pflash">/CODE.fd</loader>
> 
> into this output XML
> 
>    <loader readonly="yes" type="pflash">/CODE.fd</loader>
>    <nvram template="/VARS.fd"/>
> 
> independent of --with-loader-nvram, which was previously used to handle
> that case. And virt-install still uses this method by default. Is that
> correct?

Use of --with-loader-nvram will trigger a warning at compile time and 
use of nvram=[] in qemu.conf will trigger a warning at runtime.

And we need to fill nvram template (in the code it's loader->templt) to 
avoid looking it up qemuPrepareNVRAM(). This is what <os 
firmware='uefi'/> does too.

So the way this is supposed to work is:
- FW descriptors are preferred, if a NVRAM template is found in a FW 
descriptor file it's used,
- to give distro maintainers time to switch, --with-loader-nvram is not 
going away just yet, and will be consulted if the previous point failed,
- nvram=[] from qemu.conf is ignored loudly.

I think this is reasonable order. Also, note that the template is 
generated into live XML only, and will be generated only once - when 
NVRAM template is needed, i.e. at the first boot. The second time the 
domain is started the horrific check I'm introducing in 
qemuFirmwareFillDomain() will trigger 'return 0' and thus no template 
will be filled in.

> 
> I'm surprised we don't have an an XML test case to cover this. How hard
> is it to add?

The thing is, we don't hit the problem when generating the command line 
but later in the process in qemuPrepareNVRAM() which is called from 
qemuProcessPrepareHost() which is not called from tests. And I don't 
think we have a test that checks if live XML generated from an inactive 
one matches expected one.

> 
>>   src/qemu/qemu_firmware.c | 82 +++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 72 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c
>> index 96058c9b45..a31bde5d04 100644
>> --- a/src/qemu/qemu_firmware.c
>> +++ b/src/qemu/qemu_firmware.c
>> @@ -935,17 +935,53 @@ qemuFirmwareMatchDomain(const virDomainDef *def,
>>                           const qemuFirmware *fw,
>>                           const char *path)
>>   {
>> -    size_t i;
>> +    qemuFirmwareOSInterface want = QEMU_FIRMWARE_OS_INTERFACE_NONE;
>>       bool supportsS3 = false;
>>       bool supportsS4 = false;
>>       bool requiresSMM = false;
>>       bool supportsSEV = false;
>> +    size_t i;
>> +
>> +    switch (def->os.firmware) {
>> +    case VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS:
>> +        want = QEMU_FIRMWARE_OS_INTERFACE_BIOS;
>> +        break;
>> +    case VIR_DOMAIN_OS_DEF_FIRMWARE_EFI:
>> +        want = QEMU_FIRMWARE_OS_INTERFACE_UEFI;
>> +        break;
>> +    case VIR_DOMAIN_OS_DEF_FIRMWARE_NONE:
>> +    case VIR_DOMAIN_OS_DEF_FIRMWARE_LAST:
>> +        break;
>> +    }
>> +
> 
> This is a refactoring that could have been done first. It's hard to
> digest all the details in this patch.
> 
>> +    if (want == QEMU_FIRMWARE_OS_INTERFACE_NONE &&
>> +        def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE &&
>> +        def->os.loader) {
>> +        switch (def->os.loader->type) {
>> +        case VIR_DOMAIN_LOADER_TYPE_ROM:
>> +            want = QEMU_FIRMWARE_OS_INTERFACE_BIOS;
>> +            break;
>> +        case VIR_DOMAIN_LOADER_TYPE_PFLASH:
>> +            want = QEMU_FIRMWARE_OS_INTERFACE_UEFI;
>> +            break;
>> +        case VIR_DOMAIN_LOADER_TYPE_NONE:
>> +        case VIR_DOMAIN_LOADER_TYPE_LAST:
>> +            break;
>> +        }
>> +
> 
> And it might be overkill but it would help readability if these switches
> were moved to their own functions, like
> qemuFirmwareTypeFromInterfaceType or similar
> 
>> +        if (fw->mapping.device != QEMU_FIRMWARE_DEVICE_FLASH ||
>> +            STRNEQ(def->os.loader->path, fw->mapping.data.flash.executable.filename)) {
>> +            VIR_DEBUG("Not matching FW interface %s or loader "
>> +                      "path '%s' for user provided path '%s'",
>> +                      qemuFirmwareDeviceTypeToString(fw->mapping.device),
>> +                      fw->mapping.data.flash.executable.filename,
>> +                      def->os.loader->path);
>> +            return false;
>> +        }
>> +    }
>>   
>>       for (i = 0; i < fw->ninterfaces; i++) {
>> -        if ((def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS &&
>> -             fw->interfaces[i] == QEMU_FIRMWARE_OS_INTERFACE_BIOS) ||
>> -            (def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_EFI &&
>> -             fw->interfaces[i] == QEMU_FIRMWARE_OS_INTERFACE_UEFI))
>> +        if (fw->interfaces[i] == want)
>>               break;
>>       }
>>   
>> @@ -1211,14 +1247,33 @@ qemuFirmwareFillDomain(virQEMUDriverPtr driver,
>>       qemuFirmwarePtr *firmwares = NULL;
>>       ssize_t nfirmwares = 0;
>>       const qemuFirmware *theone = NULL;
>> +    bool needResult = true;
>>       size_t i;
>>       int ret = -1;
>>   
>>       if (!(flags & VIR_QEMU_PROCESS_START_NEW))
>>           return 0;
>>   
>> -    if (def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE)
>> -        return 0;
>> +    /* Fill in FW paths if either os.firmware is enabled, or
>> +     * loader path was provided with no nvram varstore. */
>> +    if (def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE) {
>> +        /* This is horrific check, but loosely said, if UEFI
>> +         * image was provided by the old method (by specifying
>> +         * its path in domain XML) but no template for NVRAM was
>> +         * specified ... */
>> +        if (!(def->os.loader &&
>> +              def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH &&
>> +              def->os.loader->path &&
>> +              def->os.loader->readonly == VIR_TRISTATE_BOOL_YES &&
>> +              !def->os.loader->templt &&
>> +              def->os.loader->nvram &&
>> +              !virFileExists(def->os.loader->nvram)))
>> +            return 0;
>> +
> 
> This loader checking logic is duplicated in a few places already, see
> all 4 of 5 PFLASH uses in qemu_domain.c and similar checks in xen code.
> IMO it should be factored out first

Okay, I will try to work these into v2.

Thanks,
Michal




More information about the libvir-list mailing list