From 9227cc52ccad9879575a0e5aa1f0bf991f207d2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Tue, 1 Sep 2020 15:21:54 +0200 Subject: [PATCH 01/13] hw/sd/sdhci: Fix qemu_log_mask() format string MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add missing newline character in qemu_log_mask() format. Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Richard Henderson Tested-by: Alexander Bulekov Message-Id: <20200901140411.112150-2-f4bug@amsat.org> --- hw/sd/sdhci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index 6900213083..6d4603f5b1 100644 --- a/hw/sd/sdhci.c +++ b/hw/sd/sdhci.c @@ -1112,7 +1112,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size) /* Limit block size to the maximum buffer size */ if (extract32(s->blksize, 0, 12) > s->buf_maxsz) { qemu_log_mask(LOG_GUEST_ERROR, "%s: Size 0x%x is larger than " - "the maximum buffer 0x%x", __func__, s->blksize, + "the maximum buffer 0x%x\n", __func__, s->blksize, s->buf_maxsz); s->blksize = deposit32(s->blksize, 0, 12, s->buf_maxsz); From 598a40b30f13b3cde6764173449671d0d8c4d058 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Tue, 1 Sep 2020 15:23:14 +0200 Subject: [PATCH 02/13] hw/sd/sdhci: Document the datasheet used MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add datasheet name in the file header. We can not add the direct download link since there is a disclaimers to agree first on the SD Association website (www.sdcard.org). Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Richard Henderson Tested-by: Alexander Bulekov Message-Id: <20200901140411.112150-3-f4bug@amsat.org> --- hw/sd/sdhci.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index 6d4603f5b1..ed717ab549 100644 --- a/hw/sd/sdhci.c +++ b/hw/sd/sdhci.c @@ -1,6 +1,8 @@ /* * SD Association Host Standard Specification v2.0 controller emulation * + * Datasheet: PartA2_SD_Host_Controller_Simplified_Specification_Ver2.00.pdf + * * Copyright (c) 2011 Samsung Electronics Co., Ltd. * Mitsyanko Igor * Peter A.G. Crosthwaite From dfba99f17feb6d4a129da19d38df1bcd8579d1c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Tue, 1 Sep 2020 15:22:06 +0200 Subject: [PATCH 03/13] hw/sd/sdhci: Fix DMA Transfer Block Size field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 'Transfer Block Size' field is 12-bit wide. See section '2.2.2. Block Size Register (Offset 004h)' in datasheet. Two different bug reproducer available: - https://bugs.launchpad.net/qemu/+bug/1892960 - https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Fsdhci_oob_write1 Cc: qemu-stable@nongnu.org Buglink: https://bugs.launchpad.net/qemu/+bug/1892960 Fixes: d7dfca0807a ("hw/sdhci: introduce standard SD host controller") Reported-by: Alexander Bulekov Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Prasad J Pandit Tested-by: Alexander Bulekov Message-Id: <20200901140411.112150-3-f4bug@amsat.org> --- hw/sd/sdhci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index ed717ab549..c849c3d99b 100644 --- a/hw/sd/sdhci.c +++ b/hw/sd/sdhci.c @@ -1107,7 +1107,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size) break; case SDHC_BLKSIZE: if (!TRANSFERRING_DATA(s->prnsts)) { - MASKED_WRITE(s->blksize, mask, value); + MASKED_WRITE(s->blksize, mask, extract32(value, 0, 12)); MASKED_WRITE(s->blkcnt, mask >> 16, value >> 16); } From 6a9e5cc61c52af53c71ac24411324427650e6755 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Thu, 3 Sep 2020 18:05:41 +0200 Subject: [PATCH 04/13] hw/sd/sdhci: Stop multiple transfers when block count is cleared MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clearing BlockCount stops multiple transfers. See "SD Host Controller Simplified Specification Version 2.00": - 2.2.3. Block Count Register (Offset 006h) - Table 2-8 : Determination of Transfer Type Signed-off-by: Philippe Mathieu-Daudé Tested-by: Alexander Bulekov Message-Id: <20200903172806.489710-2-f4bug@amsat.org> --- hw/sd/sdhci.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index c849c3d99b..61e469bd32 100644 --- a/hw/sd/sdhci.c +++ b/hw/sd/sdhci.c @@ -731,6 +731,12 @@ static void sdhci_do_adma(SDHCIState *s) ADMADescr dscr = {}; int i; + if (s->trnmod & SDHC_TRNS_BLK_CNT_EN && !s->blkcnt) { + /* Stop Multiple Transfer */ + sdhci_end_transfer(s); + return; + } + for (i = 0; i < SDHC_ADMA_DESCS_PER_DELAY; ++i) { s->admaerr &= ~SDHC_ADMAERR_LENGTH_MISMATCH; @@ -756,7 +762,6 @@ static void sdhci_do_adma(SDHCIState *s) 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) { From 45e5dc43b3dab096bedf0d537e9b99ee169d0784 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Thu, 3 Sep 2020 19:00:04 +0200 Subject: [PATCH 05/13] hw/sd/sdhci: Resume pending DMA transfers on MMIO accesses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If we have pending DMA requests scheduled, process them first. So far we don't need to implement a bottom half to process them. Signed-off-by: Philippe Mathieu-Daudé Tested-by: Alexander Bulekov Message-Id: <20200903172806.489710-3-f4bug@amsat.org> --- hw/sd/sdhci.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index 61e469bd32..4db77decf8 100644 --- a/hw/sd/sdhci.c +++ b/hw/sd/sdhci.c @@ -948,11 +948,21 @@ sdhci_buff_access_is_sequential(SDHCIState *s, unsigned byte_num) return true; } +static void sdhci_resume_pending_transfer(SDHCIState *s) +{ + timer_del(s->transfer_timer); + sdhci_data_transfer(s); +} + static uint64_t sdhci_read(void *opaque, hwaddr offset, unsigned size) { SDHCIState *s = (SDHCIState *)opaque; uint32_t ret = 0; + if (timer_pending(s->transfer_timer)) { + sdhci_resume_pending_transfer(s); + } + switch (offset & ~0x3) { case SDHC_SYSAD: ret = s->sdmasysad; @@ -1096,6 +1106,10 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size) uint32_t value = val; value <<= shift; + if (timer_pending(s->transfer_timer)) { + sdhci_resume_pending_transfer(s); + } + switch (offset & ~0x3) { case SDHC_SYSAD: s->sdmasysad = (s->sdmasysad & mask) | value; From 2bd9ae7e3087a5b853d67ddbedca1b94f88229cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Thu, 3 Sep 2020 18:48:36 +0200 Subject: [PATCH 06/13] hw/sd/sdhci: Let sdhci_update_irq() return if IRQ was delivered MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Philippe Mathieu-Daudé Tested-by: Alexander Bulekov Message-Id: <20200903172806.489710-4-f4bug@amsat.org> --- hw/sd/sdhci.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index 4db77decf8..b93ecefd20 100644 --- a/hw/sd/sdhci.c +++ b/hw/sd/sdhci.c @@ -218,9 +218,14 @@ static uint8_t sdhci_slotint(SDHCIState *s) ((s->norintsts & SDHC_NIS_REMOVE) && (s->wakcon & SDHC_WKUP_ON_RMV)); } -static inline void sdhci_update_irq(SDHCIState *s) +/* Return true if IRQ was pending and delivered */ +static bool sdhci_update_irq(SDHCIState *s) { - qemu_set_irq(s->irq, sdhci_slotint(s)); + bool pending = sdhci_slotint(s); + + qemu_set_irq(s->irq, pending); + + return pending; } static void sdhci_raise_insertion_irq(void *opaque) From 9321c1f2d08817fdb90ad129fbe3194207e73ba0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Thu, 3 Sep 2020 17:31:04 +0200 Subject: [PATCH 07/13] hw/sd/sdhci: Yield if interrupt delivered during multiple transfer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Descriptor Table has a bit to allow the DMA to generates Interrupt when the operation of the descriptor line is completed (see "1.13.4. Descriptor Table" of 'SD Host Controller Simplified Specification Version 2.00'). If we have pending interrupt and the descriptor requires it to be generated as soon as it is completed, reschedule pending transfers and yield to the CPU. Signed-off-by: Philippe Mathieu-Daudé Tested-by: Alexander Bulekov Message-Id: <20200903172806.489710-5-f4bug@amsat.org> --- hw/sd/sdhci.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index b93ecefd20..2f8b74a84f 100644 --- a/hw/sd/sdhci.c +++ b/hw/sd/sdhci.c @@ -837,7 +837,10 @@ static void sdhci_do_adma(SDHCIState *s) s->norintsts |= SDHC_NIS_DMA; } - sdhci_update_irq(s); + if (sdhci_update_irq(s) && !(dscr.attr & SDHC_ADMA_ATTR_END)) { + /* IRQ delivered, reschedule current transfer */ + break; + } } /* ADMA transfer terminates if blkcnt == 0 or by END attribute */ From aafe6c583696fa40677bcd2285da5e7a5210b3eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Sun, 13 Sep 2020 13:18:19 +0200 Subject: [PATCH 08/13] hw/sd/sdcard: Add trace event for ERASE command (CMD38) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Trace addresses provided to the ERASE command. Signed-off-by: Philippe Mathieu-Daudé Tested-by: Alexander Bulekov Message-Id: <20201015063824.212980-2-f4bug@amsat.org> --- hw/sd/sd.c | 2 +- hw/sd/trace-events | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/sd/sd.c b/hw/sd/sd.c index 0012882222..2606b969e3 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -749,7 +749,7 @@ static void sd_erase(SDState *sd) uint64_t erase_start = sd->erase_start; uint64_t erase_end = sd->erase_end; - trace_sdcard_erase(); + trace_sdcard_erase(sd->erase_start, sd->erase_end); if (!sd->erase_start || !sd->erase_end) { sd->card_status |= ERASE_SEQ_ERROR; return; diff --git a/hw/sd/trace-events b/hw/sd/trace-events index a87d7355fb..96c7ea5e52 100644 --- a/hw/sd/trace-events +++ b/hw/sd/trace-events @@ -46,7 +46,7 @@ sdcard_reset(void) "" sdcard_set_blocklen(uint16_t length) "0x%03x" sdcard_inserted(bool readonly) "read_only: %u" sdcard_ejected(void) "" -sdcard_erase(void) "" +sdcard_erase(uint32_t first, uint32_t last) "addr first 0x%" PRIx32" last 0x%" PRIx32 sdcard_lock(void) "" sdcard_unlock(void) "" sdcard_read_block(uint64_t addr, uint32_t len) "addr 0x%" PRIx64 " size 0x%x" From 872b8fde6c642e1da234bcfb7bb3fb9a8d746ff3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Fri, 18 Sep 2020 19:05:20 +0200 Subject: [PATCH 09/13] hw/sd/sdcard: Introduce the INVALID_ADDRESS definition MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit '0' is used as a value to indicate an invalid (or unset) address. Use a definition instead of a magic value. Signed-off-by: Philippe Mathieu-Daudé Tested-by: Alexander Bulekov Message-Id: <20201015063824.212980-3-f4bug@amsat.org> --- hw/sd/sd.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/hw/sd/sd.c b/hw/sd/sd.c index 2606b969e3..30ae435d66 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -53,6 +53,8 @@ #define SDSC_MAX_CAPACITY (2 * GiB) +#define INVALID_ADDRESS 0 + typedef enum { sd_r0 = 0, /* no response */ sd_r1, /* normal response command */ @@ -575,8 +577,8 @@ static void sd_reset(DeviceState *dev) sd->wpgrps_size = sect; sd->wp_groups = bitmap_new(sd->wpgrps_size); memset(sd->function_group, 0, sizeof(sd->function_group)); - sd->erase_start = 0; - sd->erase_end = 0; + sd->erase_start = INVALID_ADDRESS; + sd->erase_end = INVALID_ADDRESS; sd->size = size; sd->blk_len = 0x200; sd->pwd_len = 0; @@ -750,7 +752,8 @@ static void sd_erase(SDState *sd) uint64_t erase_end = sd->erase_end; trace_sdcard_erase(sd->erase_start, sd->erase_end); - if (!sd->erase_start || !sd->erase_end) { + if (sd->erase_start == INVALID_ADDRESS + || sd->erase_end == INVALID_ADDRESS) { sd->card_status |= ERASE_SEQ_ERROR; return; } @@ -763,8 +766,8 @@ static void sd_erase(SDState *sd) erase_start = sd_addr_to_wpnum(erase_start); erase_end = sd_addr_to_wpnum(erase_end); - sd->erase_start = 0; - sd->erase_end = 0; + sd->erase_start = INVALID_ADDRESS; + sd->erase_end = INVALID_ADDRESS; sd->csd[14] |= 0x40; for (i = erase_start; i <= erase_end; i++) { From 7dae0a1dd102ea5e58869a3082c61bfcadf29347 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Fri, 18 Sep 2020 19:06:41 +0200 Subject: [PATCH 10/13] hw/sd/sdcard: Do not use legal address '0' for INVALID_ADDRESS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As it is legal to WRITE/ERASE the address/block 0, change the value of this definition to an illegal address: UINT32_MAX. Unfortunately this break the migration stream, so bump the VMState version number. This affects some ARM boards and the SDHCI_PCI device (which is only used for testing). Signed-off-by: Philippe Mathieu-Daudé Tested-by: Alexander Bulekov Message-Id: <20201015063824.212980-4-f4bug@amsat.org> --- hw/sd/sd.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/hw/sd/sd.c b/hw/sd/sd.c index 30ae435d66..4c05152f18 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -53,7 +53,7 @@ #define SDSC_MAX_CAPACITY (2 * GiB) -#define INVALID_ADDRESS 0 +#define INVALID_ADDRESS UINT32_MAX typedef enum { sd_r0 = 0, /* no response */ @@ -666,8 +666,8 @@ static int sd_vmstate_pre_load(void *opaque) static const VMStateDescription sd_vmstate = { .name = "sd-card", - .version_id = 1, - .minimum_version_id = 1, + .version_id = 2, + .minimum_version_id = 2, .pre_load = sd_vmstate_pre_load, .fields = (VMStateField[]) { VMSTATE_UINT32(mode, SDState), From c8c8b3f1c179e1b8d21c2e636dc893ebfc522874 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Sun, 13 Sep 2020 13:18:31 +0200 Subject: [PATCH 11/13] hw/sd/sdcard: Reset both start/end addresses on error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the Spec "4.3.5 Erase": The host should adhere to the following command sequence: ERASE_WR_BLK_START, ERASE_WR_BLK_END and ERASE (CMD38). If an erase (CMD38) or address setting (CMD32, 33) command is received out of sequence, the card shall set the ERASE_SEQ_ERROR bit in the status register and reset the whole sequence. Reset both addresses if the ERASE command occured out of sequence (one of the start/end address is not set). Signed-off-by: Philippe Mathieu-Daudé Tested-by: Alexander Bulekov Message-Id: <20201015063824.212980-5-f4bug@amsat.org> --- hw/sd/sd.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/sd/sd.c b/hw/sd/sd.c index 4c05152f18..ee7b64023a 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -755,6 +755,8 @@ static void sd_erase(SDState *sd) if (sd->erase_start == INVALID_ADDRESS || sd->erase_end == INVALID_ADDRESS) { sd->card_status |= ERASE_SEQ_ERROR; + sd->erase_start = INVALID_ADDRESS; + sd->erase_end = INVALID_ADDRESS; return; } From 1bd6fd8ed5933bfba53e5f5eadebd845094c3707 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Sun, 13 Sep 2020 13:18:52 +0200 Subject: [PATCH 12/13] hw/sd/sdcard: Do not attempt to erase out of range addresses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While the Spec v3 is not very clear, v6 states: If the host provides an out of range address as an argument to CMD32 or CMD33, the card shall indicate OUT_OF_RANGE error in R1 (ERX) for CMD38. If an address is out of range, do not attempt to erase it: return R1 with the error bit set. Buglink: https://bugs.launchpad.net/qemu/+bug/1895310 Reported-by: Alexander Bulekov Signed-off-by: Philippe Mathieu-Daudé Tested-by: Alexander Bulekov Message-Id: <20201015063824.212980-6-f4bug@amsat.org> --- hw/sd/sd.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/hw/sd/sd.c b/hw/sd/sd.c index ee7b64023a..4454d168e2 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -766,6 +766,13 @@ static void sd_erase(SDState *sd) erase_end *= 512; } + if (sd->erase_start > sd->size || sd->erase_end > sd->size) { + sd->card_status |= OUT_OF_RANGE; + sd->erase_start = INVALID_ADDRESS; + sd->erase_end = INVALID_ADDRESS; + return; + } + erase_start = sd_addr_to_wpnum(erase_start); erase_end = sd_addr_to_wpnum(erase_end); sd->erase_start = INVALID_ADDRESS; From 84816fb63e5c57159b469a66052d1b2bc862ef77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Fri, 18 Sep 2020 19:14:52 +0200 Subject: [PATCH 13/13] hw/sd/sdcard: Assert if accessing an illegal group MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We can not have more group than 'wpgrps_size'. Assert if we are accessing a group above this limit. Signed-off-by: Philippe Mathieu-Daudé Tested-by: Alexander Bulekov Message-Id: <20201015063824.212980-7-f4bug@amsat.org> --- hw/sd/sd.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/sd/sd.c b/hw/sd/sd.c index 4454d168e2..c3febed243 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -780,6 +780,7 @@ static void sd_erase(SDState *sd) sd->csd[14] |= 0x40; for (i = erase_start; i <= erase_end; i++) { + assert(i < sd->wpgrps_size); if (test_bit(i, sd->wp_groups)) { sd->card_status |= WP_ERASE_SKIP; } @@ -794,6 +795,7 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr) wpnum = sd_addr_to_wpnum(addr); for (i = 0; i < 32; i++, wpnum++, addr += WPGROUP_SIZE) { + assert(wpnum < sd->wpgrps_size); if (addr < sd->size && test_bit(wpnum, sd->wp_groups)) { ret |= (1 << i); }