Re: [Qemu-devel] [PATCH v6 1/4] hw: introduce standard SD host controller

[Igor Mitsyanko <i.mitsyanko@samsung.com>]

On 08/06/2012 03:15 PM, Peter Maydell wrote:
> On 6 August 2012 04:25, Peter A. G. Crosthwaite
> <peter.crosthwaite@petalogix.com> wrote:
>> From: Igor Mitsyanko <i.mitsyanko@samsung.com>
>>
>> Device model for standard SD Host Controller Interface (SDHCI) compliant with
>> version 2.00 of SD association specification.
>> +typedef struct ADMADescr {
>> +    target_phys_addr_t addr;
>> +    uint16_t length;
>> +    uint8_t attr;
>> +    uint8_t incr;
>> +} ADMADescr;
>> +
>> +static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
>> +{
>> +    uint32_t adma1 = 0;
>> +    uint64_t adma2 = 0;
>> +    target_phys_addr_t entry_addr = (target_phys_addr_t)s->admasysaddr;
>> +
>> +    switch (SDHC_DMA_TYPE(s->hostctl)) {
>> +    case SDHC_CTRL_ADMA2_32:
>> +        cpu_physical_memory_read(entry_addr, (uint8_t *)&adma2, sizeof(adma2));
>> +        dscr->addr = (target_phys_addr_t)((adma2 >> 32) & 0xfffffffc);
>> +        dscr->length = (uint16_t)((adma2 >> 16) & 0xFFFF);
>> +        dscr->attr = (uint8_t)(adma2 & 0x3F);
>> +        dscr->incr = 8;
>> +        break;
>> +    case SDHC_CTRL_ADMA1_32:
>> +        cpu_physical_memory_read(entry_addr, (uint8_t *)&adma1, sizeof(adma1));
>> +        dscr->addr = (target_phys_addr_t)(adma1 & 0xFFFFF000);
>> +        dscr->attr = (uint8_t)(adma1 & 0x3F);
>> +        dscr->incr = 4;
>> +        if ((dscr->attr & SDHC_ADMA_ATTR_ACT_MASK) == SDHC_ADMA_ATTR_SET_LEN) {
>> +            dscr->length = (uint16_t)((adma1 >> 12) & 0xFFFF);
>> +        } else {
>> +            dscr->length = 4096;
>> +        }
>> +        break;
>> +    case SDHC_CTRL_ADMA2_64:
>> +        cpu_physical_memory_read(entry_addr, (uint8_t *)(&dscr->attr), 1);
>> +        cpu_physical_memory_read(entry_addr + 2, (uint8_t *)(&dscr->length), 2);
>> +        cpu_physical_memory_read(entry_addr + 4, (uint8_t *)(&dscr->addr), 8);
>> +        dscr->attr &= 0xfffffff8;
>> +        dscr->incr = 12;
>> +        break;
>> +    }
>> +}
>> +
>> +/* Advanced DMA data transfer */
>> +static void sdhci_start_adma(SDHCIState *s)
>> +{
>> +    unsigned int n, begin, length;
>> +    const uint16_t block_size = s->blksize & 0x0fff;
>> +    ADMADescr dscr;
>> +    s->admaerr &= ~SDHC_ADMAERR_LENGTH_MISMATCH;
>> +
>> +    while (1) {
>> +        get_adma_description(s, &dscr);
>> +        DPRINT_L2("ADMA loop: addr=" TARGET_FMT_plx ", len=%d, attr=%x\n",
>> +                dscr.addr, dscr.length, dscr.attr);
>> +
>> +        if ((dscr.attr & SDHC_ADMA_ATTR_VALID) == 0) {
>> +            /* Indicate that error occurred in ST_FDS state */
>> +            s->admaerr &= ~SDHC_ADMAERR_STATE_MASK;
>> +            s->admaerr |= SDHC_ADMAERR_STATE_ST_FDS;
>> +
>> +            /* Generate ADMA error interrupt */
>> +            if (s->errintstsen & SDHC_EISEN_ADMAERR) {
>> +                s->errintsts |= SDHC_EIS_ADMAERR;
>> +                s->norintsts |= SDHC_NIS_ERR;
>> +            }
>> +
>> +            sdhci_update_irq(s);
>> +            break;
>> +        }
>> +
>> +        length = dscr.length ? dscr.length : 65536;
>> +
>> +        switch (dscr.attr & SDHC_ADMA_ATTR_ACT_MASK) {
>> +        case SDHC_ADMA_ATTR_ACT_TRAN:  /* data transfer */
>> +
>> +            if (s->trnmod & SDHC_TRNS_READ) {
>> +                while (length) {
>> +                    if (s->data_count == 0) {
>> +                        for (n = 0; n < block_size; n++) {
>> +                            s->fifo_buffer[n] = sd_read_data(s->card);
>> +                        }
>> +                    }
>> +                    begin = s->data_count;
>> +                    if ((length + begin) < block_size) {
>> +                        s->data_count = length + begin;
>> +                        length = 0;
>> +                     } else {
>> +                        s->data_count = block_size;
>> +                        length -= block_size - begin;
>> +                    }
>> +                    cpu_physical_memory_write(dscr.addr, &s->fifo_buffer[begin],
>> +                            s->data_count - begin);
>> +                    dscr.addr += s->data_count - begin;
>> +                    if (s->data_count == block_size) {
>> +                        s->data_count = 0;
>> +                        if (s->trnmod & SDHC_TRNS_BLK_CNT_EN) {
>> +                            s->blkcnt--;
>> +                            if (s->blkcnt == 0) {
>> +                                break;
>> +                            }
>> +                        }
>> +                    }
>> +                }
>> +            } else {
>> +                while (length) {
>> +                    begin = s->data_count;
>> +                    if ((length + begin) < block_size) {
>> +                        s->data_count = length + begin;
>> +                        length = 0;
>> +                     } else {
>> +                        s->data_count = block_size;
>> +                        length -= block_size - begin;
>> +                    }
>> +                    cpu_physical_memory_read(dscr.addr,
>> +                            &s->fifo_buffer[begin], s->data_count);
>> +                    dscr.addr += s->data_count - begin;
>> +                    if (s->data_count == block_size) {
>> +                        for (n = 0; n < block_size; n++) {
>> +                            sd_write_data(s->card, s->fifo_buffer[n]);
>> +                        }
>> +                        s->data_count = 0;
>> +                        if (s->trnmod & SDHC_TRNS_BLK_CNT_EN) {
>> +                            s->blkcnt--;
>> +                            if (s->blkcnt == 0) {
>> +                                break;
>> +                            }
>> +                        }
>> +                    }
>> +                }
>> +            }
>> +            s->admasysaddr += dscr.incr;
>> +            break;
>> +        case SDHC_ADMA_ATTR_ACT_LINK:   /* link to next descriptor table */
>> +            s->admasysaddr = dscr.addr;
>> +            DPRINT_L1("ADMA link: admasysaddr=0x%lx\n", s->admasysaddr);
>> +            break;
>> +        default:
>> +            s->admasysaddr += dscr.incr;
>> +            break;
>> +        }
>> +
>> +        /* ADMA transfer terminates if blkcnt == 0 or by END attribute */
>> +        if (((s->trnmod & SDHC_TRNS_BLK_CNT_EN) &&
>> +                    (s->blkcnt == 0)) || (dscr.attr & SDHC_ADMA_ATTR_END)) {
>> +            DPRINT_L2("ADMA transfer completed\n");
>> +            if (length || ((dscr.attr & SDHC_ADMA_ATTR_END) &&
>> +                (s->trnmod & SDHC_TRNS_BLK_CNT_EN) &&
>> +                s->blkcnt != 0) || ((s->trnmod & SDHC_TRNS_BLK_CNT_EN) &&
>> +                s->blkcnt == 0 && (dscr.attr & SDHC_ADMA_ATTR_END) == 0)) {
>> +                ERRPRINT("SD/MMC host ADMA length mismatch\n");
>> +                s->admaerr |= SDHC_ADMAERR_LENGTH_MISMATCH |
>> +                        SDHC_ADMAERR_STATE_ST_TFR;
>> +                if (s->errintstsen & SDHC_EISEN_ADMAERR) {
>> +                    ERRPRINT("Set ADMA error flag\n");
>> +                    s->errintsts |= SDHC_EIS_ADMAERR;
>> +                    s->norintsts |= SDHC_NIS_ERR;
>> +                }
>> +
>> +                sdhci_update_irq(s);
>> +            }
>> +            SDHCI_GET_CLASS(s)->end_data_transfer(s);
>> +            break;
>> +        }
>> +
>> +        if (dscr.attr & SDHC_ADMA_ATTR_INT) {
>> +            DPRINT_L1("ADMA interrupt: admasysaddr=0x%lx\n", s->admasysaddr);
>> +            if (s->norintstsen & SDHC_NISEN_DMA) {
>> +                s->norintsts |= SDHC_NIS_DMA;
>> +            }
>> +
>> +            sdhci_update_irq(s);
>> +            break;
>> +        }
>> +    }
>> +}
> So I think the guest can make this loop never terminate if it sets up
> a loop of ACT_LINK descriptors, right? I don't know how we should
> handle this but I'm pretty sure "make qemu sit there forever not responding
> to anything and not resettable" isn't it.
>
> -- PMM
>

That could only happen if guest would do this on purpose, but I see your 
point. There's no way for us to tell if SD card read/write succeeded or 
not, so I think the only way to to this is to suspend transfer after 
some reasonable amount of loops and resume it by qemu_timer, giving CPU 
time to do something useful.
I've never seen long ADMA loops, so it wouldn't reflect on performance 
in any way.