Re: [virt-install PATCH 4/7] virtinst: guest: Fill in SEV platform specific data automatically

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Virtualization]     [KVM Development]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]     [Video 4 Linux]

  Powered by Linux