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

[Kevin Wolf <kwolf@redhat.com>]

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().
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> Not compile tested - I'm not sure what else I'd need in my environment
> to actually test this one.  I have:
> Fedora 23, dnf builddep qemu
> ./configure --enable-kvm --enable-system --disable-user --target-list=x86_64-softmmu,ppc64-softmmu --enable-debug
> ---
>  hw/block/onenand.c | 36 ++++++++++++++++++++++--------------
>  1 file changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/block/onenand.c b/hw/block/onenand.c
> index 883f4b1..3d19b0c 100644
> --- a/hw/block/onenand.c
> +++ b/hw/block/onenand.c
> @@ -224,7 +224,8 @@ static void onenand_reset(OneNANDState *s, int cold)
>          /* Lock the whole flash */
>          memset(s->blockwp, ONEN_LOCK_LOCKED, s->blocks);
> 
> -        if (s->blk_cur && blk_read(s->blk_cur, 0, s->boot[0], 8) < 0) {
> +        if (s->blk_cur && blk_pread(s->blk_cur, 0, s->boot[0],
> +                                    8 << BDRV_SECTOR_BITS) < 0) {
>              hw_error("%s: Loading the BootRAM failed.\n", __func__);
>          }
>      }
> @@ -241,7 +242,8 @@ static inline int onenand_load_main(OneNANDState *s, int sec, int secn,
>                  void *dest)
>  {
>      if (s->blk_cur) {
> -        return blk_read(s->blk_cur, sec, dest, secn) < 0;
> +        return blk_pread(s->blk_cur, sec << BDRV_SECTOR_BITS, dest,
> +                         secn << BDRV_SECTOR_BITS) < 0;
>      } else if (sec + secn > s->secs_cur) {
>          return 1;
>      }
> @@ -257,19 +259,20 @@ static inline int onenand_prog_main(OneNANDState *s, int sec, int secn,
>      int result = 0;
> 
>      if (secn > 0) {
> -        uint32_t size = (uint32_t)secn * 512;
> +        uint32_t size = (uint32_t)secn << BDRV_SECTOR_BITS;
> +        int64_t offset = sec << BDRV_SECTOR_BITS;

I'm not completely happy with the types here.

First of all, why signed? More importantly, though, sec is an int. I'm
not sure if we should cast it to uint64_t before shifting (I'm unsure
because this device seems to supports only sizes that fit in a uint32_t
anyway), but if we don't, wouldn't it make things more obvious if offset
were a uint32_t, too?

And if we decide for casting, there are more places in this patch where
an int is shifted.

>          const uint8_t *sp = (const uint8_t *)src;
>          uint8_t *dp = 0;
>          if (s->blk_cur) {
>              dp = g_malloc(size);
> -            if (!dp || blk_read(s->blk_cur, sec, dp, secn) < 0) {
> +            if (!dp || blk_pread(s->blk_cur, offset, dp, size) < 0) {
>                  result = 1;
>              }
>          } else {
>              if (sec + secn > s->secs_cur) {
>                  result = 1;
>              } else {
> -                dp = (uint8_t *)s->current + (sec << 9);
> +                dp = (uint8_t *)s->current + offset;
>              }
>          }
>          if (!result) {
> @@ -278,7 +281,7 @@ static inline int onenand_prog_main(OneNANDState *s, int sec, int secn,
>                  dp[i] &= sp[i];
>              }
>              if (s->blk_cur) {
> -                result = blk_write(s->blk_cur, sec, dp, secn) < 0;
> +                result = blk_pwrite(s->blk_cur, offset, dp, size, 0) < 0;
>              }
>          }
>          if (dp && s->blk_cur) {
> @@ -295,7 +298,8 @@ static inline int onenand_load_spare(OneNANDState *s, int sec, int secn,
>      uint8_t buf[512];
> 
>      if (s->blk_cur) {
> -        if (blk_read(s->blk_cur, s->secs_cur + (sec >> 5), buf, 1) < 0) {
> +        int32_t offset = (s->secs_cur + (sec >> 5)) << BDRV_SECTOR_BITS;

Here you have 32 bits (though still signed). In any case, some
consistency couldn't hurt.

> +        if (blk_pread(s->blk_cur, offset, buf, BDRV_SECTOR_SIZE) < 0) {
>              return 1;
>          }
>          memcpy(dest, buf + ((sec & 31) << 4), secn << 4);
> @@ -304,7 +308,7 @@ static inline int onenand_load_spare(OneNANDState *s, int sec, int secn,
>      } else {
>          memcpy(dest, s->current + (s->secs_cur << 9) + (sec << 4), secn << 4);
>      }
> - 
> +
>      return 0;
>  }
> 
> @@ -315,10 +319,11 @@ static inline int onenand_prog_spare(OneNANDState *s, int sec, int secn,
>      if (secn > 0) {
>          const uint8_t *sp = (const uint8_t *)src;
>          uint8_t *dp = 0, *dpp = 0;
> +        uint64_t offset = (s->secs_cur + (sec >> 5)) << BDRV_SECTOR_BITS;

Oh, nice, we have an unsigned one, too! :-)

Kevin