[PATCH 0/3] hw/sd: sd: erase operation fixes

[PATCH 0/3] hw/sd: sd: erase operation fixes

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

This includes several fixes related to erase operation of a SD card.

Based-on:
http://patchwork.ozlabs.org/project/qemu-devel/list/?series=226785


Bin Meng (3):
  hw/sd: sd: Fix address check in sd_erase()
  hw/sd: sd: Move the sd_block_{read,write} and macros ahead
  hw/sd: sd: Actually perform the erase operation

 hw/sd/sd.c | 53 +++++++++++++++++++++++++++++++----------------------
 1 file changed, 31 insertions(+), 22 deletions(-)

-- 
2.25.1

[PATCH 1/3] hw/sd: sd: Fix address check in sd_erase()

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

For high capacity memory cards, the erase start address and end
address are multiplied by 512, but the address check is still
based on the original block number in sd->erase_{start, end}.

Fixes: 1bd6fd8ed593 ("hw/sd/sdcard: Do not attempt to erase out of range addresses")
Signed-off-by: Bin Meng <bin.meng@windriver.com>
---

 hw/sd/sd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index c99c0e93bb..a6a0b3dcc6 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -760,7 +760,7 @@ static void sd_erase(SDState *sd)
         erase_end *= 512;
     }
 
-    if (sd->erase_start > sd->size || sd->erase_end > sd->size) {
+    if (erase_start > sd->size || erase_end > sd->size) {
         sd->card_status |= OUT_OF_RANGE;
         sd->erase_start = INVALID_ADDRESS;
         sd->erase_end = INVALID_ADDRESS;
-- 
2.25.1

[PATCH 2/3] hw/sd: sd: Move the sd_block_{read, write} and macros ahead

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

These APIs and macros may be referenced by functions that are
currently before them. Move them ahead a little bit.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
---

 hw/sd/sd.c | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index a6a0b3dcc6..1886d4b30b 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -739,6 +739,27 @@ void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert)
     qemu_set_irq(insert, sd->blk ? blk_is_inserted(sd->blk) : 0);
 }
 
+static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len)
+{
+    trace_sdcard_read_block(addr, len);
+    if (!sd->blk || blk_pread(sd->blk, addr, sd->data, len) < 0) {
+        fprintf(stderr, "sd_blk_read: read error on host side\n");
+    }
+}
+
+static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
+{
+    trace_sdcard_write_block(addr, len);
+    if (!sd->blk || blk_pwrite(sd->blk, addr, sd->data, len, 0) < 0) {
+        fprintf(stderr, "sd_blk_write: write error on host side\n");
+    }
+}
+
+#define BLK_READ_BLOCK(a, len)  sd_blk_read(sd, a, len)
+#define BLK_WRITE_BLOCK(a, len) sd_blk_write(sd, a, len)
+#define APP_READ_BLOCK(a, len)  memset(sd->data, 0xec, len)
+#define APP_WRITE_BLOCK(a, len)
+
 static void sd_erase(SDState *sd)
 {
     int i;
@@ -1742,27 +1763,6 @@ send_response:
     return rsplen;
 }
 
