On 26 December 2011 10:03, Mitsyanko Igor <i.mitsya...@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.mitsya...@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. > 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.) > + 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 ? > sd->erase_start = 0; > sd->erase_end = 0; > sd->size = size; > sd->blk_len = 0x200; > sd->pwd_len = 0; > - sd->expecting_acmd = 0; > + sd->expecting_acmd = false; > } > > static void sd_cardchange(void *opaque, bool load) > @@ -451,7 +455,7 @@ SDState *sd_init(BlockDriverState *bs, int is_spi) > sd = (SDState *) g_malloc0(sizeof(SDState)); > sd->buf = qemu_blockalign(bs, 512); > sd->spi = is_spi; > - sd->enable = 1; > + sd->enable = true; > sd_reset(sd, bs); > if (sd->bdrv) { > bdrv_attach_dev_nofail(sd->bdrv, sd); > @@ -534,7 +538,7 @@ static void sd_function_switch(SDState *sd, uint32_t arg) > sd->data[66] = crc & 0xff; > } > > -static inline int sd_wp_addr(SDState *sd, uint32_t addr) > +static inline uint8_t sd_wp_addr(SDState *sd, uint32_t addr) > { > return sd->wp_groups[addr >> > (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT)]; > @@ -560,7 +564,7 @@ static void sd_lock_command(SDState *sd) > sd->card_status |= LOCK_UNLOCK_FAILED; > return; > } > - memset(sd->wp_groups, 0, sizeof(int) * (sd->size >> > + memset(sd->wp_groups, 0, (sd->size >> > (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT))); > sd->csd[14] &= ~0x10; > sd->card_status &= ~CARD_IS_LOCKED; > @@ -1125,7 +1129,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, > if (sd->rca != rca) > return sd_r0; > > - sd->expecting_acmd = 1; > + sd->expecting_acmd = true; > sd->card_status |= APP_CMD; > return sd_r1; > > @@ -1307,7 +1311,7 @@ int sd_do_command(SDState *sd, SDRequest *req, > if (sd->card_status & CARD_IS_LOCKED) { > if (!cmd_valid_while_locked(sd, req)) { > sd->card_status |= ILLEGAL_COMMAND; > - sd->expecting_acmd = 0; > + sd->expecting_acmd = false; > fprintf(stderr, "SD: Card is locked\n"); > rtype = sd_illegal; > goto send_response; > @@ -1318,7 +1322,7 @@ int sd_do_command(SDState *sd, SDRequest *req, > sd_set_mode(sd); > > if (sd->expecting_acmd) { > - sd->expecting_acmd = 0; > + sd->expecting_acmd = false; > rtype = sd_app_command(sd, *req); > } else { > rtype = sd_normal_command(sd, *req); > @@ -1706,5 +1710,44 @@ int sd_data_ready(SDState *sd) > > void sd_enable(SDState *sd, int enable) > { > - sd->enable = enable; > + sd->enable = enable ? true : false; This kind of thing is why I don't like bool :-) > +} > + > +static int sd_vmstate_get_wpgroups_size(void *opaque, int version_id) > +{ > + SDState *sd = (SDState *)opaque; > + return sd->size >> (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT); > } > + > +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_VBUFFER_CALCSIZE(wp_groups, SDState, > + 1, NULL, 0, sd_vmstate_get_wpgroups_size), > + 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() > + } > +}; > diff --git a/hw/sd.h b/hw/sd.h > index ac4b7c4..a2489c6 100644 > --- a/hw/sd.h > +++ b/hw/sd.h > @@ -66,6 +66,7 @@ typedef struct { > } SDRequest; > > typedef struct SDState SDState; > +extern const VMStateDescription sd_vmstate; > > SDState *sd_init(BlockDriverState *bs, int is_spi); > int sd_do_command(SDState *sd, SDRequest *req, > -- > 1.7.4.1 -- PMM