Re: [PATCH v4 2/2] tests/tcg: port SYS_HEAPINFO to a system test

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 23 Jun 2021 at 14:48, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> This allows us to check our new SYS_HEAPINFO implementation generates
>> sane values.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@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