Message ID | 20140513150003.GK30030@ERROL.INI.CMU.EDU |
---|---|
State | New |
Headers | show |
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
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...
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
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;