Re: [PATCH v3 22/49] i386/sev: Introduce 'sev-snp-guest' object

[Markus Armbruster <armbru@redhat.com>]

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

> From: Brijesh Singh <brijesh.singh@amd.com>
>
> SEV-SNP support relies on a different set of properties/state than the
> existing 'sev-guest' object. This patch introduces the 'sev-snp-guest'
> object, which can be used to configure an SEV-SNP guest. For example,
> a default-configured SEV-SNP guest with no additional information
> passed in for use with attestation:
>
>   -object sev-snp-guest,id=sev0
>
> or a fully-specified SEV-SNP guest where all spec-defined binary
> blobs are passed in as base64-encoded strings:
>
>   -object sev-snp-guest,id=sev0, \
>     policy=0x30000, \
>     init-flags=0, \
>     id-block=YWFhYWFhYWFhYWFhYWFhCg==, \
>     id-auth=CxHK/OKLkXGn/KpAC7Wl1FSiisWDbGTEKz..., \
>     auth-key-enabled=on, \
>     host-data=LNkCWBRC5CcdGXirbNUV1OrsR28s..., \
>     guest-visible-workarounds=AA==, \
>
> See the QAPI schema updates included in this patch for more usage
> details.
>
> In some cases these blobs may be up to 4096 characters, but this is
> generally well below the default limit for linux hosts where
> command-line sizes are defined by the sysconf-configurable ARG_MAX
> value, which defaults to 2097152 characters for Ubuntu hosts, for
> example.
>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> Co-developed-by: Michael Roth <michael.roth@amd.com>
> Acked-by: Markus Armbruster <armbru@redhat.com> (for QAPI schema)
> Signed-off-by: Michael Roth <michael.roth@amd.com>

[...]

> diff --git a/qapi/qom.json b/qapi/qom.json
> index 66b5781ca6..b25a3043da 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -920,6 +920,55 @@
>              '*handle': 'uint32',
>              '*kernel-hashes': 'bool' } }
>  
> +##
> +# @SevSnpGuestProperties:
> +#
> +# Properties for sev-snp-guest objects. Most of these are direct arguments
> +# for the KVM_SNP_* interfaces documented in the linux kernel source

"Linux", please.

> +# under Documentation/virt/kvm/amd-memory-encryption.rst, which are in

Does not seem to exist.  Do you mean
Documentation/arch/x86/amd-memory-encryption.rst?

> +# turn closely coupled with the SNP_INIT/SNP_LAUNCH_* firmware commands
> +# documented in the SEV-SNP Firmware ABI Specification (Rev 0.9).

docs/devel/qapi-code-gen.rst:

    For legibility, wrap text paragraphs so every line is at most 70
    characters long.

    Separate sentences with two spaces.

> +#
> +# More usage information is also available in the QEMU source tree under
> +# docs/amd-memory-encryption.
> +#
> +# @policy: the 'POLICY' parameter to the SNP_LAUNCH_START command, as
> +#          defined in the SEV-SNP firmware ABI (default: 0x30000)

docs/devel/qapi-code-gen.rst:

    Descriptions start with '\@name:'.  The description text must be
    indented like this::

     # @name: Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed
     #     do eiusmod tempor incididunt ut labore et dolore magna aliqua.

> +#
> +# @guest-visible-workarounds: 16-byte, base64-encoded blob to report
> +#                             hypervisor-defined workarounds, corresponding
> +#                             to the 'GOSVW' parameter of the
> +#                             SNP_LAUNCH_START command defined in the
> +#                             SEV-SNP firmware ABI (default: all-zero)
> +#
> +# @id-block: 96-byte, base64-encoded blob to provide the 'ID Block'
> +#            structure for the SNP_LAUNCH_FINISH command defined in the
> +#            SEV-SNP firmware ABI (default: all-zero)
> +#
> +# @id-auth: 4096-byte, base64-encoded blob to provide the 'ID Authentication
> +#           Information Structure' for the SNP_LAUNCH_FINISH command defined
> +#           in the SEV-SNP firmware ABI (default: all-zero)
> +#
> +# @auth-key-enabled: true if 'id-auth' blob contains the 'AUTHOR_KEY' field
> +#                    defined SEV-SNP firmware ABI (default: false)
> +#
> +# @host-data: 32-byte, base64-encoded, user-defined blob to provide to the
> +#             guest, as documented for the 'HOST_DATA' parameter of the
> +#             SNP_LAUNCH_FINISH command in the SEV-SNP firmware ABI
> +#             (default: all-zero)
> +#
> +# Since: 7.2

