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
Thanks,
Cole
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
