[virt-tools-list] [PATCH] virtinst: Pass Xen machine type to libvirt getDomainCapabilities

[Cole Robinson]

On 06/15/2016 11:40 AM, Charles Arnold wrote:
>>>> On 6/15/2016 at 09:21 AM, Pavel Hrdina <phrdina at redhat.com> wrote: 
>> On Wed, Jun 15, 2016 at 08:31:42AM -0600, Charles Arnold wrote:
>>> Tell libvirt what machine type the user chose for Xen (PV or HVM).
>>> Without a type specified, the default is to return the capabilities of a pv
>>> machine. Passing "xenfv" will allow the "Firmware" option to show up
>>> under "Hypervisor Details" when a Xen HVM guest install is being customized.
>>> Also specify the name of the SUSE aavmf firmware for aarch64.
>>>
>>> diff --git a/virtinst/domcapabilities.py b/virtinst/domcapabilities.py
>>> index 874fa1e..605d77a 100644
>>> --- a/virtinst/domcapabilities.py
>>> +++ b/virtinst/domcapabilities.py
>>> @@ -78,13 +78,20 @@ class _Features(_CapsBlock):
>>>  
>>>  class DomainCapabilities(XMLBuilder):
>>>      @staticmethod
>>> -    def build_from_params(conn, emulator, arch, machine, hvtype):
>>> +    def build_from_params(conn, emulator, arch, machine, hvtype, os_type):
>>>          xml = None
>>>          if conn.check_support(
>>>              conn.SUPPORT_CONN_DOMAIN_CAPABILITIES):
>>> +            machine_type = machine
>>> +            # For Xen capabilities pass either xenpv or xenfv
>>> +            if hvtype == "xen":
>>> +                if os_type == "hvm":
>>> +                    machine_type = "xenfv"
>>> +                else:
>>> +                    machine_type = "xenpv"
>>
>> Hi Charles
>>
>> I'm confused about this change, there is no need to do something like this.
>>
>> virt-install creates a correct XML if you ask for it.  Please check man page 
>> for
>> virt-install, there are two options, --hvm and --paravirt.  If you don't 
>> specify
>> any of them, virt-install creates a PV guest as default.
> 
> This is tested via the installation wizard GUI. If you select (fullvirt) on the
> "Xen Type:" pop-down the machine type is not passed along to this
> libvirt call to get the capabilities. Without the machine type you
> can't customize the install and choose UEFI (along with ovmf) to boot the
> VM. See also upstream fixes to libvirt to support this at
> https://www.redhat.com/archives/libvir-list/2016-June/msg00748.html

Wrong link? That's a discussion about tar formats

We shouldn't need to use a heuristic here, the machine type should be coming
from the domain XML that virt-manager builds... though I don't think we encode
one by default for xen but instead let the xen driver fill it in for us. Can
you link to the correct thread, and give the 'virsh capabilities' output for
your xen connection?

> 
> Here is an example of the resulting XML that is desired,
>   <os>
>     <type arch='x86_64' machine='xenfv'>hvm</type>
>     <loader readonly='yes' type='pflash'>/usr/share/qemu/ovmf-x86_64-ms-code.bin</loader> 
>     <boot dev='hd'/>
>   </os>
> 
> I don't know if this is even configurable using virt-install from the command line.
> 

All of it is available, see 'virt-install --boot help'

>>
>>>              try:
>>>                  xml = conn.getDomainCapabilities(emulator, arch,
>>> -                    machine, hvtype)
>>> +                    machine_type, hvtype)
>>>              except:
>>>                  logging.debug("Error fetching domcapabilities XML",
>>>                      exc_info=True)
>>> @@ -97,7 +104,7 @@ class DomainCapabilities(XMLBuilder):
>>>      @staticmethod
>>>      def build_from_guest(guest):
>>>          return DomainCapabilities.build_from_params(guest.conn,
>>> -            guest.emulator, guest.os.arch, guest.os.machine, guest.type)
>>> +            guest.emulator, guest.os.arch, guest.os.machine, guest.type, 
>> guest.os.os_type)
>>>  
>>>      # Mapping of UEFI binary names to their associated architectures. We
>>>      # only use this info to do things automagically for the user, it 
>> shouldn't
>>> @@ -112,6 +119,7 @@ class DomainCapabilities(XMLBuilder):
>>>          "aarch64": [
>>>              ".*AAVMF_CODE\.fd",  # RHEL
>>>              ".*aarch64/QEMU_EFI.*",  # gerd's firmware repo
>>> +            ".*aavmf-aarch64-.*"  # SUSE
>>>              ".*aarch64.*",  # generic attempt at a catchall
>>>          ],
>>>      }
>>
>> This hunk should be a separate patch because it's unrelated to the rest of 
>> the
>> patch.  Please send this as a separate patch and also if possible provide 
>> some
>> source where we can validate the naming.
> 
> I'll send this as a separate patch.

Actually this bit isn't strictly required, notice the regex below your added
line will also match that path. But I guess it doesn't hurt to explicitly
document the various distro paths, so its up to you

- Cole