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

[Philippe Mathieu-Daudé]

On Thu, Feb 13, 2020 at 3:16 PM Philippe Mathieu-Daudé
<address@hidden> wrote:
> 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.

I found my 'ready' patch, it is already merged as commit 00cbd5bd74b1 =)

    hw/arm/bcm2835: Add various unimplemented peripherals

    Base addresses and sizes taken from the "BCM2835 ARM Peripherals"
    datasheet from February 06 2012:
    https://www.raspberrypi.org/app/uploads/2012/02/BCM2835-ARM-Peripherals.pdf

I'm successfully running U-boot and Linux on raspi0/1/2/3 so I guess
it is safe to remove ignore_memory_transaction_failures for the
raspi2.
It might be insufficient to run proprietary firmware (on the raspi2),
I have no idea if people use QEMU for that.

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