* [PATCH v7 01/14] x86: move xen ELF end of image to 16 MiB
2016-09-23 21:47 [PATCH v7 00/14] x86: multiboot2 protocol support Daniel Kiper
@ 2016-09-23 21:47 ` Daniel Kiper
2016-09-26 10:39 ` Jan Beulich
2016-09-23 21:47 ` [PATCH v7 02/14] x86: properly calculate xen ELF end of image address Daniel Kiper
` (12 subsequent siblings)
13 siblings, 1 reply; 51+ messages in thread
From: Daniel Kiper @ 2016-09-23 21:47 UTC (permalink / raw)
To: xen-devel
Cc: jgross, sstabellini, andrew.cooper3, cardoe, pgnet.dev, ning.sun,
julien.grall, david.vrabel, jbeulich, qiaowei.ren, gang.wei,
fu.wei
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(-)
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/ : {
--
1.7.10.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v7 01/14] x86: move xen ELF end of image to 16 MiB
2016-09-23 21:47 ` [PATCH v7 01/14] x86: move xen ELF end of image to 16 MiB Daniel Kiper
@ 2016-09-26 10:39 ` Jan Beulich
2016-09-26 11:34 ` Daniel Kiper
0 siblings, 1 reply; 51+ messages in thread
From: Jan Beulich @ 2016-09-26 10:39 UTC (permalink / raw)
To: Daniel Kiper
Cc: Juergen Gross, sstabellini, andrew.cooper3, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
>>> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v7 01/14] x86: move xen ELF end of image to 16 MiB
2016-09-26 10:39 ` Jan Beulich
@ 2016-09-26 11:34 ` Daniel Kiper
2016-09-26 12:32 ` Jan Beulich
0 siblings, 1 reply; 51+ messages in thread
From: Daniel Kiper @ 2016-09-26 11:34 UTC (permalink / raw)
To: Jan Beulich
Cc: Juergen Gross, sstabellini, andrew.cooper3, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v7 01/14] x86: move xen ELF end of image to 16 MiB
2016-09-26 11:34 ` Daniel Kiper
@ 2016-09-26 12:32 ` Jan Beulich
2016-09-27 17:42 ` Daniel Kiper
0 siblings, 1 reply; 51+ messages in thread
From: Jan Beulich @ 2016-09-26 12:32 UTC (permalink / raw)
To: Daniel Kiper
Cc: Juergen Gross, sstabellini, andrew.cooper3, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
>>> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v7 01/14] x86: move xen ELF end of image to 16 MiB
2016-09-26 12:32 ` Jan Beulich
@ 2016-09-27 17:42 ` Daniel Kiper
0 siblings, 0 replies; 51+ messages in thread
From: Daniel Kiper @ 2016-09-27 17:42 UTC (permalink / raw)
To: Jan Beulich
Cc: Juergen Gross, sstabellini, andrew.cooper3, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v7 02/14] x86: properly calculate xen ELF end of image address
2016-09-23 21:47 [PATCH v7 00/14] x86: multiboot2 protocol support Daniel Kiper
2016-09-23 21:47 ` [PATCH v7 01/14] x86: move xen ELF end of image to 16 MiB Daniel Kiper
@ 2016-09-23 21:47 ` Daniel Kiper
2016-09-26 10:45 ` Jan Beulich
2016-09-23 21:47 ` [PATCH v7 03/14] x86: add multiboot2 protocol support Daniel Kiper
` (11 subsequent siblings)
13 siblings, 1 reply; 51+ messages in thread
From: Daniel Kiper @ 2016-09-23 21:47 UTC (permalink / raw)
To: xen-devel
Cc: jgross, sstabellini, andrew.cooper3, cardoe, pgnet.dev, ning.sun,
julien.grall, david.vrabel, jbeulich, qiaowei.ren, gang.wei,
fu.wei
Currently xen ELF end of image address is calculated using first line from
"nm -nr xen/xen-syms" output. However, sometimes it may contain random
symbol address not related to the end of image in any way. It can happen
if a symbol is introduced with address larger than __end_of_image__ symbol
address. Such situation encountered when I linked xen ELF binary with
xen/arch/x86/efi/relocs-dummy.S. Then first line from "nm -nr xen/xen-syms"
contained "ffff82d0c0000000 A ALT_START" and xen ELF image memory size
was silently set to 1023 MiB. This issue happened because there is no
check which symbol address is used to calculate end of image address. So,
let's fix it and take ELF end of image address by reading __end_of_image__
symbol address from nm output. This way xen ELF image build process is
not prone to changes in order of nm output.
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
v7 - suggestions/fixes:
- use sed instead of awk
(suggested by Jan Beulich),
- improve commit message
(suggested by Jan Beulich).
---
xen/arch/x86/Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index d3875c5..c3a8920 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -91,7 +91,7 @@ endif
$(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET) 0x100000 \
- `$(NM) -nr $(TARGET)-syms | head -n 1 | sed -e 's/^\([^ ]*\).*/0x\1/'`
+ `$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . __end_of_image__$$/0x\1/p'`
.PHONY: tests
tests:
--
1.7.10.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v7 02/14] x86: properly calculate xen ELF end of image address
2016-09-23 21:47 ` [PATCH v7 02/14] x86: properly calculate xen ELF end of image address Daniel Kiper
@ 2016-09-26 10:45 ` Jan Beulich
2016-09-26 12:06 ` Daniel Kiper
0 siblings, 1 reply; 51+ messages in thread
From: Jan Beulich @ 2016-09-26 10:45 UTC (permalink / raw)
To: Daniel Kiper
Cc: Juergen Gross, sstabellini, andrew.cooper3, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
>>> On 23.09.16 at 23:47, <daniel.kiper@oracle.com> wrote:
> Currently xen ELF end of image address is calculated using first line from
> "nm -nr xen/xen-syms" output. However, sometimes it may contain random
> symbol address not related to the end of image in any way.
There's nothing random here, or at least you fail to demonstrate that
there is (or might be). As said before - please don't describe an issue
you think needs fixing in a tendentious way.
> It can happen
> if a symbol is introduced with address larger than __end_of_image__ symbol
> address. Such situation encountered when I linked xen ELF binary with
> xen/arch/x86/efi/relocs-dummy.S. Then first line from "nm -nr xen/xen-syms"
> contained "ffff82d0c0000000 A ALT_START" and xen ELF image memory size
> was silently set to 1023 MiB.
What you really mean to say here is that this change is a prereq for
a later one.
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -91,7 +91,7 @@ endif
>
> $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
> ./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET) 0x100000 \
> - `$(NM) -nr $(TARGET)-syms | head -n 1 | sed -e 's/^\([^ ]*\).*/0x\1/'`
> + `$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . __end_of_image__$$/0x\1/p'`
I think it was Konrad who did already point out the bogus tab here.
I'm not against you adding some indentation, but then please do so
using spaces, such that the command line argument here aligns with
the first one on the previous line.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v7 02/14] x86: properly calculate xen ELF end of image address
2016-09-26 10:45 ` Jan Beulich
@ 2016-09-26 12:06 ` Daniel Kiper
2016-09-26 12:34 ` Jan Beulich
0 siblings, 1 reply; 51+ messages in thread
From: Daniel Kiper @ 2016-09-26 12:06 UTC (permalink / raw)
To: Jan Beulich
Cc: Juergen Gross, sstabellini, andrew.cooper3, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
On Mon, Sep 26, 2016 at 04:45:25AM -0600, Jan Beulich wrote:
> >>> On 23.09.16 at 23:47, <daniel.kiper@oracle.com> wrote:
> > Currently xen ELF end of image address is calculated using first line from
> > "nm -nr xen/xen-syms" output. However, sometimes it may contain random
> > symbol address not related to the end of image in any way.
>
> There's nothing random here, or at least you fail to demonstrate that
> there is (or might be). As said before - please don't describe an issue
> you think needs fixing in a tendentious way.
OK, maybe I should say that something which is not valid end of image address, e.g.:
"However, sometimes it may contain symbol address not related to the end
of image in any way." Is it OK for you?
> > It can happen
> > if a symbol is introduced with address larger than __end_of_image__ symbol
> > address. Such situation encountered when I linked xen ELF binary with
> > xen/arch/x86/efi/relocs-dummy.S. Then first line from "nm -nr xen/xen-syms"
> > contained "ffff82d0c0000000 A ALT_START" and xen ELF image memory size
> > was silently set to 1023 MiB.
>
> What you really mean to say here is that this change is a prereq for
> a later one.
Does it really matters here? This fix is quite generic, however, if you
wish I can add relevant sentence which says so.
Daniel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v7 02/14] x86: properly calculate xen ELF end of image address
2016-09-26 12:06 ` Daniel Kiper
@ 2016-09-26 12:34 ` Jan Beulich
0 siblings, 0 replies; 51+ messages in thread
From: Jan Beulich @ 2016-09-26 12:34 UTC (permalink / raw)
To: Daniel Kiper
Cc: Juergen Gross, sstabellini, andrew.cooper3, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
>>> On 26.09.16 at 14:06, <daniel.kiper@oracle.com> wrote:
> On Mon, Sep 26, 2016 at 04:45:25AM -0600, Jan Beulich wrote:
>> >>> On 23.09.16 at 23:47, <daniel.kiper@oracle.com> wrote:
>> > Currently xen ELF end of image address is calculated using first line from
>> > "nm -nr xen/xen-syms" output. However, sometimes it may contain random
>> > symbol address not related to the end of image in any way.
>>
>> There's nothing random here, or at least you fail to demonstrate that
>> there is (or might be). As said before - please don't describe an issue
>> you think needs fixing in a tendentious way.
>
> OK, maybe I should say that something which is not valid end of image
> address, e.g.:
> "However, sometimes it may contain symbol address not related to the end
> of image in any way." Is it OK for you?
Not really - "sometimes" still suggests that there is an active problem
even before this patch in some configurations or setups.
>> > It can happen
>> > if a symbol is introduced with address larger than __end_of_image__ symbol
>> > address. Such situation encountered when I linked xen ELF binary with
>> > xen/arch/x86/efi/relocs-dummy.S. Then first line from "nm -nr xen/xen-syms"
>> > contained "ffff82d0c0000000 A ALT_START" and xen ELF image memory size
>> > was silently set to 1023 MiB.
>>
>> What you really mean to say here is that this change is a prereq for
>> a later one.
>
> Does it really matters here? This fix is quite generic, however, if you
> wish I can add relevant sentence which says so.
You should write the description in a way that it makes sense to anyone
reading it even without being aware of the details of later patches of
yours. Please always remember that patch series don't necessarily get
committed in one go - there may be weeks or even months between
this one going in and the other one following.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v7 03/14] x86: add multiboot2 protocol support
2016-09-23 21:47 [PATCH v7 00/14] x86: multiboot2 protocol support Daniel Kiper
2016-09-23 21:47 ` [PATCH v7 01/14] x86: move xen ELF end of image to 16 MiB Daniel Kiper
2016-09-23 21:47 ` [PATCH v7 02/14] x86: properly calculate xen ELF end of image address Daniel Kiper
@ 2016-09-23 21:47 ` Daniel Kiper
2016-09-26 13:18 ` Jan Beulich
2016-09-23 21:47 ` [PATCH v7 04/14] efi: create efi_enabled() Daniel Kiper
` (10 subsequent siblings)
13 siblings, 1 reply; 51+ messages in thread
From: Daniel Kiper @ 2016-09-23 21:47 UTC (permalink / raw)
To: xen-devel
Cc: jgross, sstabellini, andrew.cooper3, cardoe, pgnet.dev, ning.sun,
julien.grall, david.vrabel, jbeulich, qiaowei.ren, gang.wei,
fu.wei
Add multiboot2 protocol support. Alter min memory limit handling as we
now may not find it from either multiboot (v1) or multiboot2.
This way we are laying the foundation for EFI + GRUB2 + Xen development.
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
v7 - suggestions/fixes:
- rename mbi_mbi/mbi2_mbi to mbi_reloc/mbi2_reloc respectively
(suggested by Jan Beulich),
- initialize mbi_out->flags using "|=" instead of "="
(suggested by Jan Beulich),
- use sizeof(*mmap_dst) instead of sizeof(memory_map_t)
if it makes sense
(suggested by Jan Beulich).
v6 - suggestions/fixes:
- properly index multiboot2_tag_mmap_t.entries[]
(suggested by Jan Beulich),
- do not index mbi_out_mods[] beyond its end
(suggested by Andrew Cooper),
- reduce number of casts
(suggested by Andrew Cooper and Jan Beulich),
- add braces to increase code readability
(suggested by Andrew Cooper).
v5 - suggestions/fixes:
- check multiboot2_tag_mmap_t.entry_size before
multiboot2_tag_mmap_t.entries[] use
(suggested by Jan Beulich),
- properly index multiboot2_tag_mmap_t.entries[]
(suggested by Jan Beulich),
- use "type name[]" instad of "type name[0]"
in xen/include/xen/multiboot2.h
(suggested by Jan Beulich),
- remove unneeded comment
(suggested by Jan Beulich).
v4 - suggestions/fixes:
- avoid assembly usage in xen/arch/x86/boot/reloc.c,
- fix boundary check issue and optimize
for() loops in mbi2_mbi(),
- move to stdcall calling convention,
- remove unneeded typeof() from ALIGN_UP() macro
(suggested by Jan Beulich),
- add and use NULL definition in xen/arch/x86/boot/reloc.c
(suggested by Jan Beulich),
- do not read data beyond the end of multiboot2
information in xen/arch/x86/boot/head.S
(suggested by Jan Beulich),
- add :req to some .macro arguments
(suggested by Jan Beulich),
- use cmovcc if possible,
- add .L to multiboot2_header_end label
(suggested by Jan Beulich),
- add .L to multiboot2_proto label
(suggested by Jan Beulich),
- improve label names
(suggested by Jan Beulich).
v3 - suggestions/fixes:
- reorder reloc() arguments
(suggested by Jan Beulich),
- remove .L from multiboot2 header labels
(suggested by Andrew Cooper, Jan Beulich and Konrad Rzeszutek Wilk),
- take into account alignment when skipping multiboot2 fixed part
(suggested by Konrad Rzeszutek Wilk),
- create modules data if modules count != 0
(suggested by Jan Beulich),
- improve macros
(suggested by Jan Beulich),
- reduce number of casts
(suggested by Jan Beulich),
- use const if possible
(suggested by Jan Beulich),
- drop static and __used__ attribute from reloc()
(suggested by Jan Beulich),
- remove isolated/stray __packed attribute from
multiboot2_memory_map_t type definition
(suggested by Jan Beulich),
- reformat xen/include/xen/multiboot2.h
(suggested by Konrad Rzeszutek Wilk),
- improve comments
(suggested by Konrad Rzeszutek Wilk),
- remove hard tabs
(suggested by Jan Beulich and Konrad Rzeszutek Wilk).
v2 - suggestions/fixes:
- generate multiboot2 header using macros
(suggested by Jan Beulich),
- improve comments
(suggested by Jan Beulich),
- simplify assembly in xen/arch/x86/boot/head.S
(suggested by Jan Beulich),
- do not include include/xen/compiler.h
in xen/arch/x86/boot/reloc.c
(suggested by Jan Beulich),
- do not read data beyond the end of multiboot2 information
(suggested by Jan Beulich).
v2 - not fixed yet:
- dynamic dependency generation for xen/arch/x86/boot/reloc.S;
this requires more work; I am not sure that it pays because
potential patch requires more changes than addition of just
multiboot2.h to Makefile
(suggested by Jan Beulich),
- isolated/stray __packed attribute usage for multiboot2_memory_map_t
(suggested by Jan Beulich).
---
xen/arch/x86/boot/Makefile | 3 +-
xen/arch/x86/boot/head.S | 107 ++++++++++++++++++++++-
xen/arch/x86/boot/reloc.c | 148 ++++++++++++++++++++++++++++++--
xen/arch/x86/x86_64/asm-offsets.c | 9 ++
xen/include/xen/multiboot2.h | 169 +++++++++++++++++++++++++++++++++++++
5 files changed, 426 insertions(+), 10 deletions(-)
create mode 100644 xen/include/xen/multiboot2.h
diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
index 5fdb5ae..06893d8 100644
--- a/xen/arch/x86/boot/Makefile
+++ b/xen/arch/x86/boot/Makefile
@@ -1,6 +1,7 @@
obj-bin-y += head.o
-RELOC_DEPS = $(BASEDIR)/include/asm-x86/config.h $(BASEDIR)/include/xen/multiboot.h
+RELOC_DEPS = $(BASEDIR)/include/asm-x86/config.h $(BASEDIR)/include/xen/multiboot.h \
+ $(BASEDIR)/include/xen/multiboot2.h
head.o: reloc.S
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 6f2c668..383ff79 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -1,5 +1,6 @@
#include <xen/config.h>
#include <xen/multiboot.h>
+#include <xen/multiboot2.h>
#include <public/xen.h>
#include <asm/asm_defns.h>
#include <asm/desc.h>
@@ -19,6 +20,28 @@
#define BOOT_PSEUDORM_CS 0x0020
#define BOOT_PSEUDORM_DS 0x0028
+#define MB2_HT(name) (MULTIBOOT2_HEADER_TAG_##name)
+#define MB2_TT(name) (MULTIBOOT2_TAG_TYPE_##name)
+
+ .macro mb2ht_args arg:req, args:vararg
+ .long \arg
+ .ifnb \args
+ mb2ht_args \args
+ .endif
+ .endm
+
+ .macro mb2ht_init type:req, req:req, args:vararg
+ .align MULTIBOOT2_TAG_ALIGN
+.Lmb2ht_init_start\@:
+ .short \type
+ .short \req
+ .long .Lmb2ht_init_end\@ - .Lmb2ht_init_start\@
+ .ifnb \args
+ mb2ht_args \args
+ .endif
+.Lmb2ht_init_end\@:
+ .endm
+
ENTRY(start)
jmp __start
@@ -34,6 +57,42 @@ multiboot1_header_start: /*** MULTIBOOT1 HEADER ****/
.long -(MULTIBOOT_HEADER_MAGIC + MULTIBOOT_HEADER_FLAGS)
multiboot1_header_end:
+/*** MULTIBOOT2 HEADER ****/
+/* Some ideas are taken from grub-2.00/grub-core/tests/boot/kernel-i386.S file. */
+ .align MULTIBOOT2_HEADER_ALIGN
+
+multiboot2_header_start:
+ /* Magic number indicating a Multiboot2 header. */
+ .long MULTIBOOT2_HEADER_MAGIC
+ /* Architecture: i386. */
+ .long MULTIBOOT2_ARCHITECTURE_I386
+ /* Multiboot2 header length. */
+ .long .Lmultiboot2_header_end - multiboot2_header_start
+ /* Multiboot2 header checksum. */
+ .long -(MULTIBOOT2_HEADER_MAGIC + MULTIBOOT2_ARCHITECTURE_I386 + \
+ (.Lmultiboot2_header_end - multiboot2_header_start))
+
+ /* Multiboot2 information request tag. */
+ mb2ht_init MB2_HT(INFORMATION_REQUEST), MB2_HT(REQUIRED), \
+ MB2_TT(BASIC_MEMINFO), MB2_TT(MMAP)
+
+ /* Align modules at page boundry. */
+ mb2ht_init MB2_HT(MODULE_ALIGN), MB2_HT(REQUIRED)
+
+ /* Console flags tag. */
+ mb2ht_init MB2_HT(CONSOLE_FLAGS), MB2_HT(OPTIONAL), \
+ MULTIBOOT2_CONSOLE_FLAGS_EGA_TEXT_SUPPORTED
+
+ /* Framebuffer tag. */
+ mb2ht_init MB2_HT(FRAMEBUFFER), MB2_HT(OPTIONAL), \
+ 0, /* Number of the columns - no preference. */ \
+ 0, /* Number of the lines - no preference. */ \
+ 0 /* Number of bits per pixel - no preference. */
+
+ /* Multiboot2 header end tag. */
+ mb2ht_init MB2_HT(END), MB2_HT(REQUIRED)
+.Lmultiboot2_header_end:
+
.section .init.rodata, "a", @progbits
.align 4
@@ -82,10 +141,52 @@ __start:
mov %ecx,%es
mov %ecx,%ss
- /* Check for Multiboot bootloader */
+ /* Bootloaders may set multiboot{1,2}.mem_lower to a nonzero value. */
+ xor %edx,%edx
+
+ /* Check for Multiboot2 bootloader. */
+ cmp $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
+ je .Lmultiboot2_proto
+
+ /* Check for Multiboot bootloader. */
cmp $MULTIBOOT_BOOTLOADER_MAGIC,%eax
jne not_multiboot
+ /* Get mem_lower from Multiboot information. */
+ testb $MBI_MEMLIMITS,MB_flags(%ebx)
+
+ /* Not available? BDA value will be fine. */
+ cmovnz MB_mem_lower(%ebx),%edx
+ jmp trampoline_setup
+
+.Lmultiboot2_proto:
+ /* Skip Multiboot2 information fixed part. */
+ lea (MB2_fixed_sizeof+MULTIBOOT2_TAG_ALIGN-1)(%ebx),%ecx
+ and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
+
+0:
+ /* Check Multiboot2 information total size. */
+ mov %ecx,%edi
+ sub %ebx,%edi
+ cmp %edi,MB2_fixed_total_size(%ebx)
+ jbe trampoline_setup
+
+ /* Get mem_lower from Multiboot2 information. */
+ cmpl $MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,MB2_tag_type(%ecx)
+ cmove MB2_mem_lower(%ecx),%edx
+ je trampoline_setup
+
+ /* Is it the end of Multiboot2 information? */
+ cmpl $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%ecx)
+ je trampoline_setup
+
+ /* Go to next Multiboot2 information tag. */
+ add MB2_tag_size(%ecx),%ecx
+ add $(MULTIBOOT2_TAG_ALIGN-1),%ecx
+ and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
+ jmp 0b
+
+trampoline_setup:
/* Set up trampoline segment 64k below EBDA */
movzwl 0x40e,%ecx /* EBDA segment */
cmp $0xa000,%ecx /* sanity check (high) */
@@ -100,9 +201,6 @@ __start:
* Compare the value in the BDA with the information from the
* multiboot structure (if available) and use the smallest.
*/
- testb $MBI_MEMLIMITS,(%ebx)
- jz 2f /* not available? BDA value will be fine */
- mov MB_mem_lower(%ebx),%edx
cmp $0x100,%edx /* is the multiboot value too small? */
jb 2f /* if so, do not use it */
shl $10-4,%edx
@@ -121,6 +219,7 @@ __start:
mov $sym_phys(cpu0_stack)+1024,%esp
push %ecx /* Boot trampoline address. */
push %ebx /* Multiboot information address. */
+ push %eax /* Multiboot magic. */
call reloc
mov %eax,sym_phys(multiboot_ptr)
diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
index ea8cb37..94cead7 100644
--- a/xen/arch/x86/boot/reloc.c
+++ b/xen/arch/x86/boot/reloc.c
@@ -5,15 +5,18 @@
* and modules. This is most easily done early with paging disabled.
*
* Copyright (c) 2009, Citrix Systems, Inc.
+ * Copyright (c) 2013-2016 Oracle and/or its affiliates. All rights reserved.
*
* Authors:
* Keir Fraser <keir@xen.org>
+ * Daniel Kiper <daniel.kiper@oracle.com>
*/
/*
* This entry point is entered from xen/arch/x86/boot/head.S with:
- * - 0x4(%esp) = MULTIBOOT_INFORMATION_ADDRESS,
- * - 0x8(%esp) = BOOT_TRAMPOLINE_ADDRESS.
+ * - 0x4(%esp) = MULTIBOOT_MAGIC,
+ * - 0x8(%esp) = MULTIBOOT_INFORMATION_ADDRESS,
+ * - 0xc(%esp) = BOOT_TRAMPOLINE_ADDRESS.
*/
asm (
" .text \n"
@@ -23,7 +26,12 @@ asm (
);
typedef unsigned int u32;
+typedef unsigned long long u64;
+
#include "../../../include/xen/multiboot.h"
+#include "../../../include/xen/multiboot2.h"
+
+#define NULL ((void *)0)
#define __stdcall __attribute__((__stdcall__))
@@ -32,6 +40,9 @@ typedef unsigned int u32;
#define _p(val) ((void *)(unsigned long)(val))
+#define get_mb2_data(tag, type, member) (((multiboot2_tag_##type##_t *)(tag))->member)
+#define get_mb2_string(tag, type, member) ((u32)get_mb2_data(tag, type, member))
+
static u32 alloc;
static u32 alloc_mem(u32 bytes)
@@ -39,6 +50,12 @@ static u32 alloc_mem(u32 bytes)
return alloc -= ALIGN_UP(bytes, 16);
}
+static void zero_mem(u32 s, u32 bytes)
+{
+ while ( bytes-- )
+ *(char *)s++ = 0;
+}
+
static u32 copy_mem(u32 src, u32 bytes)
{
u32 dst, dst_ret;
@@ -65,13 +82,11 @@ static u32 copy_string(u32 src)
return copy_mem(src, p - src + 1);
}
-multiboot_info_t __stdcall *reloc(u32 mbi_in, u32 trampoline)
+static multiboot_info_t *mbi_reloc(u32 mbi_in)
{
int i;
multiboot_info_t *mbi_out;
- alloc = trampoline;
-
mbi_out = _p(copy_mem(mbi_in, sizeof(*mbi_out)));
if ( mbi_out->flags & MBI_CMDLINE )
@@ -108,3 +123,126 @@ multiboot_info_t __stdcall *reloc(u32 mbi_in, u32 trampoline)
return mbi_out;
}
+
+static multiboot_info_t *mbi2_reloc(u32 mbi_in)
+{
+ const multiboot2_fixed_t *mbi_fix = _p(mbi_in);
+ const multiboot2_memory_map_t *mmap_src;
+ const multiboot2_tag_t *tag;
+ module_t *mbi_out_mods = NULL;
+ memory_map_t *mmap_dst;
+ multiboot_info_t *mbi_out;
+ u32 ptr;
+ unsigned int i, mod_idx = 0;
+
+ ptr = alloc_mem(sizeof(*mbi_out));
+ mbi_out = _p(ptr);
+ zero_mem(ptr, sizeof(*mbi_out));
+
+ /* Skip Multiboot2 information fixed part. */
+ ptr = ALIGN_UP(mbi_in + sizeof(multiboot2_fixed_t), MULTIBOOT2_TAG_ALIGN);
+
+ /* Get the number of modules. */
+ for ( tag = _p(ptr); (u32)tag - mbi_in < mbi_fix->total_size;
+ tag = _p(ALIGN_UP((u32)tag + tag->size, MULTIBOOT2_TAG_ALIGN)) )
+ {
+ if ( tag->type == MULTIBOOT2_TAG_TYPE_MODULE )
+ ++mbi_out->mods_count;
+ else if ( tag->type == MULTIBOOT2_TAG_TYPE_END )
+ break;
+ }
+
+ if ( mbi_out->mods_count )
+ {
+ mbi_out->flags |= MBI_MODULES;
+ mbi_out->mods_addr = alloc_mem(mbi_out->mods_count * sizeof(module_t));
+ mbi_out_mods = _p(mbi_out->mods_addr);
+ }
+
+ /* Skip Multiboot2 information fixed part. */
+ ptr = ALIGN_UP(mbi_in + sizeof(multiboot2_fixed_t), MULTIBOOT2_TAG_ALIGN);
+
+ /* Put all needed data into mbi_out. */
+ for ( tag = _p(ptr); (u32)tag - mbi_in < mbi_fix->total_size;
+ tag = _p(ALIGN_UP((u32)tag + tag->size, MULTIBOOT2_TAG_ALIGN)) )
+ switch ( tag->type )
+ {
+ case MULTIBOOT2_TAG_TYPE_BOOT_LOADER_NAME:
+ mbi_out->flags |= MBI_LOADERNAME;
+ ptr = get_mb2_string(tag, string, string);
+ mbi_out->boot_loader_name = copy_string(ptr);
+ break;
+
+ case MULTIBOOT2_TAG_TYPE_CMDLINE:
+ mbi_out->flags |= MBI_CMDLINE;
+ ptr = get_mb2_string(tag, string, string);
+ mbi_out->cmdline = copy_string(ptr);
+ break;
+
+ case MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO:
+ mbi_out->flags |= MBI_MEMLIMITS;
+ mbi_out->mem_lower = get_mb2_data(tag, basic_meminfo, mem_lower);
+ mbi_out->mem_upper = get_mb2_data(tag, basic_meminfo, mem_upper);
+ break;
+
+ case MULTIBOOT2_TAG_TYPE_MMAP:
+ if ( get_mb2_data(tag, mmap, entry_size) < sizeof(*mmap_src) )
+ break;
+
+ mbi_out->flags |= MBI_MEMMAP;
+ mbi_out->mmap_length = get_mb2_data(tag, mmap, size);
+ mbi_out->mmap_length -= sizeof(multiboot2_tag_mmap_t);
+ mbi_out->mmap_length /= get_mb2_data(tag, mmap, entry_size);
+ mbi_out->mmap_length *= sizeof(memory_map_t);
+
+ mbi_out->mmap_addr = alloc_mem(mbi_out->mmap_length);
+
+ mmap_src = get_mb2_data(tag, mmap, entries);
+ mmap_dst = _p(mbi_out->mmap_addr);
+
+ for ( i = 0; i < mbi_out->mmap_length / sizeof(*mmap_dst); i++ )
+ {
+ /* Init size member properly. */
+ mmap_dst[i].size = sizeof(*mmap_dst);
+ mmap_dst[i].size -= sizeof(((memory_map_t){0}).size);
+ /* Now copy a given region data. */
+ mmap_dst[i].base_addr_low = (u32)mmap_src->addr;
+ mmap_dst[i].base_addr_high = (u32)(mmap_src->addr >> 32);
+ mmap_dst[i].length_low = (u32)mmap_src->len;
+ mmap_dst[i].length_high = (u32)(mmap_src->len >> 32);
+ mmap_dst[i].type = mmap_src->type;
+ mmap_src = _p(mmap_src) + get_mb2_data(tag, mmap, entry_size);
+ }
+ break;
+
+ case MULTIBOOT2_TAG_TYPE_MODULE:
+ if ( mod_idx >= mbi_out->mods_count )
+ break;
+
+ mbi_out_mods[mod_idx].mod_start = get_mb2_data(tag, module, mod_start);
+ mbi_out_mods[mod_idx].mod_end = get_mb2_data(tag, module, mod_end);
+ ptr = get_mb2_string(tag, module, cmdline);
+ mbi_out_mods[mod_idx].string = copy_string(ptr);
+ mbi_out_mods[mod_idx].reserved = 0;
+ ++mod_idx;
+ break;
+
+ case MULTIBOOT2_TAG_TYPE_END:
+ return mbi_out;
+
+ default:
+ break;
+ }
+
+ return mbi_out;
+}
+
+multiboot_info_t __stdcall *reloc(u32 mb_magic, u32 mbi_in, u32 trampoline)
+{
+ alloc = trampoline;
+
+ if ( mb_magic == MULTIBOOT2_BOOTLOADER_MAGIC )
+ return mbi2_reloc(mbi_in);
+ else
+ return mbi_reloc(mbi_in);
+}
diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c
index 64905c6..b437a8f 100644
--- a/xen/arch/x86/x86_64/asm-offsets.c
+++ b/xen/arch/x86/x86_64/asm-offsets.c
@@ -13,6 +13,7 @@
#include <asm/fixmap.h>
#include <asm/hardirq.h>
#include <xen/multiboot.h>
+#include <xen/multiboot2.h>
#define DEFINE(_sym, _val) \
asm volatile ("\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\"" \
@@ -167,6 +168,14 @@ void __dummy__(void)
OFFSET(MB_flags, multiboot_info_t, flags);
OFFSET(MB_cmdline, multiboot_info_t, cmdline);
OFFSET(MB_mem_lower, multiboot_info_t, mem_lower);
+ BLANK();
+
+ DEFINE(MB2_fixed_sizeof, sizeof(multiboot2_fixed_t));
+ OFFSET(MB2_fixed_total_size, multiboot2_fixed_t, total_size);
+ OFFSET(MB2_tag_type, multiboot2_tag_t, type);
+ OFFSET(MB2_tag_size, multiboot2_tag_t, size);
+ OFFSET(MB2_mem_lower, multiboot2_tag_basic_meminfo_t, mem_lower);
+ BLANK();
OFFSET(DOMAIN_vm_assist, struct domain, vm_assist);
}
diff --git a/xen/include/xen/multiboot2.h b/xen/include/xen/multiboot2.h
new file mode 100644
index 0000000..8dd5800
--- /dev/null
+++ b/xen/include/xen/multiboot2.h
@@ -0,0 +1,169 @@
+/*
+ * Copyright (C) 1999,2003,2007,2008,2009,2010 Free Software Foundation, Inc.
+ *
+ * multiboot2.h - Multiboot 2 header file.
+ *
+ * Based on grub-2.00/include/multiboot2.h file.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL ANY
+ * DEVELOPER OR DISTRIBUTOR BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR
+ * IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#ifndef __MULTIBOOT2_H__
+#define __MULTIBOOT2_H__
+
+/* The magic field should contain this. */
+#define MULTIBOOT2_HEADER_MAGIC 0xe85250d6
+
+/* This should be in %eax on x86 architecture. */
+#define MULTIBOOT2_BOOTLOADER_MAGIC 0x36d76289
+
+/* How many bytes from the start of the file we search for the header. */
+#define MULTIBOOT2_SEARCH 32768
+
+/* Multiboot 2 header alignment. */
+#define MULTIBOOT2_HEADER_ALIGN 8
+
+/* Alignment of multiboot 2 modules. */
+#define MULTIBOOT2_MOD_ALIGN 0x00001000
+
+/* Alignment of the multiboot 2 info structure. */
+#define MULTIBOOT2_INFO_ALIGN 0x00000008
+
+/* Multiboot 2 architectures. */
+#define MULTIBOOT2_ARCHITECTURE_I386 0
+#define MULTIBOOT2_ARCHITECTURE_MIPS32 4
+
+/* Header tag types. */
+#define MULTIBOOT2_HEADER_TAG_END 0
+#define MULTIBOOT2_HEADER_TAG_INFORMATION_REQUEST 1
+#define MULTIBOOT2_HEADER_TAG_ADDRESS 2
+#define MULTIBOOT2_HEADER_TAG_ENTRY_ADDRESS 3
+#define MULTIBOOT2_HEADER_TAG_CONSOLE_FLAGS 4
+#define MULTIBOOT2_HEADER_TAG_FRAMEBUFFER 5
+#define MULTIBOOT2_HEADER_TAG_MODULE_ALIGN 6
+#define MULTIBOOT2_HEADER_TAG_EFI_BS 7
+#define MULTIBOOT2_HEADER_TAG_ENTRY_ADDRESS_EFI32 8
+#define MULTIBOOT2_HEADER_TAG_ENTRY_ADDRESS_EFI64 9
+
+/* Header tag flags. */
+#define MULTIBOOT2_HEADER_TAG_REQUIRED 0
+#define MULTIBOOT2_HEADER_TAG_OPTIONAL 1
+
+/* Header console tag console_flags. */
+#define MULTIBOOT2_CONSOLE_FLAGS_CONSOLE_REQUIRED 1
+#define MULTIBOOT2_CONSOLE_FLAGS_EGA_TEXT_SUPPORTED 2
+
+/* Flags set in the 'flags' member of the multiboot header. */
+#define MULTIBOOT2_TAG_TYPE_END 0
+#define MULTIBOOT2_TAG_TYPE_CMDLINE 1
+#define MULTIBOOT2_TAG_TYPE_BOOT_LOADER_NAME 2
+#define MULTIBOOT2_TAG_TYPE_MODULE 3
+#define MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO 4
+#define MULTIBOOT2_TAG_TYPE_BOOTDEV 5
+#define MULTIBOOT2_TAG_TYPE_MMAP 6
+#define MULTIBOOT2_TAG_TYPE_VBE 7
+#define MULTIBOOT2_TAG_TYPE_FRAMEBUFFER 8
+#define MULTIBOOT2_TAG_TYPE_ELF_SECTIONS 9
+#define MULTIBOOT2_TAG_TYPE_APM 10
+#define MULTIBOOT2_TAG_TYPE_EFI32 11
+#define MULTIBOOT2_TAG_TYPE_EFI64 12
+#define MULTIBOOT2_TAG_TYPE_SMBIOS 13
+#define MULTIBOOT2_TAG_TYPE_ACPI_OLD 14
+#define MULTIBOOT2_TAG_TYPE_ACPI_NEW 15
+#define MULTIBOOT2_TAG_TYPE_NETWORK 16
+#define MULTIBOOT2_TAG_TYPE_EFI_MMAP 17
+#define MULTIBOOT2_TAG_TYPE_EFI_BS 18
+#define MULTIBOOT2_TAG_TYPE_EFI32_IH 19
+#define MULTIBOOT2_TAG_TYPE_EFI64_IH 20
+
+/* Multiboot 2 tag alignment. */
+#define MULTIBOOT2_TAG_ALIGN 8
+
+/* Memory types. */
+#define MULTIBOOT2_MEMORY_AVAILABLE 1
+#define MULTIBOOT2_MEMORY_RESERVED 2
+#define MULTIBOOT2_MEMORY_ACPI_RECLAIMABLE 3
+#define MULTIBOOT2_MEMORY_NVS 4
+#define MULTIBOOT2_MEMORY_BADRAM 5
+
+/* Framebuffer types. */
+#define MULTIBOOT2_FRAMEBUFFER_TYPE_INDEXED 0
+#define MULTIBOOT2_FRAMEBUFFER_TYPE_RGB 1
+#define MULTIBOOT2_FRAMEBUFFER_TYPE_EGA_TEXT 2
+
+#ifndef __ASSEMBLY__
+typedef struct {
+ u32 total_size;
+ u32 reserved;
+} multiboot2_fixed_t;
+
+typedef struct {
+ u32 type;
+ u32 size;
+} multiboot2_tag_t;
+
+typedef struct {
+ u32 type;
+ u32 size;
+ char string[];
+} multiboot2_tag_string_t;
+
+typedef struct {
+ u32 type;
+ u32 size;
+ u32 mem_lower;
+ u32 mem_upper;
+} multiboot2_tag_basic_meminfo_t;
+
+typedef struct {
+ u64 addr;
+ u64 len;
+ u32 type;
+ u32 zero;
+} multiboot2_memory_map_t;
+
+typedef struct {
+ u32 type;
+ u32 size;
+ u32 entry_size;
+ u32 entry_version;
+ multiboot2_memory_map_t entries[];
+} multiboot2_tag_mmap_t;
+
+typedef struct {
+ u32 type;
+ u32 size;
+ u64 pointer;
+} multiboot2_tag_efi64_t;
+
+typedef struct {
+ u32 type;
+ u32 size;
+ u64 pointer;
+} multiboot2_tag_efi64_ih_t;
+
+typedef struct {
+ u32 type;
+ u32 size;
+ u32 mod_start;
+ u32 mod_end;
+ char cmdline[];
+} multiboot2_tag_module_t;
+#endif /* __ASSEMBLY__ */
+
+#endif /* __MULTIBOOT2_H__ */
--
1.7.10.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v7 03/14] x86: add multiboot2 protocol support
2016-09-23 21:47 ` [PATCH v7 03/14] x86: add multiboot2 protocol support Daniel Kiper
@ 2016-09-26 13:18 ` Jan Beulich
0 siblings, 0 replies; 51+ messages in thread
From: Jan Beulich @ 2016-09-26 13:18 UTC (permalink / raw)
To: Daniel Kiper
Cc: Juergen Gross, sstabellini, andrew.cooper3, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
>>> On 23.09.16 at 23:47, <daniel.kiper@oracle.com> wrote:
> +static multiboot_info_t *mbi2_reloc(u32 mbi_in)
> +{
> + const multiboot2_fixed_t *mbi_fix = _p(mbi_in);
> + const multiboot2_memory_map_t *mmap_src;
> + const multiboot2_tag_t *tag;
> + module_t *mbi_out_mods = NULL;
> + memory_map_t *mmap_dst;
> + multiboot_info_t *mbi_out;
> + u32 ptr;
> + unsigned int i, mod_idx = 0;
> +
> + ptr = alloc_mem(sizeof(*mbi_out));
> + mbi_out = _p(ptr);
> + zero_mem(ptr, sizeof(*mbi_out));
> +
> + /* Skip Multiboot2 information fixed part. */
> + ptr = ALIGN_UP(mbi_in + sizeof(multiboot2_fixed_t), MULTIBOOT2_TAG_ALIGN);
There are still instances (like this one) of sizeof() using type names when
they would better use the type of the variable/expression they actually
refer to (here: *mbi_fix).
With _all_ of them adjusted,
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v7 04/14] efi: create efi_enabled()
2016-09-23 21:47 [PATCH v7 00/14] x86: multiboot2 protocol support Daniel Kiper
` (2 preceding siblings ...)
2016-09-23 21:47 ` [PATCH v7 03/14] x86: add multiboot2 protocol support Daniel Kiper
@ 2016-09-23 21:47 ` Daniel Kiper
2016-09-23 21:47 ` [PATCH v7 05/14] x86: allow EFI reboot method neither on EFI platforms Daniel Kiper
` (9 subsequent siblings)
13 siblings, 0 replies; 51+ messages in thread
From: Daniel Kiper @ 2016-09-23 21:47 UTC (permalink / raw)
To: xen-devel
Cc: jgross, sstabellini, andrew.cooper3, cardoe, pgnet.dev, ning.sun,
julien.grall, david.vrabel, jbeulich, qiaowei.ren, gang.wei,
fu.wei
First of all we need to differentiate between legacy BIOS
and EFI platforms during runtime, not during build, because
one image will have legacy and EFI code and can be executed
on both platforms. Additionally, we need more fine grained
knowledge about EFI environment and check for EFI platform
and EFI loader separately to properly support multiboot2
protocol. In general Xen loaded by this protocol uses memory
mappings and loaded modules in similar way to Xen loaded by
multiboot (v1) protocol. Hence, create efi_enabled() which
checks available features in efi_flags. This patch defines
EFI_BOOT, EFI_LOADER and EFI_RS features. EFI_BOOT is equal
to old efi_enabled == 1. EFI_RS ease control on runtime
services usage. EFI_LOADER tells that Xen was loaded
directly from EFI as PE executable.
Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
v7 - suggestions/fixes:
- remove efi_enabled(EFI_RS) check from mapcache_current_vcpu()
(suggested by Daniel Kiper and Jan Beulich),
- remove BUG() from xen/arch/x86/efi/stub.c:efi_rs_using_pgtables()
(suggested by Daniel Kiper and Jan Beulich).
v6 - suggestions/fixes:
- define efi_enabled() as "bool efi_enabled(unsigned int feature)"
instead of "bool_t efi_enabled(int feature)"
(suggested by Jan Beulich),
- define efi_flags as unsigned int
(suggested by Jan Beulich),
- various minor cleanups and fixes
(suggested by Jan Beulich).
v5 - suggestions/fixes:
- squash three patches into one
(suggested by Jan Beulich),
- introduce all features at once
(suggested by Jan Beulich),
- efi_enabled() returns bool_t
instead of unsigned int
(suggested by Jan Beulich),
- update commit message.
v4 - suggestions/fixes:
- rename EFI_PLATFORM to EFI_BOOT
(suggested by Jan Beulich),
- move EFI_BOOT definition to efi struct definition
(suggested by Jan Beulich),
- remove unneeded efi.flags initialization
(suggested by Jan Beulich),
- use __set_bit() instead of set_bit() if possible
(suggested by Jan Beulich),
- do efi_enabled() cleanup
(suggested by Jan Beulich),
- improve comments
(suggested by Jan Beulich),
- improve commit message.
v3 - suggestions/fixes:
- define efi struct in xen/arch/x86/efi/stub.c
in earlier patch
(suggested by Jan Beulich),
- improve comments
(suggested by Jan Beulich),
- improve commit message
(suggested by Jan Beulich).
---
xen/arch/x86/dmi_scan.c | 4 ++--
xen/arch/x86/domain_page.c | 2 +-
xen/arch/x86/efi/stub.c | 8 ++++----
xen/arch/x86/mpparse.c | 4 ++--
xen/arch/x86/setup.c | 12 +++++++-----
xen/arch/x86/shutdown.c | 2 +-
xen/arch/x86/time.c | 2 +-
xen/common/efi/boot.c | 19 +++++++++++++++----
xen/common/efi/runtime.c | 14 ++++++++------
xen/common/version.c | 2 +-
xen/drivers/acpi/osl.c | 2 +-
xen/include/xen/efi.h | 8 ++++++--
12 files changed, 49 insertions(+), 30 deletions(-)
diff --git a/xen/arch/x86/dmi_scan.c b/xen/arch/x86/dmi_scan.c
index b049e31..8dcb640 100644
--- a/xen/arch/x86/dmi_scan.c
+++ b/xen/arch/x86/dmi_scan.c
@@ -238,7 +238,7 @@ const char *__init dmi_get_table(paddr_t *base, u32 *len)
{
static unsigned int __initdata instance;
- if (efi_enabled) {
+ if (efi_enabled(EFI_BOOT)) {
if (efi_smbios3_size && !(instance & 1)) {
*base = efi_smbios3_address;
*len = efi_smbios3_size;
@@ -696,7 +696,7 @@ static void __init dmi_decode(struct dmi_header *dm)
void __init dmi_scan_machine(void)
{
- if ((!efi_enabled ? dmi_iterate(dmi_decode) :
+ if ((!efi_enabled(EFI_BOOT) ? dmi_iterate(dmi_decode) :
dmi_efi_iterate(dmi_decode)) == 0)
dmi_check_system(dmi_blacklist);
else
diff --git a/xen/arch/x86/domain_page.c b/xen/arch/x86/domain_page.c
index d86f8fe..a58ef8e 100644
--- a/xen/arch/x86/domain_page.c
+++ b/xen/arch/x86/domain_page.c
@@ -36,7 +36,7 @@ static inline struct vcpu *mapcache_current_vcpu(void)
* domain's page tables but current may point at another domain's VCPU.
* Return NULL as though current is not properly set up yet.
*/
- if ( efi_enabled && efi_rs_using_pgtables() )
+ if ( efi_rs_using_pgtables() )
return NULL;
/*
diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.c
index 07c2bd0..4158124 100644
--- a/xen/arch/x86/efi/stub.c
+++ b/xen/arch/x86/efi/stub.c
@@ -4,9 +4,10 @@
#include <xen/lib.h>
#include <asm/page.h>
-#ifndef efi_enabled
-const bool_t efi_enabled = 0;
-#endif
+bool efi_enabled(unsigned int feature)
+{
+ return false;
+}
void __init efi_init_memory(void) { }
@@ -14,7 +15,6 @@ void efi_update_l4_pgtable(unsigned int l4idx, l4_pgentry_t l4e) { }
bool_t efi_rs_using_pgtables(void)
{
- BUG();
return 0;
}
diff --git a/xen/arch/x86/mpparse.c b/xen/arch/x86/mpparse.c
index ef6557c..c3d5bdc 100644
--- a/xen/arch/x86/mpparse.c
+++ b/xen/arch/x86/mpparse.c
@@ -564,7 +564,7 @@ static inline void __init construct_default_ISA_mptable(int mpc_default_type)
static __init void efi_unmap_mpf(void)
{
- if (efi_enabled)
+ if (efi_enabled(EFI_BOOT))
clear_fixmap(FIX_EFI_MPF);
}
@@ -722,7 +722,7 @@ void __init find_smp_config (void)
{
unsigned int address;
- if (efi_enabled) {
+ if (efi_enabled(EFI_BOOT)) {
efi_check_config();
return;
}
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 8ae897a..06f3970 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -439,8 +439,8 @@ static void __init parse_video_info(void)
{
struct boot_video_info *bvi = &bootsym(boot_vid_info);
- /* The EFI loader fills vga_console_info directly. */
- if ( efi_enabled )
+ /* vga_console_info is filled directly on EFI platform. */
+ if ( efi_enabled(EFI_BOOT) )
return;
if ( (bvi->orig_video_isVGA == 1) && (bvi->orig_video_mode == 3) )
@@ -726,7 +726,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
if ( !(mbi->flags & MBI_MODULES) || (mbi->mods_count == 0) )
panic("dom0 kernel not specified. Check bootloader configuration.");
- if ( efi_enabled )
+ if ( efi_enabled(EFI_LOADER) )
{
set_pdx_range(xen_phys_start >> PAGE_SHIFT,
(xen_phys_start + BOOTSTRAP_MAP_BASE) >> PAGE_SHIFT);
@@ -741,6 +741,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
memmap_type = loader;
}
+ else if ( efi_enabled(EFI_BOOT) )
+ memmap_type = "EFI";
else if ( e820_raw_nr != 0 )
{
memmap_type = "Xen-e820";
@@ -837,7 +839,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
* we can relocate the dom0 kernel and other multiboot modules. Also, on
* x86/64, we relocate Xen to higher memory.
*/
- for ( i = 0; !efi_enabled && i < mbi->mods_count; i++ )
+ for ( i = 0; !efi_enabled(EFI_LOADER) && i < mbi->mods_count; i++ )
{
if ( mod[i].mod_start & (PAGE_SIZE - 1) )
panic("Bootloader didn't honor module alignment request.");
@@ -1078,7 +1080,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
if ( !xen_phys_start )
panic("Not enough memory to relocate Xen.");
- reserve_e820_ram(&boot_e820, efi_enabled ? mbi->mem_upper : __pa(&_start),
+ reserve_e820_ram(&boot_e820, efi_enabled(EFI_LOADER) ? mbi->mem_upper : __pa(&_start),
__pa(&_end));
/* Late kexec reservation (dynamic start address). */
diff --git a/xen/arch/x86/shutdown.c b/xen/arch/x86/shutdown.c
index 0e1499d..54c2c79 100644
--- a/xen/arch/x86/shutdown.c
+++ b/xen/arch/x86/shutdown.c
@@ -116,7 +116,7 @@ void machine_halt(void)
static void default_reboot_type(void)
{
if ( reboot_type == BOOT_INVALID )
- reboot_type = efi_enabled ? BOOT_EFI
+ reboot_type = efi_enabled(EFI_RS) ? BOOT_EFI
: acpi_disabled ? BOOT_KBD
: BOOT_ACPI;
}
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 73e0f98..f043998 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -727,7 +727,7 @@ static unsigned long get_cmos_time(void)
static bool_t __read_mostly cmos_rtc_probe;
boolean_param("cmos-rtc-probe", cmos_rtc_probe);
- if ( efi_enabled )
+ if ( efi_enabled(EFI_RS) )
{
res = efi_get_time();
if ( res )
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 125c9ce..56544dc 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -934,6 +934,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
char *option_str;
bool_t use_cfg_file;
+ __set_bit(EFI_BOOT, &efi_flags);
+ __set_bit(EFI_LOADER, &efi_flags);
+
+#ifndef CONFIG_ARM /* Disabled until runtime services implemented. */
+ __set_bit(EFI_RS, &efi_flags);
+#endif
+
efi_init(ImageHandle, SystemTable);
use_cfg_file = efi_arch_use_config_file(SystemTable);
@@ -1153,7 +1160,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
#ifndef CONFIG_ARM /* TODO - runtime service support */
-static bool_t __initdata efi_rs_enable = 1;
static bool_t __initdata efi_map_uc;
static void __init parse_efi_param(char *s)
@@ -1171,7 +1177,12 @@ static void __init parse_efi_param(char *s)
*ss = '\0';
if ( !strcmp(s, "rs") )
- efi_rs_enable = val;
+ {
+ if ( val )
+ __set_bit(EFI_RS, &efi_flags);
+ else
+ __clear_bit(EFI_RS, &efi_flags);
+ }
else if ( !strcmp(s, "attr=uc") )
efi_map_uc = val;
@@ -1254,7 +1265,7 @@ void __init efi_init_memory(void)
desc->PhysicalStart, desc->PhysicalStart + len - 1,
desc->Type, desc->Attribute);
- if ( !efi_rs_enable ||
+ if ( !efi_enabled(EFI_RS) ||
(!(desc->Attribute & EFI_MEMORY_RUNTIME) &&
(!map_bs ||
(desc->Type != EfiBootServicesCode &&
@@ -1328,7 +1339,7 @@ void __init efi_init_memory(void)
}
}
- if ( !efi_rs_enable )
+ if ( !efi_enabled(EFI_RS) )
{
efi_fw_vendor = NULL;
return;
diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index c256814..5f2de80 100644
--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -10,12 +10,6 @@ DEFINE_XEN_GUEST_HANDLE(CHAR16);
#ifndef COMPAT
-/*
- * Currently runtime services are not implemented on ARM. To boot Xen with ACPI,
- * set efi_enabled to 1, so that Xen can get the ACPI root pointer from EFI.
- */
-const bool_t efi_enabled = 1;
-
#ifndef CONFIG_ARM
# include <asm/i387.h>
# include <asm/xstate.h>
@@ -43,6 +37,9 @@ UINT64 __read_mostly efi_boot_max_var_store_size;
UINT64 __read_mostly efi_boot_remain_var_store_size;
UINT64 __read_mostly efi_boot_max_var_size;
+/* Bit field representing available EFI features/properties. */
+unsigned int efi_flags;
+
struct efi __read_mostly efi = {
.acpi = EFI_INVALID_TABLE_ADDR,
.acpi20 = EFI_INVALID_TABLE_ADDR,
@@ -53,6 +50,11 @@ struct efi __read_mostly efi = {
const struct efi_pci_rom *__read_mostly efi_pci_roms;
+bool efi_enabled(unsigned int feature)
+{
+ return test_bit(feature, &efi_flags);
+}
+
#ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */
unsigned long efi_rs_enter(void)
{
diff --git a/xen/common/version.c b/xen/common/version.c
index 0d31e38..223cb52 100644
--- a/xen/common/version.c
+++ b/xen/common/version.c
@@ -160,7 +160,7 @@ static int __init xen_build_init(void)
#ifdef CONFIG_X86
/* Alternatively we may have a CodeView record from an EFI build. */
- if ( rc && efi_enabled )
+ if ( rc && efi_enabled(EFI_LOADER) )
{
const struct pe_external_debug_directory *dir = (const void *)n;
diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
index 9a49029..3616dfd 100644
--- a/xen/drivers/acpi/osl.c
+++ b/xen/drivers/acpi/osl.c
@@ -66,7 +66,7 @@ void __init acpi_os_vprintf(const char *fmt, va_list args)
acpi_physical_address __init acpi_os_get_root_pointer(void)
{
- if (efi_enabled) {
+ if (efi_enabled(EFI_BOOT)) {
if (efi.acpi20 != EFI_INVALID_TABLE_ADDR)
return efi.acpi20;
else if (efi.acpi != EFI_INVALID_TABLE_ADDR)
diff --git a/xen/include/xen/efi.h b/xen/include/xen/efi.h
index e74dad1..68c68a8 100644
--- a/xen/include/xen/efi.h
+++ b/xen/include/xen/efi.h
@@ -5,10 +5,13 @@
#include <xen/types.h>
#endif
-extern const bool_t efi_enabled;
-
#define EFI_INVALID_TABLE_ADDR (~0UL)
+extern unsigned int efi_flags;
+#define EFI_BOOT 0 /* Were we booted from EFI? */
+#define EFI_LOADER 1 /* Were we booted directly from EFI loader? */
+#define EFI_RS 2 /* Can we use runtime services? */
+
/* Add fields here only if they need to be referenced from non-EFI code. */
struct efi {
unsigned long mps; /* MPS table */
@@ -28,6 +31,7 @@ union compat_pf_efi_info;
struct xenpf_efi_runtime_call;
struct compat_pf_efi_runtime_call;
+bool efi_enabled(unsigned int feature);
void efi_init_memory(void);
bool_t efi_rs_using_pgtables(void);
unsigned long efi_get_time(void);
--
1.7.10.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH v7 05/14] x86: allow EFI reboot method neither on EFI platforms...
2016-09-23 21:47 [PATCH v7 00/14] x86: multiboot2 protocol support Daniel Kiper
` (3 preceding siblings ...)
2016-09-23 21:47 ` [PATCH v7 04/14] efi: create efi_enabled() Daniel Kiper
@ 2016-09-23 21:47 ` Daniel Kiper
2016-09-23 21:47 ` [PATCH v7 06/14] efi: build xen.gz with EFI code Daniel Kiper
` (8 subsequent siblings)
13 siblings, 0 replies; 51+ messages in thread
From: Daniel Kiper @ 2016-09-23 21:47 UTC (permalink / raw)
To: xen-devel
Cc: jgross, sstabellini, andrew.cooper3, cardoe, pgnet.dev, ning.sun,
julien.grall, david.vrabel, jbeulich, qiaowei.ren, gang.wei,
fu.wei
..nor EFI platforms with runtime services enabled.
Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
v6 - suggestions/fixes:
- move this commit behind "efi: create efi_enabled()" commit
(suggested by Jan Beulich).
v5 - suggestions/fixes:
- fix build error
(suggested by Jan Beulich),
- improve commit message.
---
xen/arch/x86/shutdown.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/xen/arch/x86/shutdown.c b/xen/arch/x86/shutdown.c
index 54c2c79..b429fd0 100644
--- a/xen/arch/x86/shutdown.c
+++ b/xen/arch/x86/shutdown.c
@@ -80,6 +80,9 @@ static void __init set_reboot_type(char *str)
break;
str++;
}
+
+ if ( reboot_type == BOOT_EFI && !efi_enabled(EFI_RS) )
+ reboot_type = BOOT_INVALID;
}
custom_param("reboot", set_reboot_type);
--
1.7.10.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH v7 06/14] efi: build xen.gz with EFI code
2016-09-23 21:47 [PATCH v7 00/14] x86: multiboot2 protocol support Daniel Kiper
` (4 preceding siblings ...)
2016-09-23 21:47 ` [PATCH v7 05/14] x86: allow EFI reboot method neither on EFI platforms Daniel Kiper
@ 2016-09-23 21:47 ` Daniel Kiper
2016-09-23 21:47 ` [PATCH RFC v7 07/14] efi: create new early memory allocator Daniel Kiper
` (7 subsequent siblings)
13 siblings, 0 replies; 51+ messages in thread
From: Daniel Kiper @ 2016-09-23 21:47 UTC (permalink / raw)
To: xen-devel
Cc: jgross, sstabellini, andrew.cooper3, cardoe, pgnet.dev, ning.sun,
julien.grall, david.vrabel, jbeulich, qiaowei.ren, gang.wei,
fu.wei
Build xen.gz with EFI code. We need this to support multiboot2
protocol on EFI platforms.
If we wish to load non-ELF file using multiboot (v1) or multiboot2 then
it must contain "linear" (or "flat") representation of code and data.
This is requirement of both boot protocols. Currently, PE file contains
many sections which are not "linear" (one after another without any holes)
or even do not have representation in a file (e.g. BSS). From EFI point
of view everything is OK and works. However, this file layout cannot be
properly interpreted by multiboot protocols family. In theory there is
a chance that we could build proper PE file (from multiboot protocols POV)
using current build system. However, it means that xen.efi further diverge
from Xen ELF file (in terms of contents and build method). On the other
hand ELF has all needed properties. So, it means that this is good starting
point for further development. Additionally, I think that this is also good
starting point for further xen.efi code and build optimizations. It looks
that there is a chance that finally we can generate xen.efi directly from
Xen ELF using just simple objcopy or other tool. This way we will have one
Xen binary which can be loaded by three boot protocols: EFI native loader,
multiboot (v1) and multiboot2.
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
v6 - suggestions/fixes:
- improve efi_enabled() checks in efi_runtime_call()
(suggested by Jan Beulich).
v5 - suggestions/fixes:
- properly calculate efi symbol address in
xen/arch/x86/xen.lds.S (I hope that this
change does not invalidate Jan's ACK).
v4 - suggestions/fixes:
- functions should return -ENOSYS instead
of -EOPNOTSUPP if EFI runtime services
are not available
(suggested by Jan Beulich),
- remove stale bits from xen/arch/x86/Makefile
(suggested by Jan Beulich).
v3 - suggestions/fixes:
- check for EFI platform in EFI code
(suggested by Jan Beulich),
- fix Makefiles
(suggested by Jan Beulich),
- improve commit message
(suggested by Jan Beulich).
v2 - suggestions/fixes:
- build EFI code only if it is supported in a given build environment
(suggested by Jan Beulich).
---
xen/arch/x86/Makefile | 2 +-
xen/arch/x86/efi/Makefile | 12 ++++--------
xen/arch/x86/xen.lds.S | 4 ++--
xen/common/efi/boot.c | 3 +++
xen/common/efi/runtime.c | 9 +++++++++
5 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index c3a8920..49d7e59 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -223,7 +223,7 @@ efi/mkreloc: efi/mkreloc.c
clean::
rm -f asm-offsets.s *.lds boot/*.o boot/*~ boot/core boot/mkelf32
rm -f $(BASEDIR)/.xen-syms.[0-9]* boot/.*.d
- rm -f $(BASEDIR)/.xen.efi.[0-9]* efi/*.o efi/.*.d efi/*.efi efi/disabled efi/mkreloc
+ rm -f $(BASEDIR)/.xen.efi.[0-9]* efi/*.efi efi/disabled efi/mkreloc
rm -f boot/reloc.S boot/reloc.lnk boot/reloc.bin
rm -f note.o
$(MAKE) -f $(BASEDIR)/Rules.mk -C test clean
diff --git a/xen/arch/x86/efi/Makefile b/xen/arch/x86/efi/Makefile
index ad3fdf7..442f3fc 100644
--- a/xen/arch/x86/efi/Makefile
+++ b/xen/arch/x86/efi/Makefile
@@ -1,18 +1,14 @@
CFLAGS += -fshort-wchar
-obj-y += stub.o
-
-create = test -e $(1) || touch -t 199901010000 $(1)
-
efi := y$(shell rm -f disabled)
efi := $(if $(efi),$(shell $(CC) $(filter-out $(CFLAGS-y) .%.d,$(CFLAGS)) -c check.c 2>disabled && echo y))
efi := $(if $(efi),$(shell $(LD) -mi386pep --subsystem=10 -o check.efi check.o 2>disabled && echo y))
-efi := $(if $(efi),$(shell rm disabled)y,$(shell $(call create,boot.init.o); $(call create,runtime.o)))
-
-extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o buildid.o
+efi := $(if $(efi),$(shell rm disabled)y)
%.o: %.ihex
$(OBJCOPY) -I ihex -O binary $< $@
-stub.o: $(extra-y)
+obj-y := stub.o
+obj-$(efi) := boot.init.o compat.o relocs-dummy.o runtime.o
+extra-$(efi) += buildid.o
nogcov-$(efi) += stub.o
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 9adb2f3..38baea3 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -267,8 +267,6 @@ SECTIONS
*(.reloc)
} :text
. = ALIGN(__section_alignment__);
-#else
- efi = .;
#endif
/* Trick the linker into setting the image size to exactly 16Mb. */
@@ -277,6 +275,8 @@ SECTIONS
__end_of_image__ = .;
} :text
+ efi = DEFINED(efi) ? efi : .;
+
/* Sections to be discarded */
/DISCARD/ : {
*(.exit.text)
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 56544dc..1ef5d0b 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1251,6 +1251,9 @@ void __init efi_init_memory(void)
} *extra, *extra_head = NULL;
#endif
+ if ( !efi_enabled(EFI_BOOT) )
+ return;
+
printk(XENLOG_INFO "EFI memory map:%s\n",
map_bs ? " (mapping BootServices)" : "");
for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index 5f2de80..41f49f7 100644
--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -174,6 +174,9 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
{
unsigned int i, n;
+ if ( !efi_enabled(EFI_BOOT) )
+ return -ENOSYS;
+
switch ( idx )
{
case XEN_FW_EFI_VERSION:
@@ -308,6 +311,12 @@ int efi_runtime_call(struct xenpf_efi_runtime_call *op)
EFI_STATUS status = EFI_NOT_STARTED;
int rc = 0;
+ if ( !efi_enabled(EFI_BOOT) )
+ return -ENOSYS;
+
+ if ( !efi_enabled(EFI_RS) )
+ return -EOPNOTSUPP;
+
switch ( op->function )
{
case XEN_EFI_get_time:
--
1.7.10.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH RFC v7 07/14] efi: create new early memory allocator
2016-09-23 21:47 [PATCH v7 00/14] x86: multiboot2 protocol support Daniel Kiper
` (5 preceding siblings ...)
2016-09-23 21:47 ` [PATCH v7 06/14] efi: build xen.gz with EFI code Daniel Kiper
@ 2016-09-23 21:47 ` Daniel Kiper
2016-09-23 23:35 ` Julien Grall
2016-09-26 13:37 ` Jan Beulich
2016-09-23 21:47 ` [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms Daniel Kiper
` (6 subsequent siblings)
13 siblings, 2 replies; 51+ messages in thread
From: Daniel Kiper @ 2016-09-23 21:47 UTC (permalink / raw)
To: xen-devel
Cc: jgross, sstabellini, andrew.cooper3, cardoe, pgnet.dev, ning.sun,
julien.grall, david.vrabel, jbeulich, qiaowei.ren, gang.wei,
fu.wei
There is a problem with place_string() which is used as early memory
allocator. It gets memory chunks starting from start symbol and goes
down. Sadly this does not work when Xen is loaded using multiboot2
protocol because then the start lives on 1 MiB address and we should
not allocate a memory from below of it. So, I tried to use mem_lower
address calculated by GRUB2. However, this solution works only on some
machines. There are machines in the wild (e.g. Dell PowerEdge R820)
which uses first ~640 KiB for boot services code or data... :-(((
Hence, we need new memory allocator for Xen EFI boot code which is
quite simple and generic and could be used by place_string() and
efi_arch_allocate_mmap_buffer(). I think about following solutions:
1) We could use native EFI allocation functions (e.g. AllocatePool()
or AllocatePages()) to get memory chunk. However, later (somewhere
in __start_xen()) we must copy its contents to safe place or reserve
it in e820 memory map and map it in Xen virtual address space. This
means that the code referring to Xen command line, loaded modules and
EFI memory map, mostly in __start_xen(), will be further complicated
and diverge from legacy BIOS cases. Additionally, both former things
have to be placed below 4 GiB because their addresses are stored in
multiboot_info_t structure which has 32-bit relevant members.
2) We may allocate memory area statically somewhere in Xen code which
could be used as memory pool for early dynamic allocations. Looks
quite simple. Additionally, it would not depend on EFI at all and
could be used on legacy BIOS platforms if we need it. However, we
must carefully choose size of this pool. We do not want increase Xen
binary size too much and waste too much memory but also we must fit
at least memory map on x86 EFI platforms. As I saw on small machine,
e.g. IBM System x3550 M2 with 8 GiB RAM, memory map may contain more
than 200 entries. Every entry on x86-64 platform is 40 bytes in size.
So, it means that we need more than 8 KiB for EFI memory map only.
Additionally, if we use this memory pool for Xen and modules command
line storage (it would be used when xen.efi is executed as EFI application)
then we should add, I think, about 1 KiB. In this case, to be on safe
side, we should assume at least 64 KiB pool for early memory allocations.
Which is about 4 times of our earlier calculations. However, during
discussion on Xen-devel Jan Beulich suggested that just in case we should
use 1 MiB memory pool like it is in original place_string() implementation.
So, let's use 1 MiB as it was proposed. If we think that we should not
waste unallocated memory in the pool on running system then we can mark
this region as __initdata and move all required data to dynamically
allocated places somewhere in __start_xen().
2a) We could put memory pool into .bss.page_aligned section. Then allocate
memory chunks starting from the lowest address. After init phase we can
free unused portion of the memory pool as in case of .init.text or .init.data
sections. This way we do not need to allocate any space in image file and
freeing of unused area in the memory pool is very simple.
Now #2a solution is implemented because it is quite simple and requires
limited number of changes, especially in __start_xen().
New allocator is quite generic and can be used on ARM platforms too.
So, let's enable most of its machinery on them.
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
v7 - suggestions/fixes:
- enable most of ebmalloc machinery on ARM platforms
(suggested by Jan Beulich),
- remove unneeded cast
(suggested by Jan Beulich),
- wrap long line
(suggested by Jan Beulich),
- improve commit message.
v6 - suggestions/fixes:
- optimize ebmalloc allocator,
- move ebmalloc machinery to xen/common/efi/boot.c
(suggested by Jan Beulich),
- enforce PAGE_SIZE ebmalloc_mem alignment
(suggested by Jan Beulich),
- ebmalloc() must allocate properly
aligned memory regions
(suggested by Jan Beulich),
- printk() should use XENLOG_INFO
(suggested by Jan Beulich).
v4 - suggestions/fixes:
- move from #2 solution to #2a solution,
- improve commit message.
---
xen/arch/arm/setup.c | 2 ++
xen/arch/x86/efi/efi-boot.h | 11 +++--------
xen/arch/x86/efi/stub.c | 2 ++
xen/arch/x86/setup.c | 5 +++--
xen/common/efi/boot.c | 38 ++++++++++++++++++++++++++++++++++++++
xen/include/xen/efi.h | 1 +
6 files changed, 49 insertions(+), 10 deletions(-)
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 38eb888..2085f35 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -38,6 +38,7 @@
#include <xen/vmap.h>
#include <xen/libfdt/libfdt.h>
#include <xen/acpi.h>
+#include <xen/efi.h>
#include <asm/alternative.h>
#include <asm/page.h>
#include <asm/current.h>
@@ -66,6 +67,7 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes);
static __used void init_done(void)
{
+ free_ebmalloc_unused_mem();
free_init_memory();
startup_cpu_idle_loop();
}
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 388c4ea..62c010e 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -114,7 +114,7 @@ static void __init relocate_trampoline(unsigned long phys)
static void __init place_string(u32 *addr, const char *s)
{
- static char *__initdata alloc = start;
+ char *alloc = NULL;
if ( s && *s )
{
@@ -122,7 +122,7 @@ static void __init place_string(u32 *addr, const char *s)
const char *old = (char *)(long)*addr;
size_t len2 = *addr ? strlen(old) + 1 : 0;
- alloc -= len1 + len2;
+ alloc = ebmalloc(len1 + len2);
/*
* Insert new string before already existing one. This is needed
* for options passed on the command line to override options from
@@ -205,12 +205,7 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size)
{
- place_string(&mbi.mem_upper, NULL);
- mbi.mem_upper -= map_size;
- mbi.mem_upper &= -__alignof__(EFI_MEMORY_DESCRIPTOR);
- if ( mbi.mem_upper < xen_phys_start )
- return NULL;
- return (void *)(long)mbi.mem_upper;
+ return ebmalloc(map_size);
}
static void __init efi_arch_pre_exit_boot(void)
diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.c
index 4158124..bc9919c 100644
--- a/xen/arch/x86/efi/stub.c
+++ b/xen/arch/x86/efi/stub.c
@@ -9,6 +9,8 @@ bool efi_enabled(unsigned int feature)
return false;
}
+void __init free_ebmalloc_unused_mem(void) { }
+
void __init efi_init_memory(void) { }
void efi_update_l4_pgtable(unsigned int l4idx, l4_pgentry_t l4e) { }
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 06f3970..5b17783 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -520,6 +520,8 @@ static void noinline init_done(void)
system_state = SYS_STATE_active;
+ free_ebmalloc_unused_mem();
+
/* MUST be done prior to removing .init data. */
unregister_init_virtual_region();
@@ -1080,8 +1082,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
if ( !xen_phys_start )
panic("Not enough memory to relocate Xen.");
- reserve_e820_ram(&boot_e820, efi_enabled(EFI_LOADER) ? mbi->mem_upper : __pa(&_start),
- __pa(&_end));
+ reserve_e820_ram(&boot_e820, __pa(&_start), __pa(&_end));
/* Late kexec reservation (dynamic start address). */
kexec_reserve_area(&boot_e820);
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 1ef5d0b..91df9b5 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -79,6 +79,10 @@ static size_t wstrlen(const CHAR16 * s);
static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz);
static bool_t match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2);
+#ifndef CONFIG_ARM
+static void *ebmalloc(size_t size);
+#endif
+
static const EFI_BOOT_SERVICES *__initdata efi_bs;
static UINT32 __initdata efi_bs_revision;
static EFI_HANDLE __initdata efi_ih;
@@ -577,6 +581,40 @@ static char *__init get_value(const struct file *cfg, const char *section,
return NULL;
}
+#define EBMALLOC_SIZE MB(1)
+
+static char __section(".bss.page_aligned") __aligned(PAGE_SIZE)
+ ebmalloc_mem[EBMALLOC_SIZE];
+static unsigned long __initdata ebmalloc_allocated;
+
+#ifndef CONFIG_ARM
+/* EFI boot allocator. */
+static void __init *ebmalloc(size_t size)
+{
+ void *ptr = ebmalloc_mem + ebmalloc_allocated;
+
+ ebmalloc_allocated += (size + sizeof(void *) - 1) & ~(sizeof(void *) - 1);
+
+ if ( ebmalloc_allocated > sizeof(ebmalloc_mem) )
+ blexit(L"Out of static memory\r\n");
+
+ return ptr;
+}
+#endif
+
+void __init free_ebmalloc_unused_mem(void)
+{
+ unsigned long start, end;
+
+ start = (unsigned long)ebmalloc_mem + PAGE_ALIGN(ebmalloc_allocated);
+ end = (unsigned long)ebmalloc_mem + sizeof(ebmalloc_mem);
+
+ destroy_xen_mappings(start, end);
+ init_xenheap_pages(__pa(start), __pa(end));
+
+ printk(XENLOG_INFO "Freed %lukB unused BSS memory\n", (end - start) >> 10);
+}
+
static void __init efi_init(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
{
efi_ih = ImageHandle;
diff --git a/xen/include/xen/efi.h b/xen/include/xen/efi.h
index 68c68a8..6c7f601 100644
--- a/xen/include/xen/efi.h
+++ b/xen/include/xen/efi.h
@@ -32,6 +32,7 @@ struct xenpf_efi_runtime_call;
struct compat_pf_efi_runtime_call;
bool efi_enabled(unsigned int feature);
+void free_ebmalloc_unused_mem(void);
void efi_init_memory(void);
bool_t efi_rs_using_pgtables(void);
unsigned long efi_get_time(void);
--
1.7.10.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH RFC v7 07/14] efi: create new early memory allocator
2016-09-23 21:47 ` [PATCH RFC v7 07/14] efi: create new early memory allocator Daniel Kiper
@ 2016-09-23 23:35 ` Julien Grall
2016-09-26 6:53 ` Jan Beulich
2016-09-26 13:37 ` Jan Beulich
1 sibling, 1 reply; 51+ messages in thread
From: Julien Grall @ 2016-09-23 23:35 UTC (permalink / raw)
To: Daniel Kiper, xen-devel
Cc: jgross, sstabellini, andrew.cooper3, cardoe, pgnet.dev, ning.sun,
david.vrabel, jbeulich, qiaowei.ren, gang.wei, fu.wei
Hi Daniel,
On 23/09/2016 22:47, Daniel Kiper wrote:
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 38eb888..2085f35 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -38,6 +38,7 @@
> #include <xen/vmap.h>
> #include <xen/libfdt/libfdt.h>
> #include <xen/acpi.h>
> +#include <xen/efi.h>
> #include <asm/alternative.h>
> #include <asm/page.h>
> #include <asm/current.h>
> @@ -66,6 +67,7 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes);
>
> static __used void init_done(void)
> {
> + free_ebmalloc_unused_mem();
I said no to this on the previous version. And I think Jan suggested a
per-arch way to do it. So why is it here?
As mentioned, the EFI stub is standalone and should stay as it for now.
With this solution, you break this assumption. I explained why it should
not be done, and the issue you will run into by doing that.
So please don't call a function on ARM where it is known that it will
not work.
Also, the EFI is not mandatory on ARM, for instance this code will not
build on ARM32.
I much prefer a TODO comment (around the #ifdef CONFIG_ARM) to say what
needs to be done for the time being.
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH RFC v7 07/14] efi: create new early memory allocator
2016-09-23 23:35 ` Julien Grall
@ 2016-09-26 6:53 ` Jan Beulich
2016-09-26 20:01 ` Julien Grall
0 siblings, 1 reply; 51+ messages in thread
From: Jan Beulich @ 2016-09-26 6:53 UTC (permalink / raw)
To: Julien Grall
Cc: Juergen Gross, sstabellini, andrew.cooper3, Daniel Kiper, cardoe,
pgnet.dev, ning.sun, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
>>> On 24.09.16 at 01:35, <julien.grall@arm.com> wrote:
> Hi Daniel,
>
> On 23/09/2016 22:47, Daniel Kiper wrote:
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index 38eb888..2085f35 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -38,6 +38,7 @@
>> #include <xen/vmap.h>
>> #include <xen/libfdt/libfdt.h>
>> #include <xen/acpi.h>
>> +#include <xen/efi.h>
>> #include <asm/alternative.h>
>> #include <asm/page.h>
>> #include <asm/current.h>
>> @@ -66,6 +67,7 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes);
>>
>> static __used void init_done(void)
>> {
>> + free_ebmalloc_unused_mem();
>
> I said no to this on the previous version. And I think Jan suggested a
> per-arch way to do it. So why is it here?
No, I specifically did not. I intended this to be universal, but then I
wasn't really aware that on ARM the EFI loader is so much different
from x86's.
Before coming to a final conclusion I'd really like to understand how
you would see dynamic memory allocation to work for pieces of data
to be communicated from EFI loader to main Xen. That'll determine
whether I'll have to grumblingly accept this code to be x86-specific.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH RFC v7 07/14] efi: create new early memory allocator
2016-09-26 6:53 ` Jan Beulich
@ 2016-09-26 20:01 ` Julien Grall
2016-09-27 8:06 ` Jan Beulich
0 siblings, 1 reply; 51+ messages in thread
From: Julien Grall @ 2016-09-26 20:01 UTC (permalink / raw)
To: Jan Beulich
Cc: Juergen Gross, sstabellini, andrew.cooper3, Daniel Kiper, cardoe,
pgnet.dev, ning.sun, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
Hi Jan,
On 25/09/2016 23:53, Jan Beulich wrote:
>>>> On 24.09.16 at 01:35, <julien.grall@arm.com> wrote:
>> Hi Daniel,
>>
>> On 23/09/2016 22:47, Daniel Kiper wrote:
>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>>> index 38eb888..2085f35 100644
>>> --- a/xen/arch/arm/setup.c
>>> +++ b/xen/arch/arm/setup.c
>>> @@ -38,6 +38,7 @@
>>> #include <xen/vmap.h>
>>> #include <xen/libfdt/libfdt.h>
>>> #include <xen/acpi.h>
>>> +#include <xen/efi.h>
>>> #include <asm/alternative.h>
>>> #include <asm/page.h>
>>> #include <asm/current.h>
>>> @@ -66,6 +67,7 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes);
>>>
>>> static __used void init_done(void)
>>> {
>>> + free_ebmalloc_unused_mem();
>>
>> I said no to this on the previous version. And I think Jan suggested a
>> per-arch way to do it. So why is it here?
>
> No, I specifically did not. I intended this to be universal, but then I
> wasn't really aware that on ARM the EFI loader is so much different
> from x86's.
>
> Before coming to a final conclusion I'd really like to understand how
> you would see dynamic memory allocation to work for pieces of data
> to be communicated from EFI loader to main Xen. That'll determine
> whether I'll have to grumblingly accept this code to be x86-specific.
In the current state, all the communication from EFI loader to main Xen
should be done via the device-tree or the data have to be in init
section (bss is zeroed when leaving the EFI stub and before entering to
Xen).
I am not against changing this behavior, however in the current state
this will not work at all. The call to the function will be misleading,
hence why I suggested a TODO for the time-being (for now the code is
only compiled on x86, anyway).
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH RFC v7 07/14] efi: create new early memory allocator
2016-09-26 20:01 ` Julien Grall
@ 2016-09-27 8:06 ` Jan Beulich
2016-09-27 23:23 ` Julien Grall
0 siblings, 1 reply; 51+ messages in thread
From: Jan Beulich @ 2016-09-27 8:06 UTC (permalink / raw)
To: Julien Grall
Cc: Juergen Gross, sstabellini, andrew.cooper3, Daniel Kiper, cardoe,
pgnet.dev, ning.sun, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
>>> On 26.09.16 at 22:01, <julien.grall@arm.com> wrote:
> On 25/09/2016 23:53, Jan Beulich wrote:
>>>>> On 24.09.16 at 01:35, <julien.grall@arm.com> wrote:
>>> On 23/09/2016 22:47, Daniel Kiper wrote:
>>>> @@ -66,6 +67,7 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes);
>>>>
>>>> static __used void init_done(void)
>>>> {
>>>> + free_ebmalloc_unused_mem();
>>>
>>> I said no to this on the previous version. And I think Jan suggested a
>>> per-arch way to do it. So why is it here?
>>
>> No, I specifically did not. I intended this to be universal, but then I
>> wasn't really aware that on ARM the EFI loader is so much different
>> from x86's.
>>
>> Before coming to a final conclusion I'd really like to understand how
>> you would see dynamic memory allocation to work for pieces of data
>> to be communicated from EFI loader to main Xen. That'll determine
>> whether I'll have to grumblingly accept this code to be x86-specific.
>
> In the current state, all the communication from EFI loader to main Xen
> should be done via the device-tree or the data have to be in init
> section (bss is zeroed when leaving the EFI stub and before entering to
> Xen).
This late .bss zeroing is something which, as Daniel had already
suggested, could (and imo should) be avoided (just like he already
needs to do for x86 in his series).
> I am not against changing this behavior, however in the current state
> this will not work at all. The call to the function will be misleading,
> hence why I suggested a TODO for the time-being (for now the code is
> only compiled on x86, anyway).
This doesn't really help make progress with the patch here. The
question isn't so much what current behavior should be, but what
sane behavior would be going forward. Again - we're needing your
input mainly to decide whether to put this allocator in common or
x86-specific code (with the goal of not having to move it later if at
all possible).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH RFC v7 07/14] efi: create new early memory allocator
2016-09-27 8:06 ` Jan Beulich
@ 2016-09-27 23:23 ` Julien Grall
0 siblings, 0 replies; 51+ messages in thread
From: Julien Grall @ 2016-09-27 23:23 UTC (permalink / raw)
To: Jan Beulich
Cc: Juergen Gross, sstabellini, andrew.cooper3, Daniel Kiper, cardoe,
pgnet.dev, ning.sun, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
Hi Jan,
On 27/09/2016 01:06, Jan Beulich wrote:
>>>> On 26.09.16 at 22:01, <julien.grall@arm.com> wrote:
>> On 25/09/2016 23:53, Jan Beulich wrote:
>>>>>> On 24.09.16 at 01:35, <julien.grall@arm.com> wrote:
>>>> On 23/09/2016 22:47, Daniel Kiper wrote:
>>>>> @@ -66,6 +67,7 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes);
>>>>>
>>>>> static __used void init_done(void)
>>>>> {
>>>>> + free_ebmalloc_unused_mem();
>>>>
>>>> I said no to this on the previous version. And I think Jan suggested a
>>>> per-arch way to do it. So why is it here?
>>>
>>> No, I specifically did not. I intended this to be universal, but then I
>>> wasn't really aware that on ARM the EFI loader is so much different
>>> from x86's.
>>>
>>> Before coming to a final conclusion I'd really like to understand how
>>> you would see dynamic memory allocation to work for pieces of data
>>> to be communicated from EFI loader to main Xen. That'll determine
>>> whether I'll have to grumblingly accept this code to be x86-specific.
>>
>> In the current state, all the communication from EFI loader to main Xen
>> should be done via the device-tree or the data have to be in init
>> section (bss is zeroed when leaving the EFI stub and before entering to
>> Xen).
>
> This late .bss zeroing is something which, as Daniel had already
> suggested, could (and imo should) be avoided (just like he already
> needs to do for x86 in his series).
I am happy to see a such patch for ARM.
>
>> I am not against changing this behavior, however in the current state
>> this will not work at all. The call to the function will be misleading,
>> hence why I suggested a TODO for the time-being (for now the code is
>> only compiled on x86, anyway).
>
> This doesn't really help make progress with the patch here. The
> question isn't so much what current behavior should be, but what
> sane behavior would be going forward. Again - we're needing your
> input mainly to decide whether to put this allocator in common or
> x86-specific code (with the goal of not having to move it later if at
> all possible).
Unifying the behavior between x86 and ARM would be the best.
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH RFC v7 07/14] efi: create new early memory allocator
2016-09-23 21:47 ` [PATCH RFC v7 07/14] efi: create new early memory allocator Daniel Kiper
2016-09-23 23:35 ` Julien Grall
@ 2016-09-26 13:37 ` Jan Beulich
2016-09-27 17:49 ` Daniel Kiper
1 sibling, 1 reply; 51+ messages in thread
From: Jan Beulich @ 2016-09-26 13:37 UTC (permalink / raw)
To: Daniel Kiper
Cc: Juergen Gross, sstabellini, andrew.cooper3, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
>>> On 23.09.16 at 23:47, <daniel.kiper@oracle.com> wrote:
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -79,6 +79,10 @@ static size_t wstrlen(const CHAR16 * s);
> static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz);
> static bool_t match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2);
>
> +#ifndef CONFIG_ARM
> +static void *ebmalloc(size_t size);
> +#endif
Leaving aside the ARM aspect (to be clarified by Julien), is there a
reason you need to forward declare this here, rather than moving
the whole addition from further down up immediately ahead of the
inclusion point of efi-boot.h?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH RFC v7 07/14] efi: create new early memory allocator
2016-09-26 13:37 ` Jan Beulich
@ 2016-09-27 17:49 ` Daniel Kiper
2016-09-28 8:48 ` Jan Beulich
0 siblings, 1 reply; 51+ messages in thread
From: Daniel Kiper @ 2016-09-27 17:49 UTC (permalink / raw)
To: Jan Beulich
Cc: Juergen Gross, sstabellini, andrew.cooper3, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
On Mon, Sep 26, 2016 at 07:37:45AM -0600, Jan Beulich wrote:
> >>> On 23.09.16 at 23:47, <daniel.kiper@oracle.com> wrote:
> > --- a/xen/common/efi/boot.c
> > +++ b/xen/common/efi/boot.c
> > @@ -79,6 +79,10 @@ static size_t wstrlen(const CHAR16 * s);
> > static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz);
> > static bool_t match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2);
> >
> > +#ifndef CONFIG_ARM
> > +static void *ebmalloc(size_t size);
> > +#endif
>
> Leaving aside the ARM aspect (to be clarified by Julien), is there a
> reason you need to forward declare this here, rather than moving
> the whole addition from further down up immediately ahead of the
> inclusion point of efi-boot.h?
If you wish I can move ebmalloc code before efi-boot.h inclusion.
There is a pretty good chance that it will work here.
Daniel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH RFC v7 07/14] efi: create new early memory allocator
2016-09-27 17:49 ` Daniel Kiper
@ 2016-09-28 8:48 ` Jan Beulich
0 siblings, 0 replies; 51+ messages in thread
From: Jan Beulich @ 2016-09-28 8:48 UTC (permalink / raw)
To: Daniel Kiper
Cc: Juergen Gross, sstabellini, andrew.cooper3, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
>>> On 27.09.16 at 19:49, <daniel.kiper@oracle.com> wrote:
> On Mon, Sep 26, 2016 at 07:37:45AM -0600, Jan Beulich wrote:
>> >>> On 23.09.16 at 23:47, <daniel.kiper@oracle.com> wrote:
>> > --- a/xen/common/efi/boot.c
>> > +++ b/xen/common/efi/boot.c
>> > @@ -79,6 +79,10 @@ static size_t wstrlen(const CHAR16 * s);
>> > static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz);
>> > static bool_t match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2);
>> >
>> > +#ifndef CONFIG_ARM
>> > +static void *ebmalloc(size_t size);
>> > +#endif
>>
>> Leaving aside the ARM aspect (to be clarified by Julien), is there a
>> reason you need to forward declare this here, rather than moving
>> the whole addition from further down up immediately ahead of the
>> inclusion point of efi-boot.h?
>
> If you wish I can move ebmalloc code before efi-boot.h inclusion.
> There is a pretty good chance that it will work here.
Yes please, the more that Julien now confirmed that he's fine
with the code staying in common/efi/boot.c.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms
2016-09-23 21:47 [PATCH v7 00/14] x86: multiboot2 protocol support Daniel Kiper
` (6 preceding siblings ...)
2016-09-23 21:47 ` [PATCH RFC v7 07/14] efi: create new early memory allocator Daniel Kiper
@ 2016-09-23 21:47 ` Daniel Kiper
2016-09-26 13:47 ` Jan Beulich
2016-09-26 14:19 ` Andrew Cooper
2016-09-23 21:47 ` [PATCH v7 09/14] x86/boot: implement early command line parser in C Daniel Kiper
` (5 subsequent siblings)
13 siblings, 2 replies; 51+ messages in thread
From: Daniel Kiper @ 2016-09-23 21:47 UTC (permalink / raw)
To: xen-devel
Cc: jgross, sstabellini, andrew.cooper3, cardoe, pgnet.dev, ning.sun,
julien.grall, david.vrabel, jbeulich, qiaowei.ren, gang.wei,
fu.wei
This way Xen can be loaded on EFI platforms using GRUB2 and
other boot loaders which support multiboot2 protocol.
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
v7 - suggestions/fixes:
- do not allocate twice memory for trampoline if we were
loaded via multiboot2 protocol on EFI platform,
- wrap long line
(suggested by Jan Beulich),
- improve comments
(suggested by Jan Beulich).
v6 - suggestions/fixes:
- improve label names in assembly
error printing code
(suggested by Jan Beulich),
- improve comments
(suggested by Jan Beulich),
- various minor cleanups and fixes
(suggested by Jan Beulich).
v4 - suggestions/fixes:
- remove redundant BSS alignment,
- update BSS alignment check,
- use __set_bit() instead of set_bit() if possible
(suggested by Jan Beulich),
- call efi_arch_cpu() from efi_multiboot2()
even if the same work is done later in
other place right now
(suggested by Jan Beulich),
- xen/arch/x86/efi/stub.c:efi_multiboot2()
fail properly on EFI platforms,
- do not read data beyond the end of multiboot2
information in xen/arch/x86/boot/head.S
(suggested by Jan Beulich),
- use 32-bit registers in x86_64 code if possible
(suggested by Jan Beulich),
- multiboot2 information address is 64-bit
in x86_64 code, so, treat it is as is
(suggested by Jan Beulich),
- use cmovcc if possible,
- leave only one space between rep and stosq
(suggested by Jan Beulich),
- improve error handling,
- improve early error messages,
(suggested by Jan Beulich),
- improve early error messages printing code,
- improve label names
(suggested by Jan Beulich),
- improve comments
(suggested by Jan Beulich),
- various minor cleanups.
v3 - suggestions/fixes:
- take into account alignment when skipping multiboot2 fixed part
(suggested by Konrad Rzeszutek Wilk),
- improve segment registers initialization
(suggested by Jan Beulich),
- improve comments
(suggested by Jan Beulich and Konrad Rzeszutek Wilk),
- improve commit message
(suggested by Jan Beulich).
v2 - suggestions/fixes:
- generate multiboot2 header using macros
(suggested by Jan Beulich),
- switch CPU to x86_32 mode before
jumping to 32-bit code
(suggested by Andrew Cooper),
- reduce code changes to increase patch readability
(suggested by Jan Beulich),
- improve comments
(suggested by Jan Beulich),
- ignore MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO tag on EFI platform
and find on my own multiboot2.mem_lower value,
- stop execution if EFI platform is detected
in legacy BIOS path.
---
xen/arch/x86/boot/head.S | 258 ++++++++++++++++++++++++++++++++++---
xen/arch/x86/efi/efi-boot.h | 49 ++++++-
xen/arch/x86/efi/stub.c | 38 ++++++
xen/arch/x86/x86_64/asm-offsets.c | 2 +
xen/arch/x86/xen.lds.S | 4 +-
xen/common/efi/boot.c | 11 ++
6 files changed, 340 insertions(+), 22 deletions(-)
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 383ff79..a371cde 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -89,6 +89,13 @@ multiboot2_header_start:
0, /* Number of the lines - no preference. */ \
0 /* Number of bits per pixel - no preference. */
+ /* Inhibit bootloader from calling ExitBootServices(). */
+ mb2ht_init MB2_HT(EFI_BS), MB2_HT(OPTIONAL)
+
+ /* EFI64 entry point. */
+ mb2ht_init MB2_HT(ENTRY_ADDRESS_EFI64), MB2_HT(OPTIONAL), \
+ sym_phys(__efi64_start)
+
/* Multiboot2 header end tag. */
mb2ht_init MB2_HT(END), MB2_HT(REQUIRED)
.Lmultiboot2_header_end:
@@ -100,20 +107,49 @@ multiboot2_header_start:
gdt_boot_descr:
.word 6*8-1
.long sym_phys(trampoline_gdt)
+ .long 0 /* Needed for 64-bit lgdt */
+
+cs32_switch_addr:
+ .long sym_phys(cs32_switch)
+ .word BOOT_CS32
+
+ .align 4
+vga_text_buffer:
+ .long 0xb8000
.Lbad_cpu_msg: .asciz "ERR: Not a 64-bit CPU!"
.Lbad_ldr_msg: .asciz "ERR: Not a Multiboot bootloader!"
+.Lbad_ldr_nbs: .asciz "ERR: Bootloader shutdown EFI x64 boot services!"
+.Lbad_ldr_nst: .asciz "ERR: EFI SystemTable is not provided by bootloader!"
+.Lbad_ldr_nih: .asciz "ERR: EFI ImageHandle is not provided by bootloader!"
+.Lbad_efi_msg: .asciz "ERR: EFI IA-32 platforms are not supported!"
.section .init.text, "ax", @progbits
bad_cpu:
mov $(sym_phys(.Lbad_cpu_msg)),%esi # Error message
- jmp print_err
+ jmp .Lget_vtb
not_multiboot:
mov $(sym_phys(.Lbad_ldr_msg)),%esi # Error message
-print_err:
- mov $0xB8000,%edi # VGA framebuffer
-1: mov (%esi),%bl
+ jmp .Lget_vtb
+.Lmb2_no_st:
+ mov $(sym_phys(.Lbad_ldr_nst)),%esi # Error message
+ jmp .Lget_vtb
+.Lmb2_no_ih:
+ mov $(sym_phys(.Lbad_ldr_nih)),%esi # Error message
+ jmp .Lget_vtb
+.Lmb2_no_bs:
+ mov $(sym_phys(.Lbad_ldr_nbs)),%esi # Error message
+ xor %edi,%edi # No VGA text buffer
+ jmp .Lsend_chr
+.Lmb2_efi_ia_32:
+ mov $(sym_phys(.Lbad_efi_msg)),%esi # Error message
+ xor %edi,%edi # No VGA text buffer
+ jmp .Lsend_chr
+.Lget_vtb:
+ mov sym_phys(vga_text_buffer),%edi
+.Lsend_chr:
+ mov (%esi),%bl
test %bl,%bl # Terminate on '\0' sentinel
je .Lhalt
mov $0x3f8+5,%dx # UART Line Status Register
@@ -123,13 +159,181 @@ print_err:
mov $0x3f8+0,%dx # UART Transmit Holding Register
mov %bl,%al
out %al,%dx # Send a character over the serial line
- movsb # Write a character to the VGA framebuffer
+ test %edi,%edi # Is the VGA text buffer available?
+ jz .Lsend_chr
+ movsb # Write a character to the VGA text buffer
mov $7,%al
- stosb # Write an attribute to the VGA framebuffer
- jmp 1b
+ stosb # Write an attribute to the VGA text buffer
+ jmp .Lsend_chr
.Lhalt: hlt
jmp .Lhalt
+ .code64
+
+__efi64_start:
+ cld
+
+ /* VGA is not available on EFI platforms. */
+ movl $0,vga_text_buffer(%rip)
+
+ /* Check for Multiboot2 bootloader. */
+ cmp $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
+ je .Lefi_multiboot2_proto
+
+ /* Jump to not_multiboot after switching CPU to x86_32 mode. */
+ lea not_multiboot(%rip),%edi
+ jmp x86_32_switch
+
+.Lefi_multiboot2_proto:
+ /* Zero EFI SystemTable and EFI ImageHandle addresses. */
+ xor %esi,%esi
+ xor %edi,%edi
+
+ /* Skip Multiboot2 information fixed part. */
+ lea (MB2_fixed_sizeof+MULTIBOOT2_TAG_ALIGN-1)(%rbx),%ecx
+ and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
+
+0:
+ /* Check Multiboot2 information total size. */
+ mov %ecx,%r8d
+ sub %ebx,%r8d
+ cmp %r8d,MB2_fixed_total_size(%rbx)
+ jbe run_bs
+
+ /* Are EFI boot services available? */
+ cmpl $MULTIBOOT2_TAG_TYPE_EFI_BS,MB2_tag_type(%rcx)
+ jne 1f
+
+ /*
+ * Yes, store that info in skip_realmode variable. I agree that
+ * its name is a bit unfortunate in this context, however, by the
+ * way we disable real mode and other legacy stuff which should
+ * not be run on EFI platforms.
+ */
+ incb skip_realmode(%rip)
+ jmp 9f
+
+1:
+ /* Get EFI SystemTable address from Multiboot2 information. */
+ cmpl $MULTIBOOT2_TAG_TYPE_EFI64,MB2_tag_type(%rcx)
+ cmove MB2_efi64_st(%rcx),%rsi
+ je 9f
+
+ /* Get EFI ImageHandle address from Multiboot2 information. */
+ cmpl $MULTIBOOT2_TAG_TYPE_EFI64_IH,MB2_tag_type(%rcx)
+ cmove MB2_efi64_ih(%rcx),%rdi
+ je 9f
+
+ /* Is it the end of Multiboot2 information? */
+ cmpl $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx)
+ je run_bs
+
+9:
+ /* Go to next Multiboot2 information tag. */
+ add MB2_tag_size(%rcx),%ecx
+ add $(MULTIBOOT2_TAG_ALIGN-1),%ecx
+ and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
+ jmp 0b
+
+run_bs:
+ /* Are EFI boot services available? */
+ cmpb $0,skip_realmode(%rip)
+ jnz 0f
+
+ /* Jump to .Lmb2_no_bs after switching CPU to x86_32 mode. */
+ lea .Lmb2_no_bs(%rip),%edi
+ jmp x86_32_switch
+
+0:
+ /* Is EFI SystemTable address provided by boot loader? */
+ test %rsi,%rsi
+ jnz 1f
+
+ /* Jump to .Lmb2_no_st after switching CPU to x86_32 mode. */
+ lea .Lmb2_no_st(%rip),%edi
+ jmp x86_32_switch
+
+1:
+ /* Is EFI ImageHandle address provided by boot loader? */
+ test %rdi,%rdi
+ jnz 2f
+
+ /* Jump to .Lmb2_no_ih after switching CPU to x86_32 mode. */
+ lea .Lmb2_no_ih(%rip),%edi
+ jmp x86_32_switch
+
+2:
+ push %rax
+ push %rdi
+
+ /*
+ * Initialize BSS (no nasty surprises!).
+ * It must be done earlier than in BIOS case
+ * because efi_multiboot2() touches it.
+ */
+ lea .startof.(.bss)(%rip),%edi
+ mov $.sizeof.(.bss),%ecx
+ shr $3,%ecx
+ xor %eax,%eax
+ rep stosq
+
+ pop %rdi
+
+ /*
+ * efi_multiboot2() is called according to System V AMD64 ABI:
+ * - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable,
+ * - OUT: %rax - highest usable memory address below 1 MiB;
+ * memory above this address is reserved for trampoline;
+ * memory below this address is used for stack and as
+ * a storage for boot data.
+ *
+ * MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO tag is not provided
+ * on EFI platforms. Hence, it could not be used like
+ * on legacy BIOS platforms.
+ */
+ call efi_multiboot2
+
+ /* Convert memory address to bytes/16 and store it in safe place. */
+ shr $4,%eax
+ mov %eax,%ecx
+
+ pop %rax
+
+ /* Jump to trampoline_setup after switching CPU to x86_32 mode. */
+ lea trampoline_setup(%rip),%edi
+
+x86_32_switch:
+ cli
+
+ /* Initialize GDTR. */
+ lgdt gdt_boot_descr(%rip)
+
+ /* Reload code selector. */
+ ljmpl *cs32_switch_addr(%rip)
+
+ .code32
+
+cs32_switch:
+ /* Initialize basic data segments. */
+ mov $BOOT_DS,%edx
+ mov %edx,%ds
+ mov %edx,%es
+ mov %edx,%ss
+ /* %esp is initialized later. */
+
+ /* Load null descriptor to unused segment registers. */
+ xor %edx,%edx
+ mov %edx,%fs
+ mov %edx,%gs
+
+ /* Disable paging. */
+ mov %cr0,%edx
+ and $(~X86_CR0_PG),%edx
+ mov %edx,%cr0
+
+ /* Jump to earlier loaded address. */
+ jmp *%edi
+
__start:
cld
cli
@@ -157,7 +361,7 @@ __start:
/* Not available? BDA value will be fine. */
cmovnz MB_mem_lower(%ebx),%edx
- jmp trampoline_setup
+ jmp trampoline_bios_setup
.Lmultiboot2_proto:
/* Skip Multiboot2 information fixed part. */
@@ -169,16 +373,24 @@ __start:
mov %ecx,%edi
sub %ebx,%edi
cmp %edi,MB2_fixed_total_size(%ebx)
- jbe trampoline_setup
+ jbe trampoline_bios_setup
/* Get mem_lower from Multiboot2 information. */
cmpl $MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,MB2_tag_type(%ecx)
cmove MB2_mem_lower(%ecx),%edx
- je trampoline_setup
+ je trampoline_bios_setup
+
+ /* EFI IA-32 platforms are not supported. */
+ cmpl $MULTIBOOT2_TAG_TYPE_EFI32,MB2_tag_type(%ecx)
+ je .Lmb2_efi_ia_32
+
+ /* Bootloader shutdown EFI x64 boot services. */
+ cmpl $MULTIBOOT2_TAG_TYPE_EFI64,MB2_tag_type(%ecx)
+ je .Lmb2_no_bs
/* Is it the end of Multiboot2 information? */
cmpl $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%ecx)
- je trampoline_setup
+ je trampoline_bios_setup
/* Go to next Multiboot2 information tag. */
add MB2_tag_size(%ecx),%ecx
@@ -186,7 +398,7 @@ __start:
and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
jmp 0b
-trampoline_setup:
+trampoline_bios_setup:
/* Set up trampoline segment 64k below EBDA */
movzwl 0x40e,%ecx /* EBDA segment */
cmp $0xa000,%ecx /* sanity check (high) */
@@ -202,16 +414,19 @@ trampoline_setup:
* multiboot structure (if available) and use the smallest.
*/
cmp $0x100,%edx /* is the multiboot value too small? */
- jb 2f /* if so, do not use it */
+ jb trampoline_setup /* if so, do not use it */
shl $10-4,%edx
cmp %ecx,%edx /* compare with BDA value */
cmovb %edx,%ecx /* and use the smaller */
-2: /* Reserve 64kb for the trampoline */
+ /* Reserve 64kb for the trampoline. */
sub $0x1000,%ecx
/* From arch/x86/smpboot.c: start_eip had better be page-aligned! */
xor %cl, %cl
+
+trampoline_setup:
+ /* Save trampoline address for later use. */
shl $4, %ecx
mov %ecx,sym_phys(trampoline_phys)
@@ -223,7 +438,14 @@ trampoline_setup:
call reloc
mov %eax,sym_phys(multiboot_ptr)
- /* Initialize BSS (no nasty surprises!) */
+ /*
+ * Do not zero BSS on EFI platform here.
+ * It was initialized earlier.
+ */
+ cmpb $0,sym_phys(skip_realmode)
+ jnz 1f
+
+ /* Initialize BSS (no nasty surprises!). */
mov $sym_phys(__bss_start),%edi
mov $sym_phys(__bss_end),%ecx
sub %edi,%ecx
@@ -231,6 +453,7 @@ trampoline_setup:
shr $2,%ecx
rep stosl
+1:
/* Interrogate CPU extended features via CPUID. */
mov $0x80000000,%eax
cpuid
@@ -282,8 +505,13 @@ trampoline_setup:
cmp $sym_phys(__trampoline_seg_stop),%edi
jb 1b
+ /* Do not parse command line on EFI platform here. */
+ cmpb $0,sym_phys(skip_realmode)
+ jnz 1f
+
call cmdline_parse_early
+1:
/* Switch to low-memory stack. */
mov sym_phys(trampoline_phys),%edi
lea 0x10000(%edi),%esp
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 62c010e..284cf97 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -210,12 +210,14 @@ static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size)
static void __init efi_arch_pre_exit_boot(void)
{
- if ( !trampoline_phys )
- {
- if ( !cfg.addr )
- blexit(L"No memory for trampoline");
+ if ( trampoline_phys )
+ return;
+
+ if ( !cfg.addr )
+ blexit(L"No memory for trampoline");
+
+ if ( efi_enabled(EFI_LOADER) )
relocate_trampoline(cfg.addr);
- }
}
static void __init noreturn efi_arch_post_exit_boot(void)
@@ -653,6 +655,43 @@ static bool_t __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable)
static void efi_arch_flush_dcache_area(const void *vaddr, UINTN size) { }
+paddr_t __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
+{
+ EFI_GRAPHICS_OUTPUT_PROTOCOL *gop;
+ UINTN cols, gop_mode = ~0, rows;
+
+ __set_bit(EFI_BOOT, &efi_flags);
+ __set_bit(EFI_RS, &efi_flags);
+
+ efi_init(ImageHandle, SystemTable);
+
+ efi_console_set_mode();
+
+ if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode,
+ &cols, &rows) == EFI_SUCCESS )
+ efi_arch_console_init(cols, rows);
+
+ gop = efi_get_gop();
+
+ if ( gop )
+ gop_mode = efi_find_gop_mode(gop, 0, 0, 0);
+
+ efi_arch_edd();
+ efi_arch_cpu();
+
+ efi_tables();
+ setup_efi_pci();
+ efi_variables();
+
+ if ( gop )
+ efi_set_gop_mode(gop, gop_mode);
+
+ efi_exit_boot(ImageHandle, SystemTable);
+
+ /* Return highest usable memory address below 1 MiB. */
+ return cfg.addr;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.c
index bc9919c..232dd12 100644
--- a/xen/arch/x86/efi/stub.c
+++ b/xen/arch/x86/efi/stub.c
@@ -3,6 +3,44 @@
#include <xen/init.h>
#include <xen/lib.h>
#include <asm/page.h>
+#include <asm/efibind.h>
+#include <efi/efidef.h>
+#include <efi/eficapsule.h>
+#include <efi/eficon.h>
+#include <efi/efidevp.h>
+#include <efi/efiapi.h>
+
+/*
+ * Here we are in EFI stub. EFI calls are not supported due to lack
+ * of relevant functionality in compiler and/or linker.
+ *
+ * efi_multiboot2() is an exception. Please look below for more details.
+ */
+
+paddr_t __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle,
+ EFI_SYSTEM_TABLE *SystemTable)
+{
+ CHAR16 *err = L"Xen does not have EFI code build in!!!\r\nSystem halted!!!\r\n";
+ SIMPLE_TEXT_OUTPUT_INTERFACE *StdErr;
+
+ StdErr = SystemTable->StdErr ? SystemTable->StdErr : SystemTable->ConOut;
+
+ /*
+ * Print error message and halt the system.
+ *
+ * We have to open code MS x64 calling convention
+ * in assembly because here this convention may
+ * not be directly supported by C compiler.
+ */
+ asm volatile(
+ " call %2 \n"
+ "0: hlt \n"
+ " jmp 0b \n"
+ : "+c" (StdErr), "+d" (err) : "g" (StdErr->OutputString)
+ : "rax", "r8", "r9", "r10", "r11", "memory");
+
+ unreachable();
+}
bool efi_enabled(unsigned int feature)
{
diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c
index b437a8f..2a22659 100644
--- a/xen/arch/x86/x86_64/asm-offsets.c
+++ b/xen/arch/x86/x86_64/asm-offsets.c
@@ -175,6 +175,8 @@ void __dummy__(void)
OFFSET(MB2_tag_type, multiboot2_tag_t, type);
OFFSET(MB2_tag_size, multiboot2_tag_t, size);
OFFSET(MB2_mem_lower, multiboot2_tag_basic_meminfo_t, mem_lower);
+ OFFSET(MB2_efi64_st, multiboot2_tag_efi64_t, pointer);
+ OFFSET(MB2_efi64_ih, multiboot2_tag_efi64_ih_t, pointer);
BLANK();
OFFSET(DOMAIN_vm_assist, struct domain, vm_assist);
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 38baea3..f4b4e99 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -334,5 +334,5 @@ ASSERT(IS_ALIGNED(__init_end, PAGE_SIZE), "__init_end misaligned")
ASSERT(IS_ALIGNED(trampoline_start, 4), "trampoline_start misaligned")
ASSERT(IS_ALIGNED(trampoline_end, 4), "trampoline_end misaligned")
-ASSERT(IS_ALIGNED(__bss_start, 4), "__bss_start misaligned")
-ASSERT(IS_ALIGNED(__bss_end, 4), "__bss_end misaligned")
+ASSERT(IS_ALIGNED(__bss_start, 8), "__bss_start misaligned")
+ASSERT(IS_ALIGNED(__bss_end, 8), "__bss_end misaligned")
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 91df9b5..7e19f8a 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -83,6 +83,17 @@ static bool_t match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2);
static void *ebmalloc(size_t size);
#endif
+static void efi_init(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable);
+static void efi_console_set_mode(void);
+static EFI_GRAPHICS_OUTPUT_PROTOCOL *efi_get_gop(void);
+static UINTN efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
+ UINTN cols, UINTN rows, UINTN depth);
+static void efi_tables(void);
+static void setup_efi_pci(void);
+static void efi_variables(void);
+static void efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN gop_mode);
+static void efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable);
+
static const EFI_BOOT_SERVICES *__initdata efi_bs;
static UINT32 __initdata efi_bs_revision;
static EFI_HANDLE __initdata efi_ih;
--
1.7.10.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms
2016-09-23 21:47 ` [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms Daniel Kiper
@ 2016-09-26 13:47 ` Jan Beulich
2016-09-27 18:11 ` Daniel Kiper
2016-09-26 14:19 ` Andrew Cooper
1 sibling, 1 reply; 51+ messages in thread
From: Jan Beulich @ 2016-09-26 13:47 UTC (permalink / raw)
To: Daniel Kiper
Cc: Juergen Gross, sstabellini, andrew.cooper3, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
>>> On 23.09.16 at 23:47, <daniel.kiper@oracle.com> wrote:
> This way Xen can be loaded on EFI platforms using GRUB2 and
> other boot loaders which support multiboot2 protocol.
>
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> ---
> v7 - suggestions/fixes:
> - do not allocate twice memory for trampoline if we were
> loaded via multiboot2 protocol on EFI platform,
If you fix bugs not noticed by a reviewer but by yourself, please
drop all acks/R-b-s covering the code in question. And then I'm
afraid I haven't even been able to locate that change - could you
please point out what you did where?
> + /*
> + * Initialize BSS (no nasty surprises!).
> + * It must be done earlier than in BIOS case
> + * because efi_multiboot2() touches it.
> + */
> + lea .startof.(.bss)(%rip),%edi
> + mov $.sizeof.(.bss),%ecx
> + shr $3,%ecx
> + xor %eax,%eax
> + rep stosq
> +
> + pop %rdi
> +
> + /*
> + * efi_multiboot2() is called according to System V AMD64 ABI:
> + * - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable,
> + * - OUT: %rax - highest usable memory address below 1 MiB;
> + * memory above this address is reserved for trampoline;
> + * memory below this address is used for stack and as
> + * a storage for boot data.
How can you validly use memory below this address? (And I'd like to
note that this also changed from v6, and the change to comments
listed in the v7 section and supposedly suggested by me can't cover
this, as I don't recall having asked for such an adjustment.)
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms
2016-09-26 13:47 ` Jan Beulich
@ 2016-09-27 18:11 ` Daniel Kiper
2016-09-28 8:57 ` Jan Beulich
0 siblings, 1 reply; 51+ messages in thread
From: Daniel Kiper @ 2016-09-27 18:11 UTC (permalink / raw)
To: Jan Beulich
Cc: Juergen Gross, sstabellini, andrew.cooper3, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
On Mon, Sep 26, 2016 at 07:47:42AM -0600, Jan Beulich wrote:
> >>> On 23.09.16 at 23:47, <daniel.kiper@oracle.com> wrote:
> > This way Xen can be loaded on EFI platforms using GRUB2 and
> > other boot loaders which support multiboot2 protocol.
> >
> > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> > Acked-by: Jan Beulich <jbeulich@suse.com>
> > ---
> > v7 - suggestions/fixes:
> > - do not allocate twice memory for trampoline if we were
> > loaded via multiboot2 protocol on EFI platform,
>
> If you fix bugs not noticed by a reviewer but by yourself, please
> drop all acks/R-b-s covering the code in question. And then I'm
OK.
> afraid I haven't even been able to locate that change - could you
> please point out what you did where?
The change is very subtle. I add trampoline_setup label behind
/* From arch/x86/smpboot.c: start_eip had better be page-aligned! */
xor %cl, %cl
instead of
cmp %ecx,%edx /* compare with BDA value */
cmovb %edx,%ecx /* and use the smaller */
> > + /*
> > + * Initialize BSS (no nasty surprises!).
> > + * It must be done earlier than in BIOS case
> > + * because efi_multiboot2() touches it.
> > + */
> > + lea .startof.(.bss)(%rip),%edi
> > + mov $.sizeof.(.bss),%ecx
> > + shr $3,%ecx
> > + xor %eax,%eax
> > + rep stosq
> > +
> > + pop %rdi
> > +
> > + /*
> > + * efi_multiboot2() is called according to System V AMD64 ABI:
> > + * - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable,
> > + * - OUT: %rax - highest usable memory address below 1 MiB;
> > + * memory above this address is reserved for trampoline;
> > + * memory below this address is used for stack and as
> > + * a storage for boot data.
>
> How can you validly use memory below this address? (And I'd like to
Right, I should not do that blindly. As quick fix we can check in efi_arch_process_memory_map()
that chosen memory region has size cfg.size plus let's say 64 KiB. Is it sufficient
for you? However, I think that later (for 4.9?) we should consider what we discussed
here https://lists.xen.org/archives/html/xen-devel/2016-09/msg01424.html
> note that this also changed from v6, and the change to comments
> listed in the v7 section and supposedly suggested by me can't cover
> this, as I don't recall having asked for such an adjustment.)
Ah, sorry about that. I should be more precise.
Daniel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms
2016-09-27 18:11 ` Daniel Kiper
@ 2016-09-28 8:57 ` Jan Beulich
2016-09-28 9:39 ` Daniel Kiper
0 siblings, 1 reply; 51+ messages in thread
From: Jan Beulich @ 2016-09-28 8:57 UTC (permalink / raw)
To: Daniel Kiper
Cc: Juergen Gross, sstabellini, andrew.cooper3, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
>>> On 27.09.16 at 20:11, <daniel.kiper@oracle.com> wrote:
> On Mon, Sep 26, 2016 at 07:47:42AM -0600, Jan Beulich wrote:
>> >>> On 23.09.16 at 23:47, <daniel.kiper@oracle.com> wrote:
>> > This way Xen can be loaded on EFI platforms using GRUB2 and
>> > other boot loaders which support multiboot2 protocol.
>> >
>> > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
>> > Acked-by: Jan Beulich <jbeulich@suse.com>
>> > ---
>> > v7 - suggestions/fixes:
>> > - do not allocate twice memory for trampoline if we were
>> > loaded via multiboot2 protocol on EFI platform,
>>
>> If you fix bugs not noticed by a reviewer but by yourself, please
>> drop all acks/R-b-s covering the code in question. And then I'm
>
> OK.
>
>> afraid I haven't even been able to locate that change - could you
>> please point out what you did where?
>
> The change is very subtle. I add trampoline_setup label behind
>
> /* From arch/x86/smpboot.c: start_eip had better be page-aligned! */
> xor %cl, %cl
>
> instead of
>
> cmp %ecx,%edx /* compare with BDA value */
> cmovb %edx,%ecx /* and use the smaller */
So how did you expect anyone having looked at previous version
to spot that? Please, in your changes section, rather than
tediously listing who suggested which change, please make that
section useful for incremental reviews.
>> > + /*
>> > + * Initialize BSS (no nasty surprises!).
>> > + * It must be done earlier than in BIOS case
>> > + * because efi_multiboot2() touches it.
>> > + */
>> > + lea .startof.(.bss)(%rip),%edi
>> > + mov $.sizeof.(.bss),%ecx
>> > + shr $3,%ecx
>> > + xor %eax,%eax
>> > + rep stosq
>> > +
>> > + pop %rdi
>> > +
>> > + /*
>> > + * efi_multiboot2() is called according to System V AMD64 ABI:
>> > + * - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable,
>> > + * - OUT: %rax - highest usable memory address below 1 MiB;
>> > + * memory above this address is reserved for trampoline;
>> > + * memory below this address is used for stack and as
>> > + * a storage for boot data.
>>
>> How can you validly use memory below this address? (And I'd like to
>
> Right, I should not do that blindly. As quick fix we can check in
> efi_arch_process_memory_map()
> that chosen memory region has size cfg.size plus let's say 64 KiB. Is it sufficient
> for you?
Depends. Part of my problem here is that you convert, in your answer,
my "validly" to "blindly". And then I'd like to see especially "storage for
boot data" better qualified: What exactly is it that you mean to store
there? I don't recall having noticed either this or that area being used
as stack anywhere in the series, so I'm afraid I've overlooked
something which may invalidate a good part of the review done.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms
2016-09-28 8:57 ` Jan Beulich
@ 2016-09-28 9:39 ` Daniel Kiper
2016-09-28 10:16 ` Jan Beulich
0 siblings, 1 reply; 51+ messages in thread
From: Daniel Kiper @ 2016-09-28 9:39 UTC (permalink / raw)
To: Jan Beulich
Cc: Juergen Gross, sstabellini, andrew.cooper3, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
On Wed, Sep 28, 2016 at 02:57:10AM -0600, Jan Beulich wrote:
> >>> On 27.09.16 at 20:11, <daniel.kiper@oracle.com> wrote:
> > On Mon, Sep 26, 2016 at 07:47:42AM -0600, Jan Beulich wrote:
> >> >>> On 23.09.16 at 23:47, <daniel.kiper@oracle.com> wrote:
> >> > This way Xen can be loaded on EFI platforms using GRUB2 and
> >> > other boot loaders which support multiboot2 protocol.
> >> >
> >> > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> >> > Acked-by: Jan Beulich <jbeulich@suse.com>
> >> > ---
> >> > v7 - suggestions/fixes:
> >> > - do not allocate twice memory for trampoline if we were
> >> > loaded via multiboot2 protocol on EFI platform,
> >>
> >> If you fix bugs not noticed by a reviewer but by yourself, please
> >> drop all acks/R-b-s covering the code in question. And then I'm
> >
> > OK.
> >
> >> afraid I haven't even been able to locate that change - could you
> >> please point out what you did where?
> >
> > The change is very subtle. I add trampoline_setup label behind
> >
> > /* From arch/x86/smpboot.c: start_eip had better be page-aligned! */
> > xor %cl, %cl
> >
> > instead of
> >
> > cmp %ecx,%edx /* compare with BDA value */
> > cmovb %edx,%ecx /* and use the smaller */
>
> So how did you expect anyone having looked at previous version
> to spot that? Please, in your changes section, rather than
> tediously listing who suggested which change, please make that
> section useful for incremental reviews.
I think that in general it is useful, however, I can agree that
this thing could be described better.
> >> > + /*
> >> > + * Initialize BSS (no nasty surprises!).
> >> > + * It must be done earlier than in BIOS case
> >> > + * because efi_multiboot2() touches it.
> >> > + */
> >> > + lea .startof.(.bss)(%rip),%edi
> >> > + mov $.sizeof.(.bss),%ecx
> >> > + shr $3,%ecx
> >> > + xor %eax,%eax
> >> > + rep stosq
> >> > +
> >> > + pop %rdi
> >> > +
> >> > + /*
> >> > + * efi_multiboot2() is called according to System V AMD64 ABI:
> >> > + * - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable,
> >> > + * - OUT: %rax - highest usable memory address below 1 MiB;
> >> > + * memory above this address is reserved for trampoline;
> >> > + * memory below this address is used for stack and as
> >> > + * a storage for boot data.
> >>
> >> How can you validly use memory below this address? (And I'd like to
> >
> > Right, I should not do that blindly. As quick fix we can check in
> > efi_arch_process_memory_map()
> > that chosen memory region has size cfg.size plus let's say 64 KiB. Is it sufficient
> > for you?
>
> Depends. Part of my problem here is that you convert, in your answer,
> my "validly" to "blindly". And then I'd like to see especially "storage for
I am not sure why the difference, IMO minor, is important for you here.
> boot data" better qualified: What exactly is it that you mean to store
> there?
mbi struct created in reloc.c from original multiboot(2) data passed by boot loader.
> I don't recall having noticed either this or that area being used
> as stack anywhere in the series, so I'm afraid I've overlooked
Hmmm... I do not know why I put stack here. It should be dropped from comment.
Daniel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms
2016-09-28 9:39 ` Daniel Kiper
@ 2016-09-28 10:16 ` Jan Beulich
0 siblings, 0 replies; 51+ messages in thread
From: Jan Beulich @ 2016-09-28 10:16 UTC (permalink / raw)
To: Daniel Kiper
Cc: Juergen Gross, sstabellini, andrew.cooper3, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
>>> On 28.09.16 at 11:39, <daniel.kiper@oracle.com> wrote:
> On Wed, Sep 28, 2016 at 02:57:10AM -0600, Jan Beulich wrote:
>> >>> On 27.09.16 at 20:11, <daniel.kiper@oracle.com> wrote:
>> > On Mon, Sep 26, 2016 at 07:47:42AM -0600, Jan Beulich wrote:
>> >> >>> On 23.09.16 at 23:47, <daniel.kiper@oracle.com> wrote:
>> >> > + /*
>> >> > + * Initialize BSS (no nasty surprises!).
>> >> > + * It must be done earlier than in BIOS case
>> >> > + * because efi_multiboot2() touches it.
>> >> > + */
>> >> > + lea .startof.(.bss)(%rip),%edi
>> >> > + mov $.sizeof.(.bss),%ecx
>> >> > + shr $3,%ecx
>> >> > + xor %eax,%eax
>> >> > + rep stosq
>> >> > +
>> >> > + pop %rdi
>> >> > +
>> >> > + /*
>> >> > + * efi_multiboot2() is called according to System V AMD64 ABI:
>> >> > + * - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable,
>> >> > + * - OUT: %rax - highest usable memory address below 1 MiB;
>> >> > + * memory above this address is reserved for trampoline;
>> >> > + * memory below this address is used for stack and as
>> >> > + * a storage for boot data.
>> >>
>> >> How can you validly use memory below this address? (And I'd like to
>> >
>> > Right, I should not do that blindly. As quick fix we can check in
>> > efi_arch_process_memory_map()
>> > that chosen memory region has size cfg.size plus let's say 64 KiB. Is it sufficient
>> > for you?
>>
>> Depends. Part of my problem here is that you convert, in your answer,
>> my "validly" to "blindly". And then I'd like to see especially "storage for
>
> I am not sure why the difference, IMO minor, is important for you here.
Because I questioned any access to that area, yet you suggested
accesses are okay with a little more checking.
>> boot data" better qualified: What exactly is it that you mean to store
>> there?
>
> mbi struct created in reloc.c from original multiboot(2) data passed by boot
> loader.
Ah, right. Please make the comment less vague then. And this then
also gives you/us a means to determine what size would need to be
available (64k looks to be way too much for mb1, but mb2 of course
is much less bounded).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms
2016-09-23 21:47 ` [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms Daniel Kiper
2016-09-26 13:47 ` Jan Beulich
@ 2016-09-26 14:19 ` Andrew Cooper
2016-09-26 14:33 ` Jan Beulich
1 sibling, 1 reply; 51+ messages in thread
From: Andrew Cooper @ 2016-09-26 14:19 UTC (permalink / raw)
To: Daniel Kiper, xen-devel
Cc: jgross, sstabellini, cardoe, pgnet.dev, ning.sun, julien.grall,
david.vrabel, jbeulich, qiaowei.ren, gang.wei, fu.wei
On 23/09/16 22:47, Daniel Kiper wrote:
> + /*
> + * Initialize BSS (no nasty surprises!).
> + * It must be done earlier than in BIOS case
> + * because efi_multiboot2() touches it.
> + */
> + lea .startof.(.bss)(%rip),%edi
> + mov $.sizeof.(.bss),%ecx
Sorry, but you cannot use this syntax, for the same reasons why I won't
accept Jan's patch making similar changes elsewhere.
Amongst other issues, you will break the Clang build with it.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms
2016-09-26 14:19 ` Andrew Cooper
@ 2016-09-26 14:33 ` Jan Beulich
2016-09-26 14:40 ` Andrew Cooper
0 siblings, 1 reply; 51+ messages in thread
From: Jan Beulich @ 2016-09-26 14:33 UTC (permalink / raw)
To: Andrew Cooper
Cc: Juergen Gross, sstabellini, Daniel Kiper, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
>>> On 26.09.16 at 16:19, <andrew.cooper3@citrix.com> wrote:
> On 23/09/16 22:47, Daniel Kiper wrote:
>> + /*
>> + * Initialize BSS (no nasty surprises!).
>> + * It must be done earlier than in BIOS case
>> + * because efi_multiboot2() touches it.
>> + */
>> + lea .startof.(.bss)(%rip),%edi
>> + mov $.sizeof.(.bss),%ecx
>
> Sorry, but you cannot use this syntax, for the same reasons why I won't
> accept Jan's patch making similar changes elsewhere.
>
> Amongst other issues, you will break the Clang build with it.
Did you verify meanwhile that this doesn't work with llvm?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms
2016-09-26 14:33 ` Jan Beulich
@ 2016-09-26 14:40 ` Andrew Cooper
2016-09-26 15:12 ` Jan Beulich
0 siblings, 1 reply; 51+ messages in thread
From: Andrew Cooper @ 2016-09-26 14:40 UTC (permalink / raw)
To: Jan Beulich
Cc: Juergen Gross, sstabellini, Daniel Kiper, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
On 26/09/16 15:33, Jan Beulich wrote:
>>>> On 26.09.16 at 16:19, <andrew.cooper3@citrix.com> wrote:
>> On 23/09/16 22:47, Daniel Kiper wrote:
>>> + /*
>>> + * Initialize BSS (no nasty surprises!).
>>> + * It must be done earlier than in BIOS case
>>> + * because efi_multiboot2() touches it.
>>> + */
>>> + lea .startof.(.bss)(%rip),%edi
>>> + mov $.sizeof.(.bss),%ecx
>> Sorry, but you cannot use this syntax, for the same reasons why I won't
>> accept Jan's patch making similar changes elsewhere.
>>
>> Amongst other issues, you will break the Clang build with it.
> Did you verify meanwhile that this doesn't work with llvm?
Yes.
andrewcoop@andrewcoop:/local/xen.git/xen$ cat foo.c
#include <stdio.h>
static unsigned int x;
int main(void)
{
asm volatile("lea .startof.(.bss)(%%rip), %%rdi;"
"mov $.sizeof.(.bss), %%rcx;"
"mov $-1, %%rax;"
"rep stosb;"
::: "memory", "rax", "rcx", "rdi");
printf("x: %#x\n", x);
}
andrewcoop@andrewcoop:/local/xen.git/xen$ gcc foo.c -o foo && ./foo
x: 0xffffffff
andrewcoop@andrewcoop:/local/xen.git/xen$ clang-3.8 foo.c -o foo && ./foo
foo.c:7:18: error: unexpected token in memory operand
asm volatile("lea .startof.(.bss)(%%rip), %%rdi;"
^
<inline asm>:1:16: note: instantiated into assembly here
lea .startof.(.bss)(%rip), %rdi;mov $.sizeof.(.bss), %rcx;mov
$-1, %rax;rep stosb;
^
foo.c:7:18: error: unexpected token in argument list
asm volatile("lea .startof.(.bss)(%%rip), %%rdi;"
^
<inline asm>:1:47: note: instantiated into assembly here
lea .startof.(.bss)(%rip), %rdi;mov $.sizeof.(.bss), %rcx;mov
$-1, %rax;rep stosb;
^
2 errors generated.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms
2016-09-26 14:40 ` Andrew Cooper
@ 2016-09-26 15:12 ` Jan Beulich
2016-09-27 18:21 ` Daniel Kiper
0 siblings, 1 reply; 51+ messages in thread
From: Jan Beulich @ 2016-09-26 15:12 UTC (permalink / raw)
To: Andrew Cooper
Cc: Juergen Gross, sstabellini, Daniel Kiper, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
>>> On 26.09.16 at 16:40, <andrew.cooper3@citrix.com> wrote:
> On 26/09/16 15:33, Jan Beulich wrote:
>>>>> On 26.09.16 at 16:19, <andrew.cooper3@citrix.com> wrote:
>>> On 23/09/16 22:47, Daniel Kiper wrote:
>>>> + /*
>>>> + * Initialize BSS (no nasty surprises!).
>>>> + * It must be done earlier than in BIOS case
>>>> + * because efi_multiboot2() touches it.
>>>> + */
>>>> + lea .startof.(.bss)(%rip),%edi
>>>> + mov $.sizeof.(.bss),%ecx
>>> Sorry, but you cannot use this syntax, for the same reasons why I won't
>>> accept Jan's patch making similar changes elsewhere.
>>>
>>> Amongst other issues, you will break the Clang build with it.
>> Did you verify meanwhile that this doesn't work with llvm?
>
> Yes.
>
> andrewcoop@andrewcoop:/local/xen.git/xen$ cat foo.c
> #include <stdio.h>
>
> static unsigned int x;
>
> int main(void)
> {
> asm volatile("lea .startof.(.bss)(%%rip), %%rdi;"
> "mov $.sizeof.(.bss), %%rcx;"
> "mov $-1, %%rax;"
> "rep stosb;"
> ::: "memory", "rax", "rcx", "rdi");
> printf("x: %#x\n", x);
> }
> andrewcoop@andrewcoop:/local/xen.git/xen$ gcc foo.c -o foo && ./foo
> x: 0xffffffff
> andrewcoop@andrewcoop:/local/xen.git/xen$ clang-3.8 foo.c -o foo && ./foo
> foo.c:7:18: error: unexpected token in memory operand
> asm volatile("lea .startof.(.bss)(%%rip), %%rdi;"
> ^
> <inline asm>:1:16: note: instantiated into assembly here
> lea .startof.(.bss)(%rip), %rdi;mov $.sizeof.(.bss), %rcx;mov
> $-1, %rax;rep stosb;
> ^
> foo.c:7:18: error: unexpected token in argument list
> asm volatile("lea .startof.(.bss)(%%rip), %%rdi;"
> ^
> <inline asm>:1:47: note: instantiated into assembly here
> lea .startof.(.bss)(%rip), %rdi;mov $.sizeof.(.bss), %rcx;mov
> $-1, %rax;rep stosb;
> ^
> 2 errors generated.
No, that's inline assembly, which does not match Daniel's use
case. That's why I referred to llvm, as with assembly sources it
should only be the linker which gets to see the symbols generated
from those constructs (and if llvm supported them I'd then consider
breaking out the assembly file changes from that patch of mine).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms
2016-09-26 15:12 ` Jan Beulich
@ 2016-09-27 18:21 ` Daniel Kiper
2016-09-28 8:58 ` Jan Beulich
0 siblings, 1 reply; 51+ messages in thread
From: Daniel Kiper @ 2016-09-27 18:21 UTC (permalink / raw)
To: Jan Beulich
Cc: Juergen Gross, sstabellini, Andrew Cooper, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
On Mon, Sep 26, 2016 at 09:12:40AM -0600, Jan Beulich wrote:
> >>> On 26.09.16 at 16:40, <andrew.cooper3@citrix.com> wrote:
> > On 26/09/16 15:33, Jan Beulich wrote:
> >>>>> On 26.09.16 at 16:19, <andrew.cooper3@citrix.com> wrote:
> >>> On 23/09/16 22:47, Daniel Kiper wrote:
> >>>> + /*
> >>>> + * Initialize BSS (no nasty surprises!).
> >>>> + * It must be done earlier than in BIOS case
> >>>> + * because efi_multiboot2() touches it.
> >>>> + */
> >>>> + lea .startof.(.bss)(%rip),%edi
> >>>> + mov $.sizeof.(.bss),%ecx
> >>> Sorry, but you cannot use this syntax, for the same reasons why I won't
> >>> accept Jan's patch making similar changes elsewhere.
> >>>
> >>> Amongst other issues, you will break the Clang build with it.
> >> Did you verify meanwhile that this doesn't work with llvm?
> >
> > Yes.
> >
> > andrewcoop@andrewcoop:/local/xen.git/xen$ cat foo.c
> > #include <stdio.h>
> >
> > static unsigned int x;
> >
> > int main(void)
> > {
> > asm volatile("lea .startof.(.bss)(%%rip), %%rdi;"
> > "mov $.sizeof.(.bss), %%rcx;"
> > "mov $-1, %%rax;"
> > "rep stosb;"
> > ::: "memory", "rax", "rcx", "rdi");
> > printf("x: %#x\n", x);
> > }
> > andrewcoop@andrewcoop:/local/xen.git/xen$ gcc foo.c -o foo && ./foo
> > x: 0xffffffff
> > andrewcoop@andrewcoop:/local/xen.git/xen$ clang-3.8 foo.c -o foo && ./foo
> > foo.c:7:18: error: unexpected token in memory operand
> > asm volatile("lea .startof.(.bss)(%%rip), %%rdi;"
> > ^
> > <inline asm>:1:16: note: instantiated into assembly here
> > lea .startof.(.bss)(%rip), %rdi;mov $.sizeof.(.bss), %rcx;mov
> > $-1, %rax;rep stosb;
> > ^
> > foo.c:7:18: error: unexpected token in argument list
> > asm volatile("lea .startof.(.bss)(%%rip), %%rdi;"
> > ^
> > <inline asm>:1:47: note: instantiated into assembly here
> > lea .startof.(.bss)(%rip), %rdi;mov $.sizeof.(.bss), %rcx;mov
> > $-1, %rax;rep stosb;
> > ^
> > 2 errors generated.
>
> No, that's inline assembly, which does not match Daniel's use
> case. That's why I referred to llvm, as with assembly sources it
> should only be the linker which gets to see the symbols generated
> from those constructs (and if llvm supported them I'd then consider
> breaking out the assembly file changes from that patch of mine).
Guys, may I suggest using sym_phys(__bss_start)/sym_phys(__bss_end) as
it is in head.S right now? If .startof.()/.sizeof.() works as expected
and both of you accept it then later we can move to new approach. Does
it make sense?
Daniel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms
2016-09-27 18:21 ` Daniel Kiper
@ 2016-09-28 8:58 ` Jan Beulich
0 siblings, 0 replies; 51+ messages in thread
From: Jan Beulich @ 2016-09-28 8:58 UTC (permalink / raw)
To: Daniel Kiper
Cc: Juergen Gross, sstabellini, Andrew Cooper, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
>>> On 27.09.16 at 20:21, <daniel.kiper@oracle.com> wrote:
> On Mon, Sep 26, 2016 at 09:12:40AM -0600, Jan Beulich wrote:
>> >>> On 26.09.16 at 16:40, <andrew.cooper3@citrix.com> wrote:
>> > On 26/09/16 15:33, Jan Beulich wrote:
>> >>>>> On 26.09.16 at 16:19, <andrew.cooper3@citrix.com> wrote:
>> >>> On 23/09/16 22:47, Daniel Kiper wrote:
>> >>>> + /*
>> >>>> + * Initialize BSS (no nasty surprises!).
>> >>>> + * It must be done earlier than in BIOS case
>> >>>> + * because efi_multiboot2() touches it.
>> >>>> + */
>> >>>> + lea .startof.(.bss)(%rip),%edi
>> >>>> + mov $.sizeof.(.bss),%ecx
>> >>> Sorry, but you cannot use this syntax, for the same reasons why I won't
>> >>> accept Jan's patch making similar changes elsewhere.
>> >>>
>> >>> Amongst other issues, you will break the Clang build with it.
>> >> Did you verify meanwhile that this doesn't work with llvm?
>> >
>> > Yes.
>> >
>> > andrewcoop@andrewcoop:/local/xen.git/xen$ cat foo.c
>> > #include <stdio.h>
>> >
>> > static unsigned int x;
>> >
>> > int main(void)
>> > {
>> > asm volatile("lea .startof.(.bss)(%%rip), %%rdi;"
>> > "mov $.sizeof.(.bss), %%rcx;"
>> > "mov $-1, %%rax;"
>> > "rep stosb;"
>> > ::: "memory", "rax", "rcx", "rdi");
>> > printf("x: %#x\n", x);
>> > }
>> > andrewcoop@andrewcoop:/local/xen.git/xen$ gcc foo.c -o foo && ./foo
>> > x: 0xffffffff
>> > andrewcoop@andrewcoop:/local/xen.git/xen$ clang-3.8 foo.c -o foo && ./foo
>> > foo.c:7:18: error: unexpected token in memory operand
>> > asm volatile("lea .startof.(.bss)(%%rip), %%rdi;"
>> > ^
>> > <inline asm>:1:16: note: instantiated into assembly here
>> > lea .startof.(.bss)(%rip), %rdi;mov $.sizeof.(.bss), %rcx;mov
>> > $-1, %rax;rep stosb;
>> > ^
>> > foo.c:7:18: error: unexpected token in argument list
>> > asm volatile("lea .startof.(.bss)(%%rip), %%rdi;"
>> > ^
>> > <inline asm>:1:47: note: instantiated into assembly here
>> > lea .startof.(.bss)(%rip), %rdi;mov $.sizeof.(.bss), %rcx;mov
>> > $-1, %rax;rep stosb;
>> > ^
>> > 2 errors generated.
>>
>> No, that's inline assembly, which does not match Daniel's use
>> case. That's why I referred to llvm, as with assembly sources it
>> should only be the linker which gets to see the symbols generated
>> from those constructs (and if llvm supported them I'd then consider
>> breaking out the assembly file changes from that patch of mine).
>
> Guys, may I suggest using sym_phys(__bss_start)/sym_phys(__bss_end) as
> it is in head.S right now? If .startof.()/.sizeof.() works as expected
> and both of you accept it then later we can move to new approach. Does
> it make sense?
While I dislike taking this step backwards, I guess that's the only
viable option to move this patch closer to being ready to go in.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v7 09/14] x86/boot: implement early command line parser in C
2016-09-23 21:47 [PATCH v7 00/14] x86: multiboot2 protocol support Daniel Kiper
` (7 preceding siblings ...)
2016-09-23 21:47 ` [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms Daniel Kiper
@ 2016-09-23 21:47 ` Daniel Kiper
2016-09-26 13:49 ` Jan Beulich
2016-09-23 21:47 ` [PATCH v7 10/14] x86: change default load address from 1 MiB to 2 MiB Daniel Kiper
` (4 subsequent siblings)
13 siblings, 1 reply; 51+ messages in thread
From: Daniel Kiper @ 2016-09-23 21:47 UTC (permalink / raw)
To: xen-devel
Cc: jgross, sstabellini, andrew.cooper3, cardoe, pgnet.dev, ning.sun,
julien.grall, david.vrabel, jbeulich, qiaowei.ren, gang.wei,
fu.wei
Current early command line parser implementation in assembler
is very difficult to change to relocatable stuff using segment
registers. This requires a lot of changes in very weird and
fragile code. So, reimplement this functionality in C. This
way code will be relocatable out of the box (without playing
with segment registers) and much easier to maintain.
Additionally, put all common cmdline.c and reloc.c definitions
into defs.h header. This way we do not duplicate needlessly
some stuff.
And finally remove unused xen/include/asm-x86/config.h
header from reloc.c dependencies.
Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
v7 - suggestions/fixes:
- add min() macro
(suggested by Jan Beulich),
- add padding to early_boot_opts_t
in more standard way
(suggested by Jan Beulich),
- simplify defs.h dependencies
(suggested by Jan Beulich).
v6 - suggestions/fixes:
- put common cmdline.c and reloc.c
definitions into defs.h header
(suggested by Jan Beulich),
- use xen/include/xen/stdbool.h
and bool type from it instead
of own defined bool_t
(suggested by Jan Beulich),
- define delim_chars as constant
(suggested by Jan Beulich),
- properly align trampoline.S:early_boot_opts struct
(suggested by Jan Beulich),
- fix overflow check in strtoui()
(suggested by Jan Beulich),
- remove unused xen/include/asm-x86/config.h
header from reloc.c dependencies,
- improve commit message.
v4 - suggestions/fixes:
- move to stdcall calling convention
(suggested by Jan Beulich),
- define bool_t and use it properly
(suggested by Jan Beulich),
- put list of delimiter chars into
static const char[]
(suggested by Jan Beulich),
- use strlen() instead of strlen_opt()
(suggested by Jan Beulich),
- change strtoi() to strtoui() and
optimize it a bit
(suggested by Jan Beulich),
- define strchr() and use it in strtoui()
(suggested by Jan Beulich),
- optimize vga_parse()
(suggested by Jan Beulich),
- move !cmdline check from assembly to C
(suggested by Jan Beulich),
- remove my name from copyright (Oracle requirement)
(suggested by Konrad Rzeszutek Wilk).
v3 - suggestions/fixes:
- optimize some code
(suggested by Jan Beulich),
- put VESA data into early_boot_opts_t members
(suggested by Jan Beulich),
- rename some functions and variables
(suggested by Jan Beulich),
- move around video.h include in xen/arch/x86/boot/trampoline.S
(suggested by Jan Beulich),
- fix coding style
(suggested by Jan Beulich),
- fix build with older GCC
(suggested by Konrad Rzeszutek Wilk),
- remove redundant comments
(suggested by Jan Beulich),
- add some comments
- improve commit message
(suggested by Jan Beulich).
---
.gitignore | 5 +-
xen/arch/x86/Makefile | 2 +-
xen/arch/x86/boot/Makefile | 11 +-
xen/arch/x86/boot/build32.mk | 2 +
xen/arch/x86/boot/cmdline.S | 367 ----------------------------------------
xen/arch/x86/boot/cmdline.c | 340 +++++++++++++++++++++++++++++++++++++
xen/arch/x86/boot/defs.h | 58 +++++++
xen/arch/x86/boot/edd.S | 3 -
xen/arch/x86/boot/head.S | 8 +
xen/arch/x86/boot/reloc.c | 13 +-
xen/arch/x86/boot/trampoline.S | 15 ++
xen/arch/x86/boot/video.S | 7 -
12 files changed, 437 insertions(+), 394 deletions(-)
delete mode 100644 xen/arch/x86/boot/cmdline.S
create mode 100644 xen/arch/x86/boot/cmdline.c
create mode 100644 xen/arch/x86/boot/defs.h
diff --git a/.gitignore b/.gitignore
index cc64fc9..9aa47a1 100644
--- a/.gitignore
+++ b/.gitignore
@@ -247,9 +247,10 @@ xen/arch/arm/xen.lds
xen/arch/x86/asm-offsets.s
xen/arch/x86/boot/mkelf32
xen/arch/x86/xen.lds
+xen/arch/x86/boot/cmdline.S
xen/arch/x86/boot/reloc.S
-xen/arch/x86/boot/reloc.bin
-xen/arch/x86/boot/reloc.lnk
+xen/arch/x86/boot/*.bin
+xen/arch/x86/boot/*.lnk
xen/arch/x86/efi.lds
xen/arch/x86/efi/check.efi
xen/arch/x86/efi/disabled
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 49d7e59..7d37728 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -224,6 +224,6 @@ clean::
rm -f asm-offsets.s *.lds boot/*.o boot/*~ boot/core boot/mkelf32
rm -f $(BASEDIR)/.xen-syms.[0-9]* boot/.*.d
rm -f $(BASEDIR)/.xen.efi.[0-9]* efi/*.efi efi/disabled efi/mkreloc
- rm -f boot/reloc.S boot/reloc.lnk boot/reloc.bin
+ rm -f boot/cmdline.S boot/reloc.S boot/*.lnk boot/*.bin
rm -f note.o
$(MAKE) -f $(BASEDIR)/Rules.mk -C test clean
diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
index 06893d8..c6246c8 100644
--- a/xen/arch/x86/boot/Makefile
+++ b/xen/arch/x86/boot/Makefile
@@ -1,9 +1,16 @@
obj-bin-y += head.o
-RELOC_DEPS = $(BASEDIR)/include/asm-x86/config.h $(BASEDIR)/include/xen/multiboot.h \
+DEFS_H_DEPS = defs.h $(BASEDIR)/include/xen/stdbool.h
+
+CMDLINE_DEPS = $(DEFS_H_DEPS) video.h
+
+RELOC_DEPS = $(DEFS_H_DEPS) $(BASEDIR)/include/xen/multiboot.h \
$(BASEDIR)/include/xen/multiboot2.h
-head.o: reloc.S
+head.o: cmdline.S reloc.S
+
+cmdline.S: cmdline.c $(CMDLINE_DEPS)
+ $(MAKE) -f build32.mk $@ CMDLINE_DEPS="$(CMDLINE_DEPS)"
reloc.S: reloc.c $(RELOC_DEPS)
$(MAKE) -f build32.mk $@ RELOC_DEPS="$(RELOC_DEPS)"
diff --git a/xen/arch/x86/boot/build32.mk b/xen/arch/x86/boot/build32.mk
index 39e6453..3d01698 100644
--- a/xen/arch/x86/boot/build32.mk
+++ b/xen/arch/x86/boot/build32.mk
@@ -32,6 +32,8 @@ CFLAGS := $(filter-out -flto,$(CFLAGS))
%.o: %.c
$(CC) $(CFLAGS) -c -fpic $< -o $@
+cmdline.o: cmdline.c $(CMDLINE_DEPS)
+
reloc.o: reloc.c $(RELOC_DEPS)
.PRECIOUS: %.bin %.lnk
diff --git a/xen/arch/x86/boot/cmdline.S b/xen/arch/x86/boot/cmdline.S
deleted file mode 100644
index 00687eb..0000000
--- a/xen/arch/x86/boot/cmdline.S
+++ /dev/null
@@ -1,367 +0,0 @@
-/******************************************************************************
- * cmdline.S
- *
- * Early command-line parsing.
- */
-
- .code32
-
-#include "video.h"
-
-# NB. String pointer on stack is modified to point past parsed digits.
-.Latoi:
- push %ebx
- push %ecx
- push %edx
- push %esi
- xor %ebx,%ebx /* %ebx = accumulator */
- mov $10,%ecx /* %ecx = base (default base 10) */
- mov 16+4(%esp),%esi /* %esi = pointer into ascii string. */
- lodsb
- cmpb $'0',%al
- jne 2f
- mov $8,%ecx /* Prefix '0' => octal (base 8) */
- lodsb
- cmpb $'x',%al
- jne 2f
- mov $16,%ecx /* Prefix '0x' => hex (base 16) */
-1: lodsb
-2: sub $'0',%al
- jb 4f
- cmp $9,%al
- jbe 3f
- sub $'A'-'0'-10,%al
- jb 4f
- cmp $15,%al
- jbe 3f
- sub $'a'-'A',%al
- jb 4f
-3: cmp %cl,%al
- jae 4f
- movzbl %al,%eax
- xchg %eax,%ebx
- mul %ecx
- xchg %eax,%ebx
- add %eax,%ebx
- jmp 1b
-4: mov %ebx,%eax
- dec %esi
- mov %esi,16+4(%esp)
- pop %esi
- pop %edx
- pop %ecx
- pop %ebx
- ret
-
-.Lstrstr:
- push %ecx
- push %edx
- push %esi
- push %edi
- xor %eax,%eax
- xor %ecx,%ecx
- not %ecx
- mov 16+4(%esp),%esi
- mov 16+8(%esp),%edi
- repne scasb
- not %ecx
- dec %ecx
- mov %ecx,%edx
-1: mov 16+8(%esp),%edi
- mov %esi,%eax
- mov %edx,%ecx
- repe cmpsb
- je 2f
- xchg %eax,%esi
- inc %esi
- cmpb $0,-1(%eax)
- jne 1b
- xor %eax,%eax
-2: pop %edi
- pop %esi
- pop %edx
- pop %ecx
- ret
-
-.Lstr_prefix:
- push %esi
- push %edi
- mov 8+4(%esp),%esi /* 1st arg is prefix string */
- mov 8+8(%esp),%edi /* 2nd arg is main string */
-1: lodsb
- test %al,%al
- jz 2f
- scasb
- je 1b
- sbb %eax,%eax
- or $1,%al
- jmp 3f
-2: xor %eax,%eax
-3: pop %edi
- pop %esi
- ret
-
-.Lstrlen:
- push %ecx
- push %esi
- push %edi
- xor %eax,%eax
- xor %ecx,%ecx
- not %ecx
- mov 12+4(%esp),%edi
- repne scasb
- not %ecx
- dec %ecx
- mov %ecx,%eax
- pop %edi
- pop %esi
- pop %ecx
- ret
-
-.Lfind_option:
- mov 4(%esp),%eax
- dec %eax
- push %ebx
-1: pushl 4+8(%esp)
- inc %eax
- push %eax
- call .Lstrstr
- add $8,%esp
- test %eax,%eax
- jz 3f
- cmp %eax,4+4(%esp)
- je 2f
- cmpb $' ',-1(%eax)
- jne 1b
-2: mov %eax,%ebx
- pushl 4+8(%esp)
- call .Lstrlen
- add $4,%esp
- xadd %eax,%ebx
- /* NUL check (as $'\0' == 0x30 in GAS) */
- cmpb $0,(%ebx)
- je 3f
- cmpb $' ',(%ebx)
- je 3f
- cmpb $'=',(%ebx)
- jne 1b
-3: pop %ebx
- ret
-
-cmdline_parse_early:
- pusha
-
- /* Bail if there is no command line to parse. */
- mov sym_phys(multiboot_ptr),%ebx
- mov MB_flags(%ebx),%eax
- test $4,%al
- jz .Lcmdline_exit
- mov MB_cmdline(%ebx),%eax
- test %eax,%eax
- jz .Lcmdline_exit
-
- /* Check for 'no-real-mode' command-line option. */
- pushl $sym_phys(.Lno_rm_opt)
- pushl MB_cmdline(%ebx)
- call .Lfind_option
- test %eax,%eax
- setnz %al
- or %al,sym_phys(skip_realmode)
-
- /* Check for 'tboot=' command-line option. */
- movl $sym_phys(.Ltboot_opt),4(%esp)
- call .Lfind_option
- test %eax,%eax
- setnz %al
- or %al,sym_phys(skip_realmode) /* tboot= implies no-real-mode */
-
-.Lparse_edd:
- /* Check for 'edd=' command-line option. */
- movl $sym_phys(.Ledd_opt),4(%esp)
- call .Lfind_option
- test %eax,%eax
- jz .Lparse_edid
- cmpb $'=',3(%eax)
- jne .Lparse_edid
- add $4,%eax
- movb $2,sym_phys(opt_edd) /* opt_edd=2: edd=off */
- cmpw $0x666f,(%eax) /* 0x666f == "of" */
- je .Lparse_edid
- decb sym_phys(opt_edd) /* opt_edd=1: edd=skipmbr */
- cmpw $0x6b73,(%eax) /* 0x6b73 == "sk" */
- je .Lparse_edid
- decb sym_phys(opt_edd) /* opt_edd=0: edd=on (default) */
-
-.Lparse_edid:
- /* Check for 'edid=' command-line option. */
- movl $sym_phys(.Ledid_opt),4(%esp)
- call .Lfind_option
- test %eax,%eax
- jz .Lparse_vga
- cmpb $'=',4(%eax)
- jne .Lparse_vga
- add $5,%eax
- mov %eax,%ebx
- push %ebx
- pushl $sym_phys(.Ledid_force)
- call .Lstr_prefix
- add $8,%esp
- movb $2,sym_phys(opt_edid) /* opt_edid=2: edid=force */
- test %eax,%eax
- jz .Lparse_vga
- push %ebx
- pushl $sym_phys(.Ledid_no)
- call .Lstr_prefix
- add $8,%esp
- decb sym_phys(opt_edid) /* opt_edid=1: edid=no */
- test %eax,%eax
- jz .Lparse_vga
- decb sym_phys(opt_edid) /* opt_edid=0: default */
-
-.Lparse_vga:
- /* Check for 'vga=' command-line option. */
- movl $sym_phys(.Lvga_opt),4(%esp)
- call .Lfind_option
- add $8,%esp
- test %eax,%eax
- jz .Lcmdline_exit
- cmpb $'=',3(%eax)
- jne .Lcmdline_exit
- add $4,%eax
-
- /* Found the 'vga=' option. Default option is to display vga menu. */
- movw $ASK_VGA,sym_phys(boot_vid_mode)
-
- /* Check for 'vga=text-80x<rows>. */
- mov %eax,%ebx
- push %ebx
- pushl $sym_phys(.Lvga_text80)
- call .Lstr_prefix
- add $8,%esp
- test %eax,%eax
- jnz .Lparse_vga_gfx
-
- /* We have 'vga=text-80x<rows>'. */
- add $8,%ebx
- push %ebx
- call .Latoi
- add $4,%esp
- mov %ax,%bx
- lea sym_phys(.Lvga_text_modes),%esi
-1: lodsw
- test %ax,%ax
- jz .Lcmdline_exit
- cmp %ax,%bx
- lodsw
- jne 1b
- mov %ax,sym_phys(boot_vid_mode)
- jmp .Lcmdline_exit
-
-.Lparse_vga_gfx:
- /* Check for 'vga=gfx-<width>x<height>x<depth>'. */
- push %ebx
- pushl $sym_phys(.Lvga_gfx)
- call .Lstr_prefix
- add $8,%esp
- test %eax,%eax
- jnz .Lparse_vga_mode
-
- /* We have 'vga=gfx-<width>x<height>x<depth>'. */
- /* skip 'gfx-' */
- add $4,%ebx
- /* parse <width> */
- push %ebx
- call .Latoi
- pop %esi
- mov %ax,sym_phys(vesa_size)+0
- /* skip 'x' */
- lodsb
- cmpb $'x',%al
- jne .Lcmdline_exit
- /* parse <height> */
- push %esi
- call .Latoi
- pop %esi
- mov %ax,sym_phys(vesa_size)+2
- /* skip 'x' */
- lodsb
- cmpb $'x',%al
- jne .Lcmdline_exit
- /* parse <depth> */
- push %esi
- call .Latoi
- pop %esi
- mov %ax,sym_phys(vesa_size)+4
- /* commit to vesa mode */
- movw $VIDEO_VESA_BY_SIZE,sym_phys(boot_vid_mode)
- jmp .Lcmdline_exit
-
-.Lparse_vga_mode:
- /* Check for 'vga=mode-<mode>'. */
- push %ebx
- pushl $sym_phys(.Lvga_mode)
- call .Lstr_prefix
- add $8,%esp
- test %eax,%eax
- jnz .Lparse_vga_current
-
- /* We have 'vga=mode-<mode>'. */
- add $5,%ebx
- push %ebx
- call .Latoi
- add $4,%esp
- mov %ax,sym_phys(boot_vid_mode)
- jmp .Lcmdline_exit
-
-.Lparse_vga_current:
- /* Check for 'vga=current'. */
- push %ebx
- pushl $sym_phys(.Lvga_current)
- call .Lstr_prefix
- add $8,%esp
- test %eax,%eax
- jnz .Lcmdline_exit
-
- /* We have 'vga=current'. */
- movw $VIDEO_CURRENT_MODE,sym_phys(boot_vid_mode)
-
-.Lcmdline_exit:
- popa
- ret
-
- .pushsection .init.rodata, "a", @progbits
-
-.Lvga_text_modes: /* rows, mode_number */
- .word 25,VIDEO_80x25
- .word 50,VIDEO_80x50
- .word 43,VIDEO_80x43
- .word 28,VIDEO_80x28
- .word 30,VIDEO_80x30
- .word 34,VIDEO_80x34
- .word 60,VIDEO_80x60
- .word 0
-
-.Lvga_opt:
- .asciz "vga"
-.Lvga_text80:
- .asciz "text-80x"
-.Lvga_gfx:
- .asciz "gfx-"
-.Lvga_mode:
- .asciz "mode-"
-.Lvga_current:
- .asciz "current"
-.Lno_rm_opt:
- .asciz "no-real-mode"
-.Ltboot_opt:
- .asciz "tboot"
-.Ledid_opt:
- .asciz "edid"
-.Ledid_force:
- .asciz "force"
-.Ledid_no:
- .asciz "no"
-.Ledd_opt:
- .asciz "edd"
-
- .popsection
diff --git a/xen/arch/x86/boot/cmdline.c b/xen/arch/x86/boot/cmdline.c
new file mode 100644
index 0000000..06aa064
--- /dev/null
+++ b/xen/arch/x86/boot/cmdline.c
@@ -0,0 +1,340 @@
+/*
+ * Copyright (c) 2015, 2016 Oracle and/or its affiliates. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ * strlen(), strncmp(), strchr(), strspn() and strcspn() were copied from
+ * Linux kernel source (linux/lib/string.c).
+ */
+
+/*
+ * This entry point is entered from xen/arch/x86/boot/head.S with:
+ * - 0x4(%esp) = &cmdline,
+ * - 0x8(%esp) = &early_boot_opts.
+ */
+asm (
+ " .text \n"
+ " .globl _start \n"
+ "_start: \n"
+ " jmp cmdline_parse_early \n"
+ );
+
+#include "defs.h"
+#include "video.h"
+
+/* Keep in sync with trampoline.S:early_boot_opts label! */
+typedef struct __packed {
+ u8 skip_realmode;
+ u8 opt_edd;
+ u8 opt_edid;
+ u8 padding;
+ u16 boot_vid_mode;
+ u16 vesa_width;
+ u16 vesa_height;
+ u16 vesa_depth;
+} early_boot_opts_t;
+
+/*
+ * Space and TAB are obvious delimiters. However, I am
+ * adding "\n" and "\r" here too. Just in case when
+ * crazy bootloader/user puts them somewhere.
+ */
+static const char delim_chars_comma[] = ", \n\r\t";
+
+#define delim_chars (delim_chars_comma + 1)
+
+static size_t strlen(const char *s)
+{
+ const char *sc;
+
+ for ( sc = s; *sc != '\0'; ++sc )
+ /* nothing */;
+ return sc - s;
+}
+
+static int strncmp(const char *cs, const char *ct, size_t count)
+{
+ unsigned char c1, c2;
+
+ while ( count )
+ {
+ c1 = *cs++;
+ c2 = *ct++;
+ if ( c1 != c2 )
+ return c1 < c2 ? -1 : 1;
+ if ( !c1 )
+ break;
+ count--;
+ }
+ return 0;
+}
+
+static char *strchr(const char *s, int c)
+{
+ for ( ; *s != (char)c; ++s )
+ if ( *s == '\0' )
+ return NULL;
+ return (char *)s;
+}
+
+static size_t strspn(const char *s, const char *accept)
+{
+ const char *p;
+ const char *a;
+ size_t count = 0;
+
+ for ( p = s; *p != '\0'; ++p )
+ {
+ for ( a = accept; *a != '\0'; ++a )
+ {
+ if ( *p == *a )
+ break;
+ }
+ if ( *a == '\0' )
+ return count;
+ ++count;
+ }
+ return count;
+}
+
+static size_t strcspn(const char *s, const char *reject)
+{
+ const char *p;
+ const char *r;
+ size_t count = 0;
+
+ for ( p = s; *p != '\0'; ++p )
+ {
+ for ( r = reject; *r != '\0'; ++r )
+ {
+ if ( *p == *r )
+ return count;
+ }
+ ++count;
+ }
+ return count;
+}
+
+static unsigned int strtoui(const char *s, const char *stop, const char **next)
+{
+ char base = 10, l;
+ unsigned long long res = 0;
+
+ if ( *s == '0' )
+ base = (tolower(*++s) == 'x') ? (++s, 16) : 8;
+
+ for ( ; *s != '\0'; ++s )
+ {
+ if ( stop && strchr(stop, *s) )
+ goto out;
+
+ if ( *s < '0' || (*s > '7' && base == 8) )
+ {
+ res = UINT_MAX;
+ goto out;
+ }
+
+ l = tolower(*s);
+
+ if ( *s > '9' && (base != 16 || l < 'a' || l > 'f') )
+ {
+ res = UINT_MAX;
+ goto out;
+ }
+
+ res *= base;
+ res += (l >= 'a') ? (l - 'a' + 10) : (*s - '0');
+
+ if ( res >= UINT_MAX )
+ {
+ res = UINT_MAX;
+ goto out;
+ }
+ }
+
+ out:
+ if ( next )
+ *next = s;
+
+ return res;
+}
+
+static int strmaxcmp(const char *cs, const char *ct, const char *_delim_chars)
+{
+ return strncmp(cs, ct, max(strcspn(cs, _delim_chars), strlen(ct)));
+}
+
+static int strsubcmp(const char *cs, const char *ct)
+{
+ return strncmp(cs, ct, strlen(ct));
+}
+
+static const char *find_opt(const char *cmdline, const char *opt, bool arg)
+{
+ size_t lc, lo;
+
+ lo = strlen(opt);
+
+ for ( ; ; )
+ {
+ cmdline += strspn(cmdline, delim_chars);
+
+ if ( *cmdline == '\0' )
+ return NULL;
+
+ if ( !strmaxcmp(cmdline, "--", delim_chars) )
+ return NULL;
+
+ lc = strcspn(cmdline, delim_chars);
+
+ if ( !strncmp(cmdline, opt, arg ? lo : max(lc, lo)) )
+ return cmdline + lo;
+
+ cmdline += lc;
+ }
+}
+
+static bool skip_realmode(const char *cmdline)
+{
+ return find_opt(cmdline, "no-real-mode", false) || find_opt(cmdline, "tboot=", true);
+}
+
+static u8 edd_parse(const char *cmdline)
+{
+ const char *c;
+
+ c = find_opt(cmdline, "edd=", true);
+
+ if ( !c )
+ return 0;
+
+ if ( !strmaxcmp(c, "off", delim_chars) )
+ return 2;
+
+ return !strmaxcmp(c, "skipmbr", delim_chars);
+}
+
+static u8 edid_parse(const char *cmdline)
+{
+ const char *c;
+
+ c = find_opt(cmdline, "edid=", true);
+
+ if ( !c )
+ return 0;
+
+ if ( !strmaxcmp(c, "force", delim_chars) )
+ return 2;
+
+ return !strmaxcmp(c, "no", delim_chars);
+}
+
+static u16 rows2vmode(unsigned int rows)
+{
+ switch ( rows )
+ {
+ case 25:
+ return VIDEO_80x25;
+
+ case 28:
+ return VIDEO_80x28;
+
+ case 30:
+ return VIDEO_80x30;
+
+ case 34:
+ return VIDEO_80x34;
+
+ case 43:
+ return VIDEO_80x43;
+
+ case 50:
+ return VIDEO_80x50;
+
+ case 60:
+ return VIDEO_80x60;
+
+ default:
+ return ASK_VGA;
+ }
+}
+
+static void vga_parse(const char *cmdline, early_boot_opts_t *ebo)
+{
+ const char *c;
+ unsigned int tmp, vesa_depth, vesa_height, vesa_width;
+
+ c = find_opt(cmdline, "vga=", true);
+
+ if ( !c )
+ return;
+
+ ebo->boot_vid_mode = ASK_VGA;
+
+ if ( !strmaxcmp(c, "current", delim_chars_comma) )
+ ebo->boot_vid_mode = VIDEO_CURRENT_MODE;
+ else if ( !strsubcmp(c, "text-80x") )
+ {
+ c += strlen("text-80x");
+ ebo->boot_vid_mode = rows2vmode(strtoui(c, delim_chars_comma, NULL));
+ }
+ else if ( !strsubcmp(c, "gfx-") )
+ {
+ vesa_width = strtoui(c + strlen("gfx-"), "x", &c);
+
+ if ( vesa_width > U16_MAX )
+ return;
+
+ /*
+ * Increment c outside of strtoui() because otherwise some
+ * compiler may complain with following message:
+ * warning: operation on 'c' may be undefined.
+ */
+ ++c;
+ vesa_height = strtoui(c, "x", &c);
+
+ if ( vesa_height > U16_MAX )
+ return;
+
+ vesa_depth = strtoui(++c, delim_chars_comma, NULL);
+
+ if ( vesa_depth > U16_MAX )
+ return;
+
+ ebo->vesa_width = vesa_width;
+ ebo->vesa_height = vesa_height;
+ ebo->vesa_depth = vesa_depth;
+ ebo->boot_vid_mode = VIDEO_VESA_BY_SIZE;
+ }
+ else if ( !strsubcmp(c, "mode-") )
+ {
+ tmp = strtoui(c + strlen("mode-"), delim_chars_comma, NULL);
+
+ if ( tmp > U16_MAX )
+ return;
+
+ ebo->boot_vid_mode = tmp;
+ }
+}
+
+void __stdcall cmdline_parse_early(const char *cmdline, early_boot_opts_t *ebo)
+{
+ if ( !cmdline )
+ return;
+
+ ebo->skip_realmode = skip_realmode(cmdline);
+ ebo->opt_edd = edd_parse(cmdline);
+ ebo->opt_edid = edid_parse(cmdline);
+ vga_parse(cmdline, ebo);
+}
diff --git a/xen/arch/x86/boot/defs.h b/xen/arch/x86/boot/defs.h
new file mode 100644
index 0000000..6abdc15
--- /dev/null
+++ b/xen/arch/x86/boot/defs.h
@@ -0,0 +1,58 @@
+/*
+ * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ * max() was copied from xen/xen/include/xen/kernel.h.
+ */
+
+#ifndef __BOOT_DEFS_H__
+#define __BOOT_DEFS_H__
+
+#include "../../../include/xen/stdbool.h"
+
+#define __packed __attribute__((__packed__))
+#define __stdcall __attribute__((__stdcall__))
+
+#define NULL ((void *)0)
+
+#define ALIGN_UP(arg, align) \
+ (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1))
+
+#define min(x,y) ({ \
+ const typeof(x) _x = (x); \
+ const typeof(y) _y = (y); \
+ (void) (&_x == &_y); \
+ _x < _y ? _x : _y; })
+
+#define max(x,y) ({ \
+ const typeof(x) _x = (x); \
+ const typeof(y) _y = (y); \
+ (void) (&_x == &_y); \
+ _x > _y ? _x : _y; })
+
+#define _p(val) ((void *)(unsigned long)(val))
+
+#define tolower(c) ((c) | 0x20)
+
+typedef unsigned char u8;
+typedef unsigned short u16;
+typedef unsigned int u32;
+typedef unsigned long long u64;
+typedef unsigned int size_t;
+
+#define U16_MAX ((u16)(~0U))
+#define UINT_MAX (~0U)
+
+#endif /* __BOOT_DEFS_H__ */
diff --git a/xen/arch/x86/boot/edd.S b/xen/arch/x86/boot/edd.S
index 5c80da6..73371f9 100644
--- a/xen/arch/x86/boot/edd.S
+++ b/xen/arch/x86/boot/edd.S
@@ -142,9 +142,6 @@ edd_next:
edd_done:
ret
-opt_edd:
- .byte 0 # edd=on/off/skipmbr
-
GLOBAL(boot_edd_info_nr)
.byte 0
GLOBAL(boot_mbr_signature_nr)
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index a371cde..c301464 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -509,6 +509,13 @@ trampoline_setup:
cmpb $0,sym_phys(skip_realmode)
jnz 1f
+ /* Bail if there is no command line to parse. */
+ mov sym_phys(multiboot_ptr),%ebx
+ testl $MBI_CMDLINE,MB_flags(%ebx)
+ jz 1f
+
+ pushl $sym_phys(early_boot_opts)
+ pushl MB_cmdline(%ebx)
call cmdline_parse_early
1:
@@ -527,6 +534,7 @@ trampoline_setup:
/* Jump into the relocated trampoline. */
lret
+cmdline_parse_early:
#include "cmdline.S"
reloc:
diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
index 94cead7..9f7cbb7 100644
--- a/xen/arch/x86/boot/reloc.c
+++ b/xen/arch/x86/boot/reloc.c
@@ -25,21 +25,10 @@ asm (
" jmp reloc \n"
);
-typedef unsigned int u32;
-typedef unsigned long long u64;
-
+#include "defs.h"
#include "../../../include/xen/multiboot.h"
#include "../../../include/xen/multiboot2.h"
-#define NULL ((void *)0)
-
-#define __stdcall __attribute__((__stdcall__))
-
-#define ALIGN_UP(arg, align) \
- (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1))
-
-#define _p(val) ((void *)(unsigned long)(val))
-
#define get_mb2_data(tag, type, member) (((multiboot2_tag_##type##_t *)(tag))->member)
#define get_mb2_string(tag, type, member) ((u32)get_mb2_data(tag, type, member))
diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S
index b013614..2715d17 100644
--- a/xen/arch/x86/boot/trampoline.S
+++ b/xen/arch/x86/boot/trampoline.S
@@ -220,8 +220,23 @@ trampoline_boot_cpu_entry:
/* Jump to the common bootstrap entry point. */
jmp trampoline_protmode_entry
+#include "video.h"
+
+ .align 2
+/* Keep in sync with cmdline.c:early_boot_opts_t type! */
+early_boot_opts:
skip_realmode:
.byte 0
+opt_edd:
+ .byte 0 /* edd=on/off/skipmbr */
+opt_edid:
+ .byte 0 /* EDID parsing option (force/no/default). */
+/* Padding. */
+ .byte 0
+GLOBAL(boot_vid_mode)
+ .word VIDEO_80x25 /* If we don't run at all, assume basic video mode 3 at 80x25. */
+vesa_size:
+ .word 0,0,0 /* width x depth x height */
GLOBAL(kbd_shift_flags)
.byte 0
diff --git a/xen/arch/x86/boot/video.S b/xen/arch/x86/boot/video.S
index 2aafbeb..335a51c 100644
--- a/xen/arch/x86/boot/video.S
+++ b/xen/arch/x86/boot/video.S
@@ -945,7 +945,6 @@ store_edid:
#endif
ret
-opt_edid: .byte 0 # EDID parsing option (force/no/default)
mt_end: .word 0 # End of video mode table if built
edit_buf: .space 6 # Line editor buffer
card_name: .word 0 # Pointer to adapter name
@@ -991,12 +990,6 @@ name_bann: .asciz "Video adapter: "
force_size: .word 0 # Use this size instead of the one in BIOS vars
-vesa_size: .word 0,0,0 # width x depth x height
-
-/* If we don't run at all, assume basic video mode 3 at 80x25. */
- .align 2
-GLOBAL(boot_vid_mode)
- .word VIDEO_80x25
GLOBAL(boot_vid_info)
.byte 0, 0 /* orig_x, orig_y */
.byte 3 /* text mode 3 */
--
1.7.10.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v7 09/14] x86/boot: implement early command line parser in C
2016-09-23 21:47 ` [PATCH v7 09/14] x86/boot: implement early command line parser in C Daniel Kiper
@ 2016-09-26 13:49 ` Jan Beulich
0 siblings, 0 replies; 51+ messages in thread
From: Jan Beulich @ 2016-09-26 13:49 UTC (permalink / raw)
To: Daniel Kiper
Cc: Juergen Gross, sstabellini, andrew.cooper3, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
>>> On 23.09.16 at 23:47, <daniel.kiper@oracle.com> wrote:
> Current early command line parser implementation in assembler
> is very difficult to change to relocatable stuff using segment
> registers. This requires a lot of changes in very weird and
> fragile code. So, reimplement this functionality in C. This
> way code will be relocatable out of the box (without playing
> with segment registers) and much easier to maintain.
>
> Additionally, put all common cmdline.c and reloc.c definitions
> into defs.h header. This way we do not duplicate needlessly
> some stuff.
>
> And finally remove unused xen/include/asm-x86/config.h
> header from reloc.c dependencies.
>
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v7 10/14] x86: change default load address from 1 MiB to 2 MiB
2016-09-23 21:47 [PATCH v7 00/14] x86: multiboot2 protocol support Daniel Kiper
` (8 preceding siblings ...)
2016-09-23 21:47 ` [PATCH v7 09/14] x86/boot: implement early command line parser in C Daniel Kiper
@ 2016-09-23 21:47 ` Daniel Kiper
2016-09-23 21:47 ` [PATCH v7 11/14] x86/setup: use XEN_IMG_OFFSET instead of Daniel Kiper
` (3 subsequent siblings)
13 siblings, 0 replies; 51+ messages in thread
From: Daniel Kiper @ 2016-09-23 21:47 UTC (permalink / raw)
To: xen-devel
Cc: jgross, sstabellini, andrew.cooper3, cardoe, pgnet.dev, ning.sun,
julien.grall, david.vrabel, jbeulich, qiaowei.ren, gang.wei,
fu.wei
Subsequent patches introducing relocatable early boot code play with
page tables using 2 MiB huge pages. If load address is not aligned at
2 MiB then code touching such page tables must have special cases for
start and end of Xen image memory region. So, let's make life easier
and move default load address from 1 MiB to 2 MiB. This way page table
code will be nice and easy. Hence, there is a chance that it will be
less error prone too... :-)))
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
v7 - suggestions/fixes:
- minor cleanups
(suggested by Jan Beulich).
---
xen/arch/x86/Makefile | 2 +-
xen/arch/x86/Rules.mk | 3 +++
xen/arch/x86/setup.c | 3 ++-
xen/arch/x86/xen.lds.S | 2 +-
4 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 7d37728..04cc59f 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -90,7 +90,7 @@ all_symbols =
endif
$(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
- ./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET) 0x100000 \
+ ./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET) $(XEN_IMG_OFFSET) \
`$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . __end_of_image__$$/0x\1/p'`
.PHONY: tests
diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
index 42be4bc..36e6386 100644
--- a/xen/arch/x86/Rules.mk
+++ b/xen/arch/x86/Rules.mk
@@ -1,9 +1,12 @@
########################################
# x86-specific definitions
+XEN_IMG_OFFSET := 0x200000
+
CFLAGS += -I$(BASEDIR)/include
CFLAGS += -I$(BASEDIR)/include/asm-x86/mach-generic
CFLAGS += -I$(BASEDIR)/include/asm-x86/mach-default
+CFLAGS += -DXEN_IMG_OFFSET=$(XEN_IMG_OFFSET)
CFLAGS += '-D__OBJECT_LABEL__=$(subst /,$$,$(subst -,_,$(subst $(BASEDIR)/,,$(CURDIR))/$@))'
# Prevent floating-point variables from creeping into Xen.
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 5b17783..f19af6e 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -957,7 +957,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
* Undo the temporary-hooking of the l1_identmap. __2M_text_start
* is contained in this PTE.
*/
- BUG_ON(l2_table_offset((unsigned long)_erodata) ==
+ BUG_ON(using_2M_mapping() &&
+ l2_table_offset((unsigned long)_erodata) ==
l2_table_offset((unsigned long)_stext));
*pl2e++ = l2e_from_pfn(xen_phys_start >> PAGE_SHIFT,
PAGE_HYPERVISOR_RX | _PAGE_PSE);
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index f4b4e99..d9561e5 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -55,7 +55,7 @@ SECTIONS
__2M_text_start = .; /* Start of 2M superpages, mapped RX. */
#endif
- . = __XEN_VIRT_START + MB(1);
+ . = __XEN_VIRT_START + XEN_IMG_OFFSET;
_start = .;
.text : {
_stext = .; /* Text and read-only data */
--
1.7.10.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH v7 11/14] x86/setup: use XEN_IMG_OFFSET instead of...
2016-09-23 21:47 [PATCH v7 00/14] x86: multiboot2 protocol support Daniel Kiper
` (9 preceding siblings ...)
2016-09-23 21:47 ` [PATCH v7 10/14] x86: change default load address from 1 MiB to 2 MiB Daniel Kiper
@ 2016-09-23 21:47 ` Daniel Kiper
2016-09-23 21:47 ` [PATCH v7 12/14] x86: make Xen early boot code relocatable Daniel Kiper
` (2 subsequent siblings)
13 siblings, 0 replies; 51+ messages in thread
From: Daniel Kiper @ 2016-09-23 21:47 UTC (permalink / raw)
To: xen-devel
Cc: jgross, sstabellini, andrew.cooper3, cardoe, pgnet.dev, ning.sun,
julien.grall, david.vrabel, jbeulich, qiaowei.ren, gang.wei,
fu.wei
..calculating its value during runtime.
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
xen/arch/x86/setup.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index f19af6e..ca1a97a 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -901,7 +901,6 @@ void __init noreturn __start_xen(unsigned long mbi_p)
l4_pgentry_t *pl4e;
l3_pgentry_t *pl3e;
l2_pgentry_t *pl2e;
- uint64_t load_start;
int i, j, k;
/* Select relocation address. */
@@ -915,9 +914,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
* with a barrier(). After this we must *not* modify static/global
* data until after we have switched to the relocated pagetables!
*/
- load_start = (unsigned long)_start - XEN_VIRT_START;
barrier();
- move_memory(e + load_start, load_start, _end - _start, 1);
+ move_memory(e + XEN_IMG_OFFSET, XEN_IMG_OFFSET, _end - _start, 1);
/* Walk initial pagetables, relocating page directory entries. */
pl4e = __va(__pa(idle_pg_table));
--
1.7.10.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH v7 12/14] x86: make Xen early boot code relocatable
2016-09-23 21:47 [PATCH v7 00/14] x86: multiboot2 protocol support Daniel Kiper
` (10 preceding siblings ...)
2016-09-23 21:47 ` [PATCH v7 11/14] x86/setup: use XEN_IMG_OFFSET instead of Daniel Kiper
@ 2016-09-23 21:47 ` Daniel Kiper
2016-09-26 15:03 ` Jan Beulich
2016-09-23 21:47 ` [PATCH v7 13/14] x86/boot: rename sym_phys() to sym_offs() Daniel Kiper
2016-09-23 21:47 ` [PATCH v7 14/14] x86: add multiboot2 protocol support for relocatable images Daniel Kiper
13 siblings, 1 reply; 51+ messages in thread
From: Daniel Kiper @ 2016-09-23 21:47 UTC (permalink / raw)
To: xen-devel
Cc: jgross, sstabellini, andrew.cooper3, cardoe, pgnet.dev, ning.sun,
julien.grall, david.vrabel, jbeulich, qiaowei.ren, gang.wei,
fu.wei
Every multiboot protocol (regardless of version) compatible image must
specify its load address (in ELF or multiboot header). Multiboot protocol
compatible loader have to load image at specified address. However, there
is no guarantee that the requested memory region (in case of Xen it starts
at 2 MiB and ends at ~5 MiB) where image should be loaded initially is a RAM
and it is free (legacy BIOS platforms are merciful for Xen but I found at
least one EFI platform on which Xen load address conflicts with EFI boot
services; it is Dell PowerEdge R820 with latest firmware). To cope with that
problem we must make Xen early boot code relocatable and help boot loader to
relocate image in proper way by suggesting, not requesting specific load
addresses as it is right now, allowed address ranges. This patch does former.
It does not add multiboot2 protocol interface which is done in "x86: add
multiboot2 protocol support for relocatable images" patch.
This patch changes following things:
- %esi and %r15d registers are used as a storage for Xen image load base
address (%r15d shortly because %rsi is used for EFI SystemTable address
in 64-bit code); both registers are (%esi is mostly) unused in early
boot code and preserved during C functions calls (%esi in 32-bit code
and %r15d in 64-bit code),
- %fs is used as base for Xen data relative addressing in 32-bit code
if it is possible; %esi is used for that thing during error printing
because it is not always possible to properly and efficiently
initialize %fs.
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
v6 - suggestions/fixes:
- leave static mapping of first
16 MiB in l2_identmap as is
(suggested by Jan Beulich),
- use xen_phys_start instead of xen_img_load_base_addr
(suggested by Daniel Kiper and Jan Beulich),
- simplify BOOT_FS segment descriptor
base address initialization
(suggested by Jan Beulich),
- fix BOOT_FS segment limit
(suggested by Jan Beulich),
- do not rename sym_phys in this patch
(suggested by Jan Beulich),
- rename esi_offset/fs_offset to
sym_esi/sym_fs respectively
(suggested by Jan Beulich),
- use add instead of lea in assembly
error printing code
(suggested by Jan Beulich),
- improve comments
(suggested by Jan Beulich),
- improve commit message
(suggested by Jan Beulich),
- various minor cleanups and fixes
(suggested by Jan Beulich).
v4 - suggestions/fixes:
- do not relocate Xen image if boot loader did work for us
(suggested by Andrew Cooper and Jan Beulich),
- initialize xen_img_load_base_addr in EFI boot code too,
- properly initialize trampoline_xen_phys_start,
- calculate Xen image load base address in
x86_64 code ourselves,
(suggested by Jan Beulich),
- change how and when Xen image base address is printed,
- use %fs instead of %esi for relative addressing
(suggested by Andrew Cooper and Jan Beulich),
- create esi_offset and fs_offset() macros in assembly,
- calculate <final-exec-addr> mkelf32 argument automatically,
- optimize and cleanup code,
- improve comments,
- improve commit message.
v3 - suggestions/fixes:
- improve segment registers initialization
(suggested by Jan Beulich),
- simplify Xen image load base address calculation
(suggested by Jan Beulich),
- use %esi and %r15d instead of %ebp to store
Xen image load base address,
- use %esi instead of %fs for relative addressing;
this way we get shorter and simpler code,
- rename some variables and constants
(suggested by Jan Beulich),
- improve comments
(suggested by Konrad Rzeszutek Wilk),
- improve commit message
(suggested by Jan Beulich).
---
xen/arch/x86/boot/head.S | 163 +++++++++++++++++++++++++++++--------
xen/arch/x86/boot/trampoline.S | 5 ++
xen/arch/x86/boot/x86_64.S | 26 +++---
xen/arch/x86/setup.c | 14 ++--
xen/arch/x86/x86_64/asm-offsets.c | 3 +
xen/include/asm-x86/page.h | 2 +-
6 files changed, 157 insertions(+), 56 deletions(-)
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index c301464..9b1fd0e 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -13,12 +13,15 @@
.code32
#define sym_phys(sym) ((sym) - __XEN_VIRT_START)
+#define sym_esi(sym) sym_phys(sym)(%esi)
+#define sym_fs(sym) %fs:sym_phys(sym)
#define BOOT_CS32 0x0008
#define BOOT_CS64 0x0010
#define BOOT_DS 0x0018
#define BOOT_PSEUDORM_CS 0x0020
#define BOOT_PSEUDORM_DS 0x0028
+#define BOOT_FS 0x0030
#define MB2_HT(name) (MULTIBOOT2_HEADER_TAG_##name)
#define MB2_TT(name) (MULTIBOOT2_TAG_TYPE_##name)
@@ -105,7 +108,8 @@ multiboot2_header_start:
.word 0
gdt_boot_descr:
- .word 6*8-1
+ .word 7*8-1
+gdt_boot_base:
.long sym_phys(trampoline_gdt)
.long 0 /* Needed for 64-bit lgdt */
@@ -127,27 +131,27 @@ vga_text_buffer:
.section .init.text, "ax", @progbits
bad_cpu:
- mov $(sym_phys(.Lbad_cpu_msg)),%esi # Error message
+ add $sym_phys(.Lbad_cpu_msg),%esi # Error message
jmp .Lget_vtb
not_multiboot:
- mov $(sym_phys(.Lbad_ldr_msg)),%esi # Error message
+ add $sym_phys(.Lbad_ldr_msg),%esi # Error message
jmp .Lget_vtb
.Lmb2_no_st:
- mov $(sym_phys(.Lbad_ldr_nst)),%esi # Error message
+ add $sym_phys(.Lbad_ldr_nst),%esi # Error message
jmp .Lget_vtb
.Lmb2_no_ih:
- mov $(sym_phys(.Lbad_ldr_nih)),%esi # Error message
+ add $sym_phys(.Lbad_ldr_nih),%esi # Error message
jmp .Lget_vtb
.Lmb2_no_bs:
- mov $(sym_phys(.Lbad_ldr_nbs)),%esi # Error message
+ add $sym_phys(.Lbad_ldr_nbs),%esi # Error message
xor %edi,%edi # No VGA text buffer
jmp .Lsend_chr
.Lmb2_efi_ia_32:
- mov $(sym_phys(.Lbad_efi_msg)),%esi # Error message
+ add $sym_phys(.Lbad_efi_msg),%esi # Error message
xor %edi,%edi # No VGA text buffer
jmp .Lsend_chr
.Lget_vtb:
- mov sym_phys(vga_text_buffer),%edi
+ mov sym_esi(vga_text_buffer),%edi
.Lsend_chr:
mov (%esi),%bl
test %bl,%bl # Terminate on '\0' sentinel
@@ -176,6 +180,9 @@ __efi64_start:
/* VGA is not available on EFI platforms. */
movl $0,vga_text_buffer(%rip)
+ /* Load Xen image load base address. */
+ lea __image_base__(%rip),%r15d
+
/* Check for Multiboot2 bootloader. */
cmp $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
je .Lefi_multiboot2_proto
@@ -299,6 +306,9 @@ run_bs:
pop %rax
+ /* Store Xen image load base address in place accessible for 32-bit code. */
+ mov %r15d,%esi
+
/* Jump to trampoline_setup after switching CPU to x86_32 mode. */
lea trampoline_setup(%rip),%edi
@@ -306,9 +316,11 @@ x86_32_switch:
cli
/* Initialize GDTR. */
+ add %esi,gdt_boot_base(%rip)
lgdt gdt_boot_descr(%rip)
/* Reload code selector. */
+ add %esi,cs32_switch_addr(%rip)
ljmpl *cs32_switch_addr(%rip)
.code32
@@ -338,12 +350,8 @@ __start:
cld
cli
- /* Initialise GDT and basic data segments. */
- lgdt %cs:sym_phys(gdt_boot_descr)
- mov $BOOT_DS,%ecx
- mov %ecx,%ds
- mov %ecx,%es
- mov %ecx,%ss
+ /* Load default Xen image load base address. */
+ mov $sym_phys(__image_base__),%esi
/* Bootloaders may set multiboot{1,2}.mem_lower to a nonzero value. */
xor %edx,%edx
@@ -399,6 +407,25 @@ __start:
jmp 0b
trampoline_bios_setup:
+ /*
+ * Called on legacy BIOS platforms only.
+ *
+ * Initialize GDTR and basic data segments.
+ */
+ add %esi,sym_esi(gdt_boot_base)
+ lgdt sym_esi(gdt_boot_descr)
+
+ mov $BOOT_DS,%ecx
+ mov %ecx,%ds
+ mov %ecx,%es
+ mov %ecx,%ss
+ /* %esp is initialized later. */
+
+ /* Load null descriptor to unused segment registers. */
+ xor %ecx,%ecx
+ mov %ecx,%fs
+ mov %ecx,%gs
+
/* Set up trampoline segment 64k below EBDA */
movzwl 0x40e,%ecx /* EBDA segment */
cmp $0xa000,%ecx /* sanity check (high) */
@@ -426,32 +453,65 @@ trampoline_bios_setup:
xor %cl, %cl
trampoline_setup:
+ /*
+ * Called on legacy BIOS and EFI platforms.
+ *
+ * Initialize 0-15 bits of BOOT_FS segment descriptor base address.
+ */
+ mov %si,BOOT_FS+2+sym_esi(trampoline_gdt)
+
+ /* Initialize 16-23 bits of BOOT_FS segment descriptor base address. */
+ mov %esi,%edx
+ shr $16,%edx
+ mov %dl,BOOT_FS+4+sym_esi(trampoline_gdt)
+
+ /* Initialize 24-31 bits of BOOT_FS segment descriptor base address. */
+ mov %dh,BOOT_FS+7+sym_esi(trampoline_gdt)
+
+ /*
+ * Initialize %fs and later use it to access Xen data if possible.
+ * According to Intel 64 and IA-32 Architectures Software Developer's
+ * Manual it is safe to do that without reloading GDTR before.
+ */
+ mov $BOOT_FS,%edx
+ mov %edx,%fs
+
/* Save trampoline address for later use. */
shl $4, %ecx
- mov %ecx,sym_phys(trampoline_phys)
+ mov %ecx,sym_fs(trampoline_phys)
+
+ /* Save Xen image load base address for later use. */
+ mov %esi,sym_fs(xen_phys_start)
+ mov %esi,sym_fs(trampoline_xen_phys_start)
+
+ /* Setup stack. %ss was initialized earlier. */
+ lea 1024+sym_esi(cpu0_stack),%esp
/* Save the Multiboot info struct (after relocation) for later use. */
- mov $sym_phys(cpu0_stack)+1024,%esp
push %ecx /* Boot trampoline address. */
push %ebx /* Multiboot information address. */
push %eax /* Multiboot magic. */
call reloc
- mov %eax,sym_phys(multiboot_ptr)
+ mov %eax,sym_fs(multiboot_ptr)
/*
* Do not zero BSS on EFI platform here.
* It was initialized earlier.
*/
- cmpb $0,sym_phys(skip_realmode)
+ cmpb $0,sym_fs(skip_realmode)
jnz 1f
/* Initialize BSS (no nasty surprises!). */
mov $sym_phys(__bss_start),%edi
mov $sym_phys(__bss_end),%ecx
+ push %fs
+ pop %es
sub %edi,%ecx
xor %eax,%eax
shr $2,%ecx
rep stosl
+ push %ds
+ pop %es
1:
/* Interrogate CPU extended features via CPUID. */
@@ -465,8 +525,8 @@ trampoline_setup:
jbe 1f
mov $0x80000001,%eax
cpuid
-1: mov %edx,sym_phys(cpuid_ext_features)
- mov %edx,sym_phys(boot_cpu_data)+CPUINFO_FEATURE_OFFSET(X86_FEATURE_LM)
+1: mov %edx,sym_fs(cpuid_ext_features)
+ mov %edx,sym_fs(boot_cpu_data)+CPUINFO_FEATURE_OFFSET(X86_FEATURE_LM)
/* Check for availability of long mode. */
bt $cpufeat_bit(X86_FEATURE_LM),%edx
@@ -474,23 +534,53 @@ trampoline_setup:
/* Stash TSC to calculate a good approximation of time-since-boot */
rdtsc
- mov %eax,sym_phys(boot_tsc_stamp)
- mov %edx,sym_phys(boot_tsc_stamp+4)
+ mov %eax,sym_fs(boot_tsc_stamp)
+ mov %edx,sym_fs(boot_tsc_stamp)+4
+
+ /*
+ * Update frame addresses in page tables excluding l2_identmap
+ * without its first entry which points to l1_identmap.
+ */
+ mov $((__page_tables_end-__page_tables_start)/8),%ecx
+ mov $(((l2_identmap-__page_tables_start)/8)+2),%edx
+1: cmp $((l2_identmap+l2_identmap_sizeof-__page_tables_start)/8),%ecx
+ cmove %edx,%ecx
+ je 2f
+ testl $_PAGE_PRESENT,sym_fs(__page_tables_start)-8(,%ecx,8)
+ jz 2f
+ add %esi,sym_fs(__page_tables_start)-8(,%ecx,8)
+2: loop 1b
+
+ /* Initialize L2 boot-map/direct map page table entries (14MB). */
+ lea sym_esi(start),%ebx
+ lea (1<<L2_PAGETABLE_SHIFT)*7+(PAGE_HYPERVISOR|_PAGE_PSE)(%ebx),%eax
+ shr $(L2_PAGETABLE_SHIFT-3),%ebx
+ mov $8,%ecx
+1: mov %eax,sym_fs(l2_bootmap)-8(%ebx,%ecx,8)
+ mov %eax,sym_fs(l2_identmap)-8(%ebx,%ecx,8)
+ sub $(1<<L2_PAGETABLE_SHIFT),%eax
+ loop 1b
+
+ /* Initialize L3 boot-map page directory entry. */
+ lea __PAGE_HYPERVISOR+(L2_PAGETABLE_ENTRIES*8)*3+sym_esi(l2_bootmap),%eax
+ mov $4,%ecx
+1: mov %eax,sym_fs(l3_bootmap)-8(,%ecx,8)
+ sub $(L2_PAGETABLE_ENTRIES*8),%eax
+ loop 1b
/*
* During boot, hook 4kB mappings of first 2MB of memory into L2.
- * This avoids mixing cachability for the legacy VGA region, and is
- * corrected when Xen relocates itself.
+ * This avoids mixing cachability for the legacy VGA region.
*/
- mov $sym_phys(l1_identmap)+__PAGE_HYPERVISOR,%edi
- mov %edi,sym_phys(l2_xenmap)
+ lea __PAGE_HYPERVISOR+sym_esi(l1_identmap),%edi
+ mov %edi,sym_fs(l2_bootmap)
/* Apply relocations to bootstrap trampoline. */
- mov sym_phys(trampoline_phys),%edx
+ mov sym_fs(trampoline_phys),%edx
mov $sym_phys(__trampoline_rel_start),%edi
1:
- mov (%edi),%eax
- add %edx,(%edi,%eax)
+ mov %fs:(%edi),%eax
+ add %edx,%fs:(%edi,%eax)
add $4,%edi
cmp $sym_phys(__trampoline_rel_stop),%edi
jb 1b
@@ -499,28 +589,29 @@ trampoline_setup:
shr $4,%edx
mov $sym_phys(__trampoline_seg_start),%edi
1:
- mov (%edi),%eax
- mov %dx,(%edi,%eax)
+ mov %fs:(%edi),%eax
+ mov %dx,%fs:(%edi,%eax)
add $4,%edi
cmp $sym_phys(__trampoline_seg_stop),%edi
jb 1b
/* Do not parse command line on EFI platform here. */
- cmpb $0,sym_phys(skip_realmode)
+ cmpb $0,sym_fs(skip_realmode)
jnz 1f
/* Bail if there is no command line to parse. */
- mov sym_phys(multiboot_ptr),%ebx
+ mov sym_fs(multiboot_ptr),%ebx
testl $MBI_CMDLINE,MB_flags(%ebx)
jz 1f
- pushl $sym_phys(early_boot_opts)
+ lea sym_esi(early_boot_opts),%eax
+ push %eax
pushl MB_cmdline(%ebx)
call cmdline_parse_early
1:
/* Switch to low-memory stack. */
- mov sym_phys(trampoline_phys),%edi
+ mov sym_fs(trampoline_phys),%edi
lea 0x10000(%edi),%esp
lea trampoline_boot_cpu_entry-trampoline_start(%edi),%eax
pushl $BOOT_CS32
@@ -529,7 +620,7 @@ trampoline_setup:
/* Copy bootstrap trampoline to low memory, below 1MB. */
mov $sym_phys(trampoline_start),%esi
mov $((trampoline_end - trampoline_start) / 4),%ecx
- rep movsl
+ rep movsl %fs:(%esi),%es:(%edi)
/* Jump into the relocated trampoline. */
lret
diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S
index 2715d17..64f8efd 100644
--- a/xen/arch/x86/boot/trampoline.S
+++ b/xen/arch/x86/boot/trampoline.S
@@ -54,6 +54,11 @@ trampoline_gdt:
/* 0x0028: real-mode data @ BOOT_TRAMPOLINE */
.long 0x0000ffff
.long 0x00009200
+ /*
+ * 0x0030: ring 0 Xen data, 16 MiB size, base
+ * address is computed during runtime.
+ */
+ .quad 0x00c0920000000fff
.pushsection .trampoline_rel, "a"
.long trampoline_gdt + BOOT_PSEUDORM_CS + 2 - .
diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index 139b2ca..7392004 100644
--- a/xen/arch/x86/boot/x86_64.S
+++ b/xen/arch/x86/boot/x86_64.S
@@ -81,7 +81,6 @@ GLOBAL(boot_cpu_compat_gdt_table)
.quad 0x0000910000000000 /* per-CPU entry (limit == cpu) */
.align PAGE_SIZE, 0
-GLOBAL(__page_tables_start)
/*
* Mapping of first 2 megabytes of memory. This is mapped with 4kB mappings
* to avoid type conflicts with fixed-range MTRRs covering the lowest megabyte
@@ -102,6 +101,13 @@ l1_identmap:
.size l1_identmap, . - l1_identmap
/*
+ * __page_tables_start does not cover l1_identmap because it (l1_identmap)
+ * contains 1-1 mappings. This means that frame addresses of these mappings
+ * are static and should not be updated during runtime.
+ */
+GLOBAL(__page_tables_start)
+
+/*
* Space for mapping the first 4GB of memory, with the first 16 megabytes
* actualy mapped (mostly using superpages). Uses 4x 4k pages.
*/
@@ -121,8 +127,9 @@ GLOBAL(l2_identmap)
* page.
*/
GLOBAL(l2_xenmap)
- idx = 0
- .rept 8
+ .quad 0
+ idx = 1
+ .rept 7
.quad sym_phys(__image_base__) + (idx << L2_PAGETABLE_SHIFT) + (PAGE_HYPERVISOR | _PAGE_PSE)
idx = idx + 1
.endr
@@ -185,21 +192,14 @@ GLOBAL(idle_pg_table)
GLOBAL(__page_tables_end)
-/* Init pagetables. Enough page directories to map into the bottom 1GB. */
+/* Init pagetables. Enough page directories to map into 4GB. */
.section .init.data, "a", @progbits
.align PAGE_SIZE, 0
GLOBAL(l2_bootmap)
- .quad sym_phys(l1_identmap) + __PAGE_HYPERVISOR
- idx = 1
- .rept 7
- .quad (idx << L2_PAGETABLE_SHIFT) | __PAGE_HYPERVISOR | _PAGE_PSE
- idx = idx + 1
- .endr
- .fill L2_PAGETABLE_ENTRIES - 8, 8, 0
+ .fill 4 * L2_PAGETABLE_ENTRIES, 8, 0
.size l2_bootmap, . - l2_bootmap
GLOBAL(l3_bootmap)
- .quad sym_phys(l2_bootmap) + __PAGE_HYPERVISOR
- .fill L3_PAGETABLE_ENTRIES - 1, 8, 0
+ .fill L3_PAGETABLE_ENTRIES, 8, 0
.size l3_bootmap, . - l3_bootmap
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index ca1a97a..e6e05f9 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -286,9 +286,6 @@ static void *__init bootstrap_map(const module_t *mod)
if ( start >= end )
return NULL;
- if ( end <= BOOTSTRAP_MAP_BASE )
- return (void *)(unsigned long)start;
-
ret = (void *)(map_cur + (unsigned long)(start & mask));
start &= ~mask;
end = (end + mask) & ~mask;
@@ -674,6 +671,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
printk("Command line: %s\n", cmdline);
+ printk("Xen image load base address: 0x%08lx\n", xen_phys_start);
+
printk("Video information:\n");
/* Print VGA display mode information. */
@@ -931,7 +930,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
/* Not present, 1GB mapping, or already relocated? */
if ( !(l3e_get_flags(*pl3e) & _PAGE_PRESENT) ||
(l3e_get_flags(*pl3e) & _PAGE_PSE) ||
- (l3e_get_pfn(*pl3e) > 0x1000) )
+ (l3e_get_pfn(*pl3e) > PFN_DOWN(xen_phys_start)) )
continue;
*pl3e = l3e_from_intpte(l3e_get_intpte(*pl3e) +
xen_phys_start);
@@ -941,7 +940,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
/* Not present, PSE, or already relocated? */
if ( !(l2e_get_flags(*pl2e) & _PAGE_PRESENT) ||
(l2e_get_flags(*pl2e) & _PAGE_PSE) ||
- (l2e_get_pfn(*pl2e) > 0x1000) )
+ (l2e_get_pfn(*pl2e) > PFN_DOWN(xen_phys_start)) )
continue;
*pl2e = l2e_from_intpte(l2e_get_intpte(*pl2e) +
xen_phys_start);
@@ -964,7 +963,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
{
unsigned int flags;
- if ( !(l2e_get_flags(*pl2e) & _PAGE_PRESENT) )
+ if ( !(l2e_get_flags(*pl2e) & _PAGE_PRESENT) ||
+ (l2e_get_pfn(*pl2e) > PFN_DOWN(xen_phys_start)) )
continue;
if ( !using_2M_mapping() )
@@ -1018,6 +1018,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
: "memory" );
bootstrap_map(NULL);
+
+ printk("New Xen image base address: %#08lx\n", xen_phys_start);
}
/* Is the region suitable for relocating the multiboot modules? */
diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c
index 2a22659..5d7a8e5 100644
--- a/xen/arch/x86/x86_64/asm-offsets.c
+++ b/xen/arch/x86/x86_64/asm-offsets.c
@@ -179,5 +179,8 @@ void __dummy__(void)
OFFSET(MB2_efi64_ih, multiboot2_tag_efi64_ih_t, pointer);
BLANK();
+ DEFINE(l2_identmap_sizeof, sizeof(l2_identmap));
+ BLANK();
+
OFFSET(DOMAIN_vm_assist, struct domain, vm_assist);
}
diff --git a/xen/include/asm-x86/page.h b/xen/include/asm-x86/page.h
index af7d3e8..a54fdd1 100644
--- a/xen/include/asm-x86/page.h
+++ b/xen/include/asm-x86/page.h
@@ -288,7 +288,7 @@ extern root_pgentry_t idle_pg_table[ROOT_PAGETABLE_ENTRIES];
extern l2_pgentry_t *compat_idle_pg_table_l2;
extern unsigned int m2p_compat_vstart;
extern l2_pgentry_t l2_xenmap[L2_PAGETABLE_ENTRIES],
- l2_bootmap[L2_PAGETABLE_ENTRIES];
+ l2_bootmap[4*L2_PAGETABLE_ENTRIES];
extern l3_pgentry_t l3_bootmap[L3_PAGETABLE_ENTRIES];
extern l2_pgentry_t l2_identmap[4*L2_PAGETABLE_ENTRIES];
extern l1_pgentry_t l1_fixmap[L1_PAGETABLE_ENTRIES];
--
1.7.10.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v7 12/14] x86: make Xen early boot code relocatable
2016-09-23 21:47 ` [PATCH v7 12/14] x86: make Xen early boot code relocatable Daniel Kiper
@ 2016-09-26 15:03 ` Jan Beulich
2016-09-27 19:55 ` Daniel Kiper
0 siblings, 1 reply; 51+ messages in thread
From: Jan Beulich @ 2016-09-26 15:03 UTC (permalink / raw)
To: Daniel Kiper
Cc: Juergen Gross, sstabellini, andrew.cooper3, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
>>> On 23.09.16 at 23:47, <daniel.kiper@oracle.com> wrote:
> @@ -426,32 +453,65 @@ trampoline_bios_setup:
> xor %cl, %cl
>
> trampoline_setup:
> + /*
> + * Called on legacy BIOS and EFI platforms.
> + *
> + * Initialize 0-15 bits of BOOT_FS segment descriptor base address.
> + */
> + mov %si,BOOT_FS+2+sym_esi(trampoline_gdt)
> +
> + /* Initialize 16-23 bits of BOOT_FS segment descriptor base address. */
> + mov %esi,%edx
> + shr $16,%edx
I'd have liked it even better if you had done this with a single
instruction, but anyway.
> @@ -474,23 +534,53 @@ trampoline_setup:
>
> /* Stash TSC to calculate a good approximation of time-since-boot */
> rdtsc
> - mov %eax,sym_phys(boot_tsc_stamp)
> - mov %edx,sym_phys(boot_tsc_stamp+4)
> + mov %eax,sym_fs(boot_tsc_stamp)
> + mov %edx,sym_fs(boot_tsc_stamp)+4
> +
> + /*
> + * Update frame addresses in page tables excluding l2_identmap
> + * without its first entry which points to l1_identmap.
> + */
> + mov $((__page_tables_end-__page_tables_start)/8),%ecx
> + mov $(((l2_identmap-__page_tables_start)/8)+2),%edx
The +2 instead of +1 here is confusing. Why don't you do the natural
thing here and ...
> +1: cmp $((l2_identmap+l2_identmap_sizeof-__page_tables_start)/8),%ecx
> + cmove %edx,%ecx
> + je 2f
... simply drop this branch?
> + testl $_PAGE_PRESENT,sym_fs(__page_tables_start)-8(,%ecx,8)
> + jz 2f
> + add %esi,sym_fs(__page_tables_start)-8(,%ecx,8)
> +2: loop 1b
> +
> + /* Initialize L2 boot-map/direct map page table entries (14MB). */
> + lea sym_esi(start),%ebx
> + lea (1<<L2_PAGETABLE_SHIFT)*7+(PAGE_HYPERVISOR|_PAGE_PSE)(%ebx),%eax
> + shr $(L2_PAGETABLE_SHIFT-3),%ebx
> + mov $8,%ecx
The comment saying 14Mb kind of contradicts this being 8 here.
> +1: mov %eax,sym_fs(l2_bootmap)-8(%ebx,%ecx,8)
> + mov %eax,sym_fs(l2_identmap)-8(%ebx,%ecx,8)
> + sub $(1<<L2_PAGETABLE_SHIFT),%eax
> + loop 1b
> +
> + /* Initialize L3 boot-map page directory entry. */
> + lea __PAGE_HYPERVISOR+(L2_PAGETABLE_ENTRIES*8)*3+sym_esi(l2_bootmap),%eax
> + mov $4,%ecx
> +1: mov %eax,sym_fs(l3_bootmap)-8(,%ecx,8)
> + sub $(L2_PAGETABLE_ENTRIES*8),%eax
> + loop 1b
>
> /*
> * During boot, hook 4kB mappings of first 2MB of memory into L2.
> - * This avoids mixing cachability for the legacy VGA region, and is
> - * corrected when Xen relocates itself.
> + * This avoids mixing cachability for the legacy VGA region.
> */
> - mov $sym_phys(l1_identmap)+__PAGE_HYPERVISOR,%edi
> - mov %edi,sym_phys(l2_xenmap)
> + lea __PAGE_HYPERVISOR+sym_esi(l1_identmap),%edi
> + mov %edi,sym_fs(l2_bootmap)
Switching from l2_xenmap to l2_bootmap here?
> @@ -121,8 +127,9 @@ GLOBAL(l2_identmap)
> * page.
> */
> GLOBAL(l2_xenmap)
> - idx = 0
> - .rept 8
> + .quad 0
> + idx = 1
> + .rept 7
> .quad sym_phys(__image_base__) + (idx << L2_PAGETABLE_SHIFT) + (PAGE_HYPERVISOR | _PAGE_PSE)
> idx = idx + 1
> .endr
How come the first entry doesn't need filling anymore? I think such
less obvious changes should really get briefly mentioned/explained
in the commit message.
> @@ -674,6 +671,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>
> printk("Command line: %s\n", cmdline);
>
> + printk("Xen image load base address: 0x%08lx\n", xen_phys_start);
Please prefer %#lx in cases like this.
> @@ -1018,6 +1018,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> : "memory" );
>
> bootstrap_map(NULL);
> +
> + printk("New Xen image base address: %#08lx\n", xen_phys_start);
# and a minimum width generally don't fit together well.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v7 12/14] x86: make Xen early boot code relocatable
2016-09-26 15:03 ` Jan Beulich
@ 2016-09-27 19:55 ` Daniel Kiper
2016-09-28 9:06 ` Jan Beulich
0 siblings, 1 reply; 51+ messages in thread
From: Daniel Kiper @ 2016-09-27 19:55 UTC (permalink / raw)
To: Jan Beulich
Cc: Juergen Gross, sstabellini, andrew.cooper3, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
On Mon, Sep 26, 2016 at 09:03:30AM -0600, Jan Beulich wrote:
> >>> On 23.09.16 at 23:47, <daniel.kiper@oracle.com> wrote:
> > @@ -426,32 +453,65 @@ trampoline_bios_setup:
> > xor %cl, %cl
> >
> > trampoline_setup:
> > + /*
> > + * Called on legacy BIOS and EFI platforms.
> > + *
> > + * Initialize 0-15 bits of BOOT_FS segment descriptor base address.
> > + */
> > + mov %si,BOOT_FS+2+sym_esi(trampoline_gdt)
> > +
> > + /* Initialize 16-23 bits of BOOT_FS segment descriptor base address. */
> > + mov %esi,%edx
> > + shr $16,%edx
>
> I'd have liked it even better if you had done this with a single
> instruction, but anyway.
Do you think about "shld $16,%esi,%edx"?
> > @@ -474,23 +534,53 @@ trampoline_setup:
> >
> > /* Stash TSC to calculate a good approximation of time-since-boot */
> > rdtsc
> > - mov %eax,sym_phys(boot_tsc_stamp)
> > - mov %edx,sym_phys(boot_tsc_stamp+4)
> > + mov %eax,sym_fs(boot_tsc_stamp)
> > + mov %edx,sym_fs(boot_tsc_stamp)+4
> > +
> > + /*
> > + * Update frame addresses in page tables excluding l2_identmap
> > + * without its first entry which points to l1_identmap.
> > + */
> > + mov $((__page_tables_end-__page_tables_start)/8),%ecx
> > + mov $(((l2_identmap-__page_tables_start)/8)+2),%edx
>
> The +2 instead of +1 here is confusing. Why don't you do the natural
> thing here and ...
>
> > +1: cmp $((l2_identmap+l2_identmap_sizeof-__page_tables_start)/8),%ecx
> > + cmove %edx,%ecx
> > + je 2f
>
> ... simply drop this branch?
Make sense. I will do that.
> > + testl $_PAGE_PRESENT,sym_fs(__page_tables_start)-8(,%ecx,8)
> > + jz 2f
> > + add %esi,sym_fs(__page_tables_start)-8(,%ecx,8)
> > +2: loop 1b
> > +
> > + /* Initialize L2 boot-map/direct map page table entries (14MB). */
> > + lea sym_esi(start),%ebx
> > + lea (1<<L2_PAGETABLE_SHIFT)*7+(PAGE_HYPERVISOR|_PAGE_PSE)(%ebx),%eax
> > + shr $(L2_PAGETABLE_SHIFT-3),%ebx
> > + mov $8,%ecx
>
> The comment saying 14Mb kind of contradicts this being 8 here.
Ahhh... Right, comment is wrong.
> > +1: mov %eax,sym_fs(l2_bootmap)-8(%ebx,%ecx,8)
> > + mov %eax,sym_fs(l2_identmap)-8(%ebx,%ecx,8)
> > + sub $(1<<L2_PAGETABLE_SHIFT),%eax
> > + loop 1b
> > +
> > + /* Initialize L3 boot-map page directory entry. */
> > + lea __PAGE_HYPERVISOR+(L2_PAGETABLE_ENTRIES*8)*3+sym_esi(l2_bootmap),%eax
> > + mov $4,%ecx
> > +1: mov %eax,sym_fs(l3_bootmap)-8(,%ecx,8)
> > + sub $(L2_PAGETABLE_ENTRIES*8),%eax
> > + loop 1b
> >
> > /*
> > * During boot, hook 4kB mappings of first 2MB of memory into L2.
> > - * This avoids mixing cachability for the legacy VGA region, and is
> > - * corrected when Xen relocates itself.
> > + * This avoids mixing cachability for the legacy VGA region.
> > */
> > - mov $sym_phys(l1_identmap)+__PAGE_HYPERVISOR,%edi
> > - mov %edi,sym_phys(l2_xenmap)
> > + lea __PAGE_HYPERVISOR+sym_esi(l1_identmap),%edi
> > + mov %edi,sym_fs(l2_bootmap)
>
> Switching from l2_xenmap to l2_bootmap here?
Do we need first 2 MiB mapped in Xen image mapping? It looks that we do not.
Am I missing something?
I must initialize l2_bootmap first entry with l1_identmap here because
I removed relevant static initialization from x86_64.S.
> > @@ -121,8 +127,9 @@ GLOBAL(l2_identmap)
> > * page.
> > */
> > GLOBAL(l2_xenmap)
> > - idx = 0
> > - .rept 8
> > + .quad 0
> > + idx = 1
> > + .rept 7
> > .quad sym_phys(__image_base__) + (idx << L2_PAGETABLE_SHIFT) + (PAGE_HYPERVISOR | _PAGE_PSE)
> > idx = idx + 1
> > .endr
>
> How come the first entry doesn't need filling anymore? I think such
> less obvious changes should really get briefly mentioned/explained
> in the commit message.
Ditto. I will add a few words about that to commit message.
> > @@ -674,6 +671,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >
> > printk("Command line: %s\n", cmdline);
> >
> > + printk("Xen image load base address: 0x%08lx\n", xen_phys_start);
>
> Please prefer %#lx in cases like this.
If I do that then 0 is displayed as 0 instead of 0x00000000. I prefer
latter. If you do not care I can use "%#lx" as you wish.
> > @@ -1018,6 +1018,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> > : "memory" );
> >
> > bootstrap_map(NULL);
> > +
> > + printk("New Xen image base address: %#08lx\n", xen_phys_start);
>
> # and a minimum width generally don't fit together well.
Why?
Daniel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v7 12/14] x86: make Xen early boot code relocatable
2016-09-27 19:55 ` Daniel Kiper
@ 2016-09-28 9:06 ` Jan Beulich
2016-09-28 9:56 ` Daniel Kiper
0 siblings, 1 reply; 51+ messages in thread
From: Jan Beulich @ 2016-09-28 9:06 UTC (permalink / raw)
To: Daniel Kiper
Cc: Juergen Gross, sstabellini, andrew.cooper3, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
>>> On 27.09.16 at 21:55, <daniel.kiper@oracle.com> wrote:
> On Mon, Sep 26, 2016 at 09:03:30AM -0600, Jan Beulich wrote:
>> >>> On 23.09.16 at 23:47, <daniel.kiper@oracle.com> wrote:
>> > @@ -426,32 +453,65 @@ trampoline_bios_setup:
>> > xor %cl, %cl
>> >
>> > trampoline_setup:
>> > + /*
>> > + * Called on legacy BIOS and EFI platforms.
>> > + *
>> > + * Initialize 0-15 bits of BOOT_FS segment descriptor base address.
>> > + */
>> > + mov %si,BOOT_FS+2+sym_esi(trampoline_gdt)
>> > +
>> > + /* Initialize 16-23 bits of BOOT_FS segment descriptor base address. */
>> > + mov %esi,%edx
>> > + shr $16,%edx
>>
>> I'd have liked it even better if you had done this with a single
>> instruction, but anyway.
>
> Do you think about "shld $16,%esi,%edx"?
Yes.
>> > +1: mov %eax,sym_fs(l2_bootmap)-8(%ebx,%ecx,8)
>> > + mov %eax,sym_fs(l2_identmap)-8(%ebx,%ecx,8)
>> > + sub $(1<<L2_PAGETABLE_SHIFT),%eax
>> > + loop 1b
>> > +
>> > + /* Initialize L3 boot-map page directory entry. */
>> > + lea __PAGE_HYPERVISOR+(L2_PAGETABLE_ENTRIES*8)*3+sym_esi(l2_bootmap),%eax
>> > + mov $4,%ecx
>> > +1: mov %eax,sym_fs(l3_bootmap)-8(,%ecx,8)
>> > + sub $(L2_PAGETABLE_ENTRIES*8),%eax
>> > + loop 1b
>> >
>> > /*
>> > * During boot, hook 4kB mappings of first 2MB of memory into L2.
>> > - * This avoids mixing cachability for the legacy VGA region, and is
>> > - * corrected when Xen relocates itself.
>> > + * This avoids mixing cachability for the legacy VGA region.
>> > */
>> > - mov $sym_phys(l1_identmap)+__PAGE_HYPERVISOR,%edi
>> > - mov %edi,sym_phys(l2_xenmap)
>> > + lea __PAGE_HYPERVISOR+sym_esi(l1_identmap),%edi
>> > + mov %edi,sym_fs(l2_bootmap)
>>
>> Switching from l2_xenmap to l2_bootmap here?
>
> Do we need first 2 MiB mapped in Xen image mapping? It looks that we do not.
> Am I missing something?
My point here isn't that the change is wrong, but that it's not
immediately obvious and hence should be explained in the commit
message. After all the need for the mapping of these 2Mb does not
- aiui - go away with this patch, but (presumably) with the earlier one
moving the load address up to 2Mb. I.e. it can generally be viewed as
an independent adjustment.
>> > @@ -674,6 +671,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>> >
>> > printk("Command line: %s\n", cmdline);
>> >
>> > + printk("Xen image load base address: 0x%08lx\n", xen_phys_start);
>>
>> Please prefer %#lx in cases like this.
>
> If I do that then 0 is displayed as 0 instead of 0x00000000. I prefer
> latter. If you do not care I can use "%#lx" as you wish.
Please do - the base address won't ever be zero anyway, and even if
it was "0" instead of "0x00000000" is quite fine when there are no
other lines to align with..
>> > @@ -1018,6 +1018,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>> > : "memory" );
>> >
>> > bootstrap_map(NULL);
>> > +
>> > + printk("New Xen image base address: %#08lx\n", xen_phys_start);
>>
>> # and a minimum width generally don't fit together well.
>
> Why?
Because this will result in e.g. 0x000123 instead of the apparently
expected by you 0x00000123, as the 0x contributes to the field
width.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v7 12/14] x86: make Xen early boot code relocatable
2016-09-28 9:06 ` Jan Beulich
@ 2016-09-28 9:56 ` Daniel Kiper
2016-09-28 10:19 ` Jan Beulich
0 siblings, 1 reply; 51+ messages in thread
From: Daniel Kiper @ 2016-09-28 9:56 UTC (permalink / raw)
To: Jan Beulich
Cc: Juergen Gross, sstabellini, andrew.cooper3, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
On Wed, Sep 28, 2016 at 03:06:31AM -0600, Jan Beulich wrote:
> >>> On 27.09.16 at 21:55, <daniel.kiper@oracle.com> wrote:
> > On Mon, Sep 26, 2016 at 09:03:30AM -0600, Jan Beulich wrote:
> >> >>> On 23.09.16 at 23:47, <daniel.kiper@oracle.com> wrote:
> >> > @@ -426,32 +453,65 @@ trampoline_bios_setup:
> >> > xor %cl, %cl
> >> >
> >> > trampoline_setup:
> >> > + /*
> >> > + * Called on legacy BIOS and EFI platforms.
> >> > + *
> >> > + * Initialize 0-15 bits of BOOT_FS segment descriptor base address.
> >> > + */
> >> > + mov %si,BOOT_FS+2+sym_esi(trampoline_gdt)
> >> > +
> >> > + /* Initialize 16-23 bits of BOOT_FS segment descriptor base address. */
> >> > + mov %esi,%edx
> >> > + shr $16,%edx
> >>
> >> I'd have liked it even better if you had done this with a single
> >> instruction, but anyway.
> >
> > Do you think about "shld $16,%esi,%edx"?
>
> Yes.
By the way, my 12 years old daughter told me how to do this...
Daddy, just mix all arguments all together. And later when
I showed her final solution she said: I told you... :-)))
> >> > +1: mov %eax,sym_fs(l2_bootmap)-8(%ebx,%ecx,8)
> >> > + mov %eax,sym_fs(l2_identmap)-8(%ebx,%ecx,8)
> >> > + sub $(1<<L2_PAGETABLE_SHIFT),%eax
> >> > + loop 1b
> >> > +
> >> > + /* Initialize L3 boot-map page directory entry. */
> >> > + lea __PAGE_HYPERVISOR+(L2_PAGETABLE_ENTRIES*8)*3+sym_esi(l2_bootmap),%eax
> >> > + mov $4,%ecx
> >> > +1: mov %eax,sym_fs(l3_bootmap)-8(,%ecx,8)
> >> > + sub $(L2_PAGETABLE_ENTRIES*8),%eax
> >> > + loop 1b
> >> >
> >> > /*
> >> > * During boot, hook 4kB mappings of first 2MB of memory into L2.
> >> > - * This avoids mixing cachability for the legacy VGA region, and is
> >> > - * corrected when Xen relocates itself.
> >> > + * This avoids mixing cachability for the legacy VGA region.
> >> > */
> >> > - mov $sym_phys(l1_identmap)+__PAGE_HYPERVISOR,%edi
> >> > - mov %edi,sym_phys(l2_xenmap)
> >> > + lea __PAGE_HYPERVISOR+sym_esi(l1_identmap),%edi
> >> > + mov %edi,sym_fs(l2_bootmap)
> >>
> >> Switching from l2_xenmap to l2_bootmap here?
> >
> > Do we need first 2 MiB mapped in Xen image mapping? It looks that we do not.
> > Am I missing something?
>
> My point here isn't that the change is wrong, but that it's not
> immediately obvious and hence should be explained in the commit
> message. After all the need for the mapping of these 2Mb does not
> - aiui - go away with this patch, but (presumably) with the earlier one
> moving the load address up to 2Mb. I.e. it can generally be viewed as
> an independent adjustment.
Probably. Should I move this change to patch #10 (x86: change default load
address from 1 MiB to 2 MiB) or leave it here with relevant comment?
Or do you wish separate patch for it?
> >> > @@ -674,6 +671,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >> >
> >> > printk("Command line: %s\n", cmdline);
> >> >
> >> > + printk("Xen image load base address: 0x%08lx\n", xen_phys_start);
> >>
> >> Please prefer %#lx in cases like this.
> >
> > If I do that then 0 is displayed as 0 instead of 0x00000000. I prefer
> > latter. If you do not care I can use "%#lx" as you wish.
>
> Please do - the base address won't ever be zero anyway, and even if
Here it can be. It happens if Xen image is loaded by boot loader without
relocation, e.g. via multiboot (v1) protocol.
> it was "0" instead of "0x00000000" is quite fine when there are no
> other lines to align with..
>
> >> > @@ -1018,6 +1018,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >> > : "memory" );
> >> >
> >> > bootstrap_map(NULL);
> >> > +
> >> > + printk("New Xen image base address: %#08lx\n", xen_phys_start);
> >>
> >> # and a minimum width generally don't fit together well.
> >
> > Why?
>
> Because this will result in e.g. 0x000123 instead of the apparently
> expected by you 0x00000123, as the 0x contributes to the field
> width.
Yep, this is a bit confusing.
Daniel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v7 12/14] x86: make Xen early boot code relocatable
2016-09-28 9:56 ` Daniel Kiper
@ 2016-09-28 10:19 ` Jan Beulich
0 siblings, 0 replies; 51+ messages in thread
From: Jan Beulich @ 2016-09-28 10:19 UTC (permalink / raw)
To: Daniel Kiper
Cc: Juergen Gross, sstabellini, andrew.cooper3, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
>>> On 28.09.16 at 11:56, <daniel.kiper@oracle.com> wrote:
> On Wed, Sep 28, 2016 at 03:06:31AM -0600, Jan Beulich wrote:
>> >>> On 27.09.16 at 21:55, <daniel.kiper@oracle.com> wrote:
>> > On Mon, Sep 26, 2016 at 09:03:30AM -0600, Jan Beulich wrote:
>> >> >>> On 23.09.16 at 23:47, <daniel.kiper@oracle.com> wrote:
>> >> > +1: mov %eax,sym_fs(l2_bootmap)-8(%ebx,%ecx,8)
>> >> > + mov %eax,sym_fs(l2_identmap)-8(%ebx,%ecx,8)
>> >> > + sub $(1<<L2_PAGETABLE_SHIFT),%eax
>> >> > + loop 1b
>> >> > +
>> >> > + /* Initialize L3 boot-map page directory entry. */
>> >> > + lea __PAGE_HYPERVISOR+(L2_PAGETABLE_ENTRIES*8)*3+sym_esi(l2_bootmap),%eax
>> >> > + mov $4,%ecx
>> >> > +1: mov %eax,sym_fs(l3_bootmap)-8(,%ecx,8)
>> >> > + sub $(L2_PAGETABLE_ENTRIES*8),%eax
>> >> > + loop 1b
>> >> >
>> >> > /*
>> >> > * During boot, hook 4kB mappings of first 2MB of memory into L2.
>> >> > - * This avoids mixing cachability for the legacy VGA region, and is
>> >> > - * corrected when Xen relocates itself.
>> >> > + * This avoids mixing cachability for the legacy VGA region.
>> >> > */
>> >> > - mov $sym_phys(l1_identmap)+__PAGE_HYPERVISOR,%edi
>> >> > - mov %edi,sym_phys(l2_xenmap)
>> >> > + lea __PAGE_HYPERVISOR+sym_esi(l1_identmap),%edi
>> >> > + mov %edi,sym_fs(l2_bootmap)
>> >>
>> >> Switching from l2_xenmap to l2_bootmap here?
>> >
>> > Do we need first 2 MiB mapped in Xen image mapping? It looks that we do not.
>> > Am I missing something?
>>
>> My point here isn't that the change is wrong, but that it's not
>> immediately obvious and hence should be explained in the commit
>> message. After all the need for the mapping of these 2Mb does not
>> - aiui - go away with this patch, but (presumably) with the earlier one
>> moving the load address up to 2Mb. I.e. it can generally be viewed as
>> an independent adjustment.
>
> Probably. Should I move this change to patch #10 (x86: change default load
> address from 1 MiB to 2 MiB) or leave it here with relevant comment?
> Or do you wish separate patch for it?
I think putting it in the other patch would be best, unless it would
end up as non-obvious as it did here, in which case a separate
patch would probably be better.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v7 13/14] x86/boot: rename sym_phys() to sym_offs()
2016-09-23 21:47 [PATCH v7 00/14] x86: multiboot2 protocol support Daniel Kiper
` (11 preceding siblings ...)
2016-09-23 21:47 ` [PATCH v7 12/14] x86: make Xen early boot code relocatable Daniel Kiper
@ 2016-09-23 21:47 ` Daniel Kiper
2016-09-26 15:47 ` Jan Beulich
2016-09-23 21:47 ` [PATCH v7 14/14] x86: add multiboot2 protocol support for relocatable images Daniel Kiper
13 siblings, 1 reply; 51+ messages in thread
From: Daniel Kiper @ 2016-09-23 21:47 UTC (permalink / raw)
To: xen-devel
Cc: jgross, sstabellini, andrew.cooper3, cardoe, pgnet.dev, ning.sun,
julien.grall, david.vrabel, jbeulich, qiaowei.ren, gang.wei,
fu.wei
This way macro name better describes its function.
There is no functional change.
Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
xen/arch/x86/boot/head.S | 40 ++++++++++++++++++++--------------------
xen/arch/x86/boot/trampoline.S | 2 +-
xen/arch/x86/boot/wakeup.S | 4 ++--
xen/arch/x86/boot/x86_64.S | 18 +++++++++---------
4 files changed, 32 insertions(+), 32 deletions(-)
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 9b1fd0e..76090eb 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -12,9 +12,9 @@
.text
.code32
-#define sym_phys(sym) ((sym) - __XEN_VIRT_START)
-#define sym_esi(sym) sym_phys(sym)(%esi)
-#define sym_fs(sym) %fs:sym_phys(sym)
+#define sym_offs(sym) ((sym) - __XEN_VIRT_START)
+#define sym_esi(sym) sym_offs(sym)(%esi)
+#define sym_fs(sym) %fs:sym_offs(sym)
#define BOOT_CS32 0x0008
#define BOOT_CS64 0x0010
@@ -97,7 +97,7 @@ multiboot2_header_start:
/* EFI64 entry point. */
mb2ht_init MB2_HT(ENTRY_ADDRESS_EFI64), MB2_HT(OPTIONAL), \
- sym_phys(__efi64_start)
+ sym_offs(__efi64_start)
/* Multiboot2 header end tag. */
mb2ht_init MB2_HT(END), MB2_HT(REQUIRED)
@@ -110,11 +110,11 @@ multiboot2_header_start:
gdt_boot_descr:
.word 7*8-1
gdt_boot_base:
- .long sym_phys(trampoline_gdt)
+ .long sym_offs(trampoline_gdt)
.long 0 /* Needed for 64-bit lgdt */
cs32_switch_addr:
- .long sym_phys(cs32_switch)
+ .long sym_offs(cs32_switch)
.word BOOT_CS32
.align 4
@@ -131,23 +131,23 @@ vga_text_buffer:
.section .init.text, "ax", @progbits
bad_cpu:
- add $sym_phys(.Lbad_cpu_msg),%esi # Error message
+ add $sym_offs(.Lbad_cpu_msg),%esi # Error message
jmp .Lget_vtb
not_multiboot:
- add $sym_phys(.Lbad_ldr_msg),%esi # Error message
+ add $sym_offs(.Lbad_ldr_msg),%esi # Error message
jmp .Lget_vtb
.Lmb2_no_st:
- add $sym_phys(.Lbad_ldr_nst),%esi # Error message
+ add $sym_offs(.Lbad_ldr_nst),%esi # Error message
jmp .Lget_vtb
.Lmb2_no_ih:
- add $sym_phys(.Lbad_ldr_nih),%esi # Error message
+ add $sym_offs(.Lbad_ldr_nih),%esi # Error message
jmp .Lget_vtb
.Lmb2_no_bs:
- add $sym_phys(.Lbad_ldr_nbs),%esi # Error message
+ add $sym_offs(.Lbad_ldr_nbs),%esi # Error message
xor %edi,%edi # No VGA text buffer
jmp .Lsend_chr
.Lmb2_efi_ia_32:
- add $sym_phys(.Lbad_efi_msg),%esi # Error message
+ add $sym_offs(.Lbad_efi_msg),%esi # Error message
xor %edi,%edi # No VGA text buffer
jmp .Lsend_chr
.Lget_vtb:
@@ -351,7 +351,7 @@ __start:
cli
/* Load default Xen image load base address. */
- mov $sym_phys(__image_base__),%esi
+ mov $sym_offs(__image_base__),%esi
/* Bootloaders may set multiboot{1,2}.mem_lower to a nonzero value. */
xor %edx,%edx
@@ -502,8 +502,8 @@ trampoline_setup:
jnz 1f
/* Initialize BSS (no nasty surprises!). */
- mov $sym_phys(__bss_start),%edi
- mov $sym_phys(__bss_end),%ecx
+ mov $sym_offs(__bss_start),%edi
+ mov $sym_offs(__bss_end),%ecx
push %fs
pop %es
sub %edi,%ecx
@@ -577,22 +577,22 @@ trampoline_setup:
/* Apply relocations to bootstrap trampoline. */
mov sym_fs(trampoline_phys),%edx
- mov $sym_phys(__trampoline_rel_start),%edi
+ mov $sym_offs(__trampoline_rel_start),%edi
1:
mov %fs:(%edi),%eax
add %edx,%fs:(%edi,%eax)
add $4,%edi
- cmp $sym_phys(__trampoline_rel_stop),%edi
+ cmp $sym_offs(__trampoline_rel_stop),%edi
jb 1b
/* Patch in the trampoline segment. */
shr $4,%edx
- mov $sym_phys(__trampoline_seg_start),%edi
+ mov $sym_offs(__trampoline_seg_start),%edi
1:
mov %fs:(%edi),%eax
mov %dx,%fs:(%edi,%eax)
add $4,%edi
- cmp $sym_phys(__trampoline_seg_stop),%edi
+ cmp $sym_offs(__trampoline_seg_stop),%edi
jb 1b
/* Do not parse command line on EFI platform here. */
@@ -618,7 +618,7 @@ trampoline_setup:
push %eax
/* Copy bootstrap trampoline to low memory, below 1MB. */
- mov $sym_phys(trampoline_start),%esi
+ mov $sym_offs(trampoline_start),%esi
mov $((trampoline_end - trampoline_start) / 4),%ecx
rep movsl %fs:(%esi),%es:(%edi)
diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S
index 64f8efd..b96607a 100644
--- a/xen/arch/x86/boot/trampoline.S
+++ b/xen/arch/x86/boot/trampoline.S
@@ -92,7 +92,7 @@ trampoline_protmode_entry:
mov %ecx,%cr4
/* Load pagetable base register. */
- mov $sym_phys(idle_pg_table),%eax
+ mov $sym_offs(idle_pg_table),%eax
add bootsym_rel(trampoline_xen_phys_start,4,%eax)
mov %eax,%cr3
diff --git a/xen/arch/x86/boot/wakeup.S b/xen/arch/x86/boot/wakeup.S
index 08ea9b2..42d57ab 100644
--- a/xen/arch/x86/boot/wakeup.S
+++ b/xen/arch/x86/boot/wakeup.S
@@ -120,7 +120,7 @@ wakeup_32:
mov $bootsym_rel(wakeup_stack, 4, %esp)
# check saved magic again
- mov $sym_phys(saved_magic), %eax
+ mov $sym_offs(saved_magic),%eax
add bootsym_rel(trampoline_xen_phys_start, 4, %eax)
mov (%eax), %eax
cmp $0x9abcdef0, %eax
@@ -133,7 +133,7 @@ wakeup_32:
mov %ecx, %cr4
/* Load pagetable base register */
- mov $sym_phys(idle_pg_table),%eax
+ mov $sym_offs(idle_pg_table),%eax
add bootsym_rel(trampoline_xen_phys_start,4,%eax)
mov %eax,%cr3
diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index 7392004..86d3775 100644
--- a/xen/arch/x86/boot/x86_64.S
+++ b/xen/arch/x86/boot/x86_64.S
@@ -112,7 +112,7 @@ GLOBAL(__page_tables_start)
* actualy mapped (mostly using superpages). Uses 4x 4k pages.
*/
GLOBAL(l2_identmap)
- .quad sym_phys(l1_identmap) + __PAGE_HYPERVISOR
+ .quad sym_offs(l1_identmap) + __PAGE_HYPERVISOR
idx = 1
.rept 7
.quad (idx << L2_PAGETABLE_SHIFT) | PAGE_HYPERVISOR | _PAGE_PSE
@@ -130,7 +130,7 @@ GLOBAL(l2_xenmap)
.quad 0
idx = 1
.rept 7
- .quad sym_phys(__image_base__) + (idx << L2_PAGETABLE_SHIFT) + (PAGE_HYPERVISOR | _PAGE_PSE)
+ .quad sym_offs(__image_base__) + (idx << L2_PAGETABLE_SHIFT) + (PAGE_HYPERVISOR | _PAGE_PSE)
idx = idx + 1
.endr
.fill L2_PAGETABLE_ENTRIES - 8, 8, 0
@@ -141,7 +141,7 @@ l2_fixmap:
idx = 0
.rept L2_PAGETABLE_ENTRIES
.if idx == l2_table_offset(FIXADDR_TOP - 1)
- .quad sym_phys(l1_fixmap) + __PAGE_HYPERVISOR
+ .quad sym_offs(l1_fixmap) + __PAGE_HYPERVISOR
.else
.quad 0
.endif
@@ -153,7 +153,7 @@ l2_fixmap:
l3_identmap:
idx = 0
.rept 4
- .quad sym_phys(l2_identmap) + (idx << PAGE_SHIFT) + __PAGE_HYPERVISOR
+ .quad sym_offs(l2_identmap) + (idx << PAGE_SHIFT) + __PAGE_HYPERVISOR
idx = idx + 1
.endr
.fill L3_PAGETABLE_ENTRIES - 4, 8, 0
@@ -164,9 +164,9 @@ l3_xenmap:
idx = 0
.rept L3_PAGETABLE_ENTRIES
.if idx == l3_table_offset(XEN_VIRT_START)
- .quad sym_phys(l2_xenmap) + __PAGE_HYPERVISOR
+ .quad sym_offs(l2_xenmap) + __PAGE_HYPERVISOR
.elseif idx == l3_table_offset(FIXADDR_TOP - 1)
- .quad sym_phys(l2_fixmap) + __PAGE_HYPERVISOR
+ .quad sym_offs(l2_fixmap) + __PAGE_HYPERVISOR
.else
.quad 0
.endif
@@ -176,13 +176,13 @@ l3_xenmap:
/* Top-level master (and idle-domain) page directory. */
GLOBAL(idle_pg_table)
- .quad sym_phys(l3_bootmap) + __PAGE_HYPERVISOR
+ .quad sym_offs(l3_bootmap) + __PAGE_HYPERVISOR
idx = 1
.rept L4_PAGETABLE_ENTRIES - 1
.if idx == l4_table_offset(DIRECTMAP_VIRT_START)
- .quad sym_phys(l3_identmap) + __PAGE_HYPERVISOR
+ .quad sym_offs(l3_identmap) + __PAGE_HYPERVISOR
.elseif idx == l4_table_offset(XEN_VIRT_START)
- .quad sym_phys(l3_xenmap) + __PAGE_HYPERVISOR
+ .quad sym_offs(l3_xenmap) + __PAGE_HYPERVISOR
.else
.quad 0
.endif
--
1.7.10.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v7 13/14] x86/boot: rename sym_phys() to sym_offs()
2016-09-23 21:47 ` [PATCH v7 13/14] x86/boot: rename sym_phys() to sym_offs() Daniel Kiper
@ 2016-09-26 15:47 ` Jan Beulich
0 siblings, 0 replies; 51+ messages in thread
From: Jan Beulich @ 2016-09-26 15:47 UTC (permalink / raw)
To: Daniel Kiper
Cc: Juergen Gross, sstabellini, andrew.cooper3, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
>>> On 23.09.16 at 23:47, <daniel.kiper@oracle.com> wrote:
> This way macro name better describes its function.
I'd appreciate if you could mention the change in function, so that
it ends up being quite obvious why the name change is desirable.
With that:
Acked-by: Jan Beulich <jbeulich@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v7 14/14] x86: add multiboot2 protocol support for relocatable images
2016-09-23 21:47 [PATCH v7 00/14] x86: multiboot2 protocol support Daniel Kiper
` (12 preceding siblings ...)
2016-09-23 21:47 ` [PATCH v7 13/14] x86/boot: rename sym_phys() to sym_offs() Daniel Kiper
@ 2016-09-23 21:47 ` Daniel Kiper
2016-09-26 15:53 ` Jan Beulich
13 siblings, 1 reply; 51+ messages in thread
From: Daniel Kiper @ 2016-09-23 21:47 UTC (permalink / raw)
To: xen-devel
Cc: jgross, sstabellini, andrew.cooper3, cardoe, pgnet.dev, ning.sun,
julien.grall, david.vrabel, jbeulich, qiaowei.ren, gang.wei,
fu.wei
Add multiboot2 protocol support for relocatable images. Only GRUB2 with
"multiboot2: Add support for relocatable images" patch understands
that feature. Older multiboot protocol (regardless of version)
compatible loaders ignore it and everything works as usual.
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
v4 - suggestions/fixes:
- do not get Xen image load base address from
multiboot2 information in x86_64 code
(suggested by Jan Beulich),
- improve label names
(suggested by Jan Beulich),
- improve comments,
(suggested by Jan Beulich).
v3 - suggestions/fixes:
- use %esi and %r15d instead of %ebp to store
Xen image load base address,
- rename some types and constants,
- reformat xen/include/xen/multiboot2.h
(suggested by Konrad Rzeszutek Wilk),
- improve comments,
- improve commit message
(suggested by Konrad Rzeszutek Wilk).
---
xen/arch/x86/boot/head.S | 19 ++++++++++++++++++-
xen/arch/x86/x86_64/asm-offsets.c | 1 +
xen/include/xen/multiboot2.h | 13 +++++++++++++
3 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 76090eb..b743092 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -82,6 +82,13 @@ multiboot2_header_start:
/* Align modules at page boundry. */
mb2ht_init MB2_HT(MODULE_ALIGN), MB2_HT(REQUIRED)
+ /* Load address preference. */
+ mb2ht_init MB2_HT(RELOCATABLE), MB2_HT(OPTIONAL), \
+ sym_offs(start), /* Min load address. */ \
+ 0xffffffff, /* The end of image max load address (4 GiB - 1). */ \
+ 0x200000, /* Load address alignment (2 MiB). */ \
+ MULTIBOOT2_LOAD_PREFERENCE_HIGH
+
/* Console flags tag. */
mb2ht_init MB2_HT(CONSOLE_FLAGS), MB2_HT(OPTIONAL), \
MULTIBOOT2_CONSOLE_FLAGS_EGA_TEXT_SUPPORTED
@@ -383,10 +390,19 @@ __start:
cmp %edi,MB2_fixed_total_size(%ebx)
jbe trampoline_bios_setup
+ /* Get Xen image load base address from Multiboot2 information. */
+ cmpl $MULTIBOOT2_TAG_TYPE_LOAD_BASE_ADDR,MB2_tag_type(%ecx)
+ jne 1f
+
+ mov MB2_load_base_addr(%ecx),%esi
+ sub $XEN_IMG_OFFSET,%esi
+ jmp 9f
+
+1:
/* Get mem_lower from Multiboot2 information. */
cmpl $MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,MB2_tag_type(%ecx)
cmove MB2_mem_lower(%ecx),%edx
- je trampoline_bios_setup
+ je 9f
/* EFI IA-32 platforms are not supported. */
cmpl $MULTIBOOT2_TAG_TYPE_EFI32,MB2_tag_type(%ecx)
@@ -400,6 +416,7 @@ __start:
cmpl $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%ecx)
je trampoline_bios_setup
+9:
/* Go to next Multiboot2 information tag. */
add MB2_tag_size(%ecx),%ecx
add $(MULTIBOOT2_TAG_ALIGN-1),%ecx
diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c
index 5d7a8e5..beac5ca 100644
--- a/xen/arch/x86/x86_64/asm-offsets.c
+++ b/xen/arch/x86/x86_64/asm-offsets.c
@@ -174,6 +174,7 @@ void __dummy__(void)
OFFSET(MB2_fixed_total_size, multiboot2_fixed_t, total_size);
OFFSET(MB2_tag_type, multiboot2_tag_t, type);
OFFSET(MB2_tag_size, multiboot2_tag_t, size);
+ OFFSET(MB2_load_base_addr, multiboot2_tag_load_base_addr_t, load_base_addr);
OFFSET(MB2_mem_lower, multiboot2_tag_basic_meminfo_t, mem_lower);
OFFSET(MB2_efi64_st, multiboot2_tag_efi64_t, pointer);
OFFSET(MB2_efi64_ih, multiboot2_tag_efi64_ih_t, pointer);
diff --git a/xen/include/xen/multiboot2.h b/xen/include/xen/multiboot2.h
index 8dd5800..feb4297 100644
--- a/xen/include/xen/multiboot2.h
+++ b/xen/include/xen/multiboot2.h
@@ -59,11 +59,17 @@
#define MULTIBOOT2_HEADER_TAG_EFI_BS 7
#define MULTIBOOT2_HEADER_TAG_ENTRY_ADDRESS_EFI32 8
#define MULTIBOOT2_HEADER_TAG_ENTRY_ADDRESS_EFI64 9
+#define MULTIBOOT2_HEADER_TAG_RELOCATABLE 10
/* Header tag flags. */
#define MULTIBOOT2_HEADER_TAG_REQUIRED 0
#define MULTIBOOT2_HEADER_TAG_OPTIONAL 1
+/* Where image should be loaded (suggestion not requirement). */
+#define MULTIBOOT2_LOAD_PREFERENCE_NONE 0
+#define MULTIBOOT2_LOAD_PREFERENCE_LOW 1
+#define MULTIBOOT2_LOAD_PREFERENCE_HIGH 2
+
/* Header console tag console_flags. */
#define MULTIBOOT2_CONSOLE_FLAGS_CONSOLE_REQUIRED 1
#define MULTIBOOT2_CONSOLE_FLAGS_EGA_TEXT_SUPPORTED 2
@@ -90,6 +96,7 @@
#define MULTIBOOT2_TAG_TYPE_EFI_BS 18
#define MULTIBOOT2_TAG_TYPE_EFI32_IH 19
#define MULTIBOOT2_TAG_TYPE_EFI64_IH 20
+#define MULTIBOOT2_TAG_TYPE_LOAD_BASE_ADDR 21
/* Multiboot 2 tag alignment. */
#define MULTIBOOT2_TAG_ALIGN 8
@@ -120,6 +127,12 @@ typedef struct {
typedef struct {
u32 type;
u32 size;
+ u32 load_base_addr;
+} multiboot2_tag_load_base_addr_t;
+
+typedef struct {
+ u32 type;
+ u32 size;
char string[];
} multiboot2_tag_string_t;
--
1.7.10.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v7 14/14] x86: add multiboot2 protocol support for relocatable images
2016-09-23 21:47 ` [PATCH v7 14/14] x86: add multiboot2 protocol support for relocatable images Daniel Kiper
@ 2016-09-26 15:53 ` Jan Beulich
2016-09-27 20:07 ` Daniel Kiper
0 siblings, 1 reply; 51+ messages in thread
From: Jan Beulich @ 2016-09-26 15:53 UTC (permalink / raw)
To: Daniel Kiper
Cc: Juergen Gross, sstabellini, andrew.cooper3, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
>>> On 23.09.16 at 23:47, <daniel.kiper@oracle.com> wrote:
> @@ -383,10 +390,19 @@ __start:
> cmp %edi,MB2_fixed_total_size(%ebx)
> jbe trampoline_bios_setup
>
> + /* Get Xen image load base address from Multiboot2 information. */
> + cmpl $MULTIBOOT2_TAG_TYPE_LOAD_BASE_ADDR,MB2_tag_type(%ecx)
> + jne 1f
> +
> + mov MB2_load_base_addr(%ecx),%esi
> + sub $XEN_IMG_OFFSET,%esi
> + jmp 9f
> +
> +1:
> /* Get mem_lower from Multiboot2 information. */
> cmpl $MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,MB2_tag_type(%ecx)
> cmove MB2_mem_lower(%ecx),%edx
> - je trampoline_bios_setup
> + je 9f
>
> /* EFI IA-32 platforms are not supported. */
> cmpl $MULTIBOOT2_TAG_TYPE_EFI32,MB2_tag_type(%ecx)
Considering that you look for another tag type here, doesn't the
branch target change above point out an issue in an earlier patch?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v7 14/14] x86: add multiboot2 protocol support for relocatable images
2016-09-26 15:53 ` Jan Beulich
@ 2016-09-27 20:07 ` Daniel Kiper
0 siblings, 0 replies; 51+ messages in thread
From: Daniel Kiper @ 2016-09-27 20:07 UTC (permalink / raw)
To: Jan Beulich
Cc: Juergen Gross, sstabellini, andrew.cooper3, cardoe, pgnet.dev,
ning.sun, julien.grall, david.vrabel, xen-devel, qiaowei.ren,
gang.wei, fu.wei
On Mon, Sep 26, 2016 at 09:53:45AM -0600, Jan Beulich wrote:
> >>> On 23.09.16 at 23:47, <daniel.kiper@oracle.com> wrote:
> > @@ -383,10 +390,19 @@ __start:
> > cmp %edi,MB2_fixed_total_size(%ebx)
> > jbe trampoline_bios_setup
> >
> > + /* Get Xen image load base address from Multiboot2 information. */
> > + cmpl $MULTIBOOT2_TAG_TYPE_LOAD_BASE_ADDR,MB2_tag_type(%ecx)
> > + jne 1f
> > +
> > + mov MB2_load_base_addr(%ecx),%esi
> > + sub $XEN_IMG_OFFSET,%esi
> > + jmp 9f
> > +
> > +1:
> > /* Get mem_lower from Multiboot2 information. */
> > cmpl $MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,MB2_tag_type(%ecx)
> > cmove MB2_mem_lower(%ecx),%edx
> > - je trampoline_bios_setup
> > + je 9f
> >
> > /* EFI IA-32 platforms are not supported. */
> > cmpl $MULTIBOOT2_TAG_TYPE_EFI32,MB2_tag_type(%ecx)
>
> Considering that you look for another tag type here, doesn't the
> branch target change above point out an issue in an earlier patch?
Good point. This change should be part of patch #8 introducing multiboot2
protocol support for EFI platforms.
Daniel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 51+ messages in thread