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

[Philippe Mathieu-Daudé]

On 2/13/20 2:59 PM, Peter Maydell wrote:
> On Sat, 8 Feb 2020 at 16:57, Philippe Mathieu-Daudé <address@hidden> 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é <address@hidden>
> > ---
> >   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!