Skip to contentSkip to conversation

fix extboot from boot with cache=off

PATCH Applied Nolan 2 participants 4 messages Mar 17, 2009 – Mar 22, 2009 KVM

lore
Share & tools
lore

Nolan

author
#0 lore raw

PATCH 1/1 fix extboot from boot with cache=off

Extboot submits requests with whatever buffer alignment the guest gave to the BIOS. This breaks with O_DIRECT disks, as they require 512 byte alignment.

Most guest bootloaders sector align their requests out of paranoia, but the OpenBSD bootloader does not.

This patch always copies. Since extboot is only used at boot time to load limited amounts of data, the overhead is not problematic. I also switched to using cpu_physical_memory_* instead of groveling around with phys_ram_base directly.

Signed-off-by Nolan Leake <nolan <at> sigbus.net>

---

 qemu/hw/extboot.c |   35 ++++++++++++++++++++++++-----------
 1 files changed, 24 insertions(+), 11 deletions(-)
qemu/hw/extboot.c +24 -11
index ada0fdd..ea16b3e 100644
@@ -77,19 +77,29 @@ static void extboot_write_cmd(void *opaque, uint32_t addr, uint32_t value)
     BlockDriverState *bs = opaque;
     int cylinders, heads, sectors, err;
     uint64_t nb_sectors;
-
-    get_translated_chs(bs, &cylinders, &heads, &sectors);
+    target_phys_addr_t pa;
+    int blen;
+    void *buf = NULL;
 
     if (cmd->type == 0x01 || cmd->type == 0x02) {
-	target_ulong pa = cmd->xfer.segment * 16 + cmd->xfer.segment;
+	pa = cmd->xfer.segment * 16 + cmd->xfer.offset;
+        blen = cmd->xfer.nb_sectors * 512;
+        buf = qemu_memalign(512, blen);
+        if (!buf) {
+            printf("qemu_memalign failed\n");
+            return;
+        }
 
 	/* possible buffer overflow */
-	if ((pa + cmd->xfer.nb_sectors * 512) > phys_ram_size)
+	if ((pa + blen) > phys_ram_size) {
+            qemu_free(buf);
 	    return;
+        }
     }
 
     switch (cmd->type) {
     case 0x00:
+        get_translated_chs(bs, &cylinders, &heads, &sectors);
 	bdrv_get_geometry(bs, &nb_sectors);
 	cmd->query_geometry.cylinders = cylinders;
 	cmd->query_geometry.heads = heads;

@@ -98,22 +108,25 @@ static void extboot_write_cmd(void *opaque, uint32_t addr, uint32_t value)
 	cpu_physical_memory_set_dirty((value & 0xFFFF) << 4);
 	break;
     case 0x01:
-	err = bdrv_read(bs, cmd->xfer.sector, phys_ram_base +
-			cmd->xfer.segment * 16 + cmd->xfer.offset,
-			cmd->xfer.nb_sectors);
+	err = bdrv_read(bs, cmd->xfer.sector, buf, cmd->xfer.nb_sectors);
 	if (err)
 	    printf("Read failed\n");
+
+        cpu_physical_memory_write(pa, buf, blen);
+
 	break;
     case 0x02:
-	err = bdrv_write(bs, cmd->xfer.sector, phys_ram_base +
-			 cmd->xfer.segment * 16 + cmd->xfer.offset,
-			 cmd->xfer.nb_sectors);
+        cpu_physical_memory_read(pa, buf, blen);
+
+	err = bdrv_write(bs, cmd->xfer.sector, buf, cmd->xfer.nb_sectors);
 	if (err)
 	    printf("Write failed\n");
 
-	cpu_physical_memory_set_dirty(cmd->xfer.segment * 16 + cmd->xfer.offset);
 	break;
     }
+
+    if (buf)
+        qemu_free(buf);
 }
 
 void extboot_init(BlockDriverState *bs, int cmd)
#1 lore raw
Quoted 33 lines · Nolan

Extboot submits requests with whatever buffer alignment the guest gave to the BIOS. This breaks with O_DIRECT disks, as they require 512 byte alignment.

Most guest bootloaders sector align their requests out of paranoia, but the OpenBSD bootloader does not.

This patch always copies. Since extboot is only used at boot time to load limited amounts of data, the overhead is not problematic. I also switched to using cpu_physical_memory_* instead of groveling around with phys_ram_base directly.

Signed-off-by Nolan Leake <nolan <at> sigbus.net>

---

 qemu/hw/extboot.c |   35 ++++++++++++++++++++++++-----------
 1 files changed, 24 insertions(+), 11 deletions(-)
qemu/hw/extboot.c +3 -2
index ada0fdd..ea16b3e 100644
@@ -77,19 +77,29 @@ static void extboot_write_cmd(void *opaque, uint32_t addr, uint32_t value)
     BlockDriverState *bs = opaque;
     int cylinders, heads, sectors, err;
     uint64_t nb_sectors;
-
-    get_translated_chs(bs, &cylinders, &heads, &sectors);
+    target_phys_addr_t pa;
+    int blen;
+    void *buf = NULL;

qemu/hw/extboot.c static void extboot_write_cmd(void *opaque, uint32_t addr, uint32_t value) · line 92

Quoted 10 lines
qemu/hw/extboot.c +7 -1
     if (cmd->type == 0x01 || cmd->type == 0x02) {
-	target_ulong pa = cmd->xfer.segment * 16 + cmd->xfer.segment;
+	pa = cmd->xfer.segment * 16 + cmd->xfer.offset;
+        blen = cmd->xfer.nb_sectors * 512;
+        buf = qemu_memalign(512, blen);
+        if (!buf) {
+            printf("qemu_memalign failed\n");
+            return;
+        }

Don't check for allocation failures. qemu_malloc() terminates gracefully on failure, qemu_memalign() does not, but should.

qemu/hw/extboot.c static void extboot_write_cmd(void *opaque, uint32_t addr, uint32_t value) · line 99

Quoted 8 lines
qemu/hw/extboot.c +3 -1
 	/* possible buffer overflow */
-	if ((pa + cmd->xfer.nb_sectors * 512) > phys_ram_size)
+	if ((pa + blen) > phys_ram_size) {
+            qemu_free(buf);
 	    return;
+        }

cpu_physical_memory_rw() will check these for you.

All in all, a nice improvement in addition to the bug fix.

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#2 lore raw

Thanks for the prompt review!

Quoted 13 lines · Avi Kivity

Review

Quoted 9 lines · Nolan
     if (cmd->type == 0x01 || cmd->type == 0x02) {
(quoted diff) +7 -1
-	target_ulong pa = cmd->xfer.segment * 16 + cmd->xfer.segment;
+	pa = cmd->xfer.segment * 16 + cmd->xfer.offset;
+        blen = cmd->xfer.nb_sectors * 512;
+        buf = qemu_memalign(512, blen);
+        if (!buf) {
+            printf("qemu_memalign failed\n");
+            return;
+        }

Don't check for allocation failures. qemu_malloc() terminates gracefully on failure, qemu_memalign() does not, but should.

OK, I've removed the check. As long as I'm in here, I've removed the check of the return value of qemu_mallocz in extboot_init as well.

I'll send a separate patch to qemu-devel with the change to make qemu_memalign abort on failure.

Quoted 9 lines

Review

Quoted 7 lines
(quoted diff) +3 -1
 	/* possible buffer overflow */
-	if ((pa + cmd->xfer.nb_sectors * 512) > phys_ram_size)
+	if ((pa + blen) > phys_ram_size) {
+            qemu_free(buf);
 	    return;
+        }

cpu_physical_memory_rw() will check these for you.

Check removed.

New patch appended:

Signed-off-by Nolan Leake <nolan <at> sigbus.net>

---

 qemu/hw/extboot.c |   35 +++++++++++++++++------------------
 1 files changed, 17 insertions(+), 18 deletions(-)
qemu/hw/extboot.c +17 -18
index ada0fdd..32e6226 100644
@@ -77,19 +77,19 @@ static void extboot_write_cmd(void *opaque, uint32_t addr, uint32_t value)
     BlockDriverState *bs = opaque;
     int cylinders, heads, sectors, err;
     uint64_t nb_sectors;
-
-    get_translated_chs(bs, &cylinders, &heads, &sectors);
+    target_phys_addr_t pa;
+    int blen;
+    void *buf = NULL;
 
     if (cmd->type == 0x01 || cmd->type == 0x02) {
-	target_ulong pa = cmd->xfer.segment * 16 + cmd->xfer.segment;
-
-	/* possible buffer overflow */
-	if ((pa + cmd->xfer.nb_sectors * 512) > phys_ram_size)
-	    return;
+	pa = cmd->xfer.segment * 16 + cmd->xfer.offset;
+        blen = cmd->xfer.nb_sectors * 512;
+        buf = qemu_memalign(512, blen);
     }
 
     switch (cmd->type) {
     case 0x00:
+        get_translated_chs(bs, &cylinders, &heads, &sectors);
 	bdrv_get_geometry(bs, &nb_sectors);
 	cmd->query_geometry.cylinders = cylinders;
 	cmd->query_geometry.heads = heads;

@@ -98,22 +98,25 @@ static void extboot_write_cmd(void *opaque, uint32_t addr, uint32_t value)
 	cpu_physical_memory_set_dirty((value & 0xFFFF) << 4);
 	break;
     case 0x01:
-	err = bdrv_read(bs, cmd->xfer.sector, phys_ram_base +
-			cmd->xfer.segment * 16 + cmd->xfer.offset,
-			cmd->xfer.nb_sectors);
+	err = bdrv_read(bs, cmd->xfer.sector, buf, cmd->xfer.nb_sectors);
 	if (err)
 	    printf("Read failed\n");
+
+        cpu_physical_memory_write(pa, buf, blen);
+
 	break;
     case 0x02:
-	err = bdrv_write(bs, cmd->xfer.sector, phys_ram_base +
-			 cmd->xfer.segment * 16 + cmd->xfer.offset,
-			 cmd->xfer.nb_sectors);
+        cpu_physical_memory_read(pa, buf, blen);
+
+	err = bdrv_write(bs, cmd->xfer.sector, buf, cmd->xfer.nb_sectors);
 	if (err)
 	    printf("Write failed\n");
 
-	cpu_physical_memory_set_dirty(cmd->xfer.segment * 16 + cmd->xfer.offset);
 	break;
     }
+
+    if (buf)
+        qemu_free(buf);
 }
 
 void extboot_init(BlockDriverState *bs, int cmd)

@@ -121,10 +124,6 @@ void extboot_init(BlockDriverState *bs, int cmd)
     int *pcmd;
 
     pcmd = qemu_mallocz(sizeof(int));
-    if (!pcmd) {
-	fprintf(stderr, "Error allocating memory\n");
-	exit(1);
-    }
 
     *pcmd = cmd;
     register_ioport_read(0x404, 1, 1, extboot_read, pcmd);
Applied
#3 lore raw
Quoted 3 lines · Nolan

New patch appended:

Applied, thanks.

Signature · 1 line
error compiling committee.c: too many arguments to function