[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/3] hw/sd.c: add SD card save/load support
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH 2/3] hw/sd.c: add SD card save/load support |
Date: |
Mon, 26 Dec 2011 14:58:19 +0000 |
On 26 December 2011 10:03, Mitsyanko Igor <address@hidden> 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 <address@hidden>
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
[Qemu-devel] [PATCH 2/3] hw/sd.c: add SD card save/load support, Mitsyanko Igor, 2011/12/26
[Qemu-devel] [PATCH 3/3] hw/: Introduce spec. ver. 2.00 compliant SD host controller, Mitsyanko Igor, 2011/12/26
[Qemu-devel] [PATCH V2 0/3] Improve SD controllers emulation, Mitsyanko Igor, 2011/12/28