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

"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>
---
 hw/sd/sd.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

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
> 
> 

--

On Fri, Jun 28, 2024 at 09:00:42AM +0200, 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>
> 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)?

This field became unused with:

commit 12c125cba9c548929ccf4da2515e5b795c94afd9
Author: Eric Blake <eblake@redhat.com>
Date:   Fri May 6 10:26:39 2016 -0600

    sd: Switch to byte-based block access
    
which was in 2.6.1 / 2.7.0


Thus if someone is using a machine type that is 2.6 or
older, I don't think it is safe to unconditionally
reuse that field.

My pending series deprecates everything upto 2.12, but
we won't remove those machine types until 2 further
release are past.

You could gamble that SD card usage is niche enough
that its highly unlikely someone will be using SD
card at the same time as these ancient machine types.

The safe thing would be a new field.
 
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Fabiano Rosas <farosas@suse.de>
> ---
>  hw/sd/sd.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)

With regards,
Daniel

On 1/7/24 10:01, Luc Michel wrote:
> 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... :)).

Thanks, very interesting datasheet! Note the argument filter:

   To query the Health Status register, CMD56 with
   argument of [00 00 00 01] is used.

Since we can program this array, I'll simply add it as R/W (KISS).

> 
>> ---
>>   hw/sd/sd.c | 17 +++++++++--------
>>   1 file changed, 9 insertions(+), 8 deletions(-)

On 2/7/24 18:04, Philippe Mathieu-Daudé wrote:
> On 1/7/24 10:01, Luc Michel wrote:
>> 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... :)).
> 
> Thanks, very interesting datasheet! Note the argument filter:
> 
>    To query the Health Status register, CMD56 with
>    argument of [00 00 00 01] is used.
> 
> Since we can program this array, I'll simply add it as R/W (KISS).

(What really matters here is that we don't overwrite the generic
  data[] array used to access the blocks)