-static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len)
-{
-    trace_sdcard_read_block(addr, len);
-    if (!sd->blk || blk_pread(sd->blk, addr, sd->data, len) < 0) {
-        fprintf(stderr, "sd_blk_read: read error on host side\n");
-    }
-}
-
-static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
-{
-    trace_sdcard_write_block(addr, len);
-    if (!sd->blk || blk_pwrite(sd->blk, addr, sd->data, len, 0) < 0) {
-        fprintf(stderr, "sd_blk_write: write error on host side\n");
-    }
-}
-
-#define BLK_READ_BLOCK(a, len)	sd_blk_read(sd, a, len)
-#define BLK_WRITE_BLOCK(a, len)	sd_blk_write(sd, a, len)
-#define APP_READ_BLOCK(a, len)	memset(sd->data, 0xec, len)
-#define APP_WRITE_BLOCK(a, len)
-
 void sd_write_byte(SDState *sd, uint8_t value)
 {
     int i;
-- 
2.25.1

[PATCH 3/3] hw/sd: sd: Actually perform the erase operation

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

At present the sd_erase() does not erase the requested range of card
data to 0xFFs. Let's make the erase operation actually happen.

Signed-off-by: Bin Meng <bin.meng@windriver.com>

---

 hw/sd/sd.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 1886d4b30b..8c397d4ad7 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -765,6 +765,8 @@ static void sd_erase(SDState *sd)
     int i;
     uint64_t erase_start = sd->erase_start;
     uint64_t erase_end = sd->erase_end;
+    uint64_t erase_addr;
+    int erase_len = 1 << HWBLOCK_SHIFT;
 
     trace_sdcard_erase(sd->erase_start, sd->erase_end);
     if (sd->erase_start == INVALID_ADDRESS
@@ -788,6 +790,13 @@ static void sd_erase(SDState *sd)
         return;
     }
 
+    memset(sd->data, 0xff, erase_len);
+    erase_addr = erase_start;
+    for (i = 0; i < (erase_end - erase_start) / erase_len + 1; i++) {
+        BLK_WRITE_BLOCK(erase_addr, erase_len);
+        erase_addr += erase_len;
+    }
+
     erase_start = sd_addr_to_wpnum(erase_start);
     erase_end = sd_addr_to_wpnum(erase_end);
     sd->erase_start = INVALID_ADDRESS;
-- 
2.25.1

Re: [PATCH 2/3] hw/sd: sd: Move the sd_block_{read, write} and macros ahead

[Cédric Le Goater]

Hello Bin,

On 1/28/21 7:43 AM, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> These APIs and macros may be referenced by functions that are
> currently before them. Move them ahead a little bit.

We could also change fprintf() by qemu_log_mask()

Thanks,

C. 

 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
> 
>  hw/sd/sd.c | 42 +++++++++++++++++++++---------------------
>  1 file changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index a6a0b3dcc6..1886d4b30b 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -739,6 +739,27 @@ void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert)
>      qemu_set_irq(insert, sd->blk ? blk_is_inserted(sd->blk) : 0);
>  }
>  
> +static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len)
> +{
> +    trace_sdcard_read_block(addr, len);
> +    if (!sd->blk || blk_pread(sd->blk, addr, sd->data, len) < 0) {
> +        fprintf(stderr, "sd_blk_read: read error on host side\n");
> +    }
> +}
> +
> +static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
> +{
> +    trace_sdcard_write_block(addr, len);
> +    if (!sd->blk || blk_pwrite(sd->blk, addr, sd->data, len, 0) < 0) {
> +        fprintf(stderr, "sd_blk_write: write error on host side\n");
> +    }
> +}
> +
> +#define BLK_READ_BLOCK(a, len)  sd_blk_read(sd, a, len)
> +#define BLK_WRITE_BLOCK(a, len) sd_blk_write(sd, a, len)
> +#define APP_READ_BLOCK(a, len)  memset(sd->data, 0xec, len)
> +#define APP_WRITE_BLOCK(a, len)
> +
>  static void sd_erase(SDState *sd)
>  {
>      int i;
> @@ -1742,27 +1763,6 @@ send_response:
>      return rsplen;
>  }
>  
> -static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len)
> -{
> -    trace_sdcard_read_block(addr, len);
> -    if (!sd->blk || blk_pread(sd->blk, addr, sd->data, len) < 0) {
> -        fprintf(stderr, "sd_blk_read: read error on host side\n");
> -    }
> -}
> -
> -static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
> -{
> -    trace_sdcard_write_block(addr, len);
> -    if (!sd->blk || blk_pwrite(sd->blk, addr, sd->data, len, 0) < 0) {
> -        fprintf(stderr, "sd_blk_write: write error on host side\n");
> -    }
> -}
> -
> -#define BLK_READ_BLOCK(a, len)	sd_blk_read(sd, a, len)
> -#define BLK_WRITE_BLOCK(a, len)	sd_blk_write(sd, a, len)
> -#define APP_READ_BLOCK(a, len)	memset(sd->data, 0xec, len)
> -#define APP_WRITE_BLOCK(a, len)
> -
>  void sd_write_byte(SDState *sd, uint8_t value)
>  {
>      int i;
> 

Re: [PATCH 0/3] hw/sd: sd: erase operation fixes

[Bin Meng]

On Thu, Jan 28, 2021 at 2:43 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> From: Bin Meng <bin.meng@windriver.com>
>
> This includes several fixes related to erase operation of a SD card.
>
> Based-on:
> http://patchwork.ozlabs.org/project/qemu-devel/list/?series=226785
>
>
> Bin Meng (3):
>   hw/sd: sd: Fix address check in sd_erase()
>   hw/sd: sd: Move the sd_block_{read,write} and macros ahead
>   hw/sd: sd: Actually perform the erase operation
>
>  hw/sd/sd.c | 53 +++++++++++++++++++++++++++++++----------------------
>  1 file changed, 31 insertions(+), 22 deletions(-)

Ping?

Re: [PATCH 1/3] hw/sd: sd: Fix address check in sd_erase()

[Philippe Mathieu-Daudé]

On 1/28/21 7:43 AM, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> For high capacity memory cards, the erase start address and end
> address are multiplied by 512, but the address check is still
> based on the original block number in sd->erase_{start, end}.

Oops, good catch.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> 
> Fixes: 1bd6fd8ed593 ("hw/sd/sdcard: Do not attempt to erase out of range addresses")
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
> 
>  hw/sd/sd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Re: [PATCH 2/3] hw/sd: sd: Move the sd_block_{read, write} and macros ahead

[Philippe Mathieu-Daudé]

On 1/28/21 8:04 AM, Cédric Le Goater wrote:
> Hello Bin,
> 
> On 1/28/21 7:43 AM, Bin Meng wrote:
>> From: Bin Meng <bin.meng@windriver.com>
>>
>> These APIs and macros may be referenced by functions that are
>> currently before them. Move them ahead a little bit.
> 
> We could also change fprintf() by qemu_log_mask()

Hmm in this case warn_report() maybe, as IIUC QEMU aims
to support using smaller block drive (which this model
currently rejects), in which case this wouldn't be a
GUEST_ERROR. Can be done on top in another patch, since
this is pure code movement.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 3/3] hw/sd: sd: Actually perform the erase operation

