[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 04/14] onenand: Switch to byte-based block ac
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v4 04/14] onenand: Switch to byte-based block access |
Date: |
Mon, 2 May 2016 15:16:18 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
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 <address@hidden>
>>
>> ---
>> 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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature