[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Philippe Mathieu-Daudé
Subject: Re: [PATCH v3 12/13] hw/arm/raspi: Use a unique raspi_machine_class_init() method
Date: Thu, 13 Feb 2020 15:15:54 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1

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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]