Re: [Qemu-devel] [PATCH 1/2] hw/ppc/spapr: Create pseries-2.6 machine

[Thomas Huth]

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