[Qemu-devel] [v2 PATCH 00/13] SMBIOS: build full tables in QEMU

[Gabriel L. Somlo]

From: "Gabriel L. Somlo" <address@hidden>

On Tue, Mar 11, 2014 at 11:03:06AM +0100, Gerd Hoffmann wrote:
> > On Thu, Mar 06, 2014 at 10:03:32AM +0100, Gerd Hoffmann wrote:
> > > So, if we manage to get the patches into shape in time for qemu 2.0 your
> > > way to do that is fine.  We are pretty close to the 2.0 freeze though,
> > > so maybe we should better plan for post-2.0 anyway, especially as you
> > > plan to add more tables.
> > 
> > Hopefully it's not too late, and the patches are in good enough shape :)
> 
> I don't feel like rushing it, and hard freeze is tomorrow ...

No big deal for me, I was just trying to help prevent the need to
deal with wn extra compatibility mode, per your earlier explanation...

> Issue #1: There are checkpatch errors (scripts/checkpatch.pl).

I fixed most of those, with one exception (patch #5):

ERROR: do not use assignment in if condition
#139: FILE: hw/i386/smbios.c:253:
+        if (value != NULL && (len = strlen(value) + 1) > 1) { \

'value' can be NULL, "", or "foo". The whole block looks like this:

  int len;
  if (value != NULL && (len = strlen(value)) > 0) {
      smbios_entries = g_realloc(smbios_entries, smbios_entries_len + len);
      ...
  } else {
      ...
  }

So, I can't call strlen before I compare 'value' against NULL, and
nesting a bunch of if statements makes things more complex, and using
goto would also suck worse. I can call strlen() twice (once in the
condition, and again to set 'len' inside the true branch, but that
would also feel a bit cheesy :)

I'd say checkpatch.pl is missing the big picture in this one instance,
what do you think ?  :)

> Issue #2: There is one build warning:
> 
> /home/kraxel/projects/qemu/hw/i386/smbios.c: In function
> 'smbios_build_type_16_table':
> /home/kraxel/projects/qemu/hw/i386/smbios.c:520:5: warning: comparison
> is always true due to limited range of data type [-Wtype-limits]
>      t->maximum_capacity = ram_size < 2ULL << 40 ? ram_size >> 10 :
> 0x80000000;
>      ^

Weird, I wasn't getting that. Might have been a 64-bit arch issue.
Anyhow, I reworked the code to avoid it altogether, and it looks
cleaner and easier to comprehend as a bonus...

> I think we should not generate a type0 table unless -smbios type0=... is
> explicitly specified on the qemu command line.  It is about the
> firmware, and we should leave it to the firmware to fill it by default.
> If you are running OVMF (EFI) instead of SeaBIOS you should see it in
> the dmidecode output.

Agreed. Type 0 is now optional, just like type 2. It won't get added
unless requested explicitly.

> -Handle 0x0401, DMI type 4, 32 bytes
> +Handle 0x0400, DMI type 4, 35 bytes
>  Processor Information
> -     Socket Designation: CPU 1
> +     Socket Designation: CPU 0
> 
> Hmm?

I think SeaBIOS starts counting from 1. Do we want to stick with that,
or count starting from 0 ? I think it's purely cosmetic, but in the
interest of minimizing initial visual noise, I've changed it to count
from 1 in QEMU as well. :)
> 
> -     Max Speed: 2000 MHz
> -     Current Speed: 2000 MHz
> +     Max Speed: Unknown
> +     Current Speed: Unknown
> 
> Where does 2000 MHz come from?  Does SeaBIOS pull something out of thin
> air or does it try to measure the speed?

Thin air, AFAICT. I hardcoded it in QEMU also, to minimize unexpected
changes.

> 
> -Handle 0x1100, DMI type 17, 21 bytes
> +Handle 0x1100, DMI type 17, 27 bytes
>  Memory Device
>       Array Handle: 0x1000
> -     Error Information Handle: 0x0003
> +     Error Information Handle: Not Provided

AFAICT, SeaBIOS doesn't set the error info handle at all, and you simply
get whatever garbage happens to be contained in that memory location at
the time. Typically it's 0x0000, but you happened to get 0x0003 :) I
figured I'd set it to a known value (0xFFFE or "n/a").

On Tue, Mar 11, 2014 at 09:27:31AM -0400, Kevin O'Connor wrote:
> I think it would be best to get the patch series to the point that
> there is no diff between old and new.  That will make review easier,
> and subsequent patches can then add new features.

Modulo the error info handle on type 17, and the fact that QEMU's version
of type 17 has v2.3 fields and SeaBIOS's does not (kinda the whole reason
I'm doing this in the first place :), that should be possible by just
tweaking the defaults in the new smbios_set_defaults() function. I just
feel weird setting "Bochs" as the default manufacturer... 

> Everything that SeaBIOS puts into table 0 is hard coded.  I'd prefer
> it if QEMU created the table (with the same hardcoded fields) because
> having split ownership of the smbios is painful.

Instinctively, I feel Gerd's right, and type 0 is really the BIOS's
jurisdiction, so I made it optional. You'd have to simply do "-smbios
type=0" on the qemu command line, and it would be inserted into fw_cfg.

New patch set follows, please let me know what you think.

Thanks,
--Gabriel


Gabriel L. Somlo (13):
  SMBIOS: Update all table definitions to smbios spec v2.3
  SMBIOS: Rename smbios_set_type1_defaults() for more general use
  SMBIOS: Use macro to set smbios defaults
  SMBIOS: Use bitmaps to check for smbios table collisions
  SMBIOS: Add code to build full smbios tables; build type 2 table
  SMBIOS: Build full tables for types 0 and 1
  SMBIOS: Remove unused code for passing individual fields to bios
  SMBIOS: Build full type 3 table
  SMBIOS: Build full type 4 tables
  SMBIOS: Build full smbios v2.3 compliant type 16 and 17 tables
  SMBIOS: Build full type 19 tables
  SMBIOS: Build full type 20 tables
  SMBIOS: Build full tables for type 32 and 127

 hw/i386/pc.c             |   3 +
 hw/i386/pc_piix.c        |  15 +-
 hw/i386/pc_q35.c         |  11 +-
 hw/i386/smbios.c         | 669 ++++++++++++++++++++++++++++++++++++++++-------
 include/hw/i386/smbios.h |  40 ++-
 5 files changed, 622 insertions(+), 116 deletions(-)

-- 
1.8.1.4