| Message ID | 20200208165645.15657-13-f4bug@amsat.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/arm/raspi: Dynamically create machines based on the board revision | expand |
On Sat, 8 Feb 2020 17:56:44 +0100 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> Reviewed-by: Igor Mammedov <imammedo@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; > + } > }; > > -#ifdef TARGET_AARCH64 > -static void raspi3_machine_class_init(ObjectClass *oc, void *data) > -{ > - MachineClass *mc = MACHINE_CLASS(oc); > - RaspiMachineClass *rmc = RASPI_MACHINE_CLASS(oc); > - uint32_t board_rev = (uint32_t)(uintptr_t)data; > - > - rmc->board_rev = board_rev; > - mc->desc = g_strdup_printf("Raspberry Pi %s", board_type(board_rev)); > - mc->init = raspi_machine_init; > - mc->block_default_type = IF_SD; > - mc->no_parallel = 1; > - mc->no_floppy = 1; > - mc->no_cdrom = 1; > - mc->max_cpus = BCM283X_NCPUS; > - mc->min_cpus = BCM283X_NCPUS; > - mc->default_cpus = BCM283X_NCPUS; > - mc->default_ram_size = board_ram_size(board_rev); > -} > -#endif > - > static const TypeInfo raspi_machine_types[] = { > { > .name = MACHINE_TYPE_NAME("raspi2"), > .parent = TYPE_RASPI_MACHINE, > - .class_init = raspi2_machine_class_init, > + .class_init = raspi_machine_class_init, > .class_data = (void *)0xa21041, > #ifdef TARGET_AARCH64 > }, { > .name = MACHINE_TYPE_NAME("raspi3"), > .parent = TYPE_RASPI_MACHINE, > - .class_init = raspi3_machine_class_init, > + .class_init = raspi_machine_class_init, > .class_data = (void *)0xa02082, > #endif > }, {
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?". 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.) 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. thanks -- PMM
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!
On Thu, 13 Feb 2020 at 14:16, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > On 2/13/20 2:59 PM, Peter Maydell wrote: > > 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. Huh? How else would you do this? I'm kinda dubious about the pattern this patch series uses of just stuffing a 32-bit board ID number into the class_data field, to be honest -- I let that pass partly not to hold up the series but partly because I expect that we'll need to turn it back into a proper pointer to a data struct soonish. thnaks -- PMM
On 2/13/20 3:32 PM, Peter Maydell wrote: > On Thu, 13 Feb 2020 at 14:16, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> On 2/13/20 2:59 PM, Peter Maydell wrote: >>> 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. > > Huh? How else would you do this? I'm kinda dubious about the > pattern this patch series uses of just stuffing a 32-bit board > ID number into the class_data field, to be honest -- I let that > pass partly not to hold up the series but partly because I > expect that we'll need to turn it back into a proper pointer > to a data struct soonish. https://www.mail-archive.com/qemu-devel@nongnu.org/msg678305.html Igor> we sometimes use .class_data when creating many derived types (typical example CPU types (x86)) where it's impractical to code leaf class_init functions. I'd use .class_data in cases where I can't get away with explicit .class_init Which I understand as: - avoid .class_data (pointers to structures) - explicitly set ObjectClass::fields in .class_init() by open-coding all.
On Thu, Feb 13, 2020 at 3:16 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > 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. 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! > >
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; + } }; -#ifdef TARGET_AARCH64 -static void raspi3_machine_class_init(ObjectClass *oc, void *data) -{ - MachineClass *mc = MACHINE_CLASS(oc); - RaspiMachineClass *rmc = RASPI_MACHINE_CLASS(oc); - uint32_t board_rev = (uint32_t)(uintptr_t)data; - - rmc->board_rev = board_rev; - mc->desc = g_strdup_printf("Raspberry Pi %s", board_type(board_rev)); - mc->init = raspi_machine_init; - mc->block_default_type = IF_SD; - mc->no_parallel = 1; - mc->no_floppy = 1; - mc->no_cdrom = 1; - mc->max_cpus = BCM283X_NCPUS; - mc->min_cpus = BCM283X_NCPUS; - mc->default_cpus = BCM283X_NCPUS; - mc->default_ram_size = board_ram_size(board_rev); -} -#endif - static const TypeInfo raspi_machine_types[] = { { .name = MACHINE_TYPE_NAME("raspi2"), .parent = TYPE_RASPI_MACHINE, - .class_init = raspi2_machine_class_init, + .class_init = raspi_machine_class_init, .class_data = (void *)0xa21041, #ifdef TARGET_AARCH64 }, { .name = MACHINE_TYPE_NAME("raspi3"), .parent = TYPE_RASPI_MACHINE, - .class_init = raspi3_machine_class_init, + .class_init = raspi_machine_class_init, .class_data = (void *)0xa02082, #endif }, {
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(-)