Re: [Qemu-devel] [PATCH v2 7/7] smbios: Set system manufacturer, product & version by default

["Michael S. Tsirkin" <mst@redhat.com>]

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
>