pc: cleanup 1.4 compat support

PATCH In review Michael S. Tsirkin 3 participants 7 messages Aug 18, 2013 – Aug 19, 2013

Share & tools

Michael S. Tsirkin

author

2013-08-18 13:48 UTC #0 lore raw

PATCH 1/1 pc: cleanup 1.4 compat support

Make 1.4 compat code call the 1.6 one, reducing code duplication. Add comment explaining why we can't make 1.4 call 1.5 as usual.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 hw/i386/pc_piix.c | 4 ++--
 hw/i386/pc_q35.c  | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

hw/i386/pc_piix.c +2 -2

index 15c932e..86cfadc 100644
@@ -272,8 +272,8 @@ static void pc_init_pci_1_4(QEMUMachineInitArgs *args)
 {
     x86_cpu_compat_set_features("n270", FEAT_1_ECX, 0, CPUID_EXT_MOVBE);
     x86_cpu_compat_set_features("Westmere", FEAT_1_ECX, 0, CPUID_EXT_PCLMULQDQ);
-    has_pci_info = false;
-    pc_init_pci(args);
+    /* 1.5 was special - it enabled pvpanic in builtin machine */
+    pc_init_pci_1_6(args);
 }
 
 static void pc_init_pci_1_3(QEMUMachineInitArgs *args)

hw/i386/pc_q35.c +2 -2

index 09d8183..8d62700 100644
@@ -239,8 +239,8 @@ static void pc_q35_init_1_4(QEMUMachineInitArgs *args)
 {
     x86_cpu_compat_set_features("n270", FEAT_1_ECX, 0, CPUID_EXT_MOVBE);
     x86_cpu_compat_set_features("Westmere", FEAT_1_ECX, 0, CPUID_EXT_PCLMULQDQ);
-    has_pci_info = false;
-    pc_q35_init(args);
+    /* 1.5 was special - it enabled pvpanic in builtin machine */
+    pc_q35_init_1_6(args);
 }
 
 static QEMUMachine pc_q35_machine_v1_6 = {

Signature · 1 line

MST

Andreas Färber

Reviewed-by on v1 1/1

2013-08-18 14:18 UTC #1 lore raw

Am 18.08.2013 15:50, schrieb Michael S. Tsirkin:

Quoted 9 lines

Make 1.4 compat code call the 1.6 one, reducing code duplication. Add comment explaining why we can't make 1.4 call 1.5 as usual.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
 hw/i386/pc_piix.c | 4 ++--
 hw/i386/pc_q35.c  | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

Reviewed-by Andreas Färber <[email protected]>

Andreas

Signature · 2 lines

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

Eduardo Habkost

2013-08-19 12:04 UTC #2 lore raw

In reply to Michael S. Tsirkin

Quoted 15 lines · Michael S. Tsirkin

Make 1.4 compat code call the 1.6 one, reducing code duplication. Add comment explaining why we can't make 1.4 call 1.5 as usual.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
 hw/i386/pc_piix.c | 4 ++--
 hw/i386/pc_q35.c  | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)
hw/i386/pc_piix.c +0 -0
index 15c932e..86cfadc 100644

hw/i386/pc_piix.c static void pc_init_pci_1_4(QEMUMachineInitArgs *args) · line 278

Quoted 9 lines

hw/i386/pc_piix.c +2 -2

@@ -272,8 +272,8 @@ static void pc_init_pci_1_4(QEMUMachineInitArgs *args)
 {
     x86_cpu_compat_set_features("n270", FEAT_1_ECX, 0, CPUID_EXT_MOVBE);
     x86_cpu_compat_set_features("Westmere", FEAT_1_ECX, 0, CPUID_EXT_PCLMULQDQ);
-    has_pci_info = false;
-    pc_init_pci(args);
+    /* 1.5 was special - it enabled pvpanic in builtin machine */
+    pc_init_pci_1_6(args);
 }

I find this confusing (and probably it will be even more confusing when people read this code a few years from now). I would prefer the following pattern:

static void pc_compat_1_5()

{
	/* do 1.5 stuff (possibly undoing 1.6 stuff) */
	has_pvpanic = true;
}

static void pc_init_pci_1_5()

{
	pc_compat_1_5();
	pc_init_pci();
}

static void pc_compat_1_4()

