[Qemu-devel] [PATCH 1/2] x86: Add 2.4 pc machine versions

[Qemu-devel] [PATCH 1/2] x86: Add 2.4 pc machine versions

[Alexander Graf]

This patch adds all the boilerplate required to handle a 2.4 machine type
and compatibility options required on older machines.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/i386/pc_piix.c    | 37 +++++++++++++++++++++++++++++++++----
 hw/i386/pc_q35.c     | 34 +++++++++++++++++++++++++++++++---
 include/hw/i386/pc.h |  3 +++
 3 files changed, 67 insertions(+), 7 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 1fe7bfb..cf14b1f 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -310,8 +310,13 @@ static void pc_init_pci(MachineState *machine)
     pc_init1(machine, 1, 1);
 }
 
+static void pc_compat_2_3(MachineState *machine)
+{
+}
+
 static void pc_compat_2_2(MachineState *machine)
 {
+    pc_compat_2_3(machine);
     rsdp_in_ram = false;
     x86_cpu_compat_set_features("kvm64", FEAT_1_EDX, 0, CPUID_VME);
     x86_cpu_compat_set_features("kvm32", FEAT_1_EDX, 0, CPUID_VME);
@@ -413,6 +418,12 @@ static void pc_compat_1_2(MachineState *machine)
     x86_cpu_compat_kvm_no_autoenable(FEAT_KVM, 1 << KVM_FEATURE_PV_EOI);
 }
 
+static void pc_init_pci_2_3(MachineState *machine)
+{
+    pc_compat_2_3(machine);
+    pc_init_pci(machine);
+}
+
 static void pc_init_pci_2_2(MachineState *machine)
 {
     pc_compat_2_2(machine);
@@ -512,25 +523,41 @@ static void pc_xen_hvm_init(MachineState *machine)
     .desc = "Standard PC (i440FX + PIIX, 1996)", \
     .hot_add_cpu = pc_hot_add_cpu
 
-#define PC_I440FX_2_3_MACHINE_OPTIONS                           \
+#define PC_I440FX_2_4_MACHINE_OPTIONS                           \
     PC_I440FX_MACHINE_OPTIONS,                                  \
     .default_machine_opts = "firmware=bios-256k.bin",           \
     .default_display = "std"
 
-static QEMUMachine pc_i440fx_machine_v2_3 = {
-    PC_I440FX_2_3_MACHINE_OPTIONS,
-    .name = "pc-i440fx-2.3",
+static QEMUMachine pc_i440fx_machine_v2_4 = {
+    PC_I440FX_2_4_MACHINE_OPTIONS,
+    .name = "pc-i440fx-2.4",
     .alias = "pc",
     .init = pc_init_pci,
     .is_default = 1,
 };
 
+#define PC_I440FX_2_3_MACHINE_OPTIONS PC_I440FX_2_4_MACHINE_OPTIONS
+
+static QEMUMachine pc_i440fx_machine_v2_3 = {
+    PC_I440FX_2_3_MACHINE_OPTIONS,
+    .name = "pc-i440fx-2.3",
+    .init = pc_init_pci_2_3,
+    .compat_props = (GlobalProperty[]) {
+        PC_COMPAT_2_3,
+        { /* end of list */ }
+    },
+};
+
 #define PC_I440FX_2_2_MACHINE_OPTIONS PC_I440FX_2_3_MACHINE_OPTIONS
 
 static QEMUMachine pc_i440fx_machine_v2_2 = {
     PC_I440FX_2_2_MACHINE_OPTIONS,
     .name = "pc-i440fx-2.2",
     .init = pc_init_pci_2_2,
+    .compat_props = (GlobalProperty[]) {
+        PC_COMPAT_2_3,
+        { /* end of list */ }
+    },
 };
 
 #define PC_I440FX_2_1_MACHINE_OPTIONS                           \
@@ -543,6 +570,7 @@ static QEMUMachine pc_i440fx_machine_v2_1 = {
     .init = pc_init_pci_2_1,
     .compat_props = (GlobalProperty[]) {
         HW_COMPAT_2_1,
+        PC_COMPAT_2_3,
         { /* end of list */ }
     },
 };
@@ -970,6 +998,7 @@ static QEMUMachine xenfv_machine = {
 
 static void pc_machine_init(void)
 {
+    qemu_register_pc_machine(&pc_i440fx_machine_v2_4);
     qemu_register_pc_machine(&pc_i440fx_machine_v2_3);
     qemu_register_pc_machine(&pc_i440fx_machine_v2_2);
     qemu_register_pc_machine(&pc_i440fx_machine_v2_1);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index dcc17c0..8ef9e27 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -289,6 +289,10 @@ static void pc_q35_init(MachineState *machine)
     }
 }
 
+static void pc_compat_2_3(MachineState *machine)
+{
+}
+
 static void pc_compat_2_2(MachineState *machine)
 {
     rsdp_in_ram = false;
@@ -361,6 +365,12 @@ static void pc_compat_1_4(MachineState *machine)
     x86_cpu_compat_set_features("Westmere", FEAT_1_ECX, 0, CPUID_EXT_PCLMULQDQ);
 }
 
+static void pc_q35_init_2_3(MachineState *machine)
+{
+    pc_compat_2_3(machine);
+    pc_q35_init(machine);
+}
+
 static void pc_q35_init_2_2(MachineState *machine)
 {
     pc_compat_2_2(machine);
@@ -410,16 +420,28 @@ static void pc_q35_init_1_4(MachineState *machine)
     .hot_add_cpu = pc_hot_add_cpu, \
     .units_per_default_bus = 1
 
-#define PC_Q35_2_3_MACHINE_OPTIONS                      \
+#define PC_Q35_2_4_MACHINE_OPTIONS                      \
     PC_Q35_MACHINE_OPTIONS,                             \
     .default_machine_opts = "firmware=bios-256k.bin",   \
     .default_display = "std"
 
+static QEMUMachine pc_q35_machine_v2_4 = {
+    PC_Q35_2_4_MACHINE_OPTIONS,
+    .name = "pc-q35-2.4",
+    .alias = "q35",
+    .init = pc_q35_init,
+};
+
+#define PC_Q35_2_3_MACHINE_OPTIONS PC_Q35_2_4_MACHINE_OPTIONS
+
 static QEMUMachine pc_q35_machine_v2_3 = {
     PC_Q35_2_3_MACHINE_OPTIONS,
     .name = "pc-q35-2.3",
-    .alias = "q35",
-    .init = pc_q35_init,
+    .init = pc_q35_init_2_3,
+    .compat_props = (GlobalProperty[]) {
+        PC_COMPAT_2_3,
+        { /* end of list */ }
+    },
 };
 
 #define PC_Q35_2_2_MACHINE_OPTIONS PC_Q35_2_3_MACHINE_OPTIONS
@@ -428,6 +450,10 @@ static QEMUMachine pc_q35_machine_v2_2 = {
     PC_Q35_2_2_MACHINE_OPTIONS,
     .name = "pc-q35-2.2",
     .init = pc_q35_init_2_2,
+    .compat_props = (GlobalProperty[]) {
+        PC_COMPAT_2_3,
+        { /* end of list */ }
+    },
 };
 
 #define PC_Q35_2_1_MACHINE_OPTIONS                      \
@@ -440,6 +466,7 @@ static QEMUMachine pc_q35_machine_v2_1 = {
     .init = pc_q35_init_2_1,
     .compat_props = (GlobalProperty[]) {
         HW_COMPAT_2_1,
+        PC_COMPAT_2_3,
         { /* end of list */ }
     },
 };
@@ -506,6 +533,7 @@ static QEMUMachine pc_q35_machine_v1_4 = {
 
 static void pc_q35_machine_init(void)
 {
+    qemu_register_pc_machine(&pc_q35_machine_v2_4);
     qemu_register_pc_machine(&pc_q35_machine_v2_3);
     qemu_register_pc_machine(&pc_q35_machine_v2_2);
     qemu_register_pc_machine(&pc_q35_machine_v2_1);
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 1b35168..05ac5f9 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -295,7 +295,10 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
 int e820_get_num_entries(void);
 bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
 
+#define PC_COMPAT_2_3 \
+
 #define PC_COMPAT_2_0 \
+        PC_COMPAT_2_3, \
         HW_COMPAT_2_1, \
         {\
             .driver   = "virtio-scsi-pci",\
-- 
1.7.12.4

[Qemu-devel] [PATCH 2/2] x86: Fix Opteron xlevels

[Alexander Graf]

The AMD Opteron family has different xlevel levels depending on the
generation. I looked up Gen1, Gen2 and Gen3 hardware and adapted the
levels according to real silicon.

The reason this came up is that there is a sanity check in KVM making
sure that SVM is only used when xlevel is high enough. Using real
hardware levels, they now are.

Reported-by: Bernhard M. Wiedemann <bwiedemann@suse.de>
Signed-off-by: Alexander Graf <agraf@suse.de>

---

v1 -> v2:

  - add compat handling for machine types < 2.4
---
 include/hw/i386/pc.h | 13 +++++++++++++
 target-i386/cpu.c    |  6 +++---
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 05ac5f9..c3cfe18 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -296,6 +296,19 @@ int e820_get_num_entries(void);
 bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
 
 #define PC_COMPAT_2_3 \
+        {\
+            .driver   = "Opteron_G1-" TYPE_X86_CPU,\
+            .property = "xlevel",\
+            .value    = stringify(0x80000008),\
+        },{\
+            .driver   = "Opteron_G2-" TYPE_X86_CPU,\
+            .property = "xlevel",\
+            .value    = stringify(0x80000008),\
+        },{\
+            .driver   = "Opteron_G3-" TYPE_X86_CPU,\
+            .property = "xlevel",\
+            .value    = stringify(0x80000008),\
+        }
 
 #define PC_COMPAT_2_0 \
         PC_COMPAT_2_3, \
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 03b33cf..d1b1b8c 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1234,7 +1234,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
             CPUID_EXT2_MTRR | CPUID_EXT2_SYSCALL | CPUID_EXT2_APIC |
             CPUID_EXT2_CX8 | CPUID_EXT2_MCE | CPUID_EXT2_PAE | CPUID_EXT2_MSR |
             CPUID_EXT2_TSC | CPUID_EXT2_PSE | CPUID_EXT2_DE | CPUID_EXT2_FPU,
-        .xlevel = 0x80000008,
+        .xlevel = 0x80000018,
         .model_id = "AMD Opteron 240 (Gen 1 Class Opteron)",
     },
     {
@@ -1262,7 +1262,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
             CPUID_EXT2_DE | CPUID_EXT2_FPU,
         .features[FEAT_8000_0001_ECX] =
             CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM,
-        .xlevel = 0x80000008,
+        .xlevel = 0x80000018,
         .model_id = "AMD Opteron 22xx (Gen 2 Class Opteron)",
     },
     {
@@ -1292,7 +1292,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .features[FEAT_8000_0001_ECX] =
             CPUID_EXT3_MISALIGNSSE | CPUID_EXT3_SSE4A |
             CPUID_EXT3_ABM | CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM,
-        .xlevel = 0x80000008,
+        .xlevel = 0x8000001A,
         .model_id = "AMD Opteron 23xx (Gen 3 Class Opteron)",
     },
     {
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 2/2] x86: Fix Opteron xlevels

[Eduardo Habkost]

On Wed, Apr 22, 2015 at 12:38:11AM +0200, Alexander Graf wrote:
> The AMD Opteron family has different xlevel levels depending on the
> generation. I looked up Gen1, Gen2 and Gen3 hardware and adapted the
> levels according to real silicon.
> 
> The reason this came up is that there is a sanity check in KVM making
> sure that SVM is only used when xlevel is high enough. Using real
> hardware levels, they now are.
> 

As noted in the reply to the previous patch: if you increase xlevel you are now
reporting new CPUID leaves as available. I still don't see any reason to start
reporting new CPUID leaves as available if they are returning useless data
(that doesn't match real hardware).

For reference, here are the new CPUID leaves you are introducing (and will
return all-zeroes):

For Opteron_G[123]:
* CPUID Fn8000_0009 Reserved
* CPUID Fn8000_000A SVM Revision and Feature Identification (supported by QEMU)
* CPUID Fn8000_00[18:0B] Reserved

For Opteron_G3:
* CPUID Fn8000_0019_EAX TLB 1GB Page Identifiers
* CPUID Fn8000_0019_EBX TLB 1GB Page Identifiers
* CPUID Fn8000_0019_E[D,C]X Reserved
* CPUID Fn8000_001A_EAX Performance Optimization Identifiers
* CPUID Fn8000_001A_E[D,C,B]X Reserved

The reserved bits look safe, but we can't be 100% sure unless we confirm that
real hardware is returning zeroes on all the reserved bits. If real hardware
return some data, we don't know what exactly that data means (and what
all-zeroes would mean) until AMD documents it.

For L1 and L2 1GB page TLB info, all-zeroes means "TLB disabled".

The Performance Optimization Identifier leaf will now be available, and will
return 0 on the following bits:

* "MOVU: MOVU SSE (multimedia) instructions are more efficient and should be
  preferred to SSE (multimedia) MOVL/MOVH. MOVUPS is more efficient than
  MOVLPS/MOVHPS. MOVUPD is more efficient than MOVLPD/MOVHP";
* "FP128: 128-bit SSE (multimedia) instructions are executed with full-width
  internal operations and pipelines rather than decomposing them into internal
  64-bit suboperations. This may impact how soft- ware performs instruction
  selection and scheduling."

There's a simple way to avoid exposing additional useless information to
guests: just return 0x8000000A on xlevel. 0x8000000A is the CPUID leaf you need
for SVM, the others are unrelated to SVM.

> Reported-by: Bernhard M. Wiedemann <bwiedemann@suse.de>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> ---
> 
> v1 -> v2:
> 
>   - add compat handling for machine types < 2.4
> ---
>  include/hw/i386/pc.h | 13 +++++++++++++
>  target-i386/cpu.c    |  6 +++---
>  2 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 05ac5f9..c3cfe18 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -296,6 +296,19 @@ int e820_get_num_entries(void);
>  bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>  
>  #define PC_COMPAT_2_3 \
> +        {\
> +            .driver   = "Opteron_G1-" TYPE_X86_CPU,\
> +            .property = "xlevel",\
> +            .value    = stringify(0x80000008),\
> +        },{\
> +            .driver   = "Opteron_G2-" TYPE_X86_CPU,\
> +            .property = "xlevel",\
> +            .value    = stringify(0x80000008),\
> +        },{\
> +            .driver   = "Opteron_G3-" TYPE_X86_CPU,\
> +            .property = "xlevel",\
> +            .value    = stringify(0x80000008),\
> +        }
>  
>  #define PC_COMPAT_2_0 \
>          PC_COMPAT_2_3, \
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 03b33cf..d1b1b8c 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1234,7 +1234,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
>              CPUID_EXT2_MTRR | CPUID_EXT2_SYSCALL | CPUID_EXT2_APIC |
>              CPUID_EXT2_CX8 | CPUID_EXT2_MCE | CPUID_EXT2_PAE | CPUID_EXT2_MSR |
>              CPUID_EXT2_TSC | CPUID_EXT2_PSE | CPUID_EXT2_DE | CPUID_EXT2_FPU,
> -        .xlevel = 0x80000008,
> +        .xlevel = 0x80000018,
>          .model_id = "AMD Opteron 240 (Gen 1 Class Opteron)",
>      },
>      {
> @@ -1262,7 +1262,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
>              CPUID_EXT2_DE | CPUID_EXT2_FPU,
>          .features[FEAT_8000_0001_ECX] =
>              CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM,
> -        .xlevel = 0x80000008,
> +        .xlevel = 0x80000018,
>          .model_id = "AMD Opteron 22xx (Gen 2 Class Opteron)",
>      },
>      {
> @@ -1292,7 +1292,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
>          .features[FEAT_8000_0001_ECX] =
>              CPUID_EXT3_MISALIGNSSE | CPUID_EXT3_SSE4A |
>              CPUID_EXT3_ABM | CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM,
> -        .xlevel = 0x80000008,
> +        .xlevel = 0x8000001A,
>          .model_id = "AMD Opteron 23xx (Gen 3 Class Opteron)",
>      },
>      {
> -- 
> 1.7.12.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/2] x86: Fix Opteron xlevels

[Alexander Graf]

> Am 22.04.2015 um 17:53 schrieb Eduardo Habkost <ehabkost@redhat.com>:
> 
>> On Wed, Apr 22, 2015 at 12:38:11AM +0200, Alexander Graf wrote:
>> The AMD Opteron family has different xlevel levels depending on the
>> generation. I looked up Gen1, Gen2 and Gen3 hardware and adapted the
>> levels according to real silicon.
>> 
>> The reason this came up is that there is a sanity check in KVM making
>> sure that SVM is only used when xlevel is high enough. Using real
>> hardware levels, they now are.
> 
> As noted in the reply to the previous patch: if you increase xlevel you are now
> reporting new CPUID leaves as available. I still don't see any reason to start
> reporting new CPUID leaves as available if they are returning useless data
> (that doesn't match real hardware).
> 
> For reference, here are the new CPUID leaves you are introducing (and will
> return all-zeroes):
> 
> For Opteron_G[123]:
> * CPUID Fn8000_0009 Reserved
> * CPUID Fn8000_000A SVM Revision and Feature Identification (supported by QEMU)
> * CPUID Fn8000_00[18:0B] Reserved
> 
> For Opteron_G3:
> * CPUID Fn8000_0019_EAX TLB 1GB Page Identifiers
> * CPUID Fn8000_0019_EBX TLB 1GB Page Identifiers
> * CPUID Fn8000_0019_E[D,C]X Reserved
> * CPUID Fn8000_001A_EAX Performance Optimization Identifiers
> * CPUID Fn8000_001A_E[D,C,B]X Reserved
> 
> The reserved bits look safe, but we can't be 100% sure unless we confirm that
> real hardware is returning zeroes on all the reserved bits. If real hardware
> return some data, we don't know what exactly that data means (and what
> all-zeroes would mean) until AMD documents it.
> 
> For L1 and L2 1GB page TLB info, all-zeroes means "TLB disabled".
> 
> The Performance Optimization Identifier leaf will now be available, and will
> return 0 on the following bits:
> 
> * "MOVU: MOVU SSE (multimedia) instructions are more efficient and should be
>  preferred to SSE (multimedia) MOVL/MOVH. MOVUPS is more efficient than
>  MOVLPS/MOVHPS. MOVUPD is more efficient than MOVLPD/MOVHP";
> * "FP128: 128-bit SSE (multimedia) instructions are executed with full-width
>  internal operations and pipelines rather than decomposing them into internal
>  64-bit suboperations. This may impact how soft- ware performs instruction
>  selection and scheduling."
> 
> There's a simple way to avoid exposing additional useless information to
> guests: just return 0x8000000A on xlevel. 0x8000000A is the CPUID leaf you need
> for SVM, the others are unrelated to SVM.

I don't think creating a more Frankenstein cpu definition is superior over unset bits in the new leafs. How about I add support for the new leafs and fill the bits with the same information as real hardware?


Alex

Re: [Qemu-devel] [PATCH 2/2] x86: Fix Opteron xlevels

[Eduardo Habkost]

On Wed, Apr 22, 2015 at 07:19:22PM +0200, Alexander Graf wrote:
> 
> 
> > Am 22.04.2015 um 17:53 schrieb Eduardo Habkost <ehabkost@redhat.com>:
> > 
> >> On Wed, Apr 22, 2015 at 12:38:11AM +0200, Alexander Graf wrote:
> >> The AMD Opteron family has different xlevel levels depending on the
> >> generation. I looked up Gen1, Gen2 and Gen3 hardware and adapted the
> >> levels according to real silicon.
> >> 
> >> The reason this came up is that there is a sanity check in KVM making
> >> sure that SVM is only used when xlevel is high enough. Using real
> >> hardware levels, they now are.
> > 
> > As noted in the reply to the previous patch: if you increase xlevel you are now
> > reporting new CPUID leaves as available. I still don't see any reason to start
> > reporting new CPUID leaves as available if they are returning useless data
> > (that doesn't match real hardware).
> > 
> > For reference, here are the new CPUID leaves you are introducing (and will
> > return all-zeroes):
> > 
> > For Opteron_G[123]:
> > * CPUID Fn8000_0009 Reserved
> > * CPUID Fn8000_000A SVM Revision and Feature Identification (supported by QEMU)
> > * CPUID Fn8000_00[18:0B] Reserved
> > 
> > For Opteron_G3:
> > * CPUID Fn8000_0019_EAX TLB 1GB Page Identifiers
> > * CPUID Fn8000_0019_EBX TLB 1GB Page Identifiers
> > * CPUID Fn8000_0019_E[D,C]X Reserved
> > * CPUID Fn8000_001A_EAX Performance Optimization Identifiers
> > * CPUID Fn8000_001A_E[D,C,B]X Reserved
> > 
> > The reserved bits look safe, but we can't be 100% sure unless we confirm that
> > real hardware is returning zeroes on all the reserved bits. If real hardware
> > return some data, we don't know what exactly that data means (and what
> > all-zeroes would mean) until AMD documents it.
> > 
> > For L1 and L2 1GB page TLB info, all-zeroes means "TLB disabled".
> > 
> > The Performance Optimization Identifier leaf will now be available, and will
> > return 0 on the following bits:
> > 
> > * "MOVU: MOVU SSE (multimedia) instructions are more efficient and should be
> >  preferred to SSE (multimedia) MOVL/MOVH. MOVUPS is more efficient than
> >  MOVLPS/MOVHPS. MOVUPD is more efficient than MOVLPD/MOVHP";
> > * "FP128: 128-bit SSE (multimedia) instructions are executed with full-width
> >  internal operations and pipelines rather than decomposing them into internal
> >  64-bit suboperations. This may impact how soft- ware performs instruction
> >  selection and scheduling."
> > 
> > There's a simple way to avoid exposing additional useless information to
> > guests: just return 0x8000000A on xlevel. 0x8000000A is the CPUID leaf you need
> > for SVM, the others are unrelated to SVM.
> 
> I don't think creating a more Frankenstein cpu definition is superior
> over unset bits in the new leafs. How about I add support for the new
> leafs and fill the bits with the same information as real hardware?

I don't see why xlevel=0x8000000A is "more Frankenstein". You seem to be
worrying more about the xlevel value (which doesn't have any meaning
except for "you can run CPUID with EAX=<value>") than the actual CPUID
contents, but I don't understand why.

Both options don't match real hardware (and can be called
"Frakensteins"). One option just hides information that don't match real
hardware, and the other exposes more mismatching information (that can
actually confuse guests).

If you volunteer to add support to the new leafs, I won't have
objections. To be honest, after the analysis above I am more bothered
about having those new leafs being added without anything mentioning
them in the cpu_x86_cpuid() code (even if just code comments), than
about what guests will do when they see them.

BTW, leaving the "Frankensteinness" discussion apart, we have an
additional issue with the higher xlevel: even if it doesn't break
anything (I don't think it will), it will cause us trouble on the day we
decide to change the contents of those new leaves to match real hardware
(because then we are going to make a guest-visible change that requires
compatibility code). But this is already an issue on Opteron_G[45]. :(

Finally, considering that both solutions are equally "Frankensteins" in
my opinion, but you are the one who spent time writing the code:

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

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/2] x86: Add 2.4 pc machine versions

[Eduardo Habkost]

(CCing the PC maintainer, Michael)

On Wed, Apr 22, 2015 at 12:38:10AM +0200, Alexander Graf wrote:
> This patch adds all the boilerplate required to handle a 2.4 machine type
> and compatibility options required on older machines.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  hw/i386/pc_piix.c    | 37 +++++++++++++++++++++++++++++++++----
>  hw/i386/pc_q35.c     | 34 +++++++++++++++++++++++++++++++---
>  include/hw/i386/pc.h |  3 +++
>  3 files changed, 67 insertions(+), 7 deletions(-)
> 

This doesn't build:

  CC    x86_64-softmmu/hw/i386/pc_piix.o
/home/ehabkost/rh/proj/virt/qemu/hw/i386/pc_piix.c:546:9: error: expected expression before ‘,’ token
         PC_COMPAT_2_3,
         ^
/home/ehabkost/rh/proj/virt/qemu/hw/i386/pc_piix.c:558:9: error: expected expression before ‘,’ token
         PC_COMPAT_2_3,
         ^

> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 1fe7bfb..cf14b1f 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -310,8 +310,13 @@ static void pc_init_pci(MachineState *machine)
>      pc_init1(machine, 1, 1);
>  }
>  
> +static void pc_compat_2_3(MachineState *machine)
> +{
> +}
> +
>  static void pc_compat_2_2(MachineState *machine)
>  {
> +    pc_compat_2_3(machine);
>      rsdp_in_ram = false;
>      x86_cpu_compat_set_features("kvm64", FEAT_1_EDX, 0, CPUID_VME);
>      x86_cpu_compat_set_features("kvm32", FEAT_1_EDX, 0, CPUID_VME);
> @@ -413,6 +418,12 @@ static void pc_compat_1_2(MachineState *machine)
>      x86_cpu_compat_kvm_no_autoenable(FEAT_KVM, 1 << KVM_FEATURE_PV_EOI);
>  }
>  
> +static void pc_init_pci_2_3(MachineState *machine)
> +{
> +    pc_compat_2_3(machine);
> +    pc_init_pci(machine);
> +}
> +
>  static void pc_init_pci_2_2(MachineState *machine)
>  {
>      pc_compat_2_2(machine);
> @@ -512,25 +523,41 @@ static void pc_xen_hvm_init(MachineState *machine)
>      .desc = "Standard PC (i440FX + PIIX, 1996)", \
>      .hot_add_cpu = pc_hot_add_cpu
>  
> -#define PC_I440FX_2_3_MACHINE_OPTIONS                           \
> +#define PC_I440FX_2_4_MACHINE_OPTIONS                           \
>      PC_I440FX_MACHINE_OPTIONS,                                  \
>      .default_machine_opts = "firmware=bios-256k.bin",           \
>      .default_display = "std"
>  
> -static QEMUMachine pc_i440fx_machine_v2_3 = {
> -    PC_I440FX_2_3_MACHINE_OPTIONS,
> -    .name = "pc-i440fx-2.3",
> +static QEMUMachine pc_i440fx_machine_v2_4 = {
> +    PC_I440FX_2_4_MACHINE_OPTIONS,
> +    .name = "pc-i440fx-2.4",
>      .alias = "pc",
>      .init = pc_init_pci,
>      .is_default = 1,
>  };
>  
> +#define PC_I440FX_2_3_MACHINE_OPTIONS PC_I440FX_2_4_MACHINE_OPTIONS
> +
> +static QEMUMachine pc_i440fx_machine_v2_3 = {
> +    PC_I440FX_2_3_MACHINE_OPTIONS,
> +    .name = "pc-i440fx-2.3",
> +    .init = pc_init_pci_2_3,
> +    .compat_props = (GlobalProperty[]) {
> +        PC_COMPAT_2_3,
> +        { /* end of list */ }
> +    },
> +};
> +
>  #define PC_I440FX_2_2_MACHINE_OPTIONS PC_I440FX_2_3_MACHINE_OPTIONS
>  
>  static QEMUMachine pc_i440fx_machine_v2_2 = {
>      PC_I440FX_2_2_MACHINE_OPTIONS,
>      .name = "pc-i440fx-2.2",
>      .init = pc_init_pci_2_2,
> +    .compat_props = (GlobalProperty[]) {
> +        PC_COMPAT_2_3,
> +        { /* end of list */ }
> +    },
>  };
>  
>  #define PC_I440FX_2_1_MACHINE_OPTIONS                           \
> @@ -543,6 +570,7 @@ static QEMUMachine pc_i440fx_machine_v2_1 = {
>      .init = pc_init_pci_2_1,
>      .compat_props = (GlobalProperty[]) {
>          HW_COMPAT_2_1,
> +        PC_COMPAT_2_3,

PC_COMPAT_2_3 must be in the beginning of the list, not in the end. To make
QEMU 2.4 behave like QEMU 2.1, first we apply the properties that make QEMU 2.4
behave as QEMU 2.3, then we apply the properties that made QEMU 2.3 behave as
QEMU 2.2, then we apply the properties that made QEMU 2.2 behave as QEMU 2.1.

Also, why did you decide to define a PC_COMPAT_2_3 macro and not define
a PC_COMPAT_2_2 macro?

We could make it more consistent and always define the PC_* macros even if they
are empty (like you already tried to do on PC_COMPAT_2_3), like this:

    #define PC_COMPAT_2_3 \
            /* empty */

    #define PC_COMPAT_2_2 \
            PC_COMPAT_2_3
            /* no extra elements */

    #define PC_COMPAT_2_1 \
            PC_COMPAT_2_2
            HW_COMPAT_2_1
            { /* item 1 */ },
            { /* item 2 */ },

    #define PC_COMPAT_2_0 \
            PC_COMPAT_2_1
            { /* item 1 */ },
            { /* item 2 */ },

Note that I moved the final commas inside the macros, to allow us to define
empty compat macros without compilation errors like the one introduced by this
patch.


>          { /* end of list */ }
>      },
>  };
> @@ -970,6 +998,7 @@ static QEMUMachine xenfv_machine = {
>  
>  static void pc_machine_init(void)
>  {
> +    qemu_register_pc_machine(&pc_i440fx_machine_v2_4);
>      qemu_register_pc_machine(&pc_i440fx_machine_v2_3);
>      qemu_register_pc_machine(&pc_i440fx_machine_v2_2);
>      qemu_register_pc_machine(&pc_i440fx_machine_v2_1);
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index dcc17c0..8ef9e27 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -289,6 +289,10 @@ static void pc_q35_init(MachineState *machine)
>      }
>  }
>  
> +static void pc_compat_2_3(MachineState *machine)
> +{
> +}
> +
>  static void pc_compat_2_2(MachineState *machine)
>  {
>      rsdp_in_ram = false;
> @@ -361,6 +365,12 @@ static void pc_compat_1_4(MachineState *machine)
>      x86_cpu_compat_set_features("Westmere", FEAT_1_ECX, 0, CPUID_EXT_PCLMULQDQ);
>  }
>  
> +static void pc_q35_init_2_3(MachineState *machine)
> +{
> +    pc_compat_2_3(machine);
> +    pc_q35_init(machine);
> +}
> +
>  static void pc_q35_init_2_2(MachineState *machine)
>  {
>      pc_compat_2_2(machine);
> @@ -410,16 +420,28 @@ static void pc_q35_init_1_4(MachineState *machine)
>      .hot_add_cpu = pc_hot_add_cpu, \
>      .units_per_default_bus = 1
>  
> -#define PC_Q35_2_3_MACHINE_OPTIONS                      \
> +#define PC_Q35_2_4_MACHINE_OPTIONS                      \
>      PC_Q35_MACHINE_OPTIONS,                             \
>      .default_machine_opts = "firmware=bios-256k.bin",   \
>      .default_display = "std"
>  
> +static QEMUMachine pc_q35_machine_v2_4 = {
> +    PC_Q35_2_4_MACHINE_OPTIONS,
> +    .name = "pc-q35-2.4",
> +    .alias = "q35",
> +    .init = pc_q35_init,
> +};
> +
> +#define PC_Q35_2_3_MACHINE_OPTIONS PC_Q35_2_4_MACHINE_OPTIONS
> +
>  static QEMUMachine pc_q35_machine_v2_3 = {
>      PC_Q35_2_3_MACHINE_OPTIONS,
>      .name = "pc-q35-2.3",
> -    .alias = "q35",
> -    .init = pc_q35_init,
> +    .init = pc_q35_init_2_3,
> +    .compat_props = (GlobalProperty[]) {
> +        PC_COMPAT_2_3,
> +        { /* end of list */ }
> +    },
>  };
>  
>  #define PC_Q35_2_2_MACHINE_OPTIONS PC_Q35_2_3_MACHINE_OPTIONS
> @@ -428,6 +450,10 @@ static QEMUMachine pc_q35_machine_v2_2 = {
>      PC_Q35_2_2_MACHINE_OPTIONS,
>      .name = "pc-q35-2.2",
>      .init = pc_q35_init_2_2,
> +    .compat_props = (GlobalProperty[]) {
> +        PC_COMPAT_2_3,
> +        { /* end of list */ }
> +    },
>  };
>  
>  #define PC_Q35_2_1_MACHINE_OPTIONS                      \
> @@ -440,6 +466,7 @@ static QEMUMachine pc_q35_machine_v2_1 = {
>      .init = pc_q35_init_2_1,
>      .compat_props = (GlobalProperty[]) {
>          HW_COMPAT_2_1,
> +        PC_COMPAT_2_3,
>          { /* end of list */ }
>      },
>  };
> @@ -506,6 +533,7 @@ static QEMUMachine pc_q35_machine_v1_4 = {
>  
>  static void pc_q35_machine_init(void)
>  {
> +    qemu_register_pc_machine(&pc_q35_machine_v2_4);
>      qemu_register_pc_machine(&pc_q35_machine_v2_3);
>      qemu_register_pc_machine(&pc_q35_machine_v2_2);
>      qemu_register_pc_machine(&pc_q35_machine_v2_1);
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 1b35168..05ac5f9 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -295,7 +295,10 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
>  int e820_get_num_entries(void);
>  bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>  
> +#define PC_COMPAT_2_3 \
> +
>  #define PC_COMPAT_2_0 \
> +        PC_COMPAT_2_3, \
>          HW_COMPAT_2_1, \
>          {\
>              .driver   = "virtio-scsi-pci",\
> -- 
> 1.7.12.4
> 
> 

-- 
Eduardo