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

[Alexander Graf]

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.

> +}
> +
> +/**
> + * 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

...

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


Alex

> + * memory.
> + *
> + * If the address is outside this range, it is assumed to be a tag
> + */
>   void *phys_to_virt(phys_addr_t paddr)
>   {
> -	return (void *)(gd->arch.ram_buf + paddr);
> +	struct sandbox_mapmem_entry *mentry;
> +	struct sandbox_state *state;
> +
> +	/* If the address is within emulated DRAM, calculate the value */
> +	if (paddr < gd->ram_size)
> +		return (void *)(gd->arch.ram_buf + paddr);
> +
> +	/*
> +	 * Otherwise search out list of tags for the correct pointer previously
> +	 * created by map_to_sysmem()
> +	 */
> +	state = state_get_current();
> +	list_for_each_entry(mentry, &state->mapmem_head, sibling_node) {
> +		if (mentry->tag == paddr) {
> +			printf("%s: Used map from %lx to %p\n", __func__,
> +			       (ulong)paddr, mentry->ptr);
> +			return mentry->ptr;
> +		}
> +	}
> +
> +	printf("%s: Cannot map sandbox address %lx (SDRAM from 0 to %lx)\n",
> +	       __func__, (ulong)paddr, (ulong)gd->ram_size);
> +	os_abort();
> +
> +	/* Not reached */
> +	return NULL;
> +}
> +
> +struct sandbox_mapmem_entry *find_tag(const void *ptr)
> +{
> +	struct sandbox_mapmem_entry *mentry;
> +	struct sandbox_state *state = state_get_current();
> +
> +	list_for_each_entry(mentry, &state->mapmem_head, sibling_node) {
> +		if (mentry->ptr == ptr) {
> +			debug("%s: Used map from %p to %lx\n", __func__, ptr,
> +			      mentry->tag);
> +			return mentry;
> +		}
> +	}
> +	return NULL;
>   }
>   
> -phys_addr_t virt_to_phys(void *vaddr)
> +phys_addr_t virt_to_phys(void *ptr)
>   {
> -	return (phys_addr_t)((uint8_t *)vaddr - gd->arch.ram_buf);
> +	struct sandbox_mapmem_entry *mentry;
> +
> +	/*
> +	 * If it is in emulated RAM, don't bother looking for a tag. Just
> +	 * calculate the pointer using the provides offset into the RAM buffer.
> +	 */
> +	if (is_in_sandbox_mem(ptr))
> +		return (phys_addr_t)((uint8_t *)ptr - gd->arch.ram_buf);
> +
> +	mentry = find_tag(ptr);
> +	if (!mentry) {
> +		/* Abort so that gdb can be used here */
> +		printf("%s: Cannot map sandbox address %p (SDRAM from 0 to %lx)\n",
> +		       __func__, ptr, (ulong)gd->ram_size);
> +		os_abort();
> +	}
> +	printf("%s: Used map from %p to %lx\n", __func__, ptr, mentry->tag);
> +
> +	return mentry->tag;
>   }
>   
>   void *map_physmem(phys_addr_t paddr, unsigned long len, unsigned long flags)
> @@ -87,24 +177,57 @@ void *map_physmem(phys_addr_t paddr, unsigned long len, unsigned long flags)
>   	return phys_to_virt(paddr);
>   }
>   
> -void unmap_physmem(const void *vaddr, unsigned long flags)
> +void unmap_physmem(const void *ptr, unsigned long flags)
>   {
>   #ifdef CONFIG_PCI
>   	if (map_dev) {
> -		pci_unmap_physmem(vaddr, map_len, map_dev);
> +		pci_unmap_physmem(ptr, map_len, map_dev);
>   		map_dev = NULL;
>   	}
>   #endif
>   }
>   
> -void sandbox_set_enable_pci_map(int enable)
> +phys_addr_t map_to_sysmem(const void *ptr)
>   {
> -	enable_pci_map = enable;
> +	struct sandbox_mapmem_entry *mentry;
> +
> +	/*
> +	 * If it is in emulated RAM, don't bother creating a tag. Just return
> +	 * the offset into the RAM buffer.
> +	 */
> +	if (is_in_sandbox_mem(ptr))
> +		return (u8 *)ptr - gd->arch.ram_buf;
> +
> +	/*
> +	 * See if there is an existing tag with this pointer. If not, set up a
> +	 * new one.
> +	 */
> +	mentry = find_tag(ptr);
> +	if (!mentry) {
> +		struct sandbox_state *state = state_get_current();
> +
> +		mentry = malloc(sizeof(*mentry));
> +		if (!mentry) {
> +			printf("%s: Error: Out of memory\n", __func__);
> +			os_exit(ENOMEM);
> +		}
> +		mentry->tag = state->next_tag++;
> +		mentry->ptr = (void *)ptr;
> +		list_add_tail(&mentry->sibling_node, &state->mapmem_head);
> +		debug("%s: Added map from %p to %lx\n", __func__, ptr,
> +		      (ulong)mentry->tag);
> +	}
> +
> +	/*
> +	 * Return the tag as the address to use. A later call to map_sysmem()
> +	 * will return ptr
> +	 */
> +	return mentry->tag;
>   }
>   
> -phys_addr_t map_to_sysmem(const void *ptr)
> +void sandbox_set_enable_pci_map(int enable)
>   {
> -	return (u8 *)ptr - gd->arch.ram_buf;
> +	enable_pci_map = enable;
>   }
>   
>   void flush_dcache_range(unsigned long start, unsigned long stop)
> diff --git a/arch/sandbox/cpu/state.c b/arch/sandbox/cpu/state.c
> index cc50819ab9..04a11fed55 100644
> --- a/arch/sandbox/cpu/state.c
> +++ b/arch/sandbox/cpu/state.c
> @@ -359,6 +359,14 @@ void state_reset_for_test(struct sandbox_state *state)
>   
>   	memset(&state->wdt, '\0', sizeof(state->wdt));
>   	memset(state->spi, '\0', sizeof(state->spi));
> +
> +	/*
> +	 * Set up the memory tag list. Use the top of emulated SDRAM for the
> +	 * first tag number, since that address offset is outside the legal
> +	 * range, and can be assumed to be a tag.
> +	 */
> +	INIT_LIST_HEAD(&state->mapmem_head);
> +	state->next_tag = state->ram_size;
>   }
>   
>   int state_init(void)
> diff --git a/arch/sandbox/include/asm/state.h b/arch/sandbox/include/asm/state.h
> index 7ed4b512d2..a612ce8944 100644
> --- a/arch/sandbox/include/asm/state.h
> +++ b/arch/sandbox/include/asm/state.h
> @@ -9,6 +9,7 @@
>   #include <config.h>
>   #include <sysreset.h>
>   #include <stdbool.h>
> +#include <linux/list.h>
>   #include <linux/stringify.h>
>   
>   /**
> @@ -45,6 +46,23 @@ struct sandbox_wdt_info {
>   	bool running;
>   };
>   
> +/**
> + * struct sandbox_mapmem_entry - maps pointers to/from U-Boot addresses
> + *
> + * When map_to_sysmem() is called with an address outside sandbox's emulated
> + * RAM, a record is created with a tag that can be used to reference that
> + * pointer. When map_sysmem() is called later with that tag, the pointer will
> + * be returned, just as it would for a normal sandbox address.
> + *
> + * @tag: Address tag (a value which U-Boot uses to refer to the address)
> + * @ptr: Associated pointer for that tag
> + */
> +struct sandbox_mapmem_entry {
> +	ulong tag;
> +	void *ptr;
> +	struct list_head sibling_node;
> +};
> +
>   /* The complete state of the test system */
>   struct sandbox_state {
>   	const char *cmd;		/* Command to execute */
> @@ -78,6 +96,9 @@ struct sandbox_state {
>   
>   	/* Information about Watchdog */
>   	struct sandbox_wdt_info wdt;
> +
> +	ulong next_tag;			/* Next address tag to allocate */
> +	struct list_head mapmem_head;	/* struct sandbox_mapmem_entry */
>   };
>   
>   /* Minimum space we guarantee in the state FDT when calling read/write*/