[Qemu-devel] [PATCH] pc: cleanup 1.4 compat support

[Qemu-devel] [PATCH] pc: cleanup 1.4 compat support

[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(-)

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

[Andreas Färber]

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

Re: [Qemu-devel] [PATCH] pc: cleanup 1.4 compat support

[Eduardo Habkost]

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

Re: [Qemu-devel] [PATCH] pc: cleanup 1.4 compat support

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PATCH] pc: cleanup 1.4 compat support

[Eduardo Habkost]

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

Re: [Qemu-devel] [PATCH] pc: cleanup 1.4 compat support

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PATCH] pc: cleanup 1.4 compat support

[Eduardo Habkost]

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