From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38354) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1axLDH-0007IB-CF for qemu-devel@nongnu.org; Mon, 02 May 2016 17:17:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1axLD5-0001Xs-Fm for qemu-devel@nongnu.org; Mon, 02 May 2016 17:17:01 -0400 References: <1461960516-4717-1-git-send-email-eblake@redhat.com> <1461960516-4717-5-git-send-email-eblake@redhat.com> <20160502153502.GB4882@noname.redhat.com> From: Eric Blake Message-ID: <5727C3A2.9040307@redhat.com> Date: Mon, 2 May 2016 15:16:18 -0600 MIME-Version: 1.0 In-Reply-To: <20160502153502.GB4882@noname.redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="AOKeomc9OPNRt618sM9n7e8TbXbSIx9h5" Subject: Re: [Qemu-devel] [PATCH v4 04/14] onenand: Switch to byte-based block access List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, "open list:Block layer core" , Max Reitz This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --AOKeomc9OPNRt618sM9n7e8TbXbSIx9h5 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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 >> >> --- >> 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=3D= 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 =3D 0; >> >> if (secn > 0) { >> - uint32_t size =3D (uint32_t)secn * 512; >> + uint32_t size =3D (uint32_t)secn << BDRV_SECTOR_BITS; >> + int64_t offset =3D sec << BDRV_SECTOR_BITS; >=20 > I'm not completely happy with the types here. >=20 > 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 offse= t > 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. >=20 > 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 =3D (s->secs_cur + (sec >> 5)) << BDRV_SECTOR_= BITS; >=20 > 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. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --AOKeomc9OPNRt618sM9n7e8TbXbSIx9h5 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJXJ8OiAAoJEKeha0olJ0Nq6GsH/RErTHimA6jqAGg6Hdv/pu9G UGJ0NpGwjnBIZDHh0PFxAT1vzhLDupKRLbhKgOW6AHa2S+UW3oPEDqO9CE+VBmNk F0JzCt6dRxT/KoPCgPwyq7K5h/jK9GyR3VkVmNnkHmGMDyHO8eQCPaqDdNIIm5fL rn94Fm0geC5y4uh2qqTF+a5SqWV4AWXj5qFJ9CnqcMDOE5aemGwJ2flNZ2yLKAGb syQ5/MWK5LiLvTS6a4PQmncDx27DI2DXwEpSIaeOXQ+Jnr6tjd9UrUf+n1IAXiLW c7Y8zg0uFsewiKhnpIM0kR/oKmwYg558q0Yb4L1hKh1wu5/+tsE9ndQc0hnD8mc= =H4JB -----END PGP SIGNATURE----- --AOKeomc9OPNRt618sM9n7e8TbXbSIx9h5--