{
	pc_compat_1_5();
	/* do 1.4 stuff (possibly undoing 1.5 stuff) */
	has_pvpanic = false;
}

static void pc_init_pci_1_4()

{
	pc_compat_1_4();
	pc_init_pci();
}

static void pc_compat_1_3()

{
	pc_compat_1_3();
	/* do 1.3 stuff (possibly undoing 1.4 stuff) */
}

static void pc_init_pci_1_3()

{
	pc_compat_1_3();
	pc_init_pci();
}

Otherwise the same problem may happen again in the future.

With this, we could even reuse the same pc_compat_*() functions on pc_piix.c and pc_q35.c.

Quoted 18 lines

hw/i386/pc_piix.c +0 -0

 static void pc_init_pci_1_3(QEMUMachineInitArgs *args)

hw/i386/pc_q35.c +2 -2

index 09d8183..8d62700 100644
@@ -239,8 +239,8 @@ static void pc_q35_init_1_4(QEMUMachineInitArgs *args)
 {
     x86_cpu_compat_set_features("n270", FEAT_1_ECX, 0, CPUID_EXT_MOVBE);
     x86_cpu_compat_set_features("Westmere", FEAT_1_ECX, 0, CPUID_EXT_PCLMULQDQ);
-    has_pci_info = false;
-    pc_q35_init(args);
+    /* 1.5 was special - it enabled pvpanic in builtin machine */
+    pc_q35_init_1_6(args);
 }
 
 static QEMUMachine pc_q35_machine_v1_6 = {

Signature · 1 line

MST

Signature · 1 line

Eduardo

Michael S. Tsirkin

author

2013-08-19 18:10 UTC #3 lore raw

Quoted 61 lines · Eduardo Habkost

Quoted 15 lines · Michael S. Tsirkin
Make 1.4 compat code call the 1.6 one, reducing code duplication. Add comment explaining why we can't make 1.4 call 1.5 as usual.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
 hw/i386/pc_piix.c | 4 ++--
 hw/i386/pc_q35.c  | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)
hw/i386/pc_piix.c +0 -0
index 15c932e..86cfadc 100644
hw/i386/pc_piix.c static void pc_init_pci_1_4(QEMUMachineInitArgs *args) · line 278
Quoted 9 lines
hw/i386/pc_piix.c +2 -2
@@ -272,8 +272,8 @@ static void pc_init_pci_1_4(QEMUMachineInitArgs *args)
 {
     x86_cpu_compat_set_features("n270", FEAT_1_ECX, 0, CPUID_EXT_MOVBE);
     x86_cpu_compat_set_features("Westmere", FEAT_1_ECX, 0, CPUID_EXT_PCLMULQDQ);
-    has_pci_info = false;
-    pc_init_pci(args);
+    /* 1.5 was special - it enabled pvpanic in builtin machine */
+    pc_init_pci_1_6(args);
 }
 
I find this confusing (and probably it will be even more confusing when people read this code a few years from now). I would prefer the following pattern:
static void pc_compat_1_5()
{
	/* do 1.5 stuff (possibly undoing 1.6 stuff) */
	has_pvpanic = true;
}
static void pc_init_pci_1_5()
{
	pc_compat_1_5();
	pc_init_pci();
}
static void pc_compat_1_4()
{
	pc_compat_1_5();
	/* do 1.4 stuff (possibly undoing 1.5 stuff) */
	has_pvpanic = false;
}
static void pc_init_pci_1_4()
{
	pc_compat_1_4();
	pc_init_pci();
}
static void pc_compat_1_3()
{
	pc_compat_1_3();
	/* do 1.3 stuff (possibly undoing 1.4 stuff) */
}

Quoted 7 lines

static void pc_init_pci_1_3()
{
	pc_compat_1_3();
	pc_init_pci();
}
Otherwise the same problem may happen again in the future.

Sure, why not. Want to send a patch like this? Prefer a patch on top of this one, or instead of it?

Quoted 3 lines

With this, we could even reuse the same pc_compat_*() functions on pc_piix.c and pc_q35.c.

That would be nice.

Quoted 21 lines

Quoted 18 lines

hw/i386/pc_piix.c +0 -0

 static void pc_init_pci_1_3(QEMUMachineInitArgs *args)

hw/i386/pc_q35.c +2 -2

