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

[Igor Mitsyanko]

On 03/06/2013 10:52 PM, Peter Maydell wrote:

On 7 March 2013 02:31, Michael Walle <address@hidden> wrote:

Sorry for digging out such an old thread :) but this patch introduced a memory
corruption, see below.

CC'ing Igor as the author of the patch...

Am Dienstag 30 Oktober 2012, 09:44:24 schrieb Peter Maydell:

From: Igor Mitsyanko <address@hidden>

This patch updates SD card model to support save/load of card's state.

+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_BITMAP(wp_groups, SDState, 0, wpgrps_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),

buf is dynamically allocated in the sd_init(), see also the SDState:

struct SDState {
[...]
    uint8_t *buf;

    bool enable;
};

Agreed, VMSTATE_BUFFER_UNSAFE() is for buffers that are inline
in the struct, not for buffers that the struct only points to.
I guess we want one of the VMSTATE_VARRAY_* types instead.

thanks
-- PMM

Oops, sorry. It needs VMS_POINTER flag apparently. Looks like hw/onenand.c has the same problem with it's "otp" variable?
I can think of two good ways to fix it:

1. Use VMSTATE_BUFFER_UNSAFE_INFO instead. Just pass a custom VMStateInfo to it where we will dereference address and get a proper data pointer.
hw/pci/pci.c does this, for example.

2. Introduce a new vmstate macro VMSTATE_BUFFER_POINTER_UNSAFE, which will be the same as VMSTATE_BUFFER_UNSAFE,
but with an extra VMS_POINTER flag. The best option in my opinion, it also could be reused for hw/onenand.c