[v44] hw/sd/sdcard: Cleanups before adding eMMC support

[PATCH v44 2/3] hw/sd/sdcard: Do not store vendor data on block drive (CMD56)

Posted by Philippe Mathieu-Daudé 1 year, 7 months ago

"General command" (GEN_CMD, CMD56) is described as:

  GEN_CMD is the same as the single block read or write
  commands (CMD24 or CMD17). The difference is that [...]
  the data block is not a memory payload data but has a
  vendor specific format and meaning.

Thus this block must not be stored overwriting data block
on underlying storage drive. Keep it in a dedicated
'vendor_data[]' array.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
v43: Do not re-use VMSTATE_UNUSED_V (danpb)
v44: Use subsection (Luc)
---
 hw/sd/sd.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 808dc1cea6..000b923c73 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -153,6 +153,8 @@ struct SDState {
     uint32_t data_offset;
     size_t data_size;
     uint8_t data[512];
+    uint8_t vendor_data[512];
+
     qemu_irq readonly_cb;
     qemu_irq inserted_cb;
     QEMUTimer *ocr_power_timer;
@@ -719,6 +721,7 @@ static void sd_reset(DeviceState *dev)
     sd->wp_switch = sd->blk ? !blk_is_writable(sd->blk) : false;
     sd->wp_group_bits = sect;
     sd->wp_group_bmap = bitmap_new(sd->wp_group_bits);
+    memset(sd->vendor_data, 0xec, sizeof(sd->vendor_data));
     memset(sd->function_group, 0, sizeof(sd->function_group));
     sd->erase_start = INVALID_ADDRESS;
     sd->erase_end = INVALID_ADDRESS;
@@ -793,6 +796,16 @@ static const VMStateDescription sd_ocr_vmstate = {
     },
 };
 
