| Message ID | 20180618140835.195901-13-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: > Use a starting address of 256MB which should be available. This helps to > make sandbox RAM buffers pointers more recognisable. > > Signed-off-by: Simon Glass <sjg@chromium.org> Nak, this has a non-0 chance of failing, in case something else is already mapped at that address. You don't want to have your CI blow up 1% of the time due to this. Alex > --- > > 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/os.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c > index a1a982af2d..1553aa687d 100644 > --- a/arch/sandbox/cpu/os.c > +++ b/arch/sandbox/cpu/os.c > @@ -145,7 +145,12 @@ void *os_malloc(size_t length) > struct os_mem_hdr *hdr; > int page_size = getpagesize(); > > - hdr = mmap(NULL, length + page_size, > + /* > + * Use an address that is hopefully available to us so that pointers > + * to this memory are fairly obvious. If we end up with a different > + * address, that's fine too. > + */ > + hdr = mmap((void *)0x10000000, length + page_size, > PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); > if (hdr == MAP_FAILED) > return NULL;
Hi Alex, On 18 June 2018 at 08:45, Alexander Graf <agraf@suse.de> wrote: > On 06/18/2018 04:08 PM, Simon Glass wrote: >> >> Use a starting address of 256MB which should be available. This helps to >> make sandbox RAM buffers pointers more recognisable. >> >> Signed-off-by: Simon Glass <sjg@chromium.org> > > > Nak, this has a non-0 chance of failing, in case something else is already > mapped at that address. You don't want to have your CI blow up 1% of the > time due to this. It's just a hint though. Everything will still work if it doesn't get this exact address. Regards, Simon
On 06/20/2018 12:02 AM, Simon Glass wrote: > Hi Alex, > > On 18 June 2018 at 08:45, Alexander Graf <agraf@suse.de> wrote: >> On 06/18/2018 04:08 PM, Simon Glass wrote: >>> Use a starting address of 256MB which should be available. This helps to >>> make sandbox RAM buffers pointers more recognisable. >>> >>> Signed-off-by: Simon Glass <sjg@chromium.org> >> >> Nak, this has a non-0 chance of failing, in case something else is already >> mapped at that address. You don't want to have your CI blow up 1% of the >> time due to this. > It's just a hint though. Everything will still work if it doesn't get > this exact address. I don't see what it buys us then. Alex
Hi Alex, On 20 June 2018 at 02:51, Alexander Graf <agraf@suse.de> wrote: > On 06/20/2018 12:02 AM, Simon Glass wrote: >> >> Hi Alex, >> >> On 18 June 2018 at 08:45, Alexander Graf <agraf@suse.de> wrote: >>> >>> On 06/18/2018 04:08 PM, Simon Glass wrote: >>>> >>>> Use a starting address of 256MB which should be available. This helps to >>>> make sandbox RAM buffers pointers more recognisable. >>>> >>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>> >>> >>> Nak, this has a non-0 chance of failing, in case something else is >>> already >>> mapped at that address. You don't want to have your CI blow up 1% of the >>> time due to this. >> >> It's just a hint though. Everything will still work if it doesn't get >> this exact address. > > > I don't see what it buys us then. These are my thoughts: 1. We get an address before 4GB which is needed for grub (so far as I can tell) 2. It will normally be a nice, easy-on-the-eyes address 3. If it isn't, it will hopefully still be an address below 4GB 4. Even if not (e.g. on some very strange system) it will still allow sandbox to start up But anyway, as I said, this will never fail, it is just a hint. So I think your original comment is incorrect. Regards, Simon
On 06/21/2018 04:02 AM, Simon Glass wrote: > Hi Alex, > > On 20 June 2018 at 02:51, Alexander Graf <agraf@suse.de> wrote: >> On 06/20/2018 12:02 AM, Simon Glass wrote: >>> Hi Alex, >>> >>> On 18 June 2018 at 08:45, Alexander Graf <agraf@suse.de> wrote: >>>> On 06/18/2018 04:08 PM, Simon Glass wrote: >>>>> Use a starting address of 256MB which should be available. This helps to >>>>> make sandbox RAM buffers pointers more recognisable. >>>>> >>>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>> >>>> Nak, this has a non-0 chance of failing, in case something else is >>>> already >>>> mapped at that address. You don't want to have your CI blow up 1% of the >>>> time due to this. >>> It's just a hint though. Everything will still work if it doesn't get >>> this exact address. >> >> I don't see what it buys us then. > These are my thoughts: > > 1. We get an address before 4GB which is needed for grub (so far as I can tell) We only need that in the memory map which you want virtual (U-Boot address space) anyway. So there's no need to also have the Linux address be <4GB. > 2. It will normally be a nice, easy-on-the-eyes address I'd rather say it's going to confuse people, because now it's easy to mistake for a U-Boot address. > 3. If it isn't, it will hopefully still be an address below 4GB > 4. Even if not (e.g. on some very strange system) it will still allow > sandbox to start up > > But anyway, as I said, this will never fail, it is just a hint. So I > think your original comment is incorrect. It will not fail, it will just potentially not map at exactly the spot you wanted the map at, which IMHO would cause even more confusion. Imagine people now start to assume that %p output is stable between invocations - because it is 99% of the cases. They start to integrate that into their CI suites and suddenly things fail 1% of the time because the linker figured today's a great day to map glibc at that address. Alex
Hi Alex, On 21 June 2018 at 03:58, Alexander Graf <agraf@suse.de> wrote: > On 06/21/2018 04:02 AM, Simon Glass wrote: >> >> Hi Alex, >> >> On 20 June 2018 at 02:51, Alexander Graf <agraf@suse.de> wrote: >>> >>> On 06/20/2018 12:02 AM, Simon Glass wrote: >>>> >>>> Hi Alex, >>>> >>>> On 18 June 2018 at 08:45, Alexander Graf <agraf@suse.de> wrote: >>>>> >>>>> On 06/18/2018 04:08 PM, Simon Glass wrote: >>>>>> >>>>>> Use a starting address of 256MB which should be available. This helps >>>>>> to >>>>>> make sandbox RAM buffers pointers more recognisable. >>>>>> >>>>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>>> >>>>> >>>>> Nak, this has a non-0 chance of failing, in case something else is >>>>> already >>>>> mapped at that address. You don't want to have your CI blow up 1% of >>>>> the >>>>> time due to this. >>>> >>>> It's just a hint though. Everything will still work if it doesn't get >>>> this exact address. >>> >>> >>> I don't see what it buys us then. >> >> These are my thoughts: >> >> 1. We get an address before 4GB which is needed for grub (so far as I can >> tell) > > > We only need that in the memory map which you want virtual (U-Boot address > space) anyway. So there's no need to also have the Linux address be <4GB. Grub cannot work without <4GB, right? I don't mind either way, but I think it that if we are picking an address, picking a smaller one is better. > >> 2. It will normally be a nice, easy-on-the-eyes address > > > I'd rather say it's going to confuse people, because now it's easy to > mistake for a U-Boot address. That's possibly true, although at present it is outside the range of valid U-Boot addresses. What do you suggest? > >> 3. If it isn't, it will hopefully still be an address below 4GB >> 4. Even if not (e.g. on some very strange system) it will still allow >> sandbox to start up >> >> But anyway, as I said, this will never fail, it is just a hint. So I >> think your original comment is incorrect. > > > It will not fail, it will just potentially not map at exactly the spot you > wanted the map at, which IMHO would cause even more confusion. > > Imagine people now start to assume that %p output is stable between > invocations - because it is 99% of the cases. They start to integrate that > into their CI suites and suddenly things fail 1% of the time because the > linker figured today's a great day to map glibc at that address. Remember these are internal sandbox addresses. They should not be visible to most of the code and it is very hard for me to imagine a test that could fail because the address is mapped in different places. How would you access the address from a test? Are you thinking of something that would assume the ram buffer address is something in particular and then call map_to_sysmem() on the pointer? I am really not sure that we should worry about that too much. Otherwise I just cannot imagine what sort of test you are referring to here? Regards, Simon
On 06/21/2018 09:45 PM, Simon Glass wrote: > Hi Alex, > > On 21 June 2018 at 03:58, Alexander Graf <agraf@suse.de> wrote: >> On 06/21/2018 04:02 AM, Simon Glass wrote: >>> Hi Alex, >>> >>> On 20 June 2018 at 02:51, Alexander Graf <agraf@suse.de> wrote: >>>> On 06/20/2018 12:02 AM, Simon Glass wrote: >>>>> Hi Alex, >>>>> >>>>> On 18 June 2018 at 08:45, Alexander Graf <agraf@suse.de> wrote: >>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote: >>>>>>> Use a starting address of 256MB which should be available. This helps >>>>>>> to >>>>>>> make sandbox RAM buffers pointers more recognisable. >>>>>>> >>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>>>> >>>>>> Nak, this has a non-0 chance of failing, in case something else is >>>>>> already >>>>>> mapped at that address. You don't want to have your CI blow up 1% of >>>>>> the >>>>>> time due to this. >>>>> It's just a hint though. Everything will still work if it doesn't get >>>>> this exact address. >>>> >>>> I don't see what it buys us then. >>> These are my thoughts: >>> >>> 1. We get an address before 4GB which is needed for grub (so far as I can >>> tell) >> >> We only need that in the memory map which you want virtual (U-Boot address >> space) anyway. So there's no need to also have the Linux address be <4GB. > Grub cannot work without <4GB, right? I don't mind either way, but I > think it that if we are picking an address, picking a smaller one is > better. Only if you expose host pointers as memory addresses. We don't anymore, and grub doesn't care whether a pointer is >4G. > >>> 2. It will normally be a nice, easy-on-the-eyes address >> >> I'd rather say it's going to confuse people, because now it's easy to >> mistake for a U-Boot address. > That's possibly true, although at present it is outside the range of > valid U-Boot addresses. What do you suggest? I see little reason for change. Also if you want to change it, it's out of scope of the efi_loader enablement ;). > >>> 3. If it isn't, it will hopefully still be an address below 4GB >>> 4. Even if not (e.g. on some very strange system) it will still allow >>> sandbox to start up >>> >>> But anyway, as I said, this will never fail, it is just a hint. So I >>> think your original comment is incorrect. >> >> It will not fail, it will just potentially not map at exactly the spot you >> wanted the map at, which IMHO would cause even more confusion. >> >> Imagine people now start to assume that %p output is stable between >> invocations - because it is 99% of the cases. They start to integrate that >> into their CI suites and suddenly things fail 1% of the time because the >> linker figured today's a great day to map glibc at that address. > Remember these are internal sandbox addresses. They should not be > visible to most of the code and it is very hard for me to imagine a > test that could fail because the address is mapped in different > places. How would you access the address from a test? Are you thinking > of something that would assume the ram buffer address is something in > particular and then call map_to_sysmem() on the pointer? I am really > not sure that we should worry about that too much. No, I'm mostly worried about people assuming that 2 invocations of a U-Boot binary give them the same console output, even when there are printf()s that contain a %p. Alex
Hi Alex, On 22 June 2018 at 06:10, Alexander Graf <agraf@suse.de> wrote: > On 06/21/2018 09:45 PM, Simon Glass wrote: >> >> Hi Alex, >> >> On 21 June 2018 at 03:58, Alexander Graf <agraf@suse.de> wrote: >>> >>> On 06/21/2018 04:02 AM, Simon Glass wrote: >>>> >>>> Hi Alex, >>>> >>>> On 20 June 2018 at 02:51, Alexander Graf <agraf@suse.de> wrote: >>>>> >>>>> On 06/20/2018 12:02 AM, Simon Glass wrote: >>>>>> >>>>>> Hi Alex, >>>>>> >>>>>> On 18 June 2018 at 08:45, Alexander Graf <agraf@suse.de> wrote: >>>>>>> >>>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote: >>>>>>>> >>>>>>>> Use a starting address of 256MB which should be available. This >>>>>>>> helps >>>>>>>> to >>>>>>>> make sandbox RAM buffers pointers more recognisable. >>>>>>>> >>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>>>>> >>>>>>> >>>>>>> Nak, this has a non-0 chance of failing, in case something else is >>>>>>> already >>>>>>> mapped at that address. You don't want to have your CI blow up 1% of >>>>>>> the >>>>>>> time due to this. >>>>>> >>>>>> It's just a hint though. Everything will still work if it doesn't get >>>>>> this exact address. >>>>> >>>>> >>>>> I don't see what it buys us then. >>>> >>>> These are my thoughts: >>>> >>>> 1. We get an address before 4GB which is needed for grub (so far as I >>>> can >>>> tell) >>> >>> >>> We only need that in the memory map which you want virtual (U-Boot >>> address >>> space) anyway. So there's no need to also have the Linux address be <4GB. >> >> Grub cannot work without <4GB, right? I don't mind either way, but I >> think it that if we are picking an address, picking a smaller one is >> better. > > > Only if you expose host pointers as memory addresses. We don't anymore, and > grub doesn't care whether a pointer is >4G. We have to provide grub with pointers to memory with sandbox. These will be pointers into the sandbox RAM buffer. If this buffer is >=4GB then grub will not work, from my testing. > >> >>>> 2. It will normally be a nice, easy-on-the-eyes address >>> >>> >>> I'd rather say it's going to confuse people, because now it's easy to >>> mistake for a U-Boot address. >> >> That's possibly true, although at present it is outside the range of >> valid U-Boot addresses. What do you suggest? > > > I see little reason for change. Also if you want to change it, it's out of > scope of the efi_loader enablement ;). Yes it can be dealt with separately. > >> >>>> 3. If it isn't, it will hopefully still be an address below 4GB >>>> 4. Even if not (e.g. on some very strange system) it will still allow >>>> sandbox to start up >>>> >>>> But anyway, as I said, this will never fail, it is just a hint. So I >>>> think your original comment is incorrect. >>> >>> >>> It will not fail, it will just potentially not map at exactly the spot >>> you >>> wanted the map at, which IMHO would cause even more confusion. >>> >>> Imagine people now start to assume that %p output is stable between >>> invocations - because it is 99% of the cases. They start to integrate >>> that >>> into their CI suites and suddenly things fail 1% of the time because the >>> linker figured today's a great day to map glibc at that address. >> >> Remember these are internal sandbox addresses. They should not be >> visible to most of the code and it is very hard for me to imagine a >> test that could fail because the address is mapped in different >> places. How would you access the address from a test? Are you thinking >> of something that would assume the ram buffer address is something in >> particular and then call map_to_sysmem() on the pointer? I am really >> not sure that we should worry about that too much. > > > No, I'm mostly worried about people assuming that 2 invocations of a U-Boot > binary give them the same console output, even when there are printf()s that > contain a %p. We should NEVER printf a %p in code that might be used with sandbox. That is another point I should have made. U-Boot (including sandbox) uses addresses and we should print addresses, not pointers. Regards, Simon
> Am 22.06.2018 um 21:28 schrieb Simon Glass <sjg@chromium.org>: > > Hi Alex, > >> On 22 June 2018 at 06:10, Alexander Graf <agraf@suse.de> wrote: >>> On 06/21/2018 09:45 PM, Simon Glass wrote: >>> >>> Hi Alex, >>> >>>> On 21 June 2018 at 03:58, Alexander Graf <agraf@suse.de> wrote: >>>> >>>>> On 06/21/2018 04:02 AM, Simon Glass wrote: >>>>> >>>>> Hi Alex, >>>>> >>>>>> On 20 June 2018 at 02:51, Alexander Graf <agraf@suse.de> wrote: >>>>>> >>>>>>> On 06/20/2018 12:02 AM, Simon Glass wrote: >>>>>>> >>>>>>> Hi Alex, >>>>>>> >>>>>>>> On 18 June 2018 at 08:45, Alexander Graf <agraf@suse.de> wrote: >>>>>>>> >>>>>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote: >>>>>>>>> >>>>>>>>> Use a starting address of 256MB which should be available. This >>>>>>>>> helps >>>>>>>>> to >>>>>>>>> make sandbox RAM buffers pointers more recognisable. >>>>>>>>> >>>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>>>>>> >>>>>>>> >>>>>>>> Nak, this has a non-0 chance of failing, in case something else is >>>>>>>> already >>>>>>>> mapped at that address. You don't want to have your CI blow up 1% of >>>>>>>> the >>>>>>>> time due to this. >>>>>>> >>>>>>> It's just a hint though. Everything will still work if it doesn't get >>>>>>> this exact address. >>>>>> >>>>>> >>>>>> I don't see what it buys us then. >>>>> >>>>> These are my thoughts: >>>>> >>>>> 1. We get an address before 4GB which is needed for grub (so far as I >>>>> can >>>>> tell) >>>> >>>> >>>> We only need that in the memory map which you want virtual (U-Boot >>>> address >>>> space) anyway. So there's no need to also have the Linux address be <4GB. >>> >>> Grub cannot work without <4GB, right? I don't mind either way, but I >>> think it that if we are picking an address, picking a smaller one is >>> better. >> >> >> Only if you expose host pointers as memory addresses. We don't anymore, and >> grub doesn't care whether a pointer is >4G. > > We have to provide grub with pointers to memory with sandbox. These > will be pointers into the sandbox RAM buffer. If this buffer is >=4GB > then grub will not work, from my testing. It works just fine for me? Alex
Hi Alex, On 22 June 2018 at 15:29, Alexander Graf <agraf@suse.de> wrote: > > >> Am 22.06.2018 um 21:28 schrieb Simon Glass <sjg@chromium.org>: >> >> Hi Alex, >> >>> On 22 June 2018 at 06:10, Alexander Graf <agraf@suse.de> wrote: >>>> On 06/21/2018 09:45 PM, Simon Glass wrote: >>>> >>>> Hi Alex, >>>> >>>>> On 21 June 2018 at 03:58, Alexander Graf <agraf@suse.de> wrote: >>>>> >>>>>> On 06/21/2018 04:02 AM, Simon Glass wrote: >>>>>> >>>>>> Hi Alex, >>>>>> >>>>>>> On 20 June 2018 at 02:51, Alexander Graf <agraf@suse.de> wrote: >>>>>>> >>>>>>>> On 06/20/2018 12:02 AM, Simon Glass wrote: >>>>>>>> >>>>>>>> Hi Alex, >>>>>>>> >>>>>>>>> On 18 June 2018 at 08:45, Alexander Graf <agraf@suse.de> wrote: >>>>>>>>> >>>>>>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote: >>>>>>>>>> >>>>>>>>>> Use a starting address of 256MB which should be available. This >>>>>>>>>> helps >>>>>>>>>> to >>>>>>>>>> make sandbox RAM buffers pointers more recognisable. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>>>>>>> >>>>>>>>> >>>>>>>>> Nak, this has a non-0 chance of failing, in case something else is >>>>>>>>> already >>>>>>>>> mapped at that address. You don't want to have your CI blow up 1% of >>>>>>>>> the >>>>>>>>> time due to this. >>>>>>>> >>>>>>>> It's just a hint though. Everything will still work if it doesn't get >>>>>>>> this exact address. >>>>>>> >>>>>>> >>>>>>> I don't see what it buys us then. >>>>>> >>>>>> These are my thoughts: >>>>>> >>>>>> 1. We get an address before 4GB which is needed for grub (so far as I >>>>>> can >>>>>> tell) >>>>> >>>>> >>>>> We only need that in the memory map which you want virtual (U-Boot >>>>> address >>>>> space) anyway. So there's no need to also have the Linux address be <4GB. >>>> >>>> Grub cannot work without <4GB, right? I don't mind either way, but I >>>> think it that if we are picking an address, picking a smaller one is >>>> better. >>> >>> >>> Only if you expose host pointers as memory addresses. We don't anymore, and >>> grub doesn't care whether a pointer is >4G. >> >> We have to provide grub with pointers to memory with sandbox. These >> will be pointers into the sandbox RAM buffer. If this buffer is >=4GB >> then grub will not work, from my testing. > > It works just fine for me? OK that's strange, but perhaps I am building grub wrong. I'l ltry it again when things land. Regards, Simon
diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c index a1a982af2d..1553aa687d 100644 --- a/arch/sandbox/cpu/os.c +++ b/arch/sandbox/cpu/os.c @@ -145,7 +145,12 @@ void *os_malloc(size_t length) struct os_mem_hdr *hdr; int page_size = getpagesize(); - hdr = mmap(NULL, length + page_size, + /* + * Use an address that is hopefully available to us so that pointers + * to this memory are fairly obvious. If we end up with a different + * address, that's fine too. + */ + hdr = mmap((void *)0x10000000, length + page_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); if (hdr == MAP_FAILED) return NULL;
Use a starting address of 256MB which should be available. This helps to make sandbox RAM buffers pointers more recognisable. 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/os.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)