Re: [Qemu-devel] [PATCH] BCM2837 and machine raspi3

[bzt bzt]

Hi,

On Mon, Jan 22, 2018 at 1:12 PM, Peter Maydell <address@hidden>
wrote:

> On 18 January 2018 at 21:39, bzt bzt <address@hidden> 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
>