| Message ID | 1320156451-8961-1-git-send-email-weisserm@arcor.de |
|---|---|
| State | Superseded |
| Headers | show |
On Tuesday 01 November 2011 10:07:31 Matthias Weisser wrote: > --- a/arch/sandbox/cpu/os.c > +++ b/arch/sandbox/cpu/os.c > > +void *os_mmap(void *addr, size_t length, int prot, int flags, int fd, > + off_t offset) > +{ > + return mmap((void *)addr, length, PROT_READ | PROT_WRITE, void cast here is unnecessary > --- a/arch/sandbox/lib/board.c > +++ b/arch/sandbox/lib/board.c > > +static gd_t gd_mem; i don't think this indirection is necessary. probably can replace the static gd_mem with: gd_t gd; > +#define CONFIG_SYS_SDRAM_BASE 0x20000000 > ... > - mem = malloc(size); > + mem = os_mmap((void *)CONFIG_SYS_SDRAM_BASE, CONFIG_SYS_SDRAM_SIZE, > + 0, 0, -1, 0); mmap() makes no guarantee that the requested address is what you'll get back. so there's no point in having this SDRAM_BASE define. punt it and pass in NULL instead. also, with Simon's other patch to md to use the remap func, the address of our memory backing store doesn't matter. -mike
Am 01.11.2011 16:28, schrieb Mike Frysinger: > On Tuesday 01 November 2011 10:07:31 Matthias Weisser wrote: >> --- a/arch/sandbox/cpu/os.c >> +++ b/arch/sandbox/cpu/os.c >> >> +void *os_mmap(void *addr, size_t length, int prot, int flags, int fd, >> + off_t offset) >> +{ >> + return mmap((void *)addr, length, PROT_READ | PROT_WRITE, > > void cast here is unnecessary Right. >> --- a/arch/sandbox/lib/board.c >> +++ b/arch/sandbox/lib/board.c >> >> +static gd_t gd_mem; > > i don't think this indirection is necessary. probably can replace the static > gd_mem with: > gd_t gd; AFAIK gd is a pointer. So I think we always need some sort of memory where the actual structure is stored. >> +#define CONFIG_SYS_SDRAM_BASE 0x20000000 >> ... >> - mem = malloc(size); >> + mem = os_mmap((void *)CONFIG_SYS_SDRAM_BASE, CONFIG_SYS_SDRAM_SIZE, >> + 0, 0, -1, 0); > > mmap() makes no guarantee that the requested address is what you'll get back. > so there's no point in having this SDRAM_BASE define. punt it and pass in NULL > instead. But it works in most cases :-) The point of adding it was that I really like to have memory aligned on a 256MB boundary or so like it is in most SOCs. But thats a personal preference. And if it doesn't work you can still get the address of physical memory from bdinfo. > also, with Simon's other patch to md to use the remap func, the address of our > memory backing store doesn't matter. Well, you are right. But with the posted patch you don't need any remap function and md/mm/mtest/mw works out of the box. Regards Matthias
Hi Matthias, On Tue, Nov 1, 2011 at 7:07 AM, Matthias Weisser <weisserm@arcor.de> wrote: > Using mmap we can simulate RAM at a specific address. This also > eliminated the use of system malloc function. > > Signed-off-by: Matthias Weisser <weisserm@arcor.de> Thanks for the patch! > --- > arch/sandbox/cpu/os.c | 9 +++++++++ > arch/sandbox/lib/board.c | 19 +++++++++++-------- > include/configs/sandbox.h | 1 + > include/os.h | 13 +++++++++++++ > 4 files changed, 34 insertions(+), 8 deletions(-) > > diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c > index 6c175d4..cd2fb05 100644 > --- a/arch/sandbox/cpu/os.c > +++ b/arch/sandbox/cpu/os.c > @@ -24,6 +24,7 @@ > #include <unistd.h> > #include <sys/types.h> > #include <sys/stat.h> > +#include <sys/mman.h> > > #include <os.h> > > @@ -53,3 +54,11 @@ void os_exit(int exit_code) > { > exit(exit_code); > } > + > +void *os_mmap(void *addr, size_t length, int prot, int flags, int fd, > + off_t offset) I would prefer that you define something like os_malloc() instead, and your implementation use mmap(). After all your flags cannot sensibly be used elsewhere in U-Boot, and we might as well define only the feature that we actually want to use. I don't see a general use for mmap(). > +{ > + return mmap((void *)addr, length, PROT_READ | PROT_WRITE, > + MAP_PRIVATE | MAP_ANONYMOUS, > + -1, 0); > +} > diff --git a/arch/sandbox/lib/board.c b/arch/sandbox/lib/board.c > index ae5a517..0912a8b 100644 > --- a/arch/sandbox/lib/board.c > +++ b/arch/sandbox/lib/board.c > @@ -45,8 +45,12 @@ > #include <version.h> > #include <serial.h> > > +#include <os.h> > + > DECLARE_GLOBAL_DATA_PTR; > > +static gd_t gd_mem; > + > /************************************************************************ > * Init Utilities * > ************************************************************************ > @@ -112,7 +116,7 @@ typedef int (init_fnc_t) (void); > > void __dram_init_banksize(void) > { > - gd->bd->bi_dram[0].start = 0; > + gd->bd->bi_dram[0].start = (ulong) gd->ram_buf; > gd->bd->bi_dram[0].size = gd->ram_size; > } > > @@ -147,7 +151,7 @@ void board_init_f(ulong bootflag) > uchar *mem; > unsigned long addr_sp, addr, size; > > - gd = malloc(sizeof(gd_t)); > + gd = &gd_mem; > assert(gd); > > memset((void *)gd, 0, sizeof(gd_t)); > @@ -158,7 +162,8 @@ void board_init_f(ulong bootflag) > } > > size = CONFIG_SYS_SDRAM_SIZE; > - mem = malloc(size); > + mem = os_mmap((void *)CONFIG_SYS_SDRAM_BASE, CONFIG_SYS_SDRAM_SIZE, What is the base used for here? > + 0, 0, -1, 0); > assert(mem); > gd->ram_buf = mem; > addr = (ulong)(mem + size); > @@ -214,11 +219,9 @@ void board_init_r(gd_t *id, ulong dest_addr) > post_output_backlog(); > #endif > > -#if 0 /* Sandbox uses system malloc for now */ > - /* The Malloc area is immediately below the monitor copy in DRAM */ > - malloc_start = dest_addr - TOTAL_MALLOC_LEN; > - mem_malloc_init(malloc_start, TOTAL_MALLOC_LEN); > -#endif > + /* The Malloc area is at the top of simulated DRAM */ > + mem_malloc_init(gd->ram_buf + gd->ram_size - TOTAL_MALLOC_LEN, > + TOTAL_MALLOC_LEN); > > /* initialize environment */ > env_relocate(); > diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h > index 0230256..ce4675d 100644 > --- a/include/configs/sandbox.h > +++ b/include/configs/sandbox.h > @@ -60,6 +60,7 @@ > #define CONFIG_PHYS_64BIT > > /* Size of our emulated memory */ > +#define CONFIG_SYS_SDRAM_BASE 0x20000000 > #define CONFIG_SYS_SDRAM_SIZE (128 << 20) > > #define CONFIG_BAUDRATE 115200 > diff --git a/include/os.h b/include/os.h > index 3ea6d2d..ed4cd4f 100644 > --- a/include/os.h > +++ b/include/os.h > @@ -71,3 +71,16 @@ int os_close(int fd); > * @param exit_code exit code for U-Boot > */ > void os_exit(int exit_code); > + > +/** > + * Access to the OS mmap() system call. Currently all > + * > + * \param addr Prefered address of the mapping > + * \param length Length in bytes > + * \param prot Ignored. Always PROT_READ | PROT_WRITE > + * \param flags Ignored. Always MAP_PRIVATE | MAP_ANONYMOUS > + * \param fd Ignored. Always -1 > + * \param offset Ignored. Always 0 Hmm please see above - we don't have to mimic the POSIX API exactly. The preferred address doesn't really matter since in sandbox it will be accessed as an offset from the start. Apart from length the other parameters should probably not be included. Regards, Simon > + */ > +void *os_mmap(void *addr, size_t length, int prot, int flags, int fd, > + off_t offset); > -- > 1.7.4.1 > >
Hi Matthias, On Tue, Nov 1, 2011 at 9:05 AM, Matthias Weisser <weisserm@arcor.de> wrote: > Am 01.11.2011 16:28, schrieb Mike Frysinger: >> On Tuesday 01 November 2011 10:07:31 Matthias Weisser wrote: >>> --- a/arch/sandbox/cpu/os.c >>> +++ b/arch/sandbox/cpu/os.c >>> >>> +void *os_mmap(void *addr, size_t length, int prot, int flags, int fd, >>> + off_t offset) >>> +{ >>> + return mmap((void *)addr, length, PROT_READ | PROT_WRITE, >> >> void cast here is unnecessary > > Right. > >>> --- a/arch/sandbox/lib/board.c >>> +++ b/arch/sandbox/lib/board.c >>> >>> +static gd_t gd_mem; >> >> i don't think this indirection is necessary. probably can replace the static >> gd_mem with: >> gd_t gd; > > AFAIK gd is a pointer. So I think we always need some sort of memory > where the actual structure is stored. Yes I think that's right. > >>> +#define CONFIG_SYS_SDRAM_BASE 0x20000000 >>> ... >>> - mem = malloc(size); >>> + mem = os_mmap((void *)CONFIG_SYS_SDRAM_BASE, CONFIG_SYS_SDRAM_SIZE, >>> + 0, 0, -1, 0); >> >> mmap() makes no guarantee that the requested address is what you'll get back. >> so there's no point in having this SDRAM_BASE define. punt it and pass in NULL >> instead. > > But it works in most cases :-) The point of adding it was that I really > like to have memory aligned on a 256MB boundary or so like it is in most > SOCs. But thats a personal preference. And if it doesn't work you can > still get the address of physical memory from bdinfo. Most? > >> also, with Simon's other patch to md to use the remap func, the address of our >> memory backing store doesn't matter. > > Well, you are right. But with the posted patch you don't need any remap > function and md/mm/mtest/mw works out of the box. This is interesting. What is the test purpose of specifying a particular virtual address for your memory? If this is useful then we should make the mmap function fail if it cannot honour the address, since otherwise presumably some tests will fail. Regards, Simon > > Regards > Matthias > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot >
On Tuesday 01 November 2011 12:05:05 Matthias Weisser wrote: > Am 01.11.2011 16:28, schrieb Mike Frysinger: > > On Tuesday 01 November 2011 10:07:31 Matthias Weisser wrote: > >> --- a/arch/sandbox/lib/board.c > >> +++ b/arch/sandbox/lib/board.c > >> > >> +static gd_t gd_mem; > > > > i don't think this indirection is necessary. probably can replace the > > static > > > > gd_mem with: > > gd_t gd; > > AFAIK gd is a pointer. So I think we always need some sort of memory > where the actual structure is stored. only if you use the DECLARE macro. but it's fine either way. > >> +#define CONFIG_SYS_SDRAM_BASE 0x20000000 > >> ... > >> - mem = malloc(size); > >> + mem = os_mmap((void *)CONFIG_SYS_SDRAM_BASE, CONFIG_SYS_SDRAM_SIZE, > >> + 0, 0, -1, 0); > > > > mmap() makes no guarantee that the requested address is what you'll get > > back. so there's no point in having this SDRAM_BASE define. punt it and > > pass in NULL instead. > > But it works in most cases :-) The point of adding it was that I really > like to have memory aligned on a 256MB boundary or so like it is in most > SOCs. But thats a personal preference. And if it doesn't work you can > still get the address of physical memory from bdinfo. > > > also, with Simon's other patch to md to use the remap func, the address > > of our memory backing store doesn't matter. > > Well, you are right. But with the posted patch you don't need any remap > function and md/mm/mtest/mw works out of the box. except for when that base address happens to already be used, then any assumptions about memory addresses are now invalid. the internal memory layout should be disconnected from the user-api facing layout. so while internally sandbox does an anon mmap of some chunk of memory, where that is based should not matter. the memory as seen by commands run from the console should always be the same. which is what Simon's patch gets us. as for sandbox working the same as random SoCs, that's going to be highly SoC and architecture specific. i'd keep it simple and have sandbox expose its chunk of memory starting at address 0 and work its way up. -mike
Am 01.11.2011 17:45, schrieb Simon Glass: >>>> +#define CONFIG_SYS_SDRAM_BASE 0x20000000 >>>> ... >>>> - mem = malloc(size); >>>> + mem = os_mmap((void *)CONFIG_SYS_SDRAM_BASE, CONFIG_SYS_SDRAM_SIZE, >>>> + 0, 0, -1, 0); >>> >>> mmap() makes no guarantee that the requested address is what you'll get back. >>> so there's no point in having this SDRAM_BASE define. punt it and pass in NULL >>> instead. >> >> But it works in most cases :-) The point of adding it was that I really >> like to have memory aligned on a 256MB boundary or so like it is in most >> SOCs. But thats a personal preference. And if it doesn't work you can >> still get the address of physical memory from bdinfo. > > Most? I don't know the memory layout strategies for all the Linux/Unix variant out there. But typically text/bss/data is in the lower address range (some megs above 0) and in the upper range (right under kernel space) there is space used for stack and shared objects. Even with ASLR the address I used in the patch should be mappable. >>> also, with Simon's other patch to md to use the remap func, the address of our >>> memory backing store doesn't matter. >> >> Well, you are right. But with the posted patch you don't need any remap >> function and md/mm/mtest/mw works out of the box. > > This is interesting. What is the test purpose of specifying a > particular virtual address for your memory? Emulating the behavior of a u-boot on real hardware as close as possible. We have some chunk of memory at a given location. Thats it. With a minimum of additional work we can also simulate additional banks of RAM using multiple mmaps. > If this is useful then we should make the mmap function fail if it > cannot honour the address, since otherwise presumably some tests will > fail. Maybe. But on the other hand tests can always extract the actual address of RAM from gd->bd->bi_dram[0].start. If test use these address we can abandon the use of of fixed address. But I still thinks its nice to have an aligned address of RAM start as this is what we have on (all?) real hardware. Regards Matthias
On Tuesday 01 November 2011 14:37:24 Matthias Weisser wrote: > I don't know the memory layout strategies for all the Linux/Unix variant > out there. But typically text/bss/data is in the lower address range > (some megs above 0) and in the upper range (right under kernel space) > there is space used for stack and shared objects. Even with ASLR the > address I used in the patch should be mappable. relying on any address layout behavior is doomed to fail. and in this case, it's unnecessary. so let's not bother. > >>> also, with Simon's other patch to md to use the remap func, the address > >>> of our memory backing store doesn't matter. > >> > >> Well, you are right. But with the posted patch you don't need any remap > >> function and md/mm/mtest/mw works out of the box. > > > > This is interesting. What is the test purpose of specifying a > > particular virtual address for your memory? > > Emulating the behavior of a u-boot on real hardware as close as > possible. We have some chunk of memory at a given location. Thats it. > With a minimum of additional work we can also simulate additional banks > of RAM using multiple mmaps. adding a CONFIG knob to control the virtual address of the memory is fine > > If this is useful then we should make the mmap function fail if it > > cannot honour the address, since otherwise presumably some tests will > > fail. > > Maybe. But on the other hand tests can always extract the actual address > of RAM from gd->bd->bi_dram[0].start. If test use these address we can > abandon the use of of fixed address. But I still thinks its nice to have > an aligned address of RAM start as this is what we have on (all?) real > hardware. i'd rather see bi_dram die than extend it. if you want to utilize the virtual memory, let's use the already existing map helper. -mike
Am 01.11.2011 19:52, schrieb Mike Frysinger: > On Tuesday 01 November 2011 14:37:24 Matthias Weisser wrote: >> I don't know the memory layout strategies for all the Linux/Unix variant >> out there. But typically text/bss/data is in the lower address range >> (some megs above 0) and in the upper range (right under kernel space) >> there is space used for stack and shared objects. Even with ASLR the >> address I used in the patch should be mappable. > > relying on any address layout behavior is doomed to fail. and in this case, > it's unnecessary. so let's not bother. Accepted but not convinced :-) As the address passed to mmap is only a hint it will not fail. But I will remove the hint in V2 of the patch. Regards Matthias
On Tuesday 01 November 2011 15:01:55 Matthias Weisser wrote: > Am 01.11.2011 19:52, schrieb Mike Frysinger: > > On Tuesday 01 November 2011 14:37:24 Matthias Weisser wrote: > >> I don't know the memory layout strategies for all the Linux/Unix variant > >> out there. But typically text/bss/data is in the lower address range > >> (some megs above 0) and in the upper range (right under kernel space) > >> there is space used for stack and shared objects. Even with ASLR the > >> address I used in the patch should be mappable. > > > > relying on any address layout behavior is doomed to fail. and in this > > case, it's unnecessary. so let's not bother. > > Accepted but not convinced :-) As the address passed to mmap is only a > hint it will not fail. But I will remove the hint in V2 of the patch. by "fail" i meant "relying on the address being the same all the time". not "mmap returned MAP_FAILED". -mike
Am 01.11.2011 21:10, schrieb Mike Frysinger: > On Tuesday 01 November 2011 15:01:55 Matthias Weisser wrote: >> Am 01.11.2011 19:52, schrieb Mike Frysinger: >>> On Tuesday 01 November 2011 14:37:24 Matthias Weisser wrote: >>>> I don't know the memory layout strategies for all the Linux/Unix variant >>>> out there. But typically text/bss/data is in the lower address range >>>> (some megs above 0) and in the upper range (right under kernel space) >>>> there is space used for stack and shared objects. Even with ASLR the >>>> address I used in the patch should be mappable. >>> >>> relying on any address layout behavior is doomed to fail. and in this >>> case, it's unnecessary. so let's not bother. >> >> Accepted but not convinced :-) As the address passed to mmap is only a >> hint it will not fail. But I will remove the hint in V2 of the patch. > > by "fail" i meant "relying on the address being the same all the time". not > "mmap returned MAP_FAILED". > -mike I just implemented it as suggested. One thing I observed now is that an unhinted mmap now returns an address > 4GB (on my x86_64 machine) which is directly mapped under the shared object address range. $ cat /proc/9150/maps ... 7f0e6f09b000-7f0e7709b000 rw-p 00000000 00:00 0 # <u-boot RAM 7f0e7709b000-7f0e77225000 r-xp 00000000 08:02 1198174 # libc ... After applying Simons md patch I can now successfully md the 128MBytes of simulated RAM. If I use an "virtual" address close to the end of this area: =>md 7ffff80 md 7ffff80 07ffff80: 00000000 00000000 00000000 00000000 ................ 07ffff90: 00000000 00000000 00000000 00000000 ................ 07ffffa0: 00000000 00000000 00000000 00000000 ................ 07ffffb0: 00000000 00000000 00000000 00000000 ................ 07ffffc0: 00000000 00000000 00000000 00000000 ................ 07ffffd0: 00000000 00000000 00000000 00000000 ................ 07ffffe0: 00000000 00000000 00000000 00000000 ................ 07fffff0: 00000000 00000000 00000000 00000000 ................ 08000000: 464c457f 00010102 00000000 00000000 .ELF............ 08000010: 003e0003 00000001 0001f010 00000000 ..>............. 08000020: 00000040 00000000 0018ed28 00000000 @.......(....... 08000030: 00000000 00380040 0040000a 00460047 ....@.8...@.G.F. 08000040: 00000006 00000005 00000040 00000000 ........@....... 08000050: 00000040 00000000 00000040 00000000 @.......@....... 08000060: 00000230 00000000 00000230 00000000 0.......0....... 08000070: 00000008 00000000 00000003 00000004 ................ So, as you can see, dumping to far is not faulting. It just dumps (in this case) the first bytes of my local libc. Regards Matthias
On Wednesday 02 November 2011 14:35:55 Matthias Weisser wrote: > So, as you can see, dumping to far is not faulting. It just dumps (in > this case) the first bytes of my local libc. yes, and i think this is acceptable behavior. take any SoC out there, and if you do `md` on addresses that aren't actually backed by memory, you get chip- specific behavior. it could be an exception, a hardware error signal, random garbage, or something else. -mike
diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c index 6c175d4..cd2fb05 100644 --- a/arch/sandbox/cpu/os.c +++ b/arch/sandbox/cpu/os.c @@ -24,6 +24,7 @@ #include <unistd.h> #include <sys/types.h> #include <sys/stat.h> +#include <sys/mman.h> #include <os.h> @@ -53,3 +54,11 @@ void os_exit(int exit_code) { exit(exit_code); } + +void *os_mmap(void *addr, size_t length, int prot, int flags, int fd, + off_t offset) +{ + return mmap((void *)addr, length, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, + -1, 0); +} diff --git a/arch/sandbox/lib/board.c b/arch/sandbox/lib/board.c index ae5a517..0912a8b 100644 --- a/arch/sandbox/lib/board.c +++ b/arch/sandbox/lib/board.c @@ -45,8 +45,12 @@ #include <version.h> #include <serial.h> +#include <os.h> + DECLARE_GLOBAL_DATA_PTR; +static gd_t gd_mem; + /************************************************************************ * Init Utilities * ************************************************************************ @@ -112,7 +116,7 @@ typedef int (init_fnc_t) (void); void __dram_init_banksize(void) { - gd->bd->bi_dram[0].start = 0; + gd->bd->bi_dram[0].start = (ulong) gd->ram_buf; gd->bd->bi_dram[0].size = gd->ram_size; } @@ -147,7 +151,7 @@ void board_init_f(ulong bootflag) uchar *mem; unsigned long addr_sp, addr, size; - gd = malloc(sizeof(gd_t)); + gd = &gd_mem; assert(gd); memset((void *)gd, 0, sizeof(gd_t)); @@ -158,7 +162,8 @@ void board_init_f(ulong bootflag) } size = CONFIG_SYS_SDRAM_SIZE; - mem = malloc(size); + mem = os_mmap((void *)CONFIG_SYS_SDRAM_BASE, CONFIG_SYS_SDRAM_SIZE, + 0, 0, -1, 0); assert(mem); gd->ram_buf = mem; addr = (ulong)(mem + size); @@ -214,11 +219,9 @@ void board_init_r(gd_t *id, ulong dest_addr) post_output_backlog(); #endif -#if 0 /* Sandbox uses system malloc for now */ - /* The Malloc area is immediately below the monitor copy in DRAM */ - malloc_start = dest_addr - TOTAL_MALLOC_LEN; - mem_malloc_init(malloc_start, TOTAL_MALLOC_LEN); -#endif + /* The Malloc area is at the top of simulated DRAM */ + mem_malloc_init(gd->ram_buf + gd->ram_size - TOTAL_MALLOC_LEN, + TOTAL_MALLOC_LEN); /* initialize environment */ env_relocate(); diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h index 0230256..ce4675d 100644 --- a/include/configs/sandbox.h +++ b/include/configs/sandbox.h @@ -60,6 +60,7 @@ #define CONFIG_PHYS_64BIT /* Size of our emulated memory */ +#define CONFIG_SYS_SDRAM_BASE 0x20000000 #define CONFIG_SYS_SDRAM_SIZE (128 << 20) #define CONFIG_BAUDRATE 115200 diff --git a/include/os.h b/include/os.h index 3ea6d2d..ed4cd4f 100644 --- a/include/os.h +++ b/include/os.h @@ -71,3 +71,16 @@ int os_close(int fd); * @param exit_code exit code for U-Boot */ void os_exit(int exit_code); + +/** + * Access to the OS mmap() system call. Currently all + * + * \param addr Prefered address of the mapping + * \param length Length in bytes + * \param prot Ignored. Always PROT_READ | PROT_WRITE + * \param flags Ignored. Always MAP_PRIVATE | MAP_ANONYMOUS + * \param fd Ignored. Always -1 + * \param offset Ignored. Always 0 + */ +void *os_mmap(void *addr, size_t length, int prot, int flags, int fd, + off_t offset);
Using mmap we can simulate RAM at a specific address. This also eliminated the use of system malloc function. Signed-off-by: Matthias Weisser <weisserm@arcor.de> --- arch/sandbox/cpu/os.c | 9 +++++++++ arch/sandbox/lib/board.c | 19 +++++++++++-------- include/configs/sandbox.h | 1 + include/os.h | 13 +++++++++++++ 4 files changed, 34 insertions(+), 8 deletions(-)