Re: [PATCH v2] docs: expand firmware descriptor to allow flash without NVRAM

[Kashyap Chamarthy <kchamart@redhat.com>]

On Mon, Jan 31, 2022 at 12:55:09PM +0000, Daniel P. Berrangé wrote:
> The current firmware descriptor schema for flash requires that both the
> executable to NVRAM template paths be provided. This is fine for the
> most common usage of EDK2 builds in virtualization where the separate
> _CODE and _VARS files are provided.
> 
> With confidential computing technology like AMD SEV, persistent storage
> of variables may be completely disabled because the firmware requires a
> known clean state on every cold boot. There is no way to express this
> in the firmware descriptor today.
> 
> Even with regular EDK2 builds it is possible to create a firmware that
> has both executable code and variable persistence in a single file. This
> hasn't been commonly used, since it would mean every guest bootup would
> need to clone the full firmware file, leading to redundant duplicate
> storage of the code portion. In some scenarios this may not matter and
> might even be beneficial. For example if a public cloud allows users to
> bring their own firmware, such that the user can pre-enroll their own
> secure boot keys, you're going to have this copied on disk for each
> tenant already. At this point the it can be simpler to just deal with
> a single file rather than split builds. The firmware descriptor ought
> to be able to express this combined firmware model too.

Cool, TIL that it's possible to include both the executable and the
variables file into a single file.

I briefly wondered if in this "combined" mode whether the no. of
duplicate copies can ever fill up the storage.  I doubt that, as the
combined size of _VARS + _CODE is just about 2MB.  So it only starts
mattering if you're running tens of thousands of guests.

> This all points towards expanding the schema for flash with a 'mode'
> concept:
> 
>  - "split" - the current implicit behaviour with separate files
>    for code and variables.
> 
>  - "combined" - the alternate behaviour where a single file contains
>    both code and variables.
> 
>  - "stateless" - the confidential computing use case where storage
>    of variables is completely disable, leaving only the code.
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  docs/interop/firmware.json | 54 ++++++++++++++++++++++++++++++++------
>  1 file changed, 46 insertions(+), 8 deletions(-)
> 
> In v2:
> 
>  - Mark 'mode' as optional field
>  - Misc typos in docs
> 
> diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
> index 8d8b0be030..f5d1d0b6e7 100644
> --- a/docs/interop/firmware.json
> +++ b/docs/interop/firmware.json
> @@ -210,24 +210,61 @@
>    'data'   : { 'filename' : 'str',
>                 'format'   : 'BlockdevDriver' } }
>  
> +
> +##
> +# @FirmwareFlashType:
> +#
> +# Describes how the firmware build handles code versus variable
> +# persistence.
> +#
> +# @split: the executable file contains code while the NVRAM
> +#         template provides variable storage. The executable
> +#         must be configured read-only and can be shared between
> +#         multiple guests. The NVRAM template must be cloned
> +#         for each new guest and configured read-write.
> +#
> +# @combined: the executable file contains both code and
> +#            variable storage. The executable must be cloned
> +#            for each new guest and configured read-write.
> +#            No NVRAM template will be specified.

Given my above wondering, is it worth adding a note here about storage
considerations when running large number of guests in the "combined"
mode?  If not, ignore my comment.

> +# @stateless: the executable file contains code and variable
> +#             storage is not persisted. The executed must

I guess you meant: s/executed/executable/

Whoever is applying the patch can touch it up.

> +#             be configured read-only and can be shared
> +#             between multiple guests. No NVRAM template
> +#             will be specified.
> +#
> +# Since: 7.0.0
> +##
> +{ 'enum': 'FirmwareFlashMode',
> +  'data': [ 'split', 'combined', 'stateless' ] }
> +
>  ##
>  # @FirmwareMappingFlash:
>  #
>  # Describes loading and mapping properties for the firmware executable
>  # and its accompanying NVRAM file, when @FirmwareDevice is @flash.
>  #
> -# @executable: Identifies the firmware executable. The firmware
> -#              executable may be shared by multiple virtual machine
> -#              definitions. The preferred corresponding QEMU command
> -#              line options are
> +# @mode: describes how the firmware build handles code versus variable
> +#        storage. If not present, it must be treated as if it was
> +#        configured with value ``split``. Since: 7.0.0

For consistency, might want to capitalize the first word:
s/describes/Describes/  

(Here too, maintainer can touch it up.)

[...]

The concept looks very clear, and obviously useful.  FWIW:

    Reviewed-by: Kashyap Chamarthy <kchamart@redhat.com>    

> -- 
> 2.34.1
> 

-- 
/kashyap