Re: [libvirt PATCH 3/3] conf: domain: sev: Make 'cbitpos' & 'reducedPhysBits' attrs optional

[Daniel Henrique Barboza <danielhb413@xxxxxxxxx>]

On 10/9/20 11:13 AM, Erik Skultety wrote:
> These XML attributes have been mandatory since the introduction of SEV
> support to libvirt. This design decision was based on QEMU's
> requirement for these to be mandatory for migration purposes, as
> differences in these values across platforms must result in the
> pre-migration checks failing (not that migration with SEV works at the
> time of this patch).
>
> Expecting the user to specify these is cumbersome and the same XML
> cannot be re-used across different revisions of SEV. Since
> we have SEV platform information saved in QEMU capabilities, we can
> make the attributes optional and should fill them in automatically
> in the QEMU driver right before starting it.
>
> Resolves: https://gitlab.com/libvirt/libvirt/-/issues/57
>
> Signed-off-by: Erik Skultety <eskultet@xxxxxxxxxx>
> ---


Already considering that changed that was squashed in later:

Reviewed-by: Daniel Henrique Barboza <danielhb413@xxxxxxxxx>



>   docs/schemas/domaincommon.rng                 | 16 ++++---
>   src/conf/domain_conf.c                        | 46 ++++++++++++-------
>   ...v-missing-platform-info.x86_64-2.12.0.args | 37 +++++++++++++++
>   ...nch-security-sev-missing-platform-info.xml | 35 ++++++++++++++
>   tests/qemuxml2argvtest.c                      |  1 +
>   5 files changed, 113 insertions(+), 22 deletions(-)
>   create mode 100644 tests/qemuxml2argvdata/launch-security-sev-missing-platform-info.x86_64-2.12.0.args
>   create mode 100644 tests/qemuxml2argvdata/launch-security-sev-missing-platform-info.xml
>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 7d4b105981..9963fad4a6 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -467,12 +467,16 @@
>           <value>sev</value>
>         </attribute>
>         <interleave>
> -        <element name="cbitpos">
> -          <data type="unsignedInt"/>
> -        </element>
> -        <element name="reducedPhysBits">
> -          <data type="unsignedInt"/>
> -        </element>
> +        <optional>
> +          <element name="cbitpos">
> +            <data type="unsignedInt"/>
> +          </element>
> +        </optional>
> +        <optional>
> +          <element name="reducedPhysBits">
> +            <data type="unsignedInt"/>
> +          </element>
> +        </optional>
>           <element name="policy">
>             <ref name="hexuint"/>
>           </element>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 51efeb0e42..648a47ac84 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -16756,6 +16756,7 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode,
>       virDomainSEVDefPtr def;
>       unsigned long policy;
>       g_autofree char *type = NULL;
> +    int rc = -1;
>   
>       def = g_new0(virDomainSEVDef, 1);
>   
> @@ -16780,25 +16781,35 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode,
>           goto error;
>       }
>   
> -    if (virXPathUInt("string(./cbitpos)", ctxt, &def->cbitpos) < 0) {
> -        virReportError(VIR_ERR_XML_ERROR, "%s",
> -                       _("failed to get launch security cbitpos"));
> -        goto error;
> -    }
> -
> -    if (virXPathUInt("string(./reducedPhysBits)", ctxt,
> -                     &def->reduced_phys_bits) < 0) {
> -        virReportError(VIR_ERR_XML_ERROR, "%s",
> -                       _("failed to get launch security reduced-phys-bits"));
> -        goto error;
> -    }
> -
>       if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0) {
>           virReportError(VIR_ERR_XML_ERROR, "%s",
>                          _("failed to get launch security policy"));
>           goto error;
>       }
>   
> +    /* the following attributes are platform dependent and if missing, we can
> +     * autofill them from domain capabilities later
> +     */
> +    rc = virXPathUInt("string(./cbitpos)", ctxt, &def->cbitpos);
> +    if (rc == 0) {
> +        def->haveCbitpos = VIR_TRISTATE_BOOL_YES;
> +    } else if (rc == -2) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("Invalid format for launch security cbitpos"));
> +        goto error;
> +    }
> +
> +    rc = virXPathUInt("string(./reducedPhysBits)", ctxt,
> +                      &def->reduced_phys_bits);
> +    if (rc == 0) {
> +        def->haveReducedPhysBits = VIR_TRISTATE_BOOL_YES;
> +    } else if (rc == -2) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("Invalid format for launch security "
> +                         "reduced-phys-bits"));
> +        goto error;
> +    }
> +
>       def->policy = policy;
>       def->dh_cert = virXPathString("string(./dhCert)", ctxt);
>       def->session = virXPathString("string(./session)", ctxt);
> @@ -28937,9 +28948,12 @@ virDomainSEVDefFormat(virBufferPtr buf, virDomainSEVDefPtr sev)
>                         virDomainLaunchSecurityTypeToString(sev->sectype));
>       virBufferAdjustIndent(buf, 2);
>   
> -    virBufferAsprintf(buf, "<cbitpos>%d</cbitpos>\n", sev->cbitpos);
> -    virBufferAsprintf(buf, "<reducedPhysBits>%d</reducedPhysBits>\n",
> -                      sev->reduced_phys_bits);
> +    if (sev->haveCbitpos)
> +        virBufferAsprintf(buf, "<cbitpos>%d</cbitpos>\n", sev->cbitpos);
> +
> +    if (sev->haveReducedPhysBits)
> +        virBufferAsprintf(buf, "<reducedPhysBits>%d</reducedPhysBits>\n",
> +                          sev->reduced_phys_bits);
>       virBufferAsprintf(buf, "<policy>0x%04x</policy>\n", sev->policy);
>       if (sev->dh_cert)
>           virBufferEscapeString(buf, "<dhCert>%s</dhCert>\n", sev->dh_cert);
> diff --git a/tests/qemuxml2argvdata/launch-security-sev-missing-platform-info.x86_64-2.12.0.args b/tests/qemuxml2argvdata/launch-security-sev-missing-platform-info.x86_64-2.12.0.args
> new file mode 100644
> index 0000000000..378c3b681c
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/launch-security-sev-missing-platform-info.x86_64-2.12.0.args
> @@ -0,0 +1,37 @@
> +LC_ALL=C \
> +PATH=/bin \
> +HOME=/tmp/lib/domain--1-QEMUGuest1 \
> +USER=test \
> +LOGNAME=test \
> +XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \
> +XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \
> +XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \
> +QEMU_AUDIO_DRV=none \
> +/usr/bin/qemu-system-x86_64 \
> +-name guest=QEMUGuest1,debug-threads=on \
> +-S \
> +-object secret,id=masterKey0,format=raw,\
> +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \
> +-machine pc-1.0,accel=kvm,usb=off,dump-guest-core=off,memory-encryption=sev0 \
> +-m 214 \
> +-realtime mlock=off \
> +-smp 1,sockets=1,cores=1,threads=1 \
> +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
> +-display none \
> +-no-user-config \
> +-nodefaults \
> +-chardev socket,id=charmonitor,fd=1729,server,nowait \
> +-mon chardev=charmonitor,id=monitor,mode=control \
> +-rtc base=utc \
> +-no-shutdown \
> +-no-acpi \
> +-boot strict=on \
> +-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \
> +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
> +-device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \
> +-object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=1,policy=0x1,\
> +dh-cert-file=/tmp/lib/domain--1-QEMUGuest1/dh_cert.base64,\
> +session-file=/tmp/lib/domain--1-QEMUGuest1/session.base64 \
> +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
> +resourcecontrol=deny \
> +-msg timestamp=on
> diff --git a/tests/qemuxml2argvdata/launch-security-sev-missing-platform-info.xml b/tests/qemuxml2argvdata/launch-security-sev-missing-platform-info.xml
> new file mode 100644
> index 0000000000..41ec4cb872
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/launch-security-sev-missing-platform-info.xml
> @@ -0,0 +1,35 @@
> +<domain type='kvm'>
> +  <name>QEMUGuest1</name>
> +  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> +  <memory unit='KiB'>219100</memory>
> +  <currentMemory unit='KiB'>219100</currentMemory>
> +  <vcpu placement='static'>1</vcpu>
> +  <os>
> +    <type arch='x86_64' machine='pc-1.0'>hvm</type>
> +    <boot dev='hd'/>
> +  </os>
> +  <clock offset='utc'/>
> +  <on_poweroff>destroy</on_poweroff>
> +  <on_reboot>restart</on_reboot>
> +  <on_crash>destroy</on_crash>
> +  <devices>
> +    <emulator>/usr/bin/qemu-system-x86_64</emulator>
> +    <disk type='block' device='disk'>
> +      <driver name='qemu' type='raw'/>
> +      <source dev='/dev/HostVG/QEMUGuest1'/>
> +      <target dev='hda' bus='ide'/>
> +      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
> +    </disk>
> +    <controller type='usb' index='0'/>
> +    <controller type='ide' index='0'/>
> +    <controller type='pci' index='0' model='pci-root'/>
> +    <input type='mouse' bus='ps2'/>
> +    <input type='keyboard' bus='ps2'/>
> +    <memballoon model='none'/>
> +  </devices>
> +  <launchSecurity type='sev'>
> +    <policy>0x0001</policy>
> +    <dhCert>AQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAA</dhCert>
> +    <session>IHAVENOIDEABUTJUSTPROVIDINGASTRING</session>
> +  </launchSecurity>
> +</domain>
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 49567326d4..56247177e1 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -3314,6 +3314,7 @@ mymain(void)
>       DO_TEST_CAPS_ARCH_LATEST("vhost-vsock-ccw-auto", "s390x");
>   
>       DO_TEST_CAPS_VER("launch-security-sev", "2.12.0");
> +    DO_TEST_CAPS_VER("launch-security-sev-missing-platform-info", "2.12.0");
>   
>       DO_TEST_CAPS_LATEST("vhost-user-fs-fd-memory");
>       DO_TEST_CAPS_LATEST("vhost-user-fs-hugepages");