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
