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

[Mitsyanko Igor]

On 12/26/2011 06:58 PM, Peter Maydell wrote:
> 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.

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: address@hidden