[Qemu-devel] [PATCH 0/2][RFC] Fix regression in load_linux for stable

[Qemu-devel] [PATCH 0/2][RFC] Fix regression in load_linux for stable

[Anthony Liguori]

Instead of just applying Mark's fix, I've reverted the original broken backport,
cherry picked the original patch, included Mark's backport fix, and committed
the result.

This gives us one commit in stable that can be cherry picked that is regression
free.  I did this to make things easier for packagers.

Any objections?

[Qemu-devel] [PATCH 1/2] Revert "reset state for load_linux"

[Anthony Liguori]

This reverts commit 2da1e398641d9fccf683645c808dee0d088f84cf.

This fix on the stable branch:

  commit 2da1e398641d9fccf683645c808dee0d088f84cf
  Author: Glauber Costa <glommer@redhat.com>
  Date:   Fri May 8 02:22:13 2009 -0300

    reset state for load_linux

Caused -kernel to break.

The problem is that we're passing the ROM's ram_addr_t to
load_linux() rather than its target_phys_addr_t. We also
need to register the memory before trying to write to
it.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/hw/pc.c b/hw/pc.c
index 1b16373..c33cd75 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -471,7 +471,7 @@ static void bochs_bios_init(void)
 
 /* Generate an initial boot sector which sets state and jump to
    a specified vector */
