On 6/6/19 4:51 PM, 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
>
>> 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
>
>> +
>> #############################
>> # 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
> bool(self.features.sev.supported)
>
>> XML_NAME = "domainCapabilities"
>> os = XMLChildProperty(_OS, is_single=True)
>> diff --git a/virtinst/guest.py b/virtinst/guest.py
>> index 474afc7a..2477d73a 100644
>> --- a/virtinst/guest.py
>> +++ b/virtinst/guest.py
>> @@ -666,6 +666,7 @@ class Guest(XMLBuilder):
>>
>> self._add_implied_controllers()
>> self._add_spice_devices()
>> + self._set_default_launch_security()
>>
>>
>> ########################
>> @@ -952,3 +953,9 @@ class Guest(XMLBuilder):
>> self._add_spice_channels()
>> self._add_spice_sound()
>> self._add_spice_usbredir()
>> +
>> + def _set_default_launch_security(self):
>> + if not self.launchSecurity.enabled():
>> + return
>> +
>> + return self.launchSecurity.set_defaults(self)
>>
>
> The .enabled() check isn't required because set_defaults() already
> protects itself from any changes with the is_sev() So you can kill this
> set_default_launch_security function, move this last line with the other
> singleton default setting, after self.os.set_defaults call
>
Also another comment about the general idea. If the idea is just to
encode values in the XML because the host might change in the future and
we need those values stored for migration compat, that seems exactly the
type of thing that libvirt should do for us if the values are left
unspecified. So similar comment applies as to the policy= value
Thanks,
Cole
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
