From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=33964 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PNk4r-0005NI-SC for qemu-devel@nongnu.org; Wed, 01 Dec 2010 05:38:23 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PNk4q-0001T9-Jh for qemu-devel@nongnu.org; Wed, 01 Dec 2010 05:38:21 -0500 Received: from mail-ww0-f41.google.com ([74.125.82.41]:36223) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PNk4q-0001Sv-A0 for qemu-devel@nongnu.org; Wed, 01 Dec 2010 05:38:20 -0500 Received: by wwi18 with SMTP id 18so846764wwi.4 for ; Wed, 01 Dec 2010 02:38:19 -0800 (PST) MIME-Version: 1.0 In-Reply-To: References: Date: Wed, 1 Dec 2010 10:38:19 +0000 Message-ID: Subject: Re: [Qemu-devel] [PATCH] Add basic read, write and create support for AMD SimNow HDD images. From: Stefan Hajnoczi Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?ISO-8859-1?Q?Fran=E7ois_Revol?= Cc: qemu-devel@nongnu.org On Sun, Nov 28, 2010 at 7:08 PM, Fran=E7ois Revol wrote: > From b0602bc2b02dcd7b15f0f9a143f850defd767509 Mon Sep 17 00:00:00 2001 > From: =3D?UTF-8?q?Fran=3DC3=3DA7ois=3D20Revol?=3D > Date: Sun, 28 Nov 2010 20:01:03 +0100 > Subject: [PATCH] Add basic read, write and create support for AMD SimNow = HDD images. > MIME-Version: 1.0 > Content-Type: text/plain; charset=3DUTF-8 > Content-Transfer-Encoding: 8bit > > > Signed-off-by: Fran=E7ois Revol > --- > =A0Makefile.objs | =A0 =A02 +- > =A0block/hdd.c =A0 | =A0354 +++++++++++++++++++++++++++++++++++++++++++++= ++++++++++++ > =A02 files changed, 355 insertions(+), 1 deletions(-) > =A0create mode 100644 block/hdd.c This block driver does not implement the asynchronous APIs (bdrv_aio_writev, bdrv_aio_readv, bdrv_aio_flush) which are necessary for running a VM properly. Some block drivers are currently written without async support and that limits them to being used as qemu-img formats. It's a bad idea to run a VM with these block drivers because I/O will block the VM from making progress (it is synchronous). Anthony mentioned this on IRC and you explained that this requirement isn't obvious from the QEMU source. Luckily the changes needed are small so it's definitely worth making SimNow HDD images async. > +typedef struct BDRVHddState { > + =A0 =A0uint8_t identify_data[SECTOR_SIZE]; Perhaps identify_data[] should be uint16_t since it gets casted on every us= e. > +static void padstr(char *str, const char *src, int len) > +{ > + =A0 =A0int i, v; > + =A0 =A0for(i =3D 0; i < len; i++) { > + =A0 =A0 =A0 =A0if (*src) > + =A0 =A0 =A0 =A0 =A0 =A0v =3D *src++; > + =A0 =A0 =A0 =A0else > + =A0 =A0 =A0 =A0 =A0 =A0v =3D ' '; > + =A0 =A0 =A0 =A0str[i^1] =3D v; > + =A0 =A0} > +} This function is confusing, it uses int v instead of char. The name padstr() doesn't hint that it also byteswaps the string. QEMU coding style uses {} even for one-line if statement bodies (Section 4 in the CODING_STYLE file). > +static int hdd_probe(const uint8_t *buf, int buf_size, const char *filen= ame) > +{ > + =A0 =A0int name_len; > + =A0 =A0uint16_t *p =3D (uint16_t *)buf; > + =A0 =A0int64_t nb_sectors; > + =A0 =A0uint32_t nb_sectors_clipped; > + =A0 =A0int result =3D 0; > + =A0 =A0int i; > + > + =A0 =A0if (buf_size < SECTOR_SIZE) { > + =A0 =A0 =A0 =A0/* Header too small, no VDI. */ > + =A0 =A0 =A0 =A0return 0; > + =A0 =A0} > + > + =A0 =A0/* best effort sanity check */ > + =A0 =A0/* TODO: check more (CHS size...) */ > + > + =A0 =A0/* serial number */ > + =A0 =A0for (i =3D 10 * 2; i < 10 * 2 + 20; i++) { > + =A0 =A0 =A0 =A0if (!isvalid_ide_chr(buf[i])) { > + =A0 =A0 =A0 =A0 =A0 =A0return 0; > + =A0 =A0 =A0 =A0} > + =A0 =A0} > + =A0 =A0result +=3D 20; > + > + =A0 =A0/* firmware version */ > + =A0 =A0for (i =3D 23 * 2; i < 23 * 2 + 8; i++) { > + =A0 =A0 =A0 =A0if (!isvalid_ide_chr(buf[i])) { > + =A0 =A0 =A0 =A0 =A0 =A0return 0; > + =A0 =A0 =A0 =A0} > + =A0 =A0} > + =A0 =A0result +=3D 8; > + > + =A0 =A0/* model */ > + =A0 =A0for (i =3D 27 * 2; i < 27 * 2 + 40; i++) { > + =A0 =A0 =A0 =A0if (!isvalid_ide_chr(buf[i])) { > + =A0 =A0 =A0 =A0 =A0 =A0return 0; > + =A0 =A0 =A0 =A0} > + =A0 =A0} > + =A0 =A0result +=3D 40; > + > + =A0 =A0nb_sectors =3D le16_to_cpu(p[100]); > + =A0 =A0nb_sectors |=3D (uint64_t)le16_to_cpu(p[101]) << 16; > + =A0 =A0nb_sectors |=3D (uint64_t)le16_to_cpu(p[102]) << 32; > + =A0 =A0nb_sectors |=3D (uint64_t)le16_to_cpu(p[103]) << 48; > + > + =A0 =A0nb_sectors_clipped =3D le16_to_cpu(p[60]) | (le16_to_cpu(p[61]) = << 16); > + > + =A0 =A0if (nb_sectors < 1 || ((uint32_t)nb_sectors) !=3D nb_sectors_cli= pped) { > + =A0 =A0 =A0 =A0return 0; > + =A0 =A0} > + =A0 =A0result +=3D 10; > + > + =A0 =A0if (filename !=3D NULL) { > + =A0 =A0 =A0 =A0name_len =3D strlen(filename); > + =A0 =A0 =A0 =A0if (name_len > 4 && !strcmp(filename + name_len - 4, ".h= dd")) > + =A0 =A0 =A0 =A0 =A0 =A0result +=3D 20; > + =A0 =A0} > + > + =A0 =A0return result; > +} HDD has no magic by which to identify valid files. We need to avoid false positives because existing image files could be corrupted or VMs at least made unbootable. Although using filename extensions to test for formats is lame, in this case I don't think we have another choice. The result variable calculations are not needed. Either we reject the file and return 0, or we end up having added the same constants (result =3D 20 + 8 + 40 + 10 + (endswith(filename, ".hdd") ? 20 : 0)). > + =A0 =A0int sectors; > + =A0 =A0int cylinders; > + =A0 =A0int heads; Unused variables? > + =A0 =A0if (bdrv_read(bs->file, 0, s->identify_data, 1) < 0) { > + =A0 =A0 =A0 =A0goto fail; > + =A0 =A0} We're assuming that BDRV_SECTOR_SIZE =3D=3D SECTOR_SIZE =3D=3D 512 througho= ut the code. It would be safer to explicitly calculate against BDRV_SECTOR_SIZE. It would be clearer to rename SECTOR_SIZE to ATA_IDENTIFY_SIZE. > + =A0 =A0if (hdd_probe(s->identify_data, SECTOR_SIZE, NULL) =3D=3D 0) { > + =A0 =A0 =A0 =A0goto fail; > + =A0 =A0} [...] > + fail: > + =A0 =A0return -1; Please don't throw away specific error values. > + =A0 =A0/* hints */ > + =A0 =A0/* > + =A0 =A0bs->cyls =3D le16_to_cpu(p[54]); > + =A0 =A0bs->heads =3D le16_to_cpu(p[55]); > + =A0 =A0bs->secs =3D le16_to_cpu(p[56]); > + =A0 =A0*/ Deadcode. > +static int hdd_read(BlockDriverState *bs, int64_t sector_num, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0uint8_t *buf, int nb_sectors) > +{ > + =A0 =A0int ret; > + =A0 =A0if (bdrv_read(bs->file, sector_num + DATA_START, buf, nb_sectors= ) < 0) { > + =A0 =A0 =A0 =A0return -1; > + =A0 =A0} > + =A0 =A0return 0; > +} Replace with async version: static int hdd_aio_readv(BlockDriverState *bs, int64_t sector_num, QEMUIOVector *qiov, int nb_sectors, BlockDriverCompletionFunc *cb, void *opaque) { return bdrv_aio_readv(bs->file, sector_num + DATA_START, qiov, nb_sectors, cb, opaque); } > +static int hdd_write(BlockDriverState *bs, int64_t sector_num, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0const uint8_t *buf, int nb_secto= rs) > +{ > + =A0 =A0int ret; > + > + =A0 =A0if (sector_num > bs->total_sectors) { > + =A0 =A0 =A0 =A0fprintf(stderr, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"(HDD) Wrong offset: sector_num=3D0x%" P= RIx64 > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0" total_sectors=3D0x%" PRIx64 "\n", > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0sector_num, bs->total_sectors); > + =A0 =A0 =A0 =A0return -1; > + =A0 =A0} Not needed since block.c already checks request range. > + =A0 =A0/* TODO: check if already allocated, else truncate() */ > + =A0 =A0if (bdrv_write(bs->file, sector_num + DATA_START, buf, nb_sector= s) < 0) { > + =A0 =A0 =A0 =A0return -1; > + =A0 =A0} > + =A0 =A0return 0; > +} Should also be replaced with async version. bdrv_aio_flush() is trivial too. It is necessary for running VMs since they may rely on flush working for data integrity when a mode other than -drive ...,cache=3Dwritethrough is selected. > + =A0 =A0fd =3D open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | = O_LARGEFILE, > + =A0 =A0 =A0 =A0 =A0 =A0 =A00644); > + =A0 =A0if (fd < 0) { > + =A0 =A0 =A0 =A0return -errno; > + =A0 =A0} Please use bdrv_create_file(), bdrv_file_open(), and bdrv_pwrite() instead of POSIX file I/O. See block/qcow2.c:qcow_create2 as an example. > + =A0 =A0/* TODO: specs says it can grow, so no need to always do this */ > + =A0 =A0if (static_image) { > + =A0 =A0 =A0 =A0if (ftruncate(fd, sizeof(header) + blocks * SECTOR_SIZE)= ) { > + =A0 =A0 =A0 =A0 =A0 =A0result =3D -errno; > + =A0 =A0 =A0 =A0} > + =A0 =A0} Is there an issue with leaving ftruncate() out? I don't see a reason to truncate. If we want to preallocate then ftruncate() isn't appropriate anyway. > +static void hdd_close(BlockDriverState *bs) > +{ > + =A0 =A0BDRVHddState *s =3D bs->opaque; Unused variable. Stefan