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.
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
