[virt-tools-list] [PATCH virt-manager] details: Add UI for enabling UEFI via 'customize before install'

[Cole Robinson]

On 09/21/2014 10:58 AM, Laszlo Ersek wrote:
> Hi Cole and Michal,
> 
> I'm attaching three patches:
> 
> On 09/20/14 02:37, Cole Robinson wrote:
>> On 09/17/2014 06:55 PM, Cole Robinson wrote:
>>> We expose a simple combobox with two entries: BIOS, and UEFI. The
>>> UEFI option is only selectable if 1) libvirt supports the necessary
>>> domcapabilities bits, 2) it detects that qemu supports the necessary
>>> command line options, and 3) libvirt detects a UEFI binary on the
>>> host that maps to a known template via qemu.conf
>>>
>>> If those conditions aren't met, we disable the UEFI option, and show
>>> a small warning icon with an explanatory tooltip.
>>>
>>> The option can only be changed via New VM->Customize Before Install.
>>> For existing x86 VMs, it's a readonly label.
>>
>> I've pushed this now, with follow up patches to handle a couple points
>> we discussed:
>>
>> - Remove the nvram deletion from delete.py, since it won't work in the
>> non-root case, and all uses of nvram should also be with a libvirt +
>> driver that provides UNDEFINE_NVRAM
>>
>> - Before using UNDEFINE_NVRAM, match the same condition that libvirt
>> uses: loader_ro is True and loader_type == "pflash" and bool(nvram)
> 
> (1) unfortunately virt-manager commit 3feedb76 ("domain: Match
> UNDEFINE_NVRAM conditions with libvirt") is not correct (it doesn't
> work), and not what I had in mind:
> 
>> diff --git a/virtManager/domain.py b/virtManager/domain.py
>> index 29f3861..abf3146 100644
>> --- a/virtManager/domain.py
>> +++ b/virtManager/domain.py
>> @@ -1403,7 +1403,9 @@ class vmmDomain(vmmLibvirtObject):
>>              flags |= getattr(libvirt,
>>                               "VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA", 0)
>>              flags |= getattr(libvirt, "VIR_DOMAIN_UNDEFINE_MANAGED_SAVE", 0)
>> -            if self.get_xmlobj().os.nvram:
>> +            if (self.get_xmlobj().os.loader_ro is True and
>> +                self.get_xmlobj().os.loader_type == "pflash" and
>> +                self.get_xmlobj().os.nvram):
>>                  flags |= getattr(libvirt, "VIR_DOMAIN_UNDEFINE_NVRAM", 0)
>>          try:
>>              self._backend.undefineFlags(flags)
> 
> Before virt-manager commit 3feedb76, the condition on which to set the
> flag was:
> 
>   self.get_xmlobj().os.nvram
> 
> and it didn't work for all possible cases.
> 
> After the patch, what you have is
> 
>   self.get_xmlobj().os.nvram *and* blah
> 
> The commit only constrains (further tightens, further restricts) the
> original condition, which had been too restrictive to begin with. Again,
> the nvram element (or its text() child) can be completely absent from
> the domain XML -- libvirtd will generate the pathname of the varstore
> file on the fly, and it need not be part of the serialized XML file. So
> in the XML all you'll have is
> 
>   <os>
>     <type arch='x86_64' machine='pc-i440fx-rhel7.0.0'>hvm</type>
>     <loader readonly='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader>
>     <boot dev='cdrom'/>
>   </os>
> 
> or
> 
>   <os>
>     <type arch='x86_64' machine='pc-i440fx-rhel7.0.0'>hvm</type>
>     <loader readonly='yes' type='pflash'>/custom/OVMF_CODE.fd</loader>
>     <nvram template='/custom/OVMF_VARS.fd'/>
>     <boot dev='cdrom'/>
>   </os>
> 
> My suggestion was to *replace* the original condition with the
> (loader_ro && loader_type=="pflash") one. If you check my earlier
> message, I wrote *iff*, meaning "if and only if".
> 
> Cole, can you please apply the first attached patch?
> 
> 
> (2) Unfortunately, even libvirtd needs to be modified, in addition.
> 
> My patch for (1) *does* pass VIR_DOMAIN_UNDEFINE_NVRAM to libvirtd (I
> verified that with gdb), but libvirtd doesn't act upon it correctly.
> Namely, in the libvirtd source code, in qemuDomainUndefineFlags(),
> commit 273b6581 added:
> 
>     if (!virDomainObjIsActive(vm) &&
>         vm->def->os.loader && vm->def->os.loader->nvram &&
>         virFileExists(vm->def->os.loader->nvram)) {
>         if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) {
>             virReportError(VIR_ERR_OPERATION_INVALID, "%s",
>                            _("cannot delete inactive domain with nvram"));
>             goto cleanup;
>         }
> 
>         if (unlink(vm->def->os.loader->nvram) < 0) {
>             virReportSystemError(errno,
>                                  _("failed to remove nvram: %s"),
>                                  vm->def->os.loader->nvram);
>             goto cleanup;
>         }
>     }
> 
> Here "vm->def->os.loader->nvram" evaluates to NULL:
> 
>   6468        if (!virDomainObjIsActive(vm) &&
>   6469            vm->def->os.loader && vm->def->os.loader->nvram &&
>   6470            virFileExists(vm->def->os.loader->nvram)) {
>   6471            if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) {
>   6472                virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> 
>   (gdb) print vm->def->os.loader
>   $1 = (virDomainLoaderDefPtr) 0x7ff59c00bf20
> 
>   (gdb) print vm->def->os.loader->nvram
>   $2 = 0x0
> 
> I thought that the auto-generation of the nvram pathname (internal to
> libvirtd, ie. not visible in the XML file) would occur on the undefine
> path as well. Apparently this is not the case.
> 
> Indeed, the only call to qemuPrepareNVRAM() is from qemuProcessStart().
> 
> Michal, would it be possible generate the *pathname* of the separate
> varstore in qemuDomainUndefineFlags() too?
> 
> Please see the second attached patch; it works for me. (This patch is
> best looked at with "git diff -b" (or "git show -b").)
> 

Thanks for the analysis Laszlo.

Things worked fine for me originally... but now that I've played with this
some more, there is definitely some funkiness here at the libvirt level that
explains it.

When the VM is defined, its XML looks like this:

  <os>
    <type arch='x86_64' machine='pc-i440fx-2.1'>hvm</type>
    <loader readonly='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader>
    <boot dev='network'/>
  </os>

Which is fine. After the first time the VM starts, the XML looks like:

  <os>
    <type arch='x86_64' machine='pc-i440fx-2.1'>hvm</type>
    <loader readonly='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader>
    <nvram>/var/lib/libvirt/qemu/nvram/ovmf_VARS.fd</nvram>
    <boot dev='network'/>
  </os>

The generated nvram path is in the XML regardless of VM state from that point
forward. Seems fine.

However, when libvirtd is restarted, the generated nvram path is gone. So some
part of the process is not actually saving the path to disk. That seems
problematic.

I say that at define time, if there's a template, we should do the copying
then and hardcode the nvram path before even initially saving the XML to disk.
Then it's safely hardcoded forever. That's similar to the qemu-ga socket path
auto generation, where libvirt will generate a path for us in the proper
location, it seems to be hardcoded at define time (though it doesn't create
the socket).

(also, another issue I just noticed... libvirt tries to use
/var/lib/libvirt...nvram path if connected to qemu:///session, probably wants
whatever the typical path under $HOME is)

> 
> (3) I just realized that a domain's name can change during its lifetime.
> Renaming a domain becomes a problem when the varstore's pathname is
> deduced from the domain's name. For this reason, the auto-generation
> should use the domain's UUID (which never changes). Michal, what do you
> think of the 3rd attached patch?
> 
> 
> I can resubmit these as standalone patches / series as well (the first
> to virt-tools-list, and the last two to libvir-list), if that's
> necessary.
> 

Thanks for the virt-manager patch, I've applied it now.

- Cole