[PATCH 1/4] (Refactored) provide vhd support to qemu

[PATCH 1/4] (Refactored) provide vhd support to qemu

[Ben Guthro]

[-- Attachment #1: Type: text/plain, Size: 271 bytes --]

[PATCH 1/4] (Refactored) provide vhd support to qemu
qemu-vhd-support.patch
Provides integration of vdisk library, and implementation of block-vhd
into qemu.
Signed-off-by: Boris Ostrovsky <bostrovsky@virtualiron.com>
Signed-off-by: Ben Guthro <bguthro@virtualiron.com>


[-- Attachment #2: qemu-vhd-support.patch --]
[-- Type: text/x-patch, Size: 7866 bytes --]

diff -r 005dd6b1cf8e tools/ioemu/Makefile.target
--- a/tools/ioemu/Makefile.target	Wed Jun 20 15:33:14 2007 +0100
+++ b/tools/ioemu/Makefile.target	Thu Jun 21 13:04:26 2007 -0400
@@ -18,6 +18,8 @@ CPPFLAGS=-I. -I.. -I$(TARGET_PATH) -I$(S
 CPPFLAGS=-I. -I.. -I$(TARGET_PATH) -I$(SRC_PATH)
 CPPFLAGS+= -I$(XEN_ROOT)/tools/libxc
 CPPFLAGS+= -I$(XEN_ROOT)/tools/xenstore
+CPPFLAGS+= -I$(XEN_ROOT)/tools/vdisk -I$(XEN_ROOT)/tools/blktap/drivers
+CPPFLAGS+= -I$(XEN_ROOT)/tools/libaio/src
 ifdef CONFIG_DARWIN_USER
 VPATH+=:$(SRC_PATH)/darwin-user
 CPPFLAGS+=-I$(SRC_PATH)/darwin-user -I$(SRC_PATH)/darwin-user/$(TARGET_ARCH)
@@ -198,6 +200,7 @@ LIBS+=-L../../libxc -lxenctrl -lxenguest
 LIBS+=-L../../libxc -lxenctrl -lxenguest
 LIBS+=-L../../xenstore -lxenstore
 LIBS+=-lpthread
+LIBS+=-L$(XEN_ROOT)/tools/vdisk -lvdisk -L$(XEN_ROOT)/tools/libaio/src -laio -ldl
 ifndef CONFIG_USER_ONLY
 LIBS+=-lz
 endif
@@ -345,6 +348,7 @@ VL_OBJS+=cutils.o
 VL_OBJS+=cutils.o
 VL_OBJS+=block.o block-raw.o
 VL_OBJS+=block-cow.o block-qcow.o aes.o block-vmdk.o block-cloop.o block-dmg.o block-bochs.o block-vpc.o block-vvfat.o block-qcow2.o
+VL_OBJS+=block-vhd.o
 ifdef CONFIG_WIN32
 VL_OBJS+=tap-win32.o
 endif
diff -r 005dd6b1cf8e tools/ioemu/block-vhd.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/ioemu/block-vhd.c	Thu Jun 21 13:04:31 2007 -0400
@@ -0,0 +1,186 @@
+// Copyright (c) 2003-2007, Virtual Iron Software, Inc.
+//
+// Portions have been modified by Virtual Iron Software, Inc.
+// (c) 2007. This file and the modifications can be redistributed and/or
+// modified under the terms and conditions of the GNU General Public
+// License, version 2.1 and not any later version of the GPL, as published
+// by the Free Software Foundation.
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <linux/stddef.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <errno.h>
+#include <time.h>
+#include <string.h>
+#include <dlfcn.h>
+
+#include "vl.h"
+#include "block_int.h"
+#include "exec-all.h"		// for FILE* logfile
+
+#ifdef LIST_HEAD
+#undef LIST_HEAD  // defined in audio/sys-queue.h (included via vl.h)
+#endif
+#include <vdisk.h>
+
+
+
+typedef struct BDRVVhdState {
+	vdisk_dev_t *vdisk;
+} BDRVVhdState;
+
+/*
+ * See whether the disk is in VHD format.
+ * XXX: We do it based on file siffix, which is silly.
+ * We should probably open it and try to read headers/footers. 
+ */
+static int qemu_vhd_probe(const uint8_t *buf, int buf_size, const char *filename)
+{
+	char *file_fmt;
+
+	// Find file suffix
+	file_fmt = strrchr(filename, '.');
+	if (file_fmt) {
+
+		file_fmt++; // Skip '.'
+
+		if (!strcmp(file_fmt, "vhd")) {
+			printf("Detected VHD file %s\n", filename);
+			return (100);
+		}
+	}
+	return 0;
+}
+
+static void qemu_vhd_close(BlockDriverState *bs)
+{
+    BDRVVhdState *s;
+
+    // We may be called more than once
+    // XXX: Or not?
+    if (bs == NULL)
+	    return;
+
+    s = bs->opaque;
+    if ((s == NULL) || (s->vdisk == NULL))
+	    return;
+
+    vdisk_fini(s->vdisk);
+    qemu_free(s->vdisk);
+    s->vdisk = NULL;
+}
+
+#if 1
+#define DEBUG_VHD_OPN_CLS_PRINT( formatCstr, args... ) fprintf( logfile, formatCstr, ##args ); fflush( logfile )
+#else
+#define DEBUG_VHD_OPN_CLS_PRINT( formatCstr, args... )
+#endif
+
+static int qemu_vhd_open(BlockDriverState *bs, const char *filename)
+{
+	BDRVVhdState *s = bs->opaque;
+	struct program_props props;
+	int ret = 0;
+
+    DEBUG_VHD_OPN_CLS_PRINT("vhd_open(%s)\n", filename);
+
+	s->vdisk = qemu_malloc(sizeof(struct vdisk_dev));
+	if (s->vdisk == NULL) {
+		printf("Can't allocate memory for vdisk\n");
+		return (-ENOMEM);
+	}
+
+	props.alloc_func = qemu_malloc;
+	props.free_func = qemu_free;
+	props.out_target = VDISK_OUT_STDERR;
+	ret = vdisk_init(s->vdisk, (char *)filename, &props, 1);
+	if (ret) {
+		printf("Can't initialize vdisk for %s\n", filename);
+		qemu_free(s->vdisk);
+		goto out;
+	}
+
+	/* qemu does not use aio */
+	s->vdisk->aio_fd = -1;
+	s->vdisk->use_aio = 0;
+
+	// VHD format limits geometry to roughly 136GB (0xffff cylinders,
+        // 0x10 heads and 0xff sectors per cylinder). We'll report "original
+        // size" (as specified by the header), not CHS product
+	// (Note that currently QEMU doesn't support disks ober 127GB anyway.
+	// This change is made so that QEMU and tapdisk return consistent
+	// disk size)
+	bs->total_sectors = s->vdisk->sz >> 9;
+	printf("Disk size is %ld sectors\n", bs->total_sectors);
+
+	bs->read_only = 0; // XXX: Really?
+
+out:
+	if (ret>0)
+		ret *= -1; // Need to return negative number on error;
+
+	return ret;	
+
+}
+
+static int qemu_vhd_read(BlockDriverState *bs, int64_t sector_num, 
+			 uint8_t *buf, int nb_sectors)
+{
+	BDRVVhdState *s = bs->opaque;
+	int res = 0;
+
+	res = vdisk_rw(s->vdisk, sector_num, buf,
+		       nb_sectors, VDISK_READ, NULL);
+
+	// QEMU is not very consistent in how it treats return codes from 
+	// bdrv_read()/bdrv_write(). Sometimes negative number is an error,
+	// sometimes it's a non-zero value. vdisk_rw() returns number of
+	// bytes that have been processed as syncIO
+	if (res >= 0)
+		res = 0;
+
+	return (res);
+}
+
+static int qemu_vhd_write(BlockDriverState *bs, int64_t sector_num, 
+			  const uint8_t *buf, int nb_sectors)
+{
+	BDRVVhdState *s = bs->opaque;
+	int res = 0;
+
+	res = vdisk_rw(s->vdisk, sector_num, (uint8_t *)buf, 
+		       nb_sectors, VDISK_WRITE, NULL);
+
+	// See comment in qemu_vhd_read()
+	if (res >= 0)
+		res = 0;
+
+	return (res);
+}
+
+static int qemu_vhd_create(const char *filename, int64_t total_size,
+                      const char *backing_file, int flags)
+{
+	printf("Creating VHD disks is not supported\n");
+
+	return ENOTSUP;
+}
+
+BlockDriver bdrv_vhd = {
+    "vhd",
+    sizeof(BDRVVhdState),
+    qemu_vhd_probe,
+    qemu_vhd_open,
+    qemu_vhd_read,
+    qemu_vhd_write,
+    qemu_vhd_close,
+    qemu_vhd_create,
+    NULL, /* flush */
+    NULL, /* is_allocated */
+    NULL, /* set_key */
+    NULL  /* make_empty */
+};
diff -r 005dd6b1cf8e tools/ioemu/block.c
--- a/tools/ioemu/block.c	Wed Jun 20 15:33:14 2007 +0100
+++ b/tools/ioemu/block.c	Thu Jun 21 13:04:08 2007 -0400
@@ -1246,6 +1246,7 @@ void bdrv_init(void)
     bdrv_register(&bdrv_vpc);
     bdrv_register(&bdrv_vvfat);
     bdrv_register(&bdrv_qcow2);
+    bdrv_register(&bdrv_vhd);
 }
 
 void *qemu_aio_get(BlockDriverState *bs, BlockDriverCompletionFunc *cb,
diff -r 005dd6b1cf8e tools/ioemu/vl.c
--- a/tools/ioemu/vl.c	Wed Jun 20 15:33:14 2007 +0100
+++ b/tools/ioemu/vl.c	Thu Jun 21 13:04:08 2007 -0400
@@ -7223,14 +7223,23 @@ int main(int argc, char **argv)
             case QEMU_OPTION_hdb:
             case QEMU_OPTION_hdc:
             case QEMU_OPTION_hdd:
-                {
-                    int hd_index;
-                    hd_index = popt->index - QEMU_OPTION_hda;
-                    hd_filename[hd_index] = optarg;
-                    if (hd_index == cdrom_index)
-                        cdrom_index = -1;
-                }
-                break;
+	    {
+		int hd_index;
+		char *fname;
+		
+		fname = strchr(optarg, ':');
+		if (fname == NULL)
+		    fname = optarg;
+		else
+		    fname++;
+		
+		hd_index = popt->index - QEMU_OPTION_hda;
+		hd_filename[hd_index] = fname;
+		if (hd_index == cdrom_index)
+		    cdrom_index = -1;
+	    }
+	    
+	    break;
 #endif /* !CONFIG_DM */
             case QEMU_OPTION_snapshot:
                 snapshot = 1;
diff -r 005dd6b1cf8e tools/ioemu/vl.h
--- a/tools/ioemu/vl.h	Wed Jun 20 15:33:14 2007 +0100
+++ b/tools/ioemu/vl.h	Thu Jun 21 13:04:08 2007 -0400
@@ -589,6 +589,7 @@ extern BlockDriver bdrv_vpc;
 extern BlockDriver bdrv_vpc;
 extern BlockDriver bdrv_vvfat;
 extern BlockDriver bdrv_qcow2;
+extern BlockDriver bdrv_vhd;
 
 typedef struct BlockDriverInfo {
     /* in bytes, 0 if irrelevant */


[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH 1/4] (Refactored) provide vhd support to qemu

[Andrew Warfield]

Hi Ben,

   Thanks very much for these patches -- it would be great to see VHD
support added to blktap.  I've taken a high-level pass over your
patches and have a few comments/suggestions:

  1. Your patches add a vdisk abstraction.  It's not clear that adding
another virtual disk interface (in addition to what is already
presented with tapdisk and used by the existing drivers) is a big win
-- especially since VHD is the only consumer of the new abstraction.
If the tapdisk interface falls short, can we evolve it to address
deficiencies rather than add another parallel interface?

  2. Having both qemu and tapdisk instances go directly at the disk
isn't ideal.  It would be much better to plumb qemu through tapdisk,
say through a dom0 block-attach, so that we implement drivers in a
single place,  cache image metadata in a single entity at runtime,
etc.

  3. It doesn't look like your VHD implementation does chained disks.
Am I missing something, or is this future work?


a.

On 6/21/07, Ben Guthro <bguthro@virtualiron.com> wrote:
> [PATCH 1/4] (Refactored) provide vhd support to qemu
> qemu-vhd-support.patch
> Provides integration of vdisk library, and implementation of block-vhd
> into qemu.
> Signed-off-by: Boris Ostrovsky <bostrovsky@virtualiron.com>
> Signed-off-by: Ben Guthro <bguthro@virtualiron.com>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>
>
>

Re: [PATCH 1/4] (Refactored) provide vhd support to qemu

[Steve Ofsthun]

Andrew Warfield wrote:
> Hi Ben,
> 
>   Thanks very much for these patches -- it would be great to see VHD
> support added to blktap.  I've taken a high-level pass over your
> patches and have a few comments/suggestions:
> 
>  1. Your patches add a vdisk abstraction.  It's not clear that adding
> another virtual disk interface (in addition to what is already
> presented with tapdisk and used by the existing drivers) is a big win
> -- especially since VHD is the only consumer of the new abstraction.
> If the tapdisk interface falls short, can we evolve it to address
> deficiencies rather than add another parallel interface?

The vdisk abstraction was mainly designed to allow qemu & tapdisk to share
the same format translation code.  Existing formats seemed to duplicate
code between the two consumers.

>  2. Having both qemu and tapdisk instances go directly at the disk
> isn't ideal.  It would be much better to plumb qemu through tapdisk,
> say through a dom0 block-attach, so that we implement drivers in a
> single place,  cache image metadata in a single entity at runtime,
> etc.

I'm confused here, are you saying that the current disk format decoders are
not duplicated in both qemu and tapdisk?  I didn't think upstream qemu even
knew about tapdisk.

In any case, I would think Qemu performance overhead is significant enough
without adding another hop through tapdisk.

>  3. It doesn't look like your VHD implementation does chained disks.
> Am I missing something, or is this future work?

Disk chaining (parent/child relationships) is currently handled by the
vdisk layer, not the VHD format translation directly.

Steve
-- 
Steve Ofsthun - Virtual Iron Software, Inc.