[PATCH 0/5] Add support for loading SMBIOS OEM strings from a file

[PATCH 0/5] Add support for loading SMBIOS OEM strings from a file

[Daniel P. Berrangé]

I previously added support for SMBIOS OEM strings tables but only
allowed for data to be passed inline. Potential users indicated they
wanted to pass some quite large data blobs which is inconvenient todo
inline. Thus I'm adding support for passing the data from a file.

In testing this I discovered the hard way that on x86 we're limited to
using the SMBIOS 2.1 entry point currently. This has a maximum size of
0xffff, and if you exceed this all sorts of wierd behaviour happens.

QEMU forces SMBIOS 2.1 on x86 because the default SeaBIOS firmware does
not support SMBIOS 3.0. The EDK2 firmware supports SMBIOS 3.0 and QEMU
defaults to this on the ARM virt machine type.

This series adds support for checking the SMBIOS 2.1 limits to protect
users from impossible to diagnose problems.

There is also a fix needed to SeaBIOS which fails to check for
integer overflow when it appends the type 0 table.

  https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/thread/3EMIOY=
6YS6MG5UQN3JJJS2A3DJZOVFR6/

IIUC, SMBIOS 3.0 should onlky be limited by what you can fit into RAM,
but in testing, EDK2 appears to hang shortly after the SMBIOS 3.0 data
size exceeds 128 KB. I've not spotted an obvious flaw in EDK2 or QEMU,
nor do I attempt to enforce a limit in QEMU for SMBIOS 3.0.

The 4th and 5th patches are what I used to test x86 machine types with
EDK2 support for SMBIOS 3.0.  I'm ambivalent on whether we really need
them or not, but it does feel desirable to have parity of features
between x86 and ARM when using SMBIOS with EDK2.

Daniel P. Berrang=C3=A9 (5):
  hw/smbios: support loading OEM strings values from a file
  hw/smbios: report error if table size is too large
  qemu-options: document SMBIOS type 11 settings
  hw/smbios: use qapi for SMBIOS entry point type enum
  hw/i386: expose a "smbios_ep" PC machine property

 hw/arm/virt.c                |  2 +-
 hw/i386/pc.c                 | 26 ++++++++++
 hw/i386/pc_piix.c            |  2 +-
 hw/i386/pc_q35.c             |  2 +-
 hw/smbios/smbios.c           | 93 +++++++++++++++++++++++++++++-------
 include/hw/firmware/smbios.h |  9 +---
 include/hw/i386/pc.h         |  3 ++
 qapi/machine.json            | 12 +++++
 qemu-options.hx              | 41 ++++++++++++++++
 9 files changed, 164 insertions(+), 26 deletions(-)

--=20
2.26.2

[PATCH 1/5] hw/smbios: support loading OEM strings values from a file

[Daniel P. Berrangé]

Some applications want to pass quite large values for the OEM strings
entries. Rather than having huge strings on the command line, it would
be better to load them from a file, as supported with -fw_cfg.

This introduces the "valuefile" parameter allowing for:

  $ echo -n "thisthing" > mydata.txt
  $ qemu-system-x86_64 \
    -smbios type=11,value=something \
    -smbios type=11,valuefile=mydata.txt \
    -smbios type=11,value=somemore \
    ...other args...

Now in the guest

$ dmidecide -t 11
Getting SMBIOS data from sysfs.
SMBIOS 2.8 present.

