[virt-tools-list] [virt-manager PATCH 6/8] domcapabilities: introduce get_cpu_security_features

[Cole Robinson]

On 3/15/19 2:24 PM, Cole Robinson wrote:
> On 3/15/19 12:41 PM, Pavel Hrdina wrote:
>> Get all CPU security features that we should enable for guests.
>>
>> In order to do that we need to get CPU definition from domain
>> capabilities and modify the XML so it is in required format for
>> libvirt CPU baseline APIs.  We will prefer the baselineHypervisorCPU
>> API because that considers what QEMU actually supports and we will
>> fallback to baselineCPU API if the better one is not supported by
>> libvirt.
>>
>> This way we can figure out which of the security features are actually
>> available on that specific host for that specific QEMU binary.
>>
>> Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
>> ---
>>   virtinst/domcapabilities.py | 43 +++++++++++++++++++++++++++++++++++++
>>   virtinst/support.py         |  4 ++++
>>   2 files changed, 47 insertions(+)
>>
>> diff --git a/virtinst/domcapabilities.py b/virtinst/domcapabilities.py
>> index deef9e6a..7576472d 100644
>> --- a/virtinst/domcapabilities.py
>> +++ b/virtinst/domcapabilities.py
>> @@ -8,7 +8,11 @@
>>   import logging
>>   import re
>> +import xml.etree.ElementTree as ET
>> +import libvirt
>> +
>> +from .domain import DomainCpu
>>   from .xmlbuilder import XMLBuilder, XMLChildProperty, XMLProperty
>> @@ -243,6 +247,45 @@ class DomainCapabilities(XMLBuilder):
>>           return models
>> +    def _convert_mode_to_cpu(self, xml):
>> +        root = ET.fromstring(xml)
>> +        root.tag = "cpu"
>> +        root.attrib = None
>> +        arch = ET.SubElement(root, "arch")
>> +        arch.text = self.arch
>> +        return ET.tostring(root, encoding="unicode")
>> +
>> +    def get_cpu_security_features(self):
>> +        sec_features = [
>> +                'pcid',
>> +                'spec-ctrl',
>> +                'ssbd',
>> +                'pdpe1gb',
>> +                'ibpb',
>> +                'virt-ssbd',
>> +                'amd-ssbd',
>> +                'amd-no-ssb']
>> +
>> +        features = []
>> +
>> +        for m in self.cpu.modes:
>> +            if m.name == "host-model":
> 
> If you check for != "host-model" you can 'continue' and save a bunch of 
> indentation
> 
> Adding new API calls for the default virt-install invocation makes me a 
> bit nervous, so let's be on the safe side, I suggest moving everything 
> up to the 'cpu' building into its own function, then wrap the call in 
> try/except to make errors non-fatal. Add a logging.warn call when it 
> fails so if virt-install hits this the user is notified and it's more 
> likely we will get a bug report.
> 
>> +                cpuXML = self._convert_mode_to_cpu(m.get_xml())
> 
> Please log the generated XML here, something like 'CPU XML for security 
> flag baseline: %s
> 
>> +                if 
>> self.conn.check_support(self.conn.SUPPORT_CONN_HYPERVISOR_BASELINE):
>> +                    expandedXML = self.conn.baselineHypervisorCPU(
>> +                            self.path, self.arch, self.machine, 
>> self.domain, [cpuXML],
>> +                            
>> libvirt.VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES)
>> +                else:
>> +                    expandedXML = self.conn.baselineCPU([cpuXML],
>> +                            
>> libvirt.VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES)
> 
> And log the expandedXML here too
> 
>> +                cpu = DomainCpu(self.conn, expandedXML) > +
>> +                for feature in cpu.features:
>> +                    if feature.name in sec_features:
>> +                        features.append(feature.name)
>> +
>> +        return features
>> +
>>       XML_NAME = "domainCapabilities"
>>       os = XMLChildProperty(_OS, is_single=True)
>> diff --git a/virtinst/support.py b/virtinst/support.py
>> index 7ace3ef6..077be426 100644
>> --- a/virtinst/support.py
>> +++ b/virtinst/support.py
>> @@ -278,6 +278,10 @@ SUPPORT_CONN_DISK_DRIVER_NAME_QEMU = _make(
>>       hv_version={"qemu": 0, "xen": "4.2.0"},
>>       hv_libvirt_version={"qemu": 0, "xen": "1.1.0"})
>> +SUPPORT_CONN_HYPERVISOR_BASELINE = _make(
>> +    version="4.4.0",
>> +    hv_libvirt_version={"test": 0})
>> +
>>
> 
> I think this should be hv_libvirt_version={"qemu", 0}, meaning: assume 
> support if libvirt version >= 4.4.0, driver=qemu, qemu driver version > 
> 0. The test driver doesn't appear to implement this API
> 

Actually I suspect because of the hackery we do with our unit test suite 
this will cause issues: most test cases will think they are 'qemu' in 
this instance, but of course we are actually interacting with the 'test' 
driver, which doesn't implement the API, so that 'qemu' version check 
will eventually cause us to raise an error. I think it's better to drop 
the support.py check, unconditionally try the 
self.conn.baselineHypervisorCPU, and if it throws an error try the older 
baseline call instead.

- Cole