[U-Boot] [PATCH v8 17/30] sandbox: Enhance map_to_sysmem() to handle foreign pointers

[Simon Glass]

Hi Alex,

On 21 June 2018 at 04:10, Alexander Graf <agraf at suse.de> wrote:
> On 06/21/2018 04:01 AM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 18 June 2018 at 08:50, Alexander Graf <agraf at suse.de> wrote:
>>>
>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>
>>>> At present map_sysmem() maps an address into the sandbox RAM buffer,
>>>> return a pointer, while map_to_sysmem() goes the other way.
>>>>
>>>> The mapping is currently just 1:1 since a case was not found where a
>>>> more
>>>> flexible mapping was needed. PCI does have a separate and more complex
>>>> mapping, but uses its own mechanism.
>>>>
>>>> However this arrange cannot handle one important case, which is where a
>>>> test declares a stack variable and passes a pointer to it into a U-Boot
>>>> function which uses map_to_sysmem() to turn it into a address. Since the
>>>> pointer is not inside emulated DRAM, this will fail.
>>>>
>>>> Add a mapping feature which can handle any such pointer, mapping it to a
>>>> simple tag value which can be passed around in U-Boot as an address.
>>>>
>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>>> ---
>>>>
>>>> Changes in v8: None
>>>> Changes in v7: None
>>>> Changes in v6: None
>>>> Changes in v5: None
>>>> Changes in v4: None
>>>> Changes in v3: None
>>>> Changes in v2: None
>>>>
>>>>    arch/sandbox/cpu/cpu.c           | 141
>>>> +++++++++++++++++++++++++++++--
>>>>    arch/sandbox/cpu/state.c         |   8 ++
>>>>    arch/sandbox/include/asm/state.h |  21 +++++
>>>>    3 files changed, 161 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
>>>> index cde0b055a6..fd77603ebf 100644
>>>> --- a/arch/sandbox/cpu/cpu.c
>>>> +++ b/arch/sandbox/cpu/cpu.c
>>>> @@ -57,14 +57,104 @@ int cleanup_before_linux_select(int flags)
>>>>          return 0;
>>>>    }
>>>>    +/**
>>>> + * is_in_sandbox_mem() - Checks if a pointer is within sandbox's
>>>> emulated
>>>> DRAM
>>>> + *
>>>> + * This provides a way to check if a pointer is owned by sandbox (and
>>>> is
>>>> within
>>>> + * its RAM) or not. Sometimes pointers come from a test which
>>>> conceptually runs
>>>> + * output sandbox, potentially with direct access to the C-library
>>>> malloc()
>>>> + * function, or the sandbox stack (which is not actually within the
>>>> emulated
>>>> + * DRAM.
>>>> + *
>>>> + * Such pointers obviously cannot be mapped into sandbox's DRAM, so we
>>>> must
>>>> + * detect them an process them separately, by recording a mapping to a
>>>> tag,
>>>> + * which we can use to map back to the pointer later.
>>>> + *
>>>> + * @ptr: Pointer to check
>>>> + * @return true if this is within sandbox emulated DRAM, false if not
>>>> + */
>>>> +static bool is_in_sandbox_mem(const void *ptr)
>>>> +{
>>>> +       return (const uint8_t *)ptr >= gd->arch.ram_buf &&
>>>> +               (const uint8_t *)ptr < gd->arch.ram_buf + gd->ram_size;
>>>
>>>
>>> Greater/Less comparisons on pointers are invalid (albeit used) C IIRC.
>>> You
>>> should cast to uintptr_t instead and compare those.
>>
>> Reference? That is news to me :-)
>
>
> https://port70.net/~nsz/c/c11/n1570.html#6.5.8
>
> Point 5. Since we're not in an array or a struct member, we fall into the
> "In all other cases, the behavior is undefined" case. Our compiler people
> basically recommend to convert to uintptr_t first and compare then. That way
> behavior is always defined.

There are two pointers being compared:

 (const uint8_t *)ptr

and

(const uint8_t *)gd->arch.ram_buf + gd->ram_size;

which is just a const uint_8 *, call it 'x' if you like.

The bahaviour is clearly defined in the spec you pointed to:

"both operands are pointers to qualified or unqualified versions of
compatible object types."

Regards,
Simon