+static const VMStateDescription sd_vendordata_vmstate = {
+    .name = "sd-card/vendor_data-state",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (const VMStateField[]) {
+        VMSTATE_UINT8_ARRAY(vendor_data, SDState, 512),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 static int sd_vmstate_pre_load(void *opaque)
 {
     SDState *sd = opaque;
@@ -840,6 +853,7 @@ static const VMStateDescription sd_vmstate = {
     },
     .subsections = (const VMStateDescription * const []) {
         &sd_ocr_vmstate,
+        &sd_vendordata_vmstate,
         NULL
     },
 };
@@ -2187,9 +2201,8 @@ void sd_write_byte(SDState *sd, uint8_t value)
         break;
 
     case 56:  /* CMD56:  GEN_CMD */
-        sd->data[sd->data_offset ++] = value;
-        if (sd->data_offset >= sd->blk_len) {
-            APP_WRITE_BLOCK(sd->data_start, sd->data_offset);
+        sd->vendor_data[sd->data_offset++] = value;
+        if (sd->data_offset >= sizeof(sd->vendor_data)) {
             sd->state = sd_transfer_state;
         }
         break;
@@ -2261,12 +2274,11 @@ uint8_t sd_read_byte(SDState *sd)
         break;
 
     case 56:  /* CMD56:  GEN_CMD */
-        if (sd->data_offset == 0)
-            APP_READ_BLOCK(sd->data_start, sd->blk_len);
-        ret = sd->data[sd->data_offset ++];
+        ret = sd->vendor_data[sd->data_offset++];
 
-        if (sd->data_offset >= sd->blk_len)
+        if (sd->data_offset >= sizeof(sd->vendor_data)) {
             sd->state = sd_transfer_state;
+        }
         break;
 
     default:
-- 
2.41.0

Re: [PATCH v44 2/3] hw/sd/sdcard: Do not store vendor data on block drive (CMD56)

Posted by Philippe Mathieu-Daudé 1 year, 7 months ago

On 3/7/24 10:51, Philippe Mathieu-Daudé wrote:
> "General command" (GEN_CMD, CMD56) is described as:
> 
>    GEN_CMD is the same as the single block read or write
>    commands (CMD24 or CMD17). The difference is that [...]
>    the data block is not a memory payload data but has a
>    vendor specific format and meaning.
> 
> Thus this block must not be stored overwriting data block
> on underlying storage drive. Keep it in a dedicated
> 'vendor_data[]' array.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> v43: Do not re-use VMSTATE_UNUSED_V (danpb)
> v44: Use subsection (Luc)
> ---
>   hw/sd/sd.c | 26 +++++++++++++++++++-------
>   1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 808dc1cea6..000b923c73 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -153,6 +153,8 @@ struct SDState {
>       uint32_t data_offset;
>       size_t data_size;
>       uint8_t data[512];
> +    uint8_t vendor_data[512];
> +
>       qemu_irq readonly_cb;
>       qemu_irq inserted_cb;
>       QEMUTimer *ocr_power_timer;
> @@ -719,6 +721,7 @@ static void sd_reset(DeviceState *dev)
>       sd->wp_switch = sd->blk ? !blk_is_writable(sd->blk) : false;
>       sd->wp_group_bits = sect;
>       sd->wp_group_bmap = bitmap_new(sd->wp_group_bits);
> +    memset(sd->vendor_data, 0xec, sizeof(sd->vendor_data));
>       memset(sd->function_group, 0, sizeof(sd->function_group));
>       sd->erase_start = INVALID_ADDRESS;
>       sd->erase_end = INVALID_ADDRESS;
> @@ -793,6 +796,16 @@ static const VMStateDescription sd_ocr_vmstate = {
>       },
>   };
>   
> +static const VMStateDescription sd_vendordata_vmstate = {
> +    .name = "sd-card/vendor_data-state",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (const VMStateField[]) {
> +        VMSTATE_UINT8_ARRAY(vendor_data, SDState, 512),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
>   static int sd_vmstate_pre_load(void *opaque)
>   {
>       SDState *sd = opaque;
> @@ -840,6 +853,7 @@ static const VMStateDescription sd_vmstate = {
>       },
>       .subsections = (const VMStateDescription * const []) {
>           &sd_ocr_vmstate,
> +        &sd_vendordata_vmstate,
>           NULL
>       },
>   };

Sigh, forgot to squash:

-- >8 --
@@ -919,3 +918,0 @@ static void sd_blk_write(SDState *sd, uint64_t addr, 
uint32_t len)
-#define APP_READ_BLOCK(a, len)  memset(sd->data, 0xec, len)
-#define APP_WRITE_BLOCK(a, len)
-
---

> @@ -2187,9 +2201,8 @@ void sd_write_byte(SDState *sd, uint8_t value)
>           break;
>   
>       case 56:  /* CMD56:  GEN_CMD */
> -        sd->data[sd->data_offset ++] = value;
> -        if (sd->data_offset >= sd->blk_len) {
> -            APP_WRITE_BLOCK(sd->data_start, sd->data_offset);
> +        sd->vendor_data[sd->data_offset++] = value;
> +        if (sd->data_offset >= sizeof(sd->vendor_data)) {
>               sd->state = sd_transfer_state;
>           }
>           break;
> @@ -2261,12 +2274,11 @@ uint8_t sd_read_byte(SDState *sd)
>           break;
>   
>       case 56:  /* CMD56:  GEN_CMD */
> -        if (sd->data_offset == 0)
> -            APP_READ_BLOCK(sd->data_start, sd->blk_len);
> -        ret = sd->data[sd->data_offset ++];
> +        ret = sd->vendor_data[sd->data_offset++];
>   
> -        if (sd->data_offset >= sd->blk_len)
> +        if (sd->data_offset >= sizeof(sd->vendor_data)) {
>               sd->state = sd_transfer_state;
> +        }
>           break;
>   
>       default:

[PATCH v44 1/3] hw/sd/sdcard: Use spec v3.01 by default
[PATCH v44 2/3] hw/sd/sdcard: Do not store vendor data on block drive (CMD56)
[PATCH v44 3/3] hw/sd/sdcard: Remove leftover comment about removed 'spi' Property