Re: [Qemu-devel] [PATCH] Add basic read, write and create support for AMD SimNow HDD images.
Am 23.11.2011 15:20, schrieb François Revol:
> Hi,
>
> Le 01/12/2010 11:38, Stefan Hajnoczi a écrit :
>> 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).
>
> I'm digging old stuff at the moment...
> It seems to be the coroutine calls recently introduced makes aio
> optional, does it ?
Yes. In fact, if you have co_* functions, aio_* will never be called
because the coroutine ones are always preferred.
> I added co versions of the calls in the code and it seems to work.
> It passes the qemu-iotests:
> Not run: 006 007 013 014 015 016 017 018 019 020 022 023 024 025 026 027 028
> Passed all 11 tests
>
> But before I submit the patch again I'll rather have it written correctly.
>
>>> +typedef struct BDRVHddState {
>>> +uint8_t identify_data[SECTOR_SIZE];
>>
>> Perhaps identify_data[] should be uint16_t since it gets casted on every use.
>
> I tried doing so but you end up adding many other casts everywhere
> and another #define to ...SIZE/sizeof(uint16_t) which doesn't look
> better though.
Yeah, I can see that there are many byte accesses as well. Probably best
to leave it as it is.
>
>>> +static void padstr(char *str, const char *src, int len)
>>> +{
>>> +int i, v;
>>> +for(i = 0; i < len; i++) {
>>> +if (*src)
>>> +v = *src++;
>>> +else
>>> +v = ' ';
>>> +str[i^1] = v;
>>> +}
>>> +}
>>
>> This function is confusing, it uses int v instead of char. The name
>> padstr() doesn't hint that it also byteswaps the string.
>
> Well it was copied verbatim from another file.
> I now added a comment mentioning it.
Should be a common helper function somewhere then. Duplicating code is
always bad.
>> QEMU coding style uses {} even for one-line if statement bodies
>> (Section 4 in the CODING_STYLE file).
>
> Well it was copied verbatim from another file. :P
While moving it to a common place, fix the code style. ;-)
>>> +static int hdd_probe(const uint8_t *buf, int buf_size, const char
>>> *filename)
>>
>> 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.
>
> I suppose so, I didn't look any further but apart from checking the
> geometry validity at several places in the header there is not much to
> check for.
We should make sure to return a very low value so that any other
matching format will take precedence.
Or we could consider hdd_probe() { return 0; } and require the user to
specify the format explicitly. Would be a bit nasty to use, though.
>
>>> +if (bdrv_read(bs->file, 0, s->identify_data, 1) < 0) {
>>> +goto fail;
>>> +}
>>
>> We're assuming that BDRV_SECTOR_SIZE == SECTOR_SIZE == 512 throughout
>> 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.
>
> Right, though the code would not work for SECTOR_SIZE != 512 since it's
> the size of the header, so having it defined to something else would
> make blocks unaligned anyway.
> I'll use other defines but also an #error if SECTOR_SIZE doesn't fit to
> make sure people don't try things without noticing.
I think QEMU_BUILD_BUG_ON() is what you're looking for.
>>> +/* TODO: specs says it can grow, so no need to always do this */
>>> +if (static_image) {
>>> +if (ftruncate(fd, sizeof(header) + blocks * SECTOR_SIZE)) {
>>> +result = -errno;
>>> +}
>>> +}
>>
>> 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.
>
> Right, it doesn't write zeroes on ext2 anyway.
> I'd have to test without it first.
Please use bdrv_* functions instead of POSIX ones to create the image.
Kevin
Re: [Qemu-devel] [PATCH] Add basic read, write and create support for AMD SimNow HDD images.
Hi,
Le 01/12/2010 11:38, Stefan Hajnoczi a écrit :
> 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).
I'm digging old stuff at the moment...
It seems to be the coroutine calls recently introduced makes aio
optional, does it ?
I added co versions of the calls in the code and it seems to work.
It passes the qemu-iotests:
Not run: 006 007 013 014 015 016 017 018 019 020 022 023 024 025 026 027 028
Passed all 11 tests
But before I submit the patch again I'll rather have it written correctly.
>> +typedef struct BDRVHddState {
>> +uint8_t identify_data[SECTOR_SIZE];
>
> Perhaps identify_data[] should be uint16_t since it gets casted on every use.
I tried doing so but you end up adding many other casts everywhere
and another #define to ...SIZE/sizeof(uint16_t) which doesn't look
better though.
>> +static void padstr(char *str, const char *src, int len)
>> +{
>> +int i, v;
>> +for(i = 0; i < len; i++) {
>> +if (*src)
>> +v = *src++;
>> +else
>> +v = ' ';
>> +str[i^1] = v;
>> +}
>> +}
>
> This function is confusing, it uses int v instead of char. The name
> padstr() doesn't hint that it also byteswaps the string.
Well it was copied verbatim from another file.
I now added a comment mentioning it.
> QEMU coding style uses {} even for one-line if statement bodies
> (Section 4 in the CODING_STYLE file).
Well it was copied verbatim from another file. :P
>> +static int hdd_probe(const uint8_t *buf, int buf_size, const char *filename)
>
> 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.
I suppose so, I didn't look any further but apart from checking the
geometry validity at several places in the header there is not much to
check for.
>> +if (bdrv_read(bs->file, 0, s->identify_data, 1) < 0) {
>> +goto fail;
>> +}
>
> We're assuming that BDRV_SECTOR_SIZE == SECTOR_SIZE == 512 throughout
> 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.
Right, though the code would not work for SECTOR_SIZE != 512 since it's
the size of the header, so having it defined to something else would
make blocks unaligned anyway.
I'll use other defines but also an #error if SECTOR_SIZE doesn't fit to
make sure people don't try things without noticing.
>> +/* TODO: specs says it can grow, so no need to always do this */
>> +if (static_image) {
>> +if (ftruncate(fd, sizeof(header) + blocks * SECTOR_SIZE)) {
>> +result = -errno;
>> +}
>> +}
>
> 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.
Right, it doesn't write zeroes on ext2 anyway.
I'd have to test without it first.
François.
Re: [Qemu-devel] [PATCH] Add basic read, write and create support for AMD SimNow HDD images.
On Sun, Nov 28, 2010 at 7:08 PM, François Revol wrote:
> From b0602bc2b02dcd7b15f0f9a143f850defd767509 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Fran=C3=A7ois=20Revol?=
> 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=UTF-8
> Content-Transfer-Encoding: 8bit
>
>
> Signed-off-by: François Revol
> ---
> Makefile.objs | 2 +-
> block/hdd.c | 354
> +
> 2 files changed, 355 insertions(+), 1 deletions(-)
> create 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 {
> + uint8_t identify_data[SECTOR_SIZE];
Perhaps identify_data[] should be uint16_t since it gets casted on every use.
> +static void padstr(char *str, const char *src, int len)
> +{
> + int i, v;
> + for(i = 0; i < len; i++) {
> + if (*src)
> + v = *src++;
> + else
> + v = ' ';
> + str[i^1] = v;
> + }
> +}
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 *filename)
> +{
> + int name_len;
> + uint16_t *p = (uint16_t *)buf;
> + int64_t nb_sectors;
> + uint32_t nb_sectors_clipped;
> + int result = 0;
> + int i;
> +
> + if (buf_size < SECTOR_SIZE) {
> + /* Header too small, no VDI. */
> + return 0;
> + }
> +
> + /* best effort sanity check */
> + /* TODO: check more (CHS size...) */
> +
> + /* serial number */
> + for (i = 10 * 2; i < 10 * 2 + 20; i++) {
> + if (!isvalid_ide_chr(buf[i])) {
> + return 0;
> + }
> + }
> + result += 20;
> +
> + /* firmware version */
> + for (i = 23 * 2; i < 23 * 2 + 8; i++) {
> + if (!isvalid_ide_chr(buf[i])) {
> + return 0;
> + }
> + }
> + result += 8;
> +
> + /* model */
> + for (i = 27 * 2; i < 27 * 2 + 40; i++) {
> + if (!isvalid_ide_chr(buf[i])) {
> + return 0;
> + }
> + }
> + result += 40;
> +
> + nb_sectors = le16_to_cpu(p[100]);
> + nb_sectors |= (uint64_t)le16_to_cpu(p[101]) << 16;
> + nb_sectors |= (uint64_t)le16_to_cpu(p[102]) << 32;
> + nb_sectors |= (uint64_t)le16_to_cpu(p[103]) << 48;
> +
> + nb_sectors_clipped = le16_to_cpu(p[60]) | (le16_to_cpu(p[61]) << 16);
> +
> + if (nb_sectors < 1 || ((uint32_t)nb_sectors) != nb_sectors_clipped) {
> + return 0;
> + }
> + result += 10;
> +
> + if (filename != NULL) {
> + name_len = strlen(filename);
> + if (name_len > 4 && !strcmp(filename + name_len - 4, ".hdd"))
> + result += 20;
> + }
> +
> + return 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 = 20 + 8 + 40 + 10 + (endswith(filename, ".hdd") ? 20 : 0)).
> + int sectors;
> + int cylinders;
> + int heads;
Unused variables?
> + if (bdrv_read(bs->file, 0, s->identify_data, 1) < 0) {
> + goto fail;
> + }
We're assuming that BDRV_SECTOR_SIZE == SECTOR_SIZE == 512 throughout
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.
> + if (hdd_probe(s->identify_data, SECTOR_SIZE, NULL) == 0) {
> + goto fail;
> + }
[...]
> + fail:
> + return -1;
Please don't throw away specific error values.
> + /* hints */
> + /*
> + bs->cyls = le16_to_cpu(p[54]);
> + bs->heads = le16_to_cpu(p[55]);
> + bs->secs = le16_to_cpu(p[56]);
> + */
Deadcode.
> +static int hdd_read(BlockDriverState *bs, int64_t sector_num,
> + uint8_t *buf, int nb_sectors)
> +{
> + int ret;
> + if (bdrv_read(bs->file, sector_num + DATA_START, buf, nb_sectors) < 0) {
> + return -1;
>
[Qemu-devel] [PATCH] Add basic read, write and create support for AMD SimNow HDD images.
$subj.
Someone asked about this format, wanting to try Haiku in SimNow, so I wrote
this.
I got a report of successfully booting a converted image in SimNow.
It doesn't yet support automatically growing the file, so we just preallocate
on create.
François.
From b0602bc2b02dcd7b15f0f9a143f850defd767509 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fran=C3=A7ois=20Revol?=
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=UTF-8
Content-Transfer-Encoding: 8bit
Signed-off-by: François Revol
---
Makefile.objs |2 +-
block/hdd.c | 354 +
2 files changed, 355 insertions(+), 1 deletions(-)
create mode 100644 block/hdd.c
diff --git a/Makefile.objs b/Makefile.objs
index 23b17ce..20e346d 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -20,7 +20,7 @@ block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
block-nested-y += raw.o cow.o qcow.o vdi.o vmdk.o cloop.o dmg.o bochs.o vpc.o
vvfat.o
block-nested-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o
-block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o
+block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o hdd.o
block-nested-$(CONFIG_WIN32) += raw-win32.o
block-nested-$(CONFIG_POSIX) += raw-posix.o
block-nested-$(CONFIG_CURL) += curl.o
diff --git a/block/hdd.c b/block/hdd.c
new file mode 100644
index 000..aed609e
--- /dev/null
+++ b/block/hdd.c
@@ -0,0 +1,354 @@
+/*
+ * Block driver for the AMD SimNow HDD disk image
+ *
+ * Copyright (c) 2010 François Revol
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ *
+ * Reference:
+ * http://developer.amd.com/Assets/SimNowUsersManual4.6.2.pdf page 181
+ *
+ * "the hard-disk image file contains a 512-byte header before the raw data.
+ * This 512-byte header is identical to the information provided by the drive
+ * in response to the ATA command "IDENTIFY". Following the 512-byte header
+ * is the data for each sector from the device."
+ */
+
+#include "qemu-common.h"
+#include "block_int.h"
+#include "module.h"
+
+/* Command line option for static images. */
+#define BLOCK_OPT_STATIC "static"
+
+#define SECTOR_SIZE 512
+#define DATA_START 1
+#define MAX_MULT_SECTORS 1
+
+typedef struct BDRVHddState {
+uint8_t identify_data[SECTOR_SIZE];
+} BDRVHddState;
+
+
+static void padstr(char *str, const char *src, int len)
+{
+int i, v;
+for(i = 0; i < len; i++) {
+if (*src)
+v = *src++;
+else
+v = ' ';
+str[i^1] = v;
+}
+}
+
+static void put_le16(uint16_t *p, unsigned int v)
+{
+*p = cpu_to_le16(v);
+}
+
+static int isvalid_ide_chr(char c)
+{
+/* XXX: other chars also maybe? */
+return (isalnum(c) || c == ' ' || c == '.');
+}
+
+static int hdd_probe(const uint8_t *buf, int buf_size, const char *filename)
+{
+int name_len;
+uint16_t *p = (uint16_t *)buf;
+int64_t nb_sectors;
+uint32_t nb_sectors_clipped;
+int result = 0;
+int i;
+
+if (buf_size < SECTOR_SIZE) {
+/* Header too small, no VDI. */
+return 0;
+}
+
+/* best effort sanity check */
+/* TODO: check more (CHS size...) */
+
+/* serial number */
+for (i = 10 * 2; i < 10 * 2 + 20; i++) {
+if (!isvalid_ide_chr(buf[i])) {
+return 0;
+}
+}
+result += 20;
+
+/* firmware version */
+for (i = 23 * 2; i < 23 * 2 + 8; i++) {
+if (!isvalid_ide_chr(buf[i])) {
+return 0;
+}
+}
+result += 8;
+
+/* model */
+for (i = 27 * 2; i < 27 * 2 + 40; i++) {
+if (!isvalid_ide_chr(buf[i])) {
+return 0;
+}
+}
+result += 40;
+
+nb_sectors = le16_to_cpu(p[100]);
+nb_sectors |= (uint64_t)le16_to_cpu(p[101]) << 16;
+nb_sectors |= (uint64_t)le16_to_cpu(p[102]) << 32;
