[Qemu-devel] [PATCH 0/2] Couple of SMBIOS fixes

[Qemu-devel] [PATCH 0/2] Couple of SMBIOS fixes

[Michal Privoznik]

Well, strictly speaking the first is a code cleanup, the second
one implements the feature we claimed to have but it wasn't
implemented.

Michal Privoznik (2):
  pc_init1: Don't misuse int for holding up a bool
  SMBIOS: Allow users to chose base board type

 hw/i386/pc_piix.c | 10 +++++-----
 hw/i386/smbios.c  | 18 +++++++++++++++++-
 qemu-options.hx   |  4 ++--
 3 files changed, 24 insertions(+), 8 deletions(-)

-- 
2.3.6

[Qemu-devel] [PATCH 1/2] pc_init1: Don't misuse int for holding up a bool

[Michal Privoznik]

When going through code I realized, that the pc_init1() has this two
arguments @pci_enabled and @kvmclock_enabled which despite used as
booleans are of int type.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 hw/i386/pc_piix.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 212e263..97650b0 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -74,8 +74,8 @@ static bool has_reserved_memory = true;
 
 /* PC hardware initialisation */
 static void pc_init1(MachineState *machine,
-                     int pci_enabled,
-                     int kvmclock_enabled)
+                     bool pci_enabled,
+                     bool kvmclock_enabled)
 {
     PCMachineState *pc_machine = PC_MACHINE(machine);
     MemoryRegion *system_memory = get_system_memory();
@@ -307,7 +307,7 @@ static void pc_init1(MachineState *machine,
 
 static void pc_init_pci(MachineState *machine)
 {
-    pc_init1(machine, 1, 1);
+    pc_init1(machine, true, true);
 }
 
 static void pc_compat_2_3(MachineState *machine)
@@ -483,7 +483,7 @@ static void pc_init_pci_1_2(MachineState *machine)
 static void pc_init_pci_no_kvmclock(MachineState *machine)
 {
     pc_compat_1_2(machine);
-    pc_init1(machine, 1, 0);
+    pc_init1(machine, true, false);
 }
 
 static void pc_init_isa(MachineState *machine)
@@ -500,7 +500,7 @@ static void pc_init_isa(MachineState *machine)
     }
     x86_cpu_compat_kvm_no_autoenable(FEAT_KVM, 1 << KVM_FEATURE_PV_EOI);
     enable_compat_apic_id_mode();
-    pc_init1(machine, 0, 1);
+    pc_init1(machine, false, true);
 }
 
 #ifdef CONFIG_XEN
-- 
2.3.6

Re: [Qemu-devel] [PATCH 1/2] pc_init1: Don't misuse int for holding up a bool

[Paolo Bonzini]

On 20/05/2015 11:15, Michal Privoznik wrote:
> When going through code I realized, that the pc_init1() has this two
> arguments @pci_enabled and @kvmclock_enabled which despite used as
> booleans are of int type.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

Eduardo, do your patches to the PC machine types subsume this change?

Paolo

> ---
>  hw/i386/pc_piix.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 212e263..97650b0 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -74,8 +74,8 @@ static bool has_reserved_memory = true;
>  
>  /* PC hardware initialisation */
>  static void pc_init1(MachineState *machine,
> -                     int pci_enabled,
> -                     int kvmclock_enabled)
> +                     bool pci_enabled,
> +                     bool kvmclock_enabled)
>  {
>      PCMachineState *pc_machine = PC_MACHINE(machine);
>      MemoryRegion *system_memory = get_system_memory();
> @@ -307,7 +307,7 @@ static void pc_init1(MachineState *machine,
>  
>  static void pc_init_pci(MachineState *machine)
>  {
> -    pc_init1(machine, 1, 1);
> +    pc_init1(machine, true, true);
>  }
>  
>  static void pc_compat_2_3(MachineState *machine)
> @@ -483,7 +483,7 @@ static void pc_init_pci_1_2(MachineState *machine)
>  static void pc_init_pci_no_kvmclock(MachineState *machine)
>  {
>      pc_compat_1_2(machine);
> -    pc_init1(machine, 1, 0);
> +    pc_init1(machine, true, false);
>  }
>  
>  static void pc_init_isa(MachineState *machine)
> @@ -500,7 +500,7 @@ static void pc_init_isa(MachineState *machine)
>      }
>      x86_cpu_compat_kvm_no_autoenable(FEAT_KVM, 1 << KVM_FEATURE_PV_EOI);
>      enable_compat_apic_id_mode();
> -    pc_init1(machine, 0, 1);
> +    pc_init1(machine, false, true);
>  }
>  
>  #ifdef CONFIG_XEN
> 

