[virt-tools-list] [virt-install PATCH 5/7] virtins: guest: Provide further SEV support checks

[Cole Robinson]

On 6/6/19 6:00 AM, Erik Skultety wrote:
> These include platform checks - libvirt & QEMU - as well as
> configuration - SEV is only supported with UEFI.
> Another configuration requirement made in this patch is Q35 machine,
> since ADM recommends Q35 in their setups even though SEV can work with
> the legacy PC machine type, but we'd have to turn on
> virtio-non-transitional for all virtio devices with some other potential
> pitfalls along the way.
> 
> Signed-off-by: Erik Skultety <eskultet at redhat.com>
> ---
>  tests/clitest.py            |  3 +++
>  virtinst/domcapabilities.py |  1 +
>  virtinst/guest.py           | 15 +++++++++++++++
>  3 files changed, 19 insertions(+)
> 
> diff --git a/tests/clitest.py b/tests/clitest.py
> index a20836ee..29eb78b7 100644
> --- a/tests/clitest.py
> +++ b/tests/clitest.py
> @@ -880,6 +880,9 @@ c.add_compare("--boot uefi --machine q35 --launch-security type=sev,reduced_phys
>  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.add_invalid("--launch-security sev --connect " + utils.URIs.kvm_amd_q35)  # Fail if loader isn't UEFI
> +c.add_invalid("--boot uefi --launch-security sev --connect " + utils.URIs.kvm_amd_q35)  # Fail if machine type isn't Q35
> +c.add_invalid("--boot uefi --machine q35 --launch-security sev,policy=0x0001 --connect " + utils.URIs.kvm_q35)  # Fail with no SEV capabilities
>  
>  c = vinst.add_category("kvm-q35", "--noautoconsole --connect " + utils.URIs.kvm_q35)
>  c.add_compare("--boot uefi --disk none", "boot-uefi")
> diff --git a/virtinst/domcapabilities.py b/virtinst/domcapabilities.py
> index 328e8d2d..3b047fdc 100644
> --- a/virtinst/domcapabilities.py
> +++ b/virtinst/domcapabilities.py
> @@ -77,6 +77,7 @@ def _make_capsblock(xml_root_name):
>  
>  class _SEV(XMLBuilder):
>      XML_NAME = "sev"
> +    supported = XMLProperty("./@supported", is_yesno=True)
>      cbitpos = XMLProperty("./cbitpos", is_int=True)
>      reduced_phys_bits = XMLProperty("./reducedPhysBits", is_int=True)
>  
> diff --git a/virtinst/guest.py b/virtinst/guest.py
> index 2477d73a..9e566823 100644
> --- a/virtinst/guest.py
> +++ b/virtinst/guest.py
> @@ -954,8 +954,23 @@ class Guest(XMLBuilder):
>          self._add_spice_sound()
>          self._add_spice_usbredir()
>  
> +    def _check_launch_security_sev(self):
> +        # SeaBIOS doesn't have support for SEV. Q35 defaults to virtio 1.0,
> +        # which we need so let's not go through the 'virtio-transitional'
> +        # exercise for pc-i440fx to make SEV work, AMD recommends Q35 anyway
> +        if self.os.machine != "q35" or self.os.loader_type != "pflash":
> +            raise RuntimeError(_("SEV launch security requires a Q35 UEFI machine"))
> +

Oh interesting so --boot uefi is required. Does qemu or libvirt validate
this? If so I say drop it. But if it just silently fails or silently
doesn't work then we should validate it, although something lower in the
stack should make it explicitly fail as well

> +        # libvirt or QEMU might not support SEV
> +        domcaps = self.lookup_domcaps()
> +        if not domcaps.supports_sev_launch_security():
> +            raise RuntimeError(_("SEV launch security is not supported on this platform"))
> +

Hmm. The logical place to put this would be validate() in
launchSecurity, but validate doesn't take a guest instance. it probably
should but that's a problem for another day. Instead just stick this in
the set_defaults_sev function

Thanks,
Cole

>      def _set_default_launch_security(self):
>          if not self.launchSecurity.enabled():
>              return
>  
> +        if self.launchSecurity.is_sev():
> +            self._check_launch_security_sev()
> +
>          return self.launchSecurity.set_defaults(self)
>