[Qemu-devel] [PATCH 0/2] Open disk images with O_DIRECT

[Qemu-devel] [PATCH 0/2] Open disk images with O_DIRECT

[Laurent Vivier]

These patches allow to open file using O_DIRECT and bypass the host I/O cache.

[PATCH 1/2] Add "directio" parameter to "-drive"

    Using "directio=on" with "-drive" will open the disk image 
    file using "O_DIRECT".

[PATCH 2/2] Direct IDE I/O

    This patch enhances the "-drive ,directio=on" mode with IDE drive emulation
    by removing the buffer used in the IDE emulation.

Laurent

[Qemu-devel] [PATCH 2/2] Direct IDE I/O

[Laurent Vivier]

This patch enhances the "-drive ,directio=on" mode with IDE drive emulation
by removing the buffer used in the IDE emulation.
---
 block.c     |   10 +++
 block.h     |    2 
 block_int.h |    1 
 cpu-all.h   |    1 
 exec.c      |   19 ++++++
 hw/ide.c    |  176 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 vl.c        |    1 
 7 files changed, 204 insertions(+), 6 deletions(-)

Index: qemu/block.c
===================================================================
--- qemu.orig/block.c	2007-11-27 10:49:56.000000000 +0100
+++ qemu/block.c	2007-11-27 10:49:57.000000000 +0100
@@ -752,6 +752,11 @@ void bdrv_set_translation_hint(BlockDriv
     bs->translation = translation;
 }
 
