Re: [PATCH 2/3] xl/libxl: Add ability to specify SMBIOS strings

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Wed, Aug 10, 2022 at 03:48:26PM -0400, Jason Andryuk wrote:
> hvm_xs_strings.h specifies xenstore entries which can be used to set or
> override smbios strings.  hvmloader has support for reading them, but
> xl/libxl support is not wired up.
> 
> Allow specifying the strings with the new xl.cfg option:
> smbios=["bios_vendor=Xen Project","system_version=1.0"]
> 
> In terms of strings, the SMBIOS specification 3.5 says:
> https://www.dmtf.org/sites/default/files/standards/documents/DSP0134_3.5.0.pdf
> """
> Strings must be encoded as UTF-8 with no byte order mark (BOM). For
> compatibility with older SMBIOS parsers, US-ASCII characters should be
> used.  NOTE There is no limit on the length of each individual text
> string. However, the length of the entire structure table (including all
> strings) must be reported in the Structure Table Length field of the
> 32-bit Structure Table Entry Point (see 5.2.1) and/or the Structure
> Table Maximum Size field of the 64-bit Structure Table Entry Point (see
> 5.2.2).
> """
> 
> The strings aren't checked for utf-8 or length.  hvmloader has a sanity
> check on the overall length.
> 
> The libxl_smbios_type enum starts at 1 since otherwise the 0th key is
> not printed in the json output.
> 
> Signed-off-by: Jason Andryuk <jandryuk@xxxxxxxxx>
> ---
> The rendered man page and html don't have a newline at then end of the
> new section.
> """
>            battery_device_name=STRING
>        ms_vm_genid="OPTION"
> """
> 
> however the txt format is correct:
> """
>         battery_device_name=STRING
> 
>     ms_vm_genid="OPTION"
> """
> 
> I'm at a loss as to why this is happening.

Maybe it's because =back just cancel =over, and pod2man just keeps going
as if it's the same list. Adding some text for the =item or after =back
would help, but that's done in the next patch, so I guess we can live
with that for one commit.

> ---
> diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
> index 2abaab439c..9034933ea8 100644
> --- a/tools/libs/light/libxl_dom.c
> +++ b/tools/libs/light/libxl_dom.c
> @@ -771,6 +771,26 @@ static int hvm_build_set_xs_values(libxl__gc *gc,
>              goto err;
>      }
>  
> +    for (int i = 0; i < info->u.hvm.num_smbios; i++) {
> +        char *p;
> +        path = GCSPRINTF("/local/domain/%d/"HVM_XS_BIOS_STRINGS"/%s", domid,
> +                   libxl_smbios_type_to_string(info->u.hvm.smbios[i].key));
> +
> +        /* libxl defines are all "_", but the HVM_XS_ strings are "-". */

"libxl defines are all "_"" seems a bit weird to me as a comment in
the source code, maybe a better comment would the conversion we need to
do, something like:

    Convert libxl_smbios_type string to xenstore path that hvmloader
    will use, as defined by HVM_XS_*. That is convert the '_' to '-'.

> +        p = strrchr(path, '/');
> +        for ( ; *p; p++) {
> +            if (*p == '_')
> +                *p = '-';
> +        }
> +
> +        LOGD(DEBUG, domid, "Writing %d %s %s\n", i, path,

I don't think printing the value of i would be useful here. Also adding
a '=' before the value and putting the value between double-quote would
be a bit better.

> +             info->u.hvm.smbios[i].value);
> +        ret = libxl__xs_printf(gc, XBT_NULL, path, "%s",
> +                               info->u.hvm.smbios[i].value);
> +        if (ret)
> +            goto err;
> +    }
> +
>      /* Only one module can be passed. PVHv2 guests do not support this. */
>      if (dom->acpi_modules[0].guest_addr_out && 
>          info->type == LIBXL_DOMAIN_TYPE_HVM) {
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 1b5381cef0..4f3f962773 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -1220,8 +1220,9 @@ void parse_config_data(const char *config_source,
>      XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms,
>                     *usbctrls, *usbdevs, *p9devs, *vdispls, *pvcallsifs_devs;
>      XLU_ConfigList *channels, *ioports, *irqs, *iomem, *viridian, *dtdevs,
> -                   *mca_caps;
> +                   *mca_caps, *smbios;
>      int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian, 
> num_mca_caps;
> +    int num_smbios;
>      int pci_power_mgmt = 0;
>      int pci_msitranslate = 0;
>      int pci_permissive = 0;
> @@ -1783,6 +1784,47 @@ void parse_config_data(const char *config_source,
>          xlu_cfg_replace_string(config, "acpi_firmware",
>                                 &b_info->u.hvm.acpi_firmware, 0);
>  
> +        switch (xlu_cfg_get_list(config, "smbios", &smbios, &num_smbios, 0))
> +        {
> +        case 0: /* Success */
> +            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;
> +
> +                buf = xlu_cfg_get_listitem(smbios, i);
> +                if (!buf) continue;

Isn't this an error? It seems that xlu_cfg_get_listitem would return 0
if 'i' is outside of the array or if the value isn't a string. I think
it would also mean that "smbios[i].key" and ".value" would have
uninitialized value and potentially garbage.

> +
> +                if (split_string_into_pair(buf, "=",
> +                                           &option_untrimmed,
> +                                           &value_untrimmed)) {
> +                    fprintf(stderr, "xl: failed to split \"%s\" into pair\n",
> +                            buf);
> +                    exit(EXIT_FAILURE);
> +                }
> +                trim(isspace, option_untrimmed, &option);
> +                trim(isspace, value_untrimmed, &value);

I think you could free "option_untrimmed" and "value_untrimmed", as
"option" and "value" are newly allocated strings. Also, "option" won't
be needed after it's been converted to an enum value.
> +
> +                e = libxl_smbios_type_from_string(option, &v);
> +                if (e) {
> +                    fprintf(stderr,
> +                            "xl: unknown smbios type '%s'\n",
> +                            buf);
> +                    exit(-ERROR_FAIL);
> +                }
> +
> +                b_info->u.hvm.smbios[i].key = v;
> +                b_info->u.hvm.smbios[i].value = value;
> +            }
> +            break;
> +        case ESRCH: break; /* Option not present */

Could you move the "break" to a new line? It will make it easier to read
that ESRCH is just ignore instead of throwing an error.

> +        default:
> +            fprintf(stderr,"xl: Unable to parse smbios options.\n");
> +            exit(-ERROR_FAIL);
> +        }
> +
>          if (!xlu_cfg_get_string(config, "ms_vm_genid", &buf, 0)) {
>              if (!strcmp(buf, "generate")) {
>                  e = libxl_ms_vm_genid_generate(ctx, 
> &b_info->u.hvm.ms_vm_genid);

Thanks,

-- 
Anthony PERARD