| Message ID | 20180618140835.195901-18-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Alexander Graf |
| Headers | show |
| Series | efi: Enable sandbox support for EFI loader | expand |
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@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*/
Hi Alex, On 18 June 2018 at 08:50, Alexander Graf <agraf@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@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
On 06/21/2018 04:01 AM, Simon Glass wrote: > Hi Alex, > > On 18 June 2018 at 08:50, Alexander Graf <agraf@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@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. Alex
On 06/21/2018 04:01 AM, Simon Glass wrote: > Hi Alex, > > On 18 June 2018 at 08:50, Alexander Graf <agraf@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@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. Alex
Hi Alex, On 21 June 2018 at 04:10, Alexander Graf <agraf@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@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@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
Hi Alex, On 21 June 2018 at 04:00, Alexander Graf <agraf@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@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@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? Regards, Simon
On 06/21/2018 09:45 PM, Simon Glass wrote: > Hi Alex, > > On 21 June 2018 at 04:00, Alexander Graf <agraf@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@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@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
Hi Alex, On 22 June 2018 at 06:12, Alexander Graf <agraf@suse.de> wrote: > On 06/21/2018 09:45 PM, Simon Glass wrote: >> >> Hi Alex, >> >> On 21 June 2018 at 04:00, Alexander Graf <agraf@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@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@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. OK, well perhaps something to think about in the future. > > 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 ;) To some extent, yes. But if we want to prevent 'foreign' pointers coming into sandbox, we would need to handle bss, stack and the rodata/data. I'm sure it could work, but it seems like it would make sandbox into a rather strange program. I had trouble even with the minor linker-script changes I have already done, on some architectures. Regards, Simon
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; +} + +/** + * 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 + * 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*/
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 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(-)