[Philippe Mathieu-Daudé]

On 1/28/21 7:43 AM, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> At present the sd_erase() does not erase the requested range of card
> data to 0xFFs. Let's make the erase operation actually happen.
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> 
> ---
> 
>  hw/sd/sd.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 1886d4b30b..8c397d4ad7 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -765,6 +765,8 @@ static void sd_erase(SDState *sd)
>      int i;
>      uint64_t erase_start = sd->erase_start;
>      uint64_t erase_end = sd->erase_end;
> +    uint64_t erase_addr;
> +    int erase_len = 1 << HWBLOCK_SHIFT;
>  
>      trace_sdcard_erase(sd->erase_start, sd->erase_end);
>      if (sd->erase_start == INVALID_ADDRESS
> @@ -788,6 +790,13 @@ static void sd_erase(SDState *sd)
>          return;
>      }
>  
> +    memset(sd->data, 0xff, erase_len);
> +    erase_addr = erase_start;
> +    for (i = 0; i < (erase_end - erase_start) / erase_len + 1; i++) {
> +        BLK_WRITE_BLOCK(erase_addr, erase_len);
> +        erase_addr += erase_len;
> +    }

Watch out, you are erasing eventual write-protected blocks.

>      erase_start = sd_addr_to_wpnum(erase_start);
>      erase_end = sd_addr_to_wpnum(erase_end);
>      sd->erase_start = INVALID_ADDRESS;
>