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

[Alexander Graf]

On 06/21/2018 09:45 PM, Simon Glass wrote:
> Hi Alex,
>
> On 21 June 2018 at 04:00, 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 :-)
>>>
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * 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.
>>
>> No, what I'm saying is why don't we just in early bootup code move %sp to an
>> address that resides inside the U-Boot address space? Then things would
>> behave just like on real hardware.
> I was hoping that wasn't what you were saying!
>
> I worry about messing with the host system in that way. It seems
> really gross. Is it safe?

I think it's safe - coroutines do it all the time - but it would push us 
into writing architecture specific code for sandbox.

Either way, I don't particularly care whether you want to bloat up the 
mapping logic or swap stack pointers. This is sandbox only realm and you 
can do whatever you wish there ;)


Alex