| Message ID | 20240702161031.59362-3-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/sd/sdcard: Cleanups before adding eMMC support | expand |
On 18:10 Tue 02 Jul , Philippe Mathieu-Daudé wrote: > "General command" (GEN_CMD, CMD56) is described as: > > GEN_CMD is the same as the single block read or write > commands (CMD24 or CMD17). The difference is that [...] > the data block is not a memory payload data but has a > vendor specific format and meaning. > > Thus this block must not be stored overwriting data block > on underlying storage drive. Keep it in a dedicated > 'vendor_data[]' array. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Tested-by: Cédric Le Goater <clg@redhat.com> > --- > v43: Do not re-use VMSTATE_UNUSED_V (danpb) > --- > hw/sd/sd.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/hw/sd/sd.c b/hw/sd/sd.c > index 808dc1cea6..418ccb14a4 100644 > --- a/hw/sd/sd.c > +++ b/hw/sd/sd.c > @@ -153,6 +153,8 @@ struct SDState { > uint32_t data_offset; > size_t data_size; > uint8_t data[512]; > + uint8_t vendor_data[512]; > + > qemu_irq readonly_cb; > qemu_irq inserted_cb; > QEMUTimer *ocr_power_timer; > @@ -719,6 +721,7 @@ static void sd_reset(DeviceState *dev) > sd->wp_switch = sd->blk ? !blk_is_writable(sd->blk) : false; > sd->wp_group_bits = sect; > sd->wp_group_bmap = bitmap_new(sd->wp_group_bits); > + memset(sd->vendor_data, 0xec, sizeof(sd->vendor_data)); > memset(sd->function_group, 0, sizeof(sd->function_group)); > sd->erase_start = INVALID_ADDRESS; > sd->erase_end = INVALID_ADDRESS; > @@ -835,6 +838,7 @@ static const VMStateDescription sd_vmstate = { > VMSTATE_UINT32(data_offset, SDState), > VMSTATE_UINT8_ARRAY(data, SDState, 512), > VMSTATE_UNUSED_V(1, 512), > + VMSTATE_UINT8_ARRAY(vendor_data, SDState, 512), Don't you need to bump the VMState version then? > VMSTATE_BOOL(enable, SDState), > VMSTATE_END_OF_LIST() > }, > @@ -2187,9 +2191,8 @@ void sd_write_byte(SDState *sd, uint8_t value) > break; > > case 56: /* CMD56: GEN_CMD */ > - sd->data[sd->data_offset ++] = value; > - if (sd->data_offset >= sd->blk_len) { > - APP_WRITE_BLOCK(sd->data_start, sd->data_offset); > + sd->vendor_data[sd->data_offset ++] = value; > + if (sd->data_offset >= sizeof(sd->vendor_data)) { > sd->state = sd_transfer_state; > } > break; > @@ -2261,12 +2264,11 @@ uint8_t sd_read_byte(SDState *sd) > break; > > case 56: /* CMD56: GEN_CMD */ > - if (sd->data_offset == 0) > - APP_READ_BLOCK(sd->data_start, sd->blk_len); > - ret = sd->data[sd->data_offset ++]; > + ret = sd->vendor_data[sd->data_offset ++]; > > - if (sd->data_offset >= sd->blk_len) > + if (sd->data_offset >= sizeof(sd->vendor_data)) { > sd->state = sd_transfer_state; > + } > break; > > default: > -- > 2.41.0 > --
On 3/7/24 09:39, Luc Michel wrote: > On 18:10 Tue 02 Jul , Philippe Mathieu-Daudé wrote: >> "General command" (GEN_CMD, CMD56) is described as: >> >> GEN_CMD is the same as the single block read or write >> commands (CMD24 or CMD17). The difference is that [...] >> the data block is not a memory payload data but has a >> vendor specific format and meaning. >> >> Thus this block must not be stored overwriting data block >> on underlying storage drive. Keep it in a dedicated >> 'vendor_data[]' array. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> Tested-by: Cédric Le Goater <clg@redhat.com> >> --- >> v43: Do not re-use VMSTATE_UNUSED_V (danpb) >> --- >> hw/sd/sd.c | 16 +++++++++------- >> 1 file changed, 9 insertions(+), 7 deletions(-) >> >> diff --git a/hw/sd/sd.c b/hw/sd/sd.c >> index 808dc1cea6..418ccb14a4 100644 >> --- a/hw/sd/sd.c >> +++ b/hw/sd/sd.c >> @@ -153,6 +153,8 @@ struct SDState { >> uint32_t data_offset; >> size_t data_size; >> uint8_t data[512]; >> + uint8_t vendor_data[512]; >> + >> qemu_irq readonly_cb; >> qemu_irq inserted_cb; >> QEMUTimer *ocr_power_timer; >> @@ -719,6 +721,7 @@ static void sd_reset(DeviceState *dev) >> sd->wp_switch = sd->blk ? !blk_is_writable(sd->blk) : false; >> sd->wp_group_bits = sect; >> sd->wp_group_bmap = bitmap_new(sd->wp_group_bits); >> + memset(sd->vendor_data, 0xec, sizeof(sd->vendor_data)); >> memset(sd->function_group, 0, sizeof(sd->function_group)); >> sd->erase_start = INVALID_ADDRESS; >> sd->erase_end = INVALID_ADDRESS; >> @@ -835,6 +838,7 @@ static const VMStateDescription sd_vmstate = { >> VMSTATE_UINT32(data_offset, SDState), >> VMSTATE_UINT8_ARRAY(data, SDState, 512), >> VMSTATE_UNUSED_V(1, 512), >> + VMSTATE_UINT8_ARRAY(vendor_data, SDState, 512), > > Don't you need to bump the VMState version then? Indeed. I'll add a subsection which is the recommended way: https://www.qemu.org/docs/master/devel/migration/main.html#subsections The most common structure change is adding new data, e.g. when adding a newer form of device, or adding that state that you previously forgot to migrate. This is best solved using a subsection. Thanks, Phil.
diff --git a/hw/sd/sd.c b/hw/sd/sd.c index 808dc1cea6..418ccb14a4 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -153,6 +153,8 @@ struct SDState { uint32_t data_offset; size_t data_size; uint8_t data[512]; + uint8_t vendor_data[512]; + qemu_irq readonly_cb; qemu_irq inserted_cb; QEMUTimer *ocr_power_timer; @@ -719,6 +721,7 @@ static void sd_reset(DeviceState *dev) sd->wp_switch = sd->blk ? !blk_is_writable(sd->blk) : false; sd->wp_group_bits = sect; sd->wp_group_bmap = bitmap_new(sd->wp_group_bits); + memset(sd->vendor_data, 0xec, sizeof(sd->vendor_data)); memset(sd->function_group, 0, sizeof(sd->function_group)); sd->erase_start = INVALID_ADDRESS; sd->erase_end = INVALID_ADDRESS; @@ -835,6 +838,7 @@ static const VMStateDescription sd_vmstate = { VMSTATE_UINT32(data_offset, SDState), VMSTATE_UINT8_ARRAY(data, SDState, 512), VMSTATE_UNUSED_V(1, 512), + VMSTATE_UINT8_ARRAY(vendor_data, SDState, 512), VMSTATE_BOOL(enable, SDState), VMSTATE_END_OF_LIST() }, @@ -2187,9 +2191,8 @@ void sd_write_byte(SDState *sd, uint8_t value) break; case 56: /* CMD56: GEN_CMD */ - sd->data[sd->data_offset ++] = value; - if (sd->data_offset >= sd->blk_len) { - APP_WRITE_BLOCK(sd->data_start, sd->data_offset); + sd->vendor_data[sd->data_offset ++] = value; + if (sd->data_offset >= sizeof(sd->vendor_data)) { sd->state = sd_transfer_state; } break; @@ -2261,12 +2264,11 @@ uint8_t sd_read_byte(SDState *sd) break; case 56: /* CMD56: GEN_CMD */ - if (sd->data_offset == 0) - APP_READ_BLOCK(sd->data_start, sd->blk_len); - ret = sd->data[sd->data_offset ++]; + ret = sd->vendor_data[sd->data_offset ++]; - if (sd->data_offset >= sd->blk_len) + if (sd->data_offset >= sizeof(sd->vendor_data)) { sd->state = sd_transfer_state; + } break; default: