Re: [Qemu-arm] [PATCH 1/8] bcm2835_sbm: add BCM2835 mailboxes

[Grégory ESTRADE]

Hi,

The "sbm" is a typo. It was meant to be "smb": Semaphore MailBoxes...

Sorry about that.

Regards,

Gregory 

On Tue, Dec 22, 2015 at 12:15 AM, Andrew Baumann <address@hidden> wrote:

Hi Peter,

Thanks for the review!

> From: Peter Crosthwaite [mailto:address@hidden]
> Sent: Monday, 21 December 2015 14:49
> On Thu, Dec 3, 2015 at 10:01 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.
> >
> > Signed-off-by: Andrew Baumann <address@hidden>
> > ---
> >  default-configs/arm-softmmu.mak      |   1 +
> >  hw/misc/Makefile.objs                |   1 +
> >  hw/misc/bcm2835_sbm.c                | 280 ++++++++++++++++++++
>
> >  include/hw/arm/bcm2835_arm_control.h | 481
> +++++++++++++++++++++++++++++++++++
>
> Do we need this file as of this patch?

Yes, for ARM_MC_IHAVEDATAIRQEN and other related defines.

[...]
> > +static void bcm2835_sbm_update(BCM2835SbmState *s)
>
> What does Sbm stand for? If it is acronym is should be all caps in camel case.

I'm not sure -- it comes from Gregory's code; maybe he can comment? Where this device gets instantiated, there is a comment of

/* Semaphores / Doorbells / Mailboxes */

... but only mailboxes are implemented here. I'm guessing maybe it's intended as "system mailboxes". I can rename it.

> > +{
> > +    int set;
>
> bool.
>
> > +    int done, n;
> > +    uint32_t value;
> > +
> > +    /* Avoid unwanted recursive calls */
> > +    s->mbox_irq_disabled = 1;
> > +
> > +    /* Get pending responses and put them in the vc->arm mbox */
> > +    done = 0;
> > +    while (!done) {
> > +        done = 1;
> > +        if (s->mbox[0].status & ARM_MS_FULL) {
> > +            /* vc->arm mbox full, exit */
>
> break here.
>
> > +        } else {
>
> so you can drop the else and get back a level of indent.
>
> > +            for (n = 0; n < MBOX_CHAN_COUNT; n++) {
> > +                if (s->available[n]) {
> > +                    value = ldl_phys(&s->mbox_as, n<<4);
> > +                    if (value != MBOX_INVALID_DATA) {
> > +                        mbox_push(&s->mbox[0], value);
> > +                    } else {
> > +                        /* Hmmm... */
>
> Needs more comment :)

Gregory -- help! :)

[...]
> > +    s->available[irq] = level;
> > +    if (!s->mbox_irq_disabled) {
>
> I don't think you need this. IO devices are not thread safe and
> core-code knows it. So you don't need to worry about interruption and
> re-entry of your IRQ handler.

I will have to put a debugger on this to confirm, but I think the issue is that we are using a private memory region and GPIOs to route mailbox data and interrupts to the relevant sub peripherals. The ldl_phys invokes another device emulation (such as bcm2835_property.c in this series), which may cause it to signal an interrupt back to us via qemu_set_irq which will recursively run our handler. We want that IRQ to be recorded but "squashed".

[...]
> > +
> > +    switch (offset) {
> > +    case 0x80:  /* MAIL0_READ */
> > +    case 0x84:
> > +    case 0x88:
> > +    case 0x8c:
>
> case 0x80..0x8c

Woah! Is that standard C?

[...]
> > --- /dev/null
> > +++ b/include/hw/arm/bcm2835_arm_control.h
> > @@ -0,0 +1,481 @@
> > +/*
> > + *  linux/arch/arm/mach-bcm2708/arm_control.h
[...]
> When you have a regular structure like this, you should collapse it
> down using some arithmatic:

Notice that this file comes from Linux. I know it's not pretty, but can we please keep it as-is, for comparison purposes? I'm not sure there's much value in cleaning it up locally...

> > --- /dev/null
> > +++ b/include/hw/misc/bcm2835_sbm.h
> > @@ -0,0 +1,37 @@
> > +/*
> > + * Raspberry Pi emulation (c) 2012 Gregory Estrade
> > + * This code is licensed under the GNU GPLv2 and later.
> > + */
> > +
> > +#ifndef BCM2835_SBM_H
> > +#define BCM2835_SBM_H
> > +
> > +#include "hw/sysbus.h"
> > +#include "exec/address-spaces.h"
> > +#include "hw/arm/bcm2835_mbox.h"
> > +
> > +#define TYPE_BCM2835_SBM "bcm2835_sbm"
> > +#define BCM2835_SBM(obj) \
> > +        OBJECT_CHECK(BCM2835SbmState, (obj), TYPE_BCM2835_SBM)
> > +
> > +typedef struct {
> > +    MemoryRegion *mbox_mr;
> > +    AddressSpace mbox_as;
>
> I can't see where these two fields are used. A quick scan shows that
> the SbmState's copy is uses for all DMA ops. If these are needed,
> mbox_init should pass a pointer to the the SbmState then this saves
> the pointer and navigates as needed back to the container structure to
> get the AS.

You're right. They're uninitialised/unused detritus from a previous refactor. I'll remove them.

Thanks,
Andrew