[Qemu-devel] [PATCH v3 0/2] smbios nicer defaults for DMI type 1 (System)

[Qemu-devel] [PATCH v3 0/2] smbios nicer defaults for DMI type 1 (System)

[armbru]

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.

This series used to be called "[PATCH v2 0/7] smbios cleanup & nicer
defaults for type 1", but its cleanup parts [0-5/7] already went in.

Andreas didn't like "[PATCH v2 6/7] vl: Set current_machine early",
and suggested to put he machine into QEMUMachineInitArgs.  PATCH 1/2
does exactly that, replacing v2's PATCH 6/7.  PATCH 2/2 is v2's PATCH
7/7 rebased on top.

Michael didn't like the way I suppress the nicer defaults for old
machine types via static bool smbios_type1_defaults, and thought it
would be nicer to have it in QEMUMachine or some device.  I considered
this, but decided to stick to smbios_type1_defaults, because

1. There is no suitable device.
2. I'd rather not extend QEMUMachine with target-specific stuff.
3. A static bool whose default value gets flipped by some QEMUMachine
   init() methods is what we commonly do in such cases, so let's stick
   to that.

v3: Do it the way Andreas suggested
v2: Rebase, only last patch had conflicts

Markus Armbruster (2):
  hw: Pass QEMUMachine to its init() method
  smbios: Set system manufacturer, product & version by default

 hw/i386/pc_piix.c        |  9 +++++++++
 hw/i386/pc_q35.c         |  7 +++++++
 hw/i386/smbios.c         | 14 ++++++++++++++
 include/hw/boards.h      |  7 +++++--
 include/hw/i386/smbios.h |  2 ++
 vl.c                     |  3 ++-
 6 files changed, 39 insertions(+), 3 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH v3 1/2] hw: Pass QEMUMachine to its init() method

[armbru]

From: Markus Armbruster <armbru@redhat.com>

Put it in QEMUMachineInitArgs, so I don't have to touch every board.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/hw/boards.h | 7 +++++--
 vl.c                | 3 ++-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 5a7ae9f..2151460 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -6,7 +6,10 @@
 #include "sysemu/blockdev.h"
 #include "hw/qdev.h"
 
