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

[Erik Skultety]

On Thu, Jun 06, 2019 at 04:54:33PM -0400, Cole Robinson wrote:
> 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

That would be preferable, but that's not the case, QEMU doesn't complain but
SeaBIOS also doesn't boot the system when I leave the rest of the config
untouched.

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

Yeah, I'll move it there.

Thanks,
Erik