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 > > --- > > 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
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 > > > > > --- > > > > > 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 -- 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 > > > > --- > > > > 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 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 > > > --- > > > 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 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 > --- > 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
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 > --- > hw/i386/pc_piix.c | 4 ++-- > hw/i386/pc_q35.c | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) Reviewed-by: Andreas Färber Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
[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 --- 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