Re: [Qemu-devel] [PATCH v4 06/14] sd: Switch to byte-based block access

[Kevin Wolf]

Am 29.04.2016 um 22:08 hat Eric Blake geschrieben:
> Sector-based blk_write() should die; switch to byte-based
> blk_pwrite() instead.  Likewise for blk_read().
> 
> Greatly simplifies the code, now that we let the block layer
> take care of alignment and read-modify-write on our behalf :)
> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  hw/sd/sd.c | 46 +++-------------------------------------------
>  1 file changed, 3 insertions(+), 43 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index b66e5d2..3c2f2f1 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1577,57 +1577,17 @@ send_response:
> 
>  static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len)
>  {
> -    uint64_t end = addr + len;
> -
>      DPRINTF("sd_blk_read: addr = 0x%08llx, len = %d\n",
>              (unsigned long long) addr, len);
> -    if (!sd->blk || blk_read(sd->blk, addr >> 9, sd->buf, 1) < 0) {
> +    if (!sd->blk || blk_pread(sd->blk, addr, sd->data, len) < 0) {
>          fprintf(stderr, "sd_blk_read: read error on host side\n");
> -        return;
>      }
> -
> -    if (end > (addr & ~511) + 512) {
> -        memcpy(sd->data, sd->buf + (addr & 511), 512 - (addr & 511));
> -
> -        if (blk_read(sd->blk, end >> 9, sd->buf, 1) < 0) {
> -            fprintf(stderr, "sd_blk_read: read error on host side\n");
> -            return;
> -        }
> -        memcpy(sd->data + 512 - (addr & 511), sd->buf, end & 511);
> -    } else
> -        memcpy(sd->data, sd->buf + (addr & 511), len);
>  }

We can remove sd->buf from SDState, it should be unused now.

I'm not sure why a temporary buffer was ever included in the VMState,
but I guess we can make it a VMSTATE_UNUSED_BUFFER().

>  static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
>  {
> -    uint64_t end = addr + len;
> -
> -    if ((addr & 511) || len < 512)
> -        if (!sd->blk || blk_read(sd->blk, addr >> 9, sd->buf, 1) < 0) {
> -            fprintf(stderr, "sd_blk_write: read error on host side\n");
> -            return;
> -        }
> -
> -    if (end > (addr & ~511) + 512) {
> -        memcpy(sd->buf + (addr & 511), sd->data, 512 - (addr & 511));
> -        if (blk_write(sd->blk, addr >> 9, sd->buf, 1) < 0) {
> -            fprintf(stderr, "sd_blk_write: write error on host side\n");
> -            return;
> -        }
> -
> -        if (blk_read(sd->blk, end >> 9, sd->buf, 1) < 0) {
> -            fprintf(stderr, "sd_blk_write: read error on host side\n");
> -            return;
> -        }
> -        memcpy(sd->buf, sd->data + 512 - (addr & 511), end & 511);
> -        if (blk_write(sd->blk, end >> 9, sd->buf, 1) < 0) {
> -            fprintf(stderr, "sd_blk_write: write error on host side\n");
> -        }
> -    } else {
> -        memcpy(sd->buf + (addr & 511), sd->data, len);
> -        if (!sd->blk || blk_write(sd->blk, addr >> 9, sd->buf, 1) < 0) {
> -            fprintf(stderr, "sd_blk_write: write error on host side\n");
> -        }
> +    if (!sd->blk || blk_pwrite(sd->blk, addr, sd->buf, len, 0) < 0) {

I said "should" instead of "is" above because this line is buggy.
sd->data should probably be used instead.

Kevin