| Message ID | 1448616740-27332-2-git-send-email-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 27.11.15 10:32, Thomas Huth wrote: > Add a new pseries-2.6 machine class version to make sure we can > keep the old types compatible to previous versions of QEMU in > later patches. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > hw/ppc/spapr.c | 21 +++++++++++++++++++-- > 1 file changed, 19 insertions(+), 2 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 6bfb908..10b7c35 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2439,8 +2439,6 @@ static void spapr_machine_2_5_class_init(ObjectClass *oc, void *data) > > mc->name = "pseries-2.5"; > mc->desc = "pSeries Logical Partition (PAPR compliant) v2.5"; > - mc->alias = "pseries"; > - mc->is_default = 1; > smc->dr_lmb_enabled = true; > } > > @@ -2450,6 +2448,24 @@ static const TypeInfo spapr_machine_2_5_info = { > .class_init = spapr_machine_2_5_class_init, > }; > > +static void spapr_machine_2_6_class_init(ObjectClass *oc, void *data) > +{ > + MachineClass *mc = MACHINE_CLASS(oc); > + sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc); > + > + mc->name = "pseries-2.6"; > + mc->desc = "pSeries Logical Partition (PAPR compliant) v2.6"; > + mc->alias = "pseries"; > + mc->is_default = 1; > + smc->dr_lmb_enabled = true; We should probably start to follow a scheme similar to x86 where the new machine initialization calls its predecessor (2.5 in this case) to ensure we don't forget feature flags and quirks. So you can either directly call spapr_machine_2_5_class_init() from spapr_machine_2_6_class_init() or extract the quirk part (dr_lmb_enabled) into a function that gets marked as "from 2.5 on" in its function name and call it from 2_5_class_init and from a "from 2.6 on" function that gets called from 2_6_class_init. Alex
On 27/11/15 10:55, Alexander Graf wrote: > > On 27.11.15 10:32, Thomas Huth wrote: >> Add a new pseries-2.6 machine class version to make sure we can >> keep the old types compatible to previous versions of QEMU in >> later patches. >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> hw/ppc/spapr.c | 21 +++++++++++++++++++-- >> 1 file changed, 19 insertions(+), 2 deletions(-) >> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >> index 6bfb908..10b7c35 100644 >> --- a/hw/ppc/spapr.c >> +++ b/hw/ppc/spapr.c >> @@ -2450,6 +2448,24 @@ static const TypeInfo spapr_machine_2_5_info = { >> .class_init = spapr_machine_2_5_class_init, >> }; >> >> +static void spapr_machine_2_6_class_init(ObjectClass *oc, void *data) >> +{ >> + MachineClass *mc = MACHINE_CLASS(oc); >> + sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc); >> + >> + mc->name = "pseries-2.6"; >> + mc->desc = "pSeries Logical Partition (PAPR compliant) v2.6"; >> + mc->alias = "pseries"; >> + mc->is_default = 1; >> + smc->dr_lmb_enabled = true; > > We should probably start to follow a scheme similar to x86 where the new > machine initialization calls its predecessor (2.5 in this case) to > ensure we don't forget feature flags and quirks. > > So you can either directly call spapr_machine_2_5_class_init() from > spapr_machine_2_6_class_init() or extract the quirk part > (dr_lmb_enabled) into a function that gets marked as "from 2.5 on" in > its function name and call it from 2_5_class_init and from a "from 2.6 > on" function that gets called from 2_6_class_init. I like the idea of calling the functions in a chain. However, the i386 people seem to do it the other way round, for example pc_i440fx_2_4_machine_options() calls pc_i440fx_2_5_machine_options(). That of course works, too, but it sounds a little bit cumbersome to me, since when introducing a new machine class version, you do not only have to introduce a function for the new class anymore, but also you have to update the previous version to change the behavior that has been introduced by the new function (see commit 87e896abe6d926 for example). Thus here's a question for the x86 people: Was this order done on purpose and if so, why? Or has it just grown historically that way and would it maybe make sense to change the order to the IMHO more intuitive way, so that the newer machine setup function calls the older one instead of the other way round? Next question is of course: What do we do in sPAPR land? Go the x86 way or do it the "big endian" way ;-) and do it the other way round? Thomas
On Fri, Nov 27, 2015 at 06:18:30PM +0100, Thomas Huth wrote: > On 27/11/15 10:55, Alexander Graf wrote: > > > > On 27.11.15 10:32, Thomas Huth wrote: > >> Add a new pseries-2.6 machine class version to make sure we can > >> keep the old types compatible to previous versions of QEMU in > >> later patches. > >> > >> Signed-off-by: Thomas Huth <thuth@redhat.com> > >> --- > >> hw/ppc/spapr.c | 21 +++++++++++++++++++-- > >> 1 file changed, 19 insertions(+), 2 deletions(-) > >> > >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > >> index 6bfb908..10b7c35 100644 > >> --- a/hw/ppc/spapr.c > >> +++ b/hw/ppc/spapr.c > >> @@ -2450,6 +2448,24 @@ static const TypeInfo spapr_machine_2_5_info = { > >> .class_init = spapr_machine_2_5_class_init, > >> }; > >> > >> +static void spapr_machine_2_6_class_init(ObjectClass *oc, void *data) > >> +{ > >> + MachineClass *mc = MACHINE_CLASS(oc); > >> + sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc); > >> + > >> + mc->name = "pseries-2.6"; > >> + mc->desc = "pSeries Logical Partition (PAPR compliant) v2.6"; > >> + mc->alias = "pseries"; > >> + mc->is_default = 1; > >> + smc->dr_lmb_enabled = true; > > > > We should probably start to follow a scheme similar to x86 where the new > > machine initialization calls its predecessor (2.5 in this case) to > > ensure we don't forget feature flags and quirks. > > > > So you can either directly call spapr_machine_2_5_class_init() from > > spapr_machine_2_6_class_init() or extract the quirk part > > (dr_lmb_enabled) into a function that gets marked as "from 2.5 on" in > > its function name and call it from 2_5_class_init and from a "from 2.6 > > on" function that gets called from 2_6_class_init. > > I like the idea of calling the functions in a chain. However, the i386 > people seem to do it the other way round, for example > pc_i440fx_2_4_machine_options() calls pc_i440fx_2_5_machine_options(). > That of course works, too, but it sounds a little bit cumbersome to me, > since when introducing a new machine class version, you do not only have > to introduce a function for the new class anymore, but also you have to > update the previous version to change the behavior that has been > introduced by the new function (see commit 87e896abe6d926 for example). The alias/is_default changes are only needed because we don't have a generic class alias system (yet), that would allow us to declare the "pc" alias and a default machine outside the machine_options() function. I agree it's cumbersome. commit 87e896abe6d926 has the extra broken_reserved_end change because for some reason we decided to add the broken_reserved_end quirk to pc-2.4 before we even introduced pc-2.5. That was an exception. The common case is to add the pc-2.4 quirks only after we added a pc-2.5 machine. The patch that adds pc-1.6, for example, looks like this: -static void pc_i440fx_2_5_machine_options(MachineClass *m) +static void pc_i440fx_2_6_machine_options(MachineClass *m) { pc_i440fx_machine_options(m); m->alias = "pc"; m->is_default = 1; } +DEFINE_I440FX_MACHINE(v2_6, "pc-i440fx-2.6", NULL, + pc_i440fx_2_6_machine_options); + +static void pc_i440fx_2_5_machine_options(MachineClass *m) +{ + pc_i440fx_2_6_machine_options(m); + m->alias = NULL; + m->is_default = 0; + SET_MACHINE_COMPAT(m, PC_COMPAT_2_5); +} Except for the alias/is_default stuff, it looks very simple to me. That said, I don't understand what you would suggest as alternative. Let's use pc-1.7 and pc-1.6 as examples: static void pc_compat_1_7(MachineState *machine) { pc_compat_2_0(machine); smbios_defaults = false; gigabyte_align = false; option_rom_has_mr = true; legacy_acpi_table_size = 6414; x86_cpu_change_kvm_default("x2apic", NULL); } static void pc_compat_1_6(MachineState *machine) { pc_compat_1_7(machine); rom_file_has_mr = false; has_acpi_build = false; } pc-1.7 and older need the smbios_defaults/gigabyte_align/ option_rom_has_mr/legacy_acpi_table_size/x2apic quirks. pc-2.0 and later don't need those quirks. How exactly would you make pc-1.6 and older inherit the quirks from pc-1.7, if not by reusing pc_compat_1_7() inside pc_compat_1_6()? (I am showing pc_compat_*() instead of *_machine_options(), because we're still moving compat stuff from pc_compat_*() to *_machine_options() functions. But the same questions apply once we move the compat code above to *_machine_options() functions). What's the alternative you propose? > > Thus here's a question for the x86 people: Was this order done on > purpose and if so, why? Or has it just grown historically that way and > would it maybe make sense to change the order to the IMHO more intuitive > way, so that the newer machine setup function calls the older one > instead of the other way round? Why would we ever want a new machine-type call the older one? Why would a newer machine-type inherit the quirks from the older machine-types? The newer machine-types need less quirks than the older ones, not more quirks. > > Next question is of course: What do we do in sPAPR land? Go the x86 way > or do it the "big endian" way ;-) and do it the other way round? If you decide to do it in a different way, I am curious to see how you plan to make it work.
On 27/11/15 18:56, Eduardo Habkost wrote: > On Fri, Nov 27, 2015 at 06:18:30PM +0100, Thomas Huth wrote: >> On 27/11/15 10:55, Alexander Graf wrote: >>> >>> On 27.11.15 10:32, Thomas Huth wrote: >>>> Add a new pseries-2.6 machine class version to make sure we can >>>> keep the old types compatible to previous versions of QEMU in >>>> later patches. >>>> >>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>> --- >>>> hw/ppc/spapr.c | 21 +++++++++++++++++++-- >>>> 1 file changed, 19 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >>>> index 6bfb908..10b7c35 100644 >>>> --- a/hw/ppc/spapr.c >>>> +++ b/hw/ppc/spapr.c >>>> @@ -2450,6 +2448,24 @@ static const TypeInfo spapr_machine_2_5_info = { >>>> .class_init = spapr_machine_2_5_class_init, >>>> }; >>>> >>>> +static void spapr_machine_2_6_class_init(ObjectClass *oc, void *data) >>>> +{ >>>> + MachineClass *mc = MACHINE_CLASS(oc); >>>> + sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc); >>>> + >>>> + mc->name = "pseries-2.6"; >>>> + mc->desc = "pSeries Logical Partition (PAPR compliant) v2.6"; >>>> + mc->alias = "pseries"; >>>> + mc->is_default = 1; >>>> + smc->dr_lmb_enabled = true; >>> >>> We should probably start to follow a scheme similar to x86 where the new >>> machine initialization calls its predecessor (2.5 in this case) to >>> ensure we don't forget feature flags and quirks. >>> >>> So you can either directly call spapr_machine_2_5_class_init() from >>> spapr_machine_2_6_class_init() or extract the quirk part >>> (dr_lmb_enabled) into a function that gets marked as "from 2.5 on" in >>> its function name and call it from 2_5_class_init and from a "from 2.6 >>> on" function that gets called from 2_6_class_init. >> >> I like the idea of calling the functions in a chain. However, the i386 >> people seem to do it the other way round, for example >> pc_i440fx_2_4_machine_options() calls pc_i440fx_2_5_machine_options(). >> That of course works, too, but it sounds a little bit cumbersome to me, >> since when introducing a new machine class version, you do not only have >> to introduce a function for the new class anymore, but also you have to >> update the previous version to change the behavior that has been >> introduced by the new function (see commit 87e896abe6d926 for example). > > The alias/is_default changes are only needed because we don't > have a generic class alias system (yet), that would allow us to > declare the "pc" alias and a default machine outside the > machine_options() function. I agree it's cumbersome. > > commit 87e896abe6d926 has the extra broken_reserved_end change > because for some reason we decided to add the broken_reserved_end > quirk to pc-2.4 before we even introduced pc-2.5. That was an > exception. The common case is to add the pc-2.4 quirks only after > we added a pc-2.5 machine. > > The patch that adds pc-1.6, for example, looks like this: > > -static void pc_i440fx_2_5_machine_options(MachineClass *m) > +static void pc_i440fx_2_6_machine_options(MachineClass *m) > { > pc_i440fx_machine_options(m); > m->alias = "pc"; > m->is_default = 1; > } > > +DEFINE_I440FX_MACHINE(v2_6, "pc-i440fx-2.6", NULL, > + pc_i440fx_2_6_machine_options); > + > +static void pc_i440fx_2_5_machine_options(MachineClass *m) > +{ > + pc_i440fx_2_6_machine_options(m); > + m->alias = NULL; > + m->is_default = 0; > + SET_MACHINE_COMPAT(m, PC_COMPAT_2_5); > +} > > Except for the alias/is_default stuff, it looks very simple to > me. > > That said, I don't understand what you would suggest as > alternative. Let's use pc-1.7 and pc-1.6 as examples: > > static void pc_compat_1_7(MachineState *machine) > { > pc_compat_2_0(machine); > smbios_defaults = false; > gigabyte_align = false; > option_rom_has_mr = true; > legacy_acpi_table_size = 6414; > x86_cpu_change_kvm_default("x2apic", NULL); > } > > static void pc_compat_1_6(MachineState *machine) > { > pc_compat_1_7(machine); > rom_file_has_mr = false; > has_acpi_build = false; > } > > pc-1.7 and older need the smbios_defaults/gigabyte_align/ > option_rom_has_mr/legacy_acpi_table_size/x2apic quirks. pc-2.0 > and later don't need those quirks. How exactly would you make > pc-1.6 and older inherit the quirks from pc-1.7, if not by > reusing pc_compat_1_7() inside pc_compat_1_6()? > > (I am showing pc_compat_*() instead of *_machine_options(), > because we're still moving compat stuff from pc_compat_*() to > *_machine_options() functions. But the same questions apply once > we move the compat code above to *_machine_options() functions). > > What's the alternative you propose? The quirk would have be set to false in the oldest machine instead, something like: static void pc_compat_1_7(MachineState *machine) { pc_compat_1_6(machine); rom_file_has_mr = true; has_acpi_build = true; ... } static void pc_compat_1_6(MachineState *machine) { pc_compat_1_5(machine); } ... static void pc_compat_0_13(MachineState *machine) { rom_file_has_mr = false; has_acpi_build = false; } And since "false" should also be the default for these variables, they also could be omitted there and it would be sufficient to set "rom_file_has_mr = true" and "has_acpi_build = true" once in the pc_compat_1_7() function. IMHO that should work fine, too, but maybe I just miss a point since I'm quite new to these compatibility management stuff... Thomas
On Fri, Nov 27, 2015 at 11:15:10PM +0100, Thomas Huth wrote: > On 27/11/15 18:56, Eduardo Habkost wrote: > > On Fri, Nov 27, 2015 at 06:18:30PM +0100, Thomas Huth wrote: > >> On 27/11/15 10:55, Alexander Graf wrote: > >>> > >>> On 27.11.15 10:32, Thomas Huth wrote: > >>>> Add a new pseries-2.6 machine class version to make sure we can > >>>> keep the old types compatible to previous versions of QEMU in > >>>> later patches. > >>>> > >>>> Signed-off-by: Thomas Huth <thuth@redhat.com> > >>>> --- > >>>> hw/ppc/spapr.c | 21 +++++++++++++++++++-- > >>>> 1 file changed, 19 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > >>>> index 6bfb908..10b7c35 100644 > >>>> --- a/hw/ppc/spapr.c > >>>> +++ b/hw/ppc/spapr.c > >>>> @@ -2450,6 +2448,24 @@ static const TypeInfo spapr_machine_2_5_info = { > >>>> .class_init = spapr_machine_2_5_class_init, > >>>> }; > >>>> > >>>> +static void spapr_machine_2_6_class_init(ObjectClass *oc, void *data) > >>>> +{ > >>>> + MachineClass *mc = MACHINE_CLASS(oc); > >>>> + sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc); > >>>> + > >>>> + mc->name = "pseries-2.6"; > >>>> + mc->desc = "pSeries Logical Partition (PAPR compliant) v2.6"; > >>>> + mc->alias = "pseries"; > >>>> + mc->is_default = 1; > >>>> + smc->dr_lmb_enabled = true; > >>> > >>> We should probably start to follow a scheme similar to x86 where the new > >>> machine initialization calls its predecessor (2.5 in this case) to > >>> ensure we don't forget feature flags and quirks. > >>> > >>> So you can either directly call spapr_machine_2_5_class_init() from > >>> spapr_machine_2_6_class_init() or extract the quirk part > >>> (dr_lmb_enabled) into a function that gets marked as "from 2.5 on" in > >>> its function name and call it from 2_5_class_init and from a "from 2.6 > >>> on" function that gets called from 2_6_class_init. > >> > >> I like the idea of calling the functions in a chain. However, the i386 > >> people seem to do it the other way round, for example > >> pc_i440fx_2_4_machine_options() calls pc_i440fx_2_5_machine_options(). > >> That of course works, too, but it sounds a little bit cumbersome to me, > >> since when introducing a new machine class version, you do not only have > >> to introduce a function for the new class anymore, but also you have to > >> update the previous version to change the behavior that has been > >> introduced by the new function (see commit 87e896abe6d926 for example). > > > > The alias/is_default changes are only needed because we don't > > have a generic class alias system (yet), that would allow us to > > declare the "pc" alias and a default machine outside the > > machine_options() function. I agree it's cumbersome. > > > > commit 87e896abe6d926 has the extra broken_reserved_end change > > because for some reason we decided to add the broken_reserved_end > > quirk to pc-2.4 before we even introduced pc-2.5. That was an > > exception. The common case is to add the pc-2.4 quirks only after > > we added a pc-2.5 machine. > > > > The patch that adds pc-1.6, for example, looks like this: > > > > -static void pc_i440fx_2_5_machine_options(MachineClass *m) > > +static void pc_i440fx_2_6_machine_options(MachineClass *m) > > { > > pc_i440fx_machine_options(m); > > m->alias = "pc"; > > m->is_default = 1; > > } > > > > +DEFINE_I440FX_MACHINE(v2_6, "pc-i440fx-2.6", NULL, > > + pc_i440fx_2_6_machine_options); > > + > > +static void pc_i440fx_2_5_machine_options(MachineClass *m) > > +{ > > + pc_i440fx_2_6_machine_options(m); > > + m->alias = NULL; > > + m->is_default = 0; > > + SET_MACHINE_COMPAT(m, PC_COMPAT_2_5); > > +} > > > > Except for the alias/is_default stuff, it looks very simple to > > me. > > > > That said, I don't understand what you would suggest as > > alternative. Let's use pc-1.7 and pc-1.6 as examples: > > > > static void pc_compat_1_7(MachineState *machine) > > { > > pc_compat_2_0(machine); > > smbios_defaults = false; > > gigabyte_align = false; > > option_rom_has_mr = true; > > legacy_acpi_table_size = 6414; > > x86_cpu_change_kvm_default("x2apic", NULL); > > } > > > > static void pc_compat_1_6(MachineState *machine) > > { > > pc_compat_1_7(machine); > > rom_file_has_mr = false; > > has_acpi_build = false; > > } > > > > pc-1.7 and older need the smbios_defaults/gigabyte_align/ > > option_rom_has_mr/legacy_acpi_table_size/x2apic quirks. pc-2.0 > > and later don't need those quirks. How exactly would you make > > pc-1.6 and older inherit the quirks from pc-1.7, if not by > > reusing pc_compat_1_7() inside pc_compat_1_6()? > > > > (I am showing pc_compat_*() instead of *_machine_options(), > > because we're still moving compat stuff from pc_compat_*() to > > *_machine_options() functions. But the same questions apply once > > we move the compat code above to *_machine_options() functions). > > > > What's the alternative you propose? > > The quirk would have be set to false in the oldest machine instead, > something like: > > static void pc_compat_1_7(MachineState *machine) > { > pc_compat_1_6(machine); > rom_file_has_mr = true; > has_acpi_build = true; > ... > } > > static void pc_compat_1_6(MachineState *machine) > { > pc_compat_1_5(machine); > } > > ... > > static void pc_compat_0_13(MachineState *machine) > { > rom_file_has_mr = false; > has_acpi_build = false; > } > > And since "false" should also be the default for these variables, they > also could be omitted there and it would be sufficient to set > "rom_file_has_mr = true" and "has_acpi_build = true" once in the > pc_compat_1_7() function. > IMHO that should work fine, too, but maybe I just miss a point since I'm > quite new to these compatibility management stuff... I think I see what you mean. It would work, but I see two issues: 1) The defaults in the QEMU hardware emulation code is the more recently introduced (and recommended) behavior, not the oldest legacy behavior. So the oldest machine-types would really need to set the variables to false. 2) I prefer to make the newer machine-types' code simpler and with less dependencies. The existing approch moves the complexity to the older machine-types, your suggestion moves the complexity to the newer ones. Any mistake done in the old (and probably unmaintained and unused) machine-types would affect all the newer ones. Also, this prevents us from easily deleting very old machine-types we don't want to support anymore.
On 28/11/15 16:09, Eduardo Habkost wrote: > On Fri, Nov 27, 2015 at 11:15:10PM +0100, Thomas Huth wrote: >> On 27/11/15 18:56, Eduardo Habkost wrote: >>> On Fri, Nov 27, 2015 at 06:18:30PM +0100, Thomas Huth wrote: >>>> On 27/11/15 10:55, Alexander Graf wrote: >>>>> >>>>> On 27.11.15 10:32, Thomas Huth wrote: >>>>>> Add a new pseries-2.6 machine class version to make sure we can >>>>>> keep the old types compatible to previous versions of QEMU in >>>>>> later patches. [...] >>>>> We should probably start to follow a scheme similar to x86 where the new >>>>> machine initialization calls its predecessor (2.5 in this case) to >>>>> ensure we don't forget feature flags and quirks. >>>>> >>>>> So you can either directly call spapr_machine_2_5_class_init() from >>>>> spapr_machine_2_6_class_init() or extract the quirk part >>>>> (dr_lmb_enabled) into a function that gets marked as "from 2.5 on" in >>>>> its function name and call it from 2_5_class_init and from a "from 2.6 >>>>> on" function that gets called from 2_6_class_init. >>>> >>>> I like the idea of calling the functions in a chain. However, the i386 >>>> people seem to do it the other way round, for example >>>> pc_i440fx_2_4_machine_options() calls pc_i440fx_2_5_machine_options(). >>>> That of course works, too, but it sounds a little bit cumbersome to me, >>>> since when introducing a new machine class version, you do not only have >>>> to introduce a function for the new class anymore, but also you have to >>>> update the previous version to change the behavior that has been >>>> introduced by the new function (see commit 87e896abe6d926 for example). >>> >>> The alias/is_default changes are only needed because we don't >>> have a generic class alias system (yet), that would allow us to >>> declare the "pc" alias and a default machine outside the >>> machine_options() function. I agree it's cumbersome. >>> >>> commit 87e896abe6d926 has the extra broken_reserved_end change >>> because for some reason we decided to add the broken_reserved_end >>> quirk to pc-2.4 before we even introduced pc-2.5. That was an >>> exception. The common case is to add the pc-2.4 quirks only after >>> we added a pc-2.5 machine. >>> >>> The patch that adds pc-1.6, for example, looks like this: >>> >>> -static void pc_i440fx_2_5_machine_options(MachineClass *m) >>> +static void pc_i440fx_2_6_machine_options(MachineClass *m) >>> { >>> pc_i440fx_machine_options(m); >>> m->alias = "pc"; >>> m->is_default = 1; >>> } >>> >>> +DEFINE_I440FX_MACHINE(v2_6, "pc-i440fx-2.6", NULL, >>> + pc_i440fx_2_6_machine_options); >>> + >>> +static void pc_i440fx_2_5_machine_options(MachineClass *m) >>> +{ >>> + pc_i440fx_2_6_machine_options(m); >>> + m->alias = NULL; >>> + m->is_default = 0; >>> + SET_MACHINE_COMPAT(m, PC_COMPAT_2_5); >>> +} >>> >>> Except for the alias/is_default stuff, it looks very simple to >>> me. >>> >>> That said, I don't understand what you would suggest as >>> alternative. Let's use pc-1.7 and pc-1.6 as examples: >>> >>> static void pc_compat_1_7(MachineState *machine) >>> { >>> pc_compat_2_0(machine); >>> smbios_defaults = false; >>> gigabyte_align = false; >>> option_rom_has_mr = true; >>> legacy_acpi_table_size = 6414; >>> x86_cpu_change_kvm_default("x2apic", NULL); >>> } >>> >>> static void pc_compat_1_6(MachineState *machine) >>> { >>> pc_compat_1_7(machine); >>> rom_file_has_mr = false; >>> has_acpi_build = false; >>> } >>> >>> pc-1.7 and older need the smbios_defaults/gigabyte_align/ >>> option_rom_has_mr/legacy_acpi_table_size/x2apic quirks. pc-2.0 >>> and later don't need those quirks. How exactly would you make >>> pc-1.6 and older inherit the quirks from pc-1.7, if not by >>> reusing pc_compat_1_7() inside pc_compat_1_6()? >>> >>> (I am showing pc_compat_*() instead of *_machine_options(), >>> because we're still moving compat stuff from pc_compat_*() to >>> *_machine_options() functions. But the same questions apply once >>> we move the compat code above to *_machine_options() functions). >>> >>> What's the alternative you propose? >> >> The quirk would have be set to false in the oldest machine instead, >> something like: >> >> static void pc_compat_1_7(MachineState *machine) >> { >> pc_compat_1_6(machine); >> rom_file_has_mr = true; >> has_acpi_build = true; >> ... >> } >> >> static void pc_compat_1_6(MachineState *machine) >> { >> pc_compat_1_5(machine); >> } >> >> ... >> >> static void pc_compat_0_13(MachineState *machine) >> { >> rom_file_has_mr = false; >> has_acpi_build = false; >> } >> >> And since "false" should also be the default for these variables, they >> also could be omitted there and it would be sufficient to set >> "rom_file_has_mr = true" and "has_acpi_build = true" once in the >> pc_compat_1_7() function. >> IMHO that should work fine, too, but maybe I just miss a point since I'm >> quite new to these compatibility management stuff... > > I think I see what you mean. It would work, but I see two issues: > > 1) The defaults in the QEMU hardware emulation code is the more > recently introduced (and recommended) behavior, not the oldest > legacy behavior. So the oldest machine-types would really need to > set the variables to false. > > 2) I prefer to make the newer machine-types' code simpler and > with less dependencies. The existing approch moves the complexity > to the older machine-types, your suggestion moves the complexity > to the newer ones. Any mistake done in the old (and probably > unmaintained and unused) machine-types would affect all the newer > ones. Also, this prevents us from easily deleting very old > machine-types we don't want to support anymore. Ok, thanks a lot for the explanation, that makes sense! So I think we'll do it the same way in the pseries machine, too ... and apparently David has even already posted a patch to do so :-) Thomas
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 6bfb908..10b7c35 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -2439,8 +2439,6 @@ static void spapr_machine_2_5_class_init(ObjectClass *oc, void *data) mc->name = "pseries-2.5"; mc->desc = "pSeries Logical Partition (PAPR compliant) v2.5"; - mc->alias = "pseries"; - mc->is_default = 1; smc->dr_lmb_enabled = true; } @@ -2450,6 +2448,24 @@ static const TypeInfo spapr_machine_2_5_info = { .class_init = spapr_machine_2_5_class_init, }; +static void spapr_machine_2_6_class_init(ObjectClass *oc, void *data) +{ + MachineClass *mc = MACHINE_CLASS(oc); + sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc); + + mc->name = "pseries-2.6"; + mc->desc = "pSeries Logical Partition (PAPR compliant) v2.6"; + mc->alias = "pseries"; + mc->is_default = 1; + smc->dr_lmb_enabled = true; +} + +static const TypeInfo spapr_machine_2_6_info = { + .name = MACHINE_TYPE_NAME("pseries-2.6"), + .parent = TYPE_SPAPR_MACHINE, + .class_init = spapr_machine_2_6_class_init, +}; + static void spapr_machine_register_types(void) { type_register_static(&spapr_machine_info); @@ -2458,6 +2474,7 @@ static void spapr_machine_register_types(void) type_register_static(&spapr_machine_2_3_info); type_register_static(&spapr_machine_2_4_info); type_register_static(&spapr_machine_2_5_info); + type_register_static(&spapr_machine_2_6_info); } type_init(spapr_machine_register_types)
Add a new pseries-2.6 machine class version to make sure we can keep the old types compatible to previous versions of QEMU in later patches. Signed-off-by: Thomas Huth <thuth@redhat.com> --- hw/ppc/spapr.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)