| Message ID | 1498728533-23160-3-git-send-email-frederic.konrad@adacore.com |
|---|---|
| State | New |
| Headers | show |
On 29 June 2017 at 10:28, KONRAD Frederic <frederic.konrad@adacore.com> wrote: > This fixes an odd bug when a ROM is present somewhere and an alias @0x00000000 > is pointing to the ROM. The "if (rom)" test fails and we don't get a valid reset > state. QEMU later crashes with an exception because the ARMv7-M starts with the > ARM instruction set. (eg: PC & 0x01 is 0). > > This patch uses memory_region_get_offset_within_address_space introduced before > to check if an alias doesn't point to a flash somewhere. > > Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com> This is awkward, because in the "we have a ROM but it's not been copied into memory yet" case, the only thing we have is the rom->addr, which is the address which the user's ROM blob said it ought to be loaded in at. If the user didn't actually provide a ROM blob that loads at 0 that seems a bit like a user error, and I don't think this patch will catch all the cases of that sort of mistake. For instance if address 0 is real flash and the high address alias is modelled by having the high address be the alias, then if the user passes us an ELF file saying "load to the high address" then this change won't catch that I think (because doing the memory_region_find/get_offset_within_address_space will return 0, which has already been tried). You'd need to somehow have a way to say "find all the addresses within this AS where this MR is mapped" and try them all... thanks -- PMM
On 06/29/2017 05:14 PM, Peter Maydell wrote: > On 29 June 2017 at 10:28, KONRAD Frederic <frederic.konrad@adacore.com> wrote: >> This fixes an odd bug when a ROM is present somewhere and an alias @0x00000000 >> is pointing to the ROM. The "if (rom)" test fails and we don't get a valid reset >> state. QEMU later crashes with an exception because the ARMv7-M starts with the >> ARM instruction set. (eg: PC & 0x01 is 0). >> >> This patch uses memory_region_get_offset_within_address_space introduced before >> to check if an alias doesn't point to a flash somewhere. >> >> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com> > > This is awkward, because in the "we have a ROM but it's not been > copied into memory yet" case, the only thing we have is the > rom->addr, which is the address which the user's ROM blob said > it ought to be loaded in at. If the user didn't actually provide > a ROM blob that loads at 0 that seems a bit like a user error, > and I don't think this patch will catch all the cases of that > sort of mistake. I don't think it's really a user mistake because on the real HW the alias is configurable.. at least in my case. There is a "jumper" setting to mirror either the Flash or the SRAM, etc. So the binaries isn't located at 0 but at the flash address 0x8000000 or some such. That's the case with u-boot and the precompiled examples I found for this stm32fxxxx board. For instance if address 0 is real flash and the > high address alias is modelled by having the high address be the > alias, then if the user passes us an ELF file saying "load to > the high address" then this change won't catch that I think > (because doing the memory_region_find/get_offset_within_address_space > will return 0, which has already been tried). You'd need to > somehow have a way to say "find all the addresses within this > AS where this MR is mapped" and try them all... This is more likely to be a user error :). Maybe we can load the ROM before the reset but that seems a lot more invasive.. BTW isn't there a trick with the ELF entry somewhere? Or is that for the Cortex-A? Thanks, Fred > > thanks > -- PMM >
On 29 June 2017 at 17:41, KONRAD Frederic <frederic.konrad@adacore.com> wrote: > On 06/29/2017 05:14 PM, Peter Maydell wrote: >> This is awkward, because in the "we have a ROM but it's not been >> copied into memory yet" case, the only thing we have is the >> rom->addr, which is the address which the user's ROM blob said >> it ought to be loaded in at. If the user didn't actually provide >> a ROM blob that loads at 0 that seems a bit like a user error, >> and I don't think this patch will catch all the cases of that >> sort of mistake. > > > I don't think it's really a user mistake because on the real HW > the alias is configurable.. at least in my case. > > There is a "jumper" setting to mirror either the Flash or the > SRAM, etc. So the binaries isn't located at 0 but at the flash > address 0x8000000 or some such. That's the case with u-boot and > the precompiled examples I found for this stm32fxxxx board. > >> For instance if address 0 is real flash and the >> high address alias is modelled by having the high address be the >> alias, then if the user passes us an ELF file saying "load to >> the high address" then this change won't catch that I think >> (because doing the memory_region_find/get_offset_within_address_space >> will return 0, which has already been tried). You'd need to >> somehow have a way to say "find all the addresses within this >> AS where this MR is mapped" and try them all... > > > This is more likely to be a user error :). Maybe we can load > the ROM before the reset but that seems a lot more invasive.. It's the same thing, though, right? If the user's ELF file says "vector table is at 0x8000000" then we should either (a) say that's a user error, or (b) handle it right, whether we implemented the QEMU model with the flash at 0 and the alias at 0x8000000, or with the flash at 0x8000000 and the alias at 0. > BTW isn't there a trick with the ELF entry somewhere? Or is that > for the Cortex-A? We don't honour the ELF entry point on M profile (arguably a bug) -- armv7m_load_kernel() ignores the entry point returned by load_elf() in the 'entry' variable. thanks -- PMM
On 06/29/2017 06:45 PM, Peter Maydell wrote: > On 29 June 2017 at 17:41, KONRAD Frederic <frederic.konrad@adacore.com> wrote: >> On 06/29/2017 05:14 PM, Peter Maydell wrote: >>> This is awkward, because in the "we have a ROM but it's not been >>> copied into memory yet" case, the only thing we have is the >>> rom->addr, which is the address which the user's ROM blob said >>> it ought to be loaded in at. If the user didn't actually provide >>> a ROM blob that loads at 0 that seems a bit like a user error, >>> and I don't think this patch will catch all the cases of that >>> sort of mistake. >> >> >> I don't think it's really a user mistake because on the real HW >> the alias is configurable.. at least in my case. >> >> There is a "jumper" setting to mirror either the Flash or the >> SRAM, etc. So the binaries isn't located at 0 but at the flash >> address 0x8000000 or some such. That's the case with u-boot and >> the precompiled examples I found for this stm32fxxxx board. >> >>> For instance if address 0 is real flash and the >>> high address alias is modelled by having the high address be the >>> alias, then if the user passes us an ELF file saying "load to >>> the high address" then this change won't catch that I think >>> (because doing the memory_region_find/get_offset_within_address_space >>> will return 0, which has already been tried). You'd need to >>> somehow have a way to say "find all the addresses within this >>> AS where this MR is mapped" and try them all... >> >> >> This is more likely to be a user error :). Maybe we can load >> the ROM before the reset but that seems a lot more invasive.. > > It's the same thing, though, right? If the user's ELF file > says "vector table is at 0x8000000" then we should either > (a) say that's a user error, or > (b) handle it right, whether we implemented the QEMU model with > the flash at 0 and the alias at 0x8000000, or with the flash > at 0x8000000 and the alias at 0. Hi Peter, Fondamentaly yes, it is the same.. but it seems really strange to me to load the elf in the alias. If I choose (a) I'll need to objcpy all the elf to 0 or modify the build script which should work on the real board.. This seems not really a good option to me. If I choose (b) I won't be able to load it to SRAM and it is basically the same result I'll need to move or modify the config. Thanks, Fred > >> BTW isn't there a trick with the ELF entry somewhere? Or is that >> for the Cortex-A? > > We don't honour the ELF entry point on M profile (arguably > a bug) -- armv7m_load_kernel() ignores the entry point > returned by load_elf() in the 'entry' variable. > > thanks > -- PMM >
On 30 June 2017 at 09:24, KONRAD Frederic <frederic.konrad@adacore.com> wrote: > On 06/29/2017 06:45 PM, Peter Maydell wrote: >> It's the same thing, though, right? If the user's ELF file >> says "vector table is at 0x8000000" then we should either >> (a) say that's a user error, or >> (b) handle it right, whether we implemented the QEMU model with >> the flash at 0 and the alias at 0x8000000, or with the flash >> at 0x8000000 and the alias at 0. > Fondamentaly yes, it is the same.. but it seems really strange to > me to load the elf in the alias. The user creating the ELF file has no idea whether we modelled the flash at 0 and the alias at highmem or the other way around -- that is an implementation detail that should be completely invisible to the user. My two suggestions are based on that point -- either we should treat "ELF loaded at highmem" as always wrong, or we should always support it. > If I choose (a) I'll need to objcpy all the elf to 0 or modify > the build script which should work on the real board.. This seems > not really a good option to me. I agree that I don't like this. > If I choose (b) I won't be able to load it to SRAM and it is > basically the same result I'll need to move or modify the config. I don't understand this, though. Option (b) is probably painful to implement (I don't have a good idea of how to do it) but it ought to mean that the ELF files that work on the board also work for QEMU (regardless of how the board model implemented the aliased flash). thanks -- PMM
On 06/30/2017 11:06 AM, Peter Maydell wrote: > On 30 June 2017 at 09:24, KONRAD Frederic <frederic.konrad@adacore.com> wrote: >> On 06/29/2017 06:45 PM, Peter Maydell wrote: >>> It's the same thing, though, right? If the user's ELF file >>> says "vector table is at 0x8000000" then we should either >>> (a) say that's a user error, or >>> (b) handle it right, whether we implemented the QEMU model with >>> the flash at 0 and the alias at 0x8000000, or with the flash >>> at 0x8000000 and the alias at 0. > >> Fondamentaly yes, it is the same.. but it seems really strange to >> me to load the elf in the alias. > > The user creating the ELF file has no idea whether we > modelled the flash at 0 and the alias at highmem or > the other way around -- that is an implementation detail > that should be completely invisible to the user. > My two suggestions are based on that point -- either we > should treat "ELF loaded at highmem" as always wrong, or > we should always support it. > >> If I choose (a) I'll need to objcpy all the elf to 0 or modify >> the build script which should work on the real board.. This seems >> not really a good option to me. > > I agree that I don't like this. > >> If I choose (b) I won't be able to load it to SRAM and it is >> basically the same result I'll need to move or modify the config. > > I don't understand this, though. Option (b) is probably painful > to implement (I don't have a good idea of how to do it) but > it ought to mean that the ELF files that work on the board > also work for QEMU (regardless of how the board model > implemented the aliased flash). > Yes that's exactly what I want. Basically the 0x00000000 alias can point to the SRAM or the ROM during the reset depending on some boot config. The ELF is directly loaded in the ROM or in the SRAM and my patch allows to fetch the two first words in the reset handler to make it work for any boot config. For example this devices p69: www.st.com/resource/en/reference_manual/DM00031020.pdf Thanks, Fred > thanks > -- PMM >
On 3 July 2017 at 08:31, KONRAD Frederic <frederic.konrad@adacore.com> wrote: > On 06/30/2017 11:06 AM, Peter Maydell wrote: >> On 30 June 2017 at 09:24, KONRAD Frederic <frederic.konrad@adacore.com> >> wrote: >>> If I choose (b) I won't be able to load it to SRAM and it is >>> basically the same result I'll need to move or modify the config. >> >> >> I don't understand this, though. Option (b) is probably painful >> to implement (I don't have a good idea of how to do it) but >> it ought to mean that the ELF files that work on the board >> also work for QEMU (regardless of how the board model >> implemented the aliased flash). >> > > Yes that's exactly what I want. > > Basically the 0x00000000 alias can point to the SRAM or the ROM > during the reset depending on some boot config. The ELF is > directly loaded in the ROM or in the SRAM and my patch allows to > fetch the two first words in the reset handler to make it work > for any boot config. Yes, but it only works if you implemented it that way round, and not for board implementations which put the real device at 0 and the alias at high memory. I'd like a fix which deals with all of this, not just with the particular arrangement your board implementation has. thanks -- PMM
On 07/03/2017 10:51 AM, Peter Maydell wrote: > On 3 July 2017 at 08:31, KONRAD Frederic <frederic.konrad@adacore.com> wrote: >> On 06/30/2017 11:06 AM, Peter Maydell wrote: >>> On 30 June 2017 at 09:24, KONRAD Frederic <frederic.konrad@adacore.com> >>> wrote: >>>> If I choose (b) I won't be able to load it to SRAM and it is >>>> basically the same result I'll need to move or modify the config. >>> >>> >>> I don't understand this, though. Option (b) is probably painful >>> to implement (I don't have a good idea of how to do it) but >>> it ought to mean that the ELF files that work on the board >>> also work for QEMU (regardless of how the board model >>> implemented the aliased flash). >>> >> >> Yes that's exactly what I want. >> >> Basically the 0x00000000 alias can point to the SRAM or the ROM >> during the reset depending on some boot config. The ELF is >> directly loaded in the ROM or in the SRAM and my patch allows to >> fetch the two first words in the reset handler to make it work >> for any boot config. > > Yes, but it only works if you implemented it that way > round, and not for board implementations which put the > real device at 0 and the alias at high memory. I'd like a fix > which deals with all of this, not just with the particular > arrangement your board implementation has. Ok got it, I'll check if I can do something clean which can handle both ways. Fred > > thanks > -- PMM >
diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 28a9141..b8afd97 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -201,6 +201,20 @@ static void arm_cpu_reset(CPUState *s) /* Load the initial SP and PC from the vector table at address 0 */ rom = rom_ptr(0); + + if (!rom) { + /* Sometimes address 0x00000000 is an alias to a flash which + * actually have a ROM. + */ + MemoryRegionSection section; + hwaddr offset = 0; + + section = memory_region_find(s->as->root, 0, 8); + offset = memory_region_get_offset_within_address_space(section.mr); + memory_region_unref(section.mr); + rom = rom_ptr(offset); + } + if (rom) { /* Address zero is covered by ROM which hasn't yet been * copied into physical memory.
This fixes an odd bug when a ROM is present somewhere and an alias @0x00000000 is pointing to the ROM. The "if (rom)" test fails and we don't get a valid reset state. QEMU later crashes with an exception because the ARMv7-M starts with the ARM instruction set. (eg: PC & 0x01 is 0). This patch uses memory_region_get_offset_within_address_space introduced before to check if an alias doesn't point to a flash somewhere. Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com> --- target/arm/cpu.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)