+typedef struct QEMUMachine QEMUMachine;
+
 typedef struct QEMUMachineInitArgs {
+    const QEMUMachine *machine;
     ram_addr_t ram_size;
     const char *boot_order;
     const char *kernel_filename;
@@ -21,7 +24,7 @@ typedef void QEMUMachineResetFunc(void);
 
 typedef void QEMUMachineHotAddCPUFunc(const int64_t id, Error **errp);
 
-typedef struct QEMUMachine {
+struct QEMUMachine {
     const char *name;
     const char *alias;
     const char *desc;
@@ -43,7 +46,7 @@ typedef struct QEMUMachine {
     GlobalProperty *compat_props;
     struct QEMUMachine *next;
     const char *hw_version;
-} QEMUMachine;
+};
 
 int qemu_register_machine(QEMUMachine *m);
 QEMUMachine *find_default_machine(void);
diff --git a/vl.c b/vl.c
index b42ac67..63338e4 100644
--- a/vl.c
+++ b/vl.c
@@ -4236,7 +4236,8 @@ int main(int argc, char **argv, char **envp)
 
     qdev_machine_init();
 
-    QEMUMachineInitArgs args = { .ram_size = ram_size,
+    QEMUMachineInitArgs args = { .machine = machine,
+                                 .ram_size = ram_size,
                                  .boot_order = boot_order,
                                  .kernel_filename = kernel_filename,
                                  .kernel_cmdline = kernel_cmdline,
-- 
1.8.1.4

[Qemu-devel] [PATCH v3 2/2] smbios: Set system manufacturer, product & version by default

[armbru]

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>
---
 hw/i386/pc_piix.c        |  9 +++++++++
 hw/i386/pc_q35.c         |  7 +++++++
 hw/i386/smbios.c         | 14 ++++++++++++++
 include/hw/i386/smbios.h |  2 ++
 4 files changed, 32 insertions(+)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index c6042c7..417ad33 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -28,6 +28,7 @@
 #include "hw/loader.h"
 #include "hw/i386/pc.h"
 #include "hw/i386/apic.h"
+#include "hw/i386/smbios.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_ids.h"
 #include "hw/usb.h"
@@ -59,6 +60,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(QEMUMachineInitArgs *args,
@@ -124,6 +126,10 @@ static void pc_init1(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 = !pci_enabled;
+    if (smbios_type1_defaults) {
+        smbios_set_type1_defaults("QEMU", args->machine->desc,
+                                  args->machine->name);
+    }
 
     /* allocate ram and load rom/bios */
     if (!xen_enabled()) {
@@ -240,6 +246,7 @@ static void pc_compat_1_6(QEMUMachineInitArgs *args)
 {
     has_pci_info = false;
     rom_file_in_ram = false;
+    smbios_type1_defaults = false;
 }
 
 static void pc_compat_1_5(QEMUMachineInitArgs *args)
@@ -304,6 +311,7 @@ static void pc_init_pci_1_2(QEMUMachineInitArgs *args)
 static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
 {
     has_pci_info = false;
+    smbios_type1_defaults = false;
     disable_kvm_pv_eoi();
     enable_compat_apic_id_mode();
     pc_init1(args, 1, 0);
@@ -312,6 +320,7 @@ static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
 static void pc_init_isa(QEMUMachineInitArgs *args)
 {
     has_pci_info = false;
+    smbios_type1_defaults = false;
     if (!args->cpu_model) {
         args->cpu_model = "486";
     }
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index ca84e1c..9e3213f 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -39,6 +39,7 @@
 #include "hw/pci-host/q35.h"
 #include "exec/address-spaces.h"
 #include "hw/i386/ich9.h"
+#include "hw/i386/smbios.h"
 #include "hw/ide/pci.h"
 #include "hw/ide/ahci.h"
 #include "hw/usb.h"
@@ -49,6 +50,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 +113,10 @@ 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;
+    if (smbios_type1_defaults) {
+        smbios_set_type1_defaults("QEMU", args->machine->desc,
+                                  args->machine->name);
+    }
 
     /* allocate ram and load rom/bios */
     if (!xen_enabled()) {
@@ -224,6 +230,7 @@ static void pc_compat_1_6(QEMUMachineInitArgs *args)
 {
     has_pci_info = false;
     rom_file_in_ram = false;
+    smbios_type1_defaults = false;
 }
 
 static void pc_compat_1_5(QEMUMachineInitArgs *args)
diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index d3f1ee6..e8f41ad 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -256,6 +256,20 @@ static void smbios_build_type_1_fields(void)
     }
 }
 
+void smbios_set_type1_defaults(const char *manufacturer,
+                               const char *product, const char *version)
+{
+    if (!type1.manufacturer) {
+        type1.manufacturer = manufacturer;
+    }
+    if (!type1.product) {
+        type1.product = product;
+    }
+    if (!type1.version) {
+        type1.version = version;
+    }
+}
+
 uint8_t *smbios_get_table(size_t *length)
 {
     if (!smbios_immutable) {
diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h
index b08ec71..18fb970 100644
--- a/include/hw/i386/smbios.h
+++ b/include/hw/i386/smbios.h
@@ -16,6 +16,8 @@
 #include "qemu/option.h"
 
 void smbios_entry_add(QemuOpts *opts);
+void smbios_set_type1_defaults(const char *manufacturer,
+                               const char *product, const char *version);
 uint8_t *smbios_get_table(size_t *length);
 
 /*
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v3 1/2] hw: Pass QEMUMachine to its init() method

[Eduardo Habkost]

On Wed, Oct 30, 2013 at 01:56:39PM +0100, armbru@redhat.com wrote:
> From: Markus Armbruster <armbru@redhat.com>
> 
> Put it in QEMUMachineInitArgs, so I don't have to touch every board.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

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

[Eduardo Habkost]

On Wed, Oct 30, 2013 at 01:56:40PM +0100, 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: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 1/2] hw: Pass QEMUMachine to its init() method

[Andreas Färber]

Am 30.10.2013 13:56, schrieb armbru@redhat.com:
> From: Markus Armbruster <armbru@redhat.com>
> 
> Put it in QEMUMachineInitArgs, so I don't have to touch every board.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Andreas Färber <afaerber@suse.de>

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[Michael S. Tsirkin]

On Wed, Oct 30, 2013 at 01:56:40PM +0100, 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>

I feel applying this one would be a mistake.

Machine desc is for human readers.
For example, it currently says "Standard PC (Q35 + ICH9, 2009)"
but if we add a variant with IDE compatibility mode we will likely want to
tweak it to say "Standard PC (Q35 + ICH9/AHCI mode, 2009)"
and add another one saying ""Standard PC (Q35 + ICH9/compat mode,
2009)".

In other words we want the ability to tweak
description retroactively, and exposing it to guest will
break this ability.

So we really need a new field not tied to the human description.

> ---
>  hw/i386/pc_piix.c        |  9 +++++++++
>  hw/i386/pc_q35.c         |  7 +++++++
>  hw/i386/smbios.c         | 14 ++++++++++++++
>  include/hw/i386/smbios.h |  2 ++
>  4 files changed, 32 insertions(+)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index c6042c7..417ad33 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -28,6 +28,7 @@
>  #include "hw/loader.h"
>  #include "hw/i386/pc.h"
>  #include "hw/i386/apic.h"
> +#include "hw/i386/smbios.h"
>  #include "hw/pci/pci.h"
>  #include "hw/pci/pci_ids.h"
>  #include "hw/usb.h"
> @@ -59,6 +60,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(QEMUMachineInitArgs *args,
> @@ -124,6 +126,10 @@ static void pc_init1(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 = !pci_enabled;
> +    if (smbios_type1_defaults) {
> +        smbios_set_type1_defaults("QEMU", args->machine->desc,
> +                                  args->machine->name);
> +    }
>  
>      /* allocate ram and load rom/bios */
>      if (!xen_enabled()) {
> @@ -240,6 +246,7 @@ static void pc_compat_1_6(QEMUMachineInitArgs *args)
>  {
>      has_pci_info = false;
>      rom_file_in_ram = false;
> +    smbios_type1_defaults = false;
>  }
>  
>  static void pc_compat_1_5(QEMUMachineInitArgs *args)
> @@ -304,6 +311,7 @@ static void pc_init_pci_1_2(QEMUMachineInitArgs *args)
>  static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
>  {
>      has_pci_info = false;
> +    smbios_type1_defaults = false;
>      disable_kvm_pv_eoi();
>      enable_compat_apic_id_mode();
>      pc_init1(args, 1, 0);
> @@ -312,6 +320,7 @@ static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
>  static void pc_init_isa(QEMUMachineInitArgs *args)
>  {
>      has_pci_info = false;
> +    smbios_type1_defaults = false;
>      if (!args->cpu_model) {
>          args->cpu_model = "486";
>      }
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index ca84e1c..9e3213f 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -39,6 +39,7 @@
>  #include "hw/pci-host/q35.h"
>  #include "exec/address-spaces.h"
>  #include "hw/i386/ich9.h"
> +#include "hw/i386/smbios.h"
>  #include "hw/ide/pci.h"
>  #include "hw/ide/ahci.h"
>  #include "hw/usb.h"
> @@ -49,6 +50,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 +113,10 @@ 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;
> +    if (smbios_type1_defaults) {
> +        smbios_set_type1_defaults("QEMU", args->machine->desc,
> +                                  args->machine->name);
> +    }
>  
>      /* allocate ram and load rom/bios */
>      if (!xen_enabled()) {
> @@ -224,6 +230,7 @@ static void pc_compat_1_6(QEMUMachineInitArgs *args)
>  {
>      has_pci_info = false;
>      rom_file_in_ram = false;
> +    smbios_type1_defaults = false;
>  }
>  
>  static void pc_compat_1_5(QEMUMachineInitArgs *args)
> diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
> index d3f1ee6..e8f41ad 100644
> --- a/hw/i386/smbios.c
> +++ b/hw/i386/smbios.c
> @@ -256,6 +256,20 @@ static void smbios_build_type_1_fields(void)
>      }
>  }
>  
> +void smbios_set_type1_defaults(const char *manufacturer,
> +                               const char *product, const char *version)
> +{
> +    if (!type1.manufacturer) {
> +        type1.manufacturer = manufacturer;
> +    }
> +    if (!type1.product) {
> +        type1.product = product;
> +    }
> +    if (!type1.version) {
> +        type1.version = version;
> +    }
> +}
> +
>  uint8_t *smbios_get_table(size_t *length)
>  {
>      if (!smbios_immutable) {
> diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h
> index b08ec71..18fb970 100644
> --- a/include/hw/i386/smbios.h
> +++ b/include/hw/i386/smbios.h
> @@ -16,6 +16,8 @@
>  #include "qemu/option.h"
>  
>  void smbios_entry_add(QemuOpts *opts);
> +void smbios_set_type1_defaults(const char *manufacturer,
> +                               const char *product, const char *version);
>  uint8_t *smbios_get_table(size_t *length);
>  
>  /*
> -- 
> 1.8.1.4

Re: [Qemu-devel] [PATCH v3 0/2] smbios nicer defaults for DMI type 1 (System)

[Michael S. Tsirkin]

On Wed, Oct 30, 2013 at 01:56:38PM +0100, 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.
> 
> This series used to be called "[PATCH v2 0/7] smbios cleanup & nicer
> defaults for type 1", but its cleanup parts [0-5/7] already went in.
> 
> Andreas didn't like "[PATCH v2 6/7] vl: Set current_machine early",
> and suggested to put he machine into QEMUMachineInitArgs.  PATCH 1/2
> does exactly that, replacing v2's PATCH 6/7.  PATCH 2/2 is v2's PATCH
> 7/7 rebased on top.
> 
> Michael didn't like the way I suppress the nicer defaults for old
> machine types via static bool smbios_type1_defaults, and thought it
> would be nicer to have it in QEMUMachine or some device.  I considered
> this, but decided to stick to smbios_type1_defaults, because
> 
> 1. There is no suitable device.
> 2. I'd rather not extend QEMUMachine with target-specific stuff.
> 3. A static bool whose default value gets flipped by some QEMUMachine
>    init() methods is what we commonly do in such cases, so let's stick
>    to that.

Sorry, it seems I wasn't clear. What worries me is not how we supply the
defaults, it's the fact that we expose human interface to guests.
I tried to explain this better responding to the correct thread.

> v3: Do it the way Andreas suggested
> v2: Rebase, only last patch had conflicts
> 
> Markus Armbruster (2):
>   hw: Pass QEMUMachine to its init() method
>   smbios: Set system manufacturer, product & version by default
> 
>  hw/i386/pc_piix.c        |  9 +++++++++
>  hw/i386/pc_q35.c         |  7 +++++++
>  hw/i386/smbios.c         | 14 ++++++++++++++
>  include/hw/boards.h      |  7 +++++--
>  include/hw/i386/smbios.h |  2 ++
>  vl.c                     |  3 ++-
>  6 files changed, 39 insertions(+), 3 deletions(-)
> 
> -- 
> 1.8.1.4

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

[Eduardo Habkost]

On Wed, Oct 30, 2013 at 04:18:16PM +0200, Michael S. Tsirkin wrote:
> On Wed, Oct 30, 2013 at 01:56:40PM +0100, 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>
> 
> I feel applying this one would be a mistake.
> 
> Machine desc is for human readers.
> For example, it currently says "Standard PC (Q35 + ICH9, 2009)"
> but if we add a variant with IDE compatibility mode we will likely want to
> tweak it to say "Standard PC (Q35 + ICH9/AHCI mode, 2009)"
> and add another one saying ""Standard PC (Q35 + ICH9/compat mode,
> 2009)".
> 
> In other words we want the ability to tweak
> description retroactively, and exposing it to guest will
> break this ability.
> 
> So we really need a new field not tied to the human description.
> 

You have a point, but if we do that one day, then we can add a new
smbios-specific field and set it for each of the existing machine-types
so they keep the same ABI. This patch doesn't make us unable to do that
in the future.

As we are past hard freeze, I think this simple patch is better than a
more complex solution for a problem we still don't have (that can still
be implemented in 1.8).

-- 
Eduardo

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

[Michael S. Tsirkin]

On Wed, Oct 30, 2013 at 12:29:16PM -0200, Eduardo Habkost wrote:
> On Wed, Oct 30, 2013 at 04:18:16PM +0200, Michael S. Tsirkin wrote:
> > On Wed, Oct 30, 2013 at 01:56:40PM +0100, 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>
> > 
> > I feel applying this one would be a mistake.
> > 
> > Machine desc is for human readers.
> > For example, it currently says "Standard PC (Q35 + ICH9, 2009)"
> > but if we add a variant with IDE compatibility mode we will likely want to
> > tweak it to say "Standard PC (Q35 + ICH9/AHCI mode, 2009)"
> > and add another one saying ""Standard PC (Q35 + ICH9/compat mode,
> > 2009)".
> > 
> > In other words we want the ability to tweak
> > description retroactively, and exposing it to guest will
> > break this ability.
> > 
> > So we really need a new field not tied to the human description.
> > 
> 
> You have a point, but if we do that one day, then we can add a new
> smbios-specific field and set it for each of the existing machine-types
> so they keep the same ABI. This patch doesn't make us unable to do that
> in the future.

We'll likely forget and just break guest ABI.
So we really need a unit test for this, too.

> As we are past hard freeze, I think this simple patch is better than a
> more complex solution for a problem we still don't have (that can still
> be implemented in 1.8).

I don't see why we need to rush this into 1.7.
Downstreams want their info in smbios for support, branding etc,
but I don't see a burning need for this in upstream QEMU.
It's kind of nice to have it say "QEMU", but we can afford to
delay and do it properly for 1.8.

> -- 
> Eduardo

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

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Wed, Oct 30, 2013 at 01:56:40PM +0100, 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>
>
> I feel applying this one would be a mistake.
>
> Machine desc is for human readers.
> For example, it currently says "Standard PC (Q35 + ICH9, 2009)"
> but if we add a variant with IDE compatibility mode we will likely want to
> tweak it to say "Standard PC (Q35 + ICH9/AHCI mode, 2009)"
> and add another one saying ""Standard PC (Q35 + ICH9/compat mode,
> 2009)".
>
> In other words we want the ability to tweak
> description retroactively, and exposing it to guest will
> break this ability.

These will be new machine types, won't they?

> So we really need a new field not tied to the human description.

The SMBIOS string is *also* for human readers.

I can't see why we should jump through hoops *now* to separate the two.
The values are the same.  I don't expect us to change them for old
machine types.  But if we ever feel the need to change them in one place
but not the other, nothing will prevent us from splitting them up then.

Pay as you go, not pay as you fear you might have to go some day.

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

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Wed, Oct 30, 2013 at 12:29:16PM -0200, Eduardo Habkost wrote:
>> On Wed, Oct 30, 2013 at 04:18:16PM +0200, Michael S. Tsirkin wrote:
>> > On Wed, Oct 30, 2013 at 01:56:40PM +0100, 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>
>> > 
>> > I feel applying this one would be a mistake.
>> > 
>> > Machine desc is for human readers.
>> > For example, it currently says "Standard PC (Q35 + ICH9, 2009)"
>> > but if we add a variant with IDE compatibility mode we will likely want to
>> > tweak it to say "Standard PC (Q35 + ICH9/AHCI mode, 2009)"
>> > and add another one saying ""Standard PC (Q35 + ICH9/compat mode,
>> > 2009)".
>> > 
>> > In other words we want the ability to tweak
>> > description retroactively, and exposing it to guest will
>> > break this ability.
>> > 
>> > So we really need a new field not tied to the human description.
>> > 
>> 
>> You have a point, but if we do that one day, then we can add a new
>> smbios-specific field and set it for each of the existing machine-types
>> so they keep the same ABI. This patch doesn't make us unable to do that
>> in the future.
>
> We'll likely forget and just break guest ABI.
> So we really need a unit test for this, too.

More tests are good, but we I think we got bigger fish to fry than
writing tests to catch changes that are so obviously foolish as messing
with old machine type's QEMUMachine.

>> As we are past hard freeze, I think this simple patch is better than a
>> more complex solution for a problem we still don't have (that can still
>> be implemented in 1.8).
>
> I don't see why we need to rush this into 1.7.
> Downstreams want their info in smbios for support, branding etc,
> but I don't see a burning need for this in upstream QEMU.
> It's kind of nice to have it say "QEMU", but we can afford to
> delay and do it properly for 1.8.

Define "properly".  I don't see what I'd like to do differently for 1.8.

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

[Michael S. Tsirkin]

On Wed, Oct 30, 2013 at 03:48:55PM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Wed, Oct 30, 2013 at 01:56:40PM +0100, 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>
> >
> > I feel applying this one would be a mistake.
> >
> > Machine desc is for human readers.
> > For example, it currently says "Standard PC (Q35 + ICH9, 2009)"
> > but if we add a variant with IDE compatibility mode we will likely want to
> > tweak it to say "Standard PC (Q35 + ICH9/AHCI mode, 2009)"
> > and add another one saying ""Standard PC (Q35 + ICH9/compat mode,
> > 2009)".
> >
> > In other words we want the ability to tweak
> > description retroactively, and exposing it to guest will
> > break this ability.
> 
> These will be new machine types, won't they?

Description is there for humans, so we should be free to change
it to make it more readable for old types, too.

> > So we really need a new field not tied to the human description.
> 
> The SMBIOS string is *also* for human readers.
> 
> I can't see why we should jump through hoops *now* to separate the two.
> The values are the same.  I don't expect us to change them for old
> machine types.  But if we ever feel the need to change them in one place
> but not the other, nothing will prevent us from splitting them up then.
> 
> Pay as you go, not pay as you fear you might have to go some day.

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

[Michael S. Tsirkin]

On Wed, Oct 30, 2013 at 04:18:37PM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Wed, Oct 30, 2013 at 12:29:16PM -0200, Eduardo Habkost wrote:
> >> On Wed, Oct 30, 2013 at 04:18:16PM +0200, Michael S. Tsirkin wrote:
> >> > On Wed, Oct 30, 2013 at 01:56:40PM +0100, 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>
> >> > 
> >> > I feel applying this one would be a mistake.
> >> > 
> >> > Machine desc is for human readers.
> >> > For example, it currently says "Standard PC (Q35 + ICH9, 2009)"
> >> > but if we add a variant with IDE compatibility mode we will likely want to
> >> > tweak it to say "Standard PC (Q35 + ICH9/AHCI mode, 2009)"
> >> > and add another one saying ""Standard PC (Q35 + ICH9/compat mode,
> >> > 2009)".
> >> > 
> >> > In other words we want the ability to tweak
> >> > description retroactively, and exposing it to guest will
> >> > break this ability.
> >> > 
> >> > So we really need a new field not tied to the human description.
> >> > 
> >> 
> >> You have a point, but if we do that one day, then we can add a new
> >> smbios-specific field and set it for each of the existing machine-types
> >> so they keep the same ABI. This patch doesn't make us unable to do that
> >> in the future.
> >
> > We'll likely forget and just break guest ABI.
> > So we really need a unit test for this, too.
> 
> More tests are good, but we I think we got bigger fish to fry than
> writing tests to catch changes that are so obviously foolish as messing
> with old machine type's QEMUMachine.

You "messed with" old machine type's QEMUMachine in your cleanup
patches too, didn't you?

> >> As we are past hard freeze, I think this simple patch is better than a
> >> more complex solution for a problem we still don't have (that can still
> >> be implemented in 1.8).
> >
> > I don't see why we need to rush this into 1.7.
> > Downstreams want their info in smbios for support, branding etc,
> > but I don't see a burning need for this in upstream QEMU.
> > It's kind of nice to have it say "QEMU", but we can afford to
> > delay and do it properly for 1.8.
> 
> Define "properly".  I don't see what I'd like to do differently for 1.8.

With proper tests going in first before we start changing things.
Ideally with better separation between user visible and guest visible
interfaces - though if there was a test to catch guest visible changes,
I would be less worried about this lack of separation.

-- 
MST

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

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Wed, Oct 30, 2013 at 04:18:37PM +0100, Markus Armbruster wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> 
>> > On Wed, Oct 30, 2013 at 12:29:16PM -0200, Eduardo Habkost wrote:
>> >> On Wed, Oct 30, 2013 at 04:18:16PM +0200, Michael S. Tsirkin wrote:
>> >> > On Wed, Oct 30, 2013 at 01:56:40PM +0100, 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>
>> >> > 
>> >> > I feel applying this one would be a mistake.
>> >> > 
>> >> > Machine desc is for human readers.
>> >> > For example, it currently says "Standard PC (Q35 + ICH9, 2009)"
>> >> > but if we add a variant with IDE compatibility mode we will
>> >> > likely want to
>> >> > tweak it to say "Standard PC (Q35 + ICH9/AHCI mode, 2009)"
>> >> > and add another one saying ""Standard PC (Q35 + ICH9/compat mode,
>> >> > 2009)".
>> >> > 
>> >> > In other words we want the ability to tweak
>> >> > description retroactively, and exposing it to guest will
>> >> > break this ability.
>> >> > 
>> >> > So we really need a new field not tied to the human description.
>> >> > 
>> >> 
>> >> You have a point, but if we do that one day, then we can add a new
>> >> smbios-specific field and set it for each of the existing machine-types
>> >> so they keep the same ABI. This patch doesn't make us unable to do that
>> >> in the future.
>> >
>> > We'll likely forget and just break guest ABI.
>> > So we really need a unit test for this, too.
>> 
>> More tests are good, but we I think we got bigger fish to fry than
>> writing tests to catch changes that are so obviously foolish as messing
>> with old machine type's QEMUMachine.
>
> You "messed with" old machine type's QEMUMachine in your cleanup
> patches too, didn't you?

I've occasionally touched QEMUMachine initializers in cleanup series,
but nothing as frivolous as changing strings.  And I can't find anything
as frivolous as that in git.  We *are* careful and conservative there.

>> >> As we are past hard freeze, I think this simple patch is better than a
>> >> more complex solution for a problem we still don't have (that can still
>> >> be implemented in 1.8).
>> >
>> > I don't see why we need to rush this into 1.7.
>> > Downstreams want their info in smbios for support, branding etc,
>> > but I don't see a burning need for this in upstream QEMU.
>> > It's kind of nice to have it say "QEMU", but we can afford to
>> > delay and do it properly for 1.8.
>> 
>> Define "properly".  I don't see what I'd like to do differently for 1.8.
>
> With proper tests going in first before we start changing things.
> Ideally with better separation between user visible and guest visible
> interfaces - though if there was a test to catch guest visible changes,
> I would be less worried about this lack of separation.

You want me to test for unlikely developer mistakes that are far easier
to catch in review than most other guest ABI changes, and far less
harmful than pretty much any other guest ABI change.  This would
multiply the size of this mini-series by a significant factor.  I can't
justify this in good conscience to my (and your) employer.  So this
isn't going to happen.

If the maintainers agree with you, then I wasted my time.  Sad, but I'd
rather write off the work I've already done than do much more work of no
particular value just to save it.

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

[Michael S. Tsirkin]

On Wed, Oct 30, 2013 at 09:22:38PM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Wed, Oct 30, 2013 at 04:18:37PM +0100, Markus Armbruster wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> 
> >> > On Wed, Oct 30, 2013 at 12:29:16PM -0200, Eduardo Habkost wrote:
> >> >> On Wed, Oct 30, 2013 at 04:18:16PM +0200, Michael S. Tsirkin wrote:
> >> >> > On Wed, Oct 30, 2013 at 01:56:40PM +0100, 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>
> >> >> > 
> >> >> > I feel applying this one would be a mistake.
> >> >> > 
> >> >> > Machine desc is for human readers.
> >> >> > For example, it currently says "Standard PC (Q35 + ICH9, 2009)"
> >> >> > but if we add a variant with IDE compatibility mode we will
> >> >> > likely want to
> >> >> > tweak it to say "Standard PC (Q35 + ICH9/AHCI mode, 2009)"
> >> >> > and add another one saying ""Standard PC (Q35 + ICH9/compat mode,
> >> >> > 2009)".
> >> >> > 
> >> >> > In other words we want the ability to tweak
> >> >> > description retroactively, and exposing it to guest will
> >> >> > break this ability.
> >> >> > 
> >> >> > So we really need a new field not tied to the human description.
> >> >> > 
> >> >> 
> >> >> You have a point, but if we do that one day, then we can add a new
> >> >> smbios-specific field and set it for each of the existing machine-types
> >> >> so they keep the same ABI. This patch doesn't make us unable to do that
> >> >> in the future.
> >> >
> >> > We'll likely forget and just break guest ABI.
> >> > So we really need a unit test for this, too.
> >> 
> >> More tests are good, but we I think we got bigger fish to fry than
> >> writing tests to catch changes that are so obviously foolish as messing
> >> with old machine type's QEMUMachine.
> >
> > You "messed with" old machine type's QEMUMachine in your cleanup
> > patches too, didn't you?
> 
> I've occasionally touched QEMUMachine initializers in cleanup series,
> but nothing as frivolous as changing strings.  And I can't find anything
> as frivolous as that in git.  We *are* careful and conservative there.
> 
> >> >> As we are past hard freeze, I think this simple patch is better than a
> >> >> more complex solution for a problem we still don't have (that can still
> >> >> be implemented in 1.8).
> >> >
> >> > I don't see why we need to rush this into 1.7.
> >> > Downstreams want their info in smbios for support, branding etc,
> >> > but I don't see a burning need for this in upstream QEMU.
> >> > It's kind of nice to have it say "QEMU", but we can afford to
> >> > delay and do it properly for 1.8.
> >> 
> >> Define "properly".  I don't see what I'd like to do differently for 1.8.
> >
> > With proper tests going in first before we start changing things.
> > Ideally with better separation between user visible and guest visible
> > interfaces - though if there was a test to catch guest visible changes,
> > I would be less worried about this lack of separation.
> 
> You want me to test for unlikely developer mistakes that are far easier
> to catch in review than most other guest ABI changes, and far less
> harmful than pretty much any other guest ABI change.  This would
> multiply the size of this mini-series by a significant factor.  I can't
> justify this in good conscience to my (and your) employer.  So this
> isn't going to happen.
>
> If the maintainers agree with you, then I wasted my time.  Sad, but I'd
> rather write off the work I've already done than do much more work of no
> particular value just to save it.

It would be of no particular value *if we only test these strings*.
But testing smbios generally has a lot of value IMHO.

-- 
MST

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

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Wed, Oct 30, 2013 at 09:22:38PM +0100, Markus Armbruster wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> 
>> > On Wed, Oct 30, 2013 at 04:18:37PM +0100, Markus Armbruster wrote:
>> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> >> 
>> >> > On Wed, Oct 30, 2013 at 12:29:16PM -0200, Eduardo Habkost wrote:
>> >> >> On Wed, Oct 30, 2013 at 04:18:16PM +0200, Michael S. Tsirkin wrote:
>> >> >> > On Wed, Oct 30, 2013 at 01:56:40PM +0100, 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>
>> >> >> > 
>> >> >> > I feel applying this one would be a mistake.
>> >> >> > 
>> >> >> > Machine desc is for human readers.
>> >> >> > For example, it currently says "Standard PC (Q35 + ICH9, 2009)"
>> >> >> > but if we add a variant with IDE compatibility mode we will
>> >> >> > likely want to
>> >> >> > tweak it to say "Standard PC (Q35 + ICH9/AHCI mode, 2009)"
>> >> >> > and add another one saying ""Standard PC (Q35 + ICH9/compat mode,
>> >> >> > 2009)".
>> >> >> > 
>> >> >> > In other words we want the ability to tweak
>> >> >> > description retroactively, and exposing it to guest will
>> >> >> > break this ability.
>> >> >> > 
>> >> >> > So we really need a new field not tied to the human description.
>> >> >> > 
>> >> >> 
>> >> >> You have a point, but if we do that one day, then we can add a new
>> >> >> smbios-specific field and set it for each of the existing machine-types
>> >> >> so they keep the same ABI. This patch doesn't make us unable to do that
>> >> >> in the future.
>> >> >
>> >> > We'll likely forget and just break guest ABI.
>> >> > So we really need a unit test for this, too.
>> >> 
>> >> More tests are good, but we I think we got bigger fish to fry than
>> >> writing tests to catch changes that are so obviously foolish as messing
>> >> with old machine type's QEMUMachine.
>> >
>> > You "messed with" old machine type's QEMUMachine in your cleanup
>> > patches too, didn't you?
>> 
>> I've occasionally touched QEMUMachine initializers in cleanup series,
>> but nothing as frivolous as changing strings.  And I can't find anything
>> as frivolous as that in git.  We *are* careful and conservative there.
>> 
>> >> >> As we are past hard freeze, I think this simple patch is better than a
>> >> >> more complex solution for a problem we still don't have (that can still
>> >> >> be implemented in 1.8).
>> >> >
>> >> > I don't see why we need to rush this into 1.7.
>> >> > Downstreams want their info in smbios for support, branding etc,
>> >> > but I don't see a burning need for this in upstream QEMU.
>> >> > It's kind of nice to have it say "QEMU", but we can afford to
>> >> > delay and do it properly for 1.8.
>> >> 
>> >> Define "properly".  I don't see what I'd like to do differently for 1.8.
>> >
>> > With proper tests going in first before we start changing things.
>> > Ideally with better separation between user visible and guest visible
>> > interfaces - though if there was a test to catch guest visible changes,
>> > I would be less worried about this lack of separation.
>> 
>> You want me to test for unlikely developer mistakes that are far easier
>> to catch in review than most other guest ABI changes, and far less
>> harmful than pretty much any other guest ABI change.  This would
>> multiply the size of this mini-series by a significant factor.  I can't
>> justify this in good conscience to my (and your) employer.  So this
>> isn't going to happen.
>>
>> If the maintainers agree with you, then I wasted my time.  Sad, but I'd
>> rather write off the work I've already done than do much more work of no
>> particular value just to save it.
>
> It would be of no particular value *if we only test these strings*.
> But testing smbios generally has a lot of value IMHO.

Yes, it has value, but you're not going to succeed into blackmailing me
to do that work now.

[Qemu-devel] [PATCH v3 3/2] smbios: Decouple system product from QEMUMachine

[Markus Armbruster]

Michael Tsirkin doesn't trust us to keep values of QEMUMachine member
product stable in the future.  Use copies instead, and in a way that
makes it obvious that they're guest ABI.

Note that we can be trusted to keep values of member name, because
that has always been ABI.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/i386/pc_piix.c | 3 ++-
 hw/i386/pc_q35.c  | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 417ad33..1ffa1ac 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -127,7 +127,8 @@ static void pc_init1(QEMUMachineInitArgs *args,
     guest_info->has_pci_info = has_pci_info;
     guest_info->isapc_ram_fw = !pci_enabled;
     if (smbios_type1_defaults) {
-        smbios_set_type1_defaults("QEMU", args->machine->desc,
+        /* These values are guest ABI, do not change */
+        smbios_set_type1_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)",
                                   args->machine->name);
     }
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 9e3213f..0dc75c1 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -114,7 +114,8 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
     guest_info->has_pci_info = has_pci_info;
     guest_info->isapc_ram_fw = false;
     if (smbios_type1_defaults) {
-        smbios_set_type1_defaults("QEMU", args->machine->desc,
+        /* These values are guest ABI, do not change */
+        smbios_set_type1_defaults("QEMU", "Standard PC (Q35 + ICH9, 2009)",
                                   args->machine->name);
     }
 
-- 
1.8.1.4

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

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Wed, Oct 30, 2013 at 01:56:40PM +0100, 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>
>
> I feel applying this one would be a mistake.
>
> Machine desc is for human readers.
> For example, it currently says "Standard PC (Q35 + ICH9, 2009)"
> but if we add a variant with IDE compatibility mode we will likely want to
> tweak it to say "Standard PC (Q35 + ICH9/AHCI mode, 2009)"
> and add another one saying ""Standard PC (Q35 + ICH9/compat mode,
> 2009)".
>
> In other words we want the ability to tweak
> description retroactively, and exposing it to guest will
> break this ability.
>
> So we really need a new field not tied to the human description.

Can you live with the separation I just posted as PATCH 3/2?

Re: [Qemu-devel] [PATCH v3 3/2] smbios: Decouple system product from QEMUMachine

[Michael S. Tsirkin]

On Thu, Oct 31, 2013 at 06:51:50AM +0100, Markus Armbruster wrote:
> Michael Tsirkin doesn't trust us to keep values of QEMUMachine member
> product stable in the future.  Use copies instead, and in a way that
> makes it obvious that they're guest ABI.
> 
> Note that we can be trusted to keep values of member name, because
> that has always been ABI.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Nice and clean. Thanks!

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/i386/pc_piix.c | 3 ++-
>  hw/i386/pc_q35.c  | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 417ad33..1ffa1ac 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -127,7 +127,8 @@ static void pc_init1(QEMUMachineInitArgs *args,
>      guest_info->has_pci_info = has_pci_info;
>      guest_info->isapc_ram_fw = !pci_enabled;
>      if (smbios_type1_defaults) {
> -        smbios_set_type1_defaults("QEMU", args->machine->desc,
> +        /* These values are guest ABI, do not change */
> +        smbios_set_type1_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)",
>                                    args->machine->name);
>      }
>  
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 9e3213f..0dc75c1 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -114,7 +114,8 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>      guest_info->has_pci_info = has_pci_info;
>      guest_info->isapc_ram_fw = false;
>      if (smbios_type1_defaults) {
> -        smbios_set_type1_defaults("QEMU", args->machine->desc,
> +        /* These values are guest ABI, do not change */
> +        smbios_set_type1_defaults("QEMU", "Standard PC (Q35 + ICH9, 2009)",
>                                    args->machine->name);
>      }
>  
> -- 
> 1.8.1.4
> 

Re: [Qemu-devel] [PATCH v3 1/2] hw: Pass QEMUMachine to its init() method

[Michael S. Tsirkin]

On Wed, Oct 30, 2013 at 01:56:39PM +0100, armbru@redhat.com wrote:
> From: Markus Armbruster <armbru@redhat.com>
> 
> Put it in QEMUMachineInitArgs, so I don't have to touch every board.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  include/hw/boards.h | 7 +++++--
>  vl.c                | 3 ++-
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 5a7ae9f..2151460 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -6,7 +6,10 @@
>  #include "sysemu/blockdev.h"
>  #include "hw/qdev.h"
>  
> +typedef struct QEMUMachine QEMUMachine;
> +
>  typedef struct QEMUMachineInitArgs {
> +    const QEMUMachine *machine;
>      ram_addr_t ram_size;
>      const char *boot_order;
>      const char *kernel_filename;
> @@ -21,7 +24,7 @@ typedef void QEMUMachineResetFunc(void);
>  
>  typedef void QEMUMachineHotAddCPUFunc(const int64_t id, Error **errp);
>  
> -typedef struct QEMUMachine {
> +struct QEMUMachine {
>      const char *name;
>      const char *alias;
>      const char *desc;
> @@ -43,7 +46,7 @@ typedef struct QEMUMachine {
>      GlobalProperty *compat_props;
>      struct QEMUMachine *next;
>      const char *hw_version;
> -} QEMUMachine;
> +};
>  
>  int qemu_register_machine(QEMUMachine *m);
>  QEMUMachine *find_default_machine(void);
> diff --git a/vl.c b/vl.c
> index b42ac67..63338e4 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4236,7 +4236,8 @@ int main(int argc, char **argv, char **envp)
>  
>      qdev_machine_init();
>  
> -    QEMUMachineInitArgs args = { .ram_size = ram_size,
> +    QEMUMachineInitArgs args = { .machine = machine,
> +                                 .ram_size = ram_size,
>                                   .boot_order = boot_order,
>                                   .kernel_filename = kernel_filename,
>                                   .kernel_cmdline = kernel_cmdline,
> -- 
> 1.8.1.4
> 

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

[Michael S. Tsirkin]

On Thu, Oct 31, 2013 at 06:54:37AM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Wed, Oct 30, 2013 at 01:56:40PM +0100, 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>
> >
> > I feel applying this one would be a mistake.
> >
> > Machine desc is for human readers.
> > For example, it currently says "Standard PC (Q35 + ICH9, 2009)"
> > but if we add a variant with IDE compatibility mode we will likely want to
> > tweak it to say "Standard PC (Q35 + ICH9/AHCI mode, 2009)"
> > and add another one saying ""Standard PC (Q35 + ICH9/compat mode,
> > 2009)".
> >
> > In other words we want the ability to tweak
> > description retroactively, and exposing it to guest will
> > break this ability.
> >
> > So we really need a new field not tied to the human description.
> 
> Can you live with the separation I just posted as PATCH 3/2?

Yes, I think that's a good way to do this.

Re: [Qemu-devel] [PATCH v3 3/2] smbios: Decouple system product from QEMUMachine

[Eduardo Habkost]

On Thu, Oct 31, 2013 at 06:51:50AM +0100, Markus Armbruster wrote:
> Michael Tsirkin doesn't trust us to keep values of QEMUMachine member
> product stable in the future.  Use copies instead, and in a way that
> makes it obvious that they're guest ABI.
> 
> Note that we can be trusted to keep values of member name, because
> that has always been ABI.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

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

[Eduardo Habkost]

On Wed, Oct 30, 2013 at 09:22:38PM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Wed, Oct 30, 2013 at 04:18:37PM +0100, Markus Armbruster wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> 
> >> > On Wed, Oct 30, 2013 at 12:29:16PM -0200, Eduardo Habkost wrote:
> >> >> On Wed, Oct 30, 2013 at 04:18:16PM +0200, Michael S. Tsirkin wrote:
> >> >> > On Wed, Oct 30, 2013 at 01:56:40PM +0100, 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>
> >> >> > 
> >> >> > I feel applying this one would be a mistake.
> >> >> > 
> >> >> > Machine desc is for human readers.
> >> >> > For example, it currently says "Standard PC (Q35 + ICH9, 2009)"
> >> >> > but if we add a variant with IDE compatibility mode we will
> >> >> > likely want to
> >> >> > tweak it to say "Standard PC (Q35 + ICH9/AHCI mode, 2009)"
> >> >> > and add another one saying ""Standard PC (Q35 + ICH9/compat mode,
> >> >> > 2009)".
> >> >> > 
> >> >> > In other words we want the ability to tweak
> >> >> > description retroactively, and exposing it to guest will
> >> >> > break this ability.
> >> >> > 
> >> >> > So we really need a new field not tied to the human description.
> >> >> > 
> >> >> 
> >> >> You have a point, but if we do that one day, then we can add a new
> >> >> smbios-specific field and set it for each of the existing machine-types
> >> >> so they keep the same ABI. This patch doesn't make us unable to do that
> >> >> in the future.
> >> >
> >> > We'll likely forget and just break guest ABI.
> >> > So we really need a unit test for this, too.
> >> 
> >> More tests are good, but we I think we got bigger fish to fry than
> >> writing tests to catch changes that are so obviously foolish as messing
> >> with old machine type's QEMUMachine.
> >
> > You "messed with" old machine type's QEMUMachine in your cleanup
> > patches too, didn't you?
> 
> I've occasionally touched QEMUMachine initializers in cleanup series,
> but nothing as frivolous as changing strings.  And I can't find anything
> as frivolous as that in git.  We *are* careful and conservative there.

Actually, we changed .desc for old machine types before. See commit
a0dba644c139907ccf6735c505fbd254010d6938.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 0/2] smbios nicer defaults for DMI type 1 (System)

[Michael S. Tsirkin]

On Wed, Oct 30, 2013 at 01:56:38PM +0100, 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.
> 
> This series used to be called "[PATCH v2 0/7] smbios cleanup & nicer
> defaults for type 1", but its cleanup parts [0-5/7] already went in.
> 
> Andreas didn't like "[PATCH v2 6/7] vl: Set current_machine early",
> and suggested to put he machine into QEMUMachineInitArgs.  PATCH 1/2
> does exactly that, replacing v2's PATCH 6/7.  PATCH 2/2 is v2's PATCH
> 7/7 rebased on top.
> 
> Michael didn't like the way I suppress the nicer defaults for old
> machine types via static bool smbios_type1_defaults, and thought it
> would be nicer to have it in QEMUMachine or some device.  I considered
> this, but decided to stick to smbios_type1_defaults, because
> 
> 1. There is no suitable device.
> 2. I'd rather not extend QEMUMachine with target-specific stuff.
> 3. A static bool whose default value gets flipped by some QEMUMachine
>    init() methods is what we commonly do in such cases, so let's stick
>    to that.
> 
> v3: Do it the way Andreas suggested
> v2: Rebase, only last patch had conflicts

I picked this up merging 2/2 and 3/2.
Unfortunately it';s too late for 1.7 but I'll try to
merge it as soon as 1.8 opens.


> Markus Armbruster (2):
>   hw: Pass QEMUMachine to its init() method
>   smbios: Set system manufacturer, product & version by default
> 
>  hw/i386/pc_piix.c        |  9 +++++++++
>  hw/i386/pc_q35.c         |  7 +++++++
>  hw/i386/smbios.c         | 14 ++++++++++++++
>  include/hw/boards.h      |  7 +++++--
>  include/hw/i386/smbios.h |  2 ++
>  vl.c                     |  3 ++-
>  6 files changed, 39 insertions(+), 3 deletions(-)
> 
> -- 
> 1.8.1.4