Peter Maydell <peter.mayd...@linaro.org> writes:
> On Wed, 23 Jun 2021 at 14:48, Alex Bennée <alex.ben...@linaro.org> wrote:
>>
>> This allows us to check our new SYS_HEAPINFO implementation generates
>> sane values.
>>
>> Signed-off-by: Alex Bennée <alex.ben...@linaro.org>
>> ---
>> tests/tcg/aarch64/system/semiheap.c | 74 +++++++++++++++++++++++++++++
>> 1 file changed, 74 insertions(+)
>> create mode 100644 tests/tcg/aarch64/system/semiheap.c
>>
>> diff --git a/tests/tcg/aarch64/system/semiheap.c
>> b/tests/tcg/aarch64/system/semiheap.c
>> new file mode 100644
>> index 0000000000..d5613dca59
>> --- /dev/null
>> +++ b/tests/tcg/aarch64/system/semiheap.c
>> @@ -0,0 +1,74 @@
>> +/*
>> + * Semihosting System HEAPINFO Test
>> + *
>> + * Copyright (c) 2021 Linaro Ltd
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#include <inttypes.h>
>> +#include <stddef.h>
>> +#include <minilib.h>
>> +
>> +#define SYS_HEAPINFO 0x16
>> +
>> +uintptr_t __semi_call(uintptr_t type, uintptr_t arg0)
>> +{
>> + register uintptr_t t asm("x0") = type;
>> + register uintptr_t a0 asm("x1") = arg0;
>> + asm("hlt 0xf000"
>> + : "=r" (t)
>> + : "r" (t), "r" (a0));
>
> You should include "memory" in the clobbers list here, or the compiler
> has license to assume that the semihosting call doesn't actually
> write to the struct info.
>
>> +
>> + return t;
>> +}
>> +
>> +int main(int argc, char *argv[argc])
>> +{
>> + struct {
>> + void *heap_base;
>> + void *heap_limit;
>> + void *stack_base;
>> + void *stack_limit;
>> + } info;
>> + void *ptr_to_info = (void *) &info;
>> +
>> + ml_printf("Semihosting Heap Info Test\n");
>> +
>> + /* memset(&info, 0, sizeof(info)); */
>
> Why is this here but commented out ? (If you want to zero initialize
> the struct, using "= { }" when you define it above is simpler.)
>
>> + __semi_call(SYS_HEAPINFO, (uintptr_t) &ptr_to_info);
>> +
>> + if (info.heap_base == NULL || info.heap_limit == NULL) {
>> + ml_printf("null heap: %p -> %p\n", info.heap_base, info.heap_limit);
>> + return -1;
>> + }
>> +
>> + /* Error if heap base is above limit */
>> + if ((uintptr_t) info.heap_base >= (uintptr_t) info.heap_limit) {
>> + ml_printf("heap base %p >= heap_limit %p\n",
>> + info.heap_base, info.heap_limit);
>> + return -2;
>> + }
>> +
>> + if (info.stack_base == NULL) {
>> + ml_printf("null stack: %p -> %p\n", info.stack_base,
>> info.stack_limit);
>> + return -3;
>> + }
>> +
>> + /*
>> + * We don't check our local variables are inside the reported
>> + * stack because the runtime may select a different stack area (as
>> + * our boot.S code does). However we can check we don't clash with
>> + * the heap.
>> + */
>> + if (ptr_to_info > info.heap_base && ptr_to_info < info.heap_limit) {
>> + ml_printf("info appears to be inside the heap: %p in %p:%p\n",
>> + ptr_to_info, info.heap_base, info.heap_limit);
>
> I'm not sure this test is valid -- the 'struct info' is on our stack,
> so it could be anywhere in RAM, including possibly in the big
> range we got back from SYS_HEAPINFO.
It should be in this case because boot.S sets stack to be inside out
data segment.
>
> You could if you liked check that for instance the address of 'main'
> is not inside the heap (assuming that you load this test case with
> the ELF loader, it should be in a rom blob and thus excluded from
> the heap range.)
>
>> + return -4;
>> + }
>> +
>> + ml_printf("heap: %p -> %p\n", info.heap_base, info.heap_limit);
>> + ml_printf("stack: %p <- %p\n", info.stack_limit, info.stack_base);
>> + ml_printf("Passed HeapInfo checks\n");
>> + return 0;
>> +}
>
> It would also be useful to check that you can write to the memory and
> read back the value written (ie that we have not been given
> back a range that's read-only or which is not backed by anything).
> (You might need to jump through a hoop or two to check where your
> current stack is before potentially stomping on it...)
>
> thanks
> -- PMM
--
Alex Bennée
