fix extboot from boot with cache=off

Commit Message

Nolan March 17, 2009, 2:52 a.m. UTC

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(-)



--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Avi Kivity March 19, 2009, 10:03 a.m. UTC | #1

Nolan wrote:
> 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(-)
>
> diff --git a/qemu/hw/extboot.c b/qemu/hw/extboot.c
> index ada0fdd..ea16b3e 100644
> --- a/qemu/hw/extboot.c
> +++ b/qemu/hw/extboot.c
> @@ -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;
> +        }
>   

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

>  
>  	/* 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.

Patch

diff --git a/qemu/hw/extboot.c b/qemu/hw/extboot.c
index ada0fdd..ea16b3e 100644
--- a/qemu/hw/extboot.c
+++ b/qemu/hw/extboot.c
@@ -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)