Re: [PATCH v1 3/3] i386/sev: Add KVM_EXIT_SNP_REQ_CERTS support for certificate-fetching

[Markus Armbruster <armbru@redhat.com>]

Michael Roth <michael.roth@amd.com> writes:

> The GHCB specification[1] defines a VMGEXIT-based Guest Request
> hypercall to allow an SNP guest to issue encrypted requests directly to
> SNP firmware to do things like query the attestation report for the
> guest. These are generally handled purely in the kernel.
>
> In some some cases, it's useful for the host to be able to additionally
> supply the certificate chain for the signing key that SNP firmware uses
> to sign these attestation reports. To allow for this, the GHCB
> specification defines an Extended Guest Request where this certificate
> data can be provided in a special format described in the GHCB spec.
> This certificate data may be global or guest-specific depending on how
> the guest was configured. Rather than providing interfaces to manage
> these within the kernel, KVM provides a new KVM_EXIT_SNP_REQ_CERTS exit
> to request the certificate contents from userspace. Implement support
> for that here.
>
> To synchronize delivery of the certificates to the guest in a way where
> they will not be rendered invalid by updates to SNP firmware or
> attestation singing/endorsement keys by management tools outside the
> purview of QEMU, it is expected by users of KVM_EXIT_SNP_REQ_CERTS to
> obtain a shared/read lock on the certificate file prior to delivering
> them back to KVM. Only after this will the attestation report be
> retrieved from firmware and bundled with the certificate data, so QEMU
> must continue to hold the file lock until KVM confirms that the
> attestation report has been retrieved/bundled. This confirmation is done
> by way of the kvm_immediate_exit callback infrastructure that was
> introduced in a previous patch.

The "management tools outside the purview of QEMU" will all obtain the
same kind of file lock?

> [1] "Guest Hypervisor Communication Block (GHCB) Standardization",
>     https://www.amd.com/en/developer/sev.html
>
> Signed-off-by: Michael Roth <michael.roth@amd.com>
> ---
>  qapi/qom.json                 |  23 +++-
>  target/i386/kvm/kvm.c         |  10 ++
>  target/i386/sev-sysemu-stub.c |   5 +
>  target/i386/sev.c             | 249 ++++++++++++++++++++++++++++++++++
>  target/i386/sev.h             |   2 +
>  5 files changed, 288 insertions(+), 1 deletion(-)
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 28ce24cd8d..6eaf0e7721 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -1034,6 +1034,25 @@
>  #     firmware.  Set this to true to disable the use of VCEK.
>  #     (default: false) (since: 9.1)
>  #
> +# @certs-path: Path to certificate data that can be passed to guests via
> +#              SNP Extended Guest Requests. File should be in the format
> +#              described in the GHCB specification. (default: none)
> +#              (since: 10.0)

I prefer "filename" to "path".  We have many kinds of paths: pathnames
(denoting files), QOM paths (denoting objects), qdev paths, search
paths, ...  With "filename", your readers immediately know what you're
talking about.

SevGuestProperties has a member 'dh-cert-file'.  Whether that's related
to your file I can't tell from its documentation.

> +#
> +# @certs-timeout: Max time in milliseconds to wait to obtain a read lock

Please don't abbreviate "Maximum" here.

Confident millisecond granularity will suffice forever?

> +#                 on the certificate file specified by @certs-path. This
> +#                 is not a cumulative value and only affects how long
> +#                 QEMU waits before returning execution to the vCPU and
> +#                 informing the guest of the timeout, so the guest can
> +#                 still continuing retrying for as long as it likes
> +#                 (which will be about 60 seconds for linux guests at
> +#                 the time of this writing). If the guest-side timeout
> +#                 is insufficient, set this higher to allow more time to
> +#                 fetch the certificate. If the guest-side timeout is
> +#                 sufficient, set this lower to reduce the likelihood of
> +#                 soft lockups in the guest.
> +#                 (default: 100) (since: 10.0)
> +#
>  # Since: 9.1
>  ##

Please format like

   # @certs-path: Path to certificate data that can be passed to guests
   #     via SNP Extended Guest Requests.  File should be in the format
   #     described in the GHCB specification.
   #     (default: none) (since: 10.0)
   #
   # @certs-timeout: Max time in milliseconds to wait to obtain a read
   #     lock on the certificate file specified by @certs-path.  This is
   #     not a cumulative value and only affects how long QEMU waits
   #     before returning execution to the vCPU and informing the guest
   #     of the timeout, so the guest can still continuing retrying for
   #     as long as it likes (which will be about 60 seconds for linux
   #     guests at the time of this writing).  If the guest-side timeout
   #     is insufficient, set this higher to allow more time to fetch the
   #     certificate.  If the guest-side timeout is sufficient, set this
   #     lower to reduce the likelihood of soft lockups in the guest.
   #     (default: 100) (since: 10.0)

to blend in with commit a937b6aa739 (qapi: Reformat doc comments to
conform to current conventions).

>  { 'struct': 'SevSnpGuestProperties',
> @@ -1045,7 +1064,9 @@
>              '*id-auth': 'str',
>              '*author-key-enabled': 'bool',
>              '*host-data': 'str',
> -            '*vcek-disabled': 'bool' } }
> +            '*vcek-disabled': 'bool',
> +            '*certs-path': 'str',
> +            '*certs-timeout': 'uint32' } }
>  
>  ##
>  # @ThreadContextProperties:

[...]