index 09d8183..8d62700 100644
@@ -239,8 +239,8 @@ static void pc_q35_init_1_4(QEMUMachineInitArgs *args)
 {
     x86_cpu_compat_set_features("n270", FEAT_1_ECX, 0, CPUID_EXT_MOVBE);
     x86_cpu_compat_set_features("Westmere", FEAT_1_ECX, 0, CPUID_EXT_PCLMULQDQ);
-    has_pci_info = false;
-    pc_q35_init(args);
+    /* 1.5 was special - it enabled pvpanic in builtin machine */
+    pc_q35_init_1_6(args);
 }
 
 static QEMUMachine pc_q35_machine_v1_6 = {

Signature · 1 line

MST

Signature · 1 line

Eduardo

Eduardo Habkost

2013-08-19 14:19 UTC #4 lore raw

Quoted 68 lines · Michael S. Tsirkin

Quoted 67 lines · Eduardo Habkost
Quoted 15 lines · Michael S. Tsirkin
Make 1.4 compat code call the 1.6 one, reducing code duplication. Add comment explaining why we can't make 1.4 call 1.5 as usual.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
 hw/i386/pc_piix.c | 4 ++--
 hw/i386/pc_q35.c  | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)
hw/i386/pc_piix.c +0 -0
index 15c932e..86cfadc 100644
hw/i386/pc_piix.c static void pc_init_pci_1_4(QEMUMachineInitArgs *args) · line 278
Quoted 9 lines
hw/i386/pc_piix.c +2 -2
@@ -272,8 +272,8 @@ static void pc_init_pci_1_4(QEMUMachineInitArgs *args)
 {
     x86_cpu_compat_set_features("n270", FEAT_1_ECX, 0, CPUID_EXT_MOVBE);
     x86_cpu_compat_set_features("Westmere", FEAT_1_ECX, 0, CPUID_EXT_PCLMULQDQ);
-    has_pci_info = false;
-    pc_init_pci(args);
+    /* 1.5 was special - it enabled pvpanic in builtin machine */
+    pc_init_pci_1_6(args);
 }
 
I find this confusing (and probably it will be even more confusing when people read this code a few years from now). I would prefer the following pattern:
static void pc_compat_1_5()
{
	/* do 1.5 stuff (possibly undoing 1.6 stuff) */
	has_pvpanic = true;
}
static void pc_init_pci_1_5()
{
	pc_compat_1_5();
	pc_init_pci();
}
static void pc_compat_1_4()
{
	pc_compat_1_5();
	/* do 1.4 stuff (possibly undoing 1.5 stuff) */
	has_pvpanic = false;
}
static void pc_init_pci_1_4()
{
	pc_compat_1_4();
	pc_init_pci();
}
static void pc_compat_1_3()
{
	pc_compat_1_3();
	/* do 1.3 stuff (possibly undoing 1.4 stuff) */
}
static void pc_init_pci_1_3()
{
	pc_compat_1_3();
	pc_init_pci();
}

Quoted 4 lines

Quoted 1 line
Otherwise the same problem may happen again in the future.
Sure, why not. Want to send a patch like this? Prefer a patch on top of this one, or instead of it?

If you are OK with it, I will send a patch replacing this one.

Signature · 1 line

Eduardo

Michael S. Tsirkin

author

2013-08-19 14:27 UTC #5 lore raw

Quoted 69 lines · Eduardo Habkost

Quoted 68 lines · Michael S. Tsirkin
Quoted 67 lines · Eduardo Habkost
Quoted 15 lines · Michael S. Tsirkin
Make 1.4 compat code call the 1.6 one, reducing code duplication. Add comment explaining why we can't make 1.4 call 1.5 as usual.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
 hw/i386/pc_piix.c | 4 ++--
 hw/i386/pc_q35.c  | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)
hw/i386/pc_piix.c +0 -0
index 15c932e..86cfadc 100644
hw/i386/pc_piix.c static void pc_init_pci_1_4(QEMUMachineInitArgs *args) · line 278
Quoted 9 lines
hw/i386/pc_piix.c +2 -2
@@ -272,8 +272,8 @@ static void pc_init_pci_1_4(QEMUMachineInitArgs *args)
 {
     x86_cpu_compat_set_features("n270", FEAT_1_ECX, 0, CPUID_EXT_MOVBE);
     x86_cpu_compat_set_features("Westmere", FEAT_1_ECX, 0, CPUID_EXT_PCLMULQDQ);
-    has_pci_info = false;
-    pc_init_pci(args);
+    /* 1.5 was special - it enabled pvpanic in builtin machine */
+    pc_init_pci_1_6(args);
 }
 
