Re: [Qemu-arm] [PATCH v2 5/7] bcm2836_control: add bcm2836 ARM control logic

[Andrew Baumann]

> From: Peter Crosthwaite [mailto:address@hidden
> Sent: Wednesday, 30 December 2015 19:21
> On Wed, Dec 23, 2015 at 4:25 PM, Andrew Baumann
> <address@hidden> wrote:
> > This module is specific to the bcm2836 (Pi2). It implements the top
> > level interrupt controller, and mailboxes used for inter-processor
> > synchronisation.
[...]
> > --- /dev/null
> > +++ b/hw/intc/bcm2836_control.c
> > @@ -0,0 +1,338 @@
> > +/*
> > + * Rasperry Pi 2 emulation ARM control logic module.
> > + * Copyright (c) 2015, Microsoft
> > + * Written by Andrew Baumann
> > + *
> > + * At present, only implements interrupt routing, and mailboxes (i.e.,
> > + * not local timer, PMU interrupt, or AXI counters).
> > + *
> > + * Ref:
> > + *
> https://www.raspberrypi.org/documentation/hardware/raspberrypi/bcm28
> 36/QA7_rev3.4.pdf
> 
> This looks like an mpcore. Check hw/cpu/a9mpcore and friends to see
> how those are implemented. I think this is architecturally OK for the
> moment, although if you intend to expand this with Timers etc. it
> should be in hw/cpu/ and containerised. I think the containerisation
> can happen later but should be put it in hw/cpu/ straight up?

If only! I (and probably everyone that has ported an OS to it) wish it was an 
mpcore and not another custom Pi-specific thing :)

You might have an argument for moving the whole file from hw/intc to hw/cpu, 
but I don't think splitting these functions up (even if we implemented them all 
in the future, which I don't plan to do unless some guest OS turns out to use 
them) makes much sense. The spec is written as if it was one device, and the 
registers are all interleaved (e.g. the interrupt routing config registers are 
sandwiched in between the timer control/prescaler and status registers).

I really don't care which directory it lives in, however -- you can tell me 
what's best. As presently implemented (and as it is used by Linux and Windows), 
this thing is closest to an interrupt controller.

[...]
> > +    for (i = 0; i < BCM2836_NCORES; i++) {
> > +        /* handle local interrupts for this core */
> > +        if (s->localirqs[i]) {
> > +            assert(s->localirqs[i] < (1 << IRQ_MAILBOX0));
> 
> Curious, what is the source of this limitation?

That's just sanity-checking an invariant of my implementation. The local 
interrupts for each core are the four timers, the PMU interrupt (NYI), and the 
four mailboxes. However, the mailboxes use a separate set of registers (and are 
handled in logic below this), so it's an internal error if we ever see those 
bits set. This could be better commented, and the uint32_t for localirqs could 
be a uint8.

> > +            for (j = 0; j < IRQ_MAILBOX0; j++) {
> > +                if ((s->localirqs[i] & (1 << j)) != 0) {
> > +                    /* local interrupt j is set */
> > +                    if (FIQ_BIT(s->timercontrol[i], j)) {
> > +                        /* deliver a FIQ */
> > +                        s->fiqsrc[i] |= (uint32_t)1 << j;
> > +                    } else if (IRQ_BIT(s->timercontrol[i], j)) {
> > +                        /* deliver an IRQ */
> 
> Overcommented.

Maybe I'm more defensive because this is my code, but I'm respectfully going to 
disagree and ignore you on this one, at least until the next patch series :) 
I'm not a fan of deleting comments that are accurate, concise, and (IMHO) help 
the reader of the code immediately understand its logic. We can discuss it 
again in v3 if you feel strongly about it.

Andrew