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.
>
> > +
> > #############################
> > # 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
> 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
