[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] SMBIOS: Update Type 0 struct generator for mach
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [PATCH] SMBIOS: Update Type 0 struct generator for machines >= 2.1 |
Date: |
Tue, 13 May 2014 17:16:24 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 |
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 <address@hidden>
> ---
>
> 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