Patchwork [V4,08/12] hw/sd.c: add SD card save/load support

login
register
mail settings
Submitter Mitsyanko Igor
Date July 27, 2012, 7:29 p.m.
Message ID <1343417387-13953-9-git-send-email-i.mitsyanko@samsung.com>
Download mbox | patch
Permalink /patch/173777/
State New
Headers show

Comments

Mitsyanko Igor - July 27, 2012, 7:29 p.m.
This patch updates SD card model to support save/load of card's state.

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
---
 hw/sd.c |   88 +++++++++++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 64 insertions(+), 24 deletions(-)
Markus Armbruster - July 31, 2012, 9:33 a.m.
Igor Mitsyanko <i.mitsyanko@samsung.com> writes:

> This patch updates SD card model to support save/load of card's state.
>
> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
> ---
>  hw/sd.c |   88 +++++++++++++++++++++++++++++++++++++++++++++-----------------
>  1 files changed, 64 insertions(+), 24 deletions(-)
>
> diff --git a/hw/sd.c b/hw/sd.c
> index 20ebd8e..f8ab045 100644
> --- a/hw/sd.c
> +++ b/hw/sd.c
> @@ -55,24 +55,28 @@ typedef enum {
>      sd_illegal = -2,
>  } sd_rsp_type_t;
>  
> +enum {
> +    sd_inactive,
> +    sd_card_identification_mode,
> +    sd_data_transfer_mode,
> +};
> +
> +enum {
> +    sd_inactive_state = -1,
> +    sd_idle_state = 0,
> +    sd_ready_state,
> +    sd_identification_state,
> +    sd_standby_state,
> +    sd_transfer_state,
> +    sd_sendingdata_state,
> +    sd_receivingdata_state,
> +    sd_programming_state,
> +    sd_disconnect_state,
> +};
> +
>  struct SDState {
> -    enum {
> -        sd_inactive,
> -        sd_card_identification_mode,
> -        sd_data_transfer_mode,
> -    } mode;
> -    enum {
> -        sd_inactive_state = -1,
> -        sd_idle_state = 0,
> -        sd_ready_state,
> -        sd_identification_state,
> -        sd_standby_state,
> -        sd_transfer_state,
> -        sd_sendingdata_state,
> -        sd_receivingdata_state,
> -        sd_programming_state,
> -        sd_disconnect_state,
> -    } state;
> +    uint32_t mode;
> +    int32_t state;

Comment pointing to the related enum?

>      uint32_t ocr;
>      uint8_t scr[8];
>      uint8_t cid[16];

[...]
Mitsyanko Igor - July 31, 2012, 10:27 a.m.
On 07/31/2012 01:33 PM, Markus Armbruster wrote:
> Igor Mitsyanko <i.mitsyanko@samsung.com> writes:
>
>> This patch updates SD card model to support save/load of card's state.
>>
>> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
>> ---
>>   hw/sd.c |   88 +++++++++++++++++++++++++++++++++++++++++++++-----------------
>>   1 files changed, 64 insertions(+), 24 deletions(-)
>>
>> diff --git a/hw/sd.c b/hw/sd.c
>> index 20ebd8e..f8ab045 100644
>> --- a/hw/sd.c
>> +++ b/hw/sd.c
>> @@ -55,24 +55,28 @@ typedef enum {
>>       sd_illegal = -2,
>>   } sd_rsp_type_t;
>>   
>> +enum {
>> +    sd_inactive,
>> +    sd_card_identification_mode,
>> +    sd_data_transfer_mode,
>> +};
>> +
>> +enum {
>> +    sd_inactive_state = -1,
>> +    sd_idle_state = 0,
>> +    sd_ready_state,
>> +    sd_identification_state,
>> +    sd_standby_state,
>> +    sd_transfer_state,
>> +    sd_sendingdata_state,
>> +    sd_receivingdata_state,
>> +    sd_programming_state,
>> +    sd_disconnect_state,
>> +};
>> +
>>   struct SDState {
>> -    enum {
>> -        sd_inactive,
>> -        sd_card_identification_mode,
>> -        sd_data_transfer_mode,
>> -    } mode;
>> -    enum {
>> -        sd_inactive_state = -1,
>> -        sd_idle_state = 0,
>> -        sd_ready_state,
>> -        sd_identification_state,
>> -        sd_standby_state,
>> -        sd_transfer_state,
>> -        sd_sendingdata_state,
>> -        sd_receivingdata_state,
>> -        sd_programming_state,
>> -        sd_disconnect_state,
>> -    } state;
>> +    uint32_t mode;
>> +    int32_t state;
> Comment pointing to the related enum?

Ok, I'll have to give them names then.

>>       uint32_t ocr;
>>       uint8_t scr[8];
>>       uint8_t cid[16];
> [...]
>
Peter Maydell - July 31, 2012, 2:56 p.m.
On 27 July 2012 20:29, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> +        VMSTATE_BUFFER_MULTIPLY(wp_groups, SDState, 1, NULL, 0, wpgrps_size,
> +                sizeof(unsigned long)),

Isn't this trying to use wpgrps_size as the number of unsigned longs in
the bitmap, when it's actually the size of the bitmap in bits?

(Does this correctly work in migration between 32 and 64 bit systems
where 'unsigned long' is a different size? How about between a little
endian 32 bit system and a big endian 64 bit system? I don't know
enough about the vmstate macros to be confident here...)

-- PMM
Mitsyanko Igor - July 31, 2012, 6:18 p.m.
On 07/31/2012 06:56 PM, Peter Maydell wrote:
> On 27 July 2012 20:29, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
>> +        VMSTATE_BUFFER_MULTIPLY(wp_groups, SDState, 1, NULL, 0, wpgrps_size,
>> +                sizeof(unsigned long)),
>
> Isn't this trying to use wpgrps_size as the number of unsigned longs in
> the bitmap, when it's actually the size of the bitmap in bits?
>
> (Does this correctly work in migration between 32 and 64 bit systems
> where 'unsigned long' is a different size? How about between a little
> endian 32 bit system and a big endian 64 bit system? I don't know
> enough about the vmstate macros to be confident here...)
>
> -- PMM
>
>

You're right, bitmap_new() can allocated buffers of different size for 
the same number of bits but different sizeof(long) value. Maybe always 
align allocated buffer size to 8 byte? Endianess seems like even bigger 
issue.. Looks like we need to think of something else here.
Peter Maydell - Aug. 8, 2012, 3:56 p.m.
On 31 July 2012 19:18, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> On 07/31/2012 06:56 PM, Peter Maydell wrote:
>> On 27 July 2012 20:29, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
>>>
>>> +        VMSTATE_BUFFER_MULTIPLY(wp_groups, SDState, 1, NULL, 0,
>>> wpgrps_size,
>>> +                sizeof(unsigned long)),
>>
>>
>> Isn't this trying to use wpgrps_size as the number of unsigned longs in
>> the bitmap, when it's actually the size of the bitmap in bits?
>>
>> (Does this correctly work in migration between 32 and 64 bit systems
>> where 'unsigned long' is a different size? How about between a little
>> endian 32 bit system and a big endian 64 bit system? I don't know
>> enough about the vmstate macros to be confident here...)

> You're right, bitmap_new() can allocated buffers of different size for the
> same number of bits but different sizeof(long) value. Maybe always align
> allocated buffer size to 8 byte? Endianess seems like even bigger issue..
> Looks like we need to think of something else here.

Having talked with Juan on IRC, I think the right thing here is to add
support for transmitting bitmaps to vmstate, so we can say
   VMSTATE_BITMAP(wp_groups, SDState, wpgrps_size)
and the vmstate code takes care of marshalling the bitmap to a standard
wire format (probably 32 bit little endian) and back.
I'll have a go at a patch...

-- PMM

Patch

diff --git a/hw/sd.c b/hw/sd.c
index 20ebd8e..f8ab045 100644
--- a/hw/sd.c
+++ b/hw/sd.c
@@ -55,24 +55,28 @@  typedef enum {
     sd_illegal = -2,
 } sd_rsp_type_t;
 
+enum {
+    sd_inactive,
+    sd_card_identification_mode,
+    sd_data_transfer_mode,
+};
+
+enum {
+    sd_inactive_state = -1,
+    sd_idle_state = 0,
+    sd_ready_state,
+    sd_identification_state,
+    sd_standby_state,
+    sd_transfer_state,
+    sd_sendingdata_state,
+    sd_receivingdata_state,
+    sd_programming_state,
+    sd_disconnect_state,
+};
+
 struct SDState {
-    enum {
-        sd_inactive,
-        sd_card_identification_mode,
-        sd_data_transfer_mode,
-    } mode;
-    enum {
-        sd_inactive_state = -1,
-        sd_idle_state = 0,
-        sd_ready_state,
-        sd_identification_state,
-        sd_standby_state,
-        sd_transfer_state,
-        sd_sendingdata_state,
-        sd_receivingdata_state,
-        sd_programming_state,
-        sd_disconnect_state,
-    } state;
+    uint32_t mode;
+    int32_t state;
     uint32_t ocr;
     uint8_t scr[8];
     uint8_t cid[16];
@@ -83,21 +87,22 @@  struct SDState {
     uint32_t vhs;
     bool wp_switch;
     unsigned long *wp_groups;
+    uint32_t wpgrps_size;
     uint64_t size;
-    int blk_len;
+    uint32_t blk_len;
     uint32_t erase_start;
     uint32_t erase_end;
     uint8_t pwd[16];
-    int pwd_len;
-    int function_group[6];
+    uint32_t pwd_len;
+    uint8_t function_group[6];
 
     bool spi;
-    int current_cmd;
+    uint8_t current_cmd;
     /* True if we will handle the next command as an ACMD. Note that this does
      * *not* track the APP_CMD status bit!
      */
     bool expecting_acmd;
-    int blk_written;
+    uint32_t blk_written;
     uint64_t data_start;
     uint32_t data_offset;
     uint8_t data[512];
@@ -421,8 +426,9 @@  static void sd_reset(SDState *sd, BlockDriverState *bdrv)
     if (sd->wp_groups)
         g_free(sd->wp_groups);
     sd->wp_switch = bdrv ? bdrv_is_read_only(bdrv) : false;
+    sd->wpgrps_size = sect;
     sd->wp_groups = bitmap_new(sect);
-    memset(sd->function_group, 0, sizeof(int) * 6);
+    memset(sd->function_group, 0, sizeof(sd->function_group));
     sd->erase_start = 0;
     sd->erase_end = 0;
     sd->size = size;
@@ -446,6 +452,39 @@  static const BlockDevOps sd_block_ops = {
     .change_media_cb = sd_cardchange,
 };
 
+static const VMStateDescription sd_vmstate = {
+    .name = "sd-card",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(mode, SDState),
+        VMSTATE_INT32(state, SDState),
+        VMSTATE_UINT8_ARRAY(cid, SDState, 16),
+        VMSTATE_UINT8_ARRAY(csd, SDState, 16),
+        VMSTATE_UINT16(rca, SDState),
+        VMSTATE_UINT32(card_status, SDState),
+        VMSTATE_PARTIAL_BUFFER(sd_status, SDState, 1),
+        VMSTATE_UINT32(vhs, SDState),
+        VMSTATE_BUFFER_MULTIPLY(wp_groups, SDState, 1, NULL, 0, wpgrps_size,
+                sizeof(unsigned long)),
+        VMSTATE_UINT32(blk_len, SDState),
+        VMSTATE_UINT32(erase_start, SDState),
+        VMSTATE_UINT32(erase_end, SDState),
+        VMSTATE_UINT8_ARRAY(pwd, SDState, 16),
+        VMSTATE_UINT32(pwd_len, SDState),
+        VMSTATE_UINT8_ARRAY(function_group, SDState, 6),
+        VMSTATE_UINT8(current_cmd, SDState),
+        VMSTATE_BOOL(expecting_acmd, SDState),
+        VMSTATE_UINT32(blk_written, SDState),
+        VMSTATE_UINT64(data_start, SDState),
+        VMSTATE_UINT32(data_offset, SDState),
+        VMSTATE_UINT8_ARRAY(data, SDState, 512),
+        VMSTATE_BUFFER_UNSAFE(buf, SDState, 1, 512),
+        VMSTATE_BOOL(enable, SDState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 /* 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
    board to ensure that ssi transfers only occur when the chip select
@@ -463,6 +502,7 @@  SDState *sd_init(BlockDriverState *bs, bool is_spi)
         bdrv_attach_dev_nofail(sd->bdrv, sd);
         bdrv_set_dev_ops(sd->bdrv, &sd_block_ops, sd);
     }
+    vmstate_register(NULL, -1, &sd_vmstate, sd);
     return sd;
 }
 
@@ -569,7 +609,7 @@  static void sd_lock_command(SDState *sd)
             sd->card_status |= LOCK_UNLOCK_FAILED;
             return;
         }
-        bitmap_zero(sd->wp_groups, sd_addr_to_wpnum(sd->size) + 1);
+        bitmap_zero(sd->wp_groups, sd->wpgrps_size);
         sd->csd[14] &= ~0x10;
         sd->card_status &= ~CARD_IS_LOCKED;
         sd->pwd_len = 0;