+void bdrv_set_directio_hint(BlockDriverState *bs, int directio)
+{
+    bs->directio = directio;
+}
+
 void bdrv_get_geometry_hint(BlockDriverState *bs,
                             int *pcyls, int *pheads, int *psecs)
 {
@@ -780,6 +785,11 @@ int bdrv_is_read_only(BlockDriverState *
     return bs->read_only;
 }
 
+int bdrv_is_directio(BlockDriverState *bs)
+{
+    return bs->directio;
+}
+
 /* XXX: no longer used */
 void bdrv_set_change_cb(BlockDriverState *bs,
                         void (*change_cb)(void *opaque), void *opaque)
Index: qemu/block.h
===================================================================
--- qemu.orig/block.h	2007-11-27 10:49:56.000000000 +0100
+++ qemu/block.h	2007-11-27 10:49:57.000000000 +0100
@@ -112,6 +112,7 @@ void bdrv_set_geometry_hint(BlockDriverS
                             int cyls, int heads, int secs);
 void bdrv_set_type_hint(BlockDriverState *bs, int type);
 void bdrv_set_translation_hint(BlockDriverState *bs, int translation);
+void bdrv_set_directio_hint(BlockDriverState *bs, int directio);
 void bdrv_get_geometry_hint(BlockDriverState *bs,
                             int *pcyls, int *pheads, int *psecs);
 int bdrv_get_type_hint(BlockDriverState *bs);
@@ -119,6 +120,7 @@ int bdrv_get_translation_hint(BlockDrive
 int bdrv_is_removable(BlockDriverState *bs);
 int bdrv_is_read_only(BlockDriverState *bs);
 int bdrv_is_inserted(BlockDriverState *bs);
+int bdrv_is_directio(BlockDriverState *bs);
 int bdrv_media_changed(BlockDriverState *bs);
 int bdrv_is_locked(BlockDriverState *bs);
 void bdrv_set_locked(BlockDriverState *bs, int locked);
Index: qemu/block_int.h
===================================================================
--- qemu.orig/block_int.h	2007-11-27 10:46:43.000000000 +0100
+++ qemu/block_int.h	2007-11-27 10:49:57.000000000 +0100
@@ -118,6 +118,7 @@ struct BlockDriverState {
        drivers. They are not used by the block driver */
     int cyls, heads, secs, translation;
     int type;
+    int directio;
     char device_name[32];
     BlockDriverState *next;
 };
Index: qemu/vl.c
===================================================================
--- qemu.orig/vl.c	2007-11-27 10:49:56.000000000 +0100
+++ qemu/vl.c	2007-11-27 10:49:57.000000000 +0100
@@ -5108,6 +5108,7 @@ static int drive_init(const char *str, i
         bdrv_flags |= BDRV_O_SNAPSHOT;
     if (directio)
         bdrv_flags |= BDRV_O_DIRECT;
+    bdrv_set_directio_hint(bdrv, directio);
     if (bdrv_open(bdrv, file, bdrv_flags) < 0 || qemu_key_check(bdrv, file)) {
         fprintf(stderr, "qemu: could not open disk image %s\n",
                         file);
Index: qemu/hw/ide.c
===================================================================
--- qemu.orig/hw/ide.c	2007-11-27 10:49:56.000000000 +0100
+++ qemu/hw/ide.c	2007-11-27 10:49:57.000000000 +0100
@@ -816,7 +816,7 @@ static int dma_buf_rw(BMDMAState *bm, in
 }
 
 /* XXX: handle errors */
-static void ide_read_dma_cb(void *opaque, int ret)
+static void ide_read_dma_cb_buffered(void *opaque, int ret)
 {
     BMDMAState *bm = opaque;
     IDEState *s = bm->ide_if;
@@ -856,7 +856,86 @@ static void ide_read_dma_cb(void *opaque
     printf("aio_read: sector_num=%lld n=%d\n", sector_num, n);
 #endif
     bm->aiocb = bdrv_aio_read(s->bs, sector_num, s->io_buffer, n,
-                              ide_read_dma_cb, bm);
+                              ide_read_dma_cb_buffered, bm);
+}
+
+static void ide_read_dma_cb_unbuffered(void *opaque, int ret)
+{
+    BMDMAState *bm = opaque;
+    IDEState *s = bm->ide_if;
+    int64_t sector_num;
+    int nsector;
+    int len;
+    uint8_t *phy_addr;
+
+    if (s->nsector == 0) {
+        s->status = READY_STAT | SEEK_STAT;
+       ide_set_irq(s);
+    eot:
+       bm->status &= ~BM_STATUS_DMAING;
+       bm->status |= BM_STATUS_INT;
+       bm->dma_cb = NULL;
+       bm->ide_if = NULL;
+       bm->aiocb = NULL;
+       return;
+    }
+
+    /* launch next transfer */
+
+    if (bm->cur_prd_len == 0) {
+        struct {
+            uint32_t addr;
+            uint32_t size;
+        } prd;
+
+        cpu_physical_memory_read(bm->cur_addr, (uint8_t *)&prd, 8);
+
+        bm->cur_addr += 8;
+        prd.addr = le32_to_cpu(prd.addr);
+        prd.size = le32_to_cpu(prd.size);
+        len = prd.size & 0xfffe;
+        if (len == 0)
+            len = 0x10000;
+        bm->cur_prd_addr = prd.addr;
+       while ((bm->cur_addr - bm->addr) < 4096) {
+           int tmp_len;
+           cpu_physical_memory_read(bm->cur_addr, (uint8_t *)&prd, 8);
+           prd.addr = le32_to_cpu(prd.addr);
+           prd.size = le32_to_cpu(prd.size);
+           if (bm->cur_prd_addr + len != prd.addr)
+               break;
+            tmp_len = prd.size & 0xfffe;
+            if (tmp_len == 0)
+                tmp_len = 0x10000;
+           len += tmp_len;
+            bm->cur_addr += 8;
+           if (prd.size & 0x80000000)
+               break;
+       }
+        bm->cur_prd_len = len;
+    }
+
+    phy_addr = cpu_physical_page_addr(bm->cur_prd_addr);
+    if (phy_addr == (uint8_t *)-1)
+       goto eot;
+
+    len = (s->nsector<<9);
+    if (len > bm->cur_prd_len)
+       len = bm->cur_prd_len;
+
+    nsector = (len>>9);
+    bm->cur_prd_addr += (nsector<<9);
+    bm->cur_prd_len -= (nsector<<9);
+
+    sector_num = ide_get_sector(s);
+    ide_set_sector(s, sector_num + nsector);
+    s->nsector-=nsector;
+
+#ifdef DEBUG_AIO
+    printf("aio_read: sector_num=%lld n=%d\n", (unsigned long long)sector_num, nsector);
+#endif
+    bm->aiocb = bdrv_aio_read(s->bs, sector_num, phy_addr, nsector,
+                             ide_read_dma_cb_unbuffered, bm);
 }
 
 static void ide_sector_read_dma(IDEState *s)
@@ -864,7 +943,10 @@ static void ide_sector_read_dma(IDEState
     s->status = READY_STAT | SEEK_STAT | DRQ_STAT | BUSY_STAT;
     s->io_buffer_index = 0;
     s->io_buffer_size = 0;
-    ide_dma_start(s, ide_read_dma_cb);
+    if (bdrv_is_directio(s->bs))
+        ide_dma_start(s, ide_read_dma_cb_unbuffered);
+    else
+        ide_dma_start(s, ide_read_dma_cb_buffered);
 }
 
 static void ide_sector_write_timer_cb(void *opaque)
@@ -917,7 +999,7 @@ static void ide_sector_write(IDEState *s
 }
 
 /* XXX: handle errors */
-static void ide_write_dma_cb(void *opaque, int ret)
+static void ide_write_dma_cb_buffered(void *opaque, int ret)
 {
     BMDMAState *bm = opaque;
     IDEState *s = bm->ide_if;
@@ -958,7 +1040,86 @@ static void ide_write_dma_cb(void *opaqu
     printf("aio_write: sector_num=%lld n=%d\n", sector_num, n);
 #endif
     bm->aiocb = bdrv_aio_write(s->bs, sector_num, s->io_buffer, n,
-                               ide_write_dma_cb, bm);
+                               ide_write_dma_cb_buffered, bm);
+}
+
+static void ide_write_dma_cb_unbuffered(void *opaque, int ret)
+{
+    BMDMAState *bm = opaque;
+    IDEState *s = bm->ide_if;
+    int64_t sector_num;
+    int nsector;
+    int len;
+    uint8_t *phy_addr;
+
+    if (s->nsector == 0) {
+        s->status = READY_STAT | SEEK_STAT;
+       ide_set_irq(s);
+    eot:
+       bm->status &= ~BM_STATUS_DMAING;
+       bm->status |= BM_STATUS_INT;
+       bm->dma_cb = NULL;
+       bm->ide_if = NULL;
+       bm->aiocb = NULL;
+       return;
+    }
+
+    /* launch next transfer */
+
+    if (bm->cur_prd_len == 0) {
+        struct {
+            uint32_t addr;
+            uint32_t size;
+        } prd;
+
+        cpu_physical_memory_read(bm->cur_addr, (uint8_t *)&prd, 8);
+
+        bm->cur_addr += 8;
+        prd.addr = le32_to_cpu(prd.addr);
+        prd.size = le32_to_cpu(prd.size);
+        len = prd.size & 0xfffe;
+        if (len == 0)
+            len = 0x10000;
+        bm->cur_prd_addr = prd.addr;
+       while ((bm->cur_addr - bm->addr) < 4096) {
+           int tmp_len;
+           cpu_physical_memory_read(bm->cur_addr, (uint8_t *)&prd, 8);
+           prd.addr = le32_to_cpu(prd.addr);
+           prd.size = le32_to_cpu(prd.size);
+           if (bm->cur_prd_addr + len != prd.addr)
+               break;
+            tmp_len = prd.size & 0xfffe;
+            if (tmp_len == 0)
+                tmp_len = 0x10000;
+           len += tmp_len;
+            bm->cur_addr += 8;
+           if (prd.size & 0x80000000)
+               break;
+       }
+        bm->cur_prd_len = len;
+    }
+
+    phy_addr = cpu_physical_page_addr(bm->cur_prd_addr);
+    if (phy_addr == (uint8_t *)-1)
+       goto eot;
+
+    len = (s->nsector<<9);
+    if (len > bm->cur_prd_len)
+       len = bm->cur_prd_len;
+
+    nsector = (len>>9);
+    bm->cur_prd_addr += (nsector<<9);
+    bm->cur_prd_len -= (nsector<<9);
+
+    sector_num = ide_get_sector(s);
+    ide_set_sector(s, sector_num + nsector);
+    s->nsector-=nsector;
+
+#ifdef DEBUG_AIO
+    printf("aio_write: sector_num=%lld n=%d\n", (unsigned long long)sector_num, nsector);
+#endif
+    bm->aiocb = bdrv_aio_write(s->bs, sector_num, phy_addr, nsector,
+                             ide_write_dma_cb_unbuffered, bm);
 }
 
 static void ide_sector_write_dma(IDEState *s)
@@ -966,7 +1127,10 @@ static void ide_sector_write_dma(IDEStat
     s->status = READY_STAT | SEEK_STAT | DRQ_STAT | BUSY_STAT;
     s->io_buffer_index = 0;
     s->io_buffer_size = 0;
-    ide_dma_start(s, ide_write_dma_cb);
+    if (bdrv_is_directio(s->bs))
+        ide_dma_start(s, ide_write_dma_cb_unbuffered);
+    else
+        ide_dma_start(s, ide_write_dma_cb_buffered);
 }
 
 static void ide_atapi_cmd_ok(IDEState *s)
Index: qemu/exec.c
===================================================================
--- qemu.orig/exec.c	2007-11-27 10:46:43.000000000 +0100
+++ qemu/exec.c	2007-11-27 10:49:57.000000000 +0100
@@ -2054,6 +2054,25 @@ void cpu_register_physical_memory(target
     }
 }
 
+uint8_t * cpu_physical_page_addr(target_phys_addr_t addr)
+{
+    target_phys_addr_t page;
+    unsigned long pd;
+    PhysPageDesc *p;
+    unsigned long addr1;
+
+    page = addr & TARGET_PAGE_MASK;
+    p = phys_page_find(page >> TARGET_PAGE_BITS);
+    if (!p)
+       return (uint8_t*)-1;
+
+    pd = p->phys_offset;
+
+    addr1 = (pd & TARGET_PAGE_MASK) + (addr & ~TARGET_PAGE_MASK);
+
+    return phys_ram_base + addr1;
+}
+
 /* XXX: temporary until new memory mapping API */
 uint32_t cpu_get_physical_page_desc(target_phys_addr_t addr)
 {
Index: qemu/cpu-all.h
===================================================================
--- qemu.orig/cpu-all.h	2007-11-27 10:46:58.000000000 +0100
+++ qemu/cpu-all.h	2007-11-27 10:49:57.000000000 +0100
@@ -837,6 +837,7 @@ int cpu_register_io_memory(int io_index,
 CPUWriteMemoryFunc **cpu_get_io_memory_write(int io_index);
 CPUReadMemoryFunc **cpu_get_io_memory_read(int io_index);
 
+extern uint8_t * cpu_physical_page_addr(target_phys_addr_t addr);
 void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
                             int len, int is_write);
 static inline void cpu_physical_memory_read(target_phys_addr_t addr,

[Qemu-devel] [PATCH 1/2] Add "directio" parameter to "-drive"

[Laurent Vivier]

This patch add a new parameter to "-drive"

Using "directio=on" with "-drive" will open the disk image file using
"O_DIRECT".

By default, "directio" is set to "off".

example:

"-drive file=my_disk.qcow2,directio=on"
---
 block-raw.c    |    4 ++++
 block.c        |    2 +-
 block.h        |    1 +
 hw/fdc.c       |    6 +++++-
 hw/ide.c       |    8 ++++++--
 hw/scsi-disk.c |    3 ++-
 hw/sd.c        |   10 +++++++++-
 vl.c           |   26 +++++++++++++++++++++++---
 8 files changed, 51 insertions(+), 9 deletions(-)

Index: qemu/block-raw.c
===================================================================
--- qemu.orig/block-raw.c	2007-11-25 18:10:07.000000000 +0100
+++ qemu/block-raw.c	2007-11-25 18:21:04.000000000 +0100
@@ -107,6 +107,8 @@ static int raw_open(BlockDriverState *bs
     }
     if (flags & BDRV_O_CREAT)
         open_flags |= O_CREAT | O_TRUNC;
+    if (flags & BDRV_O_DIRECT)
+        open_flags |= O_DIRECT;
 
     s->type = FTYPE_FILE;
 
@@ -660,6 +662,8 @@ static int hdev_open(BlockDriverState *b
         open_flags |= O_RDONLY;
         bs->read_only = 1;
     }
+    if (flags & BDRV_O_DIRECT)
+        open_flags |= O_DIRECT;
 
     s->type = FTYPE_FILE;
 #if defined(__linux__)
Index: qemu/block.h
===================================================================
--- qemu.orig/block.h	2007-11-25 18:10:07.000000000 +0100
+++ qemu/block.h	2007-11-25 18:21:04.000000000 +0100
@@ -44,6 +44,7 @@ typedef struct QEMUSnapshotInfo {
                                      use a disk image format on top of
                                      it (default for
                                      bdrv_file_open()) */
+#define BDRV_O_DIRECT      0x0020
 
 #ifndef QEMU_IMG
 void bdrv_info(void);
Index: qemu/vl.c
===================================================================
--- qemu.orig/vl.c	2007-11-25 18:10:07.000000000 +0100
+++ qemu/vl.c	2007-11-25 18:21:04.000000000 +0100
@@ -4847,8 +4847,11 @@ static int drive_init(const char *str, i
     BlockDriverState *bdrv;
     int max_devs;
     int index;
+    int directio;
+    int bdrv_flags;
     char *params[] = { "bus", "unit", "if", "index", "cyls", "heads",
-                       "secs", "trans", "media", "snapshot", "file", NULL };
+                       "secs", "trans", "media", "snapshot", "file",
+                       "directio", NULL };
 
     if (check_params(buf, sizeof(buf), params, str) < 0) {
          fprintf(stderr, "qemu: unknowm parameter '%s' in '%s'\n",
@@ -4862,6 +4865,7 @@ static int drive_init(const char *str, i
     unit_id = -1;
     translation = BIOS_ATA_TRANSLATION_AUTO;
     index = -1;
+    directio = 0;
 
     if (!strcmp(machine->name, "realview") ||
         !strcmp(machine->name, "SS-5") ||
@@ -5001,6 +5005,17 @@ static int drive_init(const char *str, i
 	}
     }
 
+    if (get_param_value(buf, sizeof(buf), "directio", str)) {
+        if (!strcmp(buf, "off"))
+            directio = 0;
+        else if (!strcmp(buf, "on"))
+            directio = 1;
+        else {
+	    fprintf(stderr, "qemu: invalid directio option\n");
+	    return -1;
+        }
+    }
+
     get_param_value(file, sizeof(file), "file", str);
 
     /* compute bus and unit according index */
@@ -5088,8 +5103,12 @@ static int drive_init(const char *str, i
     }
     if (!file[0])
         return 0;
-    if (bdrv_open(bdrv, file, snapshot ? BDRV_O_SNAPSHOT : 0) < 0 ||
-        qemu_key_check(bdrv, file)) {
+    bdrv_flags = 0;
+    if (snapshot)
+        bdrv_flags |= BDRV_O_SNAPSHOT;
+    if (directio)
+        bdrv_flags |= BDRV_O_DIRECT;
+    if (bdrv_open(bdrv, file, bdrv_flags) < 0 || qemu_key_check(bdrv, file)) {
         fprintf(stderr, "qemu: could not open disk image %s\n",
                         file);
         return -1;
@@ -7434,6 +7453,7 @@ static void help(int exitcode)
            "-cdrom file     use 'file' as IDE cdrom image (cdrom is ide1 master)\n"
 	   "-drive [file=file][,if=type][,bus=n][,unit=m][,media=d][index=i]\n"
            "       [,cyls=c,heads=h,secs=s[,trans=t]][snapshot=on|off]\n"
+           "       [directio=on|off]\n"
 	   "                use 'file' as a drive image\n"
            "-mtdblock file  use 'file' as on-board Flash memory image\n"
            "-sd file        use 'file' as SecureDigital card image\n"
Index: qemu/hw/ide.c
===================================================================
--- qemu.orig/hw/ide.c	2007-11-25 18:10:07.000000000 +0100
+++ qemu/hw/ide.c	2007-11-25 18:21:04.000000000 +0100
@@ -365,7 +365,7 @@ typedef struct IDEState {
     EndTransferFunc *end_transfer_func;
     uint8_t *data_ptr;
     uint8_t *data_end;
-    uint8_t io_buffer[MAX_MULT_SECTORS*512 + 4];
+    uint8_t *io_buffer;
     QEMUTimer *sector_write_timer; /* only used for win2k install hack */
     uint32_t irq_count; /* counts IRQs when using win2k install hack */
     /* CF-ATA extended error */
@@ -2372,11 +2372,14 @@ struct partition {
 static int guess_disk_lchs(IDEState *s,
                            int *pcylinders, int *pheads, int *psectors)
 {
-    uint8_t buf[512];
+    uint8_t *buf;
     int ret, i, heads, sectors, cylinders;
     struct partition *p;
     uint32_t nr_sects;
 
+    ret = posix_memalign((void**)&buf, 0x200, 512);
+    if (ret < 0)
+        return -1;
     ret = bdrv_read(s->bs, 0, buf, 1);
     if (ret < 0)
         return -1;
@@ -2420,6 +2423,7 @@ static void ide_init2(IDEState *ide_stat
 
     for(i = 0; i < 2; i++) {
         s = ide_state + i;
+        posix_memalign((void**)&s->io_buffer, 0x200, MAX_MULT_SECTORS*512 + 4);
         if (i == 0)
             s->bs = hd0;
         else
Index: qemu/block.c
===================================================================
--- qemu.orig/block.c	2007-11-25 18:10:07.000000000 +0100
+++ qemu/block.c	2007-11-25 18:21:04.000000000 +0100
@@ -380,7 +380,7 @@ int bdrv_open2(BlockDriverState *bs, con
     /* Note: for compatibility, we open disk image files as RDWR, and
        RDONLY as fallback */
     if (!(flags & BDRV_O_FILE))
-        open_flags = BDRV_O_RDWR;
+        open_flags = BDRV_O_RDWR | (flags & BDRV_O_DIRECT);
     else
         open_flags = flags & ~(BDRV_O_FILE | BDRV_O_SNAPSHOT);
     ret = drv->bdrv_open(bs, filename, open_flags);
Index: qemu/hw/scsi-disk.c
===================================================================
--- qemu.orig/hw/scsi-disk.c	2007-11-25 18:10:07.000000000 +0100
+++ qemu/hw/scsi-disk.c	2007-11-25 18:21:04.000000000 +0100
@@ -46,7 +46,7 @@ typedef struct SCSIRequest {
     int sector_count;
     /* The amounnt of data in the buffer.  */
     int buf_len;
-    uint8_t dma_buf[SCSI_DMA_BUF_SIZE];
+    uint8_t *dma_buf;
     BlockDriverAIOCB *aiocb;
     struct SCSIRequest *next;
 } SCSIRequest;
@@ -78,6 +78,7 @@ static SCSIRequest *scsi_new_request(SCS
         free_requests = r->next;
     } else {
         r = qemu_malloc(sizeof(SCSIRequest));
+        posix_memalign(&r->dma_buf, 0x200, SCSI_DMA_BUF_SIZE);
     }
     r->dev = s;
     r->tag = tag;
Index: qemu/hw/sd.c
===================================================================
--- qemu.orig/hw/sd.c	2007-11-25 18:10:07.000000000 +0100
+++ qemu/hw/sd.c	2007-11-25 18:21:04.000000000 +0100
@@ -1284,10 +1284,16 @@ int sd_do_command(SDState *sd, struct sd
 static void sd_blk_read(BlockDriverState *bdrv,
                 void *data, uint32_t addr, uint32_t len)
 {
-    uint8_t buf[512];
+    uint8_t *buf;
     uint32_t end = addr + len;
 
+    if (posix_memalign(&buf, 512, 512) < 0) {
+        printf("sd_blk_read: cannot allocate memory\n");
+        return;
+    }
+
     if (!bdrv || bdrv_read(bdrv, addr >> 9, buf, 1) == -1) {
+        free(buf);
         printf("sd_blk_read: read error on host side\n");
         return;
     }
@@ -1297,11 +1303,13 @@ static void sd_blk_read(BlockDriverState
 
         if (bdrv_read(bdrv, end >> 9, buf, 1) == -1) {
             printf("sd_blk_read: read error on host side\n");
+            free(buf);
             return;
         }
         memcpy(data + 512 - (addr & 511), buf, end & 511);
     } else
         memcpy(data, buf + (addr & 511), len);
+    free(buf);
 }
 
 static void sd_blk_write(BlockDriverState *bdrv,
Index: qemu/hw/fdc.c
===================================================================
--- qemu.orig/hw/fdc.c	2007-11-25 18:10:07.000000000 +0100
+++ qemu/hw/fdc.c	2007-11-25 18:21:04.000000000 +0100
@@ -382,7 +382,7 @@ struct fdctrl_t {
     uint8_t cur_drv;
     uint8_t bootsel;
     /* Command FIFO */
-    uint8_t fifo[FD_SECTOR_LEN];
+    uint8_t *fifo;
     uint32_t data_pos;
     uint32_t data_len;
     uint8_t data_state;
@@ -598,6 +598,10 @@ fdctrl_t *fdctrl_init (qemu_irq irq, int
     fdctrl = qemu_mallocz(sizeof(fdctrl_t));
     if (!fdctrl)
         return NULL;
+    if (posix_memalign((void**)&fdctrl->fifo, 512, FD_SECTOR_LEN) <0) {
+        free(fdctrl);
+        return NULL;
+    }
     fdctrl->result_timer = qemu_new_timer(vm_clock,
                                           fdctrl_result_timer, fdctrl);
 

Re: [Qemu-devel] [PATCH 1/2] Add "directio" parameter to "-drive"

[Samuel Thibault]

Hi,

Laurent Vivier, le Wed 28 Nov 2007 15:02:50 +0100, a écrit :
> +    ret = posix_memalign((void**)&buf, 0x200, 512);

For making this more easily portable, maybe it should be a new
qemu_memalign() function?  Also, the alignment may probably be better as
a global macro, since the alignment requirements depend on the OS.

Samuel

Re: [Qemu-devel] [PATCH 0/2] Open disk images with O_DIRECT

[Daniel P. Berrange]

On Wed, Nov 28, 2007 at 03:02:50PM +0100, Laurent Vivier wrote:
> These patches allow to open file using O_DIRECT and bypass the host I/O cache.
>
> [PATCH 1/2] Add "directio" parameter to "-drive"
> 
>     Using "directio=on" with "-drive" will open the disk image 
>     file using "O_DIRECT".

I don't see the point in adding a config param for this. If it provides a
useful performance improvement (or other benefit) we should enable it by 
default all the time.

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Re: [Qemu-devel] [PATCH 0/2] Open disk images with O_DIRECT

[Samuel Thibault]

Daniel P. Berrange, le Wed 28 Nov 2007 14:27:39 +0000, a écrit :
> On Wed, Nov 28, 2007 at 03:02:50PM +0100, Laurent Vivier wrote:
> > These patches allow to open file using O_DIRECT and bypass the host I/O cache.
> >
> > [PATCH 1/2] Add "directio" parameter to "-drive"
> > 
> >     Using "directio=on" with "-drive" will open the disk image 
> >     file using "O_DIRECT".
> 
> I don't see the point in adding a config param for this. If it provides a
> useful performance improvement (or other benefit) we should enable it by 
> default all the time.

That depends on the mileage of the user.  Sometimes it is useful since
it avoids the duplication of page or buffer cache between the guest and
the host, sometimes it is not because the guest is not so i/o friendly
and hence using the host page/buffer cache is useful.

Samuel

Re: [Qemu-devel] [PATCH 0/2] Open disk images with O_DIRECT

[Anthony Liguori]

Samuel Thibault wrote:
> Daniel P. Berrange, le Wed 28 Nov 2007 14:27:39 +0000, a écrit :
>   
>> On Wed, Nov 28, 2007 at 03:02:50PM +0100, Laurent Vivier wrote:
>>     
>>> These patches allow to open file using O_DIRECT and bypass the host I/O cache.
>>>
>>> [PATCH 1/2] Add "directio" parameter to "-drive"
>>>
>>>     Using "directio=on" with "-drive" will open the disk image 
>>>     file using "O_DIRECT".
>>>       
>> I don't see the point in adding a config param for this. If it provides a
>> useful performance improvement (or other benefit) we should enable it by 
>> default all the time.
>>     
>
> That depends on the mileage of the user.  Sometimes it is useful since
> it avoids the duplication of page or buffer cache between the guest and
> the host, sometimes it is not because the guest is not so i/o friendly
> and hence using the host page/buffer cache is useful.
>   

In the very least, I think it should be named something more along the 
lines of "caching=off" or "buffering=off" since that's closer to what 
the hardware concept is without exposing implementation details to the user.

Regards,

Anthony Liguori

> Samuel
>
>
>
>   

Re: [Qemu-devel] [PATCH 1/2] Add "directio" parameter to "-drive"

[Laurent Vivier]

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

Le mercredi 28 novembre 2007 à 14:24 +0000, Samuel Thibault a écrit :
> Hi,
> 
> Laurent Vivier, le Wed 28 Nov 2007 15:02:50 +0100, a écrit :
> > +    ret = posix_memalign((void**)&buf, 0x200, 512);
> 
> For making this more easily portable, maybe it should be a new
> qemu_memalign() function?  Also, the alignment may probably be better as
> a global macro, since the alignment requirements depend on the OS.

I think O_DIRECT is linux specific whereas posix_memalign() is part of
POSIX specification. And the memory alignement should only needed by the
implemetation of O_DIRECT on linux.

But I have nothing against to write a qemu_memalign().

Laurent
-- 
------------- Laurent.Vivier@bull.net  --------------
       "Any sufficiently advanced technology is
  indistinguishable from magic." - Arthur C. Clarke

[-- Attachment #2: Ceci est une partie de message numériquement signée --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [Qemu-devel] [PATCH 1/2] Add "directio" parameter to "-drive"

[Samuel Thibault]

Laurent Vivier, le Wed 28 Nov 2007 16:00:17 +0100, a écrit :
> Le mercredi 28 novembre 2007 à 14:24 +0000, Samuel Thibault a écrit :
> > Laurent Vivier, le Wed 28 Nov 2007 15:02:50 +0100, a écrit :
> > > +    ret = posix_memalign((void**)&buf, 0x200, 512);
> > 
> > For making this more easily portable, maybe it should be a new
> > qemu_memalign() function?  Also, the alignment may probably be better as
> > a global macro, since the alignment requirements depend on the OS.
> 
> I think O_DIRECT is linux specific

Yes.

> whereas posix_memalign() is part of POSIX specification.

Yes, but not so many systems have it.

> And the memory alignement should only needed by the implemetation of
> O_DIRECT on linux.

IIRC it is also required e.g. by the equivalent raw devices of Solaris.
It happens that I also need it for the Xen stubdomains support :)

Samuel

Re: [Qemu-devel] [PATCH 0/2] Open disk images with O_DIRECT

[Daniel P. Berrange]

On Wed, Nov 28, 2007 at 02:34:02PM +0000, Samuel Thibault wrote:
> Daniel P. Berrange, le Wed 28 Nov 2007 14:27:39 +0000, a écrit :
> > On Wed, Nov 28, 2007 at 03:02:50PM +0100, Laurent Vivier wrote:
> > > These patches allow to open file using O_DIRECT and bypass the host I/O cache.
> > >
> > > [PATCH 1/2] Add "directio" parameter to "-drive"
> > > 
> > >     Using "directio=on" with "-drive" will open the disk image 
> > >     file using "O_DIRECT".
> > 
> > I don't see the point in adding a config param for this. If it provides a
> > useful performance improvement (or other benefit) we should enable it by 
> > default all the time.
> 
> That depends on the mileage of the user.  Sometimes it is useful since
> it avoids the duplication of page or buffer cache between the guest and
> the host, sometimes it is not because the guest is not so i/o friendly
> and hence using the host page/buffer cache is useful.

I don't buy that - all OS already do I/O caching because its useful even
on baremetal.  As an end-user how do you decide whether to turn on the
directoio=on/off  option or not ? Most people won't notice it & will just
run with the default setting - those who do notice are just subjected to
trial-and-error to figure out whether the setting is any use. IMHO, QEMU
should just pick the best setting.


Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Re: [Qemu-devel] [PATCH 0/2] Open disk images with O_DIRECT

[Samuel Thibault]

Daniel P. Berrange, le Wed 28 Nov 2007 15:08:20 +0000, a écrit :
> > sometimes it is not because the guest is not so i/o friendly
> > and hence using the host page/buffer cache is useful.
> 
> I don't buy that - all OS already do I/O caching because its useful even
> on baremetal.

Well, I can see the difference when installing windows for instance.

Samuel

Re: [Qemu-devel] [PATCH 1/2] Add "directio" parameter to "-drive"

[Laurent Vivier]

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

Le mercredi 28 novembre 2007 à 14:24 +0000, Samuel Thibault a écrit :
> Hi,
> 
> Laurent Vivier, le Wed 28 Nov 2007 15:02:50 +0100, a écrit :
> > +    ret = posix_memalign((void**)&buf, 0x200, 512);
> 
> For making this more easily portable, maybe it should be a new
> qemu_memalign() function?  Also, the alignment may probably be better as
> a global macro, since the alignment requirements depend on the OS.

In fact, I think I should use qemu_vmalloc().
I modify the patch for that.

Laurent
-- 
------------- Laurent.Vivier@bull.net  --------------
       "Any sufficiently advanced technology is
  indistinguishable from magic." - Arthur C. Clarke

[-- Attachment #2: Ceci est une partie de message numériquement signée --]
[-- Type: application/pgp-signature, Size: 189 bytes --]