Re: [U-Boot] [PATCH 01/10] Fix IP alignement problem
Hi Olav,
On Fri, Oct 10, 2008 at 11:53 AM, Olav Morken olavmrk@gmail.com wrote:
On Fri, Oct 10, 2008 at 7:01 PM, Ben Warren biggerbadderben@gmail.com wrote:
Olav Morken wrote:
This patch removes volatile from: volatile IP_t *ip = (IP_t *)xip;
Due to a bug, avr32-gcc will assume that ip is aligned on a word
boundary
when using volatile, which causes an exception since xip isn't aligned on a word boundary.
What other architectures have you tried this on?
None, as we don't have any other boards to test on. I do however believe that this change should have no side-effects. If any architectures relied on this function working as some sort of memory/io barrier, they would have problems with other functions such as ArpRequest, which doesn't have anything that will work as a memory/io barrier before the eth_send function.
Of course, I could be wrong. I would certainly not suggest including this change without some more testing.
The bug which causes this problem is in avr32-gcc, which makes assumptions about the alignement of IP_t when using volatile, and this change shouldn't be necessary once that bug is fixed. Until that bug is fixed, this change is needed for anyone trying to run U-Boot on this microcontroller.
I don't mean to be a pain, it's just that this code is shared by everything, so we need to be really careful. I agree with Haavard that the volatile keyword is probably used much more than it should be in the networking library.
I'll pull this into the net/testing branch in the next couple of days, and hopefully we'll get some volunteers to try it out on different architectures.
regards, Ben
On Wed, Oct 15, 2008 at 07:07, Ben Warren biggerbadderben@gmail.com wrote:
Hi Olav,
On Fri, Oct 10, 2008 at 11:53 AM, Olav Morken olavmrk@gmail.com wrote:
On Fri, Oct 10, 2008 at 7:01 PM, Ben Warren biggerbadderben@gmail.com wrote:
What other architectures have you tried this on?
None, as we don't have any other boards to test on. I do however believe that this change should have no side-effects. If any architectures relied on this function working as some sort of memory/io barrier, they would have problems with other functions such as ArpRequest, which doesn't have anything that will work as a memory/io barrier before the eth_send function.
Of course, I could be wrong. I would certainly not suggest including this change without some more testing.
The bug which causes this problem is in avr32-gcc, which makes assumptions about the alignement of IP_t when using volatile, and this change shouldn't be necessary once that bug is fixed. Until that bug is fixed, this change is needed for anyone trying to run U-Boot on this microcontroller.
I don't mean to be a pain, it's just that this code is shared by everything, so we need to be really careful. I agree with Haavard that the volatile keyword is probably used much more than it should be in the networking library.
I'll pull this into the net/testing branch in the next couple of days, and hopefully we'll get some volunteers to try it out on different architectures.
That is great.
FWIW: I have now tested it in qemu_mips, where it appears to work. (Had to revert "qemu-mips.h: Add CFI support" before I could test it.)
On 17:11 Wed 15 Oct , Olav Morken wrote:
On Wed, Oct 15, 2008 at 07:07, Ben Warren biggerbadderben@gmail.com wrote:
Hi Olav,
On Fri, Oct 10, 2008 at 11:53 AM, Olav Morken olavmrk@gmail.com wrote:
On Fri, Oct 10, 2008 at 7:01 PM, Ben Warren biggerbadderben@gmail.com wrote:
What other architectures have you tried this on?
None, as we don't have any other boards to test on. I do however believe that this change should have no side-effects. If any architectures relied on this function working as some sort of memory/io barrier, they would have problems with other functions such as ArpRequest, which doesn't have anything that will work as a memory/io barrier before the eth_send function.
Of course, I could be wrong. I would certainly not suggest including this change without some more testing.
The bug which causes this problem is in avr32-gcc, which makes assumptions about the alignement of IP_t when using volatile, and this change shouldn't be necessary once that bug is fixed. Until that bug is fixed, this change is needed for anyone trying to run U-Boot on this microcontroller.
I don't mean to be a pain, it's just that this code is shared by everything, so we need to be really careful. I agree with Haavard that the volatile keyword is probably used much more than it should be in the networking library.
I'll pull this into the net/testing branch in the next couple of days, and hopefully we'll get some volunteers to try it out on different architectures.
That is great.
FWIW: I have now tested it in qemu_mips, where it appears to work. (Had to revert "qemu-mips.h: Add CFI support" before I could test it.)
why?
Best Regards, J.
On Fri, Oct 17, 2008 at 12:47, Jean-Christophe PLAGNIOL-VILLARD plagnioj@jcrosoft.com wrote:
On 17:11 Wed 15 Oct , Olav Morken wrote:
FWIW: I have now tested it in qemu_mips, where it appears to work. (Had to revert "qemu-mips.h: Add CFI support" before I could test it.)
why?
U-Boot would hang during startup with that patch. I don't know what went wrong, and didn't really have time to debug it.
Looking at it again, this may have been caused by old versions of binutils and gcc. I followed the instructions at http://www.linux-mips.org/wiki/Toolchains, and I see now that they are quite old.
I used qemu 0.91, and compiled & ran U-Boot with the following commands: export PATH="$PATH:$HOME/mips/bin" export CROSS_COMPILE=mips-unknown-linux-gnu- cd ~/u-boot.mips make clean git clean -d -x -f make qemu_mips_config make ln -s ./u-boot.bin mips_bios.bin qemu-system-mips -tftp /tftpboot -L . /dev/null -nographic
participants (3)
-
Ben Warren -
Jean-Christophe PLAGNIOL-VILLARD -
Olav Morken