[PATCH v43 0/2] hw/sd/sdcard: Cleanups before adding eMMC support

[PATCH v43 0/2] hw/sd/sdcard: Cleanups before adding eMMC support

[Philippe Mathieu-Daudé]

Addressing review comments from Daniel on v42:
https://lore.kernel.org/qemu-devel/20240628070216.92609-1-philmd@linaro.org/
(patches already reviewed not reposted)

Supersedes: <20240628070216.92609-1-philmd@linaro.org>

Philippe Mathieu-Daudé (2):
  hw/sd/sdcard: Use spec v3.01 by default
  hw/sd/sdcard: Do not store vendor data on block drive (CMD56)

 hw/core/machine.c |  1 +
 hw/sd/sd.c        | 18 ++++++++++--------
 2 files changed, 11 insertions(+), 8 deletions(-)

-- 
2.41.0

[PATCH v43 1/2] hw/sd/sdcard: Use spec v3.01 by default

[Philippe Mathieu-Daudé]

Recent SDHCI expect cards to support the v3.01 spec
to negociate lower I/O voltage. Select it by default.

Versioned machine types with a version of 9.0 or
earlier retain the old default (spec v2.00).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
---
v43: update versioned machines (danpb)
---
 hw/core/machine.c | 1 +
 hw/sd/sd.c        | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 655d75c21f..4377f943d5 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -38,6 +38,7 @@ GlobalProperty hw_compat_9_0[] = {
     {"arm-cpu", "backcompat-cntfrq", "true" },
     {"scsi-disk-base", "migrate-emulated-scsi-request", "false" },
     {"vfio-pci", "skip-vsc-check", "false" },
+    {"sd-card", "spec_version", "2" },
 };
 const size_t hw_compat_9_0_len = G_N_ELEMENTS(hw_compat_9_0);
 
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index b158402704..808dc1cea6 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2471,7 +2471,7 @@ static void sd_realize(DeviceState *dev, Error **errp)
 
 static Property sd_properties[] = {
     DEFINE_PROP_UINT8("spec_version", SDState,
-                      spec_version, SD_PHY_SPECv2_00_VERS),
+                      spec_version, SD_PHY_SPECv3_01_VERS),
     DEFINE_PROP_DRIVE("drive", SDState, blk),
     /* We do not model the chip select pin, so allow the board to select
      * whether card should be in SSI or MMC/SD mode.  It is also up to the
-- 
2.41.0

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

[Philippe Mathieu-Daudé]

"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>
---
v43: Do not re-use VMSTATE_UNUSED_V (danpb)
---
 hw/sd/sd.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 808dc1cea6..418ccb14a4 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;
@@ -835,6 +838,7 @@ static const VMStateDescription sd_vmstate = {
         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()
     },
@@ -2187,9 +2191,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 +2264,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 v43 2/2] hw/sd/sdcard: Do not store vendor data on block drive (CMD56)

[Luc Michel]

On 18:10 Tue 02 Jul     , 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>
> ---
> v43: Do not re-use VMSTATE_UNUSED_V (danpb)
> ---
>  hw/sd/sd.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 808dc1cea6..418ccb14a4 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;
> @@ -835,6 +838,7 @@ static const VMStateDescription sd_vmstate = {
>          VMSTATE_UINT32(data_offset, SDState),
>          VMSTATE_UINT8_ARRAY(data, SDState, 512),
>          VMSTATE_UNUSED_V(1, 512),
> +        VMSTATE_UINT8_ARRAY(vendor_data, SDState, 512),

Don't you need to bump the VMState version then?

>          VMSTATE_BOOL(enable, SDState),
>          VMSTATE_END_OF_LIST()
>      },
> @@ -2187,9 +2191,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 +2264,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 v43 2/2] hw/sd/sdcard: Do not store vendor data on block drive (CMD56)

[Philippe Mathieu-Daudé]

On 3/7/24 09:39, Luc Michel wrote:
> On 18:10 Tue 02 Jul     , 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>
>> ---
>> v43: Do not re-use VMSTATE_UNUSED_V (danpb)
>> ---
>>   hw/sd/sd.c | 16 +++++++++-------
>>   1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index 808dc1cea6..418ccb14a4 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;
>> @@ -835,6 +838,7 @@ static const VMStateDescription sd_vmstate = {
>>           VMSTATE_UINT32(data_offset, SDState),
>>           VMSTATE_UINT8_ARRAY(data, SDState, 512),
>>           VMSTATE_UNUSED_V(1, 512),
>> +        VMSTATE_UINT8_ARRAY(vendor_data, SDState, 512),
> 
> Don't you need to bump the VMState version then?

Indeed. I'll add a subsection which is the recommended way:
https://www.qemu.org/docs/master/devel/migration/main.html#subsections

   The most common structure change is adding new data, e.g. when
   adding a newer form of device, or adding that state that you
   previously forgot to migrate. This is best solved using a subsection.

Thanks,

Phil.