Message ID | 20210610102617.17281-1-alex.bennee@linaro.org |
---|---|
State | New |
Headers | show |
Series | [v2] semihosting/arm-compat: remove heuristic softmmu SYS_HEAPINFO | expand |
On Thu, 10 Jun 2021 at 11:26, Alex Bennée <alex.bennee@linaro.org> wrote: > > The previous numbers were a guess at best. While we could extract the > information from a loaded ELF file via -kernel we could still get > tripped up by self decompressing or relocating code. Besides sane > library code like newlib will fall back to known symbols to determine > of the location of the heap. We can still report the limits though as > we are reasonably confident that busting out of RAM would be a bad > thing for either stack or heap. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Tested-by: Andrew Strauss <astrauss11@gmail.com> > Reviewed-by: Andrew Strauss <astrauss11@gmail.com> > Message-Id: <20210601090715.22330-1-alex.bennee@linaro.org> > > --- > v2 > - report some known information (limits) > - reword the commit message > --- > semihosting/arm-compat-semi.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c > index 1c29146dcf..8873486e8c 100644 > --- a/semihosting/arm-compat-semi.c > +++ b/semihosting/arm-compat-semi.c > @@ -1202,10 +1202,14 @@ target_ulong do_common_semihosting(CPUState *cs) > retvals[3] = 0; /* Stack limit. */ > #else > limit = current_machine->ram_size; > - /* TODO: Make this use the limit of the loaded application. */ > - retvals[0] = rambase + limit / 2; > - retvals[1] = rambase + limit; > - retvals[2] = rambase + limit; /* Stack base */ > + /* > + * Reporting 0 indicates we couldn't calculate the real > + * values which should force most software to fall back to > + * using information it has. > + */ > + retvals[0] = 0; /* Heap Base */ > + retvals[1] = rambase + limit; /* Heap Limit */ > + retvals[2] = 0; /* Stack base */ > retvals[3] = rambase; /* Stack limit. */ The spec: https://github.com/ARM-software/abi-aa/blob/main/semihosting/semihosting.rst#sys-heapinfo-0x16 doesn't document that 'return 0 for "I don't know"' is valid, so if we're going to do it we ought to at least note that we're deviating from the spec here. -- PMM
On Thu, 10 Jun 2021 at 11:26, Alex Bennée <alex.bennee@linaro.org> wrote: > > The previous numbers were a guess at best. While we could extract the > information from a loaded ELF file via -kernel we could still get > tripped up by self decompressing or relocating code. Besides sane > library code like newlib will fall back to known symbols to determine > of the location of the heap. We can still report the limits though as > we are reasonably confident that busting out of RAM would be a bad > thing for either stack or heap. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Tested-by: Andrew Strauss <astrauss11@gmail.com> > Reviewed-by: Andrew Strauss <astrauss11@gmail.com> > Message-Id: <20210601090715.22330-1-alex.bennee@linaro.org> > > --- > v2 > - report some known information (limits) > - reword the commit message > --- > semihosting/arm-compat-semi.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c > index 1c29146dcf..8873486e8c 100644 > --- a/semihosting/arm-compat-semi.c > +++ b/semihosting/arm-compat-semi.c > @@ -1202,10 +1202,14 @@ target_ulong do_common_semihosting(CPUState *cs) > retvals[3] = 0; /* Stack limit. */ > #else > limit = current_machine->ram_size; > - /* TODO: Make this use the limit of the loaded application. */ > - retvals[0] = rambase + limit / 2; > - retvals[1] = rambase + limit; > - retvals[2] = rambase + limit; /* Stack base */ > + /* > + * Reporting 0 indicates we couldn't calculate the real > + * values which should force most software to fall back to > + * using information it has. > + */ > + retvals[0] = 0; /* Heap Base */ > + retvals[1] = rambase + limit; /* Heap Limit */ > + retvals[2] = 0; /* Stack base */ > retvals[3] = rambase; /* Stack limit. */ > #endif I'm told that the Arm C compiler C library always assumes that the "stack base" value is what it should set SP to, so reporting 0 for that will break binaries that were built with it. As the TODO comment notes, the "heap base" is a bit of a guess, but putting stackbase at top-of-RAM seems generally sensible. What bug are we trying to fix here? I think one possible implementation that might not be too hard to make work would be: (1) find the guest physical address of the main machine RAM (machine->ram). You can do this with flatview_for_each_range() similar to what rom_ptr_for_as() does. (It might be mapped more than once, we could just pick the first one.) (2) find the largest contiguous extent of that RAM which is not covered by a ROM blob, by iterating through the ROM blob data. (This sounds like one of those slightly irritating but entirely tractable algorithms questions :-)) (3) report the lowest address of that extent as the heap base and the stack limit, and the highest address as the heap limit and the stack base. This would work for all guest architectures and remove the need for that Arm-specific code... -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Thu, 10 Jun 2021 at 11:26, Alex Bennée <alex.bennee@linaro.org> wrote: >> >> The previous numbers were a guess at best. While we could extract the >> information from a loaded ELF file via -kernel we could still get >> tripped up by self decompressing or relocating code. Besides sane >> library code like newlib will fall back to known symbols to determine >> of the location of the heap. We can still report the limits though as >> we are reasonably confident that busting out of RAM would be a bad >> thing for either stack or heap. >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> Tested-by: Andrew Strauss <astrauss11@gmail.com> >> Reviewed-by: Andrew Strauss <astrauss11@gmail.com> >> Message-Id: <20210601090715.22330-1-alex.bennee@linaro.org> >> >> --- >> v2 >> - report some known information (limits) >> - reword the commit message >> --- >> semihosting/arm-compat-semi.c | 12 ++++++++---- >> 1 file changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c >> index 1c29146dcf..8873486e8c 100644 >> --- a/semihosting/arm-compat-semi.c >> +++ b/semihosting/arm-compat-semi.c >> @@ -1202,10 +1202,14 @@ target_ulong do_common_semihosting(CPUState *cs) >> retvals[3] = 0; /* Stack limit. */ >> #else >> limit = current_machine->ram_size; >> - /* TODO: Make this use the limit of the loaded application. */ >> - retvals[0] = rambase + limit / 2; >> - retvals[1] = rambase + limit; >> - retvals[2] = rambase + limit; /* Stack base */ >> + /* >> + * Reporting 0 indicates we couldn't calculate the real >> + * values which should force most software to fall back to >> + * using information it has. >> + */ >> + retvals[0] = 0; /* Heap Base */ >> + retvals[1] = rambase + limit; /* Heap Limit */ >> + retvals[2] = 0; /* Stack base */ >> retvals[3] = rambase; /* Stack limit. */ > > The spec: > https://github.com/ARM-software/abi-aa/blob/main/semihosting/semihosting.rst#sys-heapinfo-0x16 > > doesn't document that 'return 0 for "I don't know"' is valid, so if we're > going to do it we ought to at least note that we're deviating from the > spec here. That comes from newlib who state: /* We skip setting SP/SL if 0 returned from semihosting. - According to semihosting docs, if 0 returned from semihosting, the system was unable to calculate the real value, so it's ok to skip setting SP/SL to 0 here. - Considering M-profile processors, We might want to initialize SP by the first entry of vector table and return 0 to SYS_HEAPINFO semihosting call, which will be skipped here. - Considering R-profile processors there is no automatic SP init by hardware so we need to initialize it by default value. */ that doesn't come from the actual semihosting spec but the ARM Compiler Development guide (ARM DUI0471M) which has a more detailed entry: 7.13 SYS_HEAPINFO (0x16) Returns the system stack and heap parameters. The values returned are typically those used by the C library during initialization. For a debug hardwareunit, such as RVI or DSTREAM, the values returned are the image location and the top of memory. The C library can override these values. The host debugger determines the actual values to return by using the top_of_memory debugger variable. <snip> Note If word one of the data block has the value zero, the C library replaces the zero with Image$$ZI$$Limit.This value corresponds to the top of the data region in the memory map. Return On exit, R1 contains the address of the pointer to the structure.If one of the values in the structure is 0, the system was unable to calculate the real value. which I think is the basis for the newlib fallback behaviour. -- Alex Bennée
Peter Maydell <peter.maydell@linaro.org> writes: > On Thu, 10 Jun 2021 at 11:26, Alex Bennée <alex.bennee@linaro.org> wrote: >> >> The previous numbers were a guess at best. While we could extract the >> information from a loaded ELF file via -kernel we could still get >> tripped up by self decompressing or relocating code. Besides sane >> library code like newlib will fall back to known symbols to determine >> of the location of the heap. We can still report the limits though as >> we are reasonably confident that busting out of RAM would be a bad >> thing for either stack or heap. >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> Tested-by: Andrew Strauss <astrauss11@gmail.com> >> Reviewed-by: Andrew Strauss <astrauss11@gmail.com> >> Message-Id: <20210601090715.22330-1-alex.bennee@linaro.org> >> >> --- >> v2 >> - report some known information (limits) >> - reword the commit message >> --- >> semihosting/arm-compat-semi.c | 12 ++++++++---- >> 1 file changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c >> index 1c29146dcf..8873486e8c 100644 >> --- a/semihosting/arm-compat-semi.c >> +++ b/semihosting/arm-compat-semi.c >> @@ -1202,10 +1202,14 @@ target_ulong do_common_semihosting(CPUState *cs) >> retvals[3] = 0; /* Stack limit. */ >> #else >> limit = current_machine->ram_size; >> - /* TODO: Make this use the limit of the loaded application. */ >> - retvals[0] = rambase + limit / 2; >> - retvals[1] = rambase + limit; >> - retvals[2] = rambase + limit; /* Stack base */ >> + /* >> + * Reporting 0 indicates we couldn't calculate the real >> + * values which should force most software to fall back to >> + * using information it has. >> + */ >> + retvals[0] = 0; /* Heap Base */ >> + retvals[1] = rambase + limit; /* Heap Limit */ >> + retvals[2] = 0; /* Stack base */ >> retvals[3] = rambase; /* Stack limit. */ >> #endif > > I'm told that the Arm C compiler C library always assumes that > the "stack base" value is what it should set SP to, so reporting 0 > for that will break binaries that were built with it. > > As the TODO comment notes, the "heap base" is a bit of a guess, > but putting stackbase at top-of-RAM seems generally sensible. > > What bug are we trying to fix here? Having newlib use a value that's wrong and therefor plant it's heap in the middle of the loaded code. > I think one possible implementation that might not be too > hard to make work would be: > > (1) find the guest physical address of the main machine > RAM (machine->ram). You can do this with flatview_for_each_range() > similar to what rom_ptr_for_as() does. (It might be mapped > more than once, we could just pick the first one.) Currently this is done by common_semi_find_region_base which pokes around get_system_memory()->subregions to find a region containing an initialised register pointer. > (2) find the largest contiguous extent of that RAM which > is not covered by a ROM blob, by iterating through the > ROM blob data. (This sounds like one of those slightly > irritating but entirely tractable algorithms questions :-)) Does that assume that any rom blob (so anything from -kernel, -pflash or -generic-loader?) will have also included space for guest data and bss? > (3) report the lowest address of that extent as the heap base > and the stack limit, and the highest address as the heap > limit and the stack base. > > This would work for all guest architectures and remove the need > for that Arm-specific code... > > -- PMM -- Alex Bennée
On Thu, 10 Jun 2021 at 15:16, Alex Bennée <alex.bennee@linaro.org> wrote: > > > Peter Maydell <peter.maydell@linaro.org> writes: > > I'm told that the Arm C compiler C library always assumes that > > the "stack base" value is what it should set SP to, so reporting 0 > > for that will break binaries that were built with it. > > > > As the TODO comment notes, the "heap base" is a bit of a guess, > > but putting stackbase at top-of-RAM seems generally sensible. > > > > What bug are we trying to fix here? > > Having newlib use a value that's wrong and therefor plant it's heap in > the middle of the loaded code. > > > I think one possible implementation that might not be too > > hard to make work would be: > > > > (1) find the guest physical address of the main machine > > RAM (machine->ram). You can do this with flatview_for_each_range() > > similar to what rom_ptr_for_as() does. (It might be mapped > > more than once, we could just pick the first one.) > > Currently this is done by common_semi_find_region_base which pokes > around get_system_memory()->subregions to find a region containing an > initialised register pointer. Yes. I am suggesting we throw that code away, since (a) assuming any register happens to point in to the main RAM is dubious and (b) iterating through the subregions of get_system_memory() is not guaranteed to work either (consider the case where the system memory is inside a container MR rather than a direct child of the system memory MR). > > (2) find the largest contiguous extent of that RAM which > > is not covered by a ROM blob, by iterating through the > > ROM blob data. (This sounds like one of those slightly > > irritating but entirely tractable algorithms questions :-)) > > Does that assume that any rom blob (so anything from -kernel, -pflash or > -generic-loader?) will have also included space for guest data and bss? Yes; the elf loader code creates rom blobs whose rom->romsize covers both initialized data from the ELF file and space to be zeroed. thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Thu, 10 Jun 2021 at 15:16, Alex Bennée <alex.bennee@linaro.org> wrote: >> >> >> Peter Maydell <peter.maydell@linaro.org> writes: >> > I'm told that the Arm C compiler C library always assumes that >> > the "stack base" value is what it should set SP to, so reporting 0 >> > for that will break binaries that were built with it. >> > >> > As the TODO comment notes, the "heap base" is a bit of a guess, >> > but putting stackbase at top-of-RAM seems generally sensible. >> > >> > What bug are we trying to fix here? >> >> Having newlib use a value that's wrong and therefor plant it's heap in >> the middle of the loaded code. >> >> > I think one possible implementation that might not be too >> > hard to make work would be: >> > >> > (1) find the guest physical address of the main machine >> > RAM (machine->ram). You can do this with flatview_for_each_range() >> > similar to what rom_ptr_for_as() does. (It might be mapped >> > more than once, we could just pick the first one.) >> >> Currently this is done by common_semi_find_region_base which pokes >> around get_system_memory()->subregions to find a region containing an >> initialised register pointer. > > Yes. I am suggesting we throw that code away, since (a) assuming > any register happens to point in to the main RAM is dubious and > (b) iterating through the subregions of get_system_memory() is > not guaranteed to work either (consider the case where the system > memory is inside a container MR rather than a direct child of the > system memory MR). > >> > (2) find the largest contiguous extent of that RAM which >> > is not covered by a ROM blob, by iterating through the >> > ROM blob data. (This sounds like one of those slightly >> > irritating but entirely tractable algorithms questions :-)) >> >> Does that assume that any rom blob (so anything from -kernel, -pflash or >> -generic-loader?) will have also included space for guest data and bss? > > Yes; the elf loader code creates rom blobs whose rom->romsize > covers both initialized data from the ELF file and space to > be zeroed. Hmm I'm not seeing the RAM get bifurcated by the loader. The flatview only has one RAM block in my test case and it covers the whole of RAM. Semihosting Heap Info Test find_heap_cb: rom:1 romd_mode:1 ram:0 0000000000000000/0000000000000000:4000000 find_heap_cb: rom:1 romd_mode:1 ram:0 0000000004000000/0000000004000000:4000000 find_heap_cb: rom:0 romd_mode:1 ram:0 0000000008000000/0000000008000000:1000 find_heap_cb: rom:0 romd_mode:1 ram:0 0000000008010000/0000000008010000:2000 find_heap_cb: rom:0 romd_mode:1 ram:0 0000000008020000/0000000008020000:1000 find_heap_cb: rom:0 romd_mode:1 ram:0 0000000009000000/0000000009000000:1000 find_heap_cb: rom:0 romd_mode:1 ram:0 0000000009010000/0000000009010000:1000 find_heap_cb: rom:0 romd_mode:1 ram:0 0000000009020000/0000000009020000:8 find_heap_cb: rom:0 romd_mode:1 ram:0 0000000009020008/0000000009020008:2 find_heap_cb: rom:0 romd_mode:1 ram:0 0000000009020010/0000000009020010:8 find_heap_cb: rom:0 romd_mode:1 ram:0 0000000009030000/0000000009030000:1000 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a000000/000000000a000000:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a000200/000000000a000200:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a000400/000000000a000400:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a000600/000000000a000600:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a000800/000000000a000800:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a000a00/000000000a000a00:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a000c00/000000000a000c00:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a000e00/000000000a000e00:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a001000/000000000a001000:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a001200/000000000a001200:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a001400/000000000a001400:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a001600/000000000a001600:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a001800/000000000a001800:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a001a00/000000000a001a00:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a001c00/000000000a001c00:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a001e00/000000000a001e00:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a002000/000000000a002000:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a002200/000000000a002200:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a002400/000000000a002400:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a002600/000000000a002600:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a002800/000000000a002800:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a002a00/000000000a002a00:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a002c00/000000000a002c00:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a002e00/000000000a002e00:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a003000/000000000a003000:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a003200/000000000a003200:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a003400/000000000a003400:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a003600/000000000a003600:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a003800/000000000a003800:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a003a00/000000000a003a00:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a003c00/000000000a003c00:200 find_heap_cb: rom:0 romd_mode:1 ram:0 000000000a003e00/000000000a003e00:200 find_heap_cb: rom:0 romd_mode:1 ram:0 0000000000000000/0000000000000000:2eff0000 find_heap_cb: rom:0 romd_mode:1 ram:0 000000003eff0000/000000003eff0000:10000 find_heap_cb: rom:0 romd_mode:1 ram:1 0000000040000000/0000000040000000:20000000 find_heap_cb: rom:0 romd_mode:1 ram:0 0000000000000000/0000004010000000:10000000 find_heap_cb: rom:0 romd_mode:1 ram:0 0000000000000000/0000000000000000:8000000000 info appears to be inside the heap: 40211fe0 in 40000000:60000000 > > thanks > -- PMM -- Alex Bennée
On Fri, 11 Jun 2021 at 18:03, Alex Bennée <alex.bennee@linaro.org> wrote: > > > Peter Maydell <peter.maydell@linaro.org> writes: > > > On Thu, 10 Jun 2021 at 15:16, Alex Bennée <alex.bennee@linaro.org> wrote: > >> > >> > >> Peter Maydell <peter.maydell@linaro.org> writes: > >> > (2) find the largest contiguous extent of that RAM which > >> > is not covered by a ROM blob, by iterating through the > >> > ROM blob data. (This sounds like one of those slightly > >> > irritating but entirely tractable algorithms questions :-)) > >> > >> Does that assume that any rom blob (so anything from -kernel, -pflash or > >> -generic-loader?) will have also included space for guest data and bss? > > > > Yes; the elf loader code creates rom blobs whose rom->romsize > > covers both initialized data from the ELF file and space to > > be zeroed. > > Hmm I'm not seeing the RAM get bifurcated by the loader. The flatview > only has one RAM block in my test case and it covers the whole of RAM. I dunno what you're tracing here, but rom blobs do not affect the MemoryRegion setup (which can be rom, ram,the romd "writes go to callbacks, reads are host memory" hybrid, or whatever). ROM blobs are just a list of "write this data into memory at this address", which gets iterated through on reset to write the data into the RAM/ROM/whatever. -- PMM
diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c index 1c29146dcf..8873486e8c 100644 --- a/semihosting/arm-compat-semi.c +++ b/semihosting/arm-compat-semi.c @@ -1202,10 +1202,14 @@ target_ulong do_common_semihosting(CPUState *cs) retvals[3] = 0; /* Stack limit. */ #else limit = current_machine->ram_size; - /* TODO: Make this use the limit of the loaded application. */ - retvals[0] = rambase + limit / 2; - retvals[1] = rambase + limit; - retvals[2] = rambase + limit; /* Stack base */ + /* + * Reporting 0 indicates we couldn't calculate the real + * values which should force most software to fall back to + * using information it has. + */ + retvals[0] = 0; /* Heap Base */ + retvals[1] = rambase + limit; /* Heap Limit */ + retvals[2] = 0; /* Stack base */ retvals[3] = rambase; /* Stack limit. */ #endif