Message ID | 20180808095433.230882-13-sjg@chromium.org |
---|---|
State | Superseded, archived |
Delegated to: | Alexander Graf |
Headers | show |
Series | efi: Enable sandbox support for EFI loader | expand |
On 08.08.18 11:54, 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@chromium.org> I think you are aware that this logic will fall apart spectacularly if any arithmetic operation happens on the virtual (U-Boot) address, right? So simple code like readl(vaddr + 1); will just fail (hopefully) or (more likely) return a completely incorrect value. I assume this is intentional, but shouldn't the tag increment be something slightly larger then? Alex
Hi Alex, On 26 August 2018 at 19:11, Alexander Graf <agraf@suse.de> wrote: > > > On 08.08.18 11:54, 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@chromium.org> > > I think you are aware that this logic will fall apart spectacularly if > any arithmetic operation happens on the virtual (U-Boot) address, right? > So simple code like > > readl(vaddr + 1); > > will just fail (hopefully) or (more likely) return a completely > incorrect value. > > I assume this is intentional, but shouldn't the tag increment be > something slightly larger then? What do you expect readl() to do on sandbox? At present it is just a no-op. I suppose we could support memory-mapped I/O but it has not been attempted yet. Regards, Simon
On 14.09.18 17:46, Simon Glass wrote: > Hi Alex, > > On 26 August 2018 at 19:11, Alexander Graf <agraf@suse.de> wrote: >> >> >> On 08.08.18 11:54, 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@chromium.org> >> >> I think you are aware that this logic will fall apart spectacularly if >> any arithmetic operation happens on the virtual (U-Boot) address, right? >> So simple code like >> >> readl(vaddr + 1); >> >> will just fail (hopefully) or (more likely) return a completely >> incorrect value. >> >> I assume this is intentional, but shouldn't the tag increment be >> something slightly larger then? > > What do you expect readl() to do on sandbox? At present it is just a > no-op. I suppose we could support memory-mapped I/O but it has not > been attempted yet. It was really just meant as an arbitrary example of something where you assume "address + 1" == "pointer(address) + 1". Alex
Hi Alex, On 15 September 2018 at 10:16, Alexander Graf <agraf@suse.de> wrote: > > > On 14.09.18 17:46, Simon Glass wrote: >> Hi Alex, >> >> On 26 August 2018 at 19:11, Alexander Graf <agraf@suse.de> wrote: >>> >>> >>> On 08.08.18 11:54, 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@chromium.org> >>> >>> I think you are aware that this logic will fall apart spectacularly if >>> any arithmetic operation happens on the virtual (U-Boot) address, right? >>> So simple code like >>> >>> readl(vaddr + 1); >>> >>> will just fail (hopefully) or (more likely) return a completely >>> incorrect value. >>> >>> I assume this is intentional, but shouldn't the tag increment be >>> something slightly larger then? >> >> What do you expect readl() to do on sandbox? At present it is just a >> no-op. I suppose we could support memory-mapped I/O but it has not >> been attempted yet. > > It was really just meant as an arbitrary example of something where you > assume "address + 1" == "pointer(address) + 1". So long as the addresses are within the range sandbox supports (normally 0 to 128MB unless you increase RAM size) then there is no problem adding 1 to either an address (or a pointer derived from it with map_sysmem()). If you use an address outside that range, it is actually a tag, not an address. Trying to use an out-of-range address is invalid anyway, so it probably doesn't matter. Regards, Simon
diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c index cde0b055a67..e523223ebf8 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; +} + +/** + * 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 this buffer, which can be used to access the + * 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 cc50819ab93..04a11fed559 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 7ed4b512d2e..a612ce89447 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*/
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@chromium.org> --- Changes in v9: - Fix 'thi' typo 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(-)