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

[Manos Pitsidianakis <manos.pitsidianakis@linaro.org>]

On Wed, 03 Jul 2024 11:59, Philippe Mathieu-Daudé <philmd@linaro.org> 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.
>


I am reading the 4.3 spec, and it says:

  The bus transaction of the GEN_CMD is the same as the single block 
  read or write commands (CMD24 or CMD17). The difference is that the 
  argument denotes the direction of the data transfer (rather than the 
  address) and the data block is not a memory payload data but has a 
  vendor specific format and meaning.

The vendor here (qemu) does not support any functionality with CMD56. I 
think the correct approach would be to read zeros and discard writes, 
without storing anything and without changing data_offset (which is for 
`data` buffer)

What do you think?


>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
>v43: Do not re-use VMSTATE_UNUSED_V (danpb)
>v44: Use subsection (Luc)
>v45: Remove APP_READ_BLOCK/APP_WRITE_BLOCK macros
>---
> hw/sd/sd.c | 29 +++++++++++++++++++----------
> 1 file changed, 19 insertions(+), 10 deletions(-)
>
>diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>index a08a452d81..5d58937dd4 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
>     },
> };
>@@ -902,9 +916,6 @@ 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)
>-
> static void sd_erase(SDState *sd)
> {
>     uint64_t erase_start = sd->erase_start;
>@@ -2187,9 +2198,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 +2271,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
>
>