[PATCH v4 0/3] libxl smbios support

[PATCH v4 0/3] libxl smbios support

[Jason Andryuk]

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.  This patches adds a new xl.cfg option
and libxl support to write the xenstore strings.

The xl syntax looks like:
smbios=["bios_vendor=Xen Project","system_version=1.0"]

The Go binding generation needed extending to support Arrays inside a
KeyedUnion, which is what the first patch does.  The generated go code
builds, but it is otherwise untested.

There are also oem strings, oem-1..oem-99, that HVM loader supports.
xl parse multiple oem strings like smbios=["oem=A,oem=B"], libxl then
iterates over them and assigned to the oem-%d entries.  Both xl and
libxl check that the 99 string limit isn't exceeded.

The rendered man page and html don't have a newline at the end of the
new section after patch 2.
"""
           battery_device_name=STRING
       ms_vm_genid="OPTION"
"""

however the txt format is correct:
"""
        battery_device_name=STRING

    ms_vm_genid="OPTION"
"""

It goes away after patch 3 is applied since it adds text about the "oem"
option in between the two lines above.  I'm at a loss as to why this is
happening.

v4 is a rebase and resend of v3.

Jason Andryuk (3):
  golang/xenlight: Extend KeyedUnion to support Arrays
  xl/libxl: Add ability to specify SMBIOS strings
  xl/libxl: Add OEM string support to smbios

 docs/man/xl.cfg.5.pod.in             | 49 +++++++++++++++++++
 tools/golang/xenlight/gengotypes.py  | 41 +++++++++-------
 tools/golang/xenlight/helpers.gen.go | 51 ++++++++++++++++++++
 tools/golang/xenlight/types.gen.go   | 28 +++++++++++
 tools/include/libxl.h                |  5 ++
 tools/libs/light/libxl_dom.c         | 33 +++++++++++++
 tools/libs/light/libxl_types.idl     | 27 +++++++++++
 tools/xl/xl_parse.c                  | 71 +++++++++++++++++++++++++++-
 8 files changed, 288 insertions(+), 17 deletions(-)

-- 
2.39.2

[PATCH v4 1/3] golang/xenlight: Extend KeyedUnion to support Arrays

[Jason Andryuk]

Generation for KeyedUnion types doesn't support Arrays.  The smbios
support will place an smbios array inside the hvm KeyedUnion, and
gentotypes doesn't generate buildable Go code.

Have KeyedUnion add an idl.Array check and issue the approriate
xenlight_golang_array_to_C and xenlight_golang_array_from_C calls when
needed.  This matches how it is done in xenlight_golang_define_to_C &
xenlight_golang_define_from_C

xenlight_golang_array_to_C and xenlight_golang_array_from_C need to be
extended to set the cvarname and govarname as approriate for the
KeyedUnion cases to match the surrounding code.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 tools/golang/xenlight/gengotypes.py | 41 ++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 16 deletions(-)

diff --git a/tools/golang/xenlight/gengotypes.py b/tools/golang/xenlight/gengotypes.py
index 9fec60602d..e4eb7ca1c1 100644
--- a/tools/golang/xenlight/gengotypes.py
+++ b/tools/golang/xenlight/gengotypes.py
@@ -376,6 +376,10 @@ def xenlight_golang_union_from_C(ty = None, union_name = '', struct_name = ''):
         s += 'tmp := (*C.{0})(unsafe.Pointer(&xc.{1}[0]))\n'.format(typename,union_name)
 
         for nf in f.type.fields:
+            if isinstance(nf.type, idl.Array):
+                s += xenlight_golang_array_from_C(nf,cvarname='tmp')
+                continue
+
             s += xenlight_golang_convert_from_C(nf,cvarname='tmp')
 
         s += 'return nil\n'
@@ -416,7 +420,7 @@ def xenlight_golang_union_from_C(ty = None, union_name = '', struct_name = ''):
 
     return (s,extras)
 
-def xenlight_golang_array_from_C(ty = None):
+def xenlight_golang_array_from_C(ty = None, cvarname = 'xc'):
     """
     Convert C array to Go slice using the method
     described here:
@@ -433,9 +437,9 @@ def xenlight_golang_array_from_C(ty = None):
     clenvar    = ty.type.lenvar.name
 
     s += 'x.{0} = nil\n'.format(goname)
-    s += 'if n := int(xc.{0}); n > 0 {{\n'.format(clenvar)
+    s += 'if n := int({0}.{1}); n > 0 {{\n'.format(cvarname,clenvar)
     s += '{0} := '.format(cslice)
-    s +='(*[1<<28]C.{0})(unsafe.Pointer(xc.{1}))[:n:n]\n'.format(ctypename, cname)
+    s +='(*[1<<28]C.{0})(unsafe.Pointer({1}.{2}))[:n:n]\n'.format(ctypename, cvarname, cname)
     s += 'x.{0} = make([]{1}, n)\n'.format(goname, gotypename)
     s += 'for i, v := range {0} {{\n'.format(cslice)
 
@@ -579,6 +583,11 @@ def xenlight_golang_union_to_C(ty = None, union_name = '',
 
         s += 'var {0} C.{1}\n'.format(f.name,cgotype)
         for uf in f.type.fields:
+            if isinstance(uf.type, idl.Array):
+                s += xenlight_golang_array_to_C(uf, cvarname=f.name,
+                                                govarname="tmp")
+                continue
+
             s += xenlight_golang_convert_to_C(uf,cvarname=f.name,
                                               govarname='tmp')
 
@@ -596,7 +605,7 @@ def xenlight_golang_union_to_C(ty = None, union_name = '',
 
     return s
 
-def xenlight_golang_array_to_C(ty = None):
+def xenlight_golang_array_to_C(ty = None, cvarname="xc", govarname="x"):
     s = ''
 
     gotypename = xenlight_golang_fmt_name(ty.type.elem_type.typename)
@@ -608,27 +617,27 @@ def xenlight_golang_array_to_C(ty = None):
 
     is_enum = isinstance(ty.type.elem_type,idl.Enumeration)
     if gotypename in go_builtin_types or is_enum:
-        s += 'if {0} := len(x.{1}); {2} > 0 {{\n'.format(golenvar,goname,golenvar)
-        s += 'xc.{0} = (*C.{1})(C.malloc(C.size_t({2}*{3})))\n'.format(cname,ctypename,
+        s += 'if {0} := len({1}.{2}); {3} > 0 {{\n'.format(golenvar,govarname,goname,golenvar)
+        s += '{0}.{1} = (*C.{2})(C.malloc(C.size_t({3}*{4})))\n'.format(cvarname,cname,ctypename,
                                                                    golenvar,golenvar)
-        s += 'xc.{0} = C.int({1})\n'.format(clenvar,golenvar)
-        s += 'c{0} := (*[1<<28]C.{1})(unsafe.Pointer(xc.{2}))[:{3}:{4}]\n'.format(goname,
-                                                                      ctypename,cname,
+        s += '{0}.{1} = C.int({2})\n'.format(cvarname,clenvar,golenvar)
+        s += 'c{0} := (*[1<<28]C.{1})(unsafe.Pointer({2}.{3}))[:{4}:{5}]\n'.format(goname,
+                                                                      ctypename,cvarname,cname,
                                                                       golenvar,golenvar)
-        s += 'for i,v := range x.{0} {{\n'.format(goname)
+        s += 'for i,v := range {0}.{1} {{\n'.format(govarname,goname)
         s += 'c{0}[i] = C.{1}(v)\n'.format(goname,ctypename)
         s += '}\n}\n'
 
         return s
 
-    s += 'if {0} := len(x.{1}); {2} > 0 {{\n'.format(golenvar,goname,golenvar)
-    s += 'xc.{0} = (*C.{1})(C.malloc(C.ulong({2})*C.sizeof_{3}))\n'.format(cname,ctypename,
+    s += 'if {0} := len({1}.{2}); {3} > 0 {{\n'.format(golenvar,govarname,goname,golenvar)
+    s += '{0}.{1} = (*C.{2})(C.malloc(C.ulong({3})*C.sizeof_{4}))\n'.format(cvarname,cname,ctypename,
                                                                    golenvar,ctypename)
-    s += 'xc.{0} = C.int({1})\n'.format(clenvar,golenvar)
-    s += 'c{0} := (*[1<<28]C.{1})(unsafe.Pointer(xc.{2}))[:{3}:{4}]\n'.format(goname,
-                                                                         ctypename,cname,
+    s += '{0}.{1} = C.int({2})\n'.format(cvarname,clenvar,golenvar)
+    s += 'c{0} := (*[1<<28]C.{1})(unsafe.Pointer({2}.{3}))[:{4}:{5}]\n'.format(goname,
+                                                                         ctypename,cvarname,cname,
                                                                          golenvar,golenvar)
-    s += 'for i,v := range x.{0} {{\n'.format(goname)
+    s += 'for i,v := range {0}.{1} {{\n'.format(govarname,goname)
     s += 'if err := v.toC(&c{0}[i]); err != nil {{\n'.format(goname)
     s += 'return fmt.Errorf("converting field {0}: %v", err)\n'.format(goname)
     s += '}\n}\n}\n'
-- 
2.39.2

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

[Jason Andryuk]

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@gmail.com>
---
v3
Disallow empty value strings
xstrdup listitem before modifying
Handle option=foo=bar -> option foo=bar
Fix compilation - remove stray }
Remove log message newline
Add multiline comment star
Rename variable v to key

v2:
Update s/_/-/ comment
Update debug print to xs_path = "value"
Error on xlu_cfg_get_listitem failure
Use EXIT_FAILURE consistently
free parsed strings
Move break to new line

The rendered man page and html don't have a newline at the 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.
---
 docs/man/xl.cfg.5.pod.in             | 45 +++++++++++++++++++++
 tools/golang/xenlight/helpers.gen.go | 51 ++++++++++++++++++++++++
 tools/golang/xenlight/types.gen.go   | 27 +++++++++++++
 tools/include/libxl.h                |  5 +++
 tools/libs/light/libxl_dom.c         | 21 ++++++++++
 tools/libs/light/libxl_types.idl     | 26 +++++++++++++
 tools/xl/xl_parse.c                  | 58 +++++++++++++++++++++++++++-
 7 files changed, 232 insertions(+), 1 deletion(-)

diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 024bceeb61..bc4386ee96 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -2079,6 +2079,51 @@ number of vendor defined SMBIOS structures (type 128 - 255). Since SMBIOS
 structures do not present their overall size, each entry in the file must be
 preceded by a 32b integer indicating the size of the following structure.
 
+=item B<smbios=[ "SMBIOS_SPEC_STRING", "SMBIOS_SPEC_STRING", ...]>
+
+Specifies the SMBIOS values to be provided to the guest.  These set or
+override specific entries in the tables provided to the guest.
+
+Each B<SMBIOS_SPEC_STRING> is a C<KEY=VALUE> string from the following list:
+
+=over 4
+
+=item B<bios_vendor=STRING>
+
+=item B<bios_version=STRING>
+
+=item B<system_manufacturer=STRING>
+
+=item B<system_product_name=STRING>
+
+=item B<system_version=STRING>
+
+=item B<system_serial_number=STRING>
+
+=item B<baseboard_manufacturer=STRING>
+
+=item B<baseboard_product_name=STRING>
+
+=item B<baseboard_version=STRING>
+
+=item B<baseboard_serial_number=STRING>
+
+=item B<baseboard_asset_tag=STRING>
+
+=item B<baseboard_location_in_chassis=STRING>
+
+=item B<enclosure_manufacturer=STRING>
+
+=item B<enclosure_serial_number=STRING>
+
+=item B<enclosure_asset_tag=STRING>
+
+=item B<battery_manufacturer=STRING>
+
+=item B<battery_device_name=STRING>
+
+=back
+
 =item B<ms_vm_genid="OPTION">
 
 Provide a VM generation ID to the guest.
diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
index 3ac4938858..0a203d2232 100644
--- a/tools/golang/xenlight/helpers.gen.go
+++ b/tools/golang/xenlight/helpers.gen.go
@@ -589,6 +589,38 @@ xc.build_id = C.CString(x.BuildId)}
  return nil
  }
 
+// NewSmbios returns an instance of Smbios initialized with defaults.
+func NewSmbios() (*Smbios, error) {
+var (
+x Smbios
+xc C.libxl_smbios)
+
+C.libxl_smbios_init(&xc)
+defer C.libxl_smbios_dispose(&xc)
+
+if err := x.fromC(&xc); err != nil {
+return nil, err }
+
+return &x, nil}
+
+func (x *Smbios) fromC(xc *C.libxl_smbios) error {
+ x.Key = SmbiosType(xc.key)
+x.Value = C.GoString(xc.value)
+
+ return nil}
+
+func (x *Smbios) toC(xc *C.libxl_smbios) (err error){defer func(){
+if err != nil{
+C.libxl_smbios_dispose(xc)}
+}()
+
+xc.key = C.libxl_smbios_type(x.Key)
+if x.Value != "" {
+xc.value = C.CString(x.Value)}
+
+ return nil
+ }
+
 // NewDomainCreateInfo returns an instance of DomainCreateInfo initialized with defaults.
 func NewDomainCreateInfo() (*DomainCreateInfo, error) {
 var (
@@ -1183,6 +1215,15 @@ return fmt.Errorf("converting field Altp2M: %v", err)
 }
 x.SystemFirmware = C.GoString(tmp.system_firmware)
 x.SmbiosFirmware = C.GoString(tmp.smbios_firmware)
+x.Smbios = nil
+if n := int(tmp.num_smbios); n > 0 {
+cSmbios := (*[1<<28]C.libxl_smbios)(unsafe.Pointer(tmp.smbios))[:n:n]
+x.Smbios = make([]Smbios, n)
+for i, v := range cSmbios {
+if err := x.Smbios[i].fromC(&v); err != nil {
+return fmt.Errorf("converting field Smbios: %v", err) }
+}
+}
 x.AcpiFirmware = C.GoString(tmp.acpi_firmware)
 x.Hdtype = Hdtype(tmp.hdtype)
 if err := x.Nographic.fromC(&tmp.nographic);err != nil {
@@ -1495,6 +1536,16 @@ if tmp.SystemFirmware != "" {
 hvm.system_firmware = C.CString(tmp.SystemFirmware)}
 if tmp.SmbiosFirmware != "" {
 hvm.smbios_firmware = C.CString(tmp.SmbiosFirmware)}
+if numSmbios := len(tmp.Smbios); numSmbios > 0 {
+hvm.smbios = (*C.libxl_smbios)(C.malloc(C.ulong(numSmbios)*C.sizeof_libxl_smbios))
+hvm.num_smbios = C.int(numSmbios)
+cSmbios := (*[1<<28]C.libxl_smbios)(unsafe.Pointer(hvm.smbios))[:numSmbios:numSmbios]
+for i,v := range tmp.Smbios {
+if err := v.toC(&cSmbios[i]); err != nil {
+return fmt.Errorf("converting field Smbios: %v", err)
+}
+}
+}
 if tmp.AcpiFirmware != "" {
 hvm.acpi_firmware = C.CString(tmp.AcpiFirmware)}
 hvm.hdtype = C.libxl_hdtype(tmp.Hdtype)
diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go
index 16ce879e3f..2d8bc7654a 100644
--- a/tools/golang/xenlight/types.gen.go
+++ b/tools/golang/xenlight/types.gen.go
@@ -392,6 +392,32 @@ Commandline string
 BuildId string
 }
 
+type SmbiosType int
+const(
+SmbiosTypeBiosVendor SmbiosType = 1
+SmbiosTypeBiosVersion SmbiosType = 2
+SmbiosTypeSystemManufacturer SmbiosType = 3
+SmbiosTypeSystemProductName SmbiosType = 4
+SmbiosTypeSystemVersion SmbiosType = 5
+SmbiosTypeSystemSerialNumber SmbiosType = 6
+SmbiosTypeBaseboardManufacturer SmbiosType = 7
+SmbiosTypeBaseboardProductName SmbiosType = 8
+SmbiosTypeBaseboardVersion SmbiosType = 9
+SmbiosTypeBaseboardSerialNumber SmbiosType = 10
+SmbiosTypeBaseboardAssetTag SmbiosType = 11
+SmbiosTypeBaseboardLocationInChassis SmbiosType = 12
+SmbiosTypeEnclosureManufacturer SmbiosType = 13
+SmbiosTypeEnclosureSerialNumber SmbiosType = 14
+SmbiosTypeEnclosureAssetTag SmbiosType = 15
+SmbiosTypeBatteryManufacturer SmbiosType = 16
+SmbiosTypeBatteryDeviceName SmbiosType = 17
+)
+
+type Smbios struct {
+Key SmbiosType
+Value string
+}
+
 type DomainCreateInfo struct {
 Type DomainType
 Hap Defbool
@@ -572,6 +598,7 @@ NestedHvm Defbool
 Altp2M Defbool
 SystemFirmware string
 SmbiosFirmware string
+Smbios []Smbios
 AcpiFirmware string
 Hdtype Hdtype
 Nographic Defbool
diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index d652895075..5c65222f1e 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -174,6 +174,11 @@
  */
 #define LIBXL_HAVE_BUILDINFO_HVM_MS_VM_GENID 1
 
+/*
+ * libxl_domain_build_info has the u.hvm.smbios field.
+ */
+#define LIBXL_HAVE_BUILDINFO_HVM_SMBIOS 1
+
 /*
  * LIBXL_HAVE_VCPUINFO_SOFT_AFFINITY indicates that a 'cpumap_soft'
  * field (of libxl_bitmap type) is present in libxl_vcpuinfo,
diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
index f6311eea6e..5433301f70 100644
--- a/tools/libs/light/libxl_dom.c
+++ b/tools/libs/light/libxl_dom.c
@@ -773,6 +773,27 @@ 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));
+
+        /* 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 %s = \"%s\"", path,
+             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/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 0cfad8508d..df4dd36697 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -423,6 +423,31 @@ libxl_version_info = Struct("version_info", [
     ("build_id",          string),
     ], dir=DIR_OUT)
 
+libxl_smbios_type = Enumeration("smbios_type", [
+    (1,  "bios_vendor"),
+    (2,  "bios_version"),
+    (3,  "system_manufacturer"),
+    (4,  "system_product_name"),
+    (5,  "system_version"),
+    (6,  "system_serial_number"),
+    (7,  "baseboard_manufacturer"),
+    (8,  "baseboard_product_name"),
+    (9,  "baseboard_version"),
+    (10, "baseboard_serial_number"),
+    (11, "baseboard_asset_tag"),
+    (12, "baseboard_location_in_chassis"),
+    (13, "enclosure_manufacturer"),
+    (14, "enclosure_serial_number"),
+    (15, "enclosure_asset_tag"),
+    (16, "battery_manufacturer"),
+    (17, "battery_device_name"),
+    ])
+
+libxl_smbios = Struct("smbios", [
+    ("key",          libxl_smbios_type),
+    ("value",        string),
+    ], dir=DIR_IN)
+
 libxl_domain_create_info = Struct("domain_create_info",[
     ("type",         libxl_domain_type),
     ("hap",          libxl_defbool),
@@ -609,6 +634,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
                                        ("altp2m",           libxl_defbool),
                                        ("system_firmware",  string),
                                        ("smbios_firmware",  string),
+                                       ("smbios",           Array(libxl_smbios, "num_smbios")),
                                        ("acpi_firmware",    string),
                                        ("hdtype",           libxl_hdtype),
                                        ("nographic",        libxl_defbool),
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 853e9f357a..ba219024b6 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1297,8 +1297,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;
@@ -1860,6 +1861,61 @@ 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++) {
+                libxl_smbios_type type;
+                char *option;
+                char *value;
+
+                buf = xlu_cfg_get_listitem(smbios, i);
+                if (!buf) {
+                    fprintf(stderr,
+                            "xl: Unable to get element #%d in smbios list\n",
+                            i);
+                    exit(EXIT_FAILURE);
+                }
+
+                option = xstrdup(buf);
+                value = strchr(option, '=');
+                if (value == NULL) {
+                    fprintf(stderr, "xl: failed to split \"%s\" at '='\n",
+                            option);
+                    exit(EXIT_FAILURE);
+                }
+
+                *value = '\0';
+                value++;
+
+                if (*value == '\0') {
+                    fprintf(stderr,
+                            "xl: empty value not allowed for smbios \"%s=\"\n",
+                            option);
+                    exit(EXIT_FAILURE);
+                }
+
+                e = libxl_smbios_type_from_string(option, &type);
+                if (e) {
+                    fprintf(stderr, "xl: unknown smbios type '%s'\n", option);
+                    exit(EXIT_FAILURE);
+                }
+
+                b_info->u.hvm.smbios[i].key = type;
+                b_info->u.hvm.smbios[i].value = xstrdup(value);
+
+                free(option);
+            }
+            break;
+        case ESRCH: /* Option not present */
+            break;
+        default:
+            fprintf(stderr,"xl: Unable to parse smbios options.\n");
+            exit(EXIT_FAILURE);
+        }
+
         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);
-- 
2.39.2

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

[Jason Andryuk]

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@gmail.com>
---
v3:
Add } from patch 2 to parse_config_data
Use EXIT_FAILURE
Print error message in libxl for > 99 OEM strings

v2:
Move oem= description to be indented in docs
Re-work oem= description
Re-word oem string limit xl error message
Replace OEM_{1,99) with just OEM and handle in libxl

This change re-introduces the newline before ms_vm_genid.
---
 docs/man/xl.cfg.5.pod.in           |  4 ++++
 tools/golang/xenlight/types.gen.go |  1 +
 tools/libs/light/libxl_dom.c       | 16 ++++++++++++++--
 tools/libs/light/libxl_types.idl   |  1 +
 tools/xl/xl_parse.c                | 13 +++++++++++++
 5 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index bc4386ee96..10f37990be 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -2122,6 +2122,10 @@ 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>
+
+oem= can be specified up to 99 times.
+
 =back
 
 =item B<ms_vm_genid="OPTION">
diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go
index 2d8bc7654a..a7c17699f8 100644
--- a/tools/golang/xenlight/types.gen.go
+++ b/tools/golang/xenlight/types.gen.go
@@ -411,6 +411,7 @@ SmbiosTypeEnclosureSerialNumber SmbiosType = 14
 SmbiosTypeEnclosureAssetTag SmbiosType = 15
 SmbiosTypeBatteryManufacturer SmbiosType = 16
 SmbiosTypeBatteryDeviceName SmbiosType = 17
+SmbiosTypeOem SmbiosType = 18
 )
 
 type Smbios struct {
diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
index 5433301f70..25fb716084 100644
--- a/tools/libs/light/libxl_dom.c
+++ b/tools/libs/light/libxl_dom.c
@@ -755,6 +755,7 @@ static int hvm_build_set_xs_values(libxl__gc *gc,
                                    const libxl_domain_build_info *info)
 {
     char *path = NULL;
+    int num_oem = 1;
     int ret = 0;
 
     if (dom->smbios_module.guest_addr_out) {
@@ -775,8 +776,19 @@ static int hvm_build_set_xs_values(libxl__gc *gc,
 
     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));
+        if (info->u.hvm.smbios[i].key == LIBXL_SMBIOS_TYPE_OEM) {
+            if (num_oem > 99) {
+                LOGD(ERROR, domid, "More than 99 SMBIOS OEM strings specified");
+                ret = ERROR_INVAL;
+                goto err;
+            }
+            path = GCSPRINTF("/local/domain/%d/"HVM_XS_OEM_STRINGS, domid,
+                             num_oem);
+            num_oem++;
+        } else {
+            path = GCSPRINTF("/local/domain/%d/"HVM_XS_BIOS_STRINGS"/%s", domid,
+                       libxl_smbios_type_to_string(info->u.hvm.smbios[i].key));
+        }
 
         /* Convert libxl_smbios_type string to xenstore path that hvmloader
          * will use, as defined by HVM_XS_*. That is convert the '_' to '-'. */
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index df4dd36697..c10292e0d7 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -441,6 +441,7 @@ libxl_smbios_type = Enumeration("smbios_type", [
     (15, "enclosure_asset_tag"),
     (16, "battery_manufacturer"),
     (17, "battery_device_name"),
+    (18, "oem"),
     ])
 
 libxl_smbios = Struct("smbios", [
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index ba219024b6..e344d4fda3 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1864,6 +1864,9 @@ 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++) {
@@ -1903,12 +1906,22 @@ void parse_config_data(const char *config_source,
                     exit(EXIT_FAILURE);
                 }
 
+                if (type == LIBXL_SMBIOS_TYPE_OEM) {
+                    if (num_oem > 99) {
+                        fprintf(stderr,
+                                "xl: smbios limited to 99 oem strings\n");
+                        exit(EXIT_FAILURE);
+                    }
+                    num_oem++;
+                }
+
                 b_info->u.hvm.smbios[i].key = type;
                 b_info->u.hvm.smbios[i].value = xstrdup(value);
 
                 free(option);
             }
             break;
+        }
         case ESRCH: /* Option not present */
             break;
         default:
-- 
2.39.2

Re: [PATCH v4 0/3] libxl smbios support

[George Dunlap]

[-- Attachment #1: Type: text/plain, Size: 764 bytes --]

On Mon, Mar 6, 2023 at 8:40 PM Jason Andryuk <jandryuk@gmail.com> 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.  This patches adds a new xl.cfg option
> and libxl support to write the xenstore strings.
>
> The xl syntax looks like:
> smbios=["bios_vendor=Xen Project","system_version=1.0"]
>
> The Go binding generation needed extending to support Arrays inside a
> KeyedUnion, which is what the first patch does.  The generated go code
> builds, but it is otherwise untested.
>

The python & generated code looks good to me.

Golang bits:
Acked-by: George Dunlap <george.dunlap@citrix.com>

>
>

[-- Attachment #2: Type: text/html, Size: 1350 bytes --]

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

[Anthony PERARD]

On Mon, Mar 06, 2023 at 03:40:23PM -0500, 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@gmail.com>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

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

[Anthony PERARD]

On Mon, Mar 06, 2023 at 03:40:24PM -0500, 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@gmail.com>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

Re: [PATCH v4 0/3] libxl smbios support

[Jan Beulich]

On 06.03.2023 21:40, 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.  This patches adds a new xl.cfg option
> and libxl support to write the xenstore strings.
> 
> The xl syntax looks like:
> smbios=["bios_vendor=Xen Project","system_version=1.0"]
> 
> The Go binding generation needed extending to support Arrays inside a
> KeyedUnion, which is what the first patch does.  The generated go code
> builds, but it is otherwise untested.
> 
> There are also oem strings, oem-1..oem-99, that HVM loader supports.
> xl parse multiple oem strings like smbios=["oem=A,oem=B"], libxl then
> iterates over them and assigned to the oem-%d entries.  Both xl and
> libxl check that the 99 string limit isn't exceeded.
> 
> The rendered man page and html don't have a newline at the end of the
> new section after patch 2.
> """
>            battery_device_name=STRING
>        ms_vm_genid="OPTION"
> """
> 
> however the txt format is correct:
> """
>         battery_device_name=STRING
> 
>     ms_vm_genid="OPTION"
> """
> 
> It goes away after patch 3 is applied since it adds text about the "oem"
> option in between the two lines above.  I'm at a loss as to why this is
> happening.
> 
> v4 is a rebase and resend of v3.
> 
> Jason Andryuk (3):
>   golang/xenlight: Extend KeyedUnion to support Arrays
>   xl/libxl: Add ability to specify SMBIOS strings
>   xl/libxl: Add OEM string support to smbios
> 
>  docs/man/xl.cfg.5.pod.in             | 49 +++++++++++++++++++
>  tools/golang/xenlight/gengotypes.py  | 41 +++++++++-------
>  tools/golang/xenlight/helpers.gen.go | 51 ++++++++++++++++++++
>  tools/golang/xenlight/types.gen.go   | 28 +++++++++++
>  tools/include/libxl.h                |  5 ++
>  tools/libs/light/libxl_dom.c         | 33 +++++++++++++
>  tools/libs/light/libxl_types.idl     | 27 +++++++++++
>  tools/xl/xl_parse.c                  | 71 +++++++++++++++++++++++++++-
>  8 files changed, 288 insertions(+), 17 deletions(-)

Is this work something that's worth mentioning in CHANGELOG.md?

Jan

Re: [PATCH v4 0/3] libxl smbios support

[Jason Andryuk]

On Thu, Mar 16, 2023 at 3:53 AM Jan Beulich <jbeulich@suse.com> wrote:
> Is this work something that's worth mentioning in CHANGELOG.md?

Sure, I'll add an entry.

Thanks,
Jason

Re: [PATCH v4 0/3] libxl smbios support

[Andrew Cooper]

On 16/03/2023 7:53 am, Jan Beulich wrote:
> On 06.03.2023 21:40, 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.  This patches adds a new xl.cfg option
>> and libxl support to write the xenstore strings.
>>
>> The xl syntax looks like:
>> smbios=["bios_vendor=Xen Project","system_version=1.0"]
>>
>> The Go binding generation needed extending to support Arrays inside a
>> KeyedUnion, which is what the first patch does.  The generated go code
>> builds, but it is otherwise untested.
>>
>> There are also oem strings, oem-1..oem-99, that HVM loader supports.
>> xl parse multiple oem strings like smbios=["oem=A,oem=B"], libxl then
>> iterates over them and assigned to the oem-%d entries.  Both xl and
>> libxl check that the 99 string limit isn't exceeded.
>>
>> The rendered man page and html don't have a newline at the end of the
>> new section after patch 2.
>> """
>>            battery_device_name=STRING
>>        ms_vm_genid="OPTION"
>> """
>>
>> however the txt format is correct:
>> """
>>         battery_device_name=STRING
>>
>>     ms_vm_genid="OPTION"
>> """
>>
>> It goes away after patch 3 is applied since it adds text about the "oem"
>> option in between the two lines above.  I'm at a loss as to why this is
>> happening.
>>
>> v4 is a rebase and resend of v3.
>>
>> Jason Andryuk (3):
>>   golang/xenlight: Extend KeyedUnion to support Arrays
>>   xl/libxl: Add ability to specify SMBIOS strings
>>   xl/libxl: Add OEM string support to smbios
>>
>>  docs/man/xl.cfg.5.pod.in             | 49 +++++++++++++++++++
>>  tools/golang/xenlight/gengotypes.py  | 41 +++++++++-------
>>  tools/golang/xenlight/helpers.gen.go | 51 ++++++++++++++++++++
>>  tools/golang/xenlight/types.gen.go   | 28 +++++++++++
>>  tools/include/libxl.h                |  5 ++
>>  tools/libs/light/libxl_dom.c         | 33 +++++++++++++
>>  tools/libs/light/libxl_types.idl     | 27 +++++++++++
>>  tools/xl/xl_parse.c                  | 71 +++++++++++++++++++++++++++-
>>  8 files changed, 288 insertions(+), 17 deletions(-)
> Is this work something that's worth mentioning in CHANGELOG.md?

Yes.  Thanks for remembering - I'd forgotten.

~Andrew