[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

[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?

[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] [PATCH 0/2] Properly set reset functions for option roms

[Glauber Costa]

This series is comprised of two patches:

* The first one, preceeds the second,
* the second one, succeeds the first.

Besides this obvious ordering adventure, they:
 - bring the option rom reset mechanism for kvm. Without it,
   we are not able to reset the machine and have it using its
   boot options again. The code is quite similar to what is in
   qemu-kvm.git
 - allow this mechanism to work with -kernel too. This is currently
   broken, both here and in qemu-kvm.git.

[Qemu-devel] [PATCH 1/2] register reset handler for option_roms

[Glauber Costa]

Currently, boot options are not preserved across a system reset.
option roms can modify themselves, or can for instance restore the real
int 0x19 vector after they tried to boot from it.

To properly do that, we need a reset handler registered to deal with option
roms. This patch is based on current version on qemu-kvm.git

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 hw/pc.c |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 61f6e7b..0025474 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -63,6 +63,30 @@ static PITState *pit;
 static IOAPICState *ioapic;
 static PCIDevice *i440fx_state;
 
+typedef struct rom_reset_data {
+    uint8_t *data;
+    target_phys_addr_t addr;
+    unsigned size;
+} RomResetData;
+
+static void option_rom_reset(void *_rrd)
+{
+    RomResetData *rrd = _rrd;
+
+    cpu_physical_memory_write_rom(rrd->addr, rrd->data, rrd->size);
+}
+
+static void option_rom_setup_reset(target_phys_addr_t addr, unsigned size)
+{
+    RomResetData *rrd = qemu_malloc(sizeof *rrd);
+
+    rrd->data = qemu_malloc(size);
+    cpu_physical_memory_read(addr, rrd->data, size);
+    rrd->addr = addr;
+    rrd->size = size;
+    qemu_register_reset(option_rom_reset, rrd);
+}
+
 static void ioport80_write(void *opaque, uint32_t addr, uint32_t data)
 {
 }
@@ -806,6 +830,7 @@ static int load_option_rom(const char *oprom, target_phys_addr_t start,
         }
         /* Round up optiom rom size to the next 2k boundary */
         size = (size + 2047) & ~2047;
+        option_rom_setup_reset(start, size);
         return size;
 }
 
-- 
1.6.2.2

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

[Glauber Costa]

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.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 hw/pc.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 0025474..6b46427 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -579,6 +579,7 @@ static void generate_bootsect(target_phys_addr_t option_rom,
     rom[sizeof(rom) - 1] = -sum;
 
     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)
@@ -746,6 +747,12 @@ 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);
 }
 
-- 
1.6.2.2