Handle 0x0E00, DMI type 11, 5 bytes
OEM Strings
	String 1: something
	String 2: thisthing
	String 3: somemore

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/smbios/smbios.c | 72 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 59 insertions(+), 13 deletions(-)

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 7cc950b41c..8450fad285 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -110,7 +110,7 @@ static struct {
 
 static struct {
     size_t nvalues;
-    const char **values;
+    char **values;
 } type11;
 
 static struct {
@@ -314,6 +314,11 @@ static const QemuOptDesc qemu_smbios_type11_opts[] = {
         .type = QEMU_OPT_STRING,
         .help = "OEM string data",
     },
+    {
+        .name = "path",
+        .type = QEMU_OPT_STRING,
+        .help = "OEM string data from file",
+    },
 };
 
 static const QemuOptDesc qemu_smbios_type17_opts[] = {
@@ -641,6 +646,8 @@ static void smbios_build_type_11_table(void)
 
     for (i = 0; i < type11.nvalues; i++) {
         SMBIOS_TABLE_SET_STR_LIST(11, type11.values[i]);
+        g_free(type11.values[i]);
+        type11.values[i] = NULL;
     }
 
     SMBIOS_BUILD_TABLE_POST;
@@ -940,9 +947,8 @@ static void save_opt(const char **dest, QemuOpts *opts, const char *name)
 
 
 struct opt_list {
-    const char *name;
     size_t *ndest;
-    const char ***dest;
+    char ***dest;
 };
 
 static int save_opt_one(void *opaque,
@@ -951,23 +957,61 @@ static int save_opt_one(void *opaque,
 {
     struct opt_list *opt = opaque;
 
-    if (!g_str_equal(name, opt->name)) {
-        return 0;
+    if (g_str_equal(name, "path")) {
+        g_autoptr(GByteArray) data = g_byte_array_new();
+        g_autofree char *buf = g_new(char, 4096);
+        ssize_t ret;
+        int fd = qemu_open(value, O_RDONLY);
+        if (fd < 0) {
+            error_setg(errp, "Unable to open %s: %s", value, strerror(errno));
+            return -1;
+        }
+
+        while (1) {
+            ret = read(fd, buf, 4096);
+            if (ret == 0) {
+                break;
+            }
+            if (ret < 0) {
+                error_setg(errp, "Unable to read from %s: %s",
+                           value, strerror(errno));
+                return -1;
+            }
+            if (memchr(buf, '\0', ret)) {
+                error_setg(errp, "NUL in OEM strings value in %s", value);
+                return -1;
+            }
+            g_byte_array_append(data, (guint8 *)buf, ret);
+        }
+
+        close(fd);
+
+        *opt->dest = g_renew(char *, *opt->dest, (*opt->ndest) + 1);
+        (*opt->dest)[*opt->ndest] = (char *)g_byte_array_free(data,  FALSE);
+        (*opt->ndest)++;
+        data = NULL;
+   } else if (g_str_equal(name, "value")) {
+        *opt->dest = g_renew(char *, *opt->dest, (*opt->ndest) + 1);
+        (*opt->dest)[*opt->ndest] = g_strdup(value);
+        (*opt->ndest)++;
+    } else if (!g_str_equal(name, "type")) {
+        error_setg(errp, "Unexpected option %s", name);
+        return -1;
     }
 
-    *opt->dest = g_renew(const char *, *opt->dest, (*opt->ndest) + 1);
-    (*opt->dest)[*opt->ndest] = value;
-    (*opt->ndest)++;
     return 0;
 }
 
-static void save_opt_list(size_t *ndest, const char ***dest,
-                          QemuOpts *opts, const char *name)
+static bool save_opt_list(size_t *ndest, char ***dest, QemuOpts *opts,
+                          Error **errp)
 {
     struct opt_list opt = {
-        name, ndest, dest,
+        ndest, dest,
     };
-    qemu_opt_foreach(opts, save_opt_one, &opt, NULL);
+    if (!qemu_opt_foreach(opts, save_opt_one, &opt, errp)) {
+        return false;
+    }
+    return true;
 }
 
 void smbios_entry_add(QemuOpts *opts, Error **errp)
@@ -1149,7 +1193,9 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
             if (!qemu_opts_validate(opts, qemu_smbios_type11_opts, errp)) {
                 return;
             }
-            save_opt_list(&type11.nvalues, &type11.values, opts, "value");
+            if (!save_opt_list(&type11.nvalues, &type11.values, opts, errp)) {
+                return;
+            }
             return;
         case 17:
             if (!qemu_opts_validate(opts, qemu_smbios_type17_opts, errp)) {
-- 
2.26.2

Re: [PATCH 1/5] hw/smbios: support loading OEM strings values from a file

[Philippe Mathieu-Daudé]

Hi Daniel,

On 9/8/20 6:54 PM, Daniel P. Berrangé wrote:
> Some applications want to pass quite large values for the OEM strings
> entries. Rather than having huge strings on the command line, it would
> be better to load them from a file, as supported with -fw_cfg.
> 
> This introduces the "valuefile" parameter allowing for:
> 
>   $ echo -n "thisthing" > mydata.txt
>   $ qemu-system-x86_64 \
>     -smbios type=11,value=something \
>     -smbios type=11,valuefile=mydata.txt \
>     -smbios type=11,value=somemore \
>     ...other args...
> 
> Now in the guest
> 
> $ dmidecide -t 11
> Getting SMBIOS data from sysfs.
> SMBIOS 2.8 present.
> 
> Handle 0x0E00, DMI type 11, 5 bytes
> OEM Strings
> 	String 1: something
> 	String 2: thisthing
> 	String 3: somemore
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  hw/smbios/smbios.c | 72 +++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 59 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index 7cc950b41c..8450fad285 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -110,7 +110,7 @@ static struct {
>  
>  static struct {
>      size_t nvalues;
> -    const char **values;
> +    char **values;
>  } type11;
>  
>  static struct {
> @@ -314,6 +314,11 @@ static const QemuOptDesc qemu_smbios_type11_opts[] = {
>          .type = QEMU_OPT_STRING,
>          .help = "OEM string data",
>      },
> +    {
> +        .name = "path",
> +        .type = QEMU_OPT_STRING,
> +        .help = "OEM string data from file",
> +    },
>  };
>  
>  static const QemuOptDesc qemu_smbios_type17_opts[] = {
> @@ -641,6 +646,8 @@ static void smbios_build_type_11_table(void)
>  
>      for (i = 0; i < type11.nvalues; i++) {
>          SMBIOS_TABLE_SET_STR_LIST(11, type11.values[i]);
> +        g_free(type11.values[i]);
> +        type11.values[i] = NULL;
>      }
>  
>      SMBIOS_BUILD_TABLE_POST;
> @@ -940,9 +947,8 @@ static void save_opt(const char **dest, QemuOpts *opts, const char *name)
>  
>  
>  struct opt_list {
> -    const char *name;
>      size_t *ndest;
> -    const char ***dest;
> +    char ***dest;
>  };
>  
>  static int save_opt_one(void *opaque,
> @@ -951,23 +957,61 @@ static int save_opt_one(void *opaque,
>  {
>      struct opt_list *opt = opaque;
>  
> -    if (!g_str_equal(name, opt->name)) {
> -        return 0;
> +    if (g_str_equal(name, "path")) {
> +        g_autoptr(GByteArray) data = g_byte_array_new();
> +        g_autofree char *buf = g_new(char, 4096);
> +        ssize_t ret;
> +        int fd = qemu_open(value, O_RDONLY);

While not use g_file_get_contents()?

> +        if (fd < 0) {
> +            error_setg(errp, "Unable to open %s: %s", value, strerror(errno));
> +            return -1;
> +        }
> +
> +        while (1) {
> +            ret = read(fd, buf, 4096);
> +            if (ret == 0) {
> +                break;
> +            }
> +            if (ret < 0) {
> +                error_setg(errp, "Unable to read from %s: %s",
> +                           value, strerror(errno));
> +                return -1;
> +            }
> +            if (memchr(buf, '\0', ret)) {
> +                error_setg(errp, "NUL in OEM strings value in %s", value);
> +                return -1;
> +            }
> +            g_byte_array_append(data, (guint8 *)buf, ret);
> +        }
> +
> +        close(fd);
[...]

Re: [PATCH 1/5] hw/smbios: support loading OEM strings values from a file

[Daniel P. Berrangé]

On Tue, Sep 08, 2020 at 08:24:35PM +0200, Philippe Mathieu-Daudé wrote:
> Hi Daniel,
> 
> On 9/8/20 6:54 PM, Daniel P. Berrangé wrote:
> > Some applications want to pass quite large values for the OEM strings
> > entries. Rather than having huge strings on the command line, it would
> > be better to load them from a file, as supported with -fw_cfg.
> > 
> > This introduces the "valuefile" parameter allowing for:
> > 
> >   $ echo -n "thisthing" > mydata.txt
> >   $ qemu-system-x86_64 \
> >     -smbios type=11,value=something \
> >     -smbios type=11,valuefile=mydata.txt \
> >     -smbios type=11,value=somemore \
> >     ...other args...
> > 
> > Now in the guest
> > 
> > $ dmidecide -t 11
> > Getting SMBIOS data from sysfs.
> > SMBIOS 2.8 present.
> > 
> > Handle 0x0E00, DMI type 11, 5 bytes
> > OEM Strings
> > 	String 1: something
> > 	String 2: thisthing
> > 	String 3: somemore
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  hw/smbios/smbios.c | 72 +++++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 59 insertions(+), 13 deletions(-)
> > 
> > diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> > index 7cc950b41c..8450fad285 100644
> > --- a/hw/smbios/smbios.c
> > +++ b/hw/smbios/smbios.c
> > @@ -110,7 +110,7 @@ static struct {
> >  
> >  static struct {
> >      size_t nvalues;
> > -    const char **values;
> > +    char **values;
> >  } type11;
> >  
> >  static struct {
> > @@ -314,6 +314,11 @@ static const QemuOptDesc qemu_smbios_type11_opts[] = {
> >          .type = QEMU_OPT_STRING,
> >          .help = "OEM string data",
> >      },
> > +    {
> > +        .name = "path",
> > +        .type = QEMU_OPT_STRING,
> > +        .help = "OEM string data from file",
> > +    },
> >  };
> >  
> >  static const QemuOptDesc qemu_smbios_type17_opts[] = {
> > @@ -641,6 +646,8 @@ static void smbios_build_type_11_table(void)
> >  
> >      for (i = 0; i < type11.nvalues; i++) {
> >          SMBIOS_TABLE_SET_STR_LIST(11, type11.values[i]);
> > +        g_free(type11.values[i]);
> > +        type11.values[i] = NULL;
> >      }
> >  
> >      SMBIOS_BUILD_TABLE_POST;
> > @@ -940,9 +947,8 @@ static void save_opt(const char **dest, QemuOpts *opts, const char *name)
> >  
> >  
> >  struct opt_list {
> > -    const char *name;
> >      size_t *ndest;
> > -    const char ***dest;
> > +    char ***dest;
> >  };
> >  
> >  static int save_opt_one(void *opaque,
> > @@ -951,23 +957,61 @@ static int save_opt_one(void *opaque,
> >  {
> >      struct opt_list *opt = opaque;
> >  
> > -    if (!g_str_equal(name, opt->name)) {
> > -        return 0;
> > +    if (g_str_equal(name, "path")) {
> > +        g_autoptr(GByteArray) data = g_byte_array_new();
> > +        g_autofree char *buf = g_new(char, 4096);
> > +        ssize_t ret;
> > +        int fd = qemu_open(value, O_RDONLY);
> 
> While not use g_file_get_contents()?

qemu_open lets mgmt apps pass in pre-opened FDs using /dev/fdset/NN
syntax.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/5] hw/smbios: support loading OEM strings values from a file

[Philippe Mathieu-Daudé]

On 9/9/20 9:35 AM, Daniel P. Berrangé wrote:
> On Tue, Sep 08, 2020 at 08:24:35PM +0200, Philippe Mathieu-Daudé wrote:
>> Hi Daniel,
>>
>> On 9/8/20 6:54 PM, Daniel P. Berrangé wrote:
>>> Some applications want to pass quite large values for the OEM strings
>>> entries. Rather than having huge strings on the command line, it would
>>> be better to load them from a file, as supported with -fw_cfg.
>>>
>>> This introduces the "valuefile" parameter allowing for:
>>>
>>>   $ echo -n "thisthing" > mydata.txt
>>>   $ qemu-system-x86_64 \
>>>     -smbios type=11,value=something \
>>>     -smbios type=11,valuefile=mydata.txt \
>>>     -smbios type=11,value=somemore \
>>>     ...other args...
>>>
>>> Now in the guest
>>>
>>> $ dmidecide -t 11
>>> Getting SMBIOS data from sysfs.
>>> SMBIOS 2.8 present.
>>>
>>> Handle 0x0E00, DMI type 11, 5 bytes
>>> OEM Strings
>>> 	String 1: something
>>> 	String 2: thisthing
>>> 	String 3: somemore
>>>
>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>> ---
>>>  hw/smbios/smbios.c | 72 +++++++++++++++++++++++++++++++++++++---------
>>>  1 file changed, 59 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
>>> index 7cc950b41c..8450fad285 100644
>>> --- a/hw/smbios/smbios.c
>>> +++ b/hw/smbios/smbios.c
>>> @@ -110,7 +110,7 @@ static struct {
>>>  
>>>  static struct {
>>>      size_t nvalues;
>>> -    const char **values;
>>> +    char **values;
>>>  } type11;
>>>  
>>>  static struct {
>>> @@ -314,6 +314,11 @@ static const QemuOptDesc qemu_smbios_type11_opts[] = {
>>>          .type = QEMU_OPT_STRING,
>>>          .help = "OEM string data",
>>>      },
>>> +    {
>>> +        .name = "path",
>>> +        .type = QEMU_OPT_STRING,
>>> +        .help = "OEM string data from file",
>>> +    },
>>>  };
>>>  
>>>  static const QemuOptDesc qemu_smbios_type17_opts[] = {
>>> @@ -641,6 +646,8 @@ static void smbios_build_type_11_table(void)
>>>  
>>>      for (i = 0; i < type11.nvalues; i++) {
>>>          SMBIOS_TABLE_SET_STR_LIST(11, type11.values[i]);
>>> +        g_free(type11.values[i]);
>>> +        type11.values[i] = NULL;
>>>      }
>>>  
>>>      SMBIOS_BUILD_TABLE_POST;
>>> @@ -940,9 +947,8 @@ static void save_opt(const char **dest, QemuOpts *opts, const char *name)
>>>  
>>>  
>>>  struct opt_list {
>>> -    const char *name;
>>>      size_t *ndest;
>>> -    const char ***dest;
>>> +    char ***dest;
>>>  };
>>>  
>>>  static int save_opt_one(void *opaque,
>>> @@ -951,23 +957,61 @@ static int save_opt_one(void *opaque,
>>>  {
>>>      struct opt_list *opt = opaque;
>>>  
>>> -    if (!g_str_equal(name, opt->name)) {
>>> -        return 0;
>>> +    if (g_str_equal(name, "path")) {
>>> +        g_autoptr(GByteArray) data = g_byte_array_new();
>>> +        g_autofree char *buf = g_new(char, 4096);
>>> +        ssize_t ret;
>>> +        int fd = qemu_open(value, O_RDONLY);
>>
>> While not use g_file_get_contents()?
> 
> qemu_open lets mgmt apps pass in pre-opened FDs using /dev/fdset/NN
> syntax.

OK, I was expecting a specific reason from a GLib advocate like you :)

Maybe add a comment on the 'qemu_open()' line? Else one can be tempted
to convert it to g_file_get_contents().

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 1/5] hw/smbios: support loading OEM strings values from a file

[Laszlo Ersek]

On 09/08/20 18:54, Daniel P. Berrangé wrote:
> Some applications want to pass quite large values for the OEM strings
> entries. Rather than having huge strings on the command line, it would
> be better to load them from a file, as supported with -fw_cfg.
> 
> This introduces the "valuefile" parameter allowing for:
> 
>   $ echo -n "thisthing" > mydata.txt
>   $ qemu-system-x86_64 \
>     -smbios type=11,value=something \
>     -smbios type=11,valuefile=mydata.txt \
>     -smbios type=11,value=somemore \
>     ...other args...
> 
> Now in the guest
> 
> $ dmidecide -t 11
> Getting SMBIOS data from sysfs.
> SMBIOS 2.8 present.
> 
> Handle 0x0E00, DMI type 11, 5 bytes
> OEM Strings
> 	String 1: something
> 	String 2: thisthing
> 	String 3: somemore
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  hw/smbios/smbios.c | 72 +++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 59 insertions(+), 13 deletions(-)

(gearing up to test this / look into the edk2 problem, just one question
in passing: could we / would we simplify this with g_file_get_contents()?)

Thanks
Laszlo

> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index 7cc950b41c..8450fad285 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -110,7 +110,7 @@ static struct {
>  
>  static struct {
>      size_t nvalues;
> -    const char **values;
> +    char **values;
>  } type11;
>  
>  static struct {
> @@ -314,6 +314,11 @@ static const QemuOptDesc qemu_smbios_type11_opts[] = {
>          .type = QEMU_OPT_STRING,
>          .help = "OEM string data",
>      },
> +    {
> +        .name = "path",
> +        .type = QEMU_OPT_STRING,
> +        .help = "OEM string data from file",
> +    },
>  };
>  
>  static const QemuOptDesc qemu_smbios_type17_opts[] = {
> @@ -641,6 +646,8 @@ static void smbios_build_type_11_table(void)
>  
>      for (i = 0; i < type11.nvalues; i++) {
>          SMBIOS_TABLE_SET_STR_LIST(11, type11.values[i]);
> +        g_free(type11.values[i]);
> +        type11.values[i] = NULL;
>      }
>  
>      SMBIOS_BUILD_TABLE_POST;
> @@ -940,9 +947,8 @@ static void save_opt(const char **dest, QemuOpts *opts, const char *name)
>  
>  
>  struct opt_list {
> -    const char *name;
>      size_t *ndest;
> -    const char ***dest;
> +    char ***dest;
>  };
>  
>  static int save_opt_one(void *opaque,
> @@ -951,23 +957,61 @@ static int save_opt_one(void *opaque,
>  {
>      struct opt_list *opt = opaque;
>  
> -    if (!g_str_equal(name, opt->name)) {
> -        return 0;
> +    if (g_str_equal(name, "path")) {
> +        g_autoptr(GByteArray) data = g_byte_array_new();
> +        g_autofree char *buf = g_new(char, 4096);
> +        ssize_t ret;
> +        int fd = qemu_open(value, O_RDONLY);
> +        if (fd < 0) {
> +            error_setg(errp, "Unable to open %s: %s", value, strerror(errno));
> +            return -1;
> +        }
> +
> +        while (1) {
> +            ret = read(fd, buf, 4096);
> +            if (ret == 0) {
> +                break;
> +            }
> +            if (ret < 0) {
> +                error_setg(errp, "Unable to read from %s: %s",
> +                           value, strerror(errno));
> +                return -1;
> +            }
> +            if (memchr(buf, '\0', ret)) {
> +                error_setg(errp, "NUL in OEM strings value in %s", value);
> +                return -1;
> +            }
> +            g_byte_array_append(data, (guint8 *)buf, ret);
> +        }
> +
> +        close(fd);
> +
> +        *opt->dest = g_renew(char *, *opt->dest, (*opt->ndest) + 1);
> +        (*opt->dest)[*opt->ndest] = (char *)g_byte_array_free(data,  FALSE);
> +        (*opt->ndest)++;
> +        data = NULL;
> +   } else if (g_str_equal(name, "value")) {
> +        *opt->dest = g_renew(char *, *opt->dest, (*opt->ndest) + 1);
> +        (*opt->dest)[*opt->ndest] = g_strdup(value);
> +        (*opt->ndest)++;
> +    } else if (!g_str_equal(name, "type")) {
> +        error_setg(errp, "Unexpected option %s", name);
> +        return -1;
>      }
>  
> -    *opt->dest = g_renew(const char *, *opt->dest, (*opt->ndest) + 1);
> -    (*opt->dest)[*opt->ndest] = value;
> -    (*opt->ndest)++;
>      return 0;
>  }
>  
> -static void save_opt_list(size_t *ndest, const char ***dest,
> -                          QemuOpts *opts, const char *name)
> +static bool save_opt_list(size_t *ndest, char ***dest, QemuOpts *opts,
> +                          Error **errp)
>  {
>      struct opt_list opt = {
> -        name, ndest, dest,
> +        ndest, dest,
>      };
> -    qemu_opt_foreach(opts, save_opt_one, &opt, NULL);
> +    if (!qemu_opt_foreach(opts, save_opt_one, &opt, errp)) {
> +        return false;
> +    }
> +    return true;
>  }
>  
>  void smbios_entry_add(QemuOpts *opts, Error **errp)
> @@ -1149,7 +1193,9 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
>              if (!qemu_opts_validate(opts, qemu_smbios_type11_opts, errp)) {
>                  return;
>              }
> -            save_opt_list(&type11.nvalues, &type11.values, opts, "value");
> +            if (!save_opt_list(&type11.nvalues, &type11.values, opts, errp)) {
> +                return;
> +            }
>              return;
>          case 17:
>              if (!qemu_opts_validate(opts, qemu_smbios_type17_opts, errp)) {
> 

Re: [PATCH 1/5] hw/smbios: support loading OEM strings values from a file

[Daniel P. Berrangé]

On Wed, Sep 09, 2020 at 10:18:47AM +0200, Laszlo Ersek wrote:
> On 09/08/20 18:54, Daniel P. Berrangé wrote:
> > Some applications want to pass quite large values for the OEM strings
> > entries. Rather than having huge strings on the command line, it would
> > be better to load them from a file, as supported with -fw_cfg.
> > 
> > This introduces the "valuefile" parameter allowing for:
> > 
> >   $ echo -n "thisthing" > mydata.txt
> >   $ qemu-system-x86_64 \
> >     -smbios type=11,value=something \
> >     -smbios type=11,valuefile=mydata.txt \
> >     -smbios type=11,value=somemore \
> >     ...other args...
> > 
> > Now in the guest
> > 
> > $ dmidecide -t 11
> > Getting SMBIOS data from sysfs.
> > SMBIOS 2.8 present.
> > 
> > Handle 0x0E00, DMI type 11, 5 bytes
> > OEM Strings
> > 	String 1: something
> > 	String 2: thisthing
> > 	String 3: somemore
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  hw/smbios/smbios.c | 72 +++++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 59 insertions(+), 13 deletions(-)
> 
> (gearing up to test this / look into the edk2 problem, just one question
> in passing: could we / would we simplify this with g_file_get_contents()?)

Yes, but at the cost of loosing the ability to pass in a pre-opened
FD, which qemu_open allows for.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/5] hw/smbios: support loading OEM strings values from a file

[Daniel P. Berrangé]

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

On Wed, Sep 09, 2020 at 10:18:47AM +0200, Laszlo Ersek wrote:
> On 09/08/20 18:54, Daniel P. Berrangé wrote:
> > Some applications want to pass quite large values for the OEM strings
> > entries. Rather than having huge strings on the command line, it would
> > be better to load them from a file, as supported with -fw_cfg.
> > 
> > This introduces the "valuefile" parameter allowing for:
> > 
> >   $ echo -n "thisthing" > mydata.txt
> >   $ qemu-system-x86_64 \
> >     -smbios type=11,value=something \
> >     -smbios type=11,valuefile=mydata.txt \
> >     -smbios type=11,value=somemore \
> >     ...other args...
> > 
> > Now in the guest
> > 
> > $ dmidecide -t 11
> > Getting SMBIOS data from sysfs.
> > SMBIOS 2.8 present.
> > 
> > Handle 0x0E00, DMI type 11, 5 bytes
> > OEM Strings
> > 	String 1: something
> > 	String 2: thisthing
> > 	String 3: somemore
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  hw/smbios/smbios.c | 72 +++++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 59 insertions(+), 13 deletions(-)
> 
> (gearing up to test this / look into the edk2 problem, just one question
> in passing: could we / would we simplify this with g_file_get_contents()?)

BTW, to test this, I'm doing the following.

See the attached 'make-tiny-initrd.py' script. It expects "busybox" on
the host OS and builds a tiny initrd containing busybox.

It can optionally copy in arbitrary other commands, and shared libraries
they link to.  By default it will launch an interactive shell in the
guest, but you can tell it to run a specific command, after which it
will poweroff.

I want to run dmidecode, so I'm using

 $ make-tiny-image.py --run "dmidecode" dmidecode

which both copies dmidecode into the initrd, and also runs it by
default.

It creates 'tiny-initrd.img'

Then I simply boot the host OS kernel using this initrd.


  ./build/qemu-system-x86_64  \
      -kernel /boot/vmlinuz-5.7.14-200.fc32.x86_64 \
      -initrd tiny-initrd.img
      -append 'console=ttyS0'
      -m 1000
      -serial stdio
      -display none
      -blockdev '{"driver":"file","filename":"/usr/share/OVMF/OVMF_CODE.fd","node-name":"libvirt-pflash0-storage","auto-read-only":true,"discard":"unmap"}'
      -blockdev '{"node-name":"libvirt-pflash0-format","read-only":true,"driver":"raw","file":"libvirt-pflash0-storage"}'
      -blockdev '{"driver":"file","filename":"/home/berrange/src/virt/qemu/OVMF_VARS.fd","node-name":"libvirt-pflash1-storage","auto-read-only":true,"discard":"unmap"}'
      -blockdev '{"node-name":"libvirt-pflash1-format","read-only":false,"driver":"raw","file":"libvirt-pflash1-storage"}'
      -machine pc-q35-4.0,accel=kvm,usb=off,smm=on,dump-guest-core=off,pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-format,smbios-ep=3_0
      -chardev file,path=firmware.log,id=firmwarelog
      -device isa-debugcon,iobase=0x402,chardev=firmwarelog
      -smbios type=11,path=smallfile.txt


I have a file 'bigfile.txt' that contains 14 MB of plain text.

I then create 'smallfile.txt' from this

  $ dd if=bigfile.txt of=littlefile.txt bs=1 count=130863

If count=130863 or smaller than EDK2 succesfully boots the guest.

If count=130864 or larger then AFAICT it gets stuck in EDK2 or
very early boot - the guest OS never runs.

If smbios-ep=2_1 (or is omitted), then the size limit is smaller of course
but QEMU validates that for you.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[-- Attachment #2: make-tiny-image.py --]
[-- Type: text/plain, Size: 3653 bytes --]

#!/usr/bin/env python3

import re
import sys
import glob
import argparse
import os
import os.path
import stat
import subprocess
from tempfile import TemporaryDirectory
from shutil import copy

def make_busybox(tmpdir, runcmd):
    usrsbin = os.path.join(tmpdir, "usr/sbin")
    bin = os.path.join(tmpdir, "bin")
    os.makedirs(usrsbin, exist_ok=True)
    os.makedirs(bin, exist_ok=True)

    busyboxin = "/usr/sbin/busybox"
    busyboxout = os.path.join(tmpdir, usrsbin, "busybox")
    copy(busyboxin, busyboxout)
    subprocess.check_call([busyboxin, "--install", "-s", bin])

    init = os.path.join(tmpdir, "init")
    with open(init, "w") as fh:
        print("""#!/bin/sh

mkdir /proc /sys
mount -t proc none /proc
mount -t sysfs none /sys

mount -n -t tmpfs none /dev
mknod -m 622 /dev/console c 5 1
mknod -m 666 /dev/null c 1 3
mknod -m 666 /dev/zero c 1 5
mknod -m 666 /dev/ptmx c 5 2
mknod -m 666 /dev/tty c 5 0
mknod -m 666 /dev/ttyS0 c 4 64
mknod -m 444 /dev/random c 1 8
mknod -m 444 /dev/urandom c 1 9

%s
poweroff -f
""" % runcmd, file=fh)
    os.chmod(init, stat.S_IRWXU)

def get_deps(binary):
    out = subprocess.check_output(["ldd", binary]).decode("utf8")
    deps = []
    for line in out.split("\n"):
        m = re.search("=> (/[^ ]+)", line)
        if m is not None:
            deps.append(m.group(1))
        else:
            m = re.match("\s*(/[^ ]+)\s+\(.*\)\s*$", line)
            if m is not None:
                deps.append(m.group(1))
    return deps
    
def make_binaries(tmpdir, binaries):
    bindir = os.path.join(tmpdir, "bin")

    seen = {}
    libs = []
    for binary in binaries:
        if binary[0] == '/':
            src = binary
            dst = os.path.join(tmpdir, binary[1:])
        else:
            src = os.path.join("/usr/bin", binary)
            if not os.path.exists(src):
                src = os.path.join("/usr/sbin", binary)
            dst = os.path.join(bindir, binary)
        if os.path.exists(dst):
            os.unlink(dst)
        copy(src, dst)

        libs.extend(get_deps(src))

    while len(libs):
        print("Pass libs")
        todo = libs
        libs = []
        for lib in todo:
            if lib in seen:
                continue

            dir = os.path.dirname(lib)
            libdir = os.path.join(tmpdir, dir[1:])
            os.makedirs(libdir, exist_ok=True)
            dst = os.path.join(tmpdir, lib[1:])
            copy(lib, dst)
            print(lib)
            seen[lib] = True
            libs.extend(get_deps(lib))

                              

def make_image(tmpdir, output, binaries, runcmd):
    make_busybox(tmpdir, runcmd)
    make_binaries(tmpdir, binaries)

    files = glob.iglob(tmpdir + "/**", recursive=True)
    prefix=len(tmpdir) + 1
    files = [f[prefix:] for f in files]
    files = files[1:]
    filelist = "\n".join(files).encode("utf8")

    with open(output, "w") as fh:
        subprocess.run(["cpio", "--quiet", "-o", "-H", "newc"],
                       cwd=tmpdir, input=filelist, stdout=fh)

parser = argparse.ArgumentParser(description='Build a tiny initrd image')
parser.add_argument('--output', default="tiny-initrd.img",
                    help='Filename of output file')
parser.add_argument('--run', default="exec setsid cttyhack /bin/sh",
                    help='Command to execute in guest (default: "exec setsid cttyhack /bin/sh")')
parser.add_argument('binary', nargs="*",
                    help='List of binaries to include')

args = parser.parse_args()

print (args.output)

with TemporaryDirectory(prefix="make-tiny-image") as tmpdir:
    make_image(tmpdir, args.output, args.binary, args.run)

Re: [PATCH 1/5] hw/smbios: support loading OEM strings values from a file

[Laszlo Ersek]

On 09/08/20 18:54, Daniel P. Berrangé wrote:
> Some applications want to pass quite large values for the OEM strings
> entries. Rather than having huge strings on the command line, it would
> be better to load them from a file, as supported with -fw_cfg.
> 
> This introduces the "valuefile" parameter allowing for:

s/valuefile/path/

> 
>   $ echo -n "thisthing" > mydata.txt
>   $ qemu-system-x86_64 \
>     -smbios type=11,value=something \
>     -smbios type=11,valuefile=mydata.txt \

s/valuefile/path/


>     -smbios type=11,value=somemore \
>     ...other args...
> 
> Now in the guest
> 
> $ dmidecide -t 11

s/decide/decode/

(sorry for the piecemeal feedback -- I report these as I run into them)

Thanks!
Laszlo

> Getting SMBIOS data from sysfs.
> SMBIOS 2.8 present.
> 
> Handle 0x0E00, DMI type 11, 5 bytes
> OEM Strings
> 	String 1: something
> 	String 2: thisthing
> 	String 3: somemore
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  hw/smbios/smbios.c | 72 +++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 59 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index 7cc950b41c..8450fad285 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -110,7 +110,7 @@ static struct {
>  
>  static struct {
>      size_t nvalues;
> -    const char **values;
> +    char **values;
>  } type11;
>  
>  static struct {
> @@ -314,6 +314,11 @@ static const QemuOptDesc qemu_smbios_type11_opts[] = {
>          .type = QEMU_OPT_STRING,
>          .help = "OEM string data",
>      },
> +    {
> +        .name = "path",
> +        .type = QEMU_OPT_STRING,
> +        .help = "OEM string data from file",
> +    },
>  };
>  
>  static const QemuOptDesc qemu_smbios_type17_opts[] = {
> @@ -641,6 +646,8 @@ static void smbios_build_type_11_table(void)
>  
>      for (i = 0; i < type11.nvalues; i++) {
>          SMBIOS_TABLE_SET_STR_LIST(11, type11.values[i]);
> +        g_free(type11.values[i]);
> +        type11.values[i] = NULL;
>      }
>  
>      SMBIOS_BUILD_TABLE_POST;
> @@ -940,9 +947,8 @@ static void save_opt(const char **dest, QemuOpts *opts, const char *name)
>  
>  
>  struct opt_list {
> -    const char *name;
>      size_t *ndest;
> -    const char ***dest;
> +    char ***dest;
>  };
>  
>  static int save_opt_one(void *opaque,
> @@ -951,23 +957,61 @@ static int save_opt_one(void *opaque,
>  {
>      struct opt_list *opt = opaque;
>  
> -    if (!g_str_equal(name, opt->name)) {
> -        return 0;
> +    if (g_str_equal(name, "path")) {
> +        g_autoptr(GByteArray) data = g_byte_array_new();
> +        g_autofree char *buf = g_new(char, 4096);
> +        ssize_t ret;
> +        int fd = qemu_open(value, O_RDONLY);
> +        if (fd < 0) {
> +            error_setg(errp, "Unable to open %s: %s", value, strerror(errno));
> +            return -1;
> +        }
> +
> +        while (1) {
> +            ret = read(fd, buf, 4096);
> +            if (ret == 0) {
> +                break;
> +            }
> +            if (ret < 0) {
> +                error_setg(errp, "Unable to read from %s: %s",
> +                           value, strerror(errno));
> +                return -1;
> +            }
> +            if (memchr(buf, '\0', ret)) {
> +                error_setg(errp, "NUL in OEM strings value in %s", value);
> +                return -1;
> +            }
> +            g_byte_array_append(data, (guint8 *)buf, ret);
> +        }
> +
> +        close(fd);
> +
> +        *opt->dest = g_renew(char *, *opt->dest, (*opt->ndest) + 1);
> +        (*opt->dest)[*opt->ndest] = (char *)g_byte_array_free(data,  FALSE);
> +        (*opt->ndest)++;
> +        data = NULL;
> +   } else if (g_str_equal(name, "value")) {
> +        *opt->dest = g_renew(char *, *opt->dest, (*opt->ndest) + 1);
> +        (*opt->dest)[*opt->ndest] = g_strdup(value);
> +        (*opt->ndest)++;
> +    } else if (!g_str_equal(name, "type")) {
> +        error_setg(errp, "Unexpected option %s", name);
> +        return -1;
>      }
>  
> -    *opt->dest = g_renew(const char *, *opt->dest, (*opt->ndest) + 1);
> -    (*opt->dest)[*opt->ndest] = value;
> -    (*opt->ndest)++;
>      return 0;
>  }
>  
> -static void save_opt_list(size_t *ndest, const char ***dest,
> -                          QemuOpts *opts, const char *name)
> +static bool save_opt_list(size_t *ndest, char ***dest, QemuOpts *opts,
> +                          Error **errp)
>  {
>      struct opt_list opt = {
> -        name, ndest, dest,
> +        ndest, dest,
>      };
> -    qemu_opt_foreach(opts, save_opt_one, &opt, NULL);
> +    if (!qemu_opt_foreach(opts, save_opt_one, &opt, errp)) {
> +        return false;
> +    }
> +    return true;
>  }
>  
>  void smbios_entry_add(QemuOpts *opts, Error **errp)
> @@ -1149,7 +1193,9 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
>              if (!qemu_opts_validate(opts, qemu_smbios_type11_opts, errp)) {
>                  return;
>              }
> -            save_opt_list(&type11.nvalues, &type11.values, opts, "value");
> +            if (!save_opt_list(&type11.nvalues, &type11.values, opts, errp)) {
> +                return;
> +            }
>              return;
>          case 17:
>              if (!qemu_opts_validate(opts, qemu_smbios_type17_opts, errp)) {
> 

[PATCH 2/5] hw/smbios: report error if table size is too large

[Daniel P. Berrangé]

The SMBIOS 2.1 entry point uses a uint16 data type for reporting the
total length of the tables. If the user passes -smbios configuration to
QEMU that causes the table size to exceed this limit then various bad
behaviours result, including

 - firmware hangs in an infinite loop
 - firmware triggers a KVM crash on bad memory access
 - firmware silently discards user's SMBIOS data replacing it with
   a generic data set.

Limiting the size to 0xffff in QEMU avoids triggering most of these
problems. There is a remaining bug in SeaBIOS which tries to prepend its
own data for table 0, and does not check whether there is sufficient
space before attempting this.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/smbios/smbios.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 8450fad285..3c87be6c91 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -365,6 +365,13 @@ static void smbios_register_config(void)
 
 opts_init(smbios_register_config);
 
+/*
+ * The SMBIOS 2.1 "structure table length" field in the
+ * entry point uses a 16-bit integer, so we're limited
+ * in total table size
+ */
+#define SMBIOS_21_MAX_TABLES_LEN 0xffff
+
 static void smbios_validate_table(MachineState *ms)
 {
     uint32_t expect_t4_count = smbios_legacy ?
@@ -375,6 +382,13 @@ static void smbios_validate_table(MachineState *ms)
                      expect_t4_count, smbios_type4_count);
         exit(1);
     }
+
+    if (smbios_ep_type == SMBIOS_ENTRY_POINT_21 &&
+        smbios_tables_len > SMBIOS_21_MAX_TABLES_LEN) {
+        error_report("SMBIOS 2.1 table length %zu exceeds %d",
+                     smbios_tables_len, SMBIOS_21_MAX_TABLES_LEN);
+        exit(1);
+    }
 }
 
 
-- 
2.26.2

Re: [PATCH 2/5] hw/smbios: report error if table size is too large

[Philippe Mathieu-Daudé]

On 9/8/20 6:54 PM, Daniel P. Berrangé wrote:
> The SMBIOS 2.1 entry point uses a uint16 data type for reporting the
> total length of the tables. If the user passes -smbios configuration to
> QEMU that causes the table size to exceed this limit then various bad
> behaviours result, including
> 
>  - firmware hangs in an infinite loop
>  - firmware triggers a KVM crash on bad memory access
>  - firmware silently discards user's SMBIOS data replacing it with
>    a generic data set.
> 
> Limiting the size to 0xffff in QEMU avoids triggering most of these
> problems. There is a remaining bug in SeaBIOS which tries to prepend its
> own data for table 0, and does not check whether there is sufficient
> space before attempting this.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  hw/smbios/smbios.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index 8450fad285..3c87be6c91 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -365,6 +365,13 @@ static void smbios_register_config(void)
>  
>  opts_init(smbios_register_config);
>  
> +/*
> + * The SMBIOS 2.1 "structure table length" field in the
> + * entry point uses a 16-bit integer, so we're limited
> + * in total table size
> + */
> +#define SMBIOS_21_MAX_TABLES_LEN 0xffff
> +
>  static void smbios_validate_table(MachineState *ms)
>  {
>      uint32_t expect_t4_count = smbios_legacy ?
> @@ -375,6 +382,13 @@ static void smbios_validate_table(MachineState *ms)
>                       expect_t4_count, smbios_type4_count);
>          exit(1);
>      }
> +
> +    if (smbios_ep_type == SMBIOS_ENTRY_POINT_21 &&
> +        smbios_tables_len > SMBIOS_21_MAX_TABLES_LEN) {
> +        error_report("SMBIOS 2.1 table length %zu exceeds %d",
> +                     smbios_tables_len, SMBIOS_21_MAX_TABLES_LEN);
> +        exit(1);
> +    }
>  }
>  
>  
> 

Re: [PATCH 2/5] hw/smbios: report error if table size is too large

[Igor Mammedov]

On Tue,  8 Sep 2020 17:54:35 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:

> The SMBIOS 2.1 entry point uses a uint16 data type for reporting the
> total length of the tables. If the user passes -smbios configuration to
> QEMU that causes the table size to exceed this limit then various bad
> behaviours result, including
> 
>  - firmware hangs in an infinite loop
>  - firmware triggers a KVM crash on bad memory access
>  - firmware silently discards user's SMBIOS data replacing it with
>    a generic data set.
> 
> Limiting the size to 0xffff in QEMU avoids triggering most of these
> problems. There is a remaining bug in SeaBIOS which tries to prepend its
> own data for table 0, and does not check whether there is sufficient
> space before attempting this.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>


Reviewed-by: Igor Mammedov <imammedo@redhat.com>

even if we not going to add support for large entries,
this patch is good on it's own, so others won't have to
deal with debugging misconfiguration, and get a clear
error instead.

Michael,
Can you take this patch via your tree?


> ---
>  hw/smbios/smbios.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index 8450fad285..3c87be6c91 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -365,6 +365,13 @@ static void smbios_register_config(void)
>  
>  opts_init(smbios_register_config);
>  
> +/*
> + * The SMBIOS 2.1 "structure table length" field in the
> + * entry point uses a 16-bit integer, so we're limited
> + * in total table size
> + */
> +#define SMBIOS_21_MAX_TABLES_LEN 0xffff
> +
>  static void smbios_validate_table(MachineState *ms)
>  {
>      uint32_t expect_t4_count = smbios_legacy ?
> @@ -375,6 +382,13 @@ static void smbios_validate_table(MachineState *ms)
>                       expect_t4_count, smbios_type4_count);
>          exit(1);
>      }
> +
> +    if (smbios_ep_type == SMBIOS_ENTRY_POINT_21 &&
> +        smbios_tables_len > SMBIOS_21_MAX_TABLES_LEN) {
> +        error_report("SMBIOS 2.1 table length %zu exceeds %d",
> +                     smbios_tables_len, SMBIOS_21_MAX_TABLES_LEN);
> +        exit(1);
> +    }
>  }
>  
>  

[PATCH 3/5] qemu-options: document SMBIOS type 11 settings

[Daniel P. Berrangé]

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qemu-options.hx | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/qemu-options.hx b/qemu-options.hx
index b0f020594e..0cd231b164 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2296,6 +2296,8 @@ DEF("smbios", HAS_ARG, QEMU_OPTION_smbios,
     "-smbios type=4[,sock_pfx=str][,manufacturer=str][,version=str][,serial=str]\n"
     "              [,asset=str][,part=str][,max-speed=%d][,current-speed=%d]\n"
     "                specify SMBIOS type 4 fields\n"
+    "-smbios type=11[,value=str][,path=filename]\n"
+    "                specify SMBIOS type 11 fields\n"
     "-smbios type=17[,loc_pfx=str][,bank=str][,manufacturer=str][,serial=str]\n"
     "               [,asset=str][,part=str][,speed=%d]\n"
     "                specify SMBIOS type 17 fields\n",
@@ -2319,6 +2321,45 @@ SRST
 ``-smbios type=4[,sock_pfx=str][,manufacturer=str][,version=str][,serial=str][,asset=str][,part=str]``
     Specify SMBIOS type 4 fields
 
+``-smbios type=11[,value=str][,path=filename]``
+    Specify SMBIOS type 11 fields inline
+
+    This argument can be repeated multiple times, and values are added in the order they are parsed.
+    Applications intending to use OEM strings data are encouraged to use their application name as
+    a prefix for the value string. This facilitates passing information for multiple applications
+    concurrently.
+
+    The ``value=str`` syntax provides the string data inline, while the ``path=filename`` syntax
+    loads data from a file on disk. Note that the file is not permitted to contain any NUL bytes.
+
+    Both the ``value`` and ``path`` options can be repeated multiple times and will be added to
+    the SMBIOS table in the order in which they appear.
+
+    Note that on the x86 architecture, the total size of all SMBIOS tables is limited to 65535
+    bytes. Thus the OEM strings data is not suitable for passing large amounts of data into the
+    guest. Instead it should be used as a indicator to inform the guest where to locate the real
+    data set, for example, by specifying the serial ID of a block device.
+
+    An example passing three strings is
+
+    .. parsed-literal::
+
+        -smbios type=11,value=cloud-init:ds=nocloud-net;s=http://10.10.0.1:8000/,\\
+                        value=anaconda:method=http://dl.fedoraproject.org/pub/fedora/linux/releases/25/x86_64/os,\\
+                        path=/some/file/with/oemstringsdata.txt
+
+    In the guest OS this is visible with the ``dmidecode`` command
+
+     .. parsed-literal::
+
+         $ dmidecode -t 11
+         Handle 0x0E00, DMI type 11, 5 bytes
+         OEM Strings
+              String 1: cloud-init:ds=nocloud-net;s=http://10.10.0.1:8000/
+              String 2: anaconda:method=http://dl.fedoraproject.org/pub/fedora/linux/releases/25/x86_64/os
+              String 3: myapp:some extra data
+
+
 ``-smbios type=17[,loc_pfx=str][,bank=str][,manufacturer=str][,serial=str][,asset=str][,part=str][,speed=%d]``
     Specify SMBIOS type 17 fields
 ERST
-- 
2.26.2

Re: [PATCH 3/5] qemu-options: document SMBIOS type 11 settings

[Philippe Mathieu-Daudé]

On 9/8/20 6:54 PM, Daniel P. Berrangé wrote:
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  qemu-options.hx | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index b0f020594e..0cd231b164 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2296,6 +2296,8 @@ DEF("smbios", HAS_ARG, QEMU_OPTION_smbios,
>      "-smbios type=4[,sock_pfx=str][,manufacturer=str][,version=str][,serial=str]\n"
>      "              [,asset=str][,part=str][,max-speed=%d][,current-speed=%d]\n"
>      "                specify SMBIOS type 4 fields\n"
> +    "-smbios type=11[,value=str][,path=filename]\n"
> +    "                specify SMBIOS type 11 fields\n"
>      "-smbios type=17[,loc_pfx=str][,bank=str][,manufacturer=str][,serial=str]\n"
>      "               [,asset=str][,part=str][,speed=%d]\n"
>      "                specify SMBIOS type 17 fields\n",
> @@ -2319,6 +2321,45 @@ SRST
>  ``-smbios type=4[,sock_pfx=str][,manufacturer=str][,version=str][,serial=str][,asset=str][,part=str]``
>      Specify SMBIOS type 4 fields
>  
> +``-smbios type=11[,value=str][,path=filename]``
> +    Specify SMBIOS type 11 fields inline
> +
> +    This argument can be repeated multiple times, and values are added in the order they are parsed.
> +    Applications intending to use OEM strings data are encouraged to use their application name as
> +    a prefix for the value string. This facilitates passing information for multiple applications
> +    concurrently.
> +
> +    The ``value=str`` syntax provides the string data inline, while the ``path=filename`` syntax
> +    loads data from a file on disk. Note that the file is not permitted to contain any NUL bytes.
> +
> +    Both the ``value`` and ``path`` options can be repeated multiple times and will be added to
> +    the SMBIOS table in the order in which they appear.
> +
> +    Note that on the x86 architecture, the total size of all SMBIOS tables is limited to 65535
> +    bytes. Thus the OEM strings data is not suitable for passing large amounts of data into the
> +    guest. Instead it should be used as a indicator to inform the guest where to locate the real
> +    data set, for example, by specifying the serial ID of a block device.
> +
> +    An example passing three strings is
> +
> +    .. parsed-literal::
> +
> +        -smbios type=11,value=cloud-init:ds=nocloud-net;s=http://10.10.0.1:8000/,\\
> +                        value=anaconda:method=http://dl.fedoraproject.org/pub/fedora/linux/releases/25/x86_64/os,\\
> +                        path=/some/file/with/oemstringsdata.txt
> +
> +    In the guest OS this is visible with the ``dmidecode`` command
> +
> +     .. parsed-literal::
> +
> +         $ dmidecode -t 11
> +         Handle 0x0E00, DMI type 11, 5 bytes
> +         OEM Strings
> +              String 1: cloud-init:ds=nocloud-net;s=http://10.10.0.1:8000/
> +              String 2: anaconda:method=http://dl.fedoraproject.org/pub/fedora/linux/releases/25/x86_64/os
> +              String 3: myapp:some extra data
> +
> +
>  ``-smbios type=17[,loc_pfx=str][,bank=str][,manufacturer=str][,serial=str][,asset=str][,part=str][,speed=%d]``
>      Specify SMBIOS type 17 fields
>  ERST
> 

[PATCH 4/5] hw/smbios: use qapi for SMBIOS entry point type enum

[Daniel P. Berrangé]

This refactoring prepares for exposing the SMBIOS entry point type as a
machine property on x86.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/arm/virt.c                |  2 +-
 hw/i386/pc_piix.c            |  2 +-
 hw/i386/pc_q35.c             |  2 +-
 hw/smbios/smbios.c           |  9 +++++----
 include/hw/firmware/smbios.h |  9 ++-------
 qapi/machine.json            | 12 ++++++++++++
 6 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index acf9bfbece..fd32b10f75 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1450,7 +1450,7 @@ static void virt_build_smbios(VirtMachineState *vms)
 
     smbios_set_defaults("QEMU", product,
                         vmc->smbios_old_sys_ver ? "1.0" : mc->name, false,
-                        true, SMBIOS_ENTRY_POINT_30);
+                        true, SMBIOS_ENTRY_POINT_TYPE_3_0);
 
     smbios_get_tables(MACHINE(vms), NULL, 0, &smbios_tables, &smbios_tables_len,
                       &smbios_anchor, &smbios_anchor_len);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 32b1453e6a..1c5bc6ae6e 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -179,7 +179,7 @@ static void pc_init1(MachineState *machine,
         smbios_set_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)",
                             mc->name, pcmc->smbios_legacy_mode,
                             pcmc->smbios_uuid_encoded,
-                            SMBIOS_ENTRY_POINT_21);
+                            SMBIOS_ENTRY_POINT_TYPE_2_1);
     }
 
     /* allocate ram and load rom/bios */
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 0cb9c18cd4..cc202407c7 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -204,7 +204,7 @@ static void pc_q35_init(MachineState *machine)
         smbios_set_defaults("QEMU", "Standard PC (Q35 + ICH9, 2009)",
                             mc->name, pcmc->smbios_legacy_mode,
                             pcmc->smbios_uuid_encoded,
-                            SMBIOS_ENTRY_POINT_21);
+                            SMBIOS_ENTRY_POINT_TYPE_2_1);
     }
 
     /* allocate ram and load rom/bios */
diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 3c87be6c91..c99c9b01ae 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -61,7 +61,7 @@ uint8_t *smbios_tables;
 size_t smbios_tables_len;
 unsigned smbios_table_max;
 unsigned smbios_table_cnt;
-static SmbiosEntryPointType smbios_ep_type = SMBIOS_ENTRY_POINT_21;
+static SmbiosEntryPointType smbios_ep_type = SMBIOS_ENTRY_POINT_TYPE_2_1;
 
 static SmbiosEntryPoint ep;
 
@@ -383,7 +383,7 @@ static void smbios_validate_table(MachineState *ms)
         exit(1);
     }
 
-    if (smbios_ep_type == SMBIOS_ENTRY_POINT_21 &&
+    if (smbios_ep_type == SMBIOS_ENTRY_POINT_TYPE_2_1 &&
         smbios_tables_len > SMBIOS_21_MAX_TABLES_LEN) {
         error_report("SMBIOS 2.1 table length %zu exceeds %d",
                      smbios_tables_len, SMBIOS_21_MAX_TABLES_LEN);
@@ -831,7 +831,7 @@ void smbios_set_defaults(const char *manufacturer, const char *product,
 static void smbios_entry_point_setup(void)
 {
     switch (smbios_ep_type) {
-    case SMBIOS_ENTRY_POINT_21:
+    case SMBIOS_ENTRY_POINT_TYPE_2_1:
         memcpy(ep.ep21.anchor_string, "_SM_", 4);
         memcpy(ep.ep21.intermediate_anchor_string, "_DMI_", 5);
         ep.ep21.length = sizeof(struct smbios_21_entry_point);
@@ -854,7 +854,7 @@ static void smbios_entry_point_setup(void)
         ep.ep21.structure_table_address = cpu_to_le32(0);
 
         break;
-    case SMBIOS_ENTRY_POINT_30:
+    case SMBIOS_ENTRY_POINT_TYPE_3_0:
         memcpy(ep.ep30.anchor_string, "_SM3_", 5);
         ep.ep30.length = sizeof(struct smbios_30_entry_point);
         ep.ep30.entry_point_revision = 1;
@@ -939,6 +939,7 @@ void smbios_get_tables(MachineState *ms,
     *tables = smbios_tables;
     *tables_len = smbios_tables_len;
     *anchor = (uint8_t *)&ep;
+    g_printerr("Total len %zu\n", smbios_tables_len);
 
     /* calculate length based on anchor string */
     if (!strncmp((char *)&ep, "_SM_", 4)) {
diff --git a/include/hw/firmware/smbios.h b/include/hw/firmware/smbios.h
index 02a0ced0a0..cb1299ad7a 100644
--- a/include/hw/firmware/smbios.h
+++ b/include/hw/firmware/smbios.h
@@ -1,6 +1,8 @@
 #ifndef QEMU_SMBIOS_H
 #define QEMU_SMBIOS_H
 
+#include "qapi/qapi-types-machine.h"
+
 /*
  * SMBIOS Support
  *
@@ -23,13 +25,6 @@ struct smbios_phys_mem_area {
     uint64_t length;
 };
 
-/*
- * SMBIOS spec defined tables
- */
-typedef enum SmbiosEntryPointType {
-    SMBIOS_ENTRY_POINT_21,
-    SMBIOS_ENTRY_POINT_30,
-} SmbiosEntryPointType;
 
 /* SMBIOS Entry Point
  * There are two types of entry points defined in the SMBIOS specification
diff --git a/qapi/machine.json b/qapi/machine.json
index abc6fd0477..a58cf2694f 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -937,3 +937,15 @@
   'data': 'NumaOptions',
   'allow-preconfig': true
 }
+
+##
+# @SmbiosEntryPointType:
+#
+# @2_1: SMBIOS version 2.1
+#
+# @3_0: SMBIOS version 3.0
+#
+# Since: 5.2
+##
+{ 'enum': 'SmbiosEntryPointType',
+  'data': [ '2_1', '3_0' ] }
-- 
2.26.2

Re: [PATCH 4/5] hw/smbios: use qapi for SMBIOS entry point type enum

[Philippe Mathieu-Daudé]

On 9/8/20 6:54 PM, Daniel P. Berrangé wrote:
> This refactoring prepares for exposing the SMBIOS entry point type as a
> machine property on x86.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  hw/arm/virt.c                |  2 +-
>  hw/i386/pc_piix.c            |  2 +-
>  hw/i386/pc_q35.c             |  2 +-
>  hw/smbios/smbios.c           |  9 +++++----
>  include/hw/firmware/smbios.h |  9 ++-------
>  qapi/machine.json            | 12 ++++++++++++
>  6 files changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index acf9bfbece..fd32b10f75 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1450,7 +1450,7 @@ static void virt_build_smbios(VirtMachineState *vms)
>  
>      smbios_set_defaults("QEMU", product,
>                          vmc->smbios_old_sys_ver ? "1.0" : mc->name, false,
> -                        true, SMBIOS_ENTRY_POINT_30);
> +                        true, SMBIOS_ENTRY_POINT_TYPE_3_0);
>  
>      smbios_get_tables(MACHINE(vms), NULL, 0, &smbios_tables, &smbios_tables_len,
>                        &smbios_anchor, &smbios_anchor_len);
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 32b1453e6a..1c5bc6ae6e 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -179,7 +179,7 @@ static void pc_init1(MachineState *machine,
>          smbios_set_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)",
>                              mc->name, pcmc->smbios_legacy_mode,
>                              pcmc->smbios_uuid_encoded,
> -                            SMBIOS_ENTRY_POINT_21);
> +                            SMBIOS_ENTRY_POINT_TYPE_2_1);
>      }
>  
>      /* allocate ram and load rom/bios */
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 0cb9c18cd4..cc202407c7 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -204,7 +204,7 @@ static void pc_q35_init(MachineState *machine)
>          smbios_set_defaults("QEMU", "Standard PC (Q35 + ICH9, 2009)",
>                              mc->name, pcmc->smbios_legacy_mode,
>                              pcmc->smbios_uuid_encoded,
> -                            SMBIOS_ENTRY_POINT_21);
> +                            SMBIOS_ENTRY_POINT_TYPE_2_1);
>      }
>  
>      /* allocate ram and load rom/bios */
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index 3c87be6c91..c99c9b01ae 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -61,7 +61,7 @@ uint8_t *smbios_tables;
>  size_t smbios_tables_len;
>  unsigned smbios_table_max;
>  unsigned smbios_table_cnt;
> -static SmbiosEntryPointType smbios_ep_type = SMBIOS_ENTRY_POINT_21;
> +static SmbiosEntryPointType smbios_ep_type = SMBIOS_ENTRY_POINT_TYPE_2_1;
>  
>  static SmbiosEntryPoint ep;
>  
> @@ -383,7 +383,7 @@ static void smbios_validate_table(MachineState *ms)
>          exit(1);
>      }
>  
> -    if (smbios_ep_type == SMBIOS_ENTRY_POINT_21 &&
> +    if (smbios_ep_type == SMBIOS_ENTRY_POINT_TYPE_2_1 &&
>          smbios_tables_len > SMBIOS_21_MAX_TABLES_LEN) {
>          error_report("SMBIOS 2.1 table length %zu exceeds %d",
>                       smbios_tables_len, SMBIOS_21_MAX_TABLES_LEN);
> @@ -831,7 +831,7 @@ void smbios_set_defaults(const char *manufacturer, const char *product,
>  static void smbios_entry_point_setup(void)
>  {
>      switch (smbios_ep_type) {
> -    case SMBIOS_ENTRY_POINT_21:
> +    case SMBIOS_ENTRY_POINT_TYPE_2_1:
>          memcpy(ep.ep21.anchor_string, "_SM_", 4);
>          memcpy(ep.ep21.intermediate_anchor_string, "_DMI_", 5);
>          ep.ep21.length = sizeof(struct smbios_21_entry_point);
> @@ -854,7 +854,7 @@ static void smbios_entry_point_setup(void)
>          ep.ep21.structure_table_address = cpu_to_le32(0);
>  
>          break;
> -    case SMBIOS_ENTRY_POINT_30:
> +    case SMBIOS_ENTRY_POINT_TYPE_3_0:
>          memcpy(ep.ep30.anchor_string, "_SM3_", 5);
>          ep.ep30.length = sizeof(struct smbios_30_entry_point);
>          ep.ep30.entry_point_revision = 1;
> @@ -939,6 +939,7 @@ void smbios_get_tables(MachineState *ms,
>      *tables = smbios_tables;
>      *tables_len = smbios_tables_len;
>      *anchor = (uint8_t *)&ep;
> +    g_printerr("Total len %zu\n", smbios_tables_len);

This seems to belong to patch 2 of this series:
"hw/smbios: report error if table size is too large"

Otherwise:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>  
>      /* calculate length based on anchor string */
>      if (!strncmp((char *)&ep, "_SM_", 4)) {
> diff --git a/include/hw/firmware/smbios.h b/include/hw/firmware/smbios.h
> index 02a0ced0a0..cb1299ad7a 100644
> --- a/include/hw/firmware/smbios.h
> +++ b/include/hw/firmware/smbios.h
> @@ -1,6 +1,8 @@
>  #ifndef QEMU_SMBIOS_H
>  #define QEMU_SMBIOS_H
>  
> +#include "qapi/qapi-types-machine.h"
> +
>  /*
>   * SMBIOS Support
>   *
> @@ -23,13 +25,6 @@ struct smbios_phys_mem_area {
>      uint64_t length;
>  };
>  
> -/*
> - * SMBIOS spec defined tables
> - */
> -typedef enum SmbiosEntryPointType {
> -    SMBIOS_ENTRY_POINT_21,
> -    SMBIOS_ENTRY_POINT_30,
> -} SmbiosEntryPointType;
>  
>  /* SMBIOS Entry Point
>   * There are two types of entry points defined in the SMBIOS specification
> diff --git a/qapi/machine.json b/qapi/machine.json
> index abc6fd0477..a58cf2694f 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -937,3 +937,15 @@
>    'data': 'NumaOptions',
>    'allow-preconfig': true
>  }
> +
> +##
> +# @SmbiosEntryPointType:
> +#
> +# @2_1: SMBIOS version 2.1
> +#
> +# @3_0: SMBIOS version 3.0
> +#
> +# Since: 5.2
> +##
> +{ 'enum': 'SmbiosEntryPointType',
> +  'data': [ '2_1', '3_0' ] }
> 

Re: [PATCH 4/5] hw/smbios: use qapi for SMBIOS entry point type enum

[Daniel P. Berrangé]

On Tue, Sep 08, 2020 at 08:29:43PM +0200, Philippe Mathieu-Daudé wrote:
> On 9/8/20 6:54 PM, Daniel P. Berrangé wrote:
> > This refactoring prepares for exposing the SMBIOS entry point type as a
> > machine property on x86.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  hw/arm/virt.c                |  2 +-
> >  hw/i386/pc_piix.c            |  2 +-
> >  hw/i386/pc_q35.c             |  2 +-
> >  hw/smbios/smbios.c           |  9 +++++----
> >  include/hw/firmware/smbios.h |  9 ++-------
> >  qapi/machine.json            | 12 ++++++++++++
> >  6 files changed, 22 insertions(+), 14 deletions(-)
> > 
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index acf9bfbece..fd32b10f75 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -1450,7 +1450,7 @@ static void virt_build_smbios(VirtMachineState *vms)
> >  
> >      smbios_set_defaults("QEMU", product,
> >                          vmc->smbios_old_sys_ver ? "1.0" : mc->name, false,
> > -                        true, SMBIOS_ENTRY_POINT_30);
> > +                        true, SMBIOS_ENTRY_POINT_TYPE_3_0);
> >  
> >      smbios_get_tables(MACHINE(vms), NULL, 0, &smbios_tables, &smbios_tables_len,
> >                        &smbios_anchor, &smbios_anchor_len);
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index 32b1453e6a..1c5bc6ae6e 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -179,7 +179,7 @@ static void pc_init1(MachineState *machine,
> >          smbios_set_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)",
> >                              mc->name, pcmc->smbios_legacy_mode,
> >                              pcmc->smbios_uuid_encoded,
> > -                            SMBIOS_ENTRY_POINT_21);
> > +                            SMBIOS_ENTRY_POINT_TYPE_2_1);
> >      }
> >  
> >      /* allocate ram and load rom/bios */
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index 0cb9c18cd4..cc202407c7 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -204,7 +204,7 @@ static void pc_q35_init(MachineState *machine)
> >          smbios_set_defaults("QEMU", "Standard PC (Q35 + ICH9, 2009)",
> >                              mc->name, pcmc->smbios_legacy_mode,
> >                              pcmc->smbios_uuid_encoded,
> > -                            SMBIOS_ENTRY_POINT_21);
> > +                            SMBIOS_ENTRY_POINT_TYPE_2_1);
> >      }
> >  
> >      /* allocate ram and load rom/bios */
> > diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> > index 3c87be6c91..c99c9b01ae 100644
> > --- a/hw/smbios/smbios.c
> > +++ b/hw/smbios/smbios.c
> > @@ -61,7 +61,7 @@ uint8_t *smbios_tables;
> >  size_t smbios_tables_len;
> >  unsigned smbios_table_max;
> >  unsigned smbios_table_cnt;
> > -static SmbiosEntryPointType smbios_ep_type = SMBIOS_ENTRY_POINT_21;
> > +static SmbiosEntryPointType smbios_ep_type = SMBIOS_ENTRY_POINT_TYPE_2_1;
> >  
> >  static SmbiosEntryPoint ep;
> >  
> > @@ -383,7 +383,7 @@ static void smbios_validate_table(MachineState *ms)
> >          exit(1);
> >      }
> >  
> > -    if (smbios_ep_type == SMBIOS_ENTRY_POINT_21 &&
> > +    if (smbios_ep_type == SMBIOS_ENTRY_POINT_TYPE_2_1 &&
> >          smbios_tables_len > SMBIOS_21_MAX_TABLES_LEN) {
> >          error_report("SMBIOS 2.1 table length %zu exceeds %d",
> >                       smbios_tables_len, SMBIOS_21_MAX_TABLES_LEN);
> > @@ -831,7 +831,7 @@ void smbios_set_defaults(const char *manufacturer, const char *product,
> >  static void smbios_entry_point_setup(void)
> >  {
> >      switch (smbios_ep_type) {
> > -    case SMBIOS_ENTRY_POINT_21:
> > +    case SMBIOS_ENTRY_POINT_TYPE_2_1:
> >          memcpy(ep.ep21.anchor_string, "_SM_", 4);
> >          memcpy(ep.ep21.intermediate_anchor_string, "_DMI_", 5);
> >          ep.ep21.length = sizeof(struct smbios_21_entry_point);
> > @@ -854,7 +854,7 @@ static void smbios_entry_point_setup(void)
> >          ep.ep21.structure_table_address = cpu_to_le32(0);
> >  
> >          break;
> > -    case SMBIOS_ENTRY_POINT_30:
> > +    case SMBIOS_ENTRY_POINT_TYPE_3_0:
> >          memcpy(ep.ep30.anchor_string, "_SM3_", 5);
> >          ep.ep30.length = sizeof(struct smbios_30_entry_point);
> >          ep.ep30.entry_point_revision = 1;
> > @@ -939,6 +939,7 @@ void smbios_get_tables(MachineState *ms,
> >      *tables = smbios_tables;
> >      *tables_len = smbios_tables_len;
> >      *anchor = (uint8_t *)&ep;
> > +    g_printerr("Total len %zu\n", smbios_tables_len);
> 
> This seems to belong to patch 2 of this series:
> "hw/smbios: report error if table size is too large"

Actually it doens't belong anywhere. This is debug junk from
investigating EDK2/SeaBIOS flaws

> 
> Otherwise:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 4/5] hw/smbios: use qapi for SMBIOS entry point type enum

[Philippe Mathieu-Daudé]

On 9/8/20 6:54 PM, Daniel P. Berrangé wrote:
> This refactoring prepares for exposing the SMBIOS entry point type as a
> machine property on x86.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  hw/arm/virt.c                |  2 +-
>  hw/i386/pc_piix.c            |  2 +-
>  hw/i386/pc_q35.c             |  2 +-
>  hw/smbios/smbios.c           |  9 +++++----
>  include/hw/firmware/smbios.h |  9 ++-------
>  qapi/machine.json            | 12 ++++++++++++
>  6 files changed, 22 insertions(+), 14 deletions(-)
> 
[...]

> diff --git a/include/hw/firmware/smbios.h b/include/hw/firmware/smbios.h
> index 02a0ced0a0..cb1299ad7a 100644
> --- a/include/hw/firmware/smbios.h
> +++ b/include/hw/firmware/smbios.h
> @@ -1,6 +1,8 @@
>  #ifndef QEMU_SMBIOS_H
>  #define QEMU_SMBIOS_H
>  
> +#include "qapi/qapi-types-machine.h"

Actually to reduce the churn pulled in ...

> +
>  /*
>   * SMBIOS Support
>   *
> @@ -23,13 +25,6 @@ struct smbios_phys_mem_area {
>      uint64_t length;
>  };
>  
> -/*
> - * SMBIOS spec defined tables
> - */
> -typedef enum SmbiosEntryPointType {
> -    SMBIOS_ENTRY_POINT_21,
> -    SMBIOS_ENTRY_POINT_30,
> -} SmbiosEntryPointType;
>  
>  /* SMBIOS Entry Point
>   * There are two types of entry points defined in the SMBIOS specification
> diff --git a/qapi/machine.json b/qapi/machine.json
> index abc6fd0477..a58cf2694f 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -937,3 +937,15 @@
>    'data': 'NumaOptions',
>    'allow-preconfig': true
>  }
> +
> +##
> +# @SmbiosEntryPointType:
> +#
> +# @2_1: SMBIOS version 2.1
> +#
> +# @3_0: SMBIOS version 3.0
> +#
> +# Since: 5.2
> +##
> +{ 'enum': 'SmbiosEntryPointType',
> +  'data': [ '2_1', '3_0' ] }
> 

... this could be a good opportunity to add a new
qapi/firmware-smbios.json (or firmware.json?) and eventually
update docs/interop/firmware.json.

Re: [PATCH 4/5] hw/smbios: use qapi for SMBIOS entry point type enum

[Eduardo Habkost]

On Tue, Sep 08, 2020 at 05:54:37PM +0100, Daniel P. Berrangé wrote:
> This refactoring prepares for exposing the SMBIOS entry point type as a
> machine property on x86.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
[...]
> +##
> +# @SmbiosEntryPointType:
> +#
> +# @2_1: SMBIOS version 2.1
> +#
> +# @3_0: SMBIOS version 3.0
> +#
> +# Since: 5.2
> +##
> +{ 'enum': 'SmbiosEntryPointType',
> +  'data': [ '2_1', '3_0' ] }

Markus, Michael Roth: would it be OK to extend QAPI to accept
dots in enum values?  I would like to reuse Daniel's patch, but I
would prefer to use "2.1" and "3.0" as option values.

-- 
Eduardo

[PATCH 5/5] hw/i386: expose a "smbios_ep" PC machine property

[Daniel P. Berrangé]

The i440fx and Q35 machine types are both hardcoded to use the legacy
SMBIOS 2.1 entry point. This is a sensible conservative choice because
SeaBIOS only supports SMBIOS 2.1

EDK2, however, can also support SMBIOS 3.0 and QEMU already uses this on
the ARM virt machine type.

This adds a property to allow the choice of SMBIOS entry point versions
For example to opt in to version 3.0

   $QEMU -machine q35,smbios_ep=3_0

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/i386/pc.c         | 26 ++++++++++++++++++++++++++
 hw/i386/pc_piix.c    |  2 +-
 hw/i386/pc_q35.c     |  2 +-
 include/hw/i386/pc.h |  3 +++
 4 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index d11daacc23..cfce279eed 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -81,6 +81,7 @@
 #include "hw/mem/nvdimm.h"
 #include "qapi/error.h"
 #include "qapi/qapi-visit-common.h"
+#include "qapi/qapi-visit-machine.h"
 #include "qapi/visitor.h"
 #include "hw/core/cpu.h"
 #include "hw/usb.h"
@@ -1834,6 +1835,23 @@ static void pc_machine_set_pit(Object *obj, bool value, Error **errp)
     pcms->pit_enabled = value;
 }
 
+static void pc_machine_get_smbios_ep(Object *obj, Visitor *v, const char *name,
+                                     void *opaque, Error **errp)
+{
+    PCMachineState *pcms = PC_MACHINE(obj);
+    SmbiosEntryPointType smbios_ep = pcms->smbios_ep;
+
+    visit_type_SmbiosEntryPointType(v, name, &smbios_ep, errp);
+}
+
+static void pc_machine_set_smbios_ep(Object *obj, Visitor *v, const char *name,
+                                     void *opaque, Error **errp)
+{
+    PCMachineState *pcms = PC_MACHINE(obj);
+
+    visit_type_SmbiosEntryPointType(v, name, &pcms->smbios_ep, errp);
+}
+
 static void pc_machine_get_max_ram_below_4g(Object *obj, Visitor *v,
                                             const char *name, void *opaque,
                                             Error **errp)
@@ -1879,6 +1897,8 @@ static void pc_machine_initfn(Object *obj)
     pcms->vmport = ON_OFF_AUTO_OFF;
 #endif /* CONFIG_VMPORT */
     pcms->max_ram_below_4g = 0; /* use default */
+    pcms->smbios_ep = SMBIOS_ENTRY_POINT_TYPE_2_1;
+
     /* acpi build is enabled by default if machine supports it */
     pcms->acpi_build_enabled = PC_MACHINE_GET_CLASS(pcms)->has_acpi_build;
     pcms->smbus_enabled = true;
@@ -2004,6 +2024,12 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
 
     object_class_property_add_bool(oc, PC_MACHINE_PIT,
         pc_machine_get_pit, pc_machine_set_pit);
+
+    object_class_property_add(oc, PC_MACHINE_SMBIOS_EP, "str",
+        pc_machine_get_smbios_ep, pc_machine_set_smbios_ep,
+        NULL, NULL);
+    object_class_property_set_description(oc, PC_MACHINE_SMBIOS_EP,
+        "SMBIOS Entry Point version [v2_1, v3_0]");
 }
 
 static const TypeInfo pc_machine_info = {
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 1c5bc6ae6e..3626e5425f 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -179,7 +179,7 @@ static void pc_init1(MachineState *machine,
         smbios_set_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)",
                             mc->name, pcmc->smbios_legacy_mode,
                             pcmc->smbios_uuid_encoded,
-                            SMBIOS_ENTRY_POINT_TYPE_2_1);
+                            pcms->smbios_ep);
     }
 
     /* allocate ram and load rom/bios */
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index cc202407c7..7cf08fa364 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -204,7 +204,7 @@ static void pc_q35_init(MachineState *machine)
         smbios_set_defaults("QEMU", "Standard PC (Q35 + ICH9, 2009)",
                             mc->name, pcmc->smbios_legacy_mode,
                             pcmc->smbios_uuid_encoded,
-                            SMBIOS_ENTRY_POINT_TYPE_2_1);
+                            pcms->smbios_ep);
     }
 
     /* allocate ram and load rom/bios */
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index fe52e165b2..f5d2ebcb49 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -10,6 +10,7 @@
 
 #include "hw/acpi/acpi_dev_interface.h"
 #include "hw/hotplug.h"
+#include "hw/firmware/smbios.h"
 
 #define HPET_INTCAP "hpet-intcap"
 
@@ -38,6 +39,7 @@ struct PCMachineState {
     /* Configuration options: */
     uint64_t max_ram_below_4g;
     OnOffAuto vmport;
+    SmbiosEntryPointType smbios_ep;
 
     bool acpi_build_enabled;
     bool smbus_enabled;
@@ -59,6 +61,7 @@ struct PCMachineState {
 #define PC_MACHINE_SMBUS            "smbus"
 #define PC_MACHINE_SATA             "sata"
 #define PC_MACHINE_PIT              "pit"
+#define PC_MACHINE_SMBIOS_EP        "smbios-ep"
 
 /**
  * PCMachineClass:
-- 
2.26.2

Re: [PATCH 5/5] hw/i386: expose a "smbios_ep" PC machine property

[Philippe Mathieu-Daudé]

On 9/8/20 6:54 PM, Daniel P. Berrangé wrote:
> The i440fx and Q35 machine types are both hardcoded to use the legacy
> SMBIOS 2.1 entry point. This is a sensible conservative choice because
> SeaBIOS only supports SMBIOS 2.1
> 
> EDK2, however, can also support SMBIOS 3.0 and QEMU already uses this on
> the ARM virt machine type.
> 
> This adds a property to allow the choice of SMBIOS entry point versions
> For example to opt in to version 3.0
> 
>    $QEMU -machine q35,smbios_ep=3_0
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  hw/i386/pc.c         | 26 ++++++++++++++++++++++++++
>  hw/i386/pc_piix.c    |  2 +-
>  hw/i386/pc_q35.c     |  2 +-
>  include/hw/i386/pc.h |  3 +++
>  4 files changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index d11daacc23..cfce279eed 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -81,6 +81,7 @@
>  #include "hw/mem/nvdimm.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-visit-common.h"
> +#include "qapi/qapi-visit-machine.h"
>  #include "qapi/visitor.h"
>  #include "hw/core/cpu.h"
>  #include "hw/usb.h"
> @@ -1834,6 +1835,23 @@ static void pc_machine_set_pit(Object *obj, bool value, Error **errp)
>      pcms->pit_enabled = value;
>  }
>  
> +static void pc_machine_get_smbios_ep(Object *obj, Visitor *v, const char *name,
> +                                     void *opaque, Error **errp)
> +{
> +    PCMachineState *pcms = PC_MACHINE(obj);
> +    SmbiosEntryPointType smbios_ep = pcms->smbios_ep;
> +
> +    visit_type_SmbiosEntryPointType(v, name, &smbios_ep, errp);
> +}
> +
> +static void pc_machine_set_smbios_ep(Object *obj, Visitor *v, const char *name,
> +                                     void *opaque, Error **errp)
> +{
> +    PCMachineState *pcms = PC_MACHINE(obj);
> +
> +    visit_type_SmbiosEntryPointType(v, name, &pcms->smbios_ep, errp);
> +}
> +
>  static void pc_machine_get_max_ram_below_4g(Object *obj, Visitor *v,
>                                              const char *name, void *opaque,
>                                              Error **errp)
> @@ -1879,6 +1897,8 @@ static void pc_machine_initfn(Object *obj)
>      pcms->vmport = ON_OFF_AUTO_OFF;
>  #endif /* CONFIG_VMPORT */
>      pcms->max_ram_below_4g = 0; /* use default */
> +    pcms->smbios_ep = SMBIOS_ENTRY_POINT_TYPE_2_1;
> +
>      /* acpi build is enabled by default if machine supports it */
>      pcms->acpi_build_enabled = PC_MACHINE_GET_CLASS(pcms)->has_acpi_build;
>      pcms->smbus_enabled = true;
> @@ -2004,6 +2024,12 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>  
>      object_class_property_add_bool(oc, PC_MACHINE_PIT,
>          pc_machine_get_pit, pc_machine_set_pit);
> +
> +    object_class_property_add(oc, PC_MACHINE_SMBIOS_EP, "str",
> +        pc_machine_get_smbios_ep, pc_machine_set_smbios_ep,
> +        NULL, NULL);
> +    object_class_property_set_description(oc, PC_MACHINE_SMBIOS_EP,
> +        "SMBIOS Entry Point version [v2_1, v3_0]");
>  }
>  
>  static const TypeInfo pc_machine_info = {
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 1c5bc6ae6e..3626e5425f 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -179,7 +179,7 @@ static void pc_init1(MachineState *machine,
>          smbios_set_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)",
>                              mc->name, pcmc->smbios_legacy_mode,
>                              pcmc->smbios_uuid_encoded,
> -                            SMBIOS_ENTRY_POINT_TYPE_2_1);
> +                            pcms->smbios_ep);
>      }
>  
>      /* allocate ram and load rom/bios */
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index cc202407c7..7cf08fa364 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -204,7 +204,7 @@ static void pc_q35_init(MachineState *machine)
>          smbios_set_defaults("QEMU", "Standard PC (Q35 + ICH9, 2009)",
>                              mc->name, pcmc->smbios_legacy_mode,
>                              pcmc->smbios_uuid_encoded,
> -                            SMBIOS_ENTRY_POINT_TYPE_2_1);
> +                            pcms->smbios_ep);
>      }
>  
>      /* allocate ram and load rom/bios */
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index fe52e165b2..f5d2ebcb49 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -10,6 +10,7 @@
>  
>  #include "hw/acpi/acpi_dev_interface.h"
>  #include "hw/hotplug.h"
> +#include "hw/firmware/smbios.h"
>  
>  #define HPET_INTCAP "hpet-intcap"
>  
> @@ -38,6 +39,7 @@ struct PCMachineState {
>      /* Configuration options: */
>      uint64_t max_ram_below_4g;
>      OnOffAuto vmport;
> +    SmbiosEntryPointType smbios_ep;
>  
>      bool acpi_build_enabled;
>      bool smbus_enabled;
> @@ -59,6 +61,7 @@ struct PCMachineState {
>  #define PC_MACHINE_SMBUS            "smbus"
>  #define PC_MACHINE_SATA             "sata"
>  #define PC_MACHINE_PIT              "pit"
> +#define PC_MACHINE_SMBIOS_EP        "smbios-ep"
>  
>  /**
>   * PCMachineClass:
> 

Re: [PATCH 5/5] hw/i386: expose a "smbios_ep" PC machine property

[Laszlo Ersek]

On 09/08/20 18:54, Daniel P. Berrangé wrote:
> The i440fx and Q35 machine types are both hardcoded to use the legacy
> SMBIOS 2.1 entry point. This is a sensible conservative choice because
> SeaBIOS only supports SMBIOS 2.1
> 
> EDK2, however, can also support SMBIOS 3.0 and QEMU already uses this on
> the ARM virt machine type.
> 
> This adds a property to allow the choice of SMBIOS entry point versions
> For example to opt in to version 3.0
> 
>    $QEMU -machine q35,smbios_ep=3_0

s/smbios_ep/smbios-ep/

Thanks
Laszlo

> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  hw/i386/pc.c         | 26 ++++++++++++++++++++++++++
>  hw/i386/pc_piix.c    |  2 +-
>  hw/i386/pc_q35.c     |  2 +-
>  include/hw/i386/pc.h |  3 +++
>  4 files changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index d11daacc23..cfce279eed 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -81,6 +81,7 @@
>  #include "hw/mem/nvdimm.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-visit-common.h"
> +#include "qapi/qapi-visit-machine.h"
>  #include "qapi/visitor.h"
>  #include "hw/core/cpu.h"
>  #include "hw/usb.h"
> @@ -1834,6 +1835,23 @@ static void pc_machine_set_pit(Object *obj, bool value, Error **errp)
>      pcms->pit_enabled = value;
>  }
>  
> +static void pc_machine_get_smbios_ep(Object *obj, Visitor *v, const char *name,
> +                                     void *opaque, Error **errp)
> +{
> +    PCMachineState *pcms = PC_MACHINE(obj);
> +    SmbiosEntryPointType smbios_ep = pcms->smbios_ep;
> +
> +    visit_type_SmbiosEntryPointType(v, name, &smbios_ep, errp);
> +}
> +
> +static void pc_machine_set_smbios_ep(Object *obj, Visitor *v, const char *name,
> +                                     void *opaque, Error **errp)
> +{
> +    PCMachineState *pcms = PC_MACHINE(obj);
> +
> +    visit_type_SmbiosEntryPointType(v, name, &pcms->smbios_ep, errp);
> +}
> +
>  static void pc_machine_get_max_ram_below_4g(Object *obj, Visitor *v,
>                                              const char *name, void *opaque,
>                                              Error **errp)
> @@ -1879,6 +1897,8 @@ static void pc_machine_initfn(Object *obj)
>      pcms->vmport = ON_OFF_AUTO_OFF;
>  #endif /* CONFIG_VMPORT */
>      pcms->max_ram_below_4g = 0; /* use default */
> +    pcms->smbios_ep = SMBIOS_ENTRY_POINT_TYPE_2_1;
> +
>      /* acpi build is enabled by default if machine supports it */
>      pcms->acpi_build_enabled = PC_MACHINE_GET_CLASS(pcms)->has_acpi_build;
>      pcms->smbus_enabled = true;
> @@ -2004,6 +2024,12 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>  
>      object_class_property_add_bool(oc, PC_MACHINE_PIT,
>          pc_machine_get_pit, pc_machine_set_pit);
> +
> +    object_class_property_add(oc, PC_MACHINE_SMBIOS_EP, "str",
> +        pc_machine_get_smbios_ep, pc_machine_set_smbios_ep,
> +        NULL, NULL);
> +    object_class_property_set_description(oc, PC_MACHINE_SMBIOS_EP,
> +        "SMBIOS Entry Point version [v2_1, v3_0]");
>  }
>  
>  static const TypeInfo pc_machine_info = {
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 1c5bc6ae6e..3626e5425f 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -179,7 +179,7 @@ static void pc_init1(MachineState *machine,
>          smbios_set_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)",
>                              mc->name, pcmc->smbios_legacy_mode,
>                              pcmc->smbios_uuid_encoded,
> -                            SMBIOS_ENTRY_POINT_TYPE_2_1);
> +                            pcms->smbios_ep);
>      }
>  
>      /* allocate ram and load rom/bios */
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index cc202407c7..7cf08fa364 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -204,7 +204,7 @@ static void pc_q35_init(MachineState *machine)
>          smbios_set_defaults("QEMU", "Standard PC (Q35 + ICH9, 2009)",
>                              mc->name, pcmc->smbios_legacy_mode,
>                              pcmc->smbios_uuid_encoded,
> -                            SMBIOS_ENTRY_POINT_TYPE_2_1);
> +                            pcms->smbios_ep);
>      }
>  
>      /* allocate ram and load rom/bios */
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index fe52e165b2..f5d2ebcb49 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -10,6 +10,7 @@
>  
>  #include "hw/acpi/acpi_dev_interface.h"
>  #include "hw/hotplug.h"
> +#include "hw/firmware/smbios.h"
>  
>  #define HPET_INTCAP "hpet-intcap"
>  
> @@ -38,6 +39,7 @@ struct PCMachineState {
>      /* Configuration options: */
>      uint64_t max_ram_below_4g;
>      OnOffAuto vmport;
> +    SmbiosEntryPointType smbios_ep;
>  
>      bool acpi_build_enabled;
>      bool smbus_enabled;
> @@ -59,6 +61,7 @@ struct PCMachineState {
>  #define PC_MACHINE_SMBUS            "smbus"
>  #define PC_MACHINE_SATA             "sata"
>  #define PC_MACHINE_PIT              "pit"
> +#define PC_MACHINE_SMBIOS_EP        "smbios-ep"
>  
>  /**
>   * PCMachineClass:
> 

Re: [PATCH 0/5] Add support for loading SMBIOS OEM strings from a file

[Laszlo Ersek]

On 09/08/20 18:54, Daniel P. Berrangé wrote:
> I previously added support for SMBIOS OEM strings tables but only
> allowed for data to be passed inline. Potential users indicated they
> wanted to pass some quite large data blobs which is inconvenient todo
> inline. Thus I'm adding support for passing the data from a file.
>
> In testing this I discovered the hard way that on x86 we're limited to
> using the SMBIOS 2.1 entry point currently. This has a maximum size of
> 0xffff, and if you exceed this all sorts of wierd behaviour happens.
>
> QEMU forces SMBIOS 2.1 on x86 because the default SeaBIOS firmware
> does not support SMBIOS 3.0. The EDK2 firmware supports SMBIOS 3.0 and
> QEMU defaults to this on the ARM virt machine type.
>
> This series adds support for checking the SMBIOS 2.1 limits to protect
> users from impossible to diagnose problems.
>
> There is also a fix needed to SeaBIOS which fails to check for
> integer overflow when it appends the type 0 table.
>
>   https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/thread/3EMIOY6YS6MG5UQN3JJJS2A3DJZOVFR6/
>
> IIUC, SMBIOS 3.0 should onlky be limited by what you can fit into RAM,
> but in testing, EDK2 appears to hang shortly after the SMBIOS 3.0 data
> size exceeds 128 KB. I've not spotted an obvious flaw in EDK2 or QEMU,
> nor do I attempt to enforce a limit in QEMU for SMBIOS 3.0.

Under UEFI, the SMBIOS-2 and SMBIOS-3 entry points ("anchors") are
represented as "UEFI configuration tables", with GUIDs that are defined
by the UEFI spec. This is not complex; I guess the simplest explanation
I can give is the BiosTablesTestMain() function in

  tests/uefi-test-tools/UefiTestToolsPkg/BiosTablesTest/BiosTablesTest.c

That UEFI application is independent of this use case -- it is a
guest-side helper utility for

  tests/qtest/bios-tables-test.c

only it comes real handy now, for explaining how the SMBIOS anchors are
located on UEFI.

On a UEFI system, the SMBIOS-2 and SMBIOS-3 entry points (and their
associated table sets) may *both* exist, at the same time. The
ArmVirtQemu firmware is configured to produce only SMBIOS-3. OVMF is
configured to produce both SMBIOS-2 and SMBIOS-3. It's actually more
precise to call these tables "32-bit" and "64-bit".

Notably, the version fields in the 32-bit entry point, such as "SMBIOS
Major Version", "SMBIOS Minor Version", and "SMBIOS BCD Revision", may
refer to versions 3.x+ of the SMBIOS specification.

What OVMF does is, it fetches the SMBIOS payload from QEMU over fw_cfg.
The SMBIOS entry point(s) and tables that the OS will see are not under
the direct control of OVMF. Those artifacts are managed by the
platform-independent driver

  MdeModulePkg/Universal/SmbiosDxe/

which publishes its services through EFI_SMBIOS_PROTOCOL. The protocol
doesn't allow callers to manage the entry points directly, only to add
tables, update strings, iterate over existent tables, and remove tables.
The entry point(s) are installed internally to the driver.

What the platform (OVMF) can do however is use the following "side
channels" to communicate with the driver:

- PcdSmbiosEntryPointProvideMethod
- PcdSmbiosVersion
- PcdSmbiosDocRev

(1) "PcdSmbiosEntryPointProvideMethod" is a bitmask; bit#0 decides
whether the 32-bit entry point will be installed, bit#1 decides whether
the 64-bit entry point will be installed. In ArmVirtQemu, we have value
0x2 for this (statically), in OVMF, we have value 0x3 (statically).

(2) "PcdSmbiosVersion" encodes the SMBIOS specification that the
installed entry points will claim compliance with. It determines the
following fields (all of them):
- 32-bit entry point (if any), SMBIOS Major Version
- 32-bit entry point (if any), SMBIOS Minor Version
- 32-bit entry point (if any), SMBIOS BCD Revision
- 64-bit entry point (if any), SMBIOS Major Version
- 64-bit entry point (if any), SMBIOS Minor Version

OVMF (and ArmVirtQemu too) set PcdSmbiosVersion dynamically, in
"OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c", as follows:

(2a) if QEMU offers a 32-bit entry point (anchor) via fw_cfg, then parse
the SMBIOS Major Version / SMBIOS Minor Version fields from that, and
encode them in PcdSmbiosVersion.

(2b) if QEMU offers a 64-bit entry point (anchor) via fw_cfg, then parse
the the SMBIOS Major Version / SMBIOS Minor Version fields from that,
and encode them in PcdSmbiosVersion.

(3) "PcdSmbiosDocRev" is only relevant for when the 64-bit entry point
is enabled via "PcdSmbiosEntryPointProvideMethod". OVMF sets this PCD
dynamically, in case QEMU offers a 64-bit entry point (anchor), namely
from the SMBIOS Docrev field.


When a table is added or a string is updated via EFI_SMBIOS_PROTOCOL,
the "MdeModulePkg/Universal/SmbiosDxe" driver evaluates the validity of
the request against *each enabled* entry point (that is, 32-bit AND/OR
64-bit). The request is then satisfied for *all* entry points that are
technically eligible. (If none of the entry points are capable of
satisfying the request, then the request is rejected.)


OVMF and ArmVirtQemu dissect the fw_cfg payload from QEMU (see
InstallAllStructures() in
"OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c", in edk2), and add the
extracted tables one by one, using EFI_SMBIOS_PROTOCOL.Add(). Finally,
if a Type 0 table has not been provided, then OVMF installs a built-in
(default) Type 0 table.


Accordingly, using the following QEMU options:

  -smbios type=11,path=/tmp/testfile \
  -machine smbios-ep=3_0 \

where /tmp/testfile (195,464 bytes) was generated like this:

  head -c $((128*1024)) /dev/zero \
  | base64 \
  | cat -n \
  | expand \
  > /tmp/testfile

we get the following log, from "MdeModulePkg/Universal/SmbiosDxe":

> SmbiosAdd: Smbios type 1 with size 0x4B is added to 32-bit table
> SmbiosAdd: Smbios type 1 with size 0x4B is added to 64-bit table
> SmbiosCreateTable: Initialize 32-bit entry point structure
> SmbiosCreateTable() re-allocate SMBIOS 32-bit table
> SmbiosCreateTable: Initialize 64-bit entry point structure
> SmbiosCreate64BitTable() re-allocate SMBIOS 64-bit table
> SmbiosAdd: Smbios type 3 with size 0x27 is added to 32-bit table
> SmbiosAdd: Smbios type 3 with size 0x27 is added to 64-bit table
> SmbiosAdd: Smbios type 4 with size 0x41 is added to 32-bit table
> SmbiosAdd: Smbios type 4 with size 0x41 is added to 64-bit table
> SmbiosAdd: Total length exceeds max 32-bit table length with type = 11 size = 0x300F6
> SmbiosAdd: Smbios type 11 with size 0x300F6 is added to 64-bit table
> SmbiosCreate64BitTable() re-allocate SMBIOS 64-bit table
> SmbiosAdd: Smbios type 16 with size 0x19 is added to 32-bit table
> SmbiosAdd: Smbios type 16 with size 0x19 is added to 64-bit table
> SmbiosAdd: Smbios type 17 with size 0x35 is added to 32-bit table
> SmbiosAdd: Smbios type 17 with size 0x35 is added to 64-bit table
> SmbiosAdd: Smbios type 19 with size 0x21 is added to 32-bit table
> SmbiosAdd: Smbios type 19 with size 0x21 is added to 64-bit table
> SmbiosAdd: Smbios type 19 with size 0x21 is added to 32-bit table
> SmbiosAdd: Smbios type 19 with size 0x21 is added to 64-bit table
> SmbiosAdd: Smbios type 32 with size 0xD is added to 32-bit table
> SmbiosAdd: Smbios type 32 with size 0xD is added to 64-bit table
> SmbiosAdd: Smbios type 0 with size 0x4A is added to 32-bit table
> SmbiosAdd: Smbios type 0 with size 0x4A is added to 64-bit table

Note that the Type 11 table (OEM Strings) is only exposed under the
64-bit entry point.

The firmware is entirely OK with this; I see the boot progress through
shim -> grub -> kernel (the kernel's UEFI stub confirms that UEFI Secure
Boot is enabled, for example).

However, the guest kernel does crash. Using the following (relevant)
kernel params:

  ignore_loglevel earlyprintk=ttyS0,115200n8 console=ttyS0,115200n8 \
  efi=debug

I get the panic below:

> [    0.000000] Linux version 5.3.6-200.fc30.x86_64 (mockbuild@bkernel04.phx2.fedoraproject.org) (gcc version 9.2.1 20190827 (Red Hat 9.2.1-1) (GCC)) #1 SMP Mon Oct 14 13:11:01 UTC 2019
> [    0.000000] Command line: BOOT_IMAGE=(hd0,gpt2)/vmlinuz-5.3.6-200.fc30.x86_64 root=/dev/mapper/fedora_192-root ro resume=/dev/mapper/fedora_192-swap rd.lvm.lv=fedora_192/root rd.lvm.lv=fedora_192/swap ignore_loglevel earlyprintk=ttyS0,115200n8 console=tty0 console=ttyS0,115200n8 no_console_suspend efi=debug video=800x480 earlyprintk=ttyS0,115200n8
> [    0.000000] x86/fpu: Supporting XSAVE feature 0x001: 'x87 floating point registers'
> [    0.000000] x86/fpu: Supporting XSAVE feature 0x002: 'SSE registers'
> [    0.000000] x86/fpu: Supporting XSAVE feature 0x004: 'AVX registers'
> [    0.000000] x86/fpu: xstate_offset[2]:  576, xstate_sizes[2]:  256
> [    0.000000] x86/fpu: Enabled xstate features 0x7, context size is 832 bytes, using 'standard' format.
> [    0.000000] BIOS-provided physical RAM map:
> [    0.000000] BIOS-e820: [mem 0x0000000000000000-0x000000000002ffff] usable
> [    0.000000] BIOS-e820: [mem 0x0000000000030000-0x000000000004ffff] reserved
> [    0.000000] BIOS-e820: [mem 0x0000000000050000-0x000000000009dfff] usable
> [    0.000000] BIOS-e820: [mem 0x000000000009e000-0x000000000009ffff] reserved
> [    0.000000] BIOS-e820: [mem 0x0000000000100000-0x0000000000806fff] usable
> [    0.000000] BIOS-e820: [mem 0x0000000000807000-0x0000000000807fff] ACPI NVS
> [    0.000000] BIOS-e820: [mem 0x0000000000808000-0x000000000080ffff] usable
> [    0.000000] BIOS-e820: [mem 0x0000000000810000-0x000000000170ffff] ACPI NVS
> [    0.000000] BIOS-e820: [mem 0x0000000001710000-0x000000007e728fff] usable
> [    0.000000] BIOS-e820: [mem 0x000000007e729000-0x000000007eb6dfff] reserved
> [    0.000000] BIOS-e820: [mem 0x000000007eb6e000-0x000000007eb7dfff] ACPI data
> [    0.000000] BIOS-e820: [mem 0x000000007eb7e000-0x000000007ebfdfff] ACPI NVS
> [    0.000000] BIOS-e820: [mem 0x000000007ebfe000-0x000000007ef3ffff] usable
> [    0.000000] BIOS-e820: [mem 0x000000007ef40000-0x000000007effffff] ACPI NVS
> [    0.000000] BIOS-e820: [mem 0x000000007f000000-0x000000007fffffff] reserved
> [    0.000000] BIOS-e820: [mem 0x00000000b0000000-0x00000000bfffffff] reserved
> [    0.000000] BIOS-e820: [mem 0x0000000100000000-0x00000001bfffffff] usable
> [    0.000000] printk: debug: ignoring loglevel setting.
> [    0.000000] printk: bootconsole [earlyser0] enabled
> [    0.000000] NX (Execute Disable) protection: active
> [    0.000000] e820: update [mem 0x7c273018-0x7c27ca57] usable ==> usable
> [    0.000000] e820: update [mem 0x7c273018-0x7c27ca57] usable ==> usable
> [    0.000000] extended physical RAM map:
> [    0.000000] reserve setup_data: [mem 0x0000000000000000-0x000000000002ffff] usable
> [    0.000000] reserve setup_data: [mem 0x0000000000030000-0x000000000004ffff] reserved
> [    0.000000] reserve setup_data: [mem 0x0000000000050000-0x000000000009dfff] usable
> [    0.000000] reserve setup_data: [mem 0x000000000009e000-0x000000000009ffff] reserved
> [    0.000000] reserve setup_data: [mem 0x0000000000100000-0x0000000000806fff] usable
> [    0.000000] reserve setup_data: [mem 0x0000000000807000-0x0000000000807fff] ACPI NVS
> [    0.000000] reserve setup_data: [mem 0x0000000000808000-0x000000000080ffff] usable
> [    0.000000] reserve setup_data: [mem 0x0000000000810000-0x000000000170ffff] ACPI NVS
> [    0.000000] reserve setup_data: [mem 0x0000000001710000-0x000000007c273017] usable
> [    0.000000] reserve setup_data: [mem 0x000000007c273018-0x000000007c27ca57] usable
> [    0.000000] reserve setup_data: [mem 0x000000007c27ca58-0x000000007e728fff] usable
> [    0.000000] reserve setup_data: [mem 0x000000007e729000-0x000000007eb6dfff] reserved
> [    0.000000] reserve setup_data: [mem 0x000000007eb6e000-0x000000007eb7dfff] ACPI data
> [    0.000000] reserve setup_data: [mem 0x000000007eb7e000-0x000000007ebfdfff] ACPI NVS
> [    0.000000] reserve setup_data: [mem 0x000000007ebfe000-0x000000007ef3ffff] usable
> [    0.000000] reserve setup_data: [mem 0x000000007ef40000-0x000000007effffff] ACPI NVS
> [    0.000000] reserve setup_data: [mem 0x000000007f000000-0x000000007fffffff] reserved
> [    0.000000] reserve setup_data: [mem 0x00000000b0000000-0x00000000bfffffff] reserved
> [    0.000000] reserve setup_data: [mem 0x0000000100000000-0x00000001bfffffff] usable
> [    0.000000] efi: EFI v2.70 by EDK II
> [    0.000000] efi:  SMBIOS=0x7e954000  SMBIOS 3.0=0x7e952000  ACPI=0x7eb7d000  ACPI 2.0=0x7eb7d014  MEMATTR=0x7cd8b018

(Note: both 32-bit and 64-bit SMBIOS entry points are present)

> [    0.000000] efi: mem00: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000000000000-0x0000000000000fff] (0MB)
> [    0.000000] efi: mem01: [Loader Data        |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000000001000-0x0000000000001fff] (0MB)
> [    0.000000] efi: mem02: [Conventional Memory|   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000000002000-0x000000000002ffff] (0MB)
> [    0.000000] efi: mem03: [Conventional Memory|   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000000050000-0x0000000000086fff] (0MB)
> [    0.000000] efi: mem04: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000000087000-0x0000000000087fff] (0MB)
> [    0.000000] efi: mem05: [Conventional Memory|   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000000088000-0x000000000009dfff] (0MB)
> [    0.000000] efi: mem06: [Reserved           |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000000009e000-0x000000000009ffff] (0MB)
> [    0.000000] efi: mem07: [Conventional Memory|   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000000100000-0x0000000000806fff] (7MB)
> [    0.000000] efi: mem08: [ACPI Memory NVS    |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000000807000-0x0000000000807fff] (0MB)
> [    0.000000] efi: mem09: [Conventional Memory|   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000000808000-0x000000000080ffff] (0MB)
> [    0.000000] efi: mem10: [ACPI Memory NVS    |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000000810000-0x000000000170ffff] (15MB)
> [    0.000000] efi: mem11: [Conventional Memory|   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000001710000-0x0000000001ffffff] (8MB)
> [    0.000000] efi: mem12: [Loader Data        |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000002000000-0x0000000004b4afff] (43MB)
> [    0.000000] efi: mem13: [Conventional Memory|   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000004b4b000-0x0000000057f19fff] (1331MB)
> [    0.000000] efi: mem14: [Loader Data        |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000057f1a000-0x000000005c372fff] (68MB)
> [    0.000000] efi: mem15: [Loader Code        |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000005c373000-0x000000007aefdfff] (491MB)
> [    0.000000] efi: mem16: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007aefe000-0x000000007af1dfff] (0MB)
> [    0.000000] efi: mem17: [Conventional Memory|   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007af1e000-0x000000007bcf2fff] (13MB)
> [    0.000000] efi: mem18: [Loader Code        |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007bcf3000-0x000000007be9bfff] (1MB)
> [    0.000000] efi: mem19: [Loader Data        |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007be9c000-0x000000007c044fff] (1MB)
> [    0.000000] efi: mem20: [Loader Code        |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007c045000-0x000000007c155fff] (1MB)
> [    0.000000] efi: mem21: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007c156000-0x000000007c159fff] (0MB)
> [    0.000000] efi: mem22: [Conventional Memory|   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007c15a000-0x000000007c169fff] (0MB)
> [    0.000000] efi: mem23: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007c16a000-0x000000007c16afff] (0MB)
> [    0.000000] efi: mem24: [Conventional Memory|   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007c16b000-0x000000007c272fff] (1MB)
> [    0.000000] efi: mem25: [Loader Data        |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007c273000-0x000000007c27dfff] (0MB)
> [    0.000000] efi: mem26: [Conventional Memory|   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007c27e000-0x000000007c281fff] (0MB)
> [    0.000000] efi: mem27: [Loader Data        |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007c282000-0x000000007c284fff] (0MB)
> [    0.000000] efi: mem28: [Conventional Memory|   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007c285000-0x000000007c288fff] (0MB)
> [    0.000000] efi: mem29: [Loader Data        |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007c289000-0x000000007c28dfff] (0MB)
> [    0.000000] efi: mem30: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007c28e000-0x000000007c28efff] (0MB)
> [    0.000000] efi: mem31: [Loader Data        |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007c28f000-0x000000007c294fff] (0MB)
> [    0.000000] efi: mem32: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007c295000-0x000000007cd83fff] (10MB)
> [    0.000000] efi: mem33: [Loader Data        |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007cd84000-0x000000007cd84fff] (0MB)
> [    0.000000] efi: mem34: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007cd85000-0x000000007cd86fff] (0MB)
> [    0.000000] efi: mem35: [Loader Data        |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007cd87000-0x000000007cd87fff] (0MB)
> [    0.000000] efi: mem36: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007cd88000-0x000000007cd88fff] (0MB)
> [    0.000000] efi: mem37: [Loader Data        |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007cd89000-0x000000007cd8afff] (0MB)
> [    0.000000] efi: mem38: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007cd8b000-0x000000007cd8cfff] (0MB)
> [    0.000000] efi: mem39: [Loader Data        |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007cd8d000-0x000000007cd8dfff] (0MB)
> [    0.000000] efi: mem40: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007cd8e000-0x000000007cd98fff] (0MB)
> [    0.000000] efi: mem41: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007cd99000-0x000000007ceb3fff] (1MB)
> [    0.000000] efi: mem42: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007ceb4000-0x000000007cf02fff] (0MB)
> [    0.000000] efi: mem43: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007cf03000-0x000000007cf4afff] (0MB)
> [    0.000000] efi: mem44: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007cf4b000-0x000000007cf4dfff] (0MB)
> [    0.000000] efi: mem45: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007cf4e000-0x000000007cf64fff] (0MB)
> [    0.000000] efi: mem46: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007cf65000-0x000000007cf6bfff] (0MB)
> [    0.000000] efi: mem47: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007cf6c000-0x000000007cf98fff] (0MB)
> [    0.000000] efi: mem48: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007cf99000-0x000000007cf9bfff] (0MB)
> [    0.000000] efi: mem49: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007cf9c000-0x000000007d00cfff] (0MB)
> [    0.000000] efi: mem50: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d00d000-0x000000007d015fff] (0MB)
> [    0.000000] efi: mem51: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d016000-0x000000007d058fff] (0MB)
> [    0.000000] efi: mem52: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d059000-0x000000007d05afff] (0MB)
> [    0.000000] efi: mem53: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d05b000-0x000000007d099fff] (0MB)
> [    0.000000] efi: mem54: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d09a000-0x000000007d09cfff] (0MB)
> [    0.000000] efi: mem55: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d09d000-0x000000007d0cefff] (0MB)
> [    0.000000] efi: mem56: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d0cf000-0x000000007d0d3fff] (0MB)
> [    0.000000] efi: mem57: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d0d4000-0x000000007d102fff] (0MB)
> [    0.000000] efi: mem58: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d103000-0x000000007d104fff] (0MB)
> [    0.000000] efi: mem59: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d105000-0x000000007d127fff] (0MB)
> [    0.000000] efi: mem60: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d128000-0x000000007d139fff] (0MB)
> [    0.000000] efi: mem61: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d13a000-0x000000007d153fff] (0MB)
> [    0.000000] efi: mem62: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d154000-0x000000007d154fff] (0MB)
> [    0.000000] efi: mem63: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d155000-0x000000007d172fff] (0MB)
> [    0.000000] efi: mem64: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d173000-0x000000007d178fff] (0MB)
> [    0.000000] efi: mem65: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d179000-0x000000007d19cfff] (0MB)
> [    0.000000] efi: mem66: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d19d000-0x000000007d1a8fff] (0MB)
> [    0.000000] efi: mem67: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d1a9000-0x000000007d1ccfff] (0MB)
> [    0.000000] efi: mem68: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d1cd000-0x000000007d1d3fff] (0MB)
> [    0.000000] efi: mem69: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d1d4000-0x000000007d1f1fff] (0MB)
> [    0.000000] efi: mem70: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d1f2000-0x000000007d1fcfff] (0MB)
> [    0.000000] efi: mem71: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d1fd000-0x000000007d255fff] (0MB)
> [    0.000000] efi: mem72: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d256000-0x000000007d25cfff] (0MB)
> [    0.000000] efi: mem73: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d25d000-0x000000007d387fff] (1MB)
> [    0.000000] efi: mem74: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d388000-0x000000007d38cfff] (0MB)
> [    0.000000] efi: mem75: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d38d000-0x000000007d396fff] (0MB)
> [    0.000000] efi: mem76: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d397000-0x000000007d3a0fff] (0MB)
> [    0.000000] efi: mem77: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d3a1000-0x000000007d3d2fff] (0MB)
> [    0.000000] efi: mem78: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d3d3000-0x000000007d3d5fff] (0MB)
> [    0.000000] efi: mem79: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d3d6000-0x000000007d3defff] (0MB)
> [    0.000000] efi: mem80: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d3df000-0x000000007d553fff] (1MB)
> [    0.000000] efi: mem81: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d554000-0x000000007d572fff] (0MB)
> [    0.000000] efi: mem82: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d573000-0x000000007d591fff] (0MB)
> [    0.000000] efi: mem83: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d592000-0x000000007d5a3fff] (0MB)
> [    0.000000] efi: mem84: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d5a4000-0x000000007d5aefff] (0MB)
> [    0.000000] efi: mem85: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d5af000-0x000000007d5c0fff] (0MB)
> [    0.000000] efi: mem86: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d5c1000-0x000000007d5cbfff] (0MB)
> [    0.000000] efi: mem87: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d5cc000-0x000000007d5fffff] (0MB)
> [    0.000000] efi: mem88: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d600000-0x000000007d804fff] (2MB)
> [    0.000000] efi: mem89: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d805000-0x000000007d82afff] (0MB)
> [    0.000000] efi: mem90: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d82b000-0x000000007d82bfff] (0MB)
> [    0.000000] efi: mem91: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d82c000-0x000000007d830fff] (0MB)
> [    0.000000] efi: mem92: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d831000-0x000000007d835fff] (0MB)
> [    0.000000] efi: mem93: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d836000-0x000000007d846fff] (0MB)
> [    0.000000] efi: mem94: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d847000-0x000000007d850fff] (0MB)
> [    0.000000] efi: mem95: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d851000-0x000000007d87efff] (0MB)
> [    0.000000] efi: mem96: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007d87f000-0x000000007dc7efff] (4MB)
> [    0.000000] efi: mem97: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007dc7f000-0x000000007dd7bfff] (0MB)
> [    0.000000] efi: mem98: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007dd7c000-0x000000007dd7efff] (0MB)
> [    0.000000] efi: mem99: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007dd7f000-0x000000007dda1fff] (0MB)
> [    0.000000] efi: mem100: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007dda2000-0x000000007dda6fff] (0MB)
> [    0.000000] efi: mem101: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007dda7000-0x000000007ddc6fff] (0MB)
> [    0.000000] efi: mem102: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007ddc7000-0x000000007ddcafff] (0MB)
> [    0.000000] efi: mem103: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007ddcb000-0x000000007ddcffff] (0MB)
> [    0.000000] efi: mem104: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007ddd0000-0x000000007dde6fff] (0MB)
> [    0.000000] efi: mem105: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007dde7000-0x000000007de1bfff] (0MB)
> [    0.000000] efi: mem106: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007de1c000-0x000000007de1cfff] (0MB)
> [    0.000000] efi: mem107: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007de1d000-0x000000007de21fff] (0MB)
> [    0.000000] efi: mem108: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007de22000-0x000000007de23fff] (0MB)
> [    0.000000] efi: mem109: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007de24000-0x000000007de24fff] (0MB)
> [    0.000000] efi: mem110: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007de25000-0x000000007e728fff] (9MB)
> [    0.000000] efi: mem111: [Runtime Data       |RUN|  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007e729000-0x000000007e96afff] (2MB)
> [    0.000000] efi: mem112: [Runtime Code       |RUN|  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007e96b000-0x000000007eaedfff] (1MB)
> [    0.000000] efi: mem113: [Reserved           |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007eaee000-0x000000007eb6dfff] (0MB)
> [    0.000000] efi: mem114: [ACPI Reclaim Memory|   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007eb6e000-0x000000007eb7dfff] (0MB)
> [    0.000000] efi: mem115: [ACPI Memory NVS    |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007eb7e000-0x000000007ebfdfff] (0MB)
> [    0.000000] efi: mem116: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007ebfe000-0x000000007edfffff] (2MB)
> [    0.000000] efi: mem117: [Conventional Memory|   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007ee00000-0x000000007ee29fff] (0MB)
> [    0.000000] efi: mem118: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007ee2a000-0x000000007ee49fff] (0MB)
> [    0.000000] efi: mem119: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007ee4a000-0x000000007eeaefff] (0MB)
> [    0.000000] efi: mem120: [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007eeaf000-0x000000007eeeffff] (0MB)
> [    0.000000] efi: mem121: [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007eef0000-0x000000007ef3ffff] (0MB)
> [    0.000000] efi: mem122: [ACPI Memory NVS    |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007ef40000-0x000000007effffff] (0MB)
> [    0.000000] efi: mem123: [Conventional Memory|   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000100000000-0x00000001bfffffff] (3072MB)
> [    0.000000] efi: mem124: [Reserved           |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000000030000-0x000000000004ffff] (0MB)
> [    0.000000] efi: mem125: [Reserved           |   |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x000000007f000000-0x000000007fffffff] (16MB)
> [    0.000000] efi: mem126: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |UC] range=[0x00000000b0000000-0x00000000bfffffff] (256MB)
> [    0.000000] secureboot: Secure boot enabled
> [    0.000000] Kernel is locked down from EFI secure boot; see man kernel_lockdown.7
> [    0.000000] ------------[ cut here ]------------
> [    0.000000] kernel BUG at arch/x86/kernel/setup.c:264!

BUG hit here

> PANIC: early exception 0x06 IP 10:ffffffff8799951f error 0 cr2 0xffff88818c40eff8
> [    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 5.3.6-200.fc30.x86_64 #1
> [    0.000000] RIP: 0010:extend_brk+0x3b/0x4f
> [    0.000000] Code: 8d 46 ff 75 02 0f 0b 48 85 c6 74 02 0f 0b 48 03 05 76 9e a8 ff 48 f7 de 49 89 c0 49 21 f0 49 8d 04 38 48 3d 00 c0 02 88 76 02 <0f> 0b 48 89 05 58 9e a8 ff 4c 89 c7 31 c0 f3 aa 4c 89 c0 c3 e8 88
> [    0.000000] RSP: 0000:ffffffff87403da8 EFLAGS: 00010006 ORIG_RAX: 0000000000000000
> [    0.000000] RAX: ffffffff8803018d RBX: ffffffff880005dc RCX: 000000000002fb89
> [    0.000000] RDX: 0000000000000031 RSI: fffffffffffffffc RDI: 000000000002fb89
> [    0.000000] RBP: ffffffffff24061f R08: ffffffff88000604 R09: ffffffff87514b40
> [    0.000000] R10: ffffffffffffffff R11: ffffffff88000065 R12: 0000000000000002
> [    0.000000] R13: ffffffff87514b40 R14: 0000000000000002 R15: ffffffffff2400b3
> [    0.000000] FS:  0000000000000000(0000) GS:ffffffff87961000(0000) knlGS:0000000000000000
> [    0.000000] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    0.000000] CR2: ffff88818c40eff8 CR3: 000000018ca10000 CR4: 00000000000406a0
> [    0.000000] Call Trace:
> [    0.000000]  ? dmi_string+0x37/0x52
> [    0.000000]  ? dmi_decode+0x349/0x49c
> [    0.000000]  ? dmi_format_ids.constprop.0+0x128/0x128
> [    0.000000]  ? dmi_decode_table+0xb8/0xd0
> [    0.000000]  ? dmi_format_ids.constprop.0+0x128/0x128
> [    0.000000]  ? dmi_walk_early+0x3d/0x5e
> [    0.000000]  ? dmi_smbios3_present+0x9a/0xe1
> [    0.000000]  ? dmi_setup+0x83/0x264
> [    0.000000]  ? printk+0x58/0x6f
> [    0.000000]  ? setup_arch+0x433/0xcb2
> [    0.000000]  ? start_kernel+0x5e/0x56c
> [    0.000000]  ? secondary_startup_64+0xa4/0xb0

This is from "drivers/firmware/dmi_scan.c". dmi_string() calls
dmi_alloc(), which is an inline function that calls extend_brk() (see
"arch/x86/include/asm/dmi.h").

Then extend_brk() in "arch/x86/kernel/setup.c" hits a BUG (note the line
numbers are from upstream v5.3.6, not from Fedora's "5.3.6-200.fc30") :

    254 void * __init extend_brk(size_t size, size_t align)
    255 {
    256         size_t mask = align - 1;
    257         void *ret;
    258
    259         BUG_ON(_brk_start == 0);
    260         BUG_ON(align & mask);
    261
    262         _brk_end = (_brk_end + mask) & ~mask;
    263         BUG_ON((char *)(_brk_end + size) > __brk_limit);
    264
    265         ret = (void *)_brk_end;
    266         _brk_end += size;
    267
    268         memset(ret, 0, size);
    269
    270         return ret;
    271 }

So we're exceeding "__brk_limit".

... I'm quite getting out of my league here, but "__brk_limit" seems to
be controlled by "brk_reservation" in "arch/x86/kernel/vmlinux.lds.S"...
and ultimately through the RESERVE_BRK() macro:

[arch/x86/include/asm/setup.h]

> /*
>  * Reserve space in the brk section.  The name must be unique within
>  * the file, and somewhat descriptive.  The size is in bytes.  Must be
>  * used at file scope.
>  *
>  * (This uses a temp function to wrap the asm so we can pass it the
>  * size parameter; otherwise we wouldn't be able to.  We can't use a
>  * "section" attribute on a normal variable because it always ends up
>  * being @progbits, which ends up allocating space in the vmlinux
>  * executable.)
>  */
> #define RESERVE_BRK(name,sz)                                            \

OK, so let's see RESERVE_BRK() invocations... The relevant match is
likely the one below:

> arch/x86/kernel/setup.c:RESERVE_BRK(dmi_alloc, 65536);

... Then see kernel commits:

- 6de6cb442e76 ("x86: use brk allocation for DMI", 2009-03-14)

- 796216a57fe4 ("x86: allow extend_brk users to reserve brk space",
2009-03-14)

- e808bae2407a ("x86: Do not reserve brk for DMI if it's not going to be
used", 2010-02-25)

Commit 796216a57fe4 is helpful:

>     Add RESERVE_BRK(name, size) macro to reserve space in the brk
>     area.  This should be a conservative (ie, larger) estimate of
>     how much space might possibly be required from the brk area.
>     Any unused space will be freed, so there's no real downside
>     on making the reservation too large (within limits).

So it seems like the 64K limit could be increased, but still
- it requires guest kernels to be rebuilt,
- it doesn't seem suitable for passing MBs of data (on x86 anyway).

Thanks
Laszlo

Re: [PATCH 0/5] Add support for loading SMBIOS OEM strings from a file

[Daniel P. Berrangé]

On Wed, Sep 09, 2020 at 11:44:40AM +0200, Laszlo Ersek wrote:
> On 09/08/20 18:54, Daniel P. Berrangé wrote:
> > I previously added support for SMBIOS OEM strings tables but only
> > allowed for data to be passed inline. Potential users indicated they
> > wanted to pass some quite large data blobs which is inconvenient todo
> > inline. Thus I'm adding support for passing the data from a file.
> >
> > In testing this I discovered the hard way that on x86 we're limited to
> > using the SMBIOS 2.1 entry point currently. This has a maximum size of
> > 0xffff, and if you exceed this all sorts of wierd behaviour happens.
> >
> > QEMU forces SMBIOS 2.1 on x86 because the default SeaBIOS firmware
> > does not support SMBIOS 3.0. The EDK2 firmware supports SMBIOS 3.0 and
> > QEMU defaults to this on the ARM virt machine type.
> >
> > This series adds support for checking the SMBIOS 2.1 limits to protect
> > users from impossible to diagnose problems.
> >
> > There is also a fix needed to SeaBIOS which fails to check for
> > integer overflow when it appends the type 0 table.
> >
> >   https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/thread/3EMIOY6YS6MG5UQN3JJJS2A3DJZOVFR6/
> >
> > IIUC, SMBIOS 3.0 should onlky be limited by what you can fit into RAM,
> > but in testing, EDK2 appears to hang shortly after the SMBIOS 3.0 data
> > size exceeds 128 KB. I've not spotted an obvious flaw in EDK2 or QEMU,
> > nor do I attempt to enforce a limit in QEMU for SMBIOS 3.0.

snip

> So we're exceeding "__brk_limit".
> 
> ... I'm quite getting out of my league here, but "__brk_limit" seems to
> be controlled by "brk_reservation" in "arch/x86/kernel/vmlinux.lds.S"...
> and ultimately through the RESERVE_BRK() macro:
> 
> [arch/x86/include/asm/setup.h]
> 
> > /*
> >  * Reserve space in the brk section.  The name must be unique within
> >  * the file, and somewhat descriptive.  The size is in bytes.  Must be
> >  * used at file scope.
> >  *
> >  * (This uses a temp function to wrap the asm so we can pass it the
> >  * size parameter; otherwise we wouldn't be able to.  We can't use a
> >  * "section" attribute on a normal variable because it always ends up
> >  * being @progbits, which ends up allocating space in the vmlinux
> >  * executable.)
> >  */
> > #define RESERVE_BRK(name,sz)                                            \
> 
> OK, so let's see RESERVE_BRK() invocations... The relevant match is
> likely the one below:
> 
> > arch/x86/kernel/setup.c:RESERVE_BRK(dmi_alloc, 65536);
> 
> ... Then see kernel commits:
> 
> - 6de6cb442e76 ("x86: use brk allocation for DMI", 2009-03-14)
> 
> - 796216a57fe4 ("x86: allow extend_brk users to reserve brk space",
> 2009-03-14)
> 
> - e808bae2407a ("x86: Do not reserve brk for DMI if it's not going to be
> used", 2010-02-25)
> 
> Commit 796216a57fe4 is helpful:
> 
> >     Add RESERVE_BRK(name, size) macro to reserve space in the brk
> >     area.  This should be a conservative (ie, larger) estimate of
> >     how much space might possibly be required from the brk area.
> >     Any unused space will be freed, so there's no real downside
> >     on making the reservation too large (within limits).
> 
> So it seems like the 64K limit could be increased, but still
> - it requires guest kernels to be rebuilt,
> - it doesn't seem suitable for passing MBs of data (on x86 anyway).

Yeah, this feels like we're just venturing into a bad part of town.
Simplest is probably to just document that applications should never
expect more than 64kb of total SMBIOS data to be viable regardless
of the SMBIOS entry point.

Given this, I'm thinking it might be overkill to even both with
supporting SMBIOS 3.0 for x86, unless it offers some other compelling
benefit over SMBIOS 2.1 that you know of ?

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/5] Add support for loading SMBIOS OEM strings from a file

[Laszlo Ersek]

On 09/09/20 11:50, Daniel P. Berrangé wrote:
> On Wed, Sep 09, 2020 at 11:44:40AM +0200, Laszlo Ersek wrote:
>> On 09/08/20 18:54, Daniel P. Berrangé wrote:
>>> I previously added support for SMBIOS OEM strings tables but only
>>> allowed for data to be passed inline. Potential users indicated they
>>> wanted to pass some quite large data blobs which is inconvenient todo
>>> inline. Thus I'm adding support for passing the data from a file.
>>>
>>> In testing this I discovered the hard way that on x86 we're limited to
>>> using the SMBIOS 2.1 entry point currently. This has a maximum size of
>>> 0xffff, and if you exceed this all sorts of wierd behaviour happens.
>>>
>>> QEMU forces SMBIOS 2.1 on x86 because the default SeaBIOS firmware
>>> does not support SMBIOS 3.0. The EDK2 firmware supports SMBIOS 3.0 and
>>> QEMU defaults to this on the ARM virt machine type.
>>>
>>> This series adds support for checking the SMBIOS 2.1 limits to protect
>>> users from impossible to diagnose problems.
>>>
>>> There is also a fix needed to SeaBIOS which fails to check for
>>> integer overflow when it appends the type 0 table.
>>>
>>>   https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/thread/3EMIOY6YS6MG5UQN3JJJS2A3DJZOVFR6/
>>>
>>> IIUC, SMBIOS 3.0 should onlky be limited by what you can fit into RAM,
>>> but in testing, EDK2 appears to hang shortly after the SMBIOS 3.0 data
>>> size exceeds 128 KB. I've not spotted an obvious flaw in EDK2 or QEMU,
>>> nor do I attempt to enforce a limit in QEMU for SMBIOS 3.0.
> 
> snip
> 
>> So we're exceeding "__brk_limit".
>>
>> ... I'm quite getting out of my league here, but "__brk_limit" seems to
>> be controlled by "brk_reservation" in "arch/x86/kernel/vmlinux.lds.S"...
>> and ultimately through the RESERVE_BRK() macro:
>>
>> [arch/x86/include/asm/setup.h]
>>
>>> /*
>>>  * Reserve space in the brk section.  The name must be unique within
>>>  * the file, and somewhat descriptive.  The size is in bytes.  Must be
>>>  * used at file scope.
>>>  *
>>>  * (This uses a temp function to wrap the asm so we can pass it the
>>>  * size parameter; otherwise we wouldn't be able to.  We can't use a
>>>  * "section" attribute on a normal variable because it always ends up
>>>  * being @progbits, which ends up allocating space in the vmlinux
>>>  * executable.)
>>>  */
>>> #define RESERVE_BRK(name,sz)                                            \
>>
>> OK, so let's see RESERVE_BRK() invocations... The relevant match is
>> likely the one below:
>>
>>> arch/x86/kernel/setup.c:RESERVE_BRK(dmi_alloc, 65536);
>>
>> ... Then see kernel commits:
>>
>> - 6de6cb442e76 ("x86: use brk allocation for DMI", 2009-03-14)
>>
>> - 796216a57fe4 ("x86: allow extend_brk users to reserve brk space",
>> 2009-03-14)
>>
>> - e808bae2407a ("x86: Do not reserve brk for DMI if it's not going to be
>> used", 2010-02-25)
>>
>> Commit 796216a57fe4 is helpful:
>>
>>>     Add RESERVE_BRK(name, size) macro to reserve space in the brk
>>>     area.  This should be a conservative (ie, larger) estimate of
>>>     how much space might possibly be required from the brk area.
>>>     Any unused space will be freed, so there's no real downside
>>>     on making the reservation too large (within limits).
>>
>> So it seems like the 64K limit could be increased, but still
>> - it requires guest kernels to be rebuilt,
>> - it doesn't seem suitable for passing MBs of data (on x86 anyway).
> 
> Yeah, this feels like we're just venturing into a bad part of town.
> Simplest is probably to just document that applications should never
> expect more than 64kb of total SMBIOS data to be viable regardless
> of the SMBIOS entry point.

Sounds OK to me personally.

In your experience, would that limit satisfy (for example) the CoreOS /
Ignition use case?

> Given this, I'm thinking it might be overkill to even both with
> supporting SMBIOS 3.0 for x86, unless it offers some other compelling
> benefit over SMBIOS 2.1 that you know of ?

I think the 32-bit entry point is sufficient for x86.

If memory serves, we only started to care about the 64-bit entry point
for aarch64. See for example

https://github.com/tianocore/edk2/commit/ca6d61b22658

x86 always has RAM under 4GB though.

Thanks
Laszlo

Thanks
Laszlo