| Message ID | 1394532186.22422.24.camel@nilsson.home.kraxel.org |
|---|---|
| State | New |
| Headers | show |
On Tue, Mar 11, 2014 at 11:03:06AM +0100, Gerd Hoffmann wrote: > Issue #3: Running a diff on the dmidecode output with and without the > patches yields this: 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. > --- dmidecode.master 2014-03-11 10:38:06.799233009 +0100 > +++ dmidecode.smbios 2014-03-11 10:39:36.664377785 +0100 > @@ -1,20 +1,20 @@ > # dmidecode 2.12 > SMBIOS 2.4 present. > -10 structures occupying 304 bytes. > -Table at 0x000F09D0. > +10 structures occupying 351 bytes. > +Table at 0x000F09A0. > > That comes from upgrading some of the tables to newer versions, ok. > > Handle 0x0000, DMI type 0, 24 bytes > BIOS Information > - Vendor: Bochs > - Version: Bochs > - Release Date: 01/01/2011 > + Vendor: QEMU > + Version: pc-i440fx-2.0 > + Release Date: 01/01/2014 > Address: 0xE8000 > Runtime Size: 96 kB > ROM Size: 64 kB > Characteristics: > BIOS characteristics not supported > Targeted content distribution is supported > - BIOS Revision: 1.0 > + BIOS Revision: 0.0 > > 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. 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. [...] > -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? > > Type: Central Processor > Family: Other > - Manufacturer: Bochs > + Manufacturer: QEMU > ID: 63 06 00 00 FD FB 8B 07 > - Version: Not Specified > + Version: pc-i440fx-2.0 > Voltage: Unknown > External Clock: Unknown > > Ok. > > - 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? The 2000 is hardcoded in SeaBIOS (see smbios_init_type_4() ). > -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 > > Same question. I don't see the 0x0003 in smbios_init_type_17() but I think it must come from some hardcode in seabios. -Kevin
From: "Gabriel L. Somlo" <somlo@cmu.edu> 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(-)
On Tue, Mar 11, 2014 at 11:16:16AM -0400, Gabriel L. Somlo wrote: > On Tue, Mar 11, 2014 at 11:03:06AM +0100, Gerd Hoffmann wrote: > > 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) { I wont comment on the value of checkpatch, but you could write it this way if you wanted: int len = value ? strlen(value) : 0; if (len > 0) { [...] > > -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"). That's a seabios bug - I'll create a seabios patch. > 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... I would suggest being "bug for bug" compatible in the first set of patches, and then add patches on top to add the additional functionality. Just my 2 cents. -Kevin
Hi, > > 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. > > 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. The information seabios puts in there isn't correct for OVMF though. type0 on ovmf looks like this: Handle 0x0000, DMI type 0, 24 bytes BIOS Information Vendor: EFI Development Kit II / OVMF Version: 0.1 Release Date: 06/03/2013 Address: 0xE8000 Runtime Size: 96 kB ROM Size: 64 kB Characteristics: BIOS characteristics not supported UEFI is supported System is a virtual machine BIOS Revision: 0.1 At very least the UEFI support bit would have to be different depending on whenever seabios or ovmf is used as firmware ... cheers, Gerd
--- dmidecode.master 2014-03-11 10:38:06.799233009 +0100 +++ dmidecode.smbios 2014-03-11 10:39:36.664377785 +0100 @@ -1,20 +1,20 @@ # dmidecode 2.12 SMBIOS 2.4 present. -10 structures occupying 304 bytes. -Table at 0x000F09D0. +10 structures occupying 351 bytes. +Table at 0x000F09A0. That comes from upgrading some of the tables to newer versions, ok. Handle 0x0000, DMI type 0, 24 bytes BIOS Information - Vendor: Bochs - Version: Bochs - Release Date: 01/01/2011 + Vendor: QEMU + Version: pc-i440fx-2.0 + Release Date: 01/01/2014 Address: 0xE8000 Runtime Size: 96 kB ROM Size: 64 kB Characteristics: BIOS characteristics not supported Targeted content distribution is supported - BIOS Revision: 1.0 + BIOS Revision: 0.0