All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Alex Bennée" <alex.bennee@linaro.org>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm <qemu-arm@nongnu.org>, QEMU Developers <qemu-devel@nongnu.org>
Subject: Re: [PATCH v4 2/2] tests/tcg: port SYS_HEAPINFO to a system test
Date: Wed, 09 Feb 2022 17:25:29 +0000	[thread overview]
Message-ID: <87o83gdk9d.fsf@linaro.org> (raw)
In-Reply-To: <CAFEAcA-UPE5+moyVM-1pJ_gi9fj3t1nWtWfZaZ13hkd6-=L5nw@mail.gmail.com>


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


  reply	other threads:[~2022-02-09 18:18 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-23 13:47 [PATCH v4 0/2] semihosting/next (SYS_HEAPINFO) Alex Bennée
2021-06-23 13:47 ` [PATCH v4 1/2] semihosting/arm-compat: replace heuristic for softmmu SYS_HEAPINFO Alex Bennée
2021-06-28 19:48   ` Peter Maydell
2022-02-09 16:29     ` Alex Bennée
2022-02-09 17:13       ` Peter Maydell
2021-06-23 13:47 ` [PATCH v4 2/2] tests/tcg: port SYS_HEAPINFO to a system test Alex Bennée
2021-06-28 20:01   ` Peter Maydell
2022-02-09 17:25     ` Alex Bennée [this message]
2022-02-09 17:44       ` Peter Maydell
2022-02-09 18:14         ` Alex Bennée
2022-02-09 19:02           ` Peter Maydell

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87o83gdk9d.fsf@linaro.org \
    --to=alex.bennee@linaro.org \
    --cc=peter.maydell@linaro.org \
    --cc=qemu-arm@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.