Re: [PATCH v4 14/24] hw/uefi: add var-service-json.c + qapi for NV vars.

[Markus Armbruster <armbru@redhat.com>]

Gerd Hoffmann <kraxel@redhat.com> writes:

> Define qapi schema for the uefi variable store state.
>
> Use it and the generated visitor helper functions to store persistent
> (EFI_VARIABLE_NON_VOLATILE) variables in JSON format on disk.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

[...]

> diff --git a/qapi/meson.build b/qapi/meson.build
> index e7bc54e5d047..eadde4db307f 100644
> --- a/qapi/meson.build
> +++ b/qapi/meson.build
> @@ -65,6 +65,7 @@ if have_system
>      'pci',
>      'rocker',
>      'tpm',
> +    'uefi',
>    ]
>  endif
>  if have_system or have_tools
> diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
> index b1581988e4eb..2877aff73d0c 100644
> --- a/qapi/qapi-schema.json
> +++ b/qapi/qapi-schema.json
> @@ -81,3 +81,4 @@
>  { 'include': 'vfio.json' }
>  { 'include': 'cryptodev.json' }
>  { 'include': 'cxl.json' }
> +{ 'include': 'uefi.json' }
> diff --git a/qapi/uefi.json b/qapi/uefi.json
> new file mode 100644
> index 000000000000..c268ed11b70c
> --- /dev/null
> +++ b/qapi/uefi.json
> @@ -0,0 +1,45 @@
> +# -*- Mode: Python -*-
> +# vim: filetype=python
> +#
> +

This ends up in section "CXL devices", which is not what you want.  I
guess you want a suitable heading right here, like

   ##
   # UEFI mumble mumble
   ##

> +##
> +# @UefiVariable:
> +#
> +# UEFI Variable

Overview text is optional.  Is this one worthwhile?

> +#
> +# @guid: variable namespace guid

How is guid usually capitalized in UEFI documentation?  GUID maybe?

> +#
> +# @name: variable name (utf-8)

UTF-8

> +#
> +# @attr: variable attributes

These are encoded as int.  How?

> +#
> +# @data: variable content (base64)

I'm not a native speaker...  for what it's worth, "contents" feels
better to me.

(base64 encoded) and (encoded in base64) are the common phrasings.

> +#
> +# @time: variable modification time (EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS).

Long line, please break it like

   # @time: variable modification time
   #     (EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS)

What's the unit and the anchor?  Seconds since the epoch?

What is EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS trying to
tell the reader?

> +#
> +# @digest: variable certificate digest (EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS).

Long line, please break it like

   # @digest: variable certificate digest
   #     (EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS)

What kind of digest is this?

> +#
> +# Since: 10.0
> +##
> +{ 'struct' : 'UefiVariable',
> +  'data' : { 'guid'  : 'str',
> +             'name'  : 'str',
> +             'attr'  : 'int',
> +             'data'  : 'str',
> +             '*time' : 'str',
> +             '*digest' : 'str'}}
> +
> +##
> +# @UefiVarStore:
> +#
> +# UEFI Variable Store

Overview is optional.  Is this one worthwhile?

> +#
> +# @version: 2

What about "currently always 2"?

> +#
> +# @variables: list of uefi variables

UEFI

> +#
> +# Since: 10.0
> +##
> +{ 'struct' : 'UefiVarStore',
> +  'data' : { 'version'   : 'int',
> +             'variables' : [ 'UefiVariable' ] }}