Re: [Qemu-devel] [PATCH 1/2] pc_init1: Don't misuse int for holding up a bool

[Eduardo Habkost]

On Wed, May 20, 2015 at 11:55:53AM +0200, Paolo Bonzini wrote:
> On 20/05/2015 11:15, Michal Privoznik wrote:
> > When going through code I realized, that the pc_init1() has this two
> > arguments @pci_enabled and @kvmclock_enabled which despite used as
> > booleans are of int type.
> > 
> > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> 
> Eduardo, do your patches to the PC machine types subsume this change?

Yes, it replaces the int function arguments with bool globals.

(Globals that will be eventually moved to PCMachineClass with the other
8 PC compat globals we have).

-- 
Eduardo

[Qemu-devel] [PATCH 2/2] SMBIOS: Allow users to chose base board type

[Michal Privoznik]

So far we only mistakenly claimed support for selecting base
board type. This patch finally implements it. There's an enum in
SMBIOS specification that lays out all the possible values.
Therefore, the type is taken as an integer of the corresponding
string value.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 hw/i386/smbios.c | 18 +++++++++++++++++-
 qemu-options.hx  |  4 ++--
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index 1341e02..b2f3809 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -79,6 +79,9 @@ static struct {
 
 static struct {
     const char *manufacturer, *product, *version, *serial, *asset, *location;
+    uint8_t family; /* It's called type in the specification, but
+                       unfortunately that name is already taken
+                       (to specify SMBIOS entry type). */
 } type2;
 
 static struct {
@@ -210,6 +213,10 @@ static const QemuOptDesc qemu_smbios_type2_opts[] = {
         .name = "location",
         .type = QEMU_OPT_STRING,
         .help = "location in chassis",
+    },{
+        .name = "family",
+        .type = QEMU_OPT_NUMBER,
+        .help = "board type",
     },
     { /* end of list */ }
 };
@@ -579,7 +586,7 @@ static void smbios_build_type_2_table(void)
     t->feature_flags = 0x01; /* Motherboard */
     SMBIOS_TABLE_SET_STR(2, location_str, type2.location);
     t->chassis_handle = cpu_to_le16(0x300); /* Type 3 (System enclosure) */
-    t->board_type = 0x0A; /* Motherboard */
+    t->board_type = type2.family ? type2.family : 0x0A; /* Motherboard */
     t->contained_element_count = 0;
 
     SMBIOS_BUILD_TABLE_POST;
@@ -1050,6 +1057,15 @@ void smbios_entry_add(QemuOpts *opts)
             save_opt(&type2.serial, opts, "serial");
             save_opt(&type2.asset, opts, "asset");
             save_opt(&type2.location, opts, "location");
+
+            val = qemu_opt_get(opts, "family");
+            if (val) {
+                if (sscanf(val, "%hhu", &type2.family) != 1 ||
+                    type2.family > 0x0D) {
+                    error_report("Invalid base board type");
+                    exit(1);
+                }
+            }
             return;
         case 3:
             qemu_opts_validate(opts, qemu_smbios_type3_opts, &local_err);
diff --git a/qemu-options.hx b/qemu-options.hx
index ec356f6..ef78849 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1393,7 +1393,7 @@ DEF("smbios", HAS_ARG, QEMU_OPTION_smbios,
     "              [,uuid=uuid][,sku=str][,family=str]\n"
     "                specify SMBIOS type 1 fields\n"
     "-smbios type=2[,manufacturer=str][,product=str][,version=str][,serial=str]\n"
-    "              [,asset=str][,location=str]\n"
+    "              [,asset=str][,location=str][,family=%d]\n"
     "                specify SMBIOS type 2 fields\n"
     "-smbios type=3[,manufacturer=str][,version=str][,serial=str][,asset=str]\n"
     "              [,sku=str]\n"
@@ -1416,7 +1416,7 @@ Specify SMBIOS type 0 fields
 @item -smbios type=1[,manufacturer=@var{str}][,product=@var{str}][,version=@var{str}][,serial=@var{str}][,uuid=@var{uuid}][,sku=@var{str}][,family=@var{str}]
 Specify SMBIOS type 1 fields
 
-@item -smbios type=2[,manufacturer=@var{str}][,product=@var{str}][,version=@var{str}][,serial=@var{str}][,asset=@var{str}][,location=@var{str}][,family=@var{str}]
+@item -smbios type=2[,manufacturer=@var{str}][,product=@var{str}][,version=@var{str}][,serial=@var{str}][,asset=@var{str}][,location=@var{str}][,family=@var{%d}]
 Specify SMBIOS type 2 fields
 
 @item -smbios type=3[,manufacturer=@var{str}][,version=@var{str}][,serial=@var{str}][,asset=@var{str}][,sku=@var{str}]
-- 
2.3.6

Re: [Qemu-devel] [PATCH 2/2] SMBIOS: Allow users to chose base board type

[Paolo Bonzini]

On 20/05/2015 11:15, Michal Privoznik wrote:
> So far we only mistakenly claimed support for selecting base
> board type. This patch finally implements it. There's an enum in
> SMBIOS specification that lays out all the possible values.
> Therefore, the type is taken as an integer of the corresponding
> string value.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

The possible values are:

	01h     Unknown
	02h     Other
	03h     Server Blade
	04h     Connectivity Switch
	05h     System Management Module
	06h     Processor Module
	07h     I/O Module
	08h     Memory Module
	09h     Daughter board
	0Ah     Motherboard (includes processor, memory, and I/O)
	0Bh     Processor/Memory Module
	0Ch     Processor/IO Module
	0Dh     Interconnect Board

What is the usecase?

Paolo

Re: [Qemu-devel] [PATCH 2/2] SMBIOS: Allow users to chose base board type

[Michal Privoznik]

On 20.05.2015 11:24, Paolo Bonzini wrote:
> 
> 
> On 20/05/2015 11:15, Michal Privoznik wrote:
>> So far we only mistakenly claimed support for selecting base
>> board type. This patch finally implements it. There's an enum in
>> SMBIOS specification that lays out all the possible values.
>> Therefore, the type is taken as an integer of the corresponding
>> string value.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> 
> The possible values are:
> 
> 	01h     Unknown
> 	02h     Other
> 	03h     Server Blade
> 	04h     Connectivity Switch
> 	05h     System Management Module
> 	06h     Processor Module
> 	07h     I/O Module
> 	08h     Memory Module
> 	09h     Daughter board
> 	0Ah     Motherboard (includes processor, memory, and I/O)
> 	0Bh     Processor/Memory Module
> 	0Ch     Processor/IO Module
> 	0Dh     Interconnect Board
> 
> What is the usecase?

I've got this bug on me:

https://bugzilla.redhat.com/show_bug.cgi?id=1220527

It's request to implement Type 2 in libvirt. However, while implementing
that, I found out not all fields are supported in qemu. So I've posted a
patch.

Michal

Re: [Qemu-devel] [PATCH 2/2] SMBIOS: Allow users to chose base board type

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 677 bytes --]

On 05/20/2015 03:15 AM, Michal Privoznik wrote:

s/chose/choose/ in the subject

> So far we only mistakenly claimed support for selecting base
> board type. This patch finally implements it. There's an enum in
> SMBIOS specification that lays out all the possible values.
> Therefore, the type is taken as an integer of the corresponding
> string value.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  hw/i386/smbios.c | 18 +++++++++++++++++-
>  qemu-options.hx  |  4 ++--
>  2 files changed, 19 insertions(+), 3 deletions(-)
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]