Hi, On Mon, Jan 22, 2018 at 1:12 PM, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 18 January 2018 at 21:39, bzt bzt <bztem...@gmail.com> wrote: > > Dear All, > > > > Since you still haven't merged Alistair's patch, I decided to include it > in > > my own. > > I've shrinked the number of modified files to two, that's the bare > minimum > > to support "-M raspi3" switch. Bcm2836.c modified minimally, the rest is > in > > raspi.c. I've renamed raspi2_init() to raspi_init() 'cos now it > initializes > > raspi3 as well, no additional function. > > > > Can you send this as a proper patch email, not as a reply to > an existing email thread, please? (This makes our automated tooling > much happier.) > Only if you're not demanding any further nonsense modifications. > > In particular we can't apply patches if they don't come with > the right Signed-off-by: lines from everybody who contributed > code to them. > > I've made some code review comments below. > > > diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c > > index 8c43291112..5119e00051 100644 > > --- a/hw/arm/bcm2836.c > > +++ b/hw/arm/bcm2836.c > > @@ -15,6 +15,7 @@ > > #include "hw/arm/bcm2836.h" > > #include "hw/arm/raspi_platform.h" > > #include "hw/sysbus.h" > > +#include "hw/boards.h" > > #include "exec/address-spaces.h" > > > > /* Peripheral base address seen by the CPU */ > > @@ -30,7 +31,7 @@ static void bcm2836_init(Object *obj) > > > > for (n = 0; n < BCM2836_NCPUS; n++) { > > object_initialize(&s->cpus[n], sizeof(s->cpus[n]), > > - "cortex-a15-" TYPE_ARM_CPU); > > + current_machine->cpu_type); > > This SoC code shouldn't be looking at the current_machine settings. > It should have a QOM property that specifies the cpu type, and the raspi.c code should set that property. (Call the property > 'cpu-type' -- if you grep in hw/arm for that string you'll find > some examples of existing devices/boards that do this.) > I'll give you some time to think about why it is nonsense that you asking for. Hint: you cannot add properties to an uninitialized object, you can't add the CPUs in the realize phase (will cause segfaults, I've tried) and SoC initialization will do call bcm2836_init(). [...] > There should be more spaces in this : "version == 3 ? ...". > If you run your patch through scripts/checkpatch.pl it should > help with this kind of style nit (though it doesn't always find > everything and sometimes it gets confused, so it's not always right). > Sure thing. > [...] > This will get confused if the user passes a -cpu argument. Instead > we should just have the machine type directly determine the version. > There are several ways to do this, but the simplest way is to have > raspi2_machine_init() set mc->init to raspi2_init() and > raspi3_machine_init() > set mc->niit to raspi3_init(), and thenthose function calls > raspi_init(machine, 2) or raspi_init(machine, 3). > Auch. Originally there were two raspi_init() functions (with two different version values), I've reduced the number to one because you asked for it. I've also tried to access the MachineClass object from raspi_init(), but it's impossible, MACHINE_CLASS() drops and assert, find_machine_class() segfaults. Nice model interface you have there. > > (The other approach would be to make the board set up a subclass > of MachineState and then have raspi2 and raspi3 be subclasses of > that, which gives you a place to define raspi-specific data fields > like "version". You can see that approach in hw/arm/vexpress.c. But > that seems like a lot of effort to go to given where we've started, > so I don't really recommend it.) > Again, that is EXACTLY what I originally had, and you (the maintainers) said there shouldn't be a new BCM2837 class, so I've removed it. Look, if you can't understand how my patch works, just say so, no shame in that. > [...] > > I don't want to add new machine types which set the > ignore_memory_transaction_failures flag to true -- this is > only for existing board models where we don't know what > guest code that used to run would break when the flag was > flipped. For new boards we know that no code runs on them > so can leave the flag at its correct default. > Just copy'n'pasted raspi2, only modified the CPU model in which they differ. > > This may mean that you need to add some extra dummy > devices to the the SoC model using > create_unimplemented_device() so that your guest image > doesn't crash trying to probe those devices. > This may mean your users will be very upset because mainline qemu won't emulate raspi3 in the foreseeable future, because it's sure like hell I won't fix something that's not broken. > > I appreciate that this is a bit annoying for a case like this > where it's adding a new variant of an existing SoC, but > this is one of the situations where the only practical > opportunity to get the implementation right without breaking > existing QEMU users is at the point where we add the new > board model. > It is not annoying at all, it's simply silly. Are you really thought I would do *your* job? "Only two things are infinite, the universe and the human stupidity, and I'm not sure about the former!". Actually I'm very entertained :-D :-D :-D But seriously, I've showed you a way how to add raspi3, the rest is up to you. I won't do anything more about it, I'm not a paid qemu maintainer whatsoever! Again, my patch is working for me, it does not broke any existing raspi2 machines, and I'm very happy about using it for more than half a year now! These are the facts, everything else is just talking. Cheers bzt > thanks > -- PMM >