[PATCH 0/3][IA64] Accelerate IDE PIO on HVM/IA64

[PATCH 0/3][IA64] Accelerate IDE PIO on HVM/IA64

[Kouya SHIMURA]

Hi,

This set of patches remarkably accelerates IDE PIO on HVM/IA64.
I got a throughput of 2.11MB/sec in disk read performance.
Without it, it was only 64kB/sec.

I posted the prototype once. 
http://lists.xensource.com/archives/html/xen-devel/2006-12/msg00077.html

The basic idea is to add a buffering mechanism in a hypervisor.
I know this approach is not sophisticated. But there is no other
good way in IA64 which has no string instructions like x86's.

This patchset is indispensable to support windows/ia64 on HVM
since installing windows and crash dumping is terribly slow.

There is no effect on x86 side. Please apply to xen-unstable/3.0.5.

Thanks,
Kouya

[PATCH 1/3][IA64] Accelerate IDE PIO on HVM/IA64 : qemu

[Kouya SHIMURA]

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 106 bytes --]

1/3 add a bufferring mechanism for IDE PIO in qemu.

Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>


[-- Attachment #2: accel_ide_qemu.patch --]
[-- Type: text/plain, Size: 7375 bytes --]

diff -r 2d3ceb082114 tools/ioemu/hw/ide.c
--- a/tools/ioemu/hw/ide.c	Mon Feb 26 09:13:50 2007 +0000
+++ b/tools/ioemu/hw/ide.c	Tue Feb 27 13:27:24 2007 +0900
@@ -434,6 +434,121 @@ static void dma_create_thread(void)
 }
 #endif /* DMA_MULTI_THREAD */
 
+#if defined(__ia64__)
+#include <xen/hvm/ioreq.h>
+
+struct buffered_piopage *buffered_pio_page;
+
+static inline struct pio_buffer *
+piobuf_by_addr(uint32_t addr)
+{
+    if (addr == 0x1F0)
+        return &buffered_pio_page->pio[PIO_BUFFER_IDE_PRIMARY];
+    if (addr == 0x170)
+        return &buffered_pio_page->pio[PIO_BUFFER_IDE_SECONDARY];
+    return NULL;
+}
+
+static void
+buffered_pio_init(void)
+{
+    struct pio_buffer *p1, *p2;
+    uint32_t off1, off2;
+
+    if (!buffered_pio_page)
+        return;
+
+    p1 = &buffered_pio_page->pio[PIO_BUFFER_IDE_PRIMARY];
+    p2 = &buffered_pio_page->pio[PIO_BUFFER_IDE_SECONDARY];
+    off1 = offsetof(struct buffered_piopage, buffer);
+    off2 = (off1 + TARGET_PAGE_SIZE)/2;
+
+    p1->buf_size = off2 - off1;
+    p1->page_offset = off1;
+
+    p2->buf_size = TARGET_PAGE_SIZE - off2;
+    p2->page_offset = off2;
+}
+
+static inline void
+buffered_pio_flush(struct pio_buffer *piobuf)
+{
+    IDEState *s = piobuf->opaque;
+    uint32_t pointer = piobuf->pointer;
+
+    if (s != NULL && pointer > 0) {
+        uint8_t *buf = (uint8_t *)buffered_pio_page + piobuf->page_offset;
+        memcpy(s->data_ptr, buf, pointer);
+        s->data_ptr += pointer;
+    }
+}
+
+static inline void
+buffered_pio_reset(IDEState *s)
+{
+    struct pio_buffer *piobuf;
+
+    if ((unsigned)s->drive_serial - 1 < 2)      /* 1,2 */
+        piobuf = &buffered_pio_page->pio[PIO_BUFFER_IDE_PRIMARY];
+    else if ((unsigned)s->drive_serial - 3 < 2) /* 3,4 */
+        piobuf = &buffered_pio_page->pio[PIO_BUFFER_IDE_SECONDARY];
+    else
+        return;
+    buffered_pio_flush(piobuf);
+    piobuf->pointer = 0;
+    piobuf->data_end = 0;
+    piobuf->opaque = NULL;
+}
+
+static inline void
+buffered_pio_write(IDEState *s, uint32_t addr, int size)
+{
+    struct pio_buffer *piobuf = piobuf_by_addr(addr);
+    uint32_t data_end;
+
+    if (!piobuf)
+        return;
+    buffered_pio_flush(piobuf);
+    data_end = s->data_end - s->data_ptr - size;
+    if (data_end <= 0)
+        data_end = 0;
+    else if (data_end > piobuf->buf_size)
+        data_end = piobuf->buf_size;
+    piobuf->pointer = 0;
+    piobuf->data_end = data_end;
+    piobuf->opaque = s;
+}
+
+static inline void
+buffered_pio_read(IDEState *s, uint32_t addr, int size)
+{
+    struct pio_buffer *piobuf = piobuf_by_addr(addr);
+    uint32_t data_end;
+
+    if (!piobuf)
+        return;
+    s->data_ptr += piobuf->pointer;
+    data_end = s->data_end - s->data_ptr - size;
+    if (data_end <= 0) {
+        data_end = 0;
+    } else {
+	uint8_t *buf = (uint8_t *)buffered_pio_page + piobuf->page_offset;
+        if (data_end > piobuf->buf_size)
+            data_end = piobuf->buf_size;
+        memcpy(buf, s->data_ptr + size, data_end);
+    }
+    piobuf->pointer = 0;
+    piobuf->data_end = data_end;
+    piobuf->opaque = NULL;
+}
+
+#else /* !__ia64__ */
+#define buffered_pio_init()         do {} while (0)
+#define buffered_pio_reset(I)       do {} while (0)
+#define buffered_pio_write(I,A,S)   do {} while (0)
+#define buffered_pio_read(I,A,S)    do {} while (0)
+#endif
+
 static void ide_dma_start(IDEState *s, IDEDMAFunc *dma_cb);
 
 static void padstr(char *str, const char *src, int len)
@@ -618,6 +733,7 @@ static void ide_transfer_start(IDEState 
     s->data_ptr = buf;
     s->data_end = buf + size;
     s->status |= DRQ_STAT;
+    buffered_pio_reset(s);
 }
 
 static void ide_transfer_stop(IDEState *s)
@@ -626,6 +742,7 @@ static void ide_transfer_stop(IDEState *
     s->data_ptr = s->io_buffer;
     s->data_end = s->io_buffer;
     s->status &= ~DRQ_STAT;
+    buffered_pio_reset(s);
 }
 
 static int64_t ide_get_sector(IDEState *s)
@@ -1562,6 +1679,7 @@ static void ide_ioport_write(void *opaqu
         ide_if[0].select = (val & ~0x10) | 0xa0;
         ide_if[1].select = (val | 0x10) | 0xa0;
         /* select drive */
+        buffered_pio_reset(ide_if->cur_drive);
         unit = (val >> 4) & 1;
         s = ide_if + unit;
         ide_if->cur_drive = s;
@@ -1928,6 +2046,7 @@ static void ide_data_writew(void *opaque
     IDEState *s = ((IDEState *)opaque)->cur_drive;
     uint8_t *p;
 
+    buffered_pio_write(s, addr, 2);
     p = s->data_ptr;
     *(uint16_t *)p = le16_to_cpu(val);
     p += 2;
@@ -1941,6 +2060,8 @@ static uint32_t ide_data_readw(void *opa
     IDEState *s = ((IDEState *)opaque)->cur_drive;
     uint8_t *p;
     int ret;
+    
+    buffered_pio_read(s, addr, 2);
     p = s->data_ptr;
     ret = cpu_to_le16(*(uint16_t *)p);
     p += 2;
@@ -1955,6 +2076,7 @@ static void ide_data_writel(void *opaque
     IDEState *s = ((IDEState *)opaque)->cur_drive;
     uint8_t *p;
 
+    buffered_pio_write(s, addr, 4);
     p = s->data_ptr;
     *(uint32_t *)p = le32_to_cpu(val);
     p += 4;
@@ -1969,6 +2091,7 @@ static uint32_t ide_data_readl(void *opa
     uint8_t *p;
     int ret;
     
+    buffered_pio_read(s, addr, 4);
     p = s->data_ptr;
     ret = cpu_to_le32(*(uint32_t *)p);
     p += 4;
@@ -2517,6 +2640,8 @@ void pci_piix3_ide_init(PCIBus *bus, Blo
     ide_init_ioport(&d->ide_if[0], 0x1f0, 0x3f6);
     ide_init_ioport(&d->ide_if[2], 0x170, 0x376);
 
+    buffered_pio_init();
+
     register_savevm("ide_pci", 0, 1, generic_pci_save, generic_pci_load, d);
 
 #ifdef DMA_MULTI_THREAD    
diff -r 2d3ceb082114 tools/ioemu/vl.c
--- a/tools/ioemu/vl.c	Mon Feb 26 09:13:50 2007 +0000
+++ b/tools/ioemu/vl.c	Tue Feb 27 13:27:24 2007 +0900
@@ -5972,6 +5972,7 @@ int main(int argc, char **argv)
     xen_pfn_t *page_array;
     extern void *shared_page;
     extern void *buffered_io_page;
+    extern void *buffered_pio_page;
 
     char qemu_dm_logfilename[64];
 
@@ -6571,6 +6572,10 @@ int main(int argc, char **argv)
                                        PROT_READ|PROT_WRITE,
                                        BUFFER_IO_PAGE_START >> PAGE_SHIFT);
 
+    buffered_pio_page = xc_map_foreign_range(xc_handle, domid, PAGE_SIZE,
+                                       PROT_READ|PROT_WRITE,
+                                       BUFFER_PIO_PAGE_START >> PAGE_SHIFT);
+
     for (i = 0; i < tmp_nr_pages; i++)
         page_array[i] = i;
 	
diff -r 2d3ceb082114 xen/include/public/hvm/ioreq.h
--- a/xen/include/public/hvm/ioreq.h	Mon Feb 26 09:13:50 2007 +0000
+++ b/xen/include/public/hvm/ioreq.h	Tue Feb 27 14:10:22 2007 +0900
@@ -81,6 +81,22 @@ struct buffered_iopage {
 };            /* sizeof this structure must be in one page */
 typedef struct buffered_iopage buffered_iopage_t;
 
+struct pio_buffer {
+    uint32_t page_offset;
+    uint32_t pointer;
+    uint32_t data_end;
+    uint32_t buf_size;
+    void *opaque;
+};
+
+#define PIO_BUFFER_IDE_PRIMARY   0 /* I/O port = 0x1F0 */
+#define PIO_BUFFER_IDE_SECONDARY 1 /* I/O port = 0x170 */
+#define PIO_BUFFER_ENTRY_NUM     2
+struct buffered_piopage {
+    struct pio_buffer pio[PIO_BUFFER_ENTRY_NUM];
+    uint8_t buffer[1];
+};
+
 #define ACPI_PM1A_EVT_BLK_ADDRESS           0x0000000000001f40
 #define ACPI_PM1A_CNT_BLK_ADDRESS           (ACPI_PM1A_EVT_BLK_ADDRESS + 0x04)
 #define ACPI_PM_TMR_BLK_ADDRESS             (ACPI_PM1A_EVT_BLK_ADDRESS + 0x08)

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

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

[PATCH 2/3][IA64] Accelerate IDE PIO on HVM/IA64 : tools

[Kouya SHIMURA]

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 102 bytes --]

2/3 add one more page to the guest for IDE PIO.

Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>


[-- Attachment #2: accel_ide_tools.patch --]
[-- Type: text/plain, Size: 2299 bytes --]

diff -r 2d3ceb082114 tools/libxc/ia64/xc_ia64_hvm_build.c
--- a/tools/libxc/ia64/xc_ia64_hvm_build.c	Mon Feb 26 09:13:50 2007 +0000
+++ b/tools/libxc/ia64/xc_ia64_hvm_build.c	Tue Feb 27 13:27:24 2007 +0900
@@ -569,6 +569,7 @@ setup_guest(int xc_handle, uint32_t dom,
     xen_pfn_t *pfn_list;
     shared_iopage_t *sp;
     void *ioreq_buffer_page;
+    void *pio_buffer_page;
     unsigned long dom_memsize = memsize << 20;
     unsigned long nr_pages = memsize << (20 - PAGE_SHIFT);
     unsigned long vcpus;
@@ -628,9 +629,10 @@ setup_guest(int xc_handle, uint32_t dom,
 
     pfn_list[0] = IO_PAGE_START >> PAGE_SHIFT;
     pfn_list[1] = STORE_PAGE_START >> PAGE_SHIFT;
-    pfn_list[2] = BUFFER_IO_PAGE_START >> PAGE_SHIFT; 
-
-    rc = xc_domain_memory_populate_physmap(xc_handle, dom, 3,
+    pfn_list[2] = BUFFER_IO_PAGE_START >> PAGE_SHIFT;
+    pfn_list[3] = BUFFER_PIO_PAGE_START >> PAGE_SHIFT;
+
+    rc = xc_domain_memory_populate_physmap(xc_handle, dom, 4,
                                            0, 0, &pfn_list[0]);
     if (rc != 0) {
         PERROR("Could not allocate IO page or store page or buffer io page.\n");
@@ -684,6 +686,12 @@ setup_guest(int xc_handle, uint32_t dom,
                                              pfn_list[2]); 
     memset(ioreq_buffer_page,0,PAGE_SIZE);
     munmap(ioreq_buffer_page, PAGE_SIZE);
+
+    pio_buffer_page = xc_map_foreign_range(xc_handle, dom, PAGE_SIZE,
+                                           PROT_READ | PROT_WRITE,
+                                           pfn_list[3]);
+    memset(pio_buffer_page,0,PAGE_SIZE);
+    munmap(pio_buffer_page, PAGE_SIZE);
     free(pfn_list);
     return 0;
 
diff -r 2d3ceb082114 tools/python/xen/xend/image.py
--- a/tools/python/xen/xend/image.py	Mon Feb 26 09:13:50 2007 +0000
+++ b/tools/python/xen/xend/image.py	Tue Feb 27 13:27:24 2007 +0900
@@ -572,8 +572,8 @@ class IA64_HVM_ImageHandler(HVMImageHand
 
     def getRequiredAvailableMemory(self, mem_kb):
         page_kb = 16
-        # ROM size for guest firmware, ioreq page and xenstore page
-        extra_pages = 1024 + 3
+        # ROM size for guest firmware, ioreq page, pio page and xenstore page
+        extra_pages = 1024 + 4
         return mem_kb + extra_pages * page_kb
 
     def getRequiredInitialReservation(self):

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

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

[PATCH 3/3][IA64] Accelerate IDE PIO on HVM/IA64 : ia64

[Kouya SHIMURA]

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 114 bytes --]

3/3 add a bufferring mechanism for IDE PIO in a hypervisor.

Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>


[-- Attachment #2: accel_ide_ia64.patch --]
[-- Type: text/plain, Size: 4058 bytes --]

diff -r 2d3ceb082114 xen/arch/ia64/vmx/mmio.c
--- a/xen/arch/ia64/vmx/mmio.c	Mon Feb 26 09:13:50 2007 +0000
+++ b/xen/arch/ia64/vmx/mmio.c	Tue Feb 27 13:27:24 2007 +0900
@@ -136,6 +136,56 @@ static void low_mmio_access(VCPU *vcpu, 
     }
     return;
 }
+
+int vmx_ide_pio_intercept(ioreq_t *p, u64 *val)
+{
+    struct buffered_piopage *pio_page =
+        (void *)(current->domain->arch.hvm_domain.buffered_pio_va);
+    struct pio_buffer *piobuf;
+    uint32_t pointer, page_offset;
+
+    if (p->addr == 0x1F0)
+	piobuf = &pio_page->pio[PIO_BUFFER_IDE_PRIMARY];
+    else if (p->addr == 0x170)
+	piobuf = &pio_page->pio[PIO_BUFFER_IDE_SECONDARY];
+    else
+	return 0;
+
+    if (p->size != 2 && p->size != 4)
+        return 0;
+
+    pointer = piobuf->pointer;
+    page_offset = piobuf->page_offset;
+
+    /* sanity check */
+    if (page_offset + pointer < offsetof(struct buffered_piopage, buffer))
+	return 0;
+    if (page_offset + piobuf->data_end > PAGE_SIZE)
+	return 0;
+
+    if (pointer + p->size < piobuf->data_end) {
+        uint8_t *bufp = (uint8_t *)pio_page + page_offset + pointer;
+        if (p->dir == IOREQ_WRITE) {
+            if (likely(p->size == 4 && (((long)bufp & 3) == 0)))
+                *(uint32_t *)bufp = *val;
+            else
+                memcpy(bufp, val, p->size);
+        } else {
+            if (likely(p->size == 4 && (((long)bufp & 3) == 0))) {
+                *val = *(uint32_t *)bufp;
+            } else {
+                *val = 0;
+                memcpy(val, bufp, p->size);
+            }
+        }
+        piobuf->pointer += p->size;
+        p->state = STATE_IORESP_READY;
+        vmx_io_assist(current);
+        return 1;
+    }
+    return 0;
+}
+
 #define TO_LEGACY_IO(pa)  (((pa)>>12<<2)|((pa)&0x3))
 
 static void legacy_io_access(VCPU *vcpu, u64 pa, u64 *val, size_t s, int dir)
@@ -160,6 +210,9 @@ static void legacy_io_access(VCPU *vcpu,
     p->df = 0;
 
     p->io_count++;
+
+    if (vmx_ide_pio_intercept(p, val))
+        return;
 
     vmx_send_assist_req(v);
     if(dir==IOREQ_READ){ //read
diff -r 2d3ceb082114 xen/arch/ia64/vmx/vmx_init.c
--- a/xen/arch/ia64/vmx/vmx_init.c	Mon Feb 26 09:13:50 2007 +0000
+++ b/xen/arch/ia64/vmx/vmx_init.c	Tue Feb 27 13:27:24 2007 +0900
@@ -390,6 +390,8 @@ void vmx_setup_platform(struct domain *d
 	spin_lock_init(&d->arch.hvm_domain.buffered_io_lock);
 	d->arch.hvm_domain.buffered_io_va =
 		(unsigned long)__va(__gpa_to_mpa(d, BUFFER_IO_PAGE_START));
+	d->arch.hvm_domain.buffered_pio_va =
+		(unsigned long)__va(__gpa_to_mpa(d, BUFFER_PIO_PAGE_START));
 	/* TEMP */
 	d->arch.vmx_platform.pib_base = 0xfee00000UL;
 
diff -r 2d3ceb082114 xen/include/asm-ia64/vmx_platform.h
--- a/xen/include/asm-ia64/vmx_platform.h	Mon Feb 26 09:13:50 2007 +0000
+++ b/xen/include/asm-ia64/vmx_platform.h	Tue Feb 27 13:27:24 2007 +0900
@@ -24,8 +24,9 @@
 #include <asm/viosapic.h>
 struct mmio_list;
 typedef struct virtual_platform_def {
-    unsigned long          buffered_io_va;
-    spinlock_t             buffered_io_lock;
+    unsigned long       buffered_io_va;
+    spinlock_t          buffered_io_lock;
+    unsigned long       buffered_pio_va;
     unsigned long       shared_page_va;
     unsigned long       pib_base;
     unsigned long       params[HVM_NR_PARAMS];
diff -r 2d3ceb082114 xen/include/public/arch-ia64.h
--- a/xen/include/public/arch-ia64.h	Mon Feb 26 09:13:50 2007 +0000
+++ b/xen/include/public/arch-ia64.h	Tue Feb 27 13:27:24 2007 +0900
@@ -91,10 +91,13 @@ typedef unsigned long xen_ulong_t;
 #define IO_PAGE_SIZE  PAGE_SIZE
 
 #define STORE_PAGE_START (IO_PAGE_START + IO_PAGE_SIZE)
-#define STORE_PAGE_SIZE	 PAGE_SIZE
-
-#define BUFFER_IO_PAGE_START (STORE_PAGE_START+PAGE_SIZE)
+#define STORE_PAGE_SIZE  PAGE_SIZE
+
+#define BUFFER_IO_PAGE_START (STORE_PAGE_START+STORE_PAGE_SIZE)
 #define BUFFER_IO_PAGE_SIZE PAGE_SIZE
+
+#define BUFFER_PIO_PAGE_START (BUFFER_IO_PAGE_START+BUFFER_IO_PAGE_SIZE)
+#define BUFFER_PIO_PAGE_SIZE PAGE_SIZE
 
 #define IO_SAPIC_START   0xfec00000UL
 #define IO_SAPIC_SIZE    0x100000

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

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

Re: [PATCH 0/3][IA64] Accelerate IDE PIO on HVM/IA64

[Keir Fraser]

On 27/2/07 09:34, "Kouya SHIMURA" <kouya@jp.fujitsu.com> wrote:

> The basic idea is to add a buffering mechanism in a hypervisor.
> I know this approach is not sophisticated. But there is no other
> good way in IA64 which has no string instructions like x86's.
> 
> This patchset is indispensable to support windows/ia64 on HVM
> since installing windows and crash dumping is terribly slow.

Can you explain how this new code works? As I understand it the problem is
that each PIO instruction decoded by Xen and propagated to qemu only
transfers a single word of data. How does this new buffering mechanism work
around this?

 -- Keir

Re: [PATCH 0/3][IA64] Accelerate IDE PIO on HVM/IA64

[Kouya SHIMURA]

Hi Keir,

For disk read, new buffering mechanism works as follows:

 1. Xen traps a single load instruction which performs PIO. 
    FYI, IA64 doesn't have IN/OUT instructions and io ports are memory
    mapped.
+2. Xen checks whether the data is already buffered. 
+3. If the data is buffered and not the last one, 
    xen just returns the data to a guest.
 4. Otherwise, xen make an i/o request to qemu as usual.
    And the guest blocks.
 5. Qemu receives the i/o request and prepares the block of data that
    is buffered in qemu.
+6. Qemu copies the block to a shared page which I add newly as
    "buffered_pio_page". That is, it exposes the qemu's buffering
    data to xen.
 7. Qemu returns the single data on i/o request to xen as usual.
 8. Xen resumes the guest.

The above lines beginning with + are newly added mechanism. 
1,2,3 are almost repeated and transactions between xen and qemu
are drastically reduced.

For disk write, it is almost the same as the above explanation.
The difference is a direction of copy. And what I have to say is
that qemu does nothing until the qemu's buffer becomes full. So
the transaction to qemu can be deferred.

Thanks,
Kouya

Keir Fraser writes:
 > On 27/2/07 09:34, "Kouya SHIMURA" <kouya@jp.fujitsu.com> wrote:
 > 
 > > The basic idea is to add a buffering mechanism in a hypervisor.
 > > I know this approach is not sophisticated. But there is no other
 > > good way in IA64 which has no string instructions like x86's.
 > > 
 > > This patchset is indispensable to support windows/ia64 on HVM
 > > since installing windows and crash dumping is terribly slow.
 > 
 > Can you explain how this new code works? As I understand it the problem is
 > that each PIO instruction decoded by Xen and propagated to qemu only
 > transfers a single word of data. How does this new buffering mechanism work
 > around this?
 > 
 >  -- Keir

Re: [PATCH 0/3][IA64] Accelerate IDE PIO on HVM/IA64

[Alex Williamson]

On Tue, 2007-02-27 at 18:34 +0900, Kouya SHIMURA wrote:
> Hi,
> 
> This set of patches remarkably accelerates IDE PIO on HVM/IA64.
> I got a throughput of 2.11MB/sec in disk read performance.
> Without it, it was only 64kB/sec.
> 
> I posted the prototype once. 
> http://lists.xensource.com/archives/html/xen-devel/2006-12/msg00077.html
> 
> The basic idea is to add a buffering mechanism in a hypervisor.
> I know this approach is not sophisticated. But there is no other
> good way in IA64 which has no string instructions like x86's.

   This seems like a pretty good performance increase for only a single
page of domain overhead in Xen.  My main concern would be that we
maintain correctness.  It is writing out the dirty buffer before the
domain shuts down, right?  I'm guessing the flush probably does that.
Looks like a bug here though:

+static inline void
+buffered_pio_write(IDEState *s, uint32_t addr, int size)
+{
+    struct pio_buffer *piobuf = piobuf_by_addr(addr);
+    uint32_t data_end;
+
+    if (!piobuf)
+        return;
+    buffered_pio_flush(piobuf);
+    data_end = s->data_end - s->data_ptr - size;
+    if (data_end <= 0)
+        data_end = 0;

   data_end is unsigned, so it will never be < 0.  Same problem on the
read func.  Thanks,

	Alex

-- 
Alex Williamson                             HP Open Source & Linux Org.

Re: [Xen-devel] Re: [PATCH 0/3][IA64] Accelerate IDE PIO on HVM/IA64

[Kouya SHIMURA]

Hi Alex,

Thanks for pointing out bugs. I'll check the whole code again.

For domain shutting down, my patch does nothing.  But the original
qemu (or real IDE controller chip also?) is involved in the same
problem. The dirty data will be missing by sudden death of a guest. 
A guest OS has the responsibility to flush the dirty buffer.

I might have to consider save/restore of hvm. That is a future work.

Thanks,
Kouya

Alex Williamson writes:
 > On Tue, 2007-02-27 at 18:34 +0900, Kouya SHIMURA wrote:
 > > Hi,
 > > 
 > > This set of patches remarkably accelerates IDE PIO on HVM/IA64.
 > > I got a throughput of 2.11MB/sec in disk read performance.
 > > Without it, it was only 64kB/sec.
 > > 
 > > I posted the prototype once. 
 > > http://lists.xensource.com/archives/html/xen-devel/2006-12/msg00077.html
 > > 
 > > The basic idea is to add a buffering mechanism in a hypervisor.
 > > I know this approach is not sophisticated. But there is no other
 > > good way in IA64 which has no string instructions like x86's.
 > 
 >    This seems like a pretty good performance increase for only a single
 > page of domain overhead in Xen.  My main concern would be that we
 > maintain correctness.  It is writing out the dirty buffer before the
 > domain shuts down, right?  I'm guessing the flush probably does that.
 > Looks like a bug here though:
 > 
 > +static inline void
 > +buffered_pio_write(IDEState *s, uint32_t addr, int size)
 > +{
 > +    struct pio_buffer *piobuf = piobuf_by_addr(addr);
 > +    uint32_t data_end;
 > +
 > +    if (!piobuf)
 > +        return;
 > +    buffered_pio_flush(piobuf);
 > +    data_end = s->data_end - s->data_ptr - size;
 > +    if (data_end <= 0)
 > +        data_end = 0;
 > 
 >    data_end is unsigned, so it will never be < 0.  Same problem on the
 > read func.  Thanks,
 > 
 > 	Alex
 > 
 > -- 
 > Alex Williamson                             HP Open Source & Linux Org.
 > 
 > 
 > _______________________________________________
 > Xen-devel mailing list
 > Xen-devel@lists.xensource.com
 > http://lists.xensource.com/xen-devel