From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=49294 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OIR8g-0001YK-9n for qemu-devel@nongnu.org; Sat, 29 May 2010 14:52:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OIR8e-0006b8-GP for qemu-devel@nongnu.org; Sat, 29 May 2010 14:52:06 -0400 Received: from hall.aurel32.net ([88.191.82.174]:57154) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OIR8e-0006aE-7f for qemu-devel@nongnu.org; Sat, 29 May 2010 14:52:04 -0400 Date: Sat, 29 May 2010 20:51:55 +0200 From: Aurelien Jarno Subject: Re: [Qemu-devel] [PATCH] arm: fix arm kernel boot for non zero start addr Message-ID: <20100529185155.GP17694@hall.aurel32.net> References: <1273351415-9963-1-git-send-email-lars@segv.dk> <20100528192421.GC4621@ohm.aurel32.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Sender: Aurelien Jarno List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Lars Munch Cc: qemu-devel@nongnu.org On Sat, May 29, 2010 at 08:42:52PM +0200, Lars Munch wrote: > On Fri, May 28, 2010 at 9:24 PM, Aurelien Jarno wrote: > > On Sat, May 08, 2010 at 10:43:35PM +0200, Lars Munch wrote: > >> Booting an arm kernel has been broken a while when booting from non zero start > >> address. This is due to the order of events: board init loads the kernel and > >> sets register 15 to the start address and then qemu_system_reset reset the cpu > >> making register 15 zero again. > >> > >> This patch fixes the usage of the register 15 start address trick in > >> combination with arm_load_kernel. > >> > >> Signed-off-by: Lars Munch > >> --- > >>  hw/arm_boot.c       |    1 + > >>  hw/gumstix.c        |    4 ---- > >>  hw/mainstone.c      |    3 --- > >>  hw/nseries.c        |    7 ------- > >>  hw/omap_sx1.c       |    5 ----- > >>  hw/palm.c           |    4 ---- > >>  hw/spitz.c          |    3 --- > >>  hw/tosa.c           |    3 --- > >>  target-arm/helper.c |    1 - > >>  9 files changed, 1 insertions(+), 30 deletions(-) > >> > >> diff --git a/hw/arm_boot.c b/hw/arm_boot.c > >> index df031a5..620550b 100644 > >> --- a/hw/arm_boot.c > >> +++ b/hw/arm_boot.c > >> @@ -187,6 +187,7 @@ static void main_cpu_reset(void *opaque) > >>              env->regs[15] = info->entry & 0xfffffffe; > >>              env->thumb = info->entry & 1; > >>          } else { > >> +            env->regs[15] = info->loader_start; > >>              if (old_param) { > >>                  set_kernel_args_old(info, info->initrd_size, > >>                                      info->loader_start); > > > > This parts looks fine. > > > >> diff --git a/hw/gumstix.c b/hw/gumstix.c > >> index 3fd31f4..b64e04e 100644 > >> --- a/hw/gumstix.c > >> +++ b/hw/gumstix.c > >> @@ -74,8 +74,6 @@ static void connex_init(ram_addr_t ram_size, > >>          exit(1); > >>      } > >> > >> -    cpu->env->regs[15] = 0x00000000; > >> - > >>      /* Interrupt line of NIC is connected to GPIO line 36 */ > >>      smc91c111_init(&nd_table[0], 0x04000300, > >>                      pxa2xx_gpio_in_get(cpu->gpio)[36]); > >> @@ -114,8 +112,6 @@ static void verdex_init(ram_addr_t ram_size, > >>          exit(1); > >>      } > >> > >> -    cpu->env->regs[15] = 0x00000000; > >> - > >>      /* Interrupt line of NIC is connected to GPIO line 99 */ > >>      smc91c111_init(&nd_table[0], 0x04000300, > >>                      pxa2xx_gpio_in_get(cpu->gpio)[99]); > >> diff --git a/hw/mainstone.c b/hw/mainstone.c > >> index a4379e3..54bacfb 100644 > >> --- a/hw/mainstone.c > >> +++ b/hw/mainstone.c > >> @@ -89,9 +89,6 @@ static void mainstone_common_init(ram_addr_t ram_size, > >>      cpu_register_physical_memory(0, MAINSTONE_ROM, > >>                      qemu_ram_alloc(MAINSTONE_ROM) | IO_MEM_ROM); > >> > >> -    /* Setup initial (reset) machine state */ > >> -    cpu->env->regs[15] = mainstone_binfo.loader_start; > >> - > >>  #ifdef TARGET_WORDS_BIGENDIAN > >>      be = 1; > >>  #else > >> diff --git a/hw/nseries.c b/hw/nseries.c > >> index 0273eee..04a028d 100644 > >> --- a/hw/nseries.c > >> +++ b/hw/nseries.c > >> @@ -1016,7 +1016,6 @@ static void n8x0_boot_init(void *opaque) > >>      n800_dss_init(&s->blizzard); > >> > >>      /* CPU setup */ > >> -    s->cpu->env->regs[15] = s->cpu->env->boot_info->loader_start; > >>      s->cpu->env->GE = 0x5; > >> > >>      /* If the machine has a slided keyboard, open it */ > >> @@ -1317,11 +1316,6 @@ static void n8x0_init(ram_addr_t ram_size, const char *boot_device, > >>      if (usb_enabled) > >>          n8x0_usb_setup(s); > >> > >> -    /* Setup initial (reset) machine state */ > >> - > >> -    /* Start at the OneNAND bootloader.  */ > >> -    s->cpu->env->regs[15] = 0; > >> - > >>      if (kernel_filename) { > >>          /* Or at the linux loader.  */ > >>          binfo->kernel_filename = kernel_filename; > >> @@ -1330,7 +1324,6 @@ static void n8x0_init(ram_addr_t ram_size, const char *boot_device, > >>          arm_load_kernel(s->cpu->env, binfo); > >> > >>          qemu_register_reset(n8x0_boot_init, s); > >> -        n8x0_boot_init(s); > >>      } > >> > >>      if (option_rom[0] && (boot_device[0] == 'n' || !kernel_filename)) { > >> diff --git a/hw/omap_sx1.c b/hw/omap_sx1.c > >> index ca0a7d1..2e9879f 100644 > >> --- a/hw/omap_sx1.c > >> +++ b/hw/omap_sx1.c > >> @@ -195,15 +195,10 @@ static void sx1_init(ram_addr_t ram_size, > >> > >>      /* Load the kernel.  */ > >>      if (kernel_filename) { > >> -        /* Start at bootloader.  */ > >> -        cpu->env->regs[15] = sx1_binfo.loader_start; > >> - > >>          sx1_binfo.kernel_filename = kernel_filename; > >>          sx1_binfo.kernel_cmdline = kernel_cmdline; > >>          sx1_binfo.initrd_filename = initrd_filename; > >>          arm_load_kernel(cpu->env, &sx1_binfo); > >> -    } else { > >> -        cpu->env->regs[15] = 0x00000000; > >>      } > >> > >>      /* TODO: fix next line */ > >> diff --git a/hw/palm.c b/hw/palm.c > >> index ba7c398..8db133d 100644 > >> --- a/hw/palm.c > >> +++ b/hw/palm.c > >> @@ -243,7 +243,6 @@ static void palmte_init(ram_addr_t ram_size, > >>              rom_size = load_image_targphys(option_rom[0], OMAP_CS0_BASE, > >>                                             flash_size); > >>              rom_loaded = 1; > >> -            cpu->env->regs[15] = 0x00000000; > >>          } > >>          if (rom_size < 0) { > >>              fprintf(stderr, "%s: error loading '%s'\n", > >> @@ -258,9 +257,6 @@ static void palmte_init(ram_addr_t ram_size, > >> > >>      /* Load the kernel.  */ > >>      if (kernel_filename) { > >> -        /* Start at bootloader.  */ > >> -        cpu->env->regs[15] = palmte_binfo.loader_start; > >> - > >>          palmte_binfo.kernel_filename = kernel_filename; > >>          palmte_binfo.kernel_cmdline = kernel_cmdline; > >>          palmte_binfo.initrd_filename = initrd_filename; > >> diff --git a/hw/spitz.c b/hw/spitz.c > >> index c3b5cd8..4f82e24 100644 > >> --- a/hw/spitz.c > >> +++ b/hw/spitz.c > >> @@ -993,9 +993,6 @@ static void spitz_common_init(ram_addr_t ram_size, > >>          /* A 4.0 GB microdrive is permanently sitting in CF slot 0.  */ > >>          spitz_microdrive_attach(cpu, 0); > >> > >> -    /* Setup initial (reset) machine state */ > >> -    cpu->env->regs[15] = spitz_binfo.loader_start; > >> - > >>      spitz_binfo.kernel_filename = kernel_filename; > >>      spitz_binfo.kernel_cmdline = kernel_cmdline; > >>      spitz_binfo.initrd_filename = initrd_filename; > >> diff --git a/hw/tosa.c b/hw/tosa.c > >> index bc6591f..fbe8d8c 100644 > >> --- a/hw/tosa.c > >> +++ b/hw/tosa.c > >> @@ -229,9 +229,6 @@ static void tosa_init(ram_addr_t ram_size, > >> > >>      tosa_tg_init(cpu); > >> > >> -    /* Setup initial (reset) machine state */ > >> -    cpu->env->regs[15] = tosa_binfo.loader_start; > >> - > >>      tosa_binfo.kernel_filename = kernel_filename; > >>      tosa_binfo.kernel_cmdline = kernel_cmdline; > >>      tosa_binfo.initrd_filename = initrd_filename; > >> diff --git a/target-arm/helper.c b/target-arm/helper.c > >> index 99e0394..63e5dc7 100644 > >> --- a/target-arm/helper.c > >> +++ b/target-arm/helper.c > >> @@ -207,7 +207,6 @@ void cpu_reset(CPUARMState *env) > >>  #else > >>      /* SVC mode with interrupts disabled.  */ > >>      env->uncached_cpsr = ARM_CPU_MODE_SVC | CPSR_A | CPSR_F | CPSR_I; > >> -    env->regs[15] = 0; > >>      /* On ARMv7-M the CPSR_I is the value of the PRIMASK register, and is > >>         clear at reset.  Initial SP and PC are loaded from ROM.  */ > >>      if (IS_M(env)) { > > > > This part doesn't. The register reset should be kept in case in case the > > test "if (info)" in main_cpu_reset() is false. > > All the "regs[15] = xyz" above are unneeded now as they are all used > in combination with arm_boot_kernel with a valid info struct. Agreed. > The last "env->regs[15] = 0" is redundant as the register is already > zeroed at that point. > Where is it zeroed? -- Aurelien Jarno GPG: 1024D/F1BCDB73 aurelien@aurel32.net http://www.aurel32.net