| Message ID | 1400105145-6628-1-git-send-email-drambo@broadcom.com |
|---|---|
| State | New |
| Headers | show |
Hello Darwin, On wo, 2014-05-14 at 15:05 -0700, Darwin Rambo wrote: > +#ifdef CONFIG_ARM64 > + /* > + * Fix relocation if u-boot is not on an aligned address. > + */ > + { > + int offset = CONFIG_SYS_TEXT_BASE % 4096; > + if (offset) { > + addr += offset; > + debug("Relocation Addr bumped to 0x%08lx\n", addr); > + } > + } > +#endif > + Do you really want to check a define at runtime? Placement is typically at the end of RAM and allocation goes down, not up as in this patch. Aren't you overlapping memory here? > > static int setup_reloc(void) > { > +#ifdef CONFIG_ARM64 > + /* > + * Fix relocation if u-boot is not on an aligned address. > + */ > + int offset = CONFIG_SYS_TEXT_BASE % 4096; > + if (offset) { > + gd->relocaddr += offset; > + debug("Relocation Addr bumped to 0x%08lx\n", gd->relocaddr); > + } > +#endif > gd->reloc_off = gd->relocaddr - CONFIG_SYS_TEXT_BASE; > memcpy(gd->new_gd, (char *)gd, sizeof(gd_t)); > This is a generic file, hell breaks loose if you start using arch / board / pre bootloader specific workarounds here afaiac. lucky for you, I am not a u-boot maintainer, but this looks at least a bit weird, glancing at it. Regards, Jeroen
Dear Darwin Rambo, In message <1400105145-6628-1-git-send-email-drambo@broadcom.com> you wrote: > If an earlier loader stage requires an image header and a specific > offset, then u-boot's base address (CONFIG_SYS_TEXT_BASE) may be > advanced beyond an aligned address. In this case the relocation Sorry, I cannot parse that. CONFIG_SYS_TEXT_BASE is a compile time constant, it cannot be "advanced" by a loader. Do you mean that some loader loads U-Boot to an incorrect address? Well, in this case the loader should be fixed, or? > This change is done under CONFIG_ARM64 conditional compilation > because it has only been tested there and may not be appropriate > for other architectures. In any case, any such changes (if there should be agreement that they are actually useful) should be done in an architecture-neutral way. Implementing it for one specific architecture only is wrong. Best regards, Wolfgang Denk
On 14-05-14 09:26 PM, Wolfgang Denk wrote: > Dear Darwin Rambo, > > In message <1400105145-6628-1-git-send-email-drambo@broadcom.com> you wrote: >> If an earlier loader stage requires an image header and a specific >> offset, then u-boot's base address (CONFIG_SYS_TEXT_BASE) may be >> advanced beyond an aligned address. In this case the relocation > Sorry, I cannot parse that. CONFIG_SYS_TEXT_BASE is a compile time > constant, it cannot be "advanced" by a loader. Do you mean that some > loader loads U-Boot to an incorrect address? Well, in this case the > loader should be fixed, or? Thank you for your comments. I mean that the loader loads u-boot to it's correct address, which is offset by a small amount because of a previous header requiring alignment. Here's an example. u-boot is compiled to run at 0x88000020 because we want to put a small header in front of the image, which starts at 0x88000000 and needs to be aligned for its own reasons. Now for arm64, I believe that u-boot cannot normally be positioned at any alignment less than 0x800 hex. So u-boot would normally run at addresses like 0x88000000, 0x88000800, 0x88001000, etc. And in these cases the relocation works fine. But if we want to position u-boot at a smaller offset than 0x800, the symbol relocation breaks for arm64. It turns out that there is a trivial fix so that u-boot can run at smaller offset addresses, which I have provided here, is tested, and solves our problem nicely, but only for arm64 right now. > >> This change is done under CONFIG_ARM64 conditional compilation >> because it has only been tested there and may not be appropriate >> for other architectures. > In any case, any such changes (if there should be agreement that they > are actually useful) should be done in an architecture-neutral way. > Implementing it for one specific architecture only is wrong. Yes, I agree, but I am not sure if this is a arm64-only problem or not. Armv7 doesn't show this problem, and I can't test other architectures for their alignment issues. So I thought that I would at least show the fix for arm64 so we can decide if and how to proceed. Any suggestions you can provide on how to proceed would be appreciated. And if the fix is not suitable for upstreaming, then we should know it. Is there a way to have architecture specific hooks like this called from the generic common/board_f.c? The fix is also in arch/arm/lib/board.c but it sounds like that file might be disappearing. Also there might be a generic fix possible that works for all architectures (by removing the ifdef CONFIG_ARM64), but I don't have the resources to test them. Maybe it would be best to decide if we want to support this feature or not first. Thanks! Regards, Darwin > > Best regards, > > Wolfgang Denk >
On 14-05-14 03:41 PM, Jeroen Hofstee wrote: > Hello Darwin, > > On wo, 2014-05-14 at 15:05 -0700, Darwin Rambo wrote: > >> +#ifdef CONFIG_ARM64 >> + /* >> + * Fix relocation if u-boot is not on an aligned address. >> + */ >> + { >> + int offset = CONFIG_SYS_TEXT_BASE % 4096; >> + if (offset) { >> + addr += offset; >> + debug("Relocation Addr bumped to 0x%08lx\n", addr); >> + } >> + } >> +#endif >> + > Do you really want to check a define at runtime? Placement is typically > at the end of RAM and allocation goes down, not up as in this patch. > Aren't you overlapping memory here? Yes, I wanted the runtime check since the adjustment to the relocation address is also done at runtime. There is no overlap here. The reason is that the original masking operation to mask to a 4K boundary removed the small offset and backed up too far. So adding the lost offset is guaranteed to not overlap, and furthermore, correct the relocation offset so that arm64 images can run at smaller alignments than we normally use. This might even be a generic fix but can't be tested easily by me. > >> >> static int setup_reloc(void) >> { >> +#ifdef CONFIG_ARM64 >> + /* >> + * Fix relocation if u-boot is not on an aligned address. >> + */ >> + int offset = CONFIG_SYS_TEXT_BASE % 4096; >> + if (offset) { >> + gd->relocaddr += offset; >> + debug("Relocation Addr bumped to 0x%08lx\n", gd->relocaddr); >> + } >> +#endif >> gd->reloc_off = gd->relocaddr - CONFIG_SYS_TEXT_BASE; >> memcpy(gd->new_gd, (char *)gd, sizeof(gd_t)); >> > This is a generic file, hell breaks loose if you start using arch / > board / pre bootloader specific workarounds here afaiac. I don't disagree with this statement. Please see my other comments to Wolfgang on this topic. > > lucky for you, I am not a u-boot maintainer, but this looks at least a > bit weird, glancing at it. > > Regards, > Jeroen > Thanks for your comments Jeroen. They are appreciated. Darwin
Dear Darwin, In message <5374CC3B.7000504@broadcom.com> you wrote: > > I mean that the loader loads u-boot to it's correct address, which is > offset by a small amount because of a previous header requiring alignment. > Here's an example. u-boot is compiled to run at 0x88000020 because we want > to put a small header in front of the image, which starts at 0x88000000 and > needs to be aligned for its own reasons. Now for arm64, I believe that u-boot > cannot normally be positioned at any alignment less than 0x800 hex. So I thing you have some misunderstanding of the meaning of the start address of the image versus the entry point address. These are not related. You can add any headers or padding you like to the image, and put the entry point at an arbitrary address. The restrictions we usually face are due to the bootmodes of the SoC, which may start from a fixed reset address (whwere we then must make sure that this is also the entry point address in the image), or where the ROM boot loader may have specific requirements. If you add some custom image header, you can also start at some random entry point addres.. Just adapt your linker script as needed. > And in these cases the relocation works fine. But if we want to position u-boot > at a smaller offset than 0x800, the symbol relocation breaks for arm64. It > turns out that there is a trivial fix so that u-boot can run at smaller offset > addresses, which I have provided here, is tested, and solves our problem nicely, > but only for arm64 right now. No, your patch is highly dubious actually. Note that the entry point address which we are talking about is where code execution starts _before_ relocation. Relocation is a totally different topic. The address where we relocate to gets computed at runtime, and does not depend on CONFIG_SYS_TEXT_BASE at all. Also, as had been pointed out before, memory allocation happens top down, so any adjustments you want to make must be to lower addresses, never upward. > Yes, I agree, but I am not sure if this is a arm64-only problem or not. Unfortunaltely you don't explain what you really want to do. We have the same task (loading the U-Boot image and starting it) in many, nmany configurations either when a ROM boot loader performs the loading, or where the SPL does it. This is a well understood procedure, and it has no such problem as you claim to have. I suspect you have bugs elsewhere in your port, may it be the linker script or your implementation of a special image header or whatever. > can provide on how to proceed would be appreciated. And if the fix is > not suitable for upstreaming, then we should know it. Please understand that this is not a question of suitable for upstreaming or not, it is a matter of correct or not. I am pretty much convinced that your modifcation cannot be right. > Also there might be a generic fix possible that works for all architectures > (by removing the ifdef CONFIG_ARM64), but I don't have the resources to test > them. Maybe it would be best to decide if we want to support this feature or > not first. Thanks! Please define "this feature" in an exact way, and why you think it would cause any problems. Best regards, Wolfgang Denk
Dear Darwin, In message <5374CD55.3010703@broadcom.com> you wrote: > > > Do you really want to check a define at runtime? Placement is typically > > at the end of RAM and allocation goes down, not up as in this patch. > > Aren't you overlapping memory here? > > Yes, I wanted the runtime check since the adjustment to the relocation > address is also done at runtime. This makes no sense to me. CONFIG_SYS_TEXT_BASE is a compile time constant. So the result of all this is always known at compile time, too. I feel you misunderstand that CONFIG_SYS_TEXT_BASE is just the start address of the text segment. If you want to offset this by a specific amount, you can just define this as needed. > There is no overlap here. The reason is that the original masking operation > to mask to a 4K boundary removed the small offset and backed up too far. So > adding the lost offset is guaranteed to not overlap, and furthermore, correct > the relocation offset so that arm64 images can run at smaller alignments than > we normally use. This might even be a generic fix but can't be tested easily > by me. Argh... This is black magic depending on specific properties of your process (which you don;t really explain). Sorry, but this is a full NAK for code that is build on sand like this. Best regards, Wolfgang Denk
On 14-05-15 08:21 AM, Wolfgang Denk wrote: > Dear Darwin, > > In message <5374CD55.3010703@broadcom.com> you wrote: >>> Do you really want to check a define at runtime? Placement is typically >>> at the end of RAM and allocation goes down, not up as in this patch. >>> Aren't you overlapping memory here? >> Yes, I wanted the runtime check since the adjustment to the relocation >> address is also done at runtime. > This makes no sense to me. CONFIG_SYS_TEXT_BASE is a compile time > constant. So the result of all this is always known at compile time, > too. I feel you misunderstand that CONFIG_SYS_TEXT_BASE is just the > start address of the text segment. If you want to offset this by a > specific amount, you can just define this as needed. May I respectfully ask that you please bear with me a just a bit longer so I can try to explain things better? CONFIG_SYS_TEXT_BASE is also used by the relocation calculations at runtime to determine the relocation offset, which is also used for the symbol fixup logic. It is known at compile time, but is used at runtime by the stock u-boot code to determine the relocation offset. I am doing nothing more than existing code in this regard. If I set it to 0x88000020, then the code crashes because the symbol fixup depends on the relocation offset which is now wrong. The reason is that the relocation offset calculated in the code doesn't account for this offset. If we adjust the relocation address, then the relocation offset is now correct and the symbol fixups work perfectly. Here's an example: CONFIG_SYS_TEXT_BASE = 0x88000020 relocated address = 0xfffa8000 (This 4K alignment assumption breaks the relocation) relocation offset = 0x77fa7fe0 (This is now wrong and the relocation breaks) My patch does the following: CONFIG_SYS_TEXT_BASE = 0x88000020 relocated address = 0xfffa8020 relocation offset = 0x77fa8000 (symbol fixups now work) So this patch just gives us a way to run u-boot at text sections with smaller alignments. In the past I never really understood why CONFIG_SYS_TEXT_BASE had to be on specific alignments like XXXXX000 for our architecture and the symbol fixup issue helps to explain this. I know this is not the normal use case but it does fix the crash in the symbol relocation with arm64 and allows u-boot to be more flexible in it's text alignment requirements. Thanks. Best regards, Darwin > >> There is no overlap here. The reason is that the original masking operation >> to mask to a 4K boundary removed the small offset and backed up too far. So >> adding the lost offset is guaranteed to not overlap, and furthermore, correct >> the relocation offset so that arm64 images can run at smaller alignments than >> we normally use. This might even be a generic fix but can't be tested easily >> by me. > Argh... This is black magic depending on specific properties of your > process (which you don;t really explain). Sorry, but this is a full > NAK for code that is build on sand like this. > > Best regards, > > Wolfgang Denk >
Dear Darwin, In message <5374E64B.1060104@broadcom.com> you wrote: > > > This makes no sense to me. CONFIG_SYS_TEXT_BASE is a compile time > > constant. So the result of all this is always known at compile time, > > too. I feel you misunderstand that CONFIG_SYS_TEXT_BASE is just the > > start address of the text segment. If you want to offset this by a > > specific amount, you can just define this as needed. > > May I respectfully ask that you please bear with me a just a bit longer so > I can try to explain things better? > > CONFIG_SYS_TEXT_BASE is also used by the relocation calculations at runtime > to determine the relocation offset, which is also used for the symbol fixup > logic. It is known at compile time, but is used at runtime by the stock > u-boot code to determine the relocation offset. I am doing nothing more than > existing code in this regard. Let's have a look at your proposed code: >> + int offset = CONFIG_SYS_TEXT_BASE % 4096; >> + if (offset) { >> + addr += offset; >> + debug("Relocation Addr bumped to 0x%08lx\n", addr); >> + } > If I set it to 0x88000020, then the code crashes because the symbol fixup > depends on the relocation offset which is now wrong. The reason is that the > relocation offset calculated in the code doesn't account for this offset. > If we adjust the relocation address, then the relocation offset is now > correct and the symbol fixups work perfectly. First, please keep in mind that you cannot set CONFIG_SYS_TEXT_BASE in arbitrary ways, at least not without adjusting your linker script accordingly; also you may have limitations due to the way how the code gets loaded / booted. _If_ your code is correct, then the target address computed for the relocation is completely irrelevant. > Here's an example: > CONFIG_SYS_TEXT_BASE = 0x88000020 > relocated address = 0xfffa8000 (This 4K alignment assumption breaks the relocation) > relocation offset = 0x77fa7fe0 (This is now wrong and the relocation breaks) > > My patch does the following: > CONFIG_SYS_TEXT_BASE = 0x88000020 > relocated address = 0xfffa8020 > relocation offset = 0x77fa8000 (symbol fixups now work) You are mixing up copying code to another address range and relocating symbols. > So this patch just gives us a way to run u-boot at text sections with > smaller alignments. In the past I never really understood why > CONFIG_SYS_TEXT_BASE had to be on specific alignments like XXXXX000 for > our architecture and the symbol fixup issue helps to explain this. In many cases there is no strict reason. When storing the images in NOR flash or other block oriented storage it is usually convenient to align them to sector boundaaries. In systems where you have a MMU on, it may be useful / convenient / necessary to have it aligned to page boundaries. But al this is totally irrelevant here. Consider the address arbitrarily chosen, it does not matter. Also, moving the relocation address around by arbitrary amounts (that are big enough not to cause alignment issues, say anything that is a multiple of the cache line size) does not change anything. If there are problems, these are somewhere in your implementation, and not in the generic code. > I know this is not the normal use case but it does fix the crash in the > symbol relocation with arm64 and allows u-boot to be more flexible in > it's text alignment requirements. You ask for a "flexibility" that actually is already there; it's just conveniend when debugging etc. to have "nice" addresses. If there is something not working, it's in your code. Setting CONFIG_SYS_TEXT_BASE to something like 0x88000020 is extremly fishy. If you want to add some header data to your image, you should not shift the text segment, but rather include your header in the start of the text segment. Or keep it completely separate, without messing with CONFIG_SYS_TEXT_BASE. Best regards, Wolfgang Denk
Hi Wolfgang, Darwin, On Thu, 15 May 2014 21:19:57 +0200, Wolfgang Denk <wd@denx.de> wrote: > Setting CONFIG_SYS_TEXT_BASE to something like 0x88000020 is extremly > fishy. If you want to add some header data to your image, you should > not shift the text segment, but rather include your header in the > start of the text segment. Or keep it completely separate, without > messing with CONFIG_SYS_TEXT_BASE. Back to the origin of the discussion and patch: Darwin, can you describe the actual technical difficulty which caused you to you write and submitting this patch? > Best regards, > > Wolfgang Denk Amicalement,
Hi Albert, The previous stage bootloader (which I had no control over) wanted it's header to be aligned to a 512 byte MMC block boundary, presumably since this allowed DMA operations without copy/shifting. At the same time, I didn't want to hack a header into start.S because I didn't want to carry another downstream patch. So I investigated if I could shift u-boot's base address as a feature that would allow an aligned header to be used without the start.S patch. I know that a custom header patch to start.S would work, and that a header plus padding will also work. But I found out that you can align the base on certain smaller offsets if you keep the relocation offset at nice boundaries like 0x1000 and if the relocation offset is a multiple of the maximum alignment requirements of the image. The original patch I submitted didn't handle an end condition properly, was ARM64-specific (wasn't tested on other architectures), and because the patch was NAK'd, I didn't bother to submit a v2 patch and consider the idea to be dead. I'm happy to abandon the patch. I hope this helps. Best regards, Darwin On 14-05-26 02:50 AM, Albert ARIBAUD wrote: > Hi Wolfgang, Darwin, > > On Thu, 15 May 2014 21:19:57 +0200, Wolfgang Denk <wd@denx.de> wrote: > >> Setting CONFIG_SYS_TEXT_BASE to something like 0x88000020 is extremly >> fishy. If you want to add some header data to your image, you should >> not shift the text segment, but rather include your header in the >> start of the text segment. Or keep it completely separate, without >> messing with CONFIG_SYS_TEXT_BASE. > > Back to the origin of the discussion and patch: > > Darwin, can you describe the actual technical difficulty which caused > you to you write and submitting this patch? > >> Best regards, >> >> Wolfgang Denk > > Amicalement, >
Hi Darwin, On Mon, 26 May 2014 09:11:35 -0700, Darwin Rambo <drambo@broadcom.com> wrote: > Hi Albert, > > The previous stage bootloader (which I had no control over) wanted it's > header to be aligned to a 512 byte MMC block boundary, presumably since > this allowed DMA operations without copy/shifting. At the same time, I > didn't want to hack a header into start.S because I didn't want to carry > another downstream patch. So I investigated if I could shift u-boot's > base address as a feature that would allow an aligned header to be used > without the start.S patch. > > I know that a custom header patch to start.S would work, and that a > header plus padding will also work. But I found out that you can align > the base on certain smaller offsets if you keep the relocation offset at > nice boundaries like 0x1000 and if the relocation offset is a multiple > of the maximum alignment requirements of the image. > > The original patch I submitted didn't handle an end condition properly, > was ARM64-specific (wasn't tested on other architectures), and because > the patch was NAK'd, I didn't bother to submit a v2 patch and consider > the idea to be dead. I'm happy to abandon the patch. I hope this helps. Thanks. If I understand correctly, your target has a requirement for storing the image on a 512-byte boundary. But how does this affect the loading of the image into RAM, where the requirement is only that the vectors table be 32-bytes aligned? I mean, if you store the image in MMC at offset 0x200 (thus satisfying the 512-byte boundary requirement) and load it to, say, offset 0x10020 in RAM, how is it a problem for your target? If my example above inadequately represents the issue, then can you please provide a similar but adequate example, a failure case scenario, so that I can hve a correct understanding of the problem? > Best regards, > Darwin Amicalement,
On 14-06-02 12:26 AM, Albert ARIBAUD wrote: > Hi Darwin, > > On Mon, 26 May 2014 09:11:35 -0700, Darwin Rambo <drambo@broadcom.com> > wrote: > >> Hi Albert, >> >> The previous stage bootloader (which I had no control over) wanted it's >> header to be aligned to a 512 byte MMC block boundary, presumably since >> this allowed DMA operations without copy/shifting. At the same time, I >> didn't want to hack a header into start.S because I didn't want to carry >> another downstream patch. So I investigated if I could shift u-boot's >> base address as a feature that would allow an aligned header to be used >> without the start.S patch. >> >> I know that a custom header patch to start.S would work, and that a >> header plus padding will also work. But I found out that you can align >> the base on certain smaller offsets if you keep the relocation offset at >> nice boundaries like 0x1000 and if the relocation offset is a multiple >> of the maximum alignment requirements of the image. >> >> The original patch I submitted didn't handle an end condition properly, >> was ARM64-specific (wasn't tested on other architectures), and because >> the patch was NAK'd, I didn't bother to submit a v2 patch and consider >> the idea to be dead. I'm happy to abandon the patch. I hope this helps. > > Thanks. > > If I understand correctly, your target has a requirement for storing > the image on a 512-byte boundary. But how does this affect the loading > of the image into RAM, where the requirement is only that the vectors > table be 32-bytes aligned? I mean, if you store the image in MMC at > offset 0x200 (thus satisfying the 512-byte boundary requirement) and > load it to, say, offset 0x10020 in RAM, how is it a problem for > your target? > > If my example above inadequately represents the issue, then can you > please provide a similar but adequate example, a failure case scenario, > so that I can hve a correct understanding of the problem? Hi Albert, The constraints I have that I can't change, are that - the 32 byte header is postprocessed and prepended to the image after the build is complete - the header is at a 512 byte alignment in MMC - the header and image are copied to SDRAM to an alignment like 0x88000000. Thus the u-boot image is linked at and starts at 0x88000020. - the vectors need to be 0x800 aligned for armv8 (.align 11 directive) So the failure case is that when the relocation happens, it relocates to a 0x1000 alignment, say something like 0xffffa000. The relocation offset is not a multiple of 0x1000 (0xffffa000 - 0x88000020) and the relocation fails. Adjusting the relocation offset to a multiple of 0x1000 (by making the relocation address end in 0xNNNNN020) fixes the issues and allows u-boot to relocate and run from this address without failing. I hope this helps explain it a bit better. Best regards, Darwin > >> Best regards, >> Darwin > > Amicalement, >
Hi Darwin, On Mon, 2 Jun 2014 17:37:25 -0700, Darwin Rambo <drambo@broadcom.com> wrote: > > > On 14-06-02 12:26 AM, Albert ARIBAUD wrote: > > Hi Darwin, > > > > On Mon, 26 May 2014 09:11:35 -0700, Darwin Rambo <drambo@broadcom.com> > > wrote: > > > >> Hi Albert, > >> > >> The previous stage bootloader (which I had no control over) wanted it's > >> header to be aligned to a 512 byte MMC block boundary, presumably since > >> this allowed DMA operations without copy/shifting. At the same time, I > >> didn't want to hack a header into start.S because I didn't want to carry > >> another downstream patch. So I investigated if I could shift u-boot's > >> base address as a feature that would allow an aligned header to be used > >> without the start.S patch. > >> > >> I know that a custom header patch to start.S would work, and that a > >> header plus padding will also work. But I found out that you can align > >> the base on certain smaller offsets if you keep the relocation offset at > >> nice boundaries like 0x1000 and if the relocation offset is a multiple > >> of the maximum alignment requirements of the image. > >> > >> The original patch I submitted didn't handle an end condition properly, > >> was ARM64-specific (wasn't tested on other architectures), and because > >> the patch was NAK'd, I didn't bother to submit a v2 patch and consider > >> the idea to be dead. I'm happy to abandon the patch. I hope this helps. > > > > Thanks. > > > > If I understand correctly, your target has a requirement for storing > > the image on a 512-byte boundary. But how does this affect the loading > > of the image into RAM, where the requirement is only that the vectors > > table be 32-bytes aligned? I mean, if you store the image in MMC at > > offset 0x200 (thus satisfying the 512-byte boundary requirement) and > > load it to, say, offset 0x10020 in RAM, how is it a problem for > > your target? > > > > If my example above inadequately represents the issue, then can you > > please provide a similar but adequate example, a failure case scenario, > > so that I can hve a correct understanding of the problem? > > Hi Albert, > > The constraints I have that I can't change, are that > - the 32 byte header is postprocessed and prepended to the image after > the build is complete > - the header is at a 512 byte alignment in MMC > - the header and image are copied to SDRAM to an alignment like > 0x88000000. Thus the u-boot image is linked at and starts at 0x88000020. > - the vectors need to be 0x800 aligned for armv8 (.align 11 directive) So far, so good -- I understand that the link-time location of the vectors table is incorrect. > So the failure case is that when the relocation happens, it relocates to > a 0x1000 alignment, say something like 0xffffa000. The relocation offset > is not a multiple of 0x1000 (0xffffa000 - 0x88000020) and the relocation > fails. What does "relocation fails" mean exactly, i.e., where and how exactly does the relocation code behave differently from expected? I'm asking because I don't understand why the relocation offset should be a multiple of 0x1000. > Adjusting the relocation offset to a multiple of 0x1000 (by > making the relocation address end in 0xNNNNN020) fixes the issues and > allows u-boot to relocate and run from this address without failing. I > hope this helps explain it a bit better. I do understand, however, that if the relocation offset must indeed be a multiple of 0x1000, then obviously the vectors table will end up as misaligned as it was before relocation. Also, personally I would like it if the vectors table was always aligned as it should, and there are at least three other boards which require a prefix/header before their vectors table, as Masahiro (cc:) indicated recently, so that makes the problem a generic one: how to properly integrate a header in-image (as opposed to an out-of-image one, which is just a matter of doing a 'cat', so to speak. Therefore I'd like a generic solution to this, where the header is prepended *and* aligned properly without breaking the start symbol alignment constraints. This /might/ be possible by cleverly adding a '.header' or '.signature' section to the linker script, possibly doing a two-stage link; but this should not require the source code to contain ad hoc relocation tricks. Let me tinker with it in the next few days; I'll try and come up with a clean and generic solution to this "in-code header" question. Thanks again for your explanation! > Best regards, > Darwin Amicalement,
On 14-06-09 03:23 AM, Albert ARIBAUD wrote: > Hi Darwin, > > On Mon, 2 Jun 2014 17:37:25 -0700, Darwin Rambo <drambo@broadcom.com> > wrote: > >> >> >> On 14-06-02 12:26 AM, Albert ARIBAUD wrote: >>> Hi Darwin, >>> >>> On Mon, 26 May 2014 09:11:35 -0700, Darwin Rambo <drambo@broadcom.com> >>> wrote: >>> >>>> Hi Albert, >>>> >>>> The previous stage bootloader (which I had no control over) wanted it's >>>> header to be aligned to a 512 byte MMC block boundary, presumably since >>>> this allowed DMA operations without copy/shifting. At the same time, I >>>> didn't want to hack a header into start.S because I didn't want to carry >>>> another downstream patch. So I investigated if I could shift u-boot's >>>> base address as a feature that would allow an aligned header to be used >>>> without the start.S patch. >>>> >>>> I know that a custom header patch to start.S would work, and that a >>>> header plus padding will also work. But I found out that you can align >>>> the base on certain smaller offsets if you keep the relocation offset at >>>> nice boundaries like 0x1000 and if the relocation offset is a multiple >>>> of the maximum alignment requirements of the image. >>>> >>>> The original patch I submitted didn't handle an end condition properly, >>>> was ARM64-specific (wasn't tested on other architectures), and because >>>> the patch was NAK'd, I didn't bother to submit a v2 patch and consider >>>> the idea to be dead. I'm happy to abandon the patch. I hope this helps. >>> >>> Thanks. >>> >>> If I understand correctly, your target has a requirement for storing >>> the image on a 512-byte boundary. But how does this affect the loading >>> of the image into RAM, where the requirement is only that the vectors >>> table be 32-bytes aligned? I mean, if you store the image in MMC at >>> offset 0x200 (thus satisfying the 512-byte boundary requirement) and >>> load it to, say, offset 0x10020 in RAM, how is it a problem for >>> your target? >>> >>> If my example above inadequately represents the issue, then can you >>> please provide a similar but adequate example, a failure case scenario, >>> so that I can hve a correct understanding of the problem? >> >> Hi Albert, >> >> The constraints I have that I can't change, are that >> - the 32 byte header is postprocessed and prepended to the image after >> the build is complete >> - the header is at a 512 byte alignment in MMC >> - the header and image are copied to SDRAM to an alignment like >> 0x88000000. Thus the u-boot image is linked at and starts at 0x88000020. >> - the vectors need to be 0x800 aligned for armv8 (.align 11 directive) > > So far, so good -- I understand that the link-time location of the > vectors table is incorrect. > >> So the failure case is that when the relocation happens, it relocates to >> a 0x1000 alignment, say something like 0xffffa000. The relocation offset >> is not a multiple of 0x1000 (0xffffa000 - 0x88000020) and the relocation >> fails. > > What does "relocation fails" mean exactly, i.e., where and how exactly > does the relocation code behave differently from expected? I'm asking > because I don't understand why the relocation offset should be a > multiple of 0x1000. > >> Adjusting the relocation offset to a multiple of 0x1000 (by >> making the relocation address end in 0xNNNNN020) fixes the issues and >> allows u-boot to relocate and run from this address without failing. I >> hope this helps explain it a bit better. > > I do understand, however, that if the relocation offset must indeed be a > multiple of 0x1000, then obviously the vectors table will end up as > misaligned as it was before relocation. > > Also, personally I would like it if the vectors table was always > aligned as it should, and there are at least three other boards which > require a prefix/header before their vectors table, as Masahiro (cc:) > indicated recently, so that makes the problem a generic one: how to > properly integrate a header in-image (as opposed to an out-of-image > one, which is just a matter of doing a 'cat', so to speak. > > Therefore I'd like a generic solution to this, where the header is > prepended *and* aligned properly without breaking the start symbol > alignment constraints. This /might/ be possible by cleverly adding > a '.header' or '.signature' section to the linker script, possibly > doing a two-stage link; but this should not require the source code to > contain ad hoc relocation tricks. > > Let me tinker with it in the next few days; I'll try and come up with a > clean and generic solution to this "in-code header" question. > > Thanks again for your explanation! > >> Best regards, >> Darwin > > Amicalement, > Perhaps an oversimplified example of the current code would help to explain this better: scenario #1: CONFIG_SYS_TEXT_BASE 0x88000000 vectors: .align 11 /* exception vectors need to be on a 0x800 byte boundary */ compile/linker produces (before relocation): _start symbol is at 0x88000000 vectors symbol is at 0x88000800 the relocation offset is: 0x77f9b000 therefore, after relocation: _start symbol is at 0xfff9b000 (0x88000000+0xfff9b000) vectors symbol is at 0xfff9b800 (0x88000800+0x77f9b000) scenario #2: CONFIG_SYS_TEXT_BASE 0x88000020 vectors: .align 11 /* exception vectors need to be on a 0x800 byte boundary */ compiler/linker produces (before relocation): _start symbol is at 0x88000020 vectors symbol is at 0x88000800 the relocation offset is: 0x77f9afe0 therefore, after relocation: _start symbol is at 0xfff9b000 (0x88000020+0x77f9afe0) vectors symbol is at 0xfff9b7e0 (0x88000800+0x77f9afe0) Note that in scenario #2, after relocation, the vectors are not on a 0x800 byte boundary anymore. Thanks, Steve
On ma, 2014-06-09 at 13:45 -0700, Steve Rae wrote: > [snip] > Note that in scenario #2, after relocation, the vectors are not on a > 0x800 byte boundary anymore. > As long as the compiler emits adrp instructions, which it already does at -O0, it is not only limited to vectors. No code works as expected after relocation. Regards, Jeroen
Hi Steve, (sorry for the duplicate) On Mon, 9 Jun 2014 13:45:50 -0700, Steve Rae <srae@broadcom.com> wrote: > > > On 14-06-09 03:23 AM, Albert ARIBAUD wrote: > > Hi Darwin, > > > > On Mon, 2 Jun 2014 17:37:25 -0700, Darwin Rambo <drambo@broadcom.com> > > wrote: > > > >> > >> > >> On 14-06-02 12:26 AM, Albert ARIBAUD wrote: > >>> Hi Darwin, > >>> > >>> On Mon, 26 May 2014 09:11:35 -0700, Darwin Rambo <drambo@broadcom.com> > >>> wrote: > >>> > >>>> Hi Albert, > >>>> > >>>> The previous stage bootloader (which I had no control over) wanted it's > >>>> header to be aligned to a 512 byte MMC block boundary, presumably since > >>>> this allowed DMA operations without copy/shifting. At the same time, I > >>>> didn't want to hack a header into start.S because I didn't want to carry > >>>> another downstream patch. So I investigated if I could shift u-boot's > >>>> base address as a feature that would allow an aligned header to be used > >>>> without the start.S patch. > >>>> > >>>> I know that a custom header patch to start.S would work, and that a > >>>> header plus padding will also work. But I found out that you can align > >>>> the base on certain smaller offsets if you keep the relocation offset at > >>>> nice boundaries like 0x1000 and if the relocation offset is a multiple > >>>> of the maximum alignment requirements of the image. > >>>> > >>>> The original patch I submitted didn't handle an end condition properly, > >>>> was ARM64-specific (wasn't tested on other architectures), and because > >>>> the patch was NAK'd, I didn't bother to submit a v2 patch and consider > >>>> the idea to be dead. I'm happy to abandon the patch. I hope this helps. > >>> > >>> Thanks. > >>> > >>> If I understand correctly, your target has a requirement for storing > >>> the image on a 512-byte boundary. But how does this affect the loading > >>> of the image into RAM, where the requirement is only that the vectors > >>> table be 32-bytes aligned? I mean, if you store the image in MMC at > >>> offset 0x200 (thus satisfying the 512-byte boundary requirement) and > >>> load it to, say, offset 0x10020 in RAM, how is it a problem for > >>> your target? > >>> > >>> If my example above inadequately represents the issue, then can you > >>> please provide a similar but adequate example, a failure case scenario, > >>> so that I can hve a correct understanding of the problem? > >> > >> Hi Albert, > >> > >> The constraints I have that I can't change, are that > >> - the 32 byte header is postprocessed and prepended to the image after > >> the build is complete > >> - the header is at a 512 byte alignment in MMC > >> - the header and image are copied to SDRAM to an alignment like > >> 0x88000000. Thus the u-boot image is linked at and starts at 0x88000020. > >> - the vectors need to be 0x800 aligned for armv8 (.align 11 directive) > > > > So far, so good -- I understand that the link-time location of the > > vectors table is incorrect. > > > >> So the failure case is that when the relocation happens, it relocates to > >> a 0x1000 alignment, say something like 0xffffa000. The relocation offset > >> is not a multiple of 0x1000 (0xffffa000 - 0x88000020) and the relocation > >> fails. > > > > What does "relocation fails" mean exactly, i.e., where and how exactly > > does the relocation code behave differently from expected? I'm asking > > because I don't understand why the relocation offset should be a > > multiple of 0x1000. > > > >> Adjusting the relocation offset to a multiple of 0x1000 (by > >> making the relocation address end in 0xNNNNN020) fixes the issues and > >> allows u-boot to relocate and run from this address without failing. I > >> hope this helps explain it a bit better. > > > > I do understand, however, that if the relocation offset must indeed be a > > multiple of 0x1000, then obviously the vectors table will end up as > > misaligned as it was before relocation. > > > > Also, personally I would like it if the vectors table was always > > aligned as it should, and there are at least three other boards which > > require a prefix/header before their vectors table, as Masahiro (cc:) > > indicated recently, so that makes the problem a generic one: how to > > properly integrate a header in-image (as opposed to an out-of-image > > one, which is just a matter of doing a 'cat', so to speak. > > > > Therefore I'd like a generic solution to this, where the header is > > prepended *and* aligned properly without breaking the start symbol > > alignment constraints. This /might/ be possible by cleverly adding > > a '.header' or '.signature' section to the linker script, possibly > > doing a two-stage link; but this should not require the source code to > > contain ad hoc relocation tricks. > > > > Let me tinker with it in the next few days; I'll try and come up with a > > clean and generic solution to this "in-code header" question. > > > > Thanks again for your explanation! > > > >> Best regards, > >> Darwin > > > > Amicalement, > > > > Perhaps an oversimplified example of the current code would help to > explain this better: > > scenario #1: > CONFIG_SYS_TEXT_BASE 0x88000000 > vectors: .align 11 /* exception vectors need to be on a 0x800 byte > boundary */ > compile/linker produces (before relocation): > _start symbol is at 0x88000000 > vectors symbol is at 0x88000800 > the relocation offset is: 0x77f9b000 > therefore, after relocation: > _start symbol is at 0xfff9b000 (0x88000000+0xfff9b000) > vectors symbol is at 0xfff9b800 (0x88000800+0x77f9b000) > > scenario #2: > CONFIG_SYS_TEXT_BASE 0x88000020 > vectors: .align 11 /* exception vectors need to be on a 0x800 byte > boundary */ > compiler/linker produces (before relocation): > _start symbol is at 0x88000020 > vectors symbol is at 0x88000800 > the relocation offset is: 0x77f9afe0 > therefore, after relocation: > _start symbol is at 0xfff9b000 (0x88000020+0x77f9afe0) > vectors symbol is at 0xfff9b7e0 (0x88000800+0x77f9afe0) > > Note that in scenario #2, after relocation, the vectors are not on a > 0x800 byte boundary anymore. Ok, so technically relocation works as it was told to, just was given "garbage in", with an image with an unaligned base and _start symbol (and an aligned vectors symbol); relocation worked as expected, and aligned base and start, thus not aligning vectors. Now, on to understanding why the CONFIG_SYS_TEXT_BASE is not aligned. Can you provide details from the case where the actual issue has arisen, or even better yet, provide a source tree and build command line, that I could reproduce the build here? > Thanks, Steve Amicalement,
On 14-06-09 10:16 PM, Albert ARIBAUD wrote: > Hi Steve, > > (sorry for the duplicate) > > On Mon, 9 Jun 2014 13:45:50 -0700, Steve Rae <srae@broadcom.com> wrote: > >> >> >> On 14-06-09 03:23 AM, Albert ARIBAUD wrote: >>> Hi Darwin, >>> >>> On Mon, 2 Jun 2014 17:37:25 -0700, Darwin Rambo <drambo@broadcom.com> >>> wrote: >>> >>>> >>>> >>>> On 14-06-02 12:26 AM, Albert ARIBAUD wrote: >>>>> Hi Darwin, >>>>> >>>>> On Mon, 26 May 2014 09:11:35 -0700, Darwin Rambo <drambo@broadcom.com> >>>>> wrote: >>>>> >>>>>> Hi Albert, >>>>>> >>>>>> The previous stage bootloader (which I had no control over) wanted it's >>>>>> header to be aligned to a 512 byte MMC block boundary, presumably since >>>>>> this allowed DMA operations without copy/shifting. At the same time, I >>>>>> didn't want to hack a header into start.S because I didn't want to carry >>>>>> another downstream patch. So I investigated if I could shift u-boot's >>>>>> base address as a feature that would allow an aligned header to be used >>>>>> without the start.S patch. >>>>>> >>>>>> I know that a custom header patch to start.S would work, and that a >>>>>> header plus padding will also work. But I found out that you can align >>>>>> the base on certain smaller offsets if you keep the relocation offset at >>>>>> nice boundaries like 0x1000 and if the relocation offset is a multiple >>>>>> of the maximum alignment requirements of the image. >>>>>> >>>>>> The original patch I submitted didn't handle an end condition properly, >>>>>> was ARM64-specific (wasn't tested on other architectures), and because >>>>>> the patch was NAK'd, I didn't bother to submit a v2 patch and consider >>>>>> the idea to be dead. I'm happy to abandon the patch. I hope this helps. >>>>> >>>>> Thanks. >>>>> >>>>> If I understand correctly, your target has a requirement for storing >>>>> the image on a 512-byte boundary. But how does this affect the loading >>>>> of the image into RAM, where the requirement is only that the vectors >>>>> table be 32-bytes aligned? I mean, if you store the image in MMC at >>>>> offset 0x200 (thus satisfying the 512-byte boundary requirement) and >>>>> load it to, say, offset 0x10020 in RAM, how is it a problem for >>>>> your target? >>>>> >>>>> If my example above inadequately represents the issue, then can you >>>>> please provide a similar but adequate example, a failure case scenario, >>>>> so that I can hve a correct understanding of the problem? >>>> >>>> Hi Albert, >>>> >>>> The constraints I have that I can't change, are that >>>> - the 32 byte header is postprocessed and prepended to the image after >>>> the build is complete >>>> - the header is at a 512 byte alignment in MMC >>>> - the header and image are copied to SDRAM to an alignment like >>>> 0x88000000. Thus the u-boot image is linked at and starts at 0x88000020. >>>> - the vectors need to be 0x800 aligned for armv8 (.align 11 directive) >>> >>> So far, so good -- I understand that the link-time location of the >>> vectors table is incorrect. >>> >>>> So the failure case is that when the relocation happens, it relocates to >>>> a 0x1000 alignment, say something like 0xffffa000. The relocation offset >>>> is not a multiple of 0x1000 (0xffffa000 - 0x88000020) and the relocation >>>> fails. >>> >>> What does "relocation fails" mean exactly, i.e., where and how exactly >>> does the relocation code behave differently from expected? I'm asking >>> because I don't understand why the relocation offset should be a >>> multiple of 0x1000. >>> >>>> Adjusting the relocation offset to a multiple of 0x1000 (by >>>> making the relocation address end in 0xNNNNN020) fixes the issues and >>>> allows u-boot to relocate and run from this address without failing. I >>>> hope this helps explain it a bit better. >>> >>> I do understand, however, that if the relocation offset must indeed be a >>> multiple of 0x1000, then obviously the vectors table will end up as >>> misaligned as it was before relocation. >>> >>> Also, personally I would like it if the vectors table was always >>> aligned as it should, and there are at least three other boards which >>> require a prefix/header before their vectors table, as Masahiro (cc:) >>> indicated recently, so that makes the problem a generic one: how to >>> properly integrate a header in-image (as opposed to an out-of-image >>> one, which is just a matter of doing a 'cat', so to speak. >>> >>> Therefore I'd like a generic solution to this, where the header is >>> prepended *and* aligned properly without breaking the start symbol >>> alignment constraints. This /might/ be possible by cleverly adding >>> a '.header' or '.signature' section to the linker script, possibly >>> doing a two-stage link; but this should not require the source code to >>> contain ad hoc relocation tricks. >>> >>> Let me tinker with it in the next few days; I'll try and come up with a >>> clean and generic solution to this "in-code header" question. >>> >>> Thanks again for your explanation! >>> >>>> Best regards, >>>> Darwin >>> >>> Amicalement, >>> >> >> Perhaps an oversimplified example of the current code would help to >> explain this better: >> >> scenario #1: >> CONFIG_SYS_TEXT_BASE 0x88000000 >> vectors: .align 11 /* exception vectors need to be on a 0x800 byte >> boundary */ >> compile/linker produces (before relocation): >> _start symbol is at 0x88000000 >> vectors symbol is at 0x88000800 >> the relocation offset is: 0x77f9b000 >> therefore, after relocation: >> _start symbol is at 0xfff9b000 (0x88000000+0xfff9b000) >> vectors symbol is at 0xfff9b800 (0x88000800+0x77f9b000) >> >> scenario #2: >> CONFIG_SYS_TEXT_BASE 0x88000020 >> vectors: .align 11 /* exception vectors need to be on a 0x800 byte >> boundary */ >> compiler/linker produces (before relocation): >> _start symbol is at 0x88000020 >> vectors symbol is at 0x88000800 >> the relocation offset is: 0x77f9afe0 >> therefore, after relocation: >> _start symbol is at 0xfff9b000 (0x88000020+0x77f9afe0) >> vectors symbol is at 0xfff9b7e0 (0x88000800+0x77f9afe0) >> >> Note that in scenario #2, after relocation, the vectors are not on a >> 0x800 byte boundary anymore. > > Ok, so technically relocation works as it was told to, just was given > "garbage in", with an image with an unaligned base and _start symbol > (and an aligned vectors symbol); relocation worked as expected, and > aligned base and start, thus not aligning vectors. > > Now, on to understanding why the CONFIG_SYS_TEXT_BASE is not aligned. > > Can you provide details from the case where the actual issue has > arisen, or even better yet, provide a source tree and build command > line, that I could reproduce the build here? > >> Thanks, Steve > > Amicalement, > Albert et al. There could be "many" reasons why the CONFIG_SYS_TEXT_BASE is not aligned on a 0x1000 byte boundary (Darwin has attempted to document his particular use-case...) But we think that the solution to support this is relatively straightforward: (1) after determining the "relocation address" (which will be on a 0x1000 byte boundary), (2) add "CONFIG_SYS_TEXT_BASE % 4096" to that "relocation address" (which effectively makes the "relocation offset" a multiple of 0x1000 too...) So, in the scenario #1, (CONFIG_SYS_TEXT_BASE % 4096) = 0, so this algorithm changes nothing. And in scenario #2, (CONFIG_SYS_TEXT_BASE % 4096) = 0x20, therefore, we would now get the following: the relocation offset is: 0x77f9b000 therefore, after relocation: _start symbol is at 0xfff9b020 (0x88000020+0x77f9b000) vectors symbol is at 0xfff9b800 (0x88000800+0x77f9b000) Which results in everything being aligned properly before AND after relocation. (3) HOWEVER, shifting the address UP may cause the end of the relocated code to run past the end of the available memory... So we could: (3.1) always relocate DOWN by ((CONFIG_SYS_TEXT_BASE % 4096) - 4096), or (3.2) add (CONFIG_SYS_TEXT_BASE % 4096) to the calculated "gd->mon_len" (which is the value used to determine the "relocation address"; this would guarantee that there is always enough space for the relocated code...) Note: 3.1 would usually waste up to 4096 bytes; whereas 3.2 would only adjust when necessary; therefore, we prefer 3.2 .... I trust that everyone will find this explanation acceptable... If so, we can publish a [v2] patch. Thanks, Steve
Dear Steve, In message <539746C4.9040004@broadcom.com> you wrote: > > There could be "many" reasons why the CONFIG_SYS_TEXT_BASE is not > aligned on a 0x1000 byte boundary (Darwin has attempted to document his > particular use-case...) We should be more precise here and ask for any _good_ reasons. I can think of many good reasons to keep the text base aligned. As for the reasons to use an unaligned address that were brought up here, I still think that it would have been better to use an aligned taxe base and do the rest with a customized linker script. > But we think that the solution to support this is relatively > straightforward: > (1) after determining the "relocation address" (which will be on a > 0x1000 byte boundary), > (2) add "CONFIG_SYS_TEXT_BASE % 4096" to that "relocation address" > (which effectively makes the "relocation offset" a multiple of 0x1000 > too...) > So, in the scenario #1, (CONFIG_SYS_TEXT_BASE % 4096) = 0, so this > algorithm changes nothing. > And in scenario #2, (CONFIG_SYS_TEXT_BASE % 4096) = 0x20, therefore, we > would now get the following: > the relocation offset is: 0x77f9b000 > therefore, after relocation: > _start symbol is at 0xfff9b020 (0x88000020+0x77f9b000) > vectors symbol is at 0xfff9b800 (0x88000800+0x77f9b000) I still cannot understand why _start and CONFIG_SYS_TEXT_BASE would have to be the same. There is no such requirement. What exactly prevents you from assigning _start a location at offset 0x20 to the start of the text segment, i. e. CONFIG_SYS_TEXT_BASE ? Then everything should be still the same for you, and no voodoo coding would be needed. > (3) HOWEVER, shifting the address UP may cause the end of the relocated > code to run past the end of the available memory... So we could: This problem is void if you just use a poperly aligned text base in combination with a prope start.S resp. linker script to make sure _start is where you want it. > I trust that everyone will find this explanation acceptable... No, I do not see a good reason to add such code. Best regards, Wolfgang Denk
On 14-06-10 11:13 AM, Wolfgang Denk wrote: > Dear Steve, > > In message <539746C4.9040004@broadcom.com> you wrote: >> >> There could be "many" reasons why the CONFIG_SYS_TEXT_BASE is not >> aligned on a 0x1000 byte boundary (Darwin has attempted to document his >> particular use-case...) > > We should be more precise here and ask for any _good_ reasons. > > I can think of many good reasons to keep the text base aligned. As > for the reasons to use an unaligned address that were brought up here, > I still think that it would have been better to use an aligned taxe > base and do the rest with a customized linker script. > >> But we think that the solution to support this is relatively >> straightforward: >> (1) after determining the "relocation address" (which will be on a >> 0x1000 byte boundary), >> (2) add "CONFIG_SYS_TEXT_BASE % 4096" to that "relocation address" >> (which effectively makes the "relocation offset" a multiple of 0x1000 >> too...) >> So, in the scenario #1, (CONFIG_SYS_TEXT_BASE % 4096) = 0, so this >> algorithm changes nothing. >> And in scenario #2, (CONFIG_SYS_TEXT_BASE % 4096) = 0x20, therefore, we >> would now get the following: >> the relocation offset is: 0x77f9b000 >> therefore, after relocation: >> _start symbol is at 0xfff9b020 (0x88000020+0x77f9b000) >> vectors symbol is at 0xfff9b800 (0x88000800+0x77f9b000) > > I still cannot understand why _start and CONFIG_SYS_TEXT_BASE would > have to be the same. There is no such requirement. What exactly > prevents you from assigning _start a location at offset 0x20 to the > start of the text segment, i. e. CONFIG_SYS_TEXT_BASE ? Wolfgang et al. I agree that they do not need to be the same... So our issue is that basically "for every ARMv7 board in the company", we are currently maintaining our own modified/customized version of "arch/arm/cpu/armv7/start.S" in order to add the appropriate 32 byte header... And we could choose to do it using other methods, but they all require us to maintain a customized version of linker scripts, or some other code, or .... The request here is that with the addition of some relatively straightforward (upstreamed) code, then this can be handled automatically by a post-processing step and we would be able to use a pristine version of the upstreamed code... Our desire is to get this into the beginnings of the "ARMv8" boards, so that we can avoid the maintenance issues we have with the current ARMv7 boards. We appreciate your consideration of this request. Thanks, Steve > > Then everything should be still the same for you, and no voodoo coding > would be needed. > >> (3) HOWEVER, shifting the address UP may cause the end of the relocated >> code to run past the end of the available memory... So we could: > > This problem is void if you just use a poperly aligned text base in > combination with a prope start.S resp. linker script to make sure > _start is where you want it. > >> I trust that everyone will find this explanation acceptable... > > No, I do not see a good reason to add such code. > > Best regards, > > Wolfgang Denk >
Dear Steve, In message <53975EC2.1080209@broadcom.com> you wrote: > > > I still cannot understand why _start and CONFIG_SYS_TEXT_BASE would > > have to be the same. There is no such requirement. What exactly > > prevents you from assigning _start a location at offset 0x20 to the > > start of the text segment, i. e. CONFIG_SYS_TEXT_BASE ? > > Wolfgang et al. > > I agree that they do not need to be the same... > So our issue is that basically "for every ARMv7 board in the company", > we are currently maintaining our own modified/customized version of > "arch/arm/cpu/armv7/start.S" in order to add the appropriate 32 byte > header... There should be more clever ways to implement this. If nothing else comes to mind, an #ifdef in "arch/arm/cpu/armv7/start.S" should be sufficient to condistionally insert / adjust any offset that might be needed for a specific board. > And we could choose to do it using other methods, but they all require > us to maintain a customized version of linker scripts, or some other > code, or .... ... or a CONFIG setting in your board config file resp. your board's defconfig. > The request here is that with the addition of some relatively > straightforward (upstreamed) code, then this can be handled > automatically by a post-processing step and we would be able to use a > pristine version of the upstreamed code... I'm sorry, but I disagree especially on the "straightforward" part. Also, I see no reason to make the existing code unnecessarily complex, and add more disadvantages (like increased meory footprint etc.) when the same purpose can be implemented without adding any special code at all. > Our desire is to get this into the beginnings of the "ARMv8" boards, so > that we can avoid the maintenance issues we have with the current ARMv7 > boards. > > We appreciate your consideration of this request. These are two different things: implementing a clean and easy way to support a necessary feature is one thing; to do it in the suggested way by adding code where none is needed is another thing. I'm all for adding support for any features that are useful, even if only for a single user, as long as they don't hurt other users. All I'm asking is to chose another way to implement this feature. So far, I did not see any arguments that my alternative proposals would cause any problems to you? Best regards, Wolfgang Denk
Hi Steve, On Tue, 10 Jun 2014 12:38:42 -0700, Steve Rae <srae@broadcom.com> wrote: > > > On 14-06-10 11:13 AM, Wolfgang Denk wrote: > > Dear Steve, > > > > In message <539746C4.9040004@broadcom.com> you wrote: > >> > >> There could be "many" reasons why the CONFIG_SYS_TEXT_BASE is not > >> aligned on a 0x1000 byte boundary (Darwin has attempted to document his > >> particular use-case...) > > > > We should be more precise here and ask for any _good_ reasons. > > > > I can think of many good reasons to keep the text base aligned. As > > for the reasons to use an unaligned address that were brought up here, > > I still think that it would have been better to use an aligned taxe > > base and do the rest with a customized linker script. > > > >> But we think that the solution to support this is relatively > >> straightforward: > >> (1) after determining the "relocation address" (which will be on a > >> 0x1000 byte boundary), > >> (2) add "CONFIG_SYS_TEXT_BASE % 4096" to that "relocation address" > >> (which effectively makes the "relocation offset" a multiple of 0x1000 > >> too...) > >> So, in the scenario #1, (CONFIG_SYS_TEXT_BASE % 4096) = 0, so this > >> algorithm changes nothing. > >> And in scenario #2, (CONFIG_SYS_TEXT_BASE % 4096) = 0x20, therefore, we > >> would now get the following: > >> the relocation offset is: 0x77f9b000 > >> therefore, after relocation: > >> _start symbol is at 0xfff9b020 (0x88000020+0x77f9b000) > >> vectors symbol is at 0xfff9b800 (0x88000800+0x77f9b000) > > > > I still cannot understand why _start and CONFIG_SYS_TEXT_BASE would > > have to be the same. There is no such requirement. What exactly > > prevents you from assigning _start a location at offset 0x20 to the > > start of the text segment, i. e. CONFIG_SYS_TEXT_BASE ? > > Wolfgang et al. > > I agree that they do not need to be the same... > So our issue is that basically "for every ARMv7 board in the company", > we are currently maintaining our own modified/customized version of > "arch/arm/cpu/armv7/start.S" in order to add the appropriate 32 byte > header... That is not a good approach -- and I should know, as there are boards out there which already insist on having a header (mind you, a simple 4-byte constant) in start.S, and I am already trying to tackle this out of start.S. Your 32-byte header should not be in start.S, it should be linked in before start.o in the linker script. Then you would only have one start.S file and as many headers as you require. > And we could choose to do it using other methods, but they all require > us to maintain a customized version of linker scripts, or some other > code, or .... Not necessarily. You'll need to configure your target, that's sure, because I guess different targets will have different (values in the bytes of their) headers, but you don't need to have different start.S files. > The request here is that with the addition of some relatively > straightforward (upstreamed) code, then this can be handled > automatically by a post-processing step and we would be able to use a > pristine version of the upstreamed code... > Our desire is to get this into the beginnings of the "ARMv8" boards, so > that we can avoid the maintenance issues we have with the current ARMv7 > boards. > > We appreciate your consideration of this request. > Thanks, Steve I (believe I) understand the problem, and I am discussing here because I too want it solved. On the one hand I do agree with Wolfgang here: > > No, I do not see a good reason to add such code. ... because I don't want to allow a feature that is designed to counter a problem; but on the other hand I want the problem fixed. So Darwin, in order to find a satisfactory solution, let me try to recap the situation and then ask a few questions. Recap: 1. A typical ARM binary image is linked to a base address, defined by preprocessor macro (aka U-Boot config option) CONFIG_SYS_TEXT_BASE. 2. A typical ARM target defines CONFIG_SYS_TEXT_BASE so that the image, and especially its exception vectors table, is properly aligned upon loading the image in RAM. 3. When the ARM image relocates itself, it does so to a destination address which is computed so that the image and its vectors table are still properly aligned after relocation. 4. Some targets require that the image stored in MMC be prepended with a 32-byte header (and aligned to a sector-related boundary). 5. For some targets (if not all) the header is not separate from the image, i.e., it will also be copied from MMC to RAM. Such headers must be prepended at the link stage or earlier, so that the link-time and run-time values of all image symbols are consistent. 6. However, because the header is put at the start of the image before the vectors table, the image is still properly aligned but th vectors tabled is not any more, both when the image is loaded from MMC to RAM, and when it is relocated. 7. This mis-alignment issue does not only affect the vectors table; it also affects any location addressed using adrp instructions (if I did correctly understand Jeroen, Cc:). This is my understanding of the issue. If it is correct, then I believe the following solves the issue without changing anything to the current code: Instead of prepending a 32-byte header to the image, prepend the header then a block of 2048-32 bytes. This way, the MMC image base address (the header address) is aligned, and the alignment of any address in the image itself is preserved. If for some reason it is impossible to pad the header (such as, the header is actually prepended by a closed-source tool which takes the 'raw' image in and spits the 'MMC image' out), then the raw image should be prepended with 2048-32 bytes beforehand, so that the 32 header bytes make up a whole 2048-bytes block, and the image remains aligned. However, I guess that you, Darwin and Steve, probably have thought of this already and dismissed it for a reason. If so, please explain what this reason is. Amicalement,
On 14-06-10 02:20 PM, Albert ARIBAUD wrote: > Hi Steve, > > On Tue, 10 Jun 2014 12:38:42 -0700, Steve Rae <srae@broadcom.com> wrote: > >> >> >> On 14-06-10 11:13 AM, Wolfgang Denk wrote: >>> Dear Steve, >>> >>> In message <539746C4.9040004@broadcom.com> you wrote: >>>> >>>> There could be "many" reasons why the CONFIG_SYS_TEXT_BASE is not >>>> aligned on a 0x1000 byte boundary (Darwin has attempted to document his >>>> particular use-case...) >>> >>> We should be more precise here and ask for any _good_ reasons. >>> >>> I can think of many good reasons to keep the text base aligned. As >>> for the reasons to use an unaligned address that were brought up here, >>> I still think that it would have been better to use an aligned taxe >>> base and do the rest with a customized linker script. >>> >>>> But we think that the solution to support this is relatively >>>> straightforward: >>>> (1) after determining the "relocation address" (which will be on a >>>> 0x1000 byte boundary), >>>> (2) add "CONFIG_SYS_TEXT_BASE % 4096" to that "relocation address" >>>> (which effectively makes the "relocation offset" a multiple of 0x1000 >>>> too...) >>>> So, in the scenario #1, (CONFIG_SYS_TEXT_BASE % 4096) = 0, so this >>>> algorithm changes nothing. >>>> And in scenario #2, (CONFIG_SYS_TEXT_BASE % 4096) = 0x20, therefore, we >>>> would now get the following: >>>> the relocation offset is: 0x77f9b000 >>>> therefore, after relocation: >>>> _start symbol is at 0xfff9b020 (0x88000020+0x77f9b000) >>>> vectors symbol is at 0xfff9b800 (0x88000800+0x77f9b000) >>> >>> I still cannot understand why _start and CONFIG_SYS_TEXT_BASE would >>> have to be the same. There is no such requirement. What exactly >>> prevents you from assigning _start a location at offset 0x20 to the >>> start of the text segment, i. e. CONFIG_SYS_TEXT_BASE ? >> >> Wolfgang et al. >> >> I agree that they do not need to be the same... >> So our issue is that basically "for every ARMv7 board in the company", >> we are currently maintaining our own modified/customized version of >> "arch/arm/cpu/armv7/start.S" in order to add the appropriate 32 byte >> header... > > That is not a good approach -- and I should know, as there are boards > out there which already insist on having a header (mind you, a simple > 4-byte constant) in start.S, and I am already trying to tackle this > out of start.S. > > Your 32-byte header should not be in start.S, it should be linked in > before start.o in the linker script. Then you would only have one > start.S file and as many headers as you require. > >> And we could choose to do it using other methods, but they all require >> us to maintain a customized version of linker scripts, or some other >> code, or .... > > Not necessarily. You'll need to configure your target, that's sure, > because I guess different targets will have different (values in > the bytes of their) headers, but you don't need to have different > start.S files. > >> The request here is that with the addition of some relatively >> straightforward (upstreamed) code, then this can be handled >> automatically by a post-processing step and we would be able to use a >> pristine version of the upstreamed code... >> Our desire is to get this into the beginnings of the "ARMv8" boards, so >> that we can avoid the maintenance issues we have with the current ARMv7 >> boards. >> >> We appreciate your consideration of this request. >> Thanks, Steve > > I (believe I) understand the problem, and I am discussing here because I > too want it solved. On the one hand I do agree with Wolfgang here: > >>> No, I do not see a good reason to add such code. > > ... because I don't want to allow a feature that is designed to counter > a problem; but on the other hand I want the problem fixed. So Darwin, > in order to find a satisfactory solution, let me try to recap the > situation and then ask a few questions. > > Recap: > > 1. A typical ARM binary image is linked to a base address, defined by > preprocessor macro (aka U-Boot config option) CONFIG_SYS_TEXT_BASE. OK > > 2. A typical ARM target defines CONFIG_SYS_TEXT_BASE so that the image, > and especially its exception vectors table, is properly aligned upon > loading the image in RAM. Nope, the vectors respect the .align directive independent of the CONFIG_SYS_TEXT_BASE setting. > > 3. When the ARM image relocates itself, it does so to a destination > address which is computed so that the image and its vectors table > are still properly aligned after relocation. Almost: the computed destination address is aligned on a 0x1000 byte boundary, so if the CONFIG_SYS_TEXT_BASE is also a multiple of 0x1000 bytes, then this statement is true. > > 4. Some targets require that the image stored in MMC be prepended with > a 32-byte header (and aligned to a sector-related boundary). OK > > 5. For some targets (if not all) the header is not separate from the > image, i.e., it will also be copied from MMC to RAM. Such headers > must be prepended at the link stage or earlier, so that the > link-time and run-time values of all image symbols are consistent. Yes - copied from MMC to RAM, but not really necessary to do this at "link stage or earlier" (we do it with a post-processing tool to prepend a header onto 'u-boot.bin') > > 6. However, because the header is put at the start of the image before > the vectors table, the image is still properly aligned but th > vectors tabled is not any more, both when the image is loaded from > MMC to RAM, and when it is relocated. Because we set the CONFIG_SYS_TEXT_BASE to 0xXXXX0020, the image is properly aligned, if CONFIG_SYS_TEXT_BASE is 0xXXXX0000, then the "before" image would not be properly aligned.... > > 7. This mis-alignment issue does not only affect the vectors table; it > also affects any location addressed using adrp instructions (if I > did correctly understand Jeroen, Cc:). Agree - it affects all of the "align" directives, in the code and the linker script file, and all instructions which assume alignment.... > > This is my understanding of the issue. If it is correct, then I > believe the following solves the issue without changing anything to > the current code: > > Instead of prepending a 32-byte header to the image, prepend the header > then a block of 2048-32 bytes. This way, the MMC image base address > (the header address) is aligned, and the alignment of any address in > the image itself is preserved. Yes - if the header is an exact multiple of 0x1000 (4096), it will work correctly without any code modifications... (not certain about 2048...) > > If for some reason it is impossible to pad the header (such as, the > header is actually prepended by a closed-source tool which takes the > 'raw' image in and spits the 'MMC image' out), then the raw image > should be prepended with 2048-32 bytes beforehand, so that the 32 > header bytes make up a whole 2048-bytes block, and the image remains > aligned. > > However, I guess that you, Darwin and Steve, probably have thought of > this already and dismissed it for a reason. If so, please explain > what this reason is. We wanted to implement a forward looking solution that worked for any size of prepended header, and which removed the constraint for the CONFIG_SYS_TEXT_BASE to be an exact multiple of 0x1000 (4096). However, since this solution does not look like it will succeed, in a parallel thread, we are pursuing an alternate proposal to conditionally reserve space for the header in u-boot.bin (which is "link stage or earlier"!!!), which would then be modified by a post-processing tool. So please review that proposal. Thanks, Steve > > Amicalement, >
Dear Albert, In message <E1WuTT8-0007AF-9l@janus> you wrote: > > 1. A typical ARM binary image is linked to a base address, defined by > preprocessor macro (aka U-Boot config option) CONFIG_SYS_TEXT_BASE. We shoul specifically keep in mind here that the "base address" is defined as the start address of the text segment, which may or may not be the same as the entry point address _start. In general, these are not the same. > 2. A typical ARM target defines CONFIG_SYS_TEXT_BASE so that the image, > and especially its exception vectors table, is properly aligned upon > loading the image in RAM. > > 3. When the ARM image relocates itself, it does so to a destination > address which is computed so that the image and its vectors table > are still properly aligned after relocation. These items are not ARM specific. > 4. Some targets require that the image stored in MMC be prepended with > a 32-byte header (and aligned to a sector-related boundary). Why would such headers have to be part of the text segment? > 5. For some targets (if not all) the header is not separate from the > image, i.e., it will also be copied from MMC to RAM. Such headers > must be prepended at the link stage or earlier, so that the > link-time and run-time values of all image symbols are consistent. If the headers are in their own segment, this should be trivial. > 6. However, because the header is put at the start of the image before > the vectors table, the image is still properly aligned but th > vectors tabled is not any more, both when the image is loaded from > MMC to RAM, and when it is relocated. This would be a bug, then. If we have an alignment requirement for the text segment, and we load other data (or code) in front of the text segment, then the size of such additional data must be made such that the text segment is still properly aligned. Usually I would expect that the heaer is in a separate segment, and the linker script would take care of the required alignment for the text segment and insert the necessary gap to keep it properly aligned. > Instead of prepending a 32-byte header to the image, prepend the header > then a block of 2048-32 bytes. This way, the MMC image base address > (the header address) is aligned, and the alignment of any address in > the image itself is preserved. Agreed. Or should .text be aligned on a 4 k boundary? Best regards, Wolfgang Denk
Dear Steve, In message <53979F6A.90607@broadcom.com> you wrote: > > Yes - copied from MMC to RAM, but not really necessary to do this at > "link stage or earlier" (we do it with a post-processing tool to prepend > a header onto 'u-boot.bin') What exactly is the fuction of this post-processing tool? Does it retrieve any data from the previously built image (like size information), or could the header also be generated before the link step, so we just include it there? Also, can we not use the U-Boot build process to generate these 32 bytes of header data? > > 6. However, because the header is put at the start of the image before > > the vectors table, the image is still properly aligned but th > > vectors tabled is not any more, both when the image is loaded from > > MMC to RAM, and when it is relocated. > > Because we set the CONFIG_SYS_TEXT_BASE to 0xXXXX0020, the image is > properly aligned, if CONFIG_SYS_TEXT_BASE is 0xXXXX0000, then the > "before" image would not be properly aligned.... ...unless your linker script properly aligns all segments. > We wanted to implement a forward looking solution that worked for any > size of prepended header, and which removed the constraint for the > CONFIG_SYS_TEXT_BASE to be an exact multiple of 0x1000 (4096). The generic approach is to use the linker to define the memory map of the resulting image, i. e. to create proper alignment of segments andproper placement of code such that specific address and alignment requirements are met. > However, since this solution does not look like it will succeed, in a > parallel thread, we are pursuing an alternate proposal to conditionally > reserve space for the header in u-boot.bin (which is "link stage or > earlier"!!!), which would then be modified by a post-processing tool. Why would such post-processing be needed? I think you should strive to get rid of such an additional step, and try to get the header generation either be done before linking, or even better as integral part of the build process. We are talking about 32 bytes of data here, so I speculate it should not be too difficult to generate these, probably even without theuse of a special external tool? Can you explain the structure of this 32 byte header? Best regards, Wolfgang Denk
diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c index 9b473b5..df520cc 100644 --- a/arch/arm/lib/board.c +++ b/arch/arm/lib/board.c @@ -449,6 +449,19 @@ void board_init_f(ulong bootflag) dram_init_banksize(); display_dram_config(); /* and display it */ +#ifdef CONFIG_ARM64 + /* + * Fix relocation if u-boot is not on an aligned address. + */ + { + int offset = CONFIG_SYS_TEXT_BASE % 4096; + if (offset) { + addr += offset; + debug("Relocation Addr bumped to 0x%08lx\n", addr); + } + } +#endif + gd->relocaddr = addr; gd->start_addr_sp = addr_sp; gd->reloc_off = addr - (ulong)&_start; diff --git a/common/board_f.c b/common/board_f.c index 4ea4cb2..1035d6f 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -728,6 +728,16 @@ static int reloc_fdt(void) static int setup_reloc(void) { +#ifdef CONFIG_ARM64 + /* + * Fix relocation if u-boot is not on an aligned address. + */ + int offset = CONFIG_SYS_TEXT_BASE % 4096; + if (offset) { + gd->relocaddr += offset; + debug("Relocation Addr bumped to 0x%08lx\n", gd->relocaddr); + } +#endif gd->reloc_off = gd->relocaddr - CONFIG_SYS_TEXT_BASE; memcpy(gd->new_gd, (char *)gd, sizeof(gd_t));