From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:50470) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SyLkq-0001MS-UJ for qemu-devel@nongnu.org; Mon, 06 Aug 2012 07:45:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SyLkp-0005vn-9N for qemu-devel@nongnu.org; Mon, 06 Aug 2012 07:45:48 -0400 Received: from mailout3.w1.samsung.com ([210.118.77.13]:60517) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SyLko-0005vV-Si for qemu-devel@nongnu.org; Mon, 06 Aug 2012 07:45:47 -0400 Received: from eusync2.samsung.com (mailout3.w1.samsung.com [210.118.77.13]) by mailout3.w1.samsung.com (Oracle Communications Messaging Server 7u4-24.01(7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTP id <0M8C00JW50P4LA40@mailout3.w1.samsung.com> for qemu-devel@nongnu.org; Mon, 06 Aug 2012 12:46:16 +0100 (BST) Received: from [106.109.9.232] by eusync2.samsung.com (Oracle Communications Messaging Server 7u4-23.01(7.0.4.23.0) 64bit (built Aug 10 2011)) with ESMTPA id <0M8C00BIN0O47A30@eusync2.samsung.com> for qemu-devel@nongnu.org; Mon, 06 Aug 2012 12:45:42 +0100 (BST) Message-id: <501FAE65.1060504@samsung.com> Date: Mon, 06 Aug 2012 15:45:41 +0400 From: Igor Mitsyanko MIME-version: 1.0 References: In-reply-to: Content-type: text/plain; charset=UTF-8; format=flowed Content-transfer-encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v6 1/4] hw: introduce standard SD host controller List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Anthony Liguori , Stefan Hajnoczi , e.voevodin@samsung.com, qemu-devel@nongnu.org, "Peter A. G. Crosthwaite" , kyungmin.park@samsung.com, d.solodkiy@samsung.com, edgar.iglesias@gmail.com, m.kozlov@samsung.com, john.williams@petalogix.com On 08/06/2012 03:15 PM, Peter Maydell wrote: > On 6 August 2012 04:25, Peter A. G. Crosthwaite > wrote: >> From: Igor Mitsyanko >> >> 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.