On 6/10/19 9:03 AM, Erik Skultety wrote:
> On Thu, Jun 06, 2019 at 04:51:01PM -0400, Cole Robinson wrote:
>> On 6/6/19 6:00 AM, Erik Skultety wrote:
>>> The data in question are 'cbitpos' denoting which addressing bit is the
>>> encryption bit and 'reduced_phys_bits' denoting how many physical
>>> address space we lose by turning on the encryption. Both of these are
>>> hypervisor dependent and thus will be the same for all the guest
>>> residing on the same host, but need to be specified for future migration
>>> purposes.
>>> But given we can probe them from domain capabilities, we don't need the
>>> user to provide them and thus enhancing cli user experience. This
>>> requires a new _SEV domaincapabilities XML class to be created so that
>>> we can query the specific properties.
>>>
>>> Signed-off-by: Erik Skultety <eskultet@xxxxxxxxxx>
>>> ---
>>> ...irt-install-x86_64-launch-security-sev.xml | 61 +++++++++++++++++++
>>> tests/clitest.py | 3 +-
>>> virtinst/domain/launch_security.py | 13 ++++
>>> virtinst/domcapabilities.py | 21 +++++++
>>> virtinst/guest.py | 7 +++
>>> 5 files changed, 104 insertions(+), 1 deletion(-)
>>> create mode 100644 tests/cli-test-xml/compare/virt-install-x86_64-launch-security-sev.xml
>>>
>>> diff --git a/tests/cli-test-xml/compare/virt-install-x86_64-launch-security-sev.xml b/tests/cli-test-xml/compare/virt-install-x86_64-launch-security-sev.xml
>>> new file mode 100644
>>> index 00000000..0c620654
>>> --- /dev/null
>>> +++ b/tests/cli-test-xml/compare/virt-install-x86_64-launch-security-sev.xml
>>> @@ -0,0 +1,61 @@
>>> +<domain type="kvm">
>>> + <name>foobar</name>
>>> + <uuid>00000000-1111-2222-3333-444444444444</uuid>
>>> + <memory>65536</memory>
>>> + <currentMemory>65536</currentMemory>
>>> + <vcpu>1</vcpu>
>>> + <os>
>>> + <type arch="x86_64" machine="q35">hvm</type>
>>> + <loader readonly="yes" type="pflash">/usr/share/edk2/ovmf/OVMF_CODE.fd</loader>
>>> + <boot dev="hd"/>
>>> + </os>
>>> + <features>
>>> + <acpi/>
>>> + <apic/>
>>> + <vmport state="off"/>
>>> + </features>
>>> + <cpu mode="host-model"/>
>>> + <clock offset="utc">
>>> + <timer name="rtc" tickpolicy="catchup"/>
>>> + <timer name="pit" tickpolicy="delay"/>
>>> + <timer name="hpet" present="no"/>
>>> + </clock>
>>> + <pm>
>>> + <suspend-to-mem enabled="no"/>
>>> + <suspend-to-disk enabled="no"/>
>>> + </pm>
>>> + <devices>
>>> + <emulator>/usr/bin/qemu-kvm</emulator>
>>> + <controller type="usb" index="0" model="ich9-ehci1"/>
>>> + <controller type="usb" index="0" model="ich9-uhci1">
>>> + <master startport="0"/>
>>> + </controller>
>>> + <controller type="usb" index="0" model="ich9-uhci2">
>>> + <master startport="2"/>
>>> + </controller>
>>> + <controller type="usb" index="0" model="ich9-uhci3">
>>> + <master startport="4"/>
>>> + </controller>
>>> + <interface type="bridge">
>>> + <source bridge="eth0"/>
>>> + <mac address="00:11:22:33:44:55"/>
>>> + <model type="e1000e"/>
>>> + </interface>
>>> + <console type="pty"/>
>>> + <input type="tablet" bus="usb"/>
>>> + <graphics type="spice" port="-1" tlsPort="-1" autoport="yes">
>>> + <image compression="off"/>
>>> + </graphics>
>>> + <sound model="ich9"/>
>>> + <video>
>>> + <model type="qxl"/>
>>> + </video>
>>> + <redirdev bus="usb" type="spicevmc"/>
>>> + <redirdev bus="usb" type="spicevmc"/>
>>> + </devices>
>>> + <launchSecurity type="sev">
>>> + <cbitpos>47</cbitpos>
>>> + <reducedPhysBits>1</reducedPhysBits>
>>> + <policy>0x0001</policy>
>>> + </launchSecurity>
>>> +</domain>
>>> diff --git a/tests/clitest.py b/tests/clitest.py
>>> index 3958871e..a20836ee 100644
>>> --- a/tests/clitest.py
>>> +++ b/tests/clitest.py
>>> @@ -877,7 +877,8 @@ c.add_invalid("--disk none --location nfs:example.com/fake --nonetworks") # Usi
>>>
>>> c = vinst.add_category("kvm-x86_64-launch-security", "--disk none --noautoconsole")
>>> c.add_compare("--boot uefi --machine q35 --launch-security type=sev,reduced_phys_bits=1,policy=0x0001,cbitpos=47,dh_cert=BASE64CERT,session=BASE64SESSION --connect " + utils.URIs.kvm_amd_q35, "x86_64-launch-security-sev-full") # Full cmdline
>>> -c.add_valid("--boot uefi --machine q35 --launch-security sev,reduced_phys_bits=1,cbitpos=47 --connect " + utils.URIs.kvm_amd_q35) # Default policy == 0x0003 will be used
>>> +c.add_compare("--boot uefi --machine q35 --launch-security sev,policy=0x0001 --connect " + utils.URIs.kvm_amd_q35, "x86_64-launch-security-sev") # Fill in platform data from domcaps
>>> +c.add_valid("--boot uefi --machine q35 --launch-security sev --connect " + utils.URIs.kvm_amd_q35) # Default policy == 0x0003 will be used
>>> c.add_invalid("--launch-security policy=0x0001 --connect " + utils.URIs.kvm_amd_q35) # Missing launch-security 'type'
>>>
>>> c = vinst.add_category("kvm-q35", "--noautoconsole --connect " + utils.URIs.kvm_q35)
>>> diff --git a/virtinst/domain/launch_security.py b/virtinst/domain/launch_security.py
>>> index a911712c..e4ffc8a2 100644
>>> --- a/virtinst/domain/launch_security.py
>>> +++ b/virtinst/domain/launch_security.py
>>> @@ -22,3 +22,16 @@ class DomainLaunchSecurity(XMLBuilder):
>>>
>>> def is_sev(self):
>>> return self.type_ == "sev"
>>> +
>>> + def _set_defaults_sev(self, guest):
>>> + sev = guest.lookup_domcaps().features.sev
>>> +
>>> + cbitpos, reduced_phys_bits = sev.get_platform_params()> + if self.cbitpos is None:
>>> + self.cbitpos = cbitpos
>>> + if self.reduced_phys_bits is None:
>>> + self.reduced_phys_bits = reduced_phys_bits
>>> +
>>> + def set_defaults(self, guest):
>>> + if self.is_sev():
>>> + return self._set_defaults_sev(guest)
>>
>> Okay I see you already use set_defaults :) Should have read ahead I guess
>
> I'll move policy bit here.
>
>>
>>> diff --git a/virtinst/domcapabilities.py b/virtinst/domcapabilities.py
>>> index acc91f81..328e8d2d 100644
>>> --- a/virtinst/domcapabilities.py
>>> +++ b/virtinst/domcapabilities.py
>>> @@ -71,6 +71,19 @@ def _make_capsblock(xml_root_name):
>>> return TmpClass
>>>
>>>
>>> +################################
>>> +# SEV launch security handling #
>>> +################################
>>> +
>>> +class _SEV(XMLBuilder):
>>> + XML_NAME = "sev"
>>> + cbitpos = XMLProperty("./cbitpos", is_int=True)
>>> + reduced_phys_bits = XMLProperty("./reducedPhysBits", is_int=True)
>>> +
>>> + def get_platform_params(self):
>>> + return (self.cbitpos, self.reduced_phys_bits)
>>> +
>>
>> Is there a reason for this abstraction? Seems okay to me to just access
>> the fields directly in set_defaults_sev
>
> Personally, I don't like the notion of public attributes (there are some
> exceptions), thus I introduced a getter, I feel like having a getter keeps
> things cleaner, but I can drop it if you insist.
>
I'm not insisting, but I think unfolding it makes the calling code more
self describing, we aren't required to come and look at the wrapper
function to understand what values it is returning. The other code in
this file uses accessor style functions to insulate callers from
whatever special logic is needed to arrive at the desired value, like
all_machine_names or is_kvm_available, even the supports_* bits. That
doesn't really apply to this case though
>>
>>> +
>>> #############################
>>> # Misc toplevel XML classes #
>>> #############################
>>> @@ -89,6 +102,7 @@ class _Devices(_CapsBlock):
>>> class _Features(_CapsBlock):
>>> XML_NAME = "features"
>>> gic = XMLChildProperty(_make_capsblock("gic"), is_single=True)
>>> + sev = XMLChildProperty(_SEV, is_single=True)
>>>
>>>
>>> ###############
>>> @@ -305,6 +319,13 @@ class DomainCapabilities(XMLBuilder):
>>>
>>> return self._features
>>>
>>> + def supports_sev_launch_security(self):
>>> + """
>>> + Returns False if either libvirt doesn't advertise support for SEV at
>>> + all (< libvirt-4.5.0) or if it explicitly advertises it as unsupported
>>> + on the platform
>>> + """
>>> + return (self.features.sev and self.features.sev.supported)
>>>
>>
>> This function should be in the next patch. 'self.features.sev' is always
>> going to evaluate to true so you can just do
>
> Is it? Even if capabilities are missing <sev> ? I'd expect self.features.sev to
> be None, if it isn't we need to ensure it is, since we need to detect both
> whether libvirt and QEMU support SEV.
>
> Erik
Yeah it's the way the XMLChildProperty stuff works. features.sev will
always exist, but if the <sev> block is not the in XML, sev.supported
will return None, so the bool(self.features.sev.supported) check will
return False if <sev> is unset, or it's explicitly sev supported=no
Thanks,
Cole
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
