| Message ID | 20240628070216.92609-7-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/sd/sdcard: Add eMMC support | expand |
On 09:00 Fri 28 Jun , Philippe Mathieu-Daudé wrote: > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. > > > "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> > --- > RFC: Is it safe to reuse VMSTATE_UNUSED_V() (which happens > to be the same size)? > > Cc: Peter Xu <peterx@redhat.com> > Cc: Fabiano Rosas <farosas@suse.de> I'm not sure about this migration question. But IMHO you can simplify your implementation to avoid having to store and migrate this vendor_data array. After some research on this command, I came to the conclusion that it's used by manufacturers to return device health related vendor-specific data. (E.g., https://images-na.ssl-images-amazon.com/images/I/91tTtUMDM3L.pdf Section 1.6.1). So I guess you can simply discard writes and return 0s on reads (or "QEMU" in ASCII or... :)). > --- > hw/sd/sd.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/hw/sd/sd.c b/hw/sd/sd.c > index 464576751a..1f3eea6e84 100644 > --- a/hw/sd/sd.c > +++ b/hw/sd/sd.c > @@ -142,6 +142,8 @@ struct SDState { > uint64_t data_start; > uint32_t data_offset; > uint8_t data[512]; > + uint8_t vendor_data[512]; > + > qemu_irq readonly_cb; > qemu_irq inserted_cb; > QEMUTimer *ocr_power_timer; > @@ -656,6 +658,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; > @@ -771,7 +774,7 @@ static const VMStateDescription sd_vmstate = { > VMSTATE_UINT64(data_start, SDState), > 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() > }, > @@ -2029,9 +2032,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; > @@ -2165,12 +2167,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 Fri, Jun 28, 2024 at 09:00:42AM +0200, 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> > --- > RFC: Is it safe to reuse VMSTATE_UNUSED_V() (which happens > to be the same size)? This field became unused with: commit 12c125cba9c548929ccf4da2515e5b795c94afd9 Author: Eric Blake <eblake@redhat.com> Date: Fri May 6 10:26:39 2016 -0600 sd: Switch to byte-based block access which was in 2.6.1 / 2.7.0 Thus if someone is using a machine type that is 2.6 or older, I don't think it is safe to unconditionally reuse that field. My pending series deprecates everything upto 2.12, but we won't remove those machine types until 2 further release are past. You could gamble that SD card usage is niche enough that its highly unlikely someone will be using SD card at the same time as these ancient machine types. The safe thing would be a new field. > Cc: Peter Xu <peterx@redhat.com> > Cc: Fabiano Rosas <farosas@suse.de> > --- > hw/sd/sd.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) With regards, Daniel
diff --git a/hw/sd/sd.c b/hw/sd/sd.c index 464576751a..1f3eea6e84 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -142,6 +142,8 @@ struct SDState { uint64_t data_start; uint32_t data_offset; uint8_t data[512]; + uint8_t vendor_data[512]; + qemu_irq readonly_cb; qemu_irq inserted_cb; QEMUTimer *ocr_power_timer; @@ -656,6 +658,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; @@ -771,7 +774,7 @@ static const VMStateDescription sd_vmstate = { VMSTATE_UINT64(data_start, SDState), 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() }, @@ -2029,9 +2032,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; @@ -2165,12 +2167,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: