[Qemu-devel] [PATCH] SMBIOS: Update Type 0 struct generator for machines >= 2.1

[Qemu-devel] [PATCH] SMBIOS: Update Type 0 struct generator for machines >= 2.1

[Gabriel L. Somlo]

A type 0 (bios info) smbios structure is only generated if explicitly
requested on the command line. This patch updates the mechanism for
generating this type of structure as follows:

  - convert bios_characteristics field to uin64_t (instead of uint8_t[8])
    as described in the current smbios spec (v2.8)

  - enable "virtual machine" bit in bios_characteristics_extension_bits

  - add command line option to enable "uefi supported" bit in
    bios_characteristics_extension_bits

These updates should make this optional smbios structure more useful when
used with edk2/ovmf. Only pc machines 2.1 and newer are affected, and only
when the user explicitly requests that a type 0 struct be generated.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---

On Mon, May 12, 2014 at 11:20:15PM +0200, Laszlo Ersek wrote:
> I think exposing the bios extension bytes on the qemu
> command line (for saying "virtual machine" and "UEFI supported") is more
> important [...]

I think the "virtual machine" bit shouldn't be negotiable. I added an
option to flip the "uefi" bit from the command line. Also cleaned up
the bios_characteristics field in the process.

Let me know what you all think.

Thanks,
  Gabriel

 hw/i386/smbios.c         | 18 +++++++++++-------
 include/hw/i386/smbios.h |  2 +-
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index 7660718..b91d45e 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -67,7 +67,7 @@ static DECLARE_BITMAP(have_fields_bitmap, SMBIOS_MAX_TYPE+1);
 
 static struct {
     const char *vendor, *version, *date;
-    bool have_major_minor;
+    bool have_major_minor, uefi;
     uint8_t major, minor;
 } type0;
 
@@ -134,6 +134,10 @@ static const QemuOptDesc qemu_smbios_type0_opts[] = {
         .name = "release",
         .type = QEMU_OPT_STRING,
         .help = "revision number",
+    },{
+        .name = "uefi",
+        .type = QEMU_OPT_BOOL,
+        .help = "uefi support",
     },
     { /* end of list */ }
 };
@@ -497,13 +501,12 @@ static void smbios_build_type_0_table(void)
 
     t->bios_rom_size = 0; /* hardcoded in SeaBIOS with FIXME comment */
 
-    /* BIOS characteristics not supported */
-    memset(t->bios_characteristics, 0, 8);
-    t->bios_characteristics[0] = 0x08;
-
-    /* Enable targeted content distribution (needed for SVVP, per SeaBIOS) */
+    t->bios_characteristics = 0x08; /* Not supported */
     t->bios_characteristics_extension_bytes[0] = 0;
-    t->bios_characteristics_extension_bytes[1] = 4;
+    t->bios_characteristics_extension_bytes[1] = 0x14; /* TCD/SVVP | VM */
+    if (type0.uefi) {
+        t->bios_characteristics_extension_bytes[1] |= 0x08; /* |= UEFI */
+    }
 
     if (type0.have_major_minor) {
         t->system_bios_major_release = type0.major;
@@ -977,6 +980,7 @@ void smbios_entry_add(QemuOpts *opts)
             save_opt(&type0.vendor, opts, "vendor");
             save_opt(&type0.version, opts, "version");
             save_opt(&type0.date, opts, "date");
+            type0.uefi = qemu_opt_get_bool(opts, "uefi", false);
 
             val = qemu_opt_get(opts, "release");
             if (val) {
diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h
index 6d854b7..5583f60 100644
--- a/include/hw/i386/smbios.h
+++ b/include/hw/i386/smbios.h
@@ -64,7 +64,7 @@ struct smbios_type_0 {
     uint16_t bios_starting_address_segment;
     uint8_t bios_release_date_str;
     uint8_t bios_rom_size;
-    uint8_t bios_characteristics[8];
+    uint64_t bios_characteristics;
     uint8_t bios_characteristics_extension_bytes[2];
     uint8_t system_bios_major_release;
     uint8_t system_bios_minor_release;
-- 
1.9.0

Re: [Qemu-devel] [PATCH] SMBIOS: Update Type 0 struct generator for machines >= 2.1

[Laszlo Ersek]

On 05/13/14 17:00, Gabriel L. Somlo wrote:
> A type 0 (bios info) smbios structure is only generated if explicitly
> requested on the command line. This patch updates the mechanism for
> generating this type of structure as follows:
> 
>   - convert bios_characteristics field to uin64_t (instead of uint8_t[8])
>     as described in the current smbios spec (v2.8)
> 
>   - enable "virtual machine" bit in bios_characteristics_extension_bits
> 
>   - add command line option to enable "uefi supported" bit in
>     bios_characteristics_extension_bits
> 
> These updates should make this optional smbios structure more useful when
> used with edk2/ovmf. Only pc machines 2.1 and newer are affected, and only
> when the user explicitly requests that a type 0 struct be generated.
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
> 
> On Mon, May 12, 2014 at 11:20:15PM +0200, Laszlo Ersek wrote:
>> I think exposing the bios extension bytes on the qemu
>> command line (for saying "virtual machine" and "UEFI supported") is more
>> important [...]
> 
> I think the "virtual machine" bit shouldn't be negotiable. I added an
> option to flip the "uefi" bit from the command line. Also cleaned up
> the bios_characteristics field in the process.
> 
> Let me know what you all think.
> 
> Thanks,
>   Gabriel
> 
>  hw/i386/smbios.c         | 18 +++++++++++-------
>  include/hw/i386/smbios.h |  2 +-
>  2 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
> index 7660718..b91d45e 100644
> --- a/hw/i386/smbios.c
> +++ b/hw/i386/smbios.c
> @@ -67,7 +67,7 @@ static DECLARE_BITMAP(have_fields_bitmap, SMBIOS_MAX_TYPE+1);
>  
>  static struct {
>      const char *vendor, *version, *date;
> -    bool have_major_minor;
> +    bool have_major_minor, uefi;
>      uint8_t major, minor;
>  } type0;
>  
> @@ -134,6 +134,10 @@ static const QemuOptDesc qemu_smbios_type0_opts[] = {
>          .name = "release",
>          .type = QEMU_OPT_STRING,
>          .help = "revision number",
> +    },{
> +        .name = "uefi",
> +        .type = QEMU_OPT_BOOL,
> +        .help = "uefi support",
>      },
>      { /* end of list */ }
>  };
> @@ -497,13 +501,12 @@ static void smbios_build_type_0_table(void)
>  
>      t->bios_rom_size = 0; /* hardcoded in SeaBIOS with FIXME comment */
>  
> -    /* BIOS characteristics not supported */
> -    memset(t->bios_characteristics, 0, 8);
> -    t->bios_characteristics[0] = 0x08;
> -
> -    /* Enable targeted content distribution (needed for SVVP, per SeaBIOS) */
> +    t->bios_characteristics = 0x08; /* Not supported */
>      t->bios_characteristics_extension_bytes[0] = 0;
> -    t->bios_characteristics_extension_bytes[1] = 4;
> +    t->bios_characteristics_extension_bytes[1] = 0x14; /* TCD/SVVP | VM */
> +    if (type0.uefi) {
> +        t->bios_characteristics_extension_bytes[1] |= 0x08; /* |= UEFI */
> +    }
>  
>      if (type0.have_major_minor) {
>          t->system_bios_major_release = type0.major;
> @@ -977,6 +980,7 @@ void smbios_entry_add(QemuOpts *opts)
>              save_opt(&type0.vendor, opts, "vendor");
>              save_opt(&type0.version, opts, "version");
>              save_opt(&type0.date, opts, "date");
> +            type0.uefi = qemu_opt_get_bool(opts, "uefi", false);
>  
>              val = qemu_opt_get(opts, "release");
>              if (val) {
> diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h
> index 6d854b7..5583f60 100644
> --- a/include/hw/i386/smbios.h
> +++ b/include/hw/i386/smbios.h
> @@ -64,7 +64,7 @@ struct smbios_type_0 {
>      uint16_t bios_starting_address_segment;
>      uint8_t bios_release_date_str;
>      uint8_t bios_rom_size;
> -    uint8_t bios_characteristics[8];
> +    uint64_t bios_characteristics;
>      uint8_t bios_characteristics_extension_bytes[2];
>      uint8_t system_bios_major_release;
>      uint8_t system_bios_minor_release;
> 

The idea and the implementation in this patch seems fine to me (and
thanks for it!), except I object to the conversion of
"bios_characteristics" to uint64_t. I think that will break when you
emulate eg. an x86_64 target (ie. an SMBIOS-consuming, little endian
guest) on a big endian host (where you produce the SMBIOS payload).

If you back out the changes to "bios_characteristics", I'll add my R-b.

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH] SMBIOS: Update Type 0 struct generator for machines >= 2.1

[Gabriel L. Somlo]

On Tue, May 13, 2014 at 05:16:24PM +0200, Laszlo Ersek wrote:
> The idea and the implementation in this patch seems fine to me (and
> thanks for it!), except I object to the conversion of
> "bios_characteristics" to uint64_t. I think that will break when you
> emulate eg. an x86_64 target (ie. an SMBIOS-consuming, little endian
> guest) on a big endian host (where you produce the SMBIOS payload).
> 
> If you back out the changes to "bios_characteristics", I'll add my R-b.

Would it be acceptable if I used

	t->bios_characteristics = cpu_to_le64(0x08);

instead ? The smbios spec pdf does say "QWORD", after all :)

If that's a bad idea for some other reason I haven't figured
out yet, I have no problem backing it out...

Thanks,
--Gabriel

PS Now that you mention emulating x86_64 on a BE machine, I think
there may actually be a few more places that require cpu_to_le*()
wrappers...

Re: [Qemu-devel] [PATCH] SMBIOS: Update Type 0 struct generator for machines >= 2.1

[Laszlo Ersek]

On 05/13/14 17:56, Gabriel L. Somlo wrote:
> On Tue, May 13, 2014 at 05:16:24PM +0200, Laszlo Ersek wrote:
>> The idea and the implementation in this patch seems fine to me (and
>> thanks for it!), except I object to the conversion of
>> "bios_characteristics" to uint64_t. I think that will break when you
>> emulate eg. an x86_64 target (ie. an SMBIOS-consuming, little endian
>> guest) on a big endian host (where you produce the SMBIOS payload).
>>
>> If you back out the changes to "bios_characteristics", I'll add my R-b.
> 
> Would it be acceptable if I used
> 
> 	t->bios_characteristics = cpu_to_le64(0x08);
> 
> instead ? The smbios spec pdf does say "QWORD", after all :)

Fine by me.

> If that's a bad idea for some other reason I haven't figured
> out yet, I have no problem backing it out...
> 
> Thanks,
> --Gabriel
> 
> PS Now that you mention emulating x86_64 on a BE machine, I think
> there may actually be a few more places that require cpu_to_le*()
> wrappers...

Oops... Sorry! :)

BTW can you also consult the seabios list about hardcoding the "virtual
machine" bit? I don't think it should hurt, but early exposure is good.

Thank you
Laszlo