Re: [PATCH v3 12/13] hw/arm/raspi: Use a unique raspi_machine_class_init() method

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 2/13/20 2:59 PM, Peter Maydell wrote:
> On Sat, 8 Feb 2020 at 16:57, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> With the exception of the ignore_memory_transaction_failures
>> flag set for the raspi2, both machine_class_init() methods
>> are now identical. Merge them to keep a unique method.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   hw/arm/raspi.c | 31 ++++++-------------------------
>>   1 file changed, 6 insertions(+), 25 deletions(-)
>>
>> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
>> index 0537fc0a2d..bee6ca0a08 100644
>> --- a/hw/arm/raspi.c
>> +++ b/hw/arm/raspi.c
>> @@ -294,7 +294,7 @@ static void raspi_machine_init(MachineState *machine)
>>       setup_boot(machine, version, machine->ram_size - vcram_size);
>>   }
>>
>> -static void raspi2_machine_class_init(ObjectClass *oc, void *data)
>> +static void raspi_machine_class_init(ObjectClass *oc, void *data)
>>   {
>>       MachineClass *mc = MACHINE_CLASS(oc);
>>       RaspiMachineClass *rmc = RASPI_MACHINE_CLASS(oc);
>> @@ -311,41 +311,22 @@ static void raspi2_machine_class_init(ObjectClass *oc, void *data)
>>       mc->min_cpus = BCM283X_NCPUS;
>>       mc->default_cpus = BCM283X_NCPUS;
>>       mc->default_ram_size = board_ram_size(board_rev);
>> -    mc->ignore_memory_transaction_failures = true;
>> +    if (board_version(board_rev) == 2) {
>> +        mc->ignore_memory_transaction_failures = true;
>> +    }
>>   };
> 
> This isn't really the correct condition here. What we want is:
>   * for the board named 'raspi2' which was introduced before
>     we added the transaction-failure support to Arm CPU emulation,
>     disable signaling transaction failures
>   * for any other board, leave it enabled (whether that new
>     board is BCM2836 based or anything else)
> 
> (This kind of follows on from my remark on patch 3: we should
> be suspicious of anything that's conditional on board_version();
> it should probably be testing something else.)
> 
> The natural way to implement this is to have the .class_data
> be a pointer to a struct which is in an array and defines
> relevant per-class stuff, the same way we do in
> bcm2836_register_types(). That way the struct can indicate
> both the board revision number and also "is this a legacy
> board that needs transaction-failures disabled?".

IIUC Igor insists explaining that he doesn't accept anymore a 
".class_data pointer to a struct which is in an array and defines 
relevant per-class stuff" and we should not use this pattern anymore.

> The other approach here, as discussed on IRC, is that if
> we're confident we really have all the devices in the SoC
> either present or stubbed out with unimplemented-device
> then we could disable ignore_memory_transaction_failures
> for raspi2. (The flag is only there because I didn't want
> to try to do the auditing and fielding of potential bug
> reports if I changed the behaviour of a bunch of our
> existing not-very-maintained board models: the real
> correct behaviour in almost all cases would be to allow
> transaction failures and just make sure we have stub devices
> as needed.)

Yes, the plan is to add all the unimplemented peripherals (patches 
ready, but out of scope of this series) and remove this flag.

> That said, this does give the right answer for our current boards,
> so I'm ok with taking this series if you want to address this
> in a followup patch.

If you are OK, I prefer to address this in a later series than delaying 
this one more longer.

Thanks!