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

[Simon Glass]

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 :-)

>
>> +}
>> +
>> +/**
>> + * phys_to_virt() - Converts a sandbox RAM address to a pointer
>> + *
>> + * Sandbox uses U-Boot addresses from 0 to the size of DRAM. These index
>> into
>> + * the emulated DRAM buffer used by sandbox. This function converts such
>> an
>> + * address to a pointer into thi sbuffer, which can be used to access the
>
>
> this

OK fixed, ta.

>
> ...
>
> Thinking about this, wouldn't it be easier to just allocate the stack inside
> U-Boot RAM?

We could change the tests so that instead of using local variables
they call a function to allocate sandbox RAM. But I'm not sure it is
easier.

This implementation of sandbox mapping is what I envisaged ages ago
when writing the original implementation. I just never got around to
it since for most cases the simple code works. For PCI I added a
hack...

So I think it is a good time to add it.

[..]

Regards,
Simon