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

[Luc Michel]

On 09:00 Fri 28 Jun     , Philippe Mathieu-Daudé wrote:
> Caution: This message originated from an External Source. Use proper caution 
> when opening attachments, clicking links, or responding.
> 
> 
> "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>
> Tested-by: Cédric Le Goater <clg@redhat.com>
> ---
> RFC: Is it safe to reuse VMSTATE_UNUSED_V() (which happens
> to be the same size)?
> 
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Fabiano Rosas <farosas@suse.de>

I'm not sure about this migration question.

But IMHO you can simplify your implementation to avoid having to store
and migrate this vendor_data array. After some research on this command,
I came to the conclusion that it's used by manufacturers to return
device health related vendor-specific data. (E.g.,
https://images-na.ssl-images-amazon.com/images/I/91tTtUMDM3L.pdf Section
1.6.1). So I guess you can simply discard writes and return 0s on reads
(or "QEMU" in ASCII or... :)).

> ---
>  hw/sd/sd.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 464576751a..1f3eea6e84 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -142,6 +142,8 @@ struct SDState {
>      uint64_t data_start;
>      uint32_t data_offset;
>      uint8_t data[512];
> +    uint8_t vendor_data[512];
> +
>      qemu_irq readonly_cb;
>      qemu_irq inserted_cb;
>      QEMUTimer *ocr_power_timer;
> @@ -656,6 +658,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;
> @@ -771,7 +774,7 @@ static const VMStateDescription sd_vmstate = {
>          VMSTATE_UINT64(data_start, SDState),
>          VMSTATE_UINT32(data_offset, SDState),
>          VMSTATE_UINT8_ARRAY(data, SDState, 512),
> -        VMSTATE_UNUSED_V(1, 512),
> +        VMSTATE_UINT8_ARRAY(vendor_data, SDState, 512),
>          VMSTATE_BOOL(enable, SDState),
>          VMSTATE_END_OF_LIST()
>      },
> @@ -2029,9 +2032,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;
> @@ -2165,12 +2167,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
> 
> 

--