Re: [Qemu-arm] [PATCH v2 1/7] bcm2835_mbox: add BCM2835 mailboxes

[Andrew Baumann]

Hi Peter,

> From: Peter Crosthwaite [mailto:address@hidden
> Sent: Wednesday, 30 December 2015 16:13
> On Wed, Dec 23, 2015 at 4:25 PM, Andrew Baumann
> <address@hidden> wrote:
> > This adds the system mailboxes which are used to communicate with a
> > number of GPU peripherals on Pi/Pi2.
[...]
> > --- /dev/null
> > +++ b/hw/misc/bcm2835_mbox.c
> > @@ -0,0 +1,323 @@
> > +/*
> > + * Raspberry Pi emulation (c) 2012 Gregory Estrade
> 
> Should we change this to "Broadcom SoC emulation"?

Until we support more than one Broadcom SoC, probably not -- this is all based 
on the Pi documentation, and I have no idea how much commonality there is to 
other parts.

[...]
> > +static void mbox_push(BCM2835Mbox *mb, uint32_t val)
> > +{
> > +    assert(mb->count < MBOX_SIZE);
> 
> mbox_update can call mbox_push with val == a DMA value, which usually
> suggests that this may be a guest controllable (and a guest error
> rather than an assert). Is the mbox AS guest accessible? (I had a look
> at the later patches, and the MBox AS seems private but can values be
> set via the guest indirectly?).

These come from child devices (bcm2835_property in the present series). The 
other case where this arises is a push in mbox 1, but that is already checked 
in bcm2835_mbox_write, so I think an assert is appropriate.

[...]
> > +static void bcm2835_mbox_set_irq(void *opaque, int irq, int level)
> > +{
> > +    BCM2835MboxState *s = opaque;
> > +
> > +    s->available[irq] = level;
> > +
> > +    /* avoid recursively calling bcm2835_mbox_update when the interrupt
> > +     * status changes due to the ldl_phys call within that function */
> 
> Space before */

Huh? You mean newline? (There's already a space.)

Andrew