| Message ID | 1474667259-27290-2-git-send-email-daniel.kiper@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 23.09.16 at 23:47, <daniel.kiper@oracle.com> wrote: > It seems that from xen ELF image POV _end symbol properly determine > image end. However, taking into account that initial Xen image mapping > covers 16 MiB it looks that it is not sufficient. Currently bootloader > potentially may load xen ELF image at 1 MiB and let's say put Linux > kernel or initrd at 8 MiB. Nothing forbids it. This means that initially > Xen image mapping may cover not only Xen image but also loaded modules. Okay, up to here you just describe the state of things, which is fine. > So, let's move end of xen ELF image to 16 MiB. This way we avoid above > mentioned issue because bootloader will be forced to allocate 15 MiB > memory region for image. Then it (bootloader) would not be able to put > in this place anything else because from its POV simply this memory > region will be allocated and used. But here you state what the patch does, but not what problem you solve. And my problem here is that I don't see any problem to be solved: There may be an overlap of address ranges, but I don't see that memory being used in two different way. In fact the 16Mb mapping is just a mapping, without us using anything outside the Xen image range afaics. Please clarify. Jan
On Mon, Sep 26, 2016 at 04:39:37AM -0600, Jan Beulich wrote: > >>> On 23.09.16 at 23:47, <daniel.kiper@oracle.com> wrote: > > It seems that from xen ELF image POV _end symbol properly determine > > image end. However, taking into account that initial Xen image mapping > > covers 16 MiB it looks that it is not sufficient. Currently bootloader > > potentially may load xen ELF image at 1 MiB and let's say put Linux > > kernel or initrd at 8 MiB. Nothing forbids it. This means that initially > > Xen image mapping may cover not only Xen image but also loaded modules. > > Okay, up to here you just describe the state of things, which is fine. > > > So, let's move end of xen ELF image to 16 MiB. This way we avoid above > > mentioned issue because bootloader will be forced to allocate 15 MiB > > memory region for image. Then it (bootloader) would not be able to put > > in this place anything else because from its POV simply this memory > > region will be allocated and used. > > But here you state what the patch does, but not what problem you > solve. And my problem here is that I don't see any problem to be > solved: There may be an overlap of address ranges, but I don't see > that memory being used in two different way. In fact the 16Mb > mapping is just a mapping, without us using anything outside the > Xen image range afaics. However, this begs the question: Why do we map 16 MiB in Xen image address space if we do not expect anything sensible beyond the _end symbol? Should not we stop just beyond the _end at 2 MiB boundary? This way if something accesses anything between _end and 16 MiB via Xen image mapping then way of failure is more predictable than now. IIRC, this region (from _end to 16 MiB) is not even zeroed, so, unexpected things may happen. Well, this is not grave error but I think that it is worth discussing. Daniel
>>> On 26.09.16 at 13:34, <daniel.kiper@oracle.com> wrote: > On Mon, Sep 26, 2016 at 04:39:37AM -0600, Jan Beulich wrote: >> >>> On 23.09.16 at 23:47, <daniel.kiper@oracle.com> wrote: >> > It seems that from xen ELF image POV _end symbol properly determine >> > image end. However, taking into account that initial Xen image mapping >> > covers 16 MiB it looks that it is not sufficient. Currently bootloader >> > potentially may load xen ELF image at 1 MiB and let's say put Linux >> > kernel or initrd at 8 MiB. Nothing forbids it. This means that initially >> > Xen image mapping may cover not only Xen image but also loaded modules. >> >> Okay, up to here you just describe the state of things, which is fine. >> >> > So, let's move end of xen ELF image to 16 MiB. This way we avoid above >> > mentioned issue because bootloader will be forced to allocate 15 MiB >> > memory region for image. Then it (bootloader) would not be able to put >> > in this place anything else because from its POV simply this memory >> > region will be allocated and used. >> >> But here you state what the patch does, but not what problem you >> solve. And my problem here is that I don't see any problem to be >> solved: There may be an overlap of address ranges, but I don't see >> that memory being used in two different way. In fact the 16Mb >> mapping is just a mapping, without us using anything outside the >> Xen image range afaics. > > However, this begs the question: Why do we map 16 MiB in Xen image address > space if we do not expect anything sensible beyond the _end symbol? Because it allows the page tables to be pre-populated at build time? > Should > not we stop just beyond the _end at 2 MiB boundary? This way if something > accesses anything between _end and 16 MiB via Xen image mapping then way of > failure is more predictable than now. But that's not necessarily better - we might end up with an early boot crash with a black screen. > IIRC, this region (from _end to 16 MiB) > is not even zeroed, so, unexpected things may happen. As long as it doesn't get accessed I don't see what unpredictable things you expect to happen. Jan
On Mon, Sep 26, 2016 at 06:32:01AM -0600, Jan Beulich wrote: > >>> On 26.09.16 at 13:34, <daniel.kiper@oracle.com> wrote: > > On Mon, Sep 26, 2016 at 04:39:37AM -0600, Jan Beulich wrote: > >> >>> On 23.09.16 at 23:47, <daniel.kiper@oracle.com> wrote: > >> > It seems that from xen ELF image POV _end symbol properly determine > >> > image end. However, taking into account that initial Xen image mapping > >> > covers 16 MiB it looks that it is not sufficient. Currently bootloader > >> > potentially may load xen ELF image at 1 MiB and let's say put Linux > >> > kernel or initrd at 8 MiB. Nothing forbids it. This means that initially > >> > Xen image mapping may cover not only Xen image but also loaded modules. > >> > >> Okay, up to here you just describe the state of things, which is fine. > >> > >> > So, let's move end of xen ELF image to 16 MiB. This way we avoid above > >> > mentioned issue because bootloader will be forced to allocate 15 MiB > >> > memory region for image. Then it (bootloader) would not be able to put > >> > in this place anything else because from its POV simply this memory > >> > region will be allocated and used. > >> > >> But here you state what the patch does, but not what problem you > >> solve. And my problem here is that I don't see any problem to be > >> solved: There may be an overlap of address ranges, but I don't see > >> that memory being used in two different way. In fact the 16Mb > >> mapping is just a mapping, without us using anything outside the > >> Xen image range afaics. > > > > However, this begs the question: Why do we map 16 MiB in Xen image address > > space if we do not expect anything sensible beyond the _end symbol? > > Because it allows the page tables to be pre-populated at build time? I do not think this is full explanation. Though I can agree that 16 MiB mapping is much easier to create during build but others probably could be created in that or quite similar way too. > > Should > > not we stop just beyond the _end at 2 MiB boundary? This way if something > > accesses anything between _end and 16 MiB via Xen image mapping then way of > > failure is more predictable than now. > > But that's not necessarily better - we might end up with an early > boot crash with a black screen. It depends when failure happens. > > IIRC, this region (from _end to 16 MiB) > > is not even zeroed, so, unexpected things may happen. > > As long as it doesn't get accessed I don't see what unpredictable > things you expect to happen. Well, I do not say about deliberate access but accesses due to bugs. Anyway, I have a feeling that you do not care about problems described by me or even you do not think that they exist. I do not agree with you (well, I agree this is not grave error) but you are maintainer, so, I will drop this patch. Daniel
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index d903c31..9adb2f3 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -266,14 +266,16 @@ SECTIONS .reloc : { *(.reloc) } :text - /* Trick the linker into setting the image size to exactly 16Mb. */ . = ALIGN(__section_alignment__); +#else + efi = .; +#endif + + /* Trick the linker into setting the image size to exactly 16Mb. */ .pad : { . = ALIGN(MB(16)); + __end_of_image__ = .; } :text -#else - efi = .; -#endif /* Sections to be discarded */ /DISCARD/ : {
It seems that from xen ELF image POV _end symbol properly determine image end. However, taking into account that initial Xen image mapping covers 16 MiB it looks that it is not sufficient. Currently bootloader potentially may load xen ELF image at 1 MiB and let's say put Linux kernel or initrd at 8 MiB. Nothing forbids it. This means that initially Xen image mapping may cover not only Xen image but also loaded modules. So, let's move end of xen ELF image to 16 MiB. This way we avoid above mentioned issue because bootloader will be forced to allocate 15 MiB memory region for image. Then it (bootloader) would not be able to put in this place anything else because from its POV simply this memory region will be allocated and used. This patch does not change xen ELF file size. It changes memory size which should be allocated for the image during load. Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> --- xen/arch/x86/xen.lds.S | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)