| Message ID | 1376659114-6630-8-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Aug 16, 2013 at 03:18:34PM +0200, armbru@redhat.com wrote: > From: Markus Armbruster <armbru@redhat.com> > > Currently, we get SeaBIOS defaults: manufacturer Bochs, product Bochs, > no version. Best SeaBIOS can do, but we can provide better defaults: > manufacturer QEMU, product & version taken from QEMUMachine desc and > name. > > Take care to do this only for new machine types, of course. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> We can do this of course, but why is this better? It seems to expose new information to the guest for no reason, we just might come to regret it later if guests start implementing hacks keying off the version number. > --- > hw/i386/pc.c | 6 +++--- > hw/i386/pc_piix.c | 5 +++++ > hw/i386/pc_q35.c | 3 +++ > hw/i386/smbios.c | 12 +++++++++++- > include/hw/i386/pc.h | 1 + > include/hw/i386/smbios.h | 2 +- > 6 files changed, 24 insertions(+), 5 deletions(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index e8bc8ce..eb7ffc4 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -604,7 +604,7 @@ static unsigned int pc_apic_id_limit(unsigned int max_cpus) > return x86_cpu_apic_id_from_index(max_cpus - 1) + 1; > } > > -static FWCfgState *bochs_bios_init(void) > +static FWCfgState *bochs_bios_init(bool smbios_type1_defaults) > { > FWCfgState *fw_cfg; > uint8_t *smbios_table; > @@ -635,7 +635,7 @@ static FWCfgState *bochs_bios_init(void) > acpi_tables, acpi_tables_len); > fw_cfg_add_i32(fw_cfg, FW_CFG_IRQ0_OVERRIDE, kvm_allows_irq0_override()); > > - smbios_table = smbios_get_table(&smbios_len); > + smbios_table = smbios_get_table(&smbios_len, smbios_type1_defaults); > if (smbios_table) > fw_cfg_add_bytes(fw_cfg, FW_CFG_SMBIOS_ENTRIES, > smbios_table, smbios_len); > @@ -1155,7 +1155,7 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory, > option_rom_mr, > 1); > > - fw_cfg = bochs_bios_init(); > + fw_cfg = bochs_bios_init(guest_info->smbios_type1_defaults); > rom_set_fw(fw_cfg); > > if (linux_boot) { > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index 6e1e654..2a621ef 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -58,6 +58,7 @@ static const int ide_irq[MAX_IDE_BUS] = { 14, 15 }; > > static bool has_pvpanic; > static bool has_pci_info = true; > +static bool smbios_type1_defaults = true; > > /* PC hardware initialisation */ > static void pc_init1(MemoryRegion *system_memory, > @@ -128,6 +129,7 @@ static void pc_init1(MemoryRegion *system_memory, > guest_info = pc_guest_info_init(below_4g_mem_size, above_4g_mem_size); > guest_info->has_pci_info = has_pci_info; > guest_info->isapc_ram_fw = !pci_enabled; > + guest_info->smbios_type1_defaults = smbios_type1_defaults; > > /* allocate ram and load rom/bios */ > if (!xen_enabled()) { > @@ -258,6 +260,7 @@ static void pc_init_pci_1_6(QEMUMachineInitArgs *args) > static void pc_init_pci_1_5(QEMUMachineInitArgs *args) > { > has_pvpanic = true; > + smbios_type1_defaults = false; > pc_init_pci_1_6(args); > } > > @@ -298,6 +301,7 @@ static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args) > const char *initrd_filename = args->initrd_filename; > const char *boot_device = args->boot_device; > has_pci_info = false; > + smbios_type1_defaults = false; > disable_kvm_pv_eoi(); > enable_compat_apic_id_mode(); > pc_init1(get_system_memory(), > @@ -316,6 +320,7 @@ static void pc_init_isa(QEMUMachineInitArgs *args) > const char *initrd_filename = args->initrd_filename; > const char *boot_device = args->boot_device; > has_pci_info = false; > + smbios_type1_defaults = false; > if (cpu_model == NULL) > cpu_model = "486"; > disable_kvm_pv_eoi(); > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > index 10e770e..05bce55 100644 > --- a/hw/i386/pc_q35.c > +++ b/hw/i386/pc_q35.c > @@ -48,6 +48,7 @@ > > static bool has_pvpanic; > static bool has_pci_info = true; > +static bool smbios_type1_defaults = true; > > /* PC hardware initialisation */ > static void pc_q35_init(QEMUMachineInitArgs *args) > @@ -111,6 +112,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args) > guest_info = pc_guest_info_init(below_4g_mem_size, above_4g_mem_size); > guest_info->has_pci_info = has_pci_info; > guest_info->isapc_ram_fw = false; > + guest_info->smbios_type1_defaults = smbios_type1_defaults; > > /* allocate ram and load rom/bios */ > if (!xen_enabled()) { > @@ -227,6 +229,7 @@ static void pc_q35_init_1_6(QEMUMachineInitArgs *args) > static void pc_q35_init_1_5(QEMUMachineInitArgs *args) > { > has_pvpanic = true; > + smbios_type1_defaults = false; > pc_q35_init_1_6(args); > } > > diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c > index a2eb9bf..e6413a5 100644 > --- a/hw/i386/smbios.c > +++ b/hw/i386/smbios.c > @@ -18,6 +18,7 @@ > #include "qemu/config-file.h" > #include "qemu/error-report.h" > #include "sysemu/sysemu.h" > +#include "hw/boards.h" > #include "hw/i386/smbios.h" > #include "hw/loader.h" > > @@ -256,9 +257,18 @@ static void smbios_build_type_1_fields(void) > } > } > > -uint8_t *smbios_get_table(size_t *length) > +uint8_t *smbios_get_table(size_t *length, bool type1_defaults) > { > if (!smbios_immutable) { > + if (type1_defaults && !type1.manufacturer) { > + type1.manufacturer = "QEMU"; > + } > + if (type1_defaults && !type1.product) { > + type1.product = current_machine->desc; > + } > + if (type1_defaults && !type1.version) { > + type1.version = current_machine->name; > + } > smbios_build_type_0_fields(); > smbios_build_type_1_fields(); > smbios_validate_table(); > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > index f79d478..5797b97 100644 > --- a/include/hw/i386/pc.h > +++ b/include/hw/i386/pc.h > @@ -20,6 +20,7 @@ typedef struct PcPciInfo { > struct PcGuestInfo { > bool has_pci_info; > bool isapc_ram_fw; > + bool smbios_type1_defaults; > FWCfgState *fw_cfg; > }; > > diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h > index b08ec71..e258d9a 100644 > --- a/include/hw/i386/smbios.h > +++ b/include/hw/i386/smbios.h > @@ -16,7 +16,7 @@ > #include "qemu/option.h" > > void smbios_entry_add(QemuOpts *opts); > -uint8_t *smbios_get_table(size_t *length); > +uint8_t *smbios_get_table(size_t *length, bool type1_defaults); > > /* > * SMBIOS spec defined tables > -- > 1.8.1.4 >
On 08/27/2013 11:04 AM, Michael S. Tsirkin wrote: > On Fri, Aug 16, 2013 at 03:18:34PM +0200, armbru@redhat.com wrote: >> From: Markus Armbruster <armbru@redhat.com> >> >> Currently, we get SeaBIOS defaults: manufacturer Bochs, product Bochs, >> no version. Best SeaBIOS can do, but we can provide better defaults: >> manufacturer QEMU, product & version taken from QEMUMachine desc and >> name. >> >> Take care to do this only for new machine types, of course. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> > > We can do this of course, but why is this better? > It seems to expose new information to the guest > for no reason, we just might come to regret it > later if guests start implementing hacks keying > off the version number. Some guests (cough: some OEM builds of windows) already DO key off of SMBIOS information to determine if they are running in a valid environment. Furthermore, making this change to qemu makes it easier to decouple the strings being presented to guests by default; it's always better to have a situation where changing just qemu works, instead of having to patch both qemu and BIOS in tandem. The choice of whether to present the different information MUST be tied to machine types (we cannot change SMBIOS data without an explicit change to a newer machine type, precisely _because_ there are guests that base their licensing decisions on constancy of BIOS information). But for a new machine type, presenting qemu as the machine type rather than being stuck to a particular SeaBIOS build seems nicer.
On Tue, Aug 27, 2013 at 05:22:19PM -0600, Eric Blake wrote: > On 08/27/2013 11:04 AM, Michael S. Tsirkin wrote: > > On Fri, Aug 16, 2013 at 03:18:34PM +0200, armbru@redhat.com wrote: > >> From: Markus Armbruster <armbru@redhat.com> > >> > >> Currently, we get SeaBIOS defaults: manufacturer Bochs, product Bochs, > >> no version. Best SeaBIOS can do, but we can provide better defaults: > >> manufacturer QEMU, product & version taken from QEMUMachine desc and > >> name. > >> > >> Take care to do this only for new machine types, of course. > >> > >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > >> Reviewed-by: Eric Blake <eblake@redhat.com> > > > > We can do this of course, but why is this better? > > It seems to expose new information to the guest > > for no reason, we just might come to regret it > > later if guests start implementing hacks keying > > off the version number. > > Some guests (cough: some OEM builds of windows) already DO key off of > SMBIOS information to determine if they are running in a valid > environment. Well they are unlikely to like either QEMU or Bochs. > Furthermore, making this change to qemu makes it easier to > decouple the strings being presented to guests by default; it's always > better to have a situation where changing just qemu works, instead of > having to patch both qemu and BIOS in tandem. > > The choice of whether to present the different information MUST be tied > to machine types (we cannot change SMBIOS data without an explicit > change to a newer machine type, precisely _because_ there are guests > that base their licensing decisions on constancy of BIOS information). > But for a new machine type, presenting qemu as the machine type rather > than being stuck to a particular SeaBIOS build seems nicer. Yes but why change anything at all? We have command line flags to set these fields for whatever you want to boot OEM windows - which is highly unlikely to match any QEMU version. Why isn't this enough? > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index e8bc8ce..eb7ffc4 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -604,7 +604,7 @@ static unsigned int pc_apic_id_limit(unsigned int max_cpus) return x86_cpu_apic_id_from_index(max_cpus - 1) + 1; } -static FWCfgState *bochs_bios_init(void) +static FWCfgState *bochs_bios_init(bool smbios_type1_defaults) { FWCfgState *fw_cfg; uint8_t *smbios_table; @@ -635,7 +635,7 @@ static FWCfgState *bochs_bios_init(void) acpi_tables, acpi_tables_len); fw_cfg_add_i32(fw_cfg, FW_CFG_IRQ0_OVERRIDE, kvm_allows_irq0_override()); - smbios_table = smbios_get_table(&smbios_len); + smbios_table = smbios_get_table(&smbios_len, smbios_type1_defaults); if (smbios_table) fw_cfg_add_bytes(fw_cfg, FW_CFG_SMBIOS_ENTRIES, smbios_table, smbios_len); @@ -1155,7 +1155,7 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory, option_rom_mr, 1); - fw_cfg = bochs_bios_init(); + fw_cfg = bochs_bios_init(guest_info->smbios_type1_defaults); rom_set_fw(fw_cfg); if (linux_boot) { diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index 6e1e654..2a621ef 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -58,6 +58,7 @@ static const int ide_irq[MAX_IDE_BUS] = { 14, 15 }; static bool has_pvpanic; static bool has_pci_info = true; +static bool smbios_type1_defaults = true; /* PC hardware initialisation */ static void pc_init1(MemoryRegion *system_memory, @@ -128,6 +129,7 @@ static void pc_init1(MemoryRegion *system_memory, guest_info = pc_guest_info_init(below_4g_mem_size, above_4g_mem_size); guest_info->has_pci_info = has_pci_info; guest_info->isapc_ram_fw = !pci_enabled; + guest_info->smbios_type1_defaults = smbios_type1_defaults; /* allocate ram and load rom/bios */ if (!xen_enabled()) { @@ -258,6 +260,7 @@ static void pc_init_pci_1_6(QEMUMachineInitArgs *args) static void pc_init_pci_1_5(QEMUMachineInitArgs *args) { has_pvpanic = true; + smbios_type1_defaults = false; pc_init_pci_1_6(args); } @@ -298,6 +301,7 @@ static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args) const char *initrd_filename = args->initrd_filename; const char *boot_device = args->boot_device; has_pci_info = false; + smbios_type1_defaults = false; disable_kvm_pv_eoi(); enable_compat_apic_id_mode(); pc_init1(get_system_memory(), @@ -316,6 +320,7 @@ static void pc_init_isa(QEMUMachineInitArgs *args) const char *initrd_filename = args->initrd_filename; const char *boot_device = args->boot_device; has_pci_info = false; + smbios_type1_defaults = false; if (cpu_model == NULL) cpu_model = "486"; disable_kvm_pv_eoi(); diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index 10e770e..05bce55 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -48,6 +48,7 @@ static bool has_pvpanic; static bool has_pci_info = true; +static bool smbios_type1_defaults = true; /* PC hardware initialisation */ static void pc_q35_init(QEMUMachineInitArgs *args) @@ -111,6 +112,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args) guest_info = pc_guest_info_init(below_4g_mem_size, above_4g_mem_size); guest_info->has_pci_info = has_pci_info; guest_info->isapc_ram_fw = false; + guest_info->smbios_type1_defaults = smbios_type1_defaults; /* allocate ram and load rom/bios */ if (!xen_enabled()) { @@ -227,6 +229,7 @@ static void pc_q35_init_1_6(QEMUMachineInitArgs *args) static void pc_q35_init_1_5(QEMUMachineInitArgs *args) { has_pvpanic = true; + smbios_type1_defaults = false; pc_q35_init_1_6(args); } diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c index a2eb9bf..e6413a5 100644 --- a/hw/i386/smbios.c +++ b/hw/i386/smbios.c @@ -18,6 +18,7 @@ #include "qemu/config-file.h" #include "qemu/error-report.h" #include "sysemu/sysemu.h" +#include "hw/boards.h" #include "hw/i386/smbios.h" #include "hw/loader.h" @@ -256,9 +257,18 @@ static void smbios_build_type_1_fields(void) } } -uint8_t *smbios_get_table(size_t *length) +uint8_t *smbios_get_table(size_t *length, bool type1_defaults) { if (!smbios_immutable) { + if (type1_defaults && !type1.manufacturer) { + type1.manufacturer = "QEMU"; + } + if (type1_defaults && !type1.product) { + type1.product = current_machine->desc; + } + if (type1_defaults && !type1.version) { + type1.version = current_machine->name; + } smbios_build_type_0_fields(); smbios_build_type_1_fields(); smbios_validate_table(); diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index f79d478..5797b97 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -20,6 +20,7 @@ typedef struct PcPciInfo { struct PcGuestInfo { bool has_pci_info; bool isapc_ram_fw; + bool smbios_type1_defaults; FWCfgState *fw_cfg; }; diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h index b08ec71..e258d9a 100644 --- a/include/hw/i386/smbios.h +++ b/include/hw/i386/smbios.h @@ -16,7 +16,7 @@ #include "qemu/option.h" void smbios_entry_add(QemuOpts *opts); -uint8_t *smbios_get_table(size_t *length); +uint8_t *smbios_get_table(size_t *length, bool type1_defaults); /* * SMBIOS spec defined tables