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

[Peter Maydell]

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 = 0;
> +                } else {
> +                    data = ldl_phys(&s->dma_as, 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