Re: [Qemu-arm] [PATCH RFC] bcm2835_dma: add emulation of Raspberry Pi DMA controller

[Grégory ESTRADE]

I'd be glad if you add a sign-off by me, but it doesn't matter much. Having this work finally getting into mainstream thanks to Andrew is what matters there.

Best regards,

Gregory

On Thu, Mar 3, 2016 at 3:24 PM, Peter Maydell <address@hidden> wrote:

On 29 February 2016 at 23:11, Andrew Baumann
<address@hidden> wrote:
> Signed-off-by: Andrew Baumann <address@hidden>
> ---
> This patch applies on top of the previous series for Windows and
> framebuffer support:
>   https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg06387.html
>
> After preparing that, I was disappointed to discover that Raspbian
> won't boot cleanly without the DMA controller. In the hope of beating
> the freeze deadline (it's still February 29 here :-) I'm sending this
> for review.
>
> After applying this patch, it is possible to boot Raspbian to the GUI
> using a command such as:
>
>   qemu-system-arm -M raspi2 -kernel raspbian-boot/kernel7.img -sd
>   2015-09-24-raspbian-jessie.img -append "rw earlyprintk loglevel=8
>   console=ttyAMA0 dwc_otg.lpm_enable=0 root=/dev/mmcblk0p2 rootwait"
>   -dtb raspbian-boot/bcm2709-rpi-2-b.dtb -serial stdio
>
> As before, this derives from the original (out of tree) work of
> Gregory Estrade, Stefan Weil and others to support Raspberry Pi
> 1. This patch in particulary is Gregory's code, which I have cleaned
> up for submission.

Should it have a Signed-off-by: line and/or authorship line
from Gregory, then?

(This is largely about giving credit where due, so it's Gregory's
choice really. I forget whether we've had this discussion before
but I couldn't find anything in a quick sweep through earlier mail
threads.)

> +static void bcm2835_dma_update(BCM2835DMAState *s, unsigned c)
> +{
> +    BCM2835DMAChan *ch = &s->chan[c];
> +    uint32_t data, xlen, ylen;
> +    int16_t dst_stride, src_stride;
> +
> +    if (!(s->enable & (1 << c))) {
> +        return;
> +    }
> +
> +    while ((s->enable & (1 << c)) && (ch->conblk_ad != 0)) {
> +        /* CB fetch */
> +        ch->ti = ldl_phys(&s->dma_as, ch->conblk_ad);
> +        ch->source_ad = ldl_phys(&s->dma_as, ch->conblk_ad + 4);
> +        ch->dest_ad = ldl_phys(&s->dma_as, ch->conblk_ad + 8);
> +        ch->txfr_len = ldl_phys(&s->dma_as, ch->conblk_ad + 12);
> +        ch->stride = ldl_phys(&s->dma_as, ch->conblk_ad + 16);
> +        ch->nextconbk = ldl_phys(&s->dma_as, ch->conblk_ad + 20);

These should use the ldl_{le,be}_phys functions. (If you care
about modelling the response to "unreadable address" you can
use address_space_ldl_{le,be}.)

> +
> +        if (ch->ti & BCM2708_DMA_TDMODE) {
> +            /* 2D transfer mode */
> +            ylen = (ch->txfr_len >> 16) & 0x3fff;
> +            xlen = ch->txfr_len & 0xffff;
> +            dst_stride = ch->stride >> 16;
> +            src_stride = ch->stride & 0xffff;
> +        } else {
> +            ylen = 1;
> +            xlen = ch->txfr_len;
> +            dst_stride = 0;
> +            src_stride = 0;
> +        }
> +
> +        while (ylen != 0) {
> +            /* Normal transfer mode */
> +            while (xlen != 0) {
> +                if (ch->ti & BCM2708_DMA_S_IGNORE) {
> +                    /* Ignore reads */
> +                    data = ""> > +                } else {
> +                    data = "" ch->source_ad);
> +                }
> +                if (ch->ti & BCM2708_DMA_S_INC) {
> +                    ch->source_ad += 4;
> +                }
> +
> +                if (ch->ti & BCM2708_DMA_D_IGNORE) {
> +                    /* Ignore writes */
> +                } else {
> +                    stl_phys(&s->dma_as, ch->dest_ad, data);
> +                }
> +                if (ch->ti & BCM2708_DMA_D_INC) {
> +                    ch->dest_ad += 4;
> +                }
> +
> +                /* update remaining transfer length */
> +                xlen -= 4;
> +                if (ch->ti & BCM2708_DMA_TDMODE) {
> +                    ch->txfr_len = (ylen << 16) | xlen;
> +                } else {
> +                    ch->txfr_len = xlen;
> +                }
> +            }
> +
> +            if (--ylen != 0) {
> +                ch->source_ad += src_stride;
> +                ch->dest_ad += dst_stride;
> +            }
> +        }
> +        ch->cs |= BCM2708_DMA_END;
> +        if (ch->ti & BCM2708_DMA_INT_EN) {
> +            ch->cs |= BCM2708_DMA_INT;
> +            s->int_status |= (1 << c);
> +            qemu_set_irq(ch->irq, 1);
> +        }
> +
> +        /* Process next CB */
> +        ch->conblk_ad = ch->nextconbk;
> +    }

This loop allows a guest to make QEMU lock up (stop responding to monitor
commands, etc) if it feeds the device a circular loop of CBs. On the other
hand I don't think we have a good approach to avoiding this problem,
so never mind.

> +    ch->cs &= ~BCM2708_DMA_ACTIVE;
> +}
> +
> +static uint64_t bcm2835_dma_read(BCM2835DMAState *s, hwaddr offset,
> +                                 unsigned size, unsigned c)
> +{
> +    BCM2835DMAChan *ch = &s->chan[c];
> +    uint32_t res = 0;
> +
> +    assert(size == 4);
> +    assert(c < BCM2835_DMA_NCHANS);

It's a bit late to assert after you've already used c as
an array index... (the compiler is free to conclude that the
condition must always be true, for instance.)

> +
> +    switch (offset) {
> +    case BCM2708_DMA_CS:
> +        res = ch->cs;
> +        break;
> +    case BCM2708_DMA_ADDR:
> +        res = ch->conblk_ad;
> +        break;
> +    case BCM2708_DMA_INFO:
> +        res = ch->ti;
> +        break;
> +    case BCM2708_DMA_SOURCE_AD:
> +        res = ch->source_ad;
> +        break;
> +    case BCM2708_DMA_DEST_AD:
> +        res = ch->dest_ad;
> +        break;
> +    case BCM2708_DMA_TXFR_LEN:
> +        res = ch->txfr_len;
> +        break;
> +    case BCM2708_DMA_STRIDE:
> +        res = ch->stride;
> +        break;
> +    case BCM2708_DMA_NEXTCB:
> +        res = ch->nextconbk;
> +        break;
> +    case BCM2708_DMA_DEBUG:
> +        res = ch->debug;
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset %"HWADDR_PRIx"\n",
> +                      __func__, offset);
> +        break;
> +    }
> +    return res;
> +}
> +
> +static void bcm2835_dma_write(BCM2835DMAState *s, hwaddr offset,
> +                              uint64_t value, unsigned size, unsigned c)
> +{
> +    BCM2835DMAChan *ch = &s->chan[c];
> +    uint32_t oldcs;
> +
> +    assert(size == 4);
> +    assert(c < BCM2835_DMA_NCHANS);
> +
> +    switch (offset) {
> +    case BCM2708_DMA_CS:
> +        oldcs = ch->cs;
> +        if (value & BCM2708_DMA_RESET) {
> +            ch->cs |= BCM2708_DMA_RESET;

The comment about this bit earlier says it's self-clearing, but I
don't see where it gets cleared.

Otherwise looks OK.

thanks
-- PMM