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 <ebl...@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
