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 <jandr...@gmail.com>
> ---
> 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
