Re: [Qemu-devel] [PATCH] pc: cleanup 1.4 compat support
On Sun, Aug 18, 2013 at 04:50:02PM +0300, Michael S. Tsirkin wrote:
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 m...@redhat.com
---
hw/i386/pc_piix.c | 4 ++--
hw/i386/pc_q35.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 15c932e..86cfadc 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -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.
static void pc_init_pci_1_3(QEMUMachineInitArgs *args)
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 09d8183..8d62700 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -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 = {
--
MST
--
Eduardo
Re: [Qemu-devel] [PATCH] pc: cleanup 1.4 compat support
On Mon, Aug 19, 2013 at 04:02:10PM +0300, Michael S. Tsirkin wrote:
On Mon, Aug 19, 2013 at 09:04:28AM -0300, Eduardo Habkost wrote:
On Sun, Aug 18, 2013 at 04:50:02PM +0300, Michael S. Tsirkin wrote:
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 m...@redhat.com
---
hw/i386/pc_piix.c | 4 ++--
hw/i386/pc_q35.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 15c932e..86cfadc 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -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.
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.
--
Eduardo
Re: [Qemu-devel] [PATCH] pc: cleanup 1.4 compat support
On Mon, Aug 19, 2013 at 11:19:23AM -0300, Eduardo Habkost wrote:
On Mon, Aug 19, 2013 at 04:02:10PM +0300, Michael S. Tsirkin wrote:
On Mon, Aug 19, 2013 at 09:04:28AM -0300, Eduardo Habkost wrote:
On Sun, Aug 18, 2013 at 04:50:02PM +0300, Michael S. Tsirkin wrote:
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 m...@redhat.com
---
hw/i386/pc_piix.c | 4 ++--
hw/i386/pc_q35.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 15c932e..86cfadc 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -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.
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.
--
Eduardo
Re: [Qemu-devel] [PATCH] pc: cleanup 1.4 compat support
On Mon, Aug 19, 2013 at 05:28:45PM +0300, Michael S. Tsirkin wrote:
On Mon, Aug 19, 2013 at 11:19:23AM -0300, Eduardo Habkost wrote:
On Mon, Aug 19, 2013 at 04:02:10PM +0300, Michael S. Tsirkin wrote:
On Mon, Aug 19, 2013 at 09:04:28AM -0300, Eduardo Habkost wrote:
On Sun, Aug 18, 2013 at 04:50:02PM +0300, Michael S. Tsirkin wrote:
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 m...@redhat.com
---
hw/i386/pc_piix.c | 4 ++--
hw/i386/pc_q35.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 15c932e..86cfadc 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -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.
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 ehabk...@redhat.com
--
Eduardo
Re: [Qemu-devel] [PATCH] pc: cleanup 1.4 compat support
On Mon, Aug 19, 2013 at 09:04:28AM -0300, Eduardo Habkost wrote:
On Sun, Aug 18, 2013 at 04:50:02PM +0300, Michael S. Tsirkin wrote:
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 m...@redhat.com
---
hw/i386/pc_piix.c | 4 ++--
hw/i386/pc_q35.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 15c932e..86cfadc 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -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.
Sure, why not. Want to send a patch like this?
Prefer a patch on top of this one, or instead of it?
With this, we could even reuse the same pc_compat_*() functions on
pc_piix.c and pc_q35.c.
That would be nice.
static void pc_init_pci_1_3(QEMUMachineInitArgs *args)
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 09d8183..8d62700 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -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 = {
--
MST
--
Eduardo
[Qemu-devel] [PATCH] 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 m...@redhat.com
---
hw/i386/pc_piix.c | 4 ++--
hw/i386/pc_q35.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 15c932e..86cfadc 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -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)
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 09d8183..8d62700 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -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 = {
--
MST
Re: [Qemu-devel] [PATCH] pc: cleanup 1.4 compat support
Am 18.08.2013 15:50, schrieb 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 m...@redhat.com --- hw/i386/pc_piix.c | 4 ++-- hw/i386/pc_q35.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) Reviewed-by: Andreas Färber afaer...@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
