Re: [PATCH 3/3] xl/libxl: Add OEM string support to smbios

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Wed, Aug 10, 2022 at 03:48:27PM -0400, Jason Andryuk wrote:
> Add support for OEM strings in the SMBIOS type 11.
> 
> hvmloader checks them sequentially, so hide the implementation detail.
> Allow multiple plain oem= items and assign the numeric values
> internally.
> 
> Signed-off-by: Jason Andryuk <jandryuk@xxxxxxxxx>
> ---
>  docs/man/xl.cfg.5.pod.in           |  4 ++
>  tools/golang/xenlight/types.gen.go | 99 ++++++++++++++++++++++++++++++
>  tools/libs/light/libxl_types.idl   | 99 ++++++++++++++++++++++++++++++
>  tools/xl/xl_parse.c                | 15 +++++
>  4 files changed, 217 insertions(+)
> 
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index 7edf5d23f3..7947bf07ea 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -2104,8 +2104,12 @@ Each B<SMBIOS_SPEC_STRING> is a C<KEY=VALUE> string 
> from the following list:
>  
>  =item B<battery_device_name=STRING>
>  
> +=item B<oem=STRING>
> +
>  =back
>  
> +oem= strings can be specified mutiple times up to a limit of 99.

This could be move to just before =back I think.

> +
>  =item B<ms_vm_genid="OPTION">
>  
>  Provide a VM generation ID to the guest.
> diff --git a/tools/libs/light/libxl_types.idl 
> b/tools/libs/light/libxl_types.idl
> index d04207748e..58f404af37 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -436,6 +436,105 @@ libxl_smbios_type = Enumeration("smbios_type", [
>      (15, "enclosure_asset_tag"),
>      (16, "battery_manufacturer"),
>      (17, "battery_device_name"),
> +    (18, "oem_1"),
> +    (19, "oem_2"),
> +    (20, "oem_3"),
[...]
> +    (115, "oem_98"),
> +    (116, "oem_99"),

Instead of this long lists of enum value, could we just have one "oem"
type, and allow it to be used more than once? I know that would mean
having a special case in libxl, but that also mean that libxl will be
the only one to deal with the implementation detail on how to write oem
string for hvmloader (as opposed to deal with this in every application
using libxl).

>      ])
>  
>  libxl_smbios = Struct("smbios", [
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 4f3f962773..fb7f1f6867 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -1787,12 +1787,16 @@ void parse_config_data(const char *config_source,
>          switch (xlu_cfg_get_list(config, "smbios", &smbios, &num_smbios, 0))
>          {
>          case 0: /* Success */
> +        {
> +            unsigned int num_oem = 1;
> +
>              b_info->u.hvm.num_smbios = num_smbios;
>              b_info->u.hvm.smbios = xcalloc(num_smbios, sizeof(libxl_smbios));
>              for (i = 0; i < num_smbios; i++) {
>                  char *option_untrimmed, *value_untrimmed;
>                  char *option, *value;
>                  libxl_smbios_type v;
> +                char oem_buf[] = "oem_99";
>  
>                  buf = xlu_cfg_get_listitem(smbios, i);
>                  if (!buf) continue;
> @@ -1807,6 +1811,16 @@ void parse_config_data(const char *config_source,
>                  trim(isspace, option_untrimmed, &option);
>                  trim(isspace, value_untrimmed, &value);
>  
> +                if (strcmp(option, "oem") == 0) {
> +                    if (num_oem > 99) {
> +                        fprintf(stderr, "xl: sbmios oem strings limited to 
> 99\n");
> +                        exit(-ERROR_FAIL);
> +                    }
> +                    sprintf(oem_buf, "oem_%d", num_oem);
> +                    num_oem++;
> +                    option = oem_buf;

This mean that my proposal to free option in the previous patch isn't
going to work well. But you could free option before assigning a new
value.

An alternative to generate a string to be converted to an enum
value would be to simply do "LIBXL_SMBIOS_TYPE_OEM_1+num_oem-1".
Or just deal with oem string in libxl as proposed above.

> +                }
> +
>                  e = libxl_smbios_type_from_string(option, &v);
>                  if (e) {
>                      fprintf(stderr,
> @@ -1819,6 +1833,7 @@ void parse_config_data(const char *config_source,
>                  b_info->u.hvm.smbios[i].value = value;
>              }
>              break;
> +        }
>          case ESRCH: break; /* Option not present */
>          default:
>              fprintf(stderr,"xl: Unable to parse smbios options.\n");

Thanks,

-- 
Anthony PERARD