| Message ID | 1461960516-4717-5-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
On 05/02/2016 09:35 AM, Kevin Wolf wrote: > 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(-) >> >> @@ -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? Hmm. I guess sec can't be negative, but I didn't check whether sec can ever be greater than 0x7fffffff/BDRV_SECTOR_SIZE. Depending on that answer determines whether the shift here overflows - but you are right that if it CAN overflow 32 bits, then we MUST cast sec to a 64-bit type PRIOR to the shift, not just merely assign it to a 64-bit value; and if CAN'T overflow, then a 32-bit type is sufficient to hold the answer. You're also right that unsigned is nicer in general for sizes that shouldn't be negative. > > And if we decide for casting, there are more places in this patch where > an int is shifted. Good catch. I guess I have to audit things more closely before respinning the series. >> @@ -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. That's certainly true, no matter what type I ultimately pick.
On 05/02/2016 03:16 PM, Eric Blake wrote: >> 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? > > Hmm. I guess sec can't be negative, but I didn't check whether sec can > ever be greater than 0x7fffffff/BDRV_SECTOR_SIZE. Depending on that > answer determines whether the shift here overflows - but you are right > that if it CAN overflow 32 bits, then we MUST cast sec to a 64-bit type > PRIOR to the shift, not just merely assign it to a 64-bit value; and if > CAN'T overflow, then a 32-bit type is sufficient to hold the answer. > You're also right that unsigned is nicer in general for sizes that > shouldn't be negative. Okay, auditing wasn't as hard as I feared: static int onenand_initfn(SysBusDevice *sbd) ... uint32_t size = 1 << (24 + ((s->id.dev >> 4) & 7)); ... s->blocks = size >> BLOCK_SHIFT; s->secs = size >> 9; so the maximum sec should ever be is 0x80000000 >> 9. I'll stick with uint32_t (since that's what the init function used), and maybe add an assert that we aren't overflowing.
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; 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; + 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; if (s->blk_cur) { dp = g_malloc(512); if (!dp - || blk_read(s->blk_cur, s->secs_cur + (sec >> 5), dp, 1) < 0) { + || blk_pread(s->blk_cur, offset, dp, BDRV_SECTOR_SIZE) < 0) { result = 1; } else { dpp = dp + ((sec & 31) << 4); @@ -336,8 +341,8 @@ static inline int onenand_prog_spare(OneNANDState *s, int sec, int secn, dpp[i] &= sp[i]; } if (s->blk_cur) { - result = blk_write(s->blk_cur, s->secs_cur + (sec >> 5), - dp, 1) < 0; + result = blk_pwrite(s->blk_cur, offset, dp, + BDRV_SECTOR_SIZE, 0) < 0; } } g_free(dp); @@ -355,14 +360,17 @@ static inline int onenand_erase(OneNANDState *s, int sec, int num) for (; num > 0; num--, sec++) { if (s->blk_cur) { int erasesec = s->secs_cur + (sec >> 5); - if (blk_write(s->blk_cur, sec, blankbuf, 1) < 0) { + if (blk_pwrite(s->blk_cur, sec << BDRV_SECTOR_BITS, blankbuf, + BDRV_SECTOR_SIZE, 0) < 0) { goto fail; } - if (blk_read(s->blk_cur, erasesec, tmpbuf, 1) < 0) { + if (blk_pread(s->blk_cur, erasesec << BDRV_SECTOR_BITS, tmpbuf, + BDRV_SECTOR_SIZE) < 0) { goto fail; } memcpy(tmpbuf + ((sec & 31) << 4), blankbuf, 1 << 4); - if (blk_write(s->blk_cur, erasesec, tmpbuf, 1) < 0) { + if (blk_pwrite(s->blk_cur, erasesec << BDRV_SECTOR_BITS, tmpbuf, + BDRV_SECTOR_SIZE, 0) < 0) { goto fail; } } else {
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(-)