I find this confusing (and probably it will be even more confusing when people read this code a few years from now). I would prefer the following pattern:
static void pc_compat_1_5()
{
	/* do 1.5 stuff (possibly undoing 1.6 stuff) */
	has_pvpanic = true;
}
static void pc_init_pci_1_5()
{
	pc_compat_1_5();
	pc_init_pci();
}
static void pc_compat_1_4()
{
	pc_compat_1_5();
	/* do 1.4 stuff (possibly undoing 1.5 stuff) */
	has_pvpanic = false;
}
static void pc_init_pci_1_4()
{
	pc_compat_1_4();
	pc_init_pci();
}
static void pc_compat_1_3()
{
	pc_compat_1_3();
	/* do 1.3 stuff (possibly undoing 1.4 stuff) */
}
static void pc_init_pci_1_3()
{
	pc_compat_1_3();
	pc_init_pci();
}

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
Otherwise the same problem may happen again in the future.
Sure, why not. Want to send a patch like this? Prefer a patch on top of this one, or instead of it?
If you are OK with it, I will send a patch replacing this one.

Sure, go ahead.

Quoted 2 lines

Signature · 1 line
Eduardo

Eduardo Habkost

Reviewed-by

2013-08-19 14:52 UTC #6 lore raw

Quoted 70 lines · Michael S. Tsirkin

Quoted 69 lines · Eduardo Habkost
Quoted 68 lines · Michael S. Tsirkin
Quoted 67 lines · Eduardo Habkost
Quoted 15 lines · Michael S. Tsirkin
Make 1.4 compat code call the 1.6 one, reducing code duplication. Add comment explaining why we can't make 1.4 call 1.5 as usual.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
 hw/i386/pc_piix.c | 4 ++--
 hw/i386/pc_q35.c  | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)
hw/i386/pc_piix.c +0 -0
index 15c932e..86cfadc 100644
hw/i386/pc_piix.c static void pc_init_pci_1_4(QEMUMachineInitArgs *args) · line 278
Quoted 9 lines
hw/i386/pc_piix.c +2 -2
@@ -272,8 +272,8 @@ static void pc_init_pci_1_4(QEMUMachineInitArgs *args)
 {
     x86_cpu_compat_set_features("n270", FEAT_1_ECX, 0, CPUID_EXT_MOVBE);
     x86_cpu_compat_set_features("Westmere", FEAT_1_ECX, 0, CPUID_EXT_PCLMULQDQ);
-    has_pci_info = false;
-    pc_init_pci(args);
+    /* 1.5 was special - it enabled pvpanic in builtin machine */
+    pc_init_pci_1_6(args);
 }
 
I find this confusing (and probably it will be even more confusing when people read this code a few years from now). I would prefer the following pattern:
static void pc_compat_1_5()
{
	/* do 1.5 stuff (possibly undoing 1.6 stuff) */
	has_pvpanic = true;
}
static void pc_init_pci_1_5()
{
	pc_compat_1_5();
	pc_init_pci();
}
static void pc_compat_1_4()
{
	pc_compat_1_5();
	/* do 1.4 stuff (possibly undoing 1.5 stuff) */
	has_pvpanic = false;
}
static void pc_init_pci_1_4()
{
	pc_compat_1_4();
	pc_init_pci();
}
static void pc_compat_1_3()
{
	pc_compat_1_3();
	/* do 1.3 stuff (possibly undoing 1.4 stuff) */
}
static void pc_init_pci_1_3()
{
	pc_compat_1_3();
	pc_init_pci();
}

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Otherwise the same problem may happen again in the future.
Sure, why not. Want to send a patch like this? Prefer a patch on top of this one, or instead of it?
If you are OK with it, I will send a patch replacing this one.
Sure, go ahead.

Actually, now that I am looking at it, the changes will be easier to review if we apply your patch first. So:

Reviewed-by Eduardo Habkost <[email protected]>

Signature · 1 line

Eduardo