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 <address@hidden>
> ---
>  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