* [Qemu-devel] [PATCH] pc: cleanup 1.4 compat support
@ 2013-08-18 13:50 Michael S. Tsirkin
2013-08-18 14:17 ` Andreas Färber
2013-08-19 12:04 ` Eduardo Habkost
0 siblings, 2 replies; 7+ messages in thread
From: Michael S. Tsirkin @ 2013-08-18 13:50 UTC (permalink / raw)
To: qemu-devel
Cc: Anthony Liguori, Eduardo Habkost, Gerd Hoffmann, Paolo Bonzini,
Andreas Färber
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 <mst@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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH] pc: cleanup 1.4 compat support
2013-08-18 13:50 [Qemu-devel] [PATCH] pc: cleanup 1.4 compat support Michael S. Tsirkin
@ 2013-08-18 14:17 ` Andreas Färber
2013-08-19 12:04 ` Eduardo Habkost
1 sibling, 0 replies; 7+ messages in thread
From: Andreas Färber @ 2013-08-18 14:17 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Paolo Bonzini, Anthony Liguori, qemu-devel, Eduardo Habkost,
Gerd Hoffmann
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 <mst@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 <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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] pc: cleanup 1.4 compat support
2013-08-18 13:50 [Qemu-devel] [PATCH] pc: cleanup 1.4 compat support Michael S. Tsirkin
2013-08-18 14:17 ` Andreas Färber
@ 2013-08-19 12:04 ` Eduardo Habkost
2013-08-19 13:02 ` Michael S. Tsirkin
1 sibling, 1 reply; 7+ messages in thread
From: Eduardo Habkost @ 2013-08-19 12:04 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Paolo Bonzini, Anthony Liguori, Gerd Hoffmann, qemu-devel,
Andreas Färber
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 <mst@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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH] pc: cleanup 1.4 compat support
2013-08-19 12:04 ` Eduardo Habkost
@ 2013-08-19 13:02 ` Michael S. Tsirkin
2013-08-19 14:19 ` Eduardo Habkost
0 siblings, 1 reply; 7+ messages in thread
From: Michael S. Tsirkin @ 2013-08-19 13:02 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Paolo Bonzini, Anthony Liguori, Gerd Hoffmann, qemu-devel,
Andreas Färber
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 <mst@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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH] pc: cleanup 1.4 compat support
2013-08-19 13:02 ` Michael S. Tsirkin
@ 2013-08-19 14:19 ` Eduardo Habkost
2013-08-19 14:28 ` Michael S. Tsirkin
0 siblings, 1 reply; 7+ messages in thread
From: Eduardo Habkost @ 2013-08-19 14:19 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Paolo Bonzini, Anthony Liguori, Gerd Hoffmann, qemu-devel,
Andreas Färber
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 <mst@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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH] pc: cleanup 1.4 compat support
2013-08-19 14:19 ` Eduardo Habkost
@ 2013-08-19 14:28 ` Michael S. Tsirkin
2013-08-19 14:52 ` Eduardo Habkost
0 siblings, 1 reply; 7+ messages in thread
From: Michael S. Tsirkin @ 2013-08-19 14:28 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Paolo Bonzini, Anthony Liguori, Gerd Hoffmann, qemu-devel,
Andreas Färber
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 <mst@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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH] pc: cleanup 1.4 compat support
2013-08-19 14:28 ` Michael S. Tsirkin
@ 2013-08-19 14:52 ` Eduardo Habkost
0 siblings, 0 replies; 7+ messages in thread
From: Eduardo Habkost @ 2013-08-19 14:52 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Paolo Bonzini, Anthony Liguori, Gerd Hoffmann, qemu-devel,
Andreas Färber
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 <mst@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 <ehabkost@redhat.com>
--
Eduardo
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-08-19 18:10 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-18 13:50 [Qemu-devel] [PATCH] pc: cleanup 1.4 compat support Michael S. Tsirkin
2013-08-18 14:17 ` Andreas Färber
2013-08-19 12:04 ` Eduardo Habkost
2013-08-19 13:02 ` Michael S. Tsirkin
2013-08-19 14:19 ` Eduardo Habkost
2013-08-19 14:28 ` Michael S. Tsirkin
2013-08-19 14:52 ` Eduardo Habkost
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).