9.1

> +##

Together:

    ##
    # @SevSnpGuestProperties:
    #
    # Properties for sev-snp-guest objects.  Most of these are direct
    # arguments for the KVM_SNP_* interfaces documented in the Linux
    # kernel source under
    # Documentation/arch/x86/amd-memory-encryption.rst, which are in turn
    # closely coupled with the SNP_INIT/SNP_LAUNCH_* firmware commands
    # documented in the SEV-SNP Firmware ABI Specification (Rev 0.9).
    #
    # More usage information is also available in the QEMU source tree
    # under docs/amd-memory-encryption.
    #
    # @policy: the 'POLICY' parameter to the SNP_LAUNCH_START command, as
    #     defined in the SEV-SNP firmware ABI (default: 0x30000)
    #
    # @guest-visible-workarounds: 16-byte, base64-encoded blob to report
    #     hypervisor-defined workarounds, corresponding to the 'GOSVW'
    #     parameter of the SNP_LAUNCH_START command defined in the SEV-SNP
    #     firmware ABI (default: all-zero)
    #
    # @id-block: 96-byte, base64-encoded blob to provide the 'ID Block'
    #     structure for the SNP_LAUNCH_FINISH command defined in the
    #     SEV-SNP firmware ABI (default: all-zero)
    #
    # @id-auth: 4096-byte, base64-encoded blob to provide the 'ID
    #     Authentication Information Structure' for the SNP_LAUNCH_FINISH
    #     command defined in the SEV-SNP firmware ABI (default: all-zero)
    #
    # @auth-key-enabled: true if 'id-auth' blob contains the 'AUTHOR_KEY'
    #     field defined SEV-SNP firmware ABI (default: false)
    #
    # @host-data: 32-byte, base64-encoded, user-defined blob to provide to
    #     the guest, as documented for the 'HOST_DATA' parameter of the
    #     SNP_LAUNCH_FINISH command in the SEV-SNP firmware ABI (default:
    #     all-zero)
    #
    # @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: 9.1
    ##

We generally prefer symbolic to numeric / binary encoding in QMP.  Can
you explain briefly why you choose numeric and binary here?

> +{ 'struct': 'SevSnpGuestProperties',
> +  'base': 'SevCommonProperties',
> +  'data': {
> +            '*policy': 'uint64',
> +            '*guest-visible-workarounds': 'str',
> +            '*id-block': 'str',
> +            '*id-auth': 'str',
> +            '*auth-key-enabled': 'bool',
> +            '*host-data': 'str' } }
> +
>  ##
>  # @ThreadContextProperties:
>  #
> @@ -998,6 +1047,7 @@
>      { 'name': 'secret_keyring',
>        'if': 'CONFIG_SECRET_KEYRING' },
>      'sev-guest',
> +    'sev-snp-guest',
>      'thread-context',
>      's390-pv-guest',
>      'throttle-group',
> @@ -1068,6 +1118,7 @@
>        'secret_keyring':             { 'type': 'SecretKeyringProperties',
>                                        'if': 'CONFIG_SECRET_KEYRING' },
>        'sev-guest':                  'SevGuestProperties',
> +      'sev-snp-guest':              'SevSnpGuestProperties',
>        'thread-context':             'ThreadContextProperties',
>        'throttle-group':             'ThrottleGroupProperties',
>        'tls-creds-anon':             'TlsCredsAnonProperties',

[...]