[virt-tools-list] [virt-install PATCH 2/7] virtinst: cli: Introduce parser support for SEV launch security

Thu Jun 6 20:26:27 UTC 2019

On 6/6/19 6:00 AM, Erik Skultety wrote:
> Introduce both the launchSecurity XML and parser classes. While at it,
> add launchSecurity as a property instance to the Guest class too.
> 
> The parser requires the 'type' argument to be mandatory since in the
> future it will determine different code paths, therefore
> '--launch-security foo=bar' is incorrect.
> 

Is there anything spec'd out what other type= XML will look like? I'm
curious

> Signed-off-by: Erik Skultety <eskultet at redhat.com>
> ---
>  ...nstall-x86_64-launch-security-sev-full.xml | 63 +++++++++++++++++++
>  tests/clitest.py                              |  5 ++
>  virtinst/cli.py                               | 39 ++++++++++++
>  virtinst/domain/__init__.py                   |  1 +
>  virtinst/domain/launch_security.py            | 24 +++++++
>  virtinst/guest.py                             |  3 +-
>  6 files changed, 134 insertions(+), 1 deletion(-)
>  create mode 100644 tests/cli-test-xml/compare/virt-install-x86_64-launch-security-sev-full.xml
>  create mode 100644 virtinst/domain/launch_security.py
> 
> diff --git a/tests/cli-test-xml/compare/virt-install-x86_64-launch-security-sev-full.xml b/tests/cli-test-xml/compare/virt-install-x86_64-launch-security-sev-full.xml
> new file mode 100644
> index 00000000..5b87a621
> --- /dev/null
> +++ b/tests/cli-test-xml/compare/virt-install-x86_64-launch-security-sev-full.xml
> @@ -0,0 +1,63 @@
> +<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>
> +    <session>BASE64SESSION</session>
> +    <dhCert>BASE64CERT</dhCert>
> +  </launchSecurity>
> +</domain>
> diff --git a/tests/clitest.py b/tests/clitest.py
> index 01f76b8a..c0efabf7 100644
> --- a/tests/clitest.py
> +++ b/tests/clitest.py
> @@ -874,6 +874,11 @@ c.add_invalid("--nodisks --boot network --arch mips --virt-type kvm")  # Invalid
>  c.add_invalid("--nodisks --boot network --paravirt --arch mips")  # Invalid arch/virt combo
>  c.add_invalid("--disk none --location nfs:example.com/fake --nonetworks")  # Using --location nfs, no longer supported
>  
> +
> +c = vinst.add_category("kvm-x86_64-launch-security", "--disk none --noautoconsole")

The surrounding code isn't consistent here but please add two newlines
between each category. You can fold the duplicate --connect bit into the
second option of add_category

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

Is the --boot uefi bit important? if not, I suggest folding the
boilerplate options into add_category so you just test --launch-security
invocations here. You'll need an install option so --pxe should be fine.

> +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)
>  c.add_compare("--boot uefi --disk none", "boot-uefi")
>  
> diff --git a/virtinst/cli.py b/virtinst/cli.py
> index 7ba6b211..0a76b074 100644
> --- a/virtinst/cli.py
> +++ b/virtinst/cli.py
> @@ -827,6 +827,12 @@ def add_guest_xml_options(geng):
>                 "--qemu-commandline='-display gtk,gl=on'\n"
>                 "--qemu-commandline env=DISPLAY=:0.1"))
>  
> +    ParserLaunchSecurity.register()
> +    geng.add_argument("--launch-security", action="append",
> +        help=_("Configure VM launch security (e.g. SEV memory encryption). Ex:\n"
> +               "--launch-security type=sev,cbitpos=47,reduced_phys_bits=1,policy=0x0001,dh_cert=BASE64CERT\n"
> +               "--launch-security sev"))
> +
> 

We are trying to have virt-install options mirror XML naming, so this
should be --launchSecurity. We can have --launchsecurity too like

add_argument("--launchSecurity", "--launchsecurity", ...)

>  def add_boot_options(insg):
>      ParserBoot.register()
> @@ -3832,6 +3838,39 @@ class ParserHostdev(VirtCLIParser):
>          cls.add_arg("rom.bar", "rom_bar", is_onoff=True)
>  
>  
> +#############################
> +# --launch-security parsing #
> +#############################
> +
> +class ParserLaunchSecurity(VirtCLIParser):
> +    cli_arg_name = "launch_security"
> +    guest_propname = "launchSecurity"
> +    remove_first = "type"
> +
> +    @classmethod
> +    def _init_class(cls, **kwargs):
> +        VirtCLIParser._init_class(**kwargs)
> +        cls.add_arg("type", "type_")
> +        cls.add_arg("cbitpos", "cbitpos")
> +        cls.add_arg("reduced_phys_bits", "reduced_phys_bits")
> +        cls.add_arg("policy", "policy")
> +        cls.add_arg("session", "session")
> +        cls.add_arg("dh_cert", "dh_cert")
> +

Similarly the suboptions should match XML naming. So the first opts
should be reducedPhysBits and dhCert, others are fine. Internal naming
doesn't matter but I have a slight preference for matching XML naming,
though the codebase isn't remotely consistent on that so it's up to you

> +    def _check_required_opts(self):
> +        type_ = self.optdict.get("type", None)
> +
> +        if not type_:
> +            fail(_("Missing mandatory attribute 'type' for --launch-security"))
> +

Generally we don't want this style of validation in the cli parser
because it's tricky to get right. In this case it will break using
virt-xml to edit a single non-type --launchSecurity field which should
be a valid thing to do.

You'll want to move this validation into a 'def validate' function in
the launch security object and the virt-install cli parser will call it
by default.

> +    def _parse(self, inst):
> +        if not inst.conn.is_qemu():
> +            logging.warning("--launch-security is only supported with QEMU")
> +

Generally I'm not a fan of these types of bits in the cli parser, and
virt-manager in general. If libvirt supports sev for xen tomorrow then
this message is now out of date. If libvirt/qemu combo is the only one
that supports this, then we either let libvirt reject it for us
(preferred), or we use domaincapabilities content to validate that sev
is available and error otherwise. But if the latter bit is the only
option then I think it's better just to not worry about it until real
users complain about it

> +        self._check_required_opts()> +        return super()._parse(inst)
> +
> +
>  ###########################
>  # Public virt parser APIs #
>  ###########################
> diff --git a/virtinst/domain/__init__.py b/virtinst/domain/__init__.py
> index f942ee59..b7157c9c 100644
> --- a/virtinst/domain/__init__.py
> +++ b/virtinst/domain/__init__.py
> @@ -19,5 +19,6 @@ from .seclabel import DomainSeclabel
>  from .sysinfo import DomainSysinfo
>  from .vcpus import DomainVCPUs
>  from .xmlnsqemu import DomainXMLNSQemu
> +from .launch_security import DomainLaunchSecurity
>  
>  __all__ = [l for l in locals() if l.startswith("Domain")]
> diff --git a/virtinst/domain/launch_security.py b/virtinst/domain/launch_security.py
> new file mode 100644
> index 00000000..a911712c
> --- /dev/null
> +++ b/virtinst/domain/launch_security.py
> @@ -0,0 +1,24 @@
> +from ..xmlbuilder import XMLBuilder, XMLProperty
> +
> +
> +class DomainLaunchSecurity(XMLBuilder):
> +    """
> +    Class for generating <launchSecurity> XML element
> +    """
> +
> +    XML_NAME = "launchSecurity"
> +    _XML_PROP_ORDER = ["type_", "cbitpos", "reduced_phys_bits", "policy",
> +            "session", "dh_cert"]
> +
> +    type_ = XMLProperty("./@type")

Why the funny naming? if it's to avoid pylint complaining about
collision with builtin type, I prefer supressing that error

Thanks,
Cole