Re: [Qemu-devel] [PATCH 2/3] hw/sd.c: add SD card save/load support

[Mitsyanko Igor <i.mitsyanko@samsung.com>]

On 12/26/2011 06:58 PM, Peter Maydell wrote:
> On 26 December 2011 10:03, Mitsyanko Igor<i.mitsyanko@samsung.com>  wrote:
>> We couldn't properly implement save/restore functionality of SD host controllers
>> states without SD card's state VMStateDescription implementation. This patch
>> updates SD card emulation to support save/load of card's state.
>>
>> Signed-off-by: Mitsyanko Igor<i.mitsyanko@samsung.com>
>
> Ah, cool. I'd had a quick go at adding save/restore to sd.c but
> ran aground on the wp_groups array issue.
>
> (cc'd Juan as vmstate/migration expert)
>
>> ---
>>   hw/milkymist-memcard.c |    2 +
>>   hw/sd.c                |  115 +++++++++++++++++++++++++++++++++---------------
>>   hw/sd.h                |    1 +
>>   3 files changed, 82 insertions(+), 36 deletions(-)
>>
>> diff --git a/hw/milkymist-memcard.c b/hw/milkymist-memcard.c
>> index 865a46c..6e8b0e4 100644
>> --- a/hw/milkymist-memcard.c
>> +++ b/hw/milkymist-memcard.c
>> @@ -266,6 +266,8 @@ static const VMStateDescription vmstate_milkymist_memcard = {
>>      .minimum_version_id = 1,
>>      .minimum_version_id_old = 1,
>>      .fields      = (VMStateField[]) {
>> +        VMSTATE_STRUCT_POINTER(card, MilkymistMemcardState, sd_vmstate,
>> +                SDState *),
>
> This doesn't seem like the right approach to me -- the migration state
> for the controller shouldn't have to include all the state for the card.
> Instead I think it would be better for sd.c to register a vmstate struct
> (by calling vmstate_register in sd_init) and thus handle its own
> saving/loading. Then if/when we make sd.c a proper qemu object we
> won't need to change the migration state for all the controllers.
>

Good point, thanks.

>>          VMSTATE_INT32(command_write_ptr, MilkymistMemcardState),
>>          VMSTATE_INT32(response_read_ptr, MilkymistMemcardState),
>>          VMSTATE_INT32(response_len, MilkymistMemcardState),
>> diff --git a/hw/sd.c b/hw/sd.c
>> index 07eb263..2b489d3 100644
>> --- a/hw/sd.c
>> +++ b/hw/sd.c
>> @@ -54,24 +54,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];
>> @@ -81,22 +85,22 @@ struct SDState {
>>      uint8_t sd_status[64];
>>      uint32_t vhs;
>>      int wp_switch;
>> -    int *wp_groups;
>> +    uint8_t *wp_groups;
>>      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];
>>
>> -    int spi;
>> -    int current_cmd;
>> +    uint8_t spi;
>> +    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!
>>       */
>> -    int expecting_acmd;
>> -    int blk_written;
>> +    bool expecting_acmd;
>
> Why have you changed expecting_acmd and enable to bool, but spi
> to a uint8_t and wp_groups[] to a uint8_t[] ? This isn't very
> consistent. (I'm vaguely against bool because you then end up
> making other changes to change '1' to 'true' and so on.)
>

I tried to preserve alignment and potentially avoid unnecessary memory 
consumption in arrays since bool size is implementation defined, 
otherwise I would have change everything to bool.
I'll convert everything to uint8_t next time, you're right, this'll make 
things simpler.

>> +    uint32_t blk_written;
>>      uint64_t data_start;
>>      uint32_t data_offset;
>>      uint8_t data[512];
>> @@ -105,7 +109,7 @@ struct SDState {
>>      BlockDriverState *bdrv;
>>      uint8_t *buf;
>>
>> -    int enable;
>> +    bool enable;
>>   };
>>
>>   static void sd_set_mode(SDState *sd)
>> @@ -415,14 +419,14 @@ 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) : 0;
>> -    sd->wp_groups = (int *) g_malloc0(sizeof(int) * sect);
>> -    memset(sd->function_group, 0, sizeof(int) * 6);
>> +    sd->wp_groups = (uint8_t *)g_malloc0(sect);
>> +    memset(sd->function_group, 0, 6);
>
> Since you're touching this line anyway, can we have
> sizeof(sd->function_group) rather than that hardcoded 6 ?
>
Sure.


-- 
Mitsyanko Igor
ASWG, Moscow R&D center, Samsung Electronics
email: i.mitsyanko@samsung.com