-static void generate_bootsect(target_phys_addr_t option_rom,
+static void generate_bootsect(uint8_t *option_rom,
                               uint32_t gpr[8], uint16_t segs[6], uint16_t ip)
 {
     uint8_t rom[512], *p, *reloc;
@@ -545,8 +545,7 @@ static void generate_bootsect(target_phys_addr_t option_rom,
         sum += rom[i];
     rom[sizeof(rom) - 1] = -sum;
 
-    cpu_physical_memory_write_rom(option_rom, rom, sizeof(rom));
-    option_rom_setup_reset(option_rom, sizeof (rom));
+    memcpy(option_rom, rom, sizeof(rom));
 }
 
 static long get_file_size(FILE *f)
@@ -563,7 +562,7 @@ static long get_file_size(FILE *f)
     return size;
 }
 
-static void load_linux(target_phys_addr_t option_rom,
+static void load_linux(uint8_t *option_rom,
                        const char *kernel_filename,
 		       const char *initrd_filename,
 		       const char *kernel_cmdline)
@@ -714,12 +713,6 @@ static void load_linux(target_phys_addr_t option_rom,
     memset(gpr, 0, sizeof gpr);
     gpr[4] = cmdline_addr-real_addr-16;	/* SP (-16 is paranoia) */
 
-    option_rom_setup_reset(real_addr, setup_size);
-    option_rom_setup_reset(prot_addr, kernel_size);
-    option_rom_setup_reset(cmdline_addr, cmdline_size);
-    if (initrd_filename)
-        option_rom_setup_reset(initrd_addr, initrd_size);
-
     generate_bootsect(option_rom, gpr, seg, 0);
 }
 
@@ -927,7 +920,7 @@ vga_bios_error:
         offset = 0;
         if (linux_boot) {
             option_rom_offset = qemu_ram_alloc(TARGET_PAGE_SIZE);
-            load_linux(option_rom_offset,
+            load_linux(phys_ram_base + option_rom_offset,
                        kernel_filename, initrd_filename, kernel_cmdline);
             cpu_register_physical_memory(0xd0000, TARGET_PAGE_SIZE,
                                          option_rom_offset);
-- 
1.6.0.6

[Qemu-devel] [PATCH 2/2] reset state for load_linux

[Anthony Liguori]

From: Glauber Costa <glommer@redhat.com>

The linux loader is just an option rom like any other, just with
some special requirements. Right now, our option rom resetting
mechanism is not being applied to it. As a result, users using
-kernel will not be able to successfully reboot their machines

This patch fixes it by saving all the data we generated in
the load_linux() function, to be used later by the option rom
resetting mechanism.

This also includes Mark's fix for -kernel

Signed-off-by: Glauber Costa <glommer@redhat.com>
Signed-off-by: Mark McLoughlin <markmc@redhat.com>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/hw/pc.c b/hw/pc.c
index c33cd75..40486aa 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -471,7 +471,7 @@ static void bochs_bios_init(void)
 
 /* Generate an initial boot sector which sets state and jump to
    a specified vector */
-static void generate_bootsect(uint8_t *option_rom,
+static void generate_bootsect(target_phys_addr_t option_rom,
                               uint32_t gpr[8], uint16_t segs[6], uint16_t ip)
 {
     uint8_t rom[512], *p, *reloc;
@@ -545,7 +545,8 @@ static void generate_bootsect(uint8_t *option_rom,
         sum += rom[i];
     rom[sizeof(rom) - 1] = -sum;
 
-    memcpy(option_rom, rom, sizeof(rom));
+    cpu_physical_memory_write_rom(option_rom, rom, sizeof(rom));
+    option_rom_setup_reset(option_rom, sizeof (rom));
 }
 
 static long get_file_size(FILE *f)
@@ -562,7 +563,7 @@ static long get_file_size(FILE *f)
     return size;
 }
 
-static void load_linux(uint8_t *option_rom,
+static void load_linux(target_phys_addr_t option_rom,
                        const char *kernel_filename,
 		       const char *initrd_filename,
 		       const char *kernel_cmdline)
@@ -713,6 +714,12 @@ static void load_linux(uint8_t *option_rom,
     memset(gpr, 0, sizeof gpr);
     gpr[4] = cmdline_addr-real_addr-16;	/* SP (-16 is paranoia) */
 
+    option_rom_setup_reset(real_addr, setup_size);
+    option_rom_setup_reset(prot_addr, kernel_size);
+    option_rom_setup_reset(cmdline_addr, cmdline_size);
+    if (initrd_filename)
+        option_rom_setup_reset(initrd_addr, initrd_size);
+
     generate_bootsect(option_rom, gpr, seg, 0);
 }
 
@@ -920,10 +927,10 @@ vga_bios_error:
         offset = 0;
         if (linux_boot) {
             option_rom_offset = qemu_ram_alloc(TARGET_PAGE_SIZE);
-            load_linux(phys_ram_base + option_rom_offset,
-                       kernel_filename, initrd_filename, kernel_cmdline);
             cpu_register_physical_memory(0xd0000, TARGET_PAGE_SIZE,
                                          option_rom_offset);
+            load_linux(0xd0000,
+                       kernel_filename, initrd_filename, kernel_cmdline);
             offset = TARGET_PAGE_SIZE;
         }
 
-- 
1.6.0.6

[Qemu-devel] Re: [PATCH 2/2] reset state for load_linux

[Mark McLoughlin]

On Wed, 2009-05-13 at 12:01 -0300, Glauber Costa wrote:
> On Wed, May 13, 2009 at 09:43:46AM -0500, Anthony Liguori wrote:
> > From: Glauber Costa <glommer@redhat.com>
> > 
> > The linux loader is just an option rom like any other, just with
> > some special requirements. Right now, our option rom resetting
> > mechanism is not being applied to it. As a result, users using
> > -kernel will not be able to successfully reboot their machines
> > 
> > This patch fixes it by saving all the data we generated in
> > the load_linux() function, to be used later by the option rom
> > resetting mechanism.
> > 
> > This also includes Mark's fix for -kernel
> Just to be clear:
> 
> We just need mark's fix in stable, right?

Yep, the code looks right in master and I'm pretty sure I tested it too.

Cheers,
Mark.

[Qemu-devel] Re: [PATCH 0/2][RFC] Fix regression in load_linux for stable

[Mark McLoughlin]

On Wed, 2009-05-13 at 09:43 -0500, Anthony Liguori wrote:
> Instead of just applying Mark's fix, I've reverted the original broken backport,
> cherry picked the original patch, included Mark's backport fix, and committed
> the result.
> 
> This gives us one commit in stable that can be cherry picked that is regression
> free.  I did this to make things easier for packagers.

Sounds like a fine plan to me.

Thanks,
Mark.

[Qemu-devel] Re: [PATCH 2/2] reset state for load_linux

[Glauber Costa]

On Wed, May 13, 2009 at 09:43:46AM -0500, Anthony Liguori wrote:
> From: Glauber Costa <glommer@redhat.com>
> 
> The linux loader is just an option rom like any other, just with
> some special requirements. Right now, our option rom resetting
> mechanism is not being applied to it. As a result, users using
> -kernel will not be able to successfully reboot their machines
> 
> This patch fixes it by saving all the data we generated in
> the load_linux() function, to be used later by the option rom
> resetting mechanism.
> 
> This also includes Mark's fix for -kernel
Just to be clear:

We just need mark's fix in stable, right?