* [PATCH v2 0/8] Change xen.efi build and add SHIM_LOCK verification into efi_multiboot2()
@ 2018-06-19 14:35 Daniel Kiper
2018-06-19 14:35 ` [PATCH v2 1/8] xen: calculate XEN_BUILD_TIME using XEN_BUILD_DATE value Daniel Kiper
` (7 more replies)
0 siblings, 8 replies; 50+ messages in thread
From: Daniel Kiper @ 2018-06-19 14:35 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, wei.liu2, george.dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, jbeulich, tamas.k.lengyel
Hey,
As in subject... This way we get:
- one binary which can be loaded by the UEFI loader, Multiboot and
Multiboot2 protocols,
- UEFI secure boot support when Xen is loaded via Multiboot2 protocol,
- if we wish, in the future we can drop xen/xen.gz and build
xen.efi only,
- crash dumps generated by the xen.efi loaded from the EFI loader
can be analyzed by crash tool,
- simpler code,
- simpler build,
- Xen build will no longer depend on ld i386pep support.
This patch series functionality does not depend on any GRUB2 changes.
So, review can commence without any obstacles. Though the GRUB2 have
to be changed to provide full verification chain. This will be
discussed in separate thread.
Daniel
xen/Makefile | 26 +++---
xen/arch/arm/efi/efi-boot.h | 4 -
xen/arch/x86/Makefile | 88 +-------------------
xen/arch/x86/Rules.mk | 2 +
xen/arch/x86/boot/head.S | 205 +++++++++++++++++++++++++++++++++++++++++++--
xen/arch/x86/efi/Makefile | 17 +---
xen/arch/x86/efi/buildid.ihex | 3 -
xen/arch/x86/efi/check.c | 4 -
xen/arch/x86/efi/efi-boot.h | 84 ++++---------------
xen/arch/x86/efi/mkreloc.c | 384 ------------------------------------------------------------------------------------
xen/arch/x86/efi/relocs-dummy.S | 11 ---
xen/arch/x86/efi/stub.c | 83 ------------------
xen/arch/x86/xen.lds.S | 95 ++++++---------------
xen/common/efi/boot.c | 21 +++--
xen/common/version.c | 51 ------------
xen/include/xen/compile.h.in | 1 +
16 files changed, 277 insertions(+), 802 deletions(-)
Daniel Kiper (8):
xen: calculate XEN_BUILD_TIME using XEN_BUILD_DATE value
xen: introduce XEN_COMPILE_POSIX_TIME
xen/x86: manually build xen.mb.efi binary
xen/x86: add some addresses to the Multiboot header
xen/x86: add some addresses to the Multiboot2 header
efi: split out efi_shim_lock()
xen/x86/efi: Verify dom0 kernel with SHIM_LOCK protocol in efi_multiboot2()
efi: drop original xen.efi code and build mechanism
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 50+ messages in thread* [PATCH v2 1/8] xen: calculate XEN_BUILD_TIME using XEN_BUILD_DATE value 2018-06-19 14:35 [PATCH v2 0/8] Change xen.efi build and add SHIM_LOCK verification into efi_multiboot2() Daniel Kiper @ 2018-06-19 14:35 ` Daniel Kiper 2018-06-25 13:48 ` Jan Beulich 2018-06-19 14:35 ` [PATCH v2 2/8] xen: introduce XEN_COMPILE_POSIX_TIME Daniel Kiper ` (6 subsequent siblings) 7 siblings, 1 reply; 50+ messages in thread From: Daniel Kiper @ 2018-06-19 14:35 UTC (permalink / raw) To: xen-devel Cc: sstabellini, wei.liu2, george.dunlap, andrew.cooper3, ian.jackson, tim, julien.grall, jbeulich, tamas.k.lengyel Otherwise times in XEN_BUILD_DATE and XEN_BUILD_TIME may differ. Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> --- xen/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xen/Makefile b/xen/Makefile index 60867e3..aa82641 100644 --- a/xen/Makefile +++ b/xen/Makefile @@ -9,7 +9,7 @@ export XEN_FULLVERSION = $(XEN_VERSION).$(XEN_SUBVERSION)$(XEN_EXTRAVERSION) export XEN_WHOAMI ?= $(USER) export XEN_DOMAIN ?= $(shell ([ -x /bin/dnsdomainname ] && /bin/dnsdomainname) || ([ -x /bin/domainname ] && /bin/domainname || echo [unknown])) export XEN_BUILD_DATE ?= $(shell LC_ALL=C date) -export XEN_BUILD_TIME ?= $(shell LC_ALL=C date +%T) +export XEN_BUILD_TIME ?= $(shell LC_ALL=C date -d '$(XEN_BUILD_DATE)' +%T) export XEN_BUILD_HOST ?= $(shell hostname) export XEN_CONFIG_EXPERT ?= n -- 1.7.10.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [PATCH v2 1/8] xen: calculate XEN_BUILD_TIME using XEN_BUILD_DATE value 2018-06-19 14:35 ` [PATCH v2 1/8] xen: calculate XEN_BUILD_TIME using XEN_BUILD_DATE value Daniel Kiper @ 2018-06-25 13:48 ` Jan Beulich 2018-07-04 12:06 ` Daniel Kiper 0 siblings, 1 reply; 50+ messages in thread From: Jan Beulich @ 2018-06-25 13:48 UTC (permalink / raw) To: Daniel Kiper Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel >>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: > --- a/xen/Makefile > +++ b/xen/Makefile > @@ -9,7 +9,7 @@ export XEN_FULLVERSION = $(XEN_VERSION).$(XEN_SUBVERSION)$(XEN_EXTRAVERSION) > export XEN_WHOAMI ?= $(USER) > export XEN_DOMAIN ?= $(shell ([ -x /bin/dnsdomainname ] && /bin/dnsdomainname) || ([ -x /bin/domainname ] && /bin/domainname || echo [unknown])) > export XEN_BUILD_DATE ?= $(shell LC_ALL=C date) > -export XEN_BUILD_TIME ?= $(shell LC_ALL=C date +%T) > +export XEN_BUILD_TIME ?= $(shell LC_ALL=C date -d '$(XEN_BUILD_DATE)' +%T) Nice idea, but I'm not sure we can rely on the non-standard -d option to be supported by all environments Xen may be built in. As per http://pubs.opengroup.org/onlinepubs/007904975/utilities/date.html the only standard option supported by date is -u. Assuming C and POSIX locales produce the same standard format, I'm afraid you may need to resort to sed-ery to achieve what you want. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 1/8] xen: calculate XEN_BUILD_TIME using XEN_BUILD_DATE value 2018-06-25 13:48 ` Jan Beulich @ 2018-07-04 12:06 ` Daniel Kiper 2018-07-04 13:58 ` Ian Jackson 0 siblings, 1 reply; 50+ messages in thread From: Daniel Kiper @ 2018-07-04 12:06 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel On Mon, Jun 25, 2018 at 07:48:34AM -0600, Jan Beulich wrote: > >>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: > > --- a/xen/Makefile > > +++ b/xen/Makefile > > @@ -9,7 +9,7 @@ export XEN_FULLVERSION = $(XEN_VERSION).$(XEN_SUBVERSION)$(XEN_EXTRAVERSION) > > export XEN_WHOAMI ?= $(USER) > > export XEN_DOMAIN ?= $(shell ([ -x /bin/dnsdomainname ] && /bin/dnsdomainname) || ([ -x /bin/domainname ] && /bin/domainname || echo [unknown])) > > export XEN_BUILD_DATE ?= $(shell LC_ALL=C date) > > -export XEN_BUILD_TIME ?= $(shell LC_ALL=C date +%T) > > +export XEN_BUILD_TIME ?= $(shell LC_ALL=C date -d '$(XEN_BUILD_DATE)' +%T) > > Nice idea, but I'm not sure we can rely on the non-standard -d > option to be supported by all environments Xen may be built in. > As per > http://pubs.opengroup.org/onlinepubs/007904975/utilities/date.html > the only standard option supported by date is -u. Assuming C and > POSIX locales produce the same standard format, I'm afraid you may > need to resort to sed-ery to achieve what you want. Well, this complicates situation further and it seems to me that sed-ery cannot be sufficient. Anyway, I will take a look how to solve that. Daniel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 1/8] xen: calculate XEN_BUILD_TIME using XEN_BUILD_DATE value 2018-07-04 12:06 ` Daniel Kiper @ 2018-07-04 13:58 ` Ian Jackson 2018-07-04 14:39 ` Daniel Kiper 0 siblings, 1 reply; 50+ messages in thread From: Ian Jackson @ 2018-07-04 13:58 UTC (permalink / raw) To: Daniel Kiper Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Tim Deegan, Julien Grall, Jan Beulich, Tamas K Lengyel, xen-devel Daniel Kiper writes ("Re: [PATCH v2 1/8] xen: calculate XEN_BUILD_TIME using XEN_BUILD_DATE value"): > Well, this complicates situation further and it seems to me that > sed-ery cannot be sufficient. Anyway, I will take a look how to > solve that. Our other current host OS is FreeBSD. FreeBSD's date(1) uses -d for something utterly different (also, something mad). Can we rely on Perl yet ? Perl can do this kind of thing easily enough. Alternatively we could have a wrapper script. Andrew Cooper: > Why are we adding yet more Xen specific logic to implement the same as > SOURCE_DATE_EPOCH from the reproducible-builds.org effort? We should clearly honour SOURCE_DATE_EPOCH. But that's a different question. I think the right thing is something like this: export XEN_BUILD_POSIX_TIME ?= $(shell echo $${SOURCE_DATE_EPOCH-date +%s}) export XEN_BUILD_DATE ?= $(shell LC_ALL=C date_parsing_epoch $(XEN_BUILD_EPOCH)) export XEN_BUILD_TIME ?= $(shell LC_ALL=C date_parsing_epoch $(XEN_BUILD_EPOCH) +%T) Where date_parsing_epoch is this on GNU systems #!/bin/sh set -e epoch=$1; shift date -u -d "@$epoch" "$@" and I think something like this on FreeBSD #!/bin/sh set -e epoch=$1; shift date -u -jn -f%s "$epoch" "$@" Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 1/8] xen: calculate XEN_BUILD_TIME using XEN_BUILD_DATE value 2018-07-04 13:58 ` Ian Jackson @ 2018-07-04 14:39 ` Daniel Kiper 2018-07-04 15:41 ` Ian Jackson 0 siblings, 1 reply; 50+ messages in thread From: Daniel Kiper @ 2018-07-04 14:39 UTC (permalink / raw) To: Ian Jackson Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Tim Deegan, Julien Grall, Jan Beulich, Tamas K Lengyel, xen-devel On Wed, Jul 04, 2018 at 02:58:17PM +0100, Ian Jackson wrote: > Daniel Kiper writes ("Re: [PATCH v2 1/8] xen: calculate XEN_BUILD_TIME using XEN_BUILD_DATE value"): > > Well, this complicates situation further and it seems to me that > > sed-ery cannot be sufficient. Anyway, I will take a look how to > > solve that. > > Our other current host OS is FreeBSD. FreeBSD's date(1) uses -d for > something utterly different (also, something mad). Can we rely on > Perl yet ? Perl can do this kind of thing easily enough. Oh, Perl, it seems like a dying language... Sniff... > Alternatively we could have a wrapper script. I think that it is a better solution. > Andrew Cooper: > > Why are we adding yet more Xen specific logic to implement the same as > > SOURCE_DATE_EPOCH from the reproducible-builds.org effort? > > We should clearly honour SOURCE_DATE_EPOCH. But that's a different > question. > > I think the right thing is something like this: > > export XEN_BUILD_POSIX_TIME ?= $(shell echo $${SOURCE_DATE_EPOCH-date +%s}) OK, but what if SOURCE_DATE_EPOCH is not defined? "date" command is not a good solution here. > export XEN_BUILD_DATE ?= $(shell LC_ALL=C date_parsing_epoch $(XEN_BUILD_EPOCH)) > export XEN_BUILD_TIME ?= $(shell LC_ALL=C date_parsing_epoch $(XEN_BUILD_EPOCH) +%T) > > Where date_parsing_epoch is this on GNU systems > > #!/bin/sh > set -e > epoch=$1; shift > date -u -d "@$epoch" "$@" > > and I think something like this on FreeBSD > > #!/bin/sh > set -e > epoch=$1; shift > date -u -jn -f%s "$epoch" "$@" If other guys are OK with that I can prepare a patch with your Suggested-by. Daniel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 1/8] xen: calculate XEN_BUILD_TIME using XEN_BUILD_DATE value 2018-07-04 14:39 ` Daniel Kiper @ 2018-07-04 15:41 ` Ian Jackson 2018-07-04 15:51 ` Roger Pau Monné 2018-07-04 16:07 ` Daniel Kiper 0 siblings, 2 replies; 50+ messages in thread From: Ian Jackson @ 2018-07-04 15:41 UTC (permalink / raw) To: Daniel Kiper Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Roger Pau Monné, Tim Deegan, Julien Grall, Jan Beulich, Tamas K Lengyel, xen-devel Daniel Kiper writes ("Re: [PATCH v2 1/8] xen: calculate XEN_BUILD_TIME using XEN_BUILD_DATE value"): > On Wed, Jul 04, 2018 at 02:58:17PM +0100, Ian Jackson wrote: > > export XEN_BUILD_POSIX_TIME ?= $(shell echo $${SOURCE_DATE_EPOCH-date +%s}) > > OK, but what if SOURCE_DATE_EPOCH is not defined? > "date" command is not a good solution here. I don't understand why you think not. I think all the versions of date we care about will do something sensible with date +%s I don't have a FreeBSD host to hand to test, CC'ing Roger. Although my rune above is wrong and should read export XEN_BUILD_POSIX_TIME ?= $(shell echo $${SOURCE_DATE_EPOCH-$$(date +%s)}) > If other guys are OK with that I can prepare a patch with your Suggested-by. How do in intend to choose which wrapper script to use ? This pile of stuff will run on awful lot of times during each make so keeping it of reasonable size is a good plan. Maybe it would be better to have a single shell script which outputs a whole bunch of variable settings for make to $(eval ) ? Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 1/8] xen: calculate XEN_BUILD_TIME using XEN_BUILD_DATE value 2018-07-04 15:41 ` Ian Jackson @ 2018-07-04 15:51 ` Roger Pau Monné 2018-07-04 16:07 ` Daniel Kiper 1 sibling, 0 replies; 50+ messages in thread From: Roger Pau Monné @ 2018-07-04 15:51 UTC (permalink / raw) To: Ian Jackson Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Daniel Kiper, Tim Deegan, Julien Grall, Jan Beulich, Tamas K Lengyel, xen-devel On Wed, Jul 04, 2018 at 04:41:30PM +0100, Ian Jackson wrote: > Daniel Kiper writes ("Re: [PATCH v2 1/8] xen: calculate XEN_BUILD_TIME using XEN_BUILD_DATE value"): > > On Wed, Jul 04, 2018 at 02:58:17PM +0100, Ian Jackson wrote: > > > export XEN_BUILD_POSIX_TIME ?= $(shell echo $${SOURCE_DATE_EPOCH-date +%s}) > > > > OK, but what if SOURCE_DATE_EPOCH is not defined? > > "date" command is not a good solution here. > > I don't understand why you think not. I think all the versions of > date we care about will do something sensible with > date +%s > > I don't have a FreeBSD host to hand to test, CC'ing Roger. date +%s does indeed work fine on FreeBSD: $ date +%s 1530719654 Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 1/8] xen: calculate XEN_BUILD_TIME using XEN_BUILD_DATE value 2018-07-04 15:41 ` Ian Jackson 2018-07-04 15:51 ` Roger Pau Monné @ 2018-07-04 16:07 ` Daniel Kiper 2018-07-04 16:30 ` Ian Jackson 1 sibling, 1 reply; 50+ messages in thread From: Daniel Kiper @ 2018-07-04 16:07 UTC (permalink / raw) To: Ian Jackson Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Roger Pau Monné, Tim Deegan, Julien Grall, Jan Beulich, Tamas K Lengyel, xen-devel On Wed, Jul 04, 2018 at 04:41:30PM +0100, Ian Jackson wrote: > Daniel Kiper writes ("Re: [PATCH v2 1/8] xen: calculate XEN_BUILD_TIME using XEN_BUILD_DATE value"): > > On Wed, Jul 04, 2018 at 02:58:17PM +0100, Ian Jackson wrote: > > > export XEN_BUILD_POSIX_TIME ?= $(shell echo $${SOURCE_DATE_EPOCH-date +%s}) > > > > OK, but what if SOURCE_DATE_EPOCH is not defined? > > "date" command is not a good solution here. > > I don't understand why you think not. I think all the versions of > date we care about will do something sensible with > date +%s > > I don't have a FreeBSD host to hand to test, CC'ing Roger. This is not the date command problem. This is more related to SOURCE_DATE_EPOCH spec (https://reproducible-builds.org/specs/source-date-epoch/#idm55) which clearly says: The value MUST be reproducible (deterministic) across different executions of the build, depending only on the source code. It SHOULD be set to the last modification time of the source, incorporating any packaging-specific modifications. So, AIUI, above implies that we should not use the date command. Am I right? > Although my rune above is wrong and should read > export XEN_BUILD_POSIX_TIME ?= $(shell echo $${SOURCE_DATE_EPOCH-$$(date +%s)}) > > > If other guys are OK with that I can prepare a patch with your Suggested-by. > > How do in intend to choose which wrapper script to use ? I though that a mechanism to differentiate GNU and BSD systems exists in the Xen build system. So, I was going to reuse it somehow. > This pile of stuff will run on awful lot of times during each make so > keeping it of reasonable size is a good plan. > > Maybe it would be better to have a single shell script which outputs a > whole bunch of variable settings for make to $(eval ) ? It looks that this variable is referenced once, so, it should not be a problem. Daniel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 1/8] xen: calculate XEN_BUILD_TIME using XEN_BUILD_DATE value 2018-07-04 16:07 ` Daniel Kiper @ 2018-07-04 16:30 ` Ian Jackson 2018-07-04 16:55 ` Daniel Kiper 0 siblings, 1 reply; 50+ messages in thread From: Ian Jackson @ 2018-07-04 16:30 UTC (permalink / raw) To: Daniel Kiper Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Roger Pau Monné, Tim Deegan, Julien Grall, Jan Beulich, Tamas K Lengyel, xen-devel Daniel Kiper writes ("Re: [PATCH v2 1/8] xen: calculate XEN_BUILD_TIME using XEN_BUILD_DATE value"): > On Wed, Jul 04, 2018 at 04:41:30PM +0100, Ian Jackson wrote: > > I don't have a FreeBSD host to hand to test, CC'ing Roger. > > This is not the date command problem. This is more related to SOURCE_DATE_EPOCH > spec (https://reproducible-builds.org/specs/source-date-epoch/#idm55) > which clearly says: > > The value MUST be reproducible (deterministic) across different > executions of the build, depending only on the source code. It SHOULD > be set to the last modification time of the source, incorporating any > packaging-specific modifications. > > So, AIUI, above implies that we should not use the date command. Am I right? No. That spec is for people *setting* SOURCE_DATE_EPOCH. Which we are not doing. For a *consumer* of SOURCE_DATE_EPOCH, if it is not set, using the output of date or the current time or something is fine. > > How do in intend to choose which wrapper script to use ? > > I though that a mechanism to differentiate GNU and BSD systems exists in > the Xen build system. So, I was going to reuse it somehow. ISTR this being slightly clumsy. But good luck :-). > > This pile of stuff will run on awful lot of times during each make so > > keeping it of reasonable size is a good plan. > > > > Maybe it would be better to have a single shell script which outputs a > > whole bunch of variable settings for make to $(eval ) ? > > It looks that this variable is referenced once, so, it should not be a problem. I think make's recursive expansion system may result in these shell runes being executed multiple times. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 1/8] xen: calculate XEN_BUILD_TIME using XEN_BUILD_DATE value 2018-07-04 16:30 ` Ian Jackson @ 2018-07-04 16:55 ` Daniel Kiper 0 siblings, 0 replies; 50+ messages in thread From: Daniel Kiper @ 2018-07-04 16:55 UTC (permalink / raw) To: Ian Jackson Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Roger Pau Monné, Tim Deegan, Julien Grall, Jan Beulich, Tamas K Lengyel, xen-devel On Wed, Jul 04, 2018 at 05:30:54PM +0100, Ian Jackson wrote: > Daniel Kiper writes ("Re: [PATCH v2 1/8] xen: calculate XEN_BUILD_TIME using XEN_BUILD_DATE value"): > > On Wed, Jul 04, 2018 at 04:41:30PM +0100, Ian Jackson wrote: > > > I don't have a FreeBSD host to hand to test, CC'ing Roger. > > > > This is not the date command problem. This is more related to SOURCE_DATE_EPOCH > > spec (https://reproducible-builds.org/specs/source-date-epoch/#idm55) > > which clearly says: > > > > The value MUST be reproducible (deterministic) across different > > executions of the build, depending only on the source code. It SHOULD > > be set to the last modification time of the source, incorporating any > > packaging-specific modifications. > > > > So, AIUI, above implies that we should not use the date command. Am I right? > > No. > > That spec is for people *setting* SOURCE_DATE_EPOCH. Which we are not > doing. > > For a *consumer* of SOURCE_DATE_EPOCH, if it is not set, using the > output of date or the current time or something is fine. Ahhh... OK, make sens then. > > > How do in intend to choose which wrapper script to use ? > > > > I though that a mechanism to differentiate GNU and BSD systems exists in > > the Xen build system. So, I was going to reuse it somehow. > > ISTR this being slightly clumsy. But good luck :-). Thanks a lot! :-))) > > > This pile of stuff will run on awful lot of times during each make so > > > keeping it of reasonable size is a good plan. > > > > > > Maybe it would be better to have a single shell script which outputs a > > > whole bunch of variable settings for make to $(eval ) ? > > > > It looks that this variable is referenced once, so, it should not be a problem. > > I think make's recursive expansion system may result in these shell > runes being executed multiple times. I will double check it. Daniel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v2 2/8] xen: introduce XEN_COMPILE_POSIX_TIME 2018-06-19 14:35 [PATCH v2 0/8] Change xen.efi build and add SHIM_LOCK verification into efi_multiboot2() Daniel Kiper 2018-06-19 14:35 ` [PATCH v2 1/8] xen: calculate XEN_BUILD_TIME using XEN_BUILD_DATE value Daniel Kiper @ 2018-06-19 14:35 ` Daniel Kiper 2018-06-25 13:54 ` Jan Beulich 2018-06-19 14:35 ` [PATCH v2 3/8] xen/x86: manually build xen.mb.efi binary Daniel Kiper ` (5 subsequent siblings) 7 siblings, 1 reply; 50+ messages in thread From: Daniel Kiper @ 2018-06-19 14:35 UTC (permalink / raw) To: xen-devel Cc: sstabellini, wei.liu2, george.dunlap, andrew.cooper3, ian.jackson, tim, julien.grall, jbeulich, tamas.k.lengyel We need the POSIX time to properly fill the TimeDateStamp field in the PE header. Additionally, realign the variables assignment in xen/Makefile to increase readability. Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> --- v2 - suggestions/fixes: - derive XEN_COMPILE_POSIX_TIME from XEN_BUILD_DATE (suggested by Jan Beulich), - echo 0 if date command does not work (suggested by Konrad Rzeszutek Wilk), - improve commit message (suggested by Konrad Rzeszutek Wilk). --- xen/Makefile | 14 ++++++++------ xen/include/xen/compile.h.in | 1 + 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/xen/Makefile b/xen/Makefile index aa82641..1bed339 100644 --- a/xen/Makefile +++ b/xen/Makefile @@ -6,12 +6,13 @@ export XEN_EXTRAVERSION ?= -rc$(XEN_VENDORVERSION) export XEN_FULLVERSION = $(XEN_VERSION).$(XEN_SUBVERSION)$(XEN_EXTRAVERSION) -include xen-version -export XEN_WHOAMI ?= $(USER) -export XEN_DOMAIN ?= $(shell ([ -x /bin/dnsdomainname ] && /bin/dnsdomainname) || ([ -x /bin/domainname ] && /bin/domainname || echo [unknown])) -export XEN_BUILD_DATE ?= $(shell LC_ALL=C date) -export XEN_BUILD_TIME ?= $(shell LC_ALL=C date -d '$(XEN_BUILD_DATE)' +%T) -export XEN_BUILD_HOST ?= $(shell hostname) -export XEN_CONFIG_EXPERT ?= n +export XEN_WHOAMI ?= $(USER) +export XEN_DOMAIN ?= $(shell ([ -x /bin/dnsdomainname ] && /bin/dnsdomainname) || ([ -x /bin/domainname ] && /bin/domainname || echo [unknown])) +export XEN_BUILD_DATE ?= $(shell LC_ALL=C date) +export XEN_BUILD_TIME ?= $(shell LC_ALL=C date -d '$(XEN_BUILD_DATE)' +%T) +export XEN_BUILD_POSIX_TIME ?= $(shell LC_ALL=C date -d '$(XEN_BUILD_DATE)' +%s || echo 0) +export XEN_BUILD_HOST ?= $(shell hostname) +export XEN_CONFIG_EXPERT ?= n export BASEDIR := $(CURDIR) export XEN_ROOT := $(BASEDIR)/.. @@ -164,6 +165,7 @@ delete-unfresh-files: include/xen/compile.h: include/xen/compile.h.in .banner @sed -e 's/@@date@@/$(XEN_BUILD_DATE)/g' \ -e 's/@@time@@/$(XEN_BUILD_TIME)/g' \ + -e 's/@@posix_time@@/$(XEN_BUILD_POSIX_TIME)/g' \ -e 's/@@whoami@@/$(XEN_WHOAMI)/g' \ -e 's/@@domain@@/$(XEN_DOMAIN)/g' \ -e 's/@@hostname@@/$(XEN_BUILD_HOST)/g' \ diff --git a/xen/include/xen/compile.h.in b/xen/include/xen/compile.h.in index 440ecb2..b2ae6f9 100644 --- a/xen/include/xen/compile.h.in +++ b/xen/include/xen/compile.h.in @@ -1,5 +1,6 @@ #define XEN_COMPILE_DATE "@@date@@" #define XEN_COMPILE_TIME "@@time@@" +#define XEN_COMPILE_POSIX_TIME @@posix_time@@ #define XEN_COMPILE_BY "@@whoami@@" #define XEN_COMPILE_DOMAIN "@@domain@@" #define XEN_COMPILE_HOST "@@hostname@@" -- 1.7.10.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [PATCH v2 2/8] xen: introduce XEN_COMPILE_POSIX_TIME 2018-06-19 14:35 ` [PATCH v2 2/8] xen: introduce XEN_COMPILE_POSIX_TIME Daniel Kiper @ 2018-06-25 13:54 ` Jan Beulich 2018-06-25 14:00 ` Andrew Cooper 0 siblings, 1 reply; 50+ messages in thread From: Jan Beulich @ 2018-06-25 13:54 UTC (permalink / raw) To: Daniel Kiper Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel >>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: > We need the POSIX time to properly fill the TimeDateStamp field in the PE > header. > > Additionally, realign the variables assignment in xen/Makefile to increase > readability. > > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> > --- > v2 - suggestions/fixes: > - derive XEN_COMPILE_POSIX_TIME from XEN_BUILD_DATE > (suggested by Jan Beulich), > - echo 0 if date command does not work > (suggested by Konrad Rzeszutek Wilk), Why would the date command produce an error, other than for not supporting -d? But yes, I'm fine with falling back to zero in that case. If anyone runs into it and cares, they can submit a patch making it work on their platform. > --- a/xen/Makefile > +++ b/xen/Makefile > @@ -6,12 +6,13 @@ export XEN_EXTRAVERSION ?= -rc$(XEN_VENDORVERSION) > export XEN_FULLVERSION = $(XEN_VERSION).$(XEN_SUBVERSION)$(XEN_EXTRAVERSION) > -include xen-version > > -export XEN_WHOAMI ?= $(USER) > -export XEN_DOMAIN ?= $(shell ([ -x /bin/dnsdomainname ] && /bin/dnsdomainname) || ([ -x /bin/domainname ] && /bin/domainname || echo [unknown])) > -export XEN_BUILD_DATE ?= $(shell LC_ALL=C date) > -export XEN_BUILD_TIME ?= $(shell LC_ALL=C date -d '$(XEN_BUILD_DATE)' +%T) > -export XEN_BUILD_HOST ?= $(shell hostname) > -export XEN_CONFIG_EXPERT ?= n > +export XEN_WHOAMI ?= $(USER) > +export XEN_DOMAIN ?= $(shell ([ -x /bin/dnsdomainname ] && /bin/dnsdomainname) || ([ -x /bin/domainname ] && /bin/domainname || echo [unknown])) > +export XEN_BUILD_DATE ?= $(shell LC_ALL=C date) > +export XEN_BUILD_TIME ?= $(shell LC_ALL=C date -d '$(XEN_BUILD_DATE)' +%T) > +export XEN_BUILD_POSIX_TIME ?= $(shell LC_ALL=C date -d '$(XEN_BUILD_DATE)' +%s || echo 0) > +export XEN_BUILD_HOST ?= $(shell hostname) > +export XEN_CONFIG_EXPERT ?= n To be honest I'd prefer if you avoided the re-indentation. Especially the XEN_DOMAIN line is already pretty long, so it would seem better to me to accept the mis-alignment the new setting will have. I continue to not be overly happy anyway with this being put here when it's needed only in a single place (and the transformation could presumably be easily done there, without the need for any new global environment variable). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 2/8] xen: introduce XEN_COMPILE_POSIX_TIME 2018-06-25 13:54 ` Jan Beulich @ 2018-06-25 14:00 ` Andrew Cooper 2018-07-04 12:19 ` Daniel Kiper 0 siblings, 1 reply; 50+ messages in thread From: Andrew Cooper @ 2018-06-25 14:00 UTC (permalink / raw) To: Jan Beulich, Daniel Kiper Cc: Stefano Stabellini, Wei Liu, George Dunlap, Tamas K Lengyel, Ian Jackson, Tim Deegan, Julien Grall, xen-devel On 25/06/18 14:54, Jan Beulich wrote: >>>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: >> We need the POSIX time to properly fill the TimeDateStamp field in the PE >> header. >> >> Additionally, realign the variables assignment in xen/Makefile to increase >> readability. >> >> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> >> --- >> v2 - suggestions/fixes: >> - derive XEN_COMPILE_POSIX_TIME from XEN_BUILD_DATE >> (suggested by Jan Beulich), >> - echo 0 if date command does not work >> (suggested by Konrad Rzeszutek Wilk), > Why would the date command produce an error, other than for not > supporting -d? But yes, I'm fine with falling back to zero in that case. > If anyone runs into it and cares, they can submit a patch making it > work on their platform. > >> --- a/xen/Makefile >> +++ b/xen/Makefile >> @@ -6,12 +6,13 @@ export XEN_EXTRAVERSION ?= -rc$(XEN_VENDORVERSION) >> export XEN_FULLVERSION = $(XEN_VERSION).$(XEN_SUBVERSION)$(XEN_EXTRAVERSION) >> -include xen-version >> >> -export XEN_WHOAMI ?= $(USER) >> -export XEN_DOMAIN ?= $(shell ([ -x /bin/dnsdomainname ] && /bin/dnsdomainname) || ([ -x /bin/domainname ] && /bin/domainname || echo [unknown])) >> -export XEN_BUILD_DATE ?= $(shell LC_ALL=C date) >> -export XEN_BUILD_TIME ?= $(shell LC_ALL=C date -d '$(XEN_BUILD_DATE)' +%T) >> -export XEN_BUILD_HOST ?= $(shell hostname) >> -export XEN_CONFIG_EXPERT ?= n >> +export XEN_WHOAMI ?= $(USER) >> +export XEN_DOMAIN ?= $(shell ([ -x /bin/dnsdomainname ] && /bin/dnsdomainname) || ([ -x /bin/domainname ] && /bin/domainname || echo [unknown])) >> +export XEN_BUILD_DATE ?= $(shell LC_ALL=C date) >> +export XEN_BUILD_TIME ?= $(shell LC_ALL=C date -d '$(XEN_BUILD_DATE)' +%T) >> +export XEN_BUILD_POSIX_TIME ?= $(shell LC_ALL=C date -d '$(XEN_BUILD_DATE)' +%s || echo 0) >> +export XEN_BUILD_HOST ?= $(shell hostname) >> +export XEN_CONFIG_EXPERT ?= n > To be honest I'd prefer if you avoided the re-indentation. Especially the > XEN_DOMAIN line is already pretty long, so it would seem better to me to > accept the mis-alignment the new setting will have. I continue to not be > overly happy anyway with this being put here when it's needed only in a > single place (and the transformation could presumably be easily done > there, without the need for any new global environment variable). Why are we adding yet more Xen specific logic to implement the same as SOURCE_DATE_EPOCH from the reproducible-builds.org effort? ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 2/8] xen: introduce XEN_COMPILE_POSIX_TIME 2018-06-25 14:00 ` Andrew Cooper @ 2018-07-04 12:19 ` Daniel Kiper 0 siblings, 0 replies; 50+ messages in thread From: Daniel Kiper @ 2018-07-04 12:19 UTC (permalink / raw) To: Andrew Cooper Cc: Stefano Stabellini, Wei Liu, George Dunlap, Tamas K Lengyel, Ian Jackson, Tim Deegan, Julien Grall, Jan Beulich, xen-devel On Mon, Jun 25, 2018 at 03:00:07PM +0100, Andrew Cooper wrote: > On 25/06/18 14:54, Jan Beulich wrote: > >>>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: > >> We need the POSIX time to properly fill the TimeDateStamp field in the PE > >> header. > >> > >> Additionally, realign the variables assignment in xen/Makefile to increase > >> readability. > >> > >> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> > >> --- > >> v2 - suggestions/fixes: > >> - derive XEN_COMPILE_POSIX_TIME from XEN_BUILD_DATE > >> (suggested by Jan Beulich), > >> - echo 0 if date command does not work > >> (suggested by Konrad Rzeszutek Wilk), > > Why would the date command produce an error, other than for not > > supporting -d? But yes, I'm fine with falling back to zero in that case. > > If anyone runs into it and cares, they can submit a patch making it > > work on their platform. > > > >> --- a/xen/Makefile > >> +++ b/xen/Makefile > >> @@ -6,12 +6,13 @@ export XEN_EXTRAVERSION ?= -rc$(XEN_VENDORVERSION) > >> export XEN_FULLVERSION = $(XEN_VERSION).$(XEN_SUBVERSION)$(XEN_EXTRAVERSION) > >> -include xen-version > >> > >> -export XEN_WHOAMI ?= $(USER) > >> -export XEN_DOMAIN ?= $(shell ([ -x /bin/dnsdomainname ] && /bin/dnsdomainname) || ([ -x /bin/domainname ] && /bin/domainname || echo [unknown])) > >> -export XEN_BUILD_DATE ?= $(shell LC_ALL=C date) > >> -export XEN_BUILD_TIME ?= $(shell LC_ALL=C date -d '$(XEN_BUILD_DATE)' +%T) > >> -export XEN_BUILD_HOST ?= $(shell hostname) > >> -export XEN_CONFIG_EXPERT ?= n > >> +export XEN_WHOAMI ?= $(USER) > >> +export XEN_DOMAIN ?= $(shell ([ -x /bin/dnsdomainname ] && /bin/dnsdomainname) || ([ -x /bin/domainname ] && /bin/domainname || echo [unknown])) > >> +export XEN_BUILD_DATE ?= $(shell LC_ALL=C date) > >> +export XEN_BUILD_TIME ?= $(shell LC_ALL=C date -d '$(XEN_BUILD_DATE)' +%T) > >> +export XEN_BUILD_POSIX_TIME ?= $(shell LC_ALL=C date -d '$(XEN_BUILD_DATE)' +%s || echo 0) > >> +export XEN_BUILD_HOST ?= $(shell hostname) > >> +export XEN_CONFIG_EXPERT ?= n > > To be honest I'd prefer if you avoided the re-indentation. Especially the > > XEN_DOMAIN line is already pretty long, so it would seem better to me to > > accept the mis-alignment the new setting will have. I continue to not be > > overly happy anyway with this being put here when it's needed only in a > > single place (and the transformation could presumably be easily done > > there, without the need for any new global environment variable). > > Why are we adding yet more Xen specific logic to implement the same as > SOURCE_DATE_EPOCH from the reproducible-builds.org effort? Do you suggest that we should migrate to this thing? If yes what we should use as a reference? A source code file? Which one? AIUI regular "date" command does not work here. Daniel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v2 3/8] xen/x86: manually build xen.mb.efi binary 2018-06-19 14:35 [PATCH v2 0/8] Change xen.efi build and add SHIM_LOCK verification into efi_multiboot2() Daniel Kiper 2018-06-19 14:35 ` [PATCH v2 1/8] xen: calculate XEN_BUILD_TIME using XEN_BUILD_DATE value Daniel Kiper 2018-06-19 14:35 ` [PATCH v2 2/8] xen: introduce XEN_COMPILE_POSIX_TIME Daniel Kiper @ 2018-06-19 14:35 ` Daniel Kiper 2018-06-25 15:36 ` Jan Beulich 2018-06-19 14:35 ` [PATCH v2 4/8] xen/x86: add some addresses to the Multiboot header Daniel Kiper ` (4 subsequent siblings) 7 siblings, 1 reply; 50+ messages in thread From: Daniel Kiper @ 2018-06-19 14:35 UTC (permalink / raw) To: xen-devel Cc: sstabellini, wei.liu2, george.dunlap, andrew.cooper3, ian.jackson, tim, julien.grall, jbeulich, tamas.k.lengyel This patch introduces xen.mb.efi which contains PE header build manually. The code executed by it is almost the same like the code executed by currently existing xen.efi. However, the file layout is simpler and completely different. This way we have better control on PE image. Hence, it allow us to play tricks which are not feasible in xen.efi. This is the first step to get: - one binary which can be loaded by the UEFI loader, Multiboot and Multiboot2 protocols, - UEFI secure boot support when Xen is loaded via Multiboot2 protocol, - if we wish, in the future we can drop xen/xen.gz and build xen.mb.efi only, - crash dumps generated by the xen.mb.efi loaded from the EFI loader can be analyzed by crash tool, - simpler code, - simpler build, - Xen build will no longer depend on ld i386pep support. Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> --- v2 - suggestions/fixes: - align __pe_SizeOfImage at 16 MiB (suggested by Jan Beulich), - use GLOBAL() instead of ENTRY() for EFI PE header labels (suggested by Boris Ostrovsky), - improve comments (suggested by Konrad Rzeszutek Wilk). Not done: - ASM PE header conversion to C; not feasible, - DOS stub code reduction; experiments showed that DOS stub code size cannot be changed due to some bugs in applications playing with PE files, e.g. objdump (more about the issue can be found in the patch itself); so, I think that if it is not possible to reduce the size of code then it does make sens change the code itself; hence, it pays to leave common DOS stub code as is. --- xen/Makefile | 6 +- xen/arch/x86/Makefile | 3 + xen/arch/x86/Rules.mk | 2 + xen/arch/x86/boot/head.S | 150 +++++++++++++++++++++++++++++++++++++++++++ xen/arch/x86/efi/efi-boot.h | 22 ++++++- xen/arch/x86/efi/stub.c | 12 +++- xen/arch/x86/xen.lds.S | 28 ++++++++ xen/include/xen/efi.h | 1 + 8 files changed, 221 insertions(+), 3 deletions(-) diff --git a/xen/Makefile b/xen/Makefile index 1bed339..a49b9b7 100644 --- a/xen/Makefile +++ b/xen/Makefile @@ -61,6 +61,10 @@ _install: $(TARGET)$(CONFIG_XEN_INSTALL_SUFFIX) ln -f -s $(T)-$(XEN_FULLVERSION)$(Z) $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION).$(XEN_SUBVERSION)$(Z) ln -f -s $(T)-$(XEN_FULLVERSION)$(Z) $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION)$(Z) ln -f -s $(T)-$(XEN_FULLVERSION)$(Z) $(D)$(BOOT_DIR)/$(T)$(Z) + $(INSTALL_DATA) $(TARGET).mb.efi $(D)$(BOOT_DIR)/$(T)-$(XEN_FULLVERSION).mb.efi + ln -f -s $(T)-$(XEN_FULLVERSION).mb.efi $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION).$(XEN_SUBVERSION).mb.efi + ln -f -s $(T)-$(XEN_FULLVERSION).mb.efi $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION).mb.efi + ln -f -s $(T)-$(XEN_FULLVERSION).mb.efi $(D)$(BOOT_DIR)/$(T).mb.efi [ -d "$(D)$(DEBUG_DIR)" ] || $(INSTALL_DIR) $(D)$(DEBUG_DIR) $(INSTALL_DATA) $(TARGET)-syms $(D)$(DEBUG_DIR)/$(T)-syms-$(XEN_FULLVERSION) $(INSTALL_DATA) $(TARGET)-syms.map $(D)$(DEBUG_DIR)/$(T)-syms-$(XEN_FULLVERSION).map @@ -121,7 +125,7 @@ _clean: delete-unfresh-files $(MAKE) -f $(BASEDIR)/Rules.mk -C test clean $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) SRCARCH=$(SRCARCH) clean find . \( -name "*.o" -o -name ".*.d" -o -name "*.gcno" \) -exec rm -f {} \; - rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi $(TARGET).efi.map $(TARGET)-syms $(TARGET)-syms.map *~ core + rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi $(TARGET).mb.efi $(TARGET).efi.map $(TARGET)-syms $(TARGET)-syms.map *~ core rm -f include/asm-*/asm-offsets.h rm -f .banner diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index 5563c81..ef3fb51 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -101,6 +101,9 @@ syms-warn-dup-$(CONFIG_SUPPRESS_DUPLICATE_SYMBOL_WARNINGS) := $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32 ./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET) $(XEN_IMG_OFFSET) \ `$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . __2M_rwdata_end$$/0x\1/p'` + $(OBJCOPY) -O binary -S --change-section-address \ + ".efi.pe.header-`$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . __image_base__$$/0x\1/p'`" \ + $(TARGET)-syms $(TARGET).mb.efi ALL_OBJS := $(BASEDIR)/arch/x86/boot/built_in.o $(BASEDIR)/arch/x86/efi/built_in.o $(ALL_OBJS) diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk index ac585a3..51331bf 100644 --- a/xen/arch/x86/Rules.mk +++ b/xen/arch/x86/Rules.mk @@ -7,6 +7,8 @@ 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 += -DXEN_LOAD_ALIGN=XEN_IMG_OFFSET +CFLAGS += -DXEN_FILE_ALIGN=0x20 CFLAGS += '-D__OBJECT_LABEL__=$(subst /,$$,$(subst -,_,$(subst $(BASEDIR)/,,$(CURDIR))/$@))' # Prevent floating-point variables from creeping into Xen. diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index db19ac6..47f254d 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -1,3 +1,4 @@ +#include <xen/compile.h> #include <xen/multiboot.h> #include <xen/multiboot2.h> #include <public/xen.h> @@ -45,6 +46,155 @@ .Lmb2ht_init_end\@: .endm + .section .efi.pe.header, "a", @progbits + +GLOBAL(efi_pe_head) + /* + * Legacy EXE header. + * + * Most of it is copied from binutils package, version 2.30, + * include/coff/pe.h:struct external_PEI_filehdr and + * bfd/peXXigen.c:_bfd_XXi_only_swap_filehdr_out(). + * + * Page is equal 512 bytes here. + * Paragraph is equal 16 bytes here. + */ + .short 0x5a4d /* EXE magic number. */ + .short 0x90 /* Bytes on last page of file. */ + .short 0x3 /* Pages in file. */ + .short 0 /* Relocations. */ + .short 0x4 /* Size of header in paragraphs. */ + .short 0 /* Minimum extra paragraphs needed. */ + .short 0xffff /* Maximum extra paragraphs needed. */ + .short 0 /* Initial (relative) SS value. */ + .short 0xb8 /* Initial SP value. */ + .short 0 /* Checksum. */ + .short 0 /* Initial IP value. */ + .short 0 /* Initial (relative) CS value. */ + .short 0x40 /* File address of relocation table. */ + .short 0 /* Overlay number. */ + .fill 4, 2, 0 /* Reserved words. */ + .short 0 /* OEM identifier. */ + .short 0 /* OEM information. */ + .fill 10, 2, 0 /* Reserved words. */ + .long pe_header - efi_pe_head /* File address of the PE header. */ + + /* + * Standard/Minimal DOS code/data. + * + * It is copied from binutils package, version 2.30, + * include/coff/pe.h:struct external_PEI_filehdr and + * bfd/peXXigen.c:_bfd_XXi_only_swap_filehdr_out(). + * + * Do not change DOS code/data size!!! Some buggy applications + * ignore PE header address from EXE header and look for PE header + * at 0x80 file offset. The size of this code assures that it is + * found at exactly that address. + */ + .long 0x0eba1f0e + .long 0xcd09b400 + .long 0x4c01b821 + .long 0x685421cd + .long 0x70207369 + .long 0x72676f72 + .long 0x63206d61 + .long 0x6f6e6e61 + .long 0x65622074 + .long 0x6e757220 + .long 0x206e6920 + .long 0x20534f44 + .long 0x65646f6d + .long 0x0a0d0d2e + .long 0x24 + .long 0 + + /* + * PE/COFF header. + * + * The PE/COFF format is defined by Microsoft, and is available from + * http://www.microsoft.com/whdc/system/platform/firmware/PECOFF.mspx + * + * Some ideas are taken from Linux kernel and Xen ARM64. + */ + +pe_header: + .ascii "PE\0\0" /* PE signature. */ + .short 0x8664 /* Machine: IMAGE_FILE_MACHINE_AMD64 */ + .short 1 /* NumberOfSections. */ + .long XEN_COMPILE_POSIX_TIME /* TimeDateStamp. */ + .long 0 /* PointerToSymbolTable. */ + .long 0 /* NumberOfSymbols. */ + .short section_table - optional_header /* SizeOfOptionalHeader. */ + .short 0x226 /* Characteristics: + * IMAGE_FILE_EXECUTABLE_IMAGE | + * IMAGE_FILE_LARGE_ADDRESS_AWARE | + * IMAGE_FILE_DEBUG_STRIPPED | + * IMAGE_FILE_LINE_NUMS_STRIPPED + */ + +optional_header: + .short 0x20b /* PE format: PE32+ */ + .byte 0x02 /* MajorLinkerVersion. */ + .byte 0x1e /* MinorLinkerVersion. */ + .long __2M_rwdata_end - efi_pe_head_end /* SizeOfCode. */ + .long 0 /* SizeOfInitializedData. */ + .long 0 /* SizeOfUninitializedData. */ + .long sym_offs(efi_mb_start) /* AddressOfEntryPoint. */ + .long sym_offs(start) /* BaseOfCode. */ + .quad sym_offs(__image_base__) /* ImageBase. */ + .long XEN_LOAD_ALIGN /* SectionAlignment. */ + .long XEN_FILE_ALIGN /* FileAlignment. */ + .short 2 /* MajorOperatingSystemVersion. */ + .short 0 /* MinorOperatingSystemVersion. */ + .short XEN_VERSION /* MajorImageVersion. */ + .short XEN_SUBVERSION /* MinorImageVersion. */ + .short 2 /* MajorSubsystemVersion. */ + .short 0 /* MinorSubsystemVersion. */ + .long 0 /* Win32VersionValue. */ + .long __pe_SizeOfImage /* SizeOfImage. */ + .long efi_pe_head_end - efi_pe_head /* SizeOfHeaders. */ + .long 0 /* CheckSum. */ + .short 0xa /* Subsystem: EFI application. */ + .short 0 /* DllCharacteristics. */ + .quad 0 /* SizeOfStackReserve. */ + .quad 0 /* SizeOfStackCommit. */ + .quad 0 /* SizeOfHeapReserve. */ + .quad 0 /* SizeOfHeapCommit. */ + .long 0 /* LoaderFlags. */ + .long 0x6 /* NumberOfRvaAndSizes. */ + + /* Data Directories. */ + .quad 0 /* Export Table. */ + .quad 0 /* Import Table. */ + .quad 0 /* Resource Table. */ + .quad 0 /* Exception Table. */ + .quad 0 /* Certificate Table. */ + .quad 0 /* Base Relocation Table. */ + +section_table: + .ascii ".text\0\0\0" /* Name. */ + .long __2M_rwdata_end - efi_pe_head_end /* VirtualSize. */ + .long sym_offs(start) /* VirtualAddress. */ + .long __pe_text_raw_end - efi_pe_head_end /* SizeOfRawData. */ + .long efi_pe_head_end - efi_pe_head /* PointerToRawData. */ + .long 0 /* PointerToRelocations. */ + .long 0 /* PointerToLinenumbers. */ + .short 0 /* NumberOfRelocations. */ + .short 0 /* NumberOfLinenumbers. */ + .long 0xe0500020 /* Characteristics: + * IMAGE_SCN_CNT_CODE | + * IMAGE_SCN_ALIGN_16BYTES | + * IMAGE_SCN_MEM_EXECUTE | + * IMAGE_SCN_MEM_READ | + * IMAGE_SCN_MEM_WRITE + */ + + .align XEN_FILE_ALIGN +GLOBAL(efi_pe_head_end) + + .text + .code32 + ENTRY(start) jmp __start diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index 5789d2c..5f0e821 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -31,7 +31,8 @@ static void __init edd_put_string(u8 *dst, size_t n, const char *src) } #define edd_put_string(d, s) edd_put_string(d, ARRAY_SIZE(d), s) -extern const intpte_t __page_tables_start[], __page_tables_end[]; +extern intpte_t __page_tables_start[], __page_tables_end[]; + #define in_page_tables(v) ((intpte_t *)(v) >= __page_tables_start && \ (intpte_t *)(v) < __page_tables_end) @@ -49,6 +50,9 @@ static void __init efi_arch_relocate_image(unsigned long delta) { const struct pe_base_relocs *base_relocs; + if ( efi_enabled(EFI_MB_LOADER) ) + return; + for ( base_relocs = __base_relocs_start; base_relocs < __base_relocs_end; ) { unsigned int i = 0, n; @@ -558,6 +562,7 @@ static void __init efi_arch_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, static void __init efi_arch_memory_setup(void) { + intpte_t *pte; unsigned int i; EFI_STATUS status; @@ -582,6 +587,12 @@ static void __init efi_arch_memory_setup(void) if ( !efi_enabled(EFI_LOADER) ) return; + if ( efi_enabled(EFI_MB_LOADER) ) + for ( pte = __page_tables_start; pte < __page_tables_end; + pte += ( pte != (intpte_t *)l2_identmap ) ? 1 : 4 * L2_PAGETABLE_ENTRIES ) + if ( get_pte_flags(*pte) & _PAGE_PRESENT ) + *pte += xen_phys_start; + /* Initialise L2 identity-map and boot-map page table entries (16MB). */ for ( i = 0; i < 8; ++i ) { @@ -674,6 +685,15 @@ static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable) static void __init efi_arch_flush_dcache_area(const void *vaddr, UINTN size) { } +void EFIAPI efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable); + +void EFIAPI __init noreturn +efi_mb_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) +{ + __set_bit(EFI_MB_LOADER, &efi_flags); + efi_start(ImageHandle, SystemTable); +} + void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) { EFI_GRAPHICS_OUTPUT_PROTOCOL *gop; diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.c index 0c481e3..2142932 100644 --- a/xen/arch/x86/efi/stub.c +++ b/xen/arch/x86/efi/stub.c @@ -14,9 +14,19 @@ * 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. + * efi_mb_start() and efi_multiboot2() are the exceptions. + * Please look below for more details. */ +asm ( + " .text \n" + " .globl efi_mb_start \n" + "efi_mb_start: \n" + " mov %rcx,%rdi \n" + " mov %rdx,%rsi \n" + " call efi_multiboot2 \n" + ); + void __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) { diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index 70afedd..1e5233a 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -60,7 +60,20 @@ SECTIONS start_pa = ABSOLUTE(start - __XEN_VIRT_START); +#ifdef EFI . = __XEN_VIRT_START + XEN_IMG_OFFSET; +#else + /* + * The PE header must be followed by .text section which + * starts at __XEN_VIRT_START + XEN_IMG_OFFSET address. + */ + . = __XEN_VIRT_START + XEN_IMG_OFFSET - efi_pe_head_end + efi_pe_head; + + DECL_SECTION(.efi.pe.header) { + *(.efi.pe.header) + } :NONE +#endif + _start = .; DECL_SECTION(.text) { _stext = .; /* Text and read-only data */ @@ -271,6 +284,9 @@ SECTIONS *(.data.rel) *(.data.rel.*) CONSTRUCTORS + /* PE file must end at XEN_FILE_ALIGN boundary. */ + . = ALIGN(XEN_FILE_ALIGN); + __pe_text_raw_end = .; } :text DECL_SECTION(.bss) { @@ -292,6 +308,8 @@ SECTIONS . = ALIGN(SECTION_ALIGN); __2M_rwdata_end = .; + __pe_SizeOfImage = ALIGN(. - __image_base__, MB(16)); + #ifdef EFI . = ALIGN(4); .reloc : { @@ -315,6 +333,7 @@ SECTIONS *(.discard.*) *(.eh_frame) #ifdef EFI + *(.efi.pe.header) *(.comment) *(.comment.*) *(.note.Xen) @@ -361,3 +380,12 @@ ASSERT((trampoline_end - trampoline_start) < TRAMPOLINE_SPACE - MBI_SPACE_MIN, "not enough room for trampoline and mbi data") ASSERT((wakeup_stack - wakeup_stack_start) >= WAKEUP_STACK_MIN, "wakeup stack too small") + +#ifndef EFI +ASSERT(efi_pe_head_end == _start, "PE header does not end at the beginning of .text section") +ASSERT(_start == __XEN_VIRT_START + XEN_IMG_OFFSET, ".text section begins at wrong address") +ASSERT(IS_ALIGNED(_start, XEN_FILE_ALIGN), "_start misaligned") +ASSERT(IS_ALIGNED(__bss_start, XEN_FILE_ALIGN), "__bss_start misaligned") +ASSERT(IS_ALIGNED(__pe_SizeOfImage, XEN_LOAD_ALIGN), "__pe_SizeOfImage is not multiple of XEN_LOAD_ALIGN") +ASSERT(XEN_LOAD_ALIGN >= XEN_FILE_ALIGN, "XEN_LOAD_ALIGN < XEN_FILE_ALIGN") +#endif diff --git a/xen/include/xen/efi.h b/xen/include/xen/efi.h index 44b7d3e..73f83c1 100644 --- a/xen/include/xen/efi.h +++ b/xen/include/xen/efi.h @@ -11,6 +11,7 @@ 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? */ +#define EFI_MB_LOADER 4 /* xen.mb.efi booted directly from EFI loader? */ /* Add fields here only if they need to be referenced from non-EFI code. */ struct efi { -- 1.7.10.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/8] xen/x86: manually build xen.mb.efi binary 2018-06-19 14:35 ` [PATCH v2 3/8] xen/x86: manually build xen.mb.efi binary Daniel Kiper @ 2018-06-25 15:36 ` Jan Beulich 2018-07-04 14:01 ` Daniel Kiper 0 siblings, 1 reply; 50+ messages in thread From: Jan Beulich @ 2018-06-25 15:36 UTC (permalink / raw) To: Daniel Kiper Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel >>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: > Not done: > - ASM PE header conversion to C; not feasible, Hmm. As long as you can convince Andrew to give you an ack, I won't veto it. But I continue to dislike it, and hence I don't currently foresee myself acking it. > - DOS stub code reduction; experiments showed that DOS stub code size > cannot be changed due to some bugs in applications playing with PE > files, e.g. objdump (more about the issue can be found in the patch > itself); so, I think that if it is not possible to reduce the size > of code then it does make sens change the code itself; hence, it > pays to leave common DOS stub code as is. Even more so here: I'm not sure I care about buggy tools. Did you at least enter a bug report (which you would want to reference in the code comment)? For all of my Win32 life I've been doing fine shrinking DLL/EXE file sizes by moving the PE header to offset 0x40. No tool has even complained. I've just tried objdump 2.25.0 on one of these DLLs - no problem afaics. Did the tool perhaps choke on something other than the non-"standard" offset of the PE header? As to leaving the code as is - if there's anything to be left as is, then the code live binutils would produce, i.e. I'd then ask you to obtain the code at build time, rather than inserting a series of magic hex values in the sources. But as said - even better would be to omit this altogether. > --- a/xen/Makefile > +++ b/xen/Makefile > @@ -61,6 +61,10 @@ _install: $(TARGET)$(CONFIG_XEN_INSTALL_SUFFIX) > ln -f -s $(T)-$(XEN_FULLVERSION)$(Z) > $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION).$(XEN_SUBVERSION)$(Z) > ln -f -s $(T)-$(XEN_FULLVERSION)$(Z) $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION)$(Z) > ln -f -s $(T)-$(XEN_FULLVERSION)$(Z) $(D)$(BOOT_DIR)/$(T)$(Z) > + $(INSTALL_DATA) $(TARGET).mb.efi $(D)$(BOOT_DIR)/$(T)-$(XEN_FULLVERSION).mb.efi > + ln -f -s $(T)-$(XEN_FULLVERSION).mb.efi $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION).$(XEN_SUBVERSION).mb.efi > + ln -f -s $(T)-$(XEN_FULLVERSION).mb.efi $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION).mb.efi > + ln -f -s $(T)-$(XEN_FULLVERSION).mb.efi $(D)$(BOOT_DIR)/$(T).mb.efi This suggests something wants to be macro-ized here, I think. > $(D)$(DEBUG_DIR)/$(T)-syms-$(XEN_FULLVERSION).map > @@ -121,7 +125,7 @@ _clean: delete-unfresh-files > $(MAKE) -f $(BASEDIR)/Rules.mk -C test clean > $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) > SRCARCH=$(SRCARCH) clean > find . \( -name "*.o" -o -name ".*.d" -o -name "*.gcno" \) -exec rm -f {} \; > - rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi $(TARGET).efi.map $(TARGET)-syms $(TARGET)-syms.map *~ core > + rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi $(TARGET).mb.efi $(TARGET).efi.map $(TARGET)-syms $(TARGET)-syms.map *~ core Perhaps simply $(TARGET)*.efi? I don't think we're at risk deleting something precious that way. > --- a/xen/arch/x86/Makefile > +++ b/xen/arch/x86/Makefile > @@ -101,6 +101,9 @@ syms-warn-dup-$(CONFIG_SUPPRESS_DUPLICATE_SYMBOL_WARNINGS) := > $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32 > ./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET) $(XEN_IMG_OFFSET) \ > `$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . __2M_rwdata_end$$/0x\1/p'` > + $(OBJCOPY) -O binary -S --change-section-address \ > + ".efi.pe.header-`$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . __image_base__$$/0x\1/p'`" \ > + $(TARGET)-syms $(TARGET).mb.efi This wants to be a separate rule of a separate $(TARGET).mb.efi target. > +GLOBAL(efi_pe_head) > + /* > + * Legacy EXE header. > + * > + * Most of it is copied from binutils package, version 2.30, > + * include/coff/pe.h:struct external_PEI_filehdr and > + * bfd/peXXigen.c:_bfd_XXi_only_swap_filehdr_out(). > + * > + * Page is equal 512 bytes here. > + * Paragraph is equal 16 bytes here. > + */ > + .short 0x5a4d /* EXE magic number. */ > + .short 0x90 /* Bytes on last page of file. */ > + .short 0x3 /* Pages in file. */ > + .short 0 /* Relocations. */ > + .short 0x4 /* Size of header in paragraphs. */ > + .short 0 /* Minimum extra paragraphs needed. */ > + .short 0xffff /* Maximum extra paragraphs needed. */ > + .short 0 /* Initial (relative) SS value. */ > + .short 0xb8 /* Initial SP value. */ > + .short 0 /* Checksum. */ > + .short 0 /* Initial IP value. */ > + .short 0 /* Initial (relative) CS value. */ > + .short 0x40 /* File address of relocation table. */ This is just the most prominent example: Why is this a hard coded number, while ... > + .short 0 /* Overlay number. */ > + .fill 4, 2, 0 /* Reserved words. */ > + .short 0 /* OEM identifier. */ > + .short 0 /* OEM information. */ > + .fill 10, 2, 0 /* Reserved words. */ > + .long pe_header - efi_pe_head /* File address of the PE header. */ ... this isn't? > + /* > + * PE/COFF header. > + * > + * The PE/COFF format is defined by Microsoft, and is available from > + * http://www.microsoft.com/whdc/system/platform/firmware/PECOFF.mspx > + * > + * Some ideas are taken from Linux kernel and Xen ARM64. > + */ > + > +pe_header: Does this and ones further down really need to be a non-local labels? > + .ascii "PE\0\0" /* PE signature. */ > + .short 0x8664 /* Machine: IMAGE_FILE_MACHINE_AMD64 */ > + .short 1 /* NumberOfSections. */ > + .long XEN_COMPILE_POSIX_TIME /* TimeDateStamp. */ > + .long 0 /* PointerToSymbolTable. */ > + .long 0 /* NumberOfSymbols. */ > + .short section_table - optional_header /* SizeOfOptionalHeader. */ > + .short 0x226 /* Characteristics: > + * IMAGE_FILE_EXECUTABLE_IMAGE | > + * IMAGE_FILE_LARGE_ADDRESS_AWARE | > + * IMAGE_FILE_DEBUG_STRIPPED | > + * IMAGE_FILE_LINE_NUMS_STRIPPED > + */ > + > +optional_header: > + .short 0x20b /* PE format: PE32+ */ > + .byte 0x02 /* MajorLinkerVersion. */ > + .byte 0x1e /* MinorLinkerVersion. */ Let's not cheat more than needed: I'm pretty sure just zeros will do fine here. > + .long __2M_rwdata_end - efi_pe_head_end /* SizeOfCode. */ > + .long 0 /* SizeOfInitializedData. */ > + .long 0 /* SizeOfUninitializedData. */ > + .long sym_offs(efi_mb_start) /* AddressOfEntryPoint. */ > + .long sym_offs(start) /* BaseOfCode. */ > + .quad sym_offs(__image_base__) /* ImageBase. */ The sym_offs() here is certainly different from what xen.efi currently has. With the plan being to have a drop-in replacement, such differences need to be explained to be benign (which here I doubt it is). > + .align XEN_FILE_ALIGN > +GLOBAL(efi_pe_head_end) > + > + .text > + .code32 Why the .code32 here? Perhaps this comes back to the question of whether this whole header should really be lumped into this file. > @@ -582,6 +587,12 @@ static void __init efi_arch_memory_setup(void) > if ( !efi_enabled(EFI_LOADER) ) > return; > > + if ( efi_enabled(EFI_MB_LOADER) ) > + for ( pte = __page_tables_start; pte < __page_tables_end; > + pte += ( pte != (intpte_t *)l2_identmap ) ? 1 : 4 * L2_PAGETABLE_ENTRIES ) Please avoid explicit casts - &l2e_get_intpte(l2_identmap[0]) or something along those lines ought to work here. Same for 4 * L2_PAGETABLE_ENTRIES - you mean ARRAY_SIZE() there. Also this whole code block needs a comment, to explain what it does and also why l2_identmap needs skipping. Furthermore - isn't this off by one, and you process l2_identmap[0] this way, skipping the rest _plus_ the first following entry? I think you'd better use ++ here and have an if() inside the for() body. Then you can also attach the related part of the comment there instead of to the loop header. And I can avoid complaining about the stray spaces inside the parentheses. > @@ -674,6 +685,15 @@ static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable) > > static void __init efi_arch_flush_dcache_area(const void *vaddr, UINTN size) { } > > +void EFIAPI efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable); > + > +void EFIAPI __init noreturn > +efi_mb_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > +{ > + __set_bit(EFI_MB_LOADER, &efi_flags); > + efi_start(ImageHandle, SystemTable); > +} Why yet another entry point? This again speaks against the image being a drop-in replacement for xen.efi. > --- a/xen/arch/x86/xen.lds.S > +++ b/xen/arch/x86/xen.lds.S > @@ -60,7 +60,20 @@ SECTIONS > > start_pa = ABSOLUTE(start - __XEN_VIRT_START); > > +#ifdef EFI > . = __XEN_VIRT_START + XEN_IMG_OFFSET; > +#else > + /* > + * The PE header must be followed by .text section which > + * starts at __XEN_VIRT_START + XEN_IMG_OFFSET address. > + */ > + . = __XEN_VIRT_START + XEN_IMG_OFFSET - efi_pe_head_end + efi_pe_head; > + > + DECL_SECTION(.efi.pe.header) { > + *(.efi.pe.header) > + } :NONE > +#endif The #ifdef wants a brief comment attached I think, since it's quite odd to see the #else side deal with EFI as well. > @@ -271,6 +284,9 @@ SECTIONS > *(.data.rel) > *(.data.rel.*) > CONSTRUCTORS > + /* PE file must end at XEN_FILE_ALIGN boundary. */ > + . = ALIGN(XEN_FILE_ALIGN); > + __pe_text_raw_end = .; Is this really a requirement on the file, or just on the label? > @@ -292,6 +308,8 @@ SECTIONS > . = ALIGN(SECTION_ALIGN); > __2M_rwdata_end = .; > > + __pe_SizeOfImage = ALIGN(. - __image_base__, MB(16)); > + > #ifdef EFI > . = ALIGN(4); > .reloc : { Considering the respective code and comment inside the #ifdef, does your addition really belong ahead of it? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/8] xen/x86: manually build xen.mb.efi binary 2018-06-25 15:36 ` Jan Beulich @ 2018-07-04 14:01 ` Daniel Kiper 2018-07-04 15:27 ` Jan Beulich 0 siblings, 1 reply; 50+ messages in thread From: Daniel Kiper @ 2018-07-04 14:01 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel On Mon, Jun 25, 2018 at 09:36:07AM -0600, Jan Beulich wrote: > >>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: > > Not done: > > - ASM PE header conversion to C; not feasible, > > Hmm. As long as you can convince Andrew to give you an ack, I > won't veto it. But I continue to dislike it, and hence I don't > currently foresee myself acking it. Well, I am not sure why are insisting on C here. Andrew and I have tried to rewrite two different Xen headers to C and we failed. We have discussed that on IRC. If you give us a hint how to proceed further with that then I am happy to try once again. Otherwise I think that it is not fair to require anything which is not technically feasible. Even if I understand that you do not like currently proposed solution. > > - DOS stub code reduction; experiments showed that DOS stub code size > > cannot be changed due to some bugs in applications playing with PE > > files, e.g. objdump (more about the issue can be found in the patch > > itself); so, I think that if it is not possible to reduce the size > > of code then it does make sens change the code itself; hence, it > > pays to leave common DOS stub code as is. > > Even more so here: I'm not sure I care about buggy tools. Did you > at least enter a bug report (which you would want to reference in the > code comment)? For all of my Win32 life I've been doing fine shrinking > DLL/EXE file sizes by moving the PE header to offset 0x40. No tool > has even complained. I've just tried objdump 2.25.0 on one of these > DLLs - no problem afaics. Did the tool perhaps choke on something > other than the non-"standard" offset of the PE header? I have tried objdump from binutils 2.22. It fails. OK, this is fixed in later versions but Xen README says that we support at least binutils 2.16.91.0.5. Of course this is not very big issue but... > As to leaving the code as is - if there's anything to be left as is, then > the code live binutils would produce, i.e. I'd then ask you to obtain > the code at build time, This probably will reintroduce dependency on i386ep emulation which we are trying to avoid. > rather than inserting a series of magic hex IIRC this magic hex is a standard DOS stub used here in there. However, if you do not like it I can open code the stub. > values in the sources. But as said - even better would be to omit this > altogether. > > > --- a/xen/Makefile > > +++ b/xen/Makefile > > @@ -61,6 +61,10 @@ _install: $(TARGET)$(CONFIG_XEN_INSTALL_SUFFIX) > > ln -f -s $(T)-$(XEN_FULLVERSION)$(Z) > > $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION).$(XEN_SUBVERSION)$(Z) > > ln -f -s $(T)-$(XEN_FULLVERSION)$(Z) $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION)$(Z) > > ln -f -s $(T)-$(XEN_FULLVERSION)$(Z) $(D)$(BOOT_DIR)/$(T)$(Z) > > + $(INSTALL_DATA) $(TARGET).mb.efi $(D)$(BOOT_DIR)/$(T)-$(XEN_FULLVERSION).mb.efi > > + ln -f -s $(T)-$(XEN_FULLVERSION).mb.efi $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION).$(XEN_SUBVERSION).mb.efi > > + ln -f -s $(T)-$(XEN_FULLVERSION).mb.efi $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION).mb.efi > > + ln -f -s $(T)-$(XEN_FULLVERSION).mb.efi $(D)$(BOOT_DIR)/$(T).mb.efi > > This suggests something wants to be macro-ized here, I think. I do not think it pays. Last patch renames xen.mb.efi to xen.efi and drops this code. > > $(D)$(DEBUG_DIR)/$(T)-syms-$(XEN_FULLVERSION).map > > @@ -121,7 +125,7 @@ _clean: delete-unfresh-files > > $(MAKE) -f $(BASEDIR)/Rules.mk -C test clean > > $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) > > SRCARCH=$(SRCARCH) clean > > find . \( -name "*.o" -o -name ".*.d" -o -name "*.gcno" \) -exec rm -f {} \; > > - rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi $(TARGET).efi.map $(TARGET)-syms $(TARGET)-syms.map *~ core > > + rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi $(TARGET).mb.efi $(TARGET).efi.map $(TARGET)-syms $(TARGET)-syms.map *~ core > > Perhaps simply $(TARGET)*.efi? I don't think we're at risk deleting something > precious that way. Ditto. > > --- a/xen/arch/x86/Makefile > > +++ b/xen/arch/x86/Makefile > > @@ -101,6 +101,9 @@ syms-warn-dup-$(CONFIG_SUPPRESS_DUPLICATE_SYMBOL_WARNINGS) := > > $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32 > > ./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET) $(XEN_IMG_OFFSET) \ > > `$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . __2M_rwdata_end$$/0x\1/p'` > > + $(OBJCOPY) -O binary -S --change-section-address \ > > + ".efi.pe.header-`$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . __image_base__$$/0x\1/p'`" \ > > + $(TARGET)-syms $(TARGET).mb.efi > > This wants to be a separate rule of a separate $(TARGET).mb.efi target. OK. > > +GLOBAL(efi_pe_head) > > + /* > > + * Legacy EXE header. > > + * > > + * Most of it is copied from binutils package, version 2.30, > > + * include/coff/pe.h:struct external_PEI_filehdr and > > + * bfd/peXXigen.c:_bfd_XXi_only_swap_filehdr_out(). > > + * > > + * Page is equal 512 bytes here. > > + * Paragraph is equal 16 bytes here. > > + */ > > + .short 0x5a4d /* EXE magic number. */ > > + .short 0x90 /* Bytes on last page of file. */ > > + .short 0x3 /* Pages in file. */ > > + .short 0 /* Relocations. */ > > + .short 0x4 /* Size of header in paragraphs. */ > > + .short 0 /* Minimum extra paragraphs needed. */ > > + .short 0xffff /* Maximum extra paragraphs needed. */ > > + .short 0 /* Initial (relative) SS value. */ > > + .short 0xb8 /* Initial SP value. */ > > + .short 0 /* Checksum. */ > > + .short 0 /* Initial IP value. */ > > + .short 0 /* Initial (relative) CS value. */ > > + .short 0x40 /* File address of relocation table. */ > > This is just the most prominent example: Why is this a hard coded Example of what? > number, while ... This is standard DOS stub. Additionally, this is not real relocation table. Just fake one. The pointer points to the DOS stub code. Of course we can change that but does it pays? > > + .short 0 /* Overlay number. */ > > + .fill 4, 2, 0 /* Reserved words. */ > > + .short 0 /* OEM identifier. */ > > + .short 0 /* OEM information. */ > > + .fill 10, 2, 0 /* Reserved words. */ > > + .long pe_header - efi_pe_head /* File address of the PE header. */ > > ... this isn't? > > > > + /* > > + * PE/COFF header. > > + * > > + * The PE/COFF format is defined by Microsoft, and is available from > > + * http://www.microsoft.com/whdc/system/platform/firmware/PECOFF.mspx > > + * > > + * Some ideas are taken from Linux kernel and Xen ARM64. > > + */ > > + > > +pe_header: > > Does this and ones further down really need to be a non-local labels? No, they do not. I will change all of them. > > + .ascii "PE\0\0" /* PE signature. */ > > + .short 0x8664 /* Machine: IMAGE_FILE_MACHINE_AMD64 */ > > + .short 1 /* NumberOfSections. */ > > + .long XEN_COMPILE_POSIX_TIME /* TimeDateStamp. */ > > + .long 0 /* PointerToSymbolTable. */ > > + .long 0 /* NumberOfSymbols. */ > > + .short section_table - optional_header /* SizeOfOptionalHeader. */ > > + .short 0x226 /* Characteristics: > > + * IMAGE_FILE_EXECUTABLE_IMAGE | > > + * IMAGE_FILE_LARGE_ADDRESS_AWARE | > > + * IMAGE_FILE_DEBUG_STRIPPED | > > + * IMAGE_FILE_LINE_NUMS_STRIPPED > > + */ > > + > > +optional_header: > > + .short 0x20b /* PE format: PE32+ */ > > + .byte 0x02 /* MajorLinkerVersion. */ > > + .byte 0x1e /* MinorLinkerVersion. */ > > Let's not cheat more than needed: I'm pretty sure just zeros will do > fine here. OK. > > + .long __2M_rwdata_end - efi_pe_head_end /* SizeOfCode. */ > > + .long 0 /* SizeOfInitializedData. */ > > + .long 0 /* SizeOfUninitializedData. */ > > + .long sym_offs(efi_mb_start) /* AddressOfEntryPoint. */ > > + .long sym_offs(start) /* BaseOfCode. */ > > + .quad sym_offs(__image_base__) /* ImageBase. */ > > The sym_offs() here is certainly different from what xen.efi > currently has. With the plan being to have a drop-in replacement, > such differences need to be explained to be benign (which here > I doubt it is). We share the code with ELF file, so, both have the same __image_base__ address. > > + .align XEN_FILE_ALIGN > > +GLOBAL(efi_pe_head_end) > > + > > + .text > > + .code32 > > Why the .code32 here? Perhaps this comes back to the question of It looks that I have just copied this from the beginning of the file during initial work on this patch. However, there is a chance that it can be dropped. > whether this whole header should really be lumped into this file. I can move it to separate S file if you wish. > > @@ -582,6 +587,12 @@ static void __init efi_arch_memory_setup(void) > > if ( !efi_enabled(EFI_LOADER) ) > > return; > > > > + if ( efi_enabled(EFI_MB_LOADER) ) > > + for ( pte = __page_tables_start; pte < __page_tables_end; > > + pte += ( pte != (intpte_t *)l2_identmap ) ? 1 : 4 * L2_PAGETABLE_ENTRIES ) > > Please avoid explicit casts - &l2e_get_intpte(l2_identmap[0]) or > something along those lines ought to work here. Same for > 4 * L2_PAGETABLE_ENTRIES - you mean ARRAY_SIZE() there. OK. > Also this whole code block needs a comment, to explain what it > does and also why l2_identmap needs skipping. > > Furthermore - isn't this off by one, and you process l2_identmap[0] > this way, skipping the rest _plus_ the first following entry? I think The code mimics similar code in head.S. > you'd better use ++ here and have an if() inside the for() body. > Then you can also attach the related part of the comment there > instead of to the loop header. And I can avoid complaining about > the stray spaces inside the parentheses. If you wish why not... > > @@ -674,6 +685,15 @@ static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable) > > > > static void __init efi_arch_flush_dcache_area(const void *vaddr, UINTN size) { } > > > > +void EFIAPI efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable); > > + > > +void EFIAPI __init noreturn > > +efi_mb_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > > +{ > > + __set_bit(EFI_MB_LOADER, &efi_flags); > > + efi_start(ImageHandle, SystemTable); > > +} > > Why yet another entry point? This again speaks against the image > being a drop-in replacement for xen.efi. At this point I have to differentiate xen.efi and xen.mb.efi. Both require different entry points because the code is a little different in each one. However, this is dropped by last patch which changes xen.mb.efi to xen.efi and removes old xen.efi code. > > --- a/xen/arch/x86/xen.lds.S > > +++ b/xen/arch/x86/xen.lds.S > > @@ -60,7 +60,20 @@ SECTIONS > > > > start_pa = ABSOLUTE(start - __XEN_VIRT_START); > > > > +#ifdef EFI > > . = __XEN_VIRT_START + XEN_IMG_OFFSET; > > +#else > > + /* > > + * The PE header must be followed by .text section which > > + * starts at __XEN_VIRT_START + XEN_IMG_OFFSET address. > > + */ > > + . = __XEN_VIRT_START + XEN_IMG_OFFSET - efi_pe_head_end + efi_pe_head; > > + > > + DECL_SECTION(.efi.pe.header) { > > + *(.efi.pe.header) > > + } :NONE > > +#endif > > The #ifdef wants a brief comment attached I think, since it's quite > odd to see the #else side deal with EFI as well. OK. > > @@ -271,6 +284,9 @@ SECTIONS > > *(.data.rel) > > *(.data.rel.*) > > CONSTRUCTORS > > + /* PE file must end at XEN_FILE_ALIGN boundary. */ > > + . = ALIGN(XEN_FILE_ALIGN); > > + __pe_text_raw_end = .; > > Is this really a requirement on the file, or just on the label? File, so, probably it can be moved behind the label. Though it means that __pe_text_raw_end will not point to the real end of .text section. Do we care? > > @@ -292,6 +308,8 @@ SECTIONS > > . = ALIGN(SECTION_ALIGN); > > __2M_rwdata_end = .; > > > > + __pe_SizeOfImage = ALIGN(. - __image_base__, MB(16)); > > + > > #ifdef EFI > > . = ALIGN(4); > > .reloc : { > > Considering the respective code and comment inside the #ifdef, does > your addition really belong ahead of it? Yes, so, it looks that it requires some comment as code above. Daniel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/8] xen/x86: manually build xen.mb.efi binary 2018-07-04 14:01 ` Daniel Kiper @ 2018-07-04 15:27 ` Jan Beulich 2018-07-04 16:35 ` Daniel Kiper 0 siblings, 1 reply; 50+ messages in thread From: Jan Beulich @ 2018-07-04 15:27 UTC (permalink / raw) To: Daniel Kiper Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel >>> On 04.07.18 at 16:01, <daniel.kiper@oracle.com> wrote: > On Mon, Jun 25, 2018 at 09:36:07AM -0600, Jan Beulich wrote: >> >>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: >> > - DOS stub code reduction; experiments showed that DOS stub code size >> > cannot be changed due to some bugs in applications playing with PE >> > files, e.g. objdump (more about the issue can be found in the patch >> > itself); so, I think that if it is not possible to reduce the size >> > of code then it does make sens change the code itself; hence, it >> > pays to leave common DOS stub code as is. >> >> Even more so here: I'm not sure I care about buggy tools. Did you >> at least enter a bug report (which you would want to reference in the >> code comment)? For all of my Win32 life I've been doing fine shrinking >> DLL/EXE file sizes by moving the PE header to offset 0x40. No tool >> has even complained. I've just tried objdump 2.25.0 on one of these >> DLLs - no problem afaics. Did the tool perhaps choke on something >> other than the non-"standard" offset of the PE header? > > I have tried objdump from binutils 2.22. It fails. OK, this is fixed > in later versions but Xen README says that we support at least binutils > 2.16.91.0.5. And objdump is needed again in which step of the build process? > Of course this is not very big issue but... > >> As to leaving the code as is - if there's anything to be left as is, then >> the code live binutils would produce, i.e. I'd then ask you to obtain >> the code at build time, > > This probably will reintroduce dependency on i386ep emulation which > we are trying to avoid. Hmm, true. >> > --- a/xen/Makefile >> > +++ b/xen/Makefile >> > @@ -61,6 +61,10 @@ _install: $(TARGET)$(CONFIG_XEN_INSTALL_SUFFIX) >> > ln -f -s $(T)-$(XEN_FULLVERSION)$(Z) >> > $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION).$(XEN_SUBVERSION)$(Z) >> > ln -f -s $(T)-$(XEN_FULLVERSION)$(Z) $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION)$(Z) >> > ln -f -s $(T)-$(XEN_FULLVERSION)$(Z) $(D)$(BOOT_DIR)/$(T)$(Z) >> > + $(INSTALL_DATA) $(TARGET).mb.efi $(D)$(BOOT_DIR)/$(T)-$(XEN_FULLVERSION).mb.efi >> > + ln -f -s $(T)-$(XEN_FULLVERSION).mb.efi $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION).$(XEN_SUBVERSION).mb.efi >> > + ln -f -s $(T)-$(XEN_FULLVERSION).mb.efi $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION).mb.efi >> > + ln -f -s $(T)-$(XEN_FULLVERSION).mb.efi $(D)$(BOOT_DIR)/$(T).mb.efi >> >> This suggests something wants to be macro-ized here, I think. > > I do not think it pays. Last patch renames xen.mb.efi to xen.efi > and drops this code. Right - I hadn't seen the later patches yet. So if everything is to go in together, then fine with me. >> > $(D)$(DEBUG_DIR)/$(T)-syms-$(XEN_FULLVERSION).map >> > @@ -121,7 +125,7 @@ _clean: delete-unfresh-files >> > $(MAKE) -f $(BASEDIR)/Rules.mk -C test clean >> > $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) >> > SRCARCH=$(SRCARCH) clean >> > find . \( -name "*.o" -o -name ".*.d" -o -name "*.gcno" \) -exec rm -f {} \; >> > - rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi $(TARGET).efi.map $(TARGET)-syms $(TARGET)-syms.map *~ core >> > + rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi $(TARGET).mb.efi $(TARGET).efi.map $(TARGET)-syms $(TARGET)-syms.map *~ core >> >> Perhaps simply $(TARGET)*.efi? I don't think we're at risk deleting > something >> precious that way. > > Ditto. No exactly - you leave this line alone in the later patch then. >> > +GLOBAL(efi_pe_head) >> > + /* >> > + * Legacy EXE header. >> > + * >> > + * Most of it is copied from binutils package, version 2.30, >> > + * include/coff/pe.h:struct external_PEI_filehdr and >> > + * bfd/peXXigen.c:_bfd_XXi_only_swap_filehdr_out(). >> > + * >> > + * Page is equal 512 bytes here. >> > + * Paragraph is equal 16 bytes here. >> > + */ >> > + .short 0x5a4d /* EXE magic number. */ >> > + .short 0x90 /* Bytes on last page of file. */ >> > + .short 0x3 /* Pages in file. */ >> > + .short 0 /* Relocations. */ >> > + .short 0x4 /* Size of header in paragraphs. */ >> > + .short 0 /* Minimum extra paragraphs needed. */ >> > + .short 0xffff /* Maximum extra paragraphs needed. */ >> > + .short 0 /* Initial (relative) SS value. */ >> > + .short 0xb8 /* Initial SP value. */ >> > + .short 0 /* Checksum. */ >> > + .short 0 /* Initial IP value. */ >> > + .short 0 /* Initial (relative) CS value. */ >> > + .short 0x40 /* File address of relocation table. */ >> >> This is just the most prominent example: Why is this a hard coded > > Example of what? Example of questionable uses of plain numbers. >> number, while ... > > This is standard DOS stub. Additionally, this is not real relocation > table. Just fake one. The pointer points to the DOS stub code. Of > course we can change that but does it pays? Put yourself in the position of a reader not knowing all the details. The first question on any of these plain numbers will be: Why is it this number, and not some arbitrary other one. Something like "number of sections" can still be derived if necessary, but I'd prefer if you used calculations instead of raw numbers wherever possible. >> > + .long __2M_rwdata_end - efi_pe_head_end /* SizeOfCode. */ >> > + .long 0 /* SizeOfInitializedData. */ >> > + .long 0 /* SizeOfUninitializedData. */ >> > + .long sym_offs(efi_mb_start) /* AddressOfEntryPoint. */ >> > + .long sym_offs(start) /* BaseOfCode. */ >> > + .quad sym_offs(__image_base__) /* ImageBase. */ >> >> The sym_offs() here is certainly different from what xen.efi >> currently has. With the plan being to have a drop-in replacement, >> such differences need to be explained to be benign (which here >> I doubt it is). > > We share the code with ELF file, so, both have the same __image_base__ > address. But the question wasn't about __image_base__, but the use of sym_offs() on it. Again - for this new binary to be a drop-in replacement for the current xen.efi, all such differences need to be explained. >> > + .align XEN_FILE_ALIGN >> > +GLOBAL(efi_pe_head_end) >> > + >> > + .text >> > + .code32 >> >> Why the .code32 here? Perhaps this comes back to the question of > > It looks that I have just copied this from the beginning of the file > during initial work on this patch. However, there is a chance that it > can be dropped. > >> whether this whole header should really be lumped into this file. > > I can move it to separate S file if you wish. This would help readability quite a bit, I think. >> > @@ -582,6 +587,12 @@ static void __init efi_arch_memory_setup(void) >> > if ( !efi_enabled(EFI_LOADER) ) >> > return; >> > >> > + if ( efi_enabled(EFI_MB_LOADER) ) >> > + for ( pte = __page_tables_start; pte < __page_tables_end; >> > + pte += ( pte != (intpte_t *)l2_identmap ) ? 1 : 4 * L2_PAGETABLE_ENTRIES ) >> >> Please avoid explicit casts - &l2e_get_intpte(l2_identmap[0]) or >> something along those lines ought to work here. Same for >> 4 * L2_PAGETABLE_ENTRIES - you mean ARRAY_SIZE() there. > > OK. > >> Also this whole code block needs a comment, to explain what it >> does and also why l2_identmap needs skipping. >> >> Furthermore - isn't this off by one, and you process l2_identmap[0] >> this way, skipping the rest _plus_ the first following entry? I think > > The code mimics similar code in head.S. I can't see a similar off-by-1 in head.S. >> > @@ -271,6 +284,9 @@ SECTIONS >> > *(.data.rel) >> > *(.data.rel.*) >> > CONSTRUCTORS >> > + /* PE file must end at XEN_FILE_ALIGN boundary. */ >> > + . = ALIGN(XEN_FILE_ALIGN); >> > + __pe_text_raw_end = .; >> >> Is this really a requirement on the file, or just on the label? > > File, so, probably it can be moved behind the label. Though it means > that __pe_text_raw_end will not point to the real end of .text section. This is an answer contradicting itself: If the requirement indeed is on the file, then things need to remain as is. I'm wondering though what entity would enforce this requirement (if such exists in the first place). >> > @@ -292,6 +308,8 @@ SECTIONS >> > . = ALIGN(SECTION_ALIGN); >> > __2M_rwdata_end = .; >> > >> > + __pe_SizeOfImage = ALIGN(. - __image_base__, MB(16)); >> > + >> > #ifdef EFI >> > . = ALIGN(4); >> > .reloc : { >> >> Considering the respective code and comment inside the #ifdef, does >> your addition really belong ahead of it? > > Yes, so, it looks that it requires some comment as code above. I'm afraid I don't understand. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/8] xen/x86: manually build xen.mb.efi binary 2018-07-04 15:27 ` Jan Beulich @ 2018-07-04 16:35 ` Daniel Kiper 2018-07-05 8:18 ` Jan Beulich 0 siblings, 1 reply; 50+ messages in thread From: Daniel Kiper @ 2018-07-04 16:35 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel On Wed, Jul 04, 2018 at 09:27:43AM -0600, Jan Beulich wrote: > >>> On 04.07.18 at 16:01, <daniel.kiper@oracle.com> wrote: > > On Mon, Jun 25, 2018 at 09:36:07AM -0600, Jan Beulich wrote: > >> >>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: > >> > - DOS stub code reduction; experiments showed that DOS stub code size > >> > cannot be changed due to some bugs in applications playing with PE > >> > files, e.g. objdump (more about the issue can be found in the patch > >> > itself); so, I think that if it is not possible to reduce the size > >> > of code then it does make sens change the code itself; hence, it > >> > pays to leave common DOS stub code as is. > >> > >> Even more so here: I'm not sure I care about buggy tools. Did you > >> at least enter a bug report (which you would want to reference in the > >> code comment)? For all of my Win32 life I've been doing fine shrinking > >> DLL/EXE file sizes by moving the PE header to offset 0x40. No tool > >> has even complained. I've just tried objdump 2.25.0 on one of these > >> DLLs - no problem afaics. Did the tool perhaps choke on something > >> other than the non-"standard" offset of the PE header? > > > > I have tried objdump from binutils 2.22. It fails. OK, this is fixed > > in later versions but Xen README says that we support at least binutils > > 2.16.91.0.5. > > And objdump is needed again in which step of the build process? It is not needed by the build process. However, if somebody builds Xen with old binutils then he/she will not be able to do PE analysis with objdump on the same machine. Nothing scary but a bit annoying... [...] > >> > $(D)$(DEBUG_DIR)/$(T)-syms-$(XEN_FULLVERSION).map > >> > @@ -121,7 +125,7 @@ _clean: delete-unfresh-files > >> > $(MAKE) -f $(BASEDIR)/Rules.mk -C test clean > >> > $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) > >> > SRCARCH=$(SRCARCH) clean > >> > find . \( -name "*.o" -o -name ".*.d" -o -name "*.gcno" \) -exec rm -f {} \; > >> > - rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi $(TARGET).efi.map $(TARGET)-syms $(TARGET)-syms.map *~ core > >> > + rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi $(TARGET).mb.efi $(TARGET).efi.map $(TARGET)-syms $(TARGET)-syms.map *~ core > >> > >> Perhaps simply $(TARGET)*.efi? I don't think we're at risk deleting > > something > >> precious that way. > > > > Ditto. > > No exactly - you leave this line alone in the later patch then. Last patch touches this line and drops "$(TARGET).mb.efi $(TARGET).efi.map". > >> > +GLOBAL(efi_pe_head) > >> > + /* > >> > + * Legacy EXE header. > >> > + * > >> > + * Most of it is copied from binutils package, version 2.30, > >> > + * include/coff/pe.h:struct external_PEI_filehdr and > >> > + * bfd/peXXigen.c:_bfd_XXi_only_swap_filehdr_out(). > >> > + * > >> > + * Page is equal 512 bytes here. > >> > + * Paragraph is equal 16 bytes here. > >> > + */ > >> > + .short 0x5a4d /* EXE magic number. */ > >> > + .short 0x90 /* Bytes on last page of file. */ > >> > + .short 0x3 /* Pages in file. */ > >> > + .short 0 /* Relocations. */ > >> > + .short 0x4 /* Size of header in paragraphs. */ > >> > + .short 0 /* Minimum extra paragraphs needed. */ > >> > + .short 0xffff /* Maximum extra paragraphs needed. */ > >> > + .short 0 /* Initial (relative) SS value. */ > >> > + .short 0xb8 /* Initial SP value. */ > >> > + .short 0 /* Checksum. */ > >> > + .short 0 /* Initial IP value. */ > >> > + .short 0 /* Initial (relative) CS value. */ > >> > + .short 0x40 /* File address of relocation table. */ > >> > >> This is just the most prominent example: Why is this a hard coded > > > > Example of what? > > Example of questionable uses of plain numbers. > > >> number, while ... > > > > This is standard DOS stub. Additionally, this is not real relocation > > table. Just fake one. The pointer points to the DOS stub code. Of > > course we can change that but does it pays? > > Put yourself in the position of a reader not knowing all the details. > The first question on any of these plain numbers will be: Why is it > this number, and not some arbitrary other one. Something like > "number of sections" can still be derived if necessary, but I'd > prefer if you used calculations instead of raw numbers wherever > possible. OK, will do. > >> > + .long __2M_rwdata_end - efi_pe_head_end /* SizeOfCode. */ > >> > + .long 0 /* SizeOfInitializedData. */ > >> > + .long 0 /* SizeOfUninitializedData. */ > >> > + .long sym_offs(efi_mb_start) /* AddressOfEntryPoint. */ > >> > + .long sym_offs(start) /* BaseOfCode. */ > >> > + .quad sym_offs(__image_base__) /* ImageBase. */ > >> > >> The sym_offs() here is certainly different from what xen.efi > >> currently has. With the plan being to have a drop-in replacement, > >> such differences need to be explained to be benign (which here > >> I doubt it is). > > > > We share the code with ELF file, so, both have the same __image_base__ > > address. > > But the question wasn't about __image_base__, but the use of sym_offs() > on it. Again - for this new binary to be a drop-in replacement for the > current xen.efi, all such differences need to be explained. OK. Do you wish this particular thing in the code or in the commit message? > >> > + .align XEN_FILE_ALIGN > >> > +GLOBAL(efi_pe_head_end) > >> > + > >> > + .text > >> > + .code32 > >> > >> Why the .code32 here? Perhaps this comes back to the question of > > > > It looks that I have just copied this from the beginning of the file > > during initial work on this patch. However, there is a chance that it > > can be dropped. > > > >> whether this whole header should really be lumped into this file. > > > > I can move it to separate S file if you wish. > > This would help readability quite a bit, I think. OK. > >> > @@ -582,6 +587,12 @@ static void __init efi_arch_memory_setup(void) > >> > if ( !efi_enabled(EFI_LOADER) ) > >> > return; > >> > > >> > + if ( efi_enabled(EFI_MB_LOADER) ) > >> > + for ( pte = __page_tables_start; pte < __page_tables_end; > >> > + pte += ( pte != (intpte_t *)l2_identmap ) ? 1 : 4 * L2_PAGETABLE_ENTRIES ) > >> > >> Please avoid explicit casts - &l2e_get_intpte(l2_identmap[0]) or > >> something along those lines ought to work here. Same for > >> 4 * L2_PAGETABLE_ENTRIES - you mean ARRAY_SIZE() there. > > > > OK. > > > >> Also this whole code block needs a comment, to explain what it > >> does and also why l2_identmap needs skipping. > >> > >> Furthermore - isn't this off by one, and you process l2_identmap[0] > >> this way, skipping the rest _plus_ the first following entry? I think > > > > The code mimics similar code in head.S. > > I can't see a similar off-by-1 in head.S. 662 /* 663 * Update frame addresses in page tables excluding l2_identmap 664 * without its first entry which points to l1_identmap. 665 */ 666 mov $((__page_tables_end-__page_tables_start)/8),%ecx 667 mov $(((l2_identmap-__page_tables_start)/8)+1),%edx 668 1: cmp $((l2_identmap+l2_identmap_sizeof-__page_tables_start)/8),%ecx 669 cmove %edx,%ecx 670 testl $_PAGE_PRESENT,sym_fs(__page_tables_start)-8(,%ecx,8) 671 jz 2f 672 add %esi,sym_fs(__page_tables_start)-8(,%ecx,8) 673 2: loop 1b Anyway, I will add the comment in both places that they should be kept in sync. > >> > @@ -271,6 +284,9 @@ SECTIONS > >> > *(.data.rel) > >> > *(.data.rel.*) > >> > CONSTRUCTORS > >> > + /* PE file must end at XEN_FILE_ALIGN boundary. */ > >> > + . = ALIGN(XEN_FILE_ALIGN); > >> > + __pe_text_raw_end = .; > >> > >> Is this really a requirement on the file, or just on the label? > > > > File, so, probably it can be moved behind the label. Though it means > > that __pe_text_raw_end will not point to the real end of .text section. > > This is an answer contradicting itself: If the requirement indeed > is on the file, then things need to remain as is. I'm wondering > though what entity would enforce this requirement (if such > exists in the first place). I am not sure what kind of entity you think about. > >> > @@ -292,6 +308,8 @@ SECTIONS > >> > . = ALIGN(SECTION_ALIGN); > >> > __2M_rwdata_end = .; > >> > > >> > + __pe_SizeOfImage = ALIGN(. - __image_base__, MB(16)); > >> > + > >> > #ifdef EFI > >> > . = ALIGN(4); > >> > .reloc : { > >> > >> Considering the respective code and comment inside the #ifdef, does > >> your addition really belong ahead of it? > > > > Yes, so, it looks that it requires some comment as code above. > > I'm afraid I don't understand. Yes, __pe_SizeOfImage ... code belongs ahead of #ifdef EFI. Looking at your question it seems to me that I should put a few words of comment here to clarify why I do that thing. Daniel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/8] xen/x86: manually build xen.mb.efi binary 2018-07-04 16:35 ` Daniel Kiper @ 2018-07-05 8:18 ` Jan Beulich 2018-07-06 14:02 ` Daniel Kiper 0 siblings, 1 reply; 50+ messages in thread From: Jan Beulich @ 2018-07-05 8:18 UTC (permalink / raw) To: Daniel Kiper Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel >>> On 04.07.18 at 18:35, <daniel.kiper@oracle.com> wrote: > On Wed, Jul 04, 2018 at 09:27:43AM -0600, Jan Beulich wrote: >> >>> On 04.07.18 at 16:01, <daniel.kiper@oracle.com> wrote: >> > On Mon, Jun 25, 2018 at 09:36:07AM -0600, Jan Beulich wrote: >> >> >>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: >> >> > $(D)$(DEBUG_DIR)/$(T)-syms-$(XEN_FULLVERSION).map >> >> > @@ -121,7 +125,7 @@ _clean: delete-unfresh-files >> >> > $(MAKE) -f $(BASEDIR)/Rules.mk -C test clean >> >> > $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) >> >> > SRCARCH=$(SRCARCH) clean >> >> > find . \( -name "*.o" -o -name ".*.d" -o -name "*.gcno" \) -exec rm -f {} \; >> >> > - rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi $(TARGET).efi.map $(TARGET)-syms $(TARGET)-syms.map *~ core >> >> > + rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi $(TARGET).mb.efi $(TARGET).efi.map $(TARGET)-syms $(TARGET)-syms.map *~ core >> >> >> >> Perhaps simply $(TARGET)*.efi? I don't think we're at risk deleting >> > something >> >> precious that way. >> > >> > Ditto. >> >> No exactly - you leave this line alone in the later patch then. > > Last patch touches this line and drops "$(TARGET).mb.efi $(TARGET).efi.map". Oh, sorry - I meant to say "Not exactly - you can simply leave this line alone in the later patch then." >> >> > + .long __2M_rwdata_end - efi_pe_head_end /* SizeOfCode. */ >> >> > + .long 0 /* SizeOfInitializedData. */ >> >> > + .long 0 /* SizeOfUninitializedData. */ >> >> > + .long sym_offs(efi_mb_start) /* AddressOfEntryPoint. */ >> >> > + .long sym_offs(start) /* BaseOfCode. */ >> >> > + .quad sym_offs(__image_base__) /* ImageBase. */ >> >> >> >> The sym_offs() here is certainly different from what xen.efi >> >> currently has. With the plan being to have a drop-in replacement, >> >> such differences need to be explained to be benign (which here >> >> I doubt it is). >> > >> > We share the code with ELF file, so, both have the same __image_base__ >> > address. >> >> But the question wasn't about __image_base__, but the use of sym_offs() >> on it. Again - for this new binary to be a drop-in replacement for the >> current xen.efi, all such differences need to be explained. > > OK. Do you wish this particular thing in the code or in the commit message? Commit message: You want to explain why the _change_ is correct. >> >> > @@ -582,6 +587,12 @@ static void __init efi_arch_memory_setup(void) >> >> > if ( !efi_enabled(EFI_LOADER) ) >> >> > return; >> >> > >> >> > + if ( efi_enabled(EFI_MB_LOADER) ) >> >> > + for ( pte = __page_tables_start; pte < __page_tables_end; >> >> > + pte += ( pte != (intpte_t *)l2_identmap ) ? 1 : 4 * L2_PAGETABLE_ENTRIES ) >> >> >> >> Please avoid explicit casts - &l2e_get_intpte(l2_identmap[0]) or >> >> something along those lines ought to work here. Same for >> >> 4 * L2_PAGETABLE_ENTRIES - you mean ARRAY_SIZE() there. >> > >> > OK. >> > >> >> Also this whole code block needs a comment, to explain what it >> >> does and also why l2_identmap needs skipping. >> >> >> >> Furthermore - isn't this off by one, and you process l2_identmap[0] >> >> this way, skipping the rest _plus_ the first following entry? I think >> > >> > The code mimics similar code in head.S. >> >> I can't see a similar off-by-1 in head.S. > > 662 /* > 663 * Update frame addresses in page tables excluding l2_identmap > 664 * without its first entry which points to l1_identmap. > 665 */ > 666 mov $((__page_tables_end-__page_tables_start)/8),%ecx > 667 mov $(((l2_identmap-__page_tables_start)/8)+1),%edx > 668 1: cmp $((l2_identmap+l2_identmap_sizeof-__page_tables_start)/8),%ecx > 669 cmove %edx,%ecx > 670 testl $_PAGE_PRESENT,sym_fs(__page_tables_start)-8(,%ecx,8) > 671 jz 2f > 672 add %esi,sym_fs(__page_tables_start)-8(,%ecx,8) > 673 2: loop 1b Well - this is the code in question, but you fail to point out where the off-by-1 is. >> >> > @@ -271,6 +284,9 @@ SECTIONS >> >> > *(.data.rel) >> >> > *(.data.rel.*) >> >> > CONSTRUCTORS >> >> > + /* PE file must end at XEN_FILE_ALIGN boundary. */ >> >> > + . = ALIGN(XEN_FILE_ALIGN); >> >> > + __pe_text_raw_end = .; >> >> >> >> Is this really a requirement on the file, or just on the label? >> > >> > File, so, probably it can be moved behind the label. Though it means >> > that __pe_text_raw_end will not point to the real end of .text section. >> >> This is an answer contradicting itself: If the requirement indeed >> is on the file, then things need to remain as is. I'm wondering >> though what entity would enforce this requirement (if such >> exists in the first place). > > I am not sure what kind of entity you think about. Taking your comment, there must be (a) something said in the spec and (b) its "violation" leading to problems. I guess if I dug carefully enough I might be able to find (a), so it is (b) that I'm asking about. >> >> > @@ -292,6 +308,8 @@ SECTIONS >> >> > . = ALIGN(SECTION_ALIGN); >> >> > __2M_rwdata_end = .; >> >> > >> >> > + __pe_SizeOfImage = ALIGN(. - __image_base__, MB(16)); >> >> > + >> >> > #ifdef EFI >> >> > . = ALIGN(4); >> >> > .reloc : { >> >> >> >> Considering the respective code and comment inside the #ifdef, does >> >> your addition really belong ahead of it? >> > >> > Yes, so, it looks that it requires some comment as code above. >> >> I'm afraid I don't understand. > > Yes, __pe_SizeOfImage ... code belongs ahead of #ifdef EFI. Looking at your > question it seems to me that I should put a few words of comment here to > clarify why I do that thing. Okay; it would have helped if you also gave the reasons here (rather than just saying "yes"). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/8] xen/x86: manually build xen.mb.efi binary 2018-07-05 8:18 ` Jan Beulich @ 2018-07-06 14:02 ` Daniel Kiper 2018-07-06 15:08 ` Jan Beulich 0 siblings, 1 reply; 50+ messages in thread From: Daniel Kiper @ 2018-07-06 14:02 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel On Thu, Jul 05, 2018 at 02:18:03AM -0600, Jan Beulich wrote: > >>> On 04.07.18 at 18:35, <daniel.kiper@oracle.com> wrote: > > On Wed, Jul 04, 2018 at 09:27:43AM -0600, Jan Beulich wrote: > >> >>> On 04.07.18 at 16:01, <daniel.kiper@oracle.com> wrote: > >> > On Mon, Jun 25, 2018 at 09:36:07AM -0600, Jan Beulich wrote: > >> >> >>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: [...] > >> >> > @@ -582,6 +587,12 @@ static void __init efi_arch_memory_setup(void) > >> >> > if ( !efi_enabled(EFI_LOADER) ) > >> >> > return; > >> >> > > >> >> > + if ( efi_enabled(EFI_MB_LOADER) ) > >> >> > + for ( pte = __page_tables_start; pte < __page_tables_end; > >> >> > + pte += ( pte != (intpte_t *)l2_identmap ) ? 1 : 4 * L2_PAGETABLE_ENTRIES ) > >> >> > >> >> Please avoid explicit casts - &l2e_get_intpte(l2_identmap[0]) or > >> >> something along those lines ought to work here. Same for > >> >> 4 * L2_PAGETABLE_ENTRIES - you mean ARRAY_SIZE() there. > >> > > >> > OK. > >> > > >> >> Also this whole code block needs a comment, to explain what it > >> >> does and also why l2_identmap needs skipping. > >> >> > >> >> Furthermore - isn't this off by one, and you process l2_identmap[0] > >> >> this way, skipping the rest _plus_ the first following entry? I think > >> > > >> > The code mimics similar code in head.S. > >> > >> I can't see a similar off-by-1 in head.S. > > > > 662 /* > > 663 * Update frame addresses in page tables excluding l2_identmap > > 664 * without its first entry which points to l1_identmap. > > 665 */ > > 666 mov $((__page_tables_end-__page_tables_start)/8),%ecx > > 667 mov $(((l2_identmap-__page_tables_start)/8)+1),%edx > > 668 1: cmp $((l2_identmap+l2_identmap_sizeof-__page_tables_start)/8),%ecx > > 669 cmove %edx,%ecx > > 670 testl $_PAGE_PRESENT,sym_fs(__page_tables_start)-8(,%ecx,8) > > 671 jz 2f > > 672 add %esi,sym_fs(__page_tables_start)-8(,%ecx,8) > > 673 2: loop 1b > > Well - this is the code in question, but you fail to point out where > the off-by-1 is. Line 667, 668 and 669. > >> >> > @@ -271,6 +284,9 @@ SECTIONS > >> >> > *(.data.rel) > >> >> > *(.data.rel.*) > >> >> > CONSTRUCTORS > >> >> > + /* PE file must end at XEN_FILE_ALIGN boundary. */ > >> >> > + . = ALIGN(XEN_FILE_ALIGN); > >> >> > + __pe_text_raw_end = .; > >> >> > >> >> Is this really a requirement on the file, or just on the label? > >> > > >> > File, so, probably it can be moved behind the label. Though it means > >> > that __pe_text_raw_end will not point to the real end of .text section. > >> > >> This is an answer contradicting itself: If the requirement indeed > >> is on the file, then things need to remain as is. I'm wondering > >> though what entity would enforce this requirement (if such > >> exists in the first place). > > > > I am not sure what kind of entity you think about. > > Taking your comment, there must be (a) something said in the spec > and (b) its "violation" leading to problems. I guess if I dug carefully > enough I might be able to find (a), so it is (b) that I'm asking about. Microsoft Portable Executable and Common Object File Format Specification, Revision 11, says this: FileAlignment: The alignment factor (in bytes) that is used to align the raw data of sections in the image file. The value should be a power of 2 between 512 and 64 K, inclusive. The default is 512. If the SectionAlignment is less than the architecture’s page size, then FileAlignment must match SectionAlignment. And e.g. at least sbsign is very picky and complains if sections are not aligned correctly in the PE file. > >> >> > @@ -292,6 +308,8 @@ SECTIONS > >> >> > . = ALIGN(SECTION_ALIGN); > >> >> > __2M_rwdata_end = .; > >> >> > > >> >> > + __pe_SizeOfImage = ALIGN(. - __image_base__, MB(16)); > >> >> > + > >> >> > #ifdef EFI > >> >> > . = ALIGN(4); > >> >> > .reloc : { > >> >> > >> >> Considering the respective code and comment inside the #ifdef, does > >> >> your addition really belong ahead of it? > >> > > >> > Yes, so, it looks that it requires some comment as code above. > >> > >> I'm afraid I don't understand. > > > > Yes, __pe_SizeOfImage ... code belongs ahead of #ifdef EFI. Looking at your > > question it seems to me that I should put a few words of comment here to > > clarify why I do that thing. > > Okay; it would have helped if you also gave the reasons here (rather > than just saying "yes"). The reason is that xen.mb.efi is build from xen-syms. So, we have to have this symbol defined for ELF output. It does not hurt if we have it defined for current xen.efi too. Daniel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/8] xen/x86: manually build xen.mb.efi binary 2018-07-06 14:02 ` Daniel Kiper @ 2018-07-06 15:08 ` Jan Beulich 2018-07-10 10:48 ` Daniel Kiper 0 siblings, 1 reply; 50+ messages in thread From: Jan Beulich @ 2018-07-06 15:08 UTC (permalink / raw) To: Daniel Kiper Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel >>> On 06.07.18 at 16:02, <daniel.kiper@oracle.com> wrote: > On Thu, Jul 05, 2018 at 02:18:03AM -0600, Jan Beulich wrote: >> >>> On 04.07.18 at 18:35, <daniel.kiper@oracle.com> wrote: >> > On Wed, Jul 04, 2018 at 09:27:43AM -0600, Jan Beulich wrote: >> >> >>> On 04.07.18 at 16:01, <daniel.kiper@oracle.com> wrote: >> >> > On Mon, Jun 25, 2018 at 09:36:07AM -0600, Jan Beulich wrote: >> >> >> >>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: >> >> >> > @@ -582,6 +587,12 @@ static void __init efi_arch_memory_setup(void) >> >> >> > if ( !efi_enabled(EFI_LOADER) ) >> >> >> > return; >> >> >> > >> >> >> > + if ( efi_enabled(EFI_MB_LOADER) ) >> >> >> > + for ( pte = __page_tables_start; pte < __page_tables_end; >> >> >> > + pte += ( pte != (intpte_t *)l2_identmap ) ? 1 : 4 * L2_PAGETABLE_ENTRIES ) >> >> >> >> >> >> Please avoid explicit casts - &l2e_get_intpte(l2_identmap[0]) or >> >> >> something along those lines ought to work here. Same for >> >> >> 4 * L2_PAGETABLE_ENTRIES - you mean ARRAY_SIZE() there. >> >> > >> >> > OK. >> >> > >> >> >> Also this whole code block needs a comment, to explain what it >> >> >> does and also why l2_identmap needs skipping. >> >> >> >> >> >> Furthermore - isn't this off by one, and you process l2_identmap[0] >> >> >> this way, skipping the rest _plus_ the first following entry? I think >> >> > >> >> > The code mimics similar code in head.S. >> >> >> >> I can't see a similar off-by-1 in head.S. >> > >> > 662 /* >> > 663 * Update frame addresses in page tables excluding l2_identmap >> > 664 * without its first entry which points to l1_identmap. >> > 665 */ >> > 666 mov $((__page_tables_end-__page_tables_start)/8),%ecx >> > 667 mov $(((l2_identmap-__page_tables_start)/8)+1),%edx >> > 668 1: cmp $((l2_identmap+l2_identmap_sizeof-__page_tables_start)/8),%ecx >> > 669 cmove %edx,%ecx >> > 670 testl $_PAGE_PRESENT,sym_fs(__page_tables_start)-8(,%ecx,8) >> > 671 jz 2f >> > 672 add %esi,sym_fs(__page_tables_start)-8(,%ecx,8) >> > 673 2: loop 1b >> >> Well - this is the code in question, but you fail to point out where >> the off-by-1 is. > > Line 667, 668 and 669. I don't think so, no. Note the -8 in lines 670 and 672. >> >> >> > @@ -271,6 +284,9 @@ SECTIONS >> >> >> > *(.data.rel) >> >> >> > *(.data.rel.*) >> >> >> > CONSTRUCTORS >> >> >> > + /* PE file must end at XEN_FILE_ALIGN boundary. */ >> >> >> > + . = ALIGN(XEN_FILE_ALIGN); >> >> >> > + __pe_text_raw_end = .; >> >> >> >> >> >> Is this really a requirement on the file, or just on the label? >> >> > >> >> > File, so, probably it can be moved behind the label. Though it means >> >> > that __pe_text_raw_end will not point to the real end of .text section. >> >> >> >> This is an answer contradicting itself: If the requirement indeed >> >> is on the file, then things need to remain as is. I'm wondering >> >> though what entity would enforce this requirement (if such >> >> exists in the first place). >> > >> > I am not sure what kind of entity you think about. >> >> Taking your comment, there must be (a) something said in the spec >> and (b) its "violation" leading to problems. I guess if I dug carefully >> enough I might be able to find (a), so it is (b) that I'm asking about. > > Microsoft Portable Executable and Common Object File Format > Specification, Revision 11, says this: > > FileAlignment: The alignment factor (in bytes) that is used to align > the raw data of sections in the image file. The value should be a power > of 2 between 512 and 64 K, inclusive. The default is 512. If the > SectionAlignment is less than the architecture’s page size, then > FileAlignment must match SectionAlignment. Well, as said before - there's a variety of Windows drivers with much smaller alignment (often 32). > And e.g. at least sbsign is very picky and complains if sections are > not aligned correctly in the PE file. That's possibly relevant, and would make up for a good reason here. In which case, with the comment minimally extended, I'm fine with the change as still visible above. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/8] xen/x86: manually build xen.mb.efi binary 2018-07-06 15:08 ` Jan Beulich @ 2018-07-10 10:48 ` Daniel Kiper 2018-07-10 13:54 ` Jan Beulich 0 siblings, 1 reply; 50+ messages in thread From: Daniel Kiper @ 2018-07-10 10:48 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel On Fri, Jul 06, 2018 at 09:08:29AM -0600, Jan Beulich wrote: > >>> On 06.07.18 at 16:02, <daniel.kiper@oracle.com> wrote: > > On Thu, Jul 05, 2018 at 02:18:03AM -0600, Jan Beulich wrote: > >> >>> On 04.07.18 at 18:35, <daniel.kiper@oracle.com> wrote: > >> > On Wed, Jul 04, 2018 at 09:27:43AM -0600, Jan Beulich wrote: > >> >> >>> On 04.07.18 at 16:01, <daniel.kiper@oracle.com> wrote: > >> >> > On Mon, Jun 25, 2018 at 09:36:07AM -0600, Jan Beulich wrote: > >> >> >> >>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: > >> >> >> > @@ -582,6 +587,12 @@ static void __init efi_arch_memory_setup(void) > >> >> >> > if ( !efi_enabled(EFI_LOADER) ) > >> >> >> > return; > >> >> >> > > >> >> >> > + if ( efi_enabled(EFI_MB_LOADER) ) > >> >> >> > + for ( pte = __page_tables_start; pte < __page_tables_end; > >> >> >> > + pte += ( pte != (intpte_t *)l2_identmap ) ? 1 : 4 * L2_PAGETABLE_ENTRIES ) > >> >> >> > >> >> >> Please avoid explicit casts - &l2e_get_intpte(l2_identmap[0]) or > >> >> >> something along those lines ought to work here. Same for > >> >> >> 4 * L2_PAGETABLE_ENTRIES - you mean ARRAY_SIZE() there. > >> >> > > >> >> > OK. > >> >> > > >> >> >> Also this whole code block needs a comment, to explain what it > >> >> >> does and also why l2_identmap needs skipping. > >> >> >> > >> >> >> Furthermore - isn't this off by one, and you process l2_identmap[0] > >> >> >> this way, skipping the rest _plus_ the first following entry? I think > >> >> > > >> >> > The code mimics similar code in head.S. > >> >> > >> >> I can't see a similar off-by-1 in head.S. > >> > > >> > 662 /* > >> > 663 * Update frame addresses in page tables excluding l2_identmap > >> > 664 * without its first entry which points to l1_identmap. > >> > 665 */ > >> > 666 mov $((__page_tables_end-__page_tables_start)/8),%ecx > >> > 667 mov $(((l2_identmap-__page_tables_start)/8)+1),%edx > >> > 668 1: cmp $((l2_identmap+l2_identmap_sizeof-__page_tables_start)/8),%ecx > >> > 669 cmove %edx,%ecx > >> > 670 testl $_PAGE_PRESENT,sym_fs(__page_tables_start)-8(,%ecx,8) > >> > 671 jz 2f > >> > 672 add %esi,sym_fs(__page_tables_start)-8(,%ecx,8) > >> > 673 2: loop 1b > >> > >> Well - this is the code in question, but you fail to point out where > >> the off-by-1 is. > > > > Line 667, 668 and 669. > > I don't think so, no. Note the -8 in lines 670 and 672. However, you are missing +1 in line 667. > >> >> >> > @@ -271,6 +284,9 @@ SECTIONS > >> >> >> > *(.data.rel) > >> >> >> > *(.data.rel.*) > >> >> >> > CONSTRUCTORS > >> >> >> > + /* PE file must end at XEN_FILE_ALIGN boundary. */ > >> >> >> > + . = ALIGN(XEN_FILE_ALIGN); > >> >> >> > + __pe_text_raw_end = .; > >> >> >> > >> >> >> Is this really a requirement on the file, or just on the label? > >> >> > > >> >> > File, so, probably it can be moved behind the label. Though it means > >> >> > that __pe_text_raw_end will not point to the real end of .text section. > >> >> > >> >> This is an answer contradicting itself: If the requirement indeed > >> >> is on the file, then things need to remain as is. I'm wondering > >> >> though what entity would enforce this requirement (if such > >> >> exists in the first place). > >> > > >> > I am not sure what kind of entity you think about. > >> > >> Taking your comment, there must be (a) something said in the spec > >> and (b) its "violation" leading to problems. I guess if I dug carefully > >> enough I might be able to find (a), so it is (b) that I'm asking about. > > > > Microsoft Portable Executable and Common Object File Format > > Specification, Revision 11, says this: > > > > FileAlignment: The alignment factor (in bytes) that is used to align > > the raw data of sections in the image file. The value should be a power > > of 2 between 512 and 64 K, inclusive. The default is 512. If the > > SectionAlignment is less than the architecture’s page size, then > > FileAlignment must match SectionAlignment. > > Well, as said before - there's a variety of Windows drivers with much > smaller alignment (often 32). The most important is first sentence here. Others I have put just for completeness. We have discussed that earlier. > > And e.g. at least sbsign is very picky and complains if sections are > > not aligned correctly in the PE file. > > That's possibly relevant, and would make up for a good reason here. > In which case, with the comment minimally extended, I'm fine with the > change as still visible above. Sure thing! Daniel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/8] xen/x86: manually build xen.mb.efi binary 2018-07-10 10:48 ` Daniel Kiper @ 2018-07-10 13:54 ` Jan Beulich 2018-07-11 11:41 ` Daniel Kiper 0 siblings, 1 reply; 50+ messages in thread From: Jan Beulich @ 2018-07-10 13:54 UTC (permalink / raw) To: Daniel Kiper Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel >>> On 10.07.18 at 12:48, <daniel.kiper@oracle.com> wrote: > On Fri, Jul 06, 2018 at 09:08:29AM -0600, Jan Beulich wrote: >> >>> On 06.07.18 at 16:02, <daniel.kiper@oracle.com> wrote: >> > On Thu, Jul 05, 2018 at 02:18:03AM -0600, Jan Beulich wrote: >> >> >>> On 04.07.18 at 18:35, <daniel.kiper@oracle.com> wrote: >> >> > On Wed, Jul 04, 2018 at 09:27:43AM -0600, Jan Beulich wrote: >> >> >> >>> On 04.07.18 at 16:01, <daniel.kiper@oracle.com> wrote: >> >> >> > On Mon, Jun 25, 2018 at 09:36:07AM -0600, Jan Beulich wrote: >> >> >> >> >>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: >> >> >> >> > @@ -582,6 +587,12 @@ static void __init efi_arch_memory_setup(void) >> >> >> >> > if ( !efi_enabled(EFI_LOADER) ) >> >> >> >> > return; >> >> >> >> > >> >> >> >> > + if ( efi_enabled(EFI_MB_LOADER) ) >> >> >> >> > + for ( pte = __page_tables_start; pte < __page_tables_end; >> >> >> >> > + pte += ( pte != (intpte_t *)l2_identmap ) ? 1 : 4 * L2_PAGETABLE_ENTRIES ) >> >> >> >> >> >> >> >> Please avoid explicit casts - &l2e_get_intpte(l2_identmap[0]) or >> >> >> >> something along those lines ought to work here. Same for >> >> >> >> 4 * L2_PAGETABLE_ENTRIES - you mean ARRAY_SIZE() there. >> >> >> > >> >> >> > OK. >> >> >> > >> >> >> >> Also this whole code block needs a comment, to explain what it >> >> >> >> does and also why l2_identmap needs skipping. >> >> >> >> >> >> >> >> Furthermore - isn't this off by one, and you process l2_identmap[0] >> >> >> >> this way, skipping the rest _plus_ the first following entry? I think >> >> >> > >> >> >> > The code mimics similar code in head.S. >> >> >> >> >> >> I can't see a similar off-by-1 in head.S. >> >> > >> >> > 662 /* >> >> > 663 * Update frame addresses in page tables excluding l2_identmap >> >> > 664 * without its first entry which points to l1_identmap. >> >> > 665 */ >> >> > 666 mov $((__page_tables_end-__page_tables_start)/8),%ecx >> >> > 667 mov $(((l2_identmap-__page_tables_start)/8)+1),%edx >> >> > 668 1: cmp $((l2_identmap+l2_identmap_sizeof-__page_tables_start)/8),%ecx >> >> > 669 cmove %edx,%ecx >> >> > 670 testl $_PAGE_PRESENT,sym_fs(__page_tables_start)-8(,%ecx,8) >> >> > 671 jz 2f >> >> > 672 add %esi,sym_fs(__page_tables_start)-8(,%ecx,8) >> >> > 673 2: loop 1b >> >> >> >> Well - this is the code in question, but you fail to point out where >> >> the off-by-1 is. >> > >> > Line 667, 668 and 669. >> >> I don't think so, no. Note the -8 in lines 670 and 672. > > However, you are missing +1 in line 667. I don't think I am: The first entry of l2_identmap actually needs processing afaics (and as the comment says), as that's the only one with non-absolute contents. IOW - part of my original comment was wrong, but the other half (you skipping one entry) still seems applicable, as does the part concerning the missing comment. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/8] xen/x86: manually build xen.mb.efi binary 2018-07-10 13:54 ` Jan Beulich @ 2018-07-11 11:41 ` Daniel Kiper [not found] ` <5B45ECF8020000FA04BD1FEB@prv1-mh.provo.novell.com> 0 siblings, 1 reply; 50+ messages in thread From: Daniel Kiper @ 2018-07-11 11:41 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel On Tue, Jul 10, 2018 at 07:54:51AM -0600, Jan Beulich wrote: > >>> On 10.07.18 at 12:48, <daniel.kiper@oracle.com> wrote: > > On Fri, Jul 06, 2018 at 09:08:29AM -0600, Jan Beulich wrote: > >> >>> On 06.07.18 at 16:02, <daniel.kiper@oracle.com> wrote: > >> > On Thu, Jul 05, 2018 at 02:18:03AM -0600, Jan Beulich wrote: > >> >> >>> On 04.07.18 at 18:35, <daniel.kiper@oracle.com> wrote: > >> >> > On Wed, Jul 04, 2018 at 09:27:43AM -0600, Jan Beulich wrote: > >> >> >> >>> On 04.07.18 at 16:01, <daniel.kiper@oracle.com> wrote: > >> >> >> > On Mon, Jun 25, 2018 at 09:36:07AM -0600, Jan Beulich wrote: > >> >> >> >> >>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: > >> >> >> >> > @@ -582,6 +587,12 @@ static void __init efi_arch_memory_setup(void) > >> >> >> >> > if ( !efi_enabled(EFI_LOADER) ) > >> >> >> >> > return; > >> >> >> >> > > >> >> >> >> > + if ( efi_enabled(EFI_MB_LOADER) ) > >> >> >> >> > + for ( pte = __page_tables_start; pte < __page_tables_end; > >> >> >> >> > + pte += ( pte != (intpte_t *)l2_identmap ) ? 1 : 4 * L2_PAGETABLE_ENTRIES ) > >> >> >> >> > >> >> >> >> Please avoid explicit casts - &l2e_get_intpte(l2_identmap[0]) or > >> >> >> >> something along those lines ought to work here. Same for > >> >> >> >> 4 * L2_PAGETABLE_ENTRIES - you mean ARRAY_SIZE() there. > >> >> >> > > >> >> >> > OK. > >> >> >> > > >> >> >> >> Also this whole code block needs a comment, to explain what it > >> >> >> >> does and also why l2_identmap needs skipping. > >> >> >> >> > >> >> >> >> Furthermore - isn't this off by one, and you process l2_identmap[0] > >> >> >> >> this way, skipping the rest _plus_ the first following entry? I think > >> >> >> > > >> >> >> > The code mimics similar code in head.S. > >> >> >> > >> >> >> I can't see a similar off-by-1 in head.S. > >> >> > > >> >> > 662 /* > >> >> > 663 * Update frame addresses in page tables excluding l2_identmap > >> >> > 664 * without its first entry which points to l1_identmap. > >> >> > 665 */ > >> >> > 666 mov $((__page_tables_end-__page_tables_start)/8),%ecx > >> >> > 667 mov $(((l2_identmap-__page_tables_start)/8)+1),%edx > >> >> > 668 1: cmp $((l2_identmap+l2_identmap_sizeof-__page_tables_start)/8),%ecx > >> >> > 669 cmove %edx,%ecx > >> >> > 670 testl $_PAGE_PRESENT,sym_fs(__page_tables_start)-8(,%ecx,8) > >> >> > 671 jz 2f > >> >> > 672 add %esi,sym_fs(__page_tables_start)-8(,%ecx,8) > >> >> > 673 2: loop 1b > >> >> > >> >> Well - this is the code in question, but you fail to point out where > >> >> the off-by-1 is. > >> > > >> > Line 667, 668 and 669. > >> > >> I don't think so, no. Note the -8 in lines 670 and 672. > > > > However, you are missing +1 in line 667. > > I don't think I am: The first entry of l2_identmap actually needs > processing afaics (and as the comment says), as that's the only one > with non-absolute contents. IOW - part of my original comment was > wrong, but the other half (you skipping one entry) still seems > applicable, as does the part concerning the missing comment. It seems correct to me. l2_identmap[0] gets updated and then we move to l3_bootmap[0]. Am I missing something? Daniel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
[parent not found: <5B45ECF8020000FA04BD1FEB@prv1-mh.provo.novell.com>]
* Re: [PATCH v2 3/8] xen/x86: manually build xen.mb.efi binary [not found] ` <5B45ECF8020000FA04BD1FEB@prv1-mh.provo.novell.com> @ 2018-07-11 12:26 ` Jan Beulich 2018-07-12 10:52 ` Daniel Kiper 0 siblings, 1 reply; 50+ messages in thread From: Jan Beulich @ 2018-07-11 12:26 UTC (permalink / raw) To: Daniel Kiper Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel >>> On 11.07.18 at 13:41, <daniel.kiper@oracle.com> wrote: > On Tue, Jul 10, 2018 at 07:54:51AM -0600, Jan Beulich wrote: >> >>> On 10.07.18 at 12:48, <daniel.kiper@oracle.com> wrote: >> > On Fri, Jul 06, 2018 at 09:08:29AM -0600, Jan Beulich wrote: >> >> >>> On 06.07.18 at 16:02, <daniel.kiper@oracle.com> wrote: >> >> > On Thu, Jul 05, 2018 at 02:18:03AM -0600, Jan Beulich wrote: >> >> >> >>> On 04.07.18 at 18:35, <daniel.kiper@oracle.com> wrote: >> >> >> > On Wed, Jul 04, 2018 at 09:27:43AM -0600, Jan Beulich wrote: >> >> >> >> >>> On 04.07.18 at 16:01, <daniel.kiper@oracle.com> wrote: >> >> >> >> > On Mon, Jun 25, 2018 at 09:36:07AM -0600, Jan Beulich wrote: >> >> >> >> >> >>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: >> >> >> >> >> > @@ -582,6 +587,12 @@ static void __init efi_arch_memory_setup(void) >> >> >> >> >> > if ( !efi_enabled(EFI_LOADER) ) >> >> >> >> >> > return; >> >> >> >> >> > >> >> >> >> >> > + if ( efi_enabled(EFI_MB_LOADER) ) >> >> >> >> >> > + for ( pte = __page_tables_start; pte < __page_tables_end; >> >> >> >> >> > + pte += ( pte != (intpte_t *)l2_identmap ) ? 1 : 4 * L2_PAGETABLE_ENTRIES ) >> >> >> >> >> >> >> >> >> >> Please avoid explicit casts - &l2e_get_intpte(l2_identmap[0]) or >> >> >> >> >> something along those lines ought to work here. Same for >> >> >> >> >> 4 * L2_PAGETABLE_ENTRIES - you mean ARRAY_SIZE() there. >> >> >> >> > >> >> >> >> > OK. >> >> >> >> > >> >> >> >> >> Also this whole code block needs a comment, to explain what it >> >> >> >> >> does and also why l2_identmap needs skipping. >> >> >> >> >> >> >> >> >> >> Furthermore - isn't this off by one, and you process l2_identmap[0] >> >> >> >> >> this way, skipping the rest _plus_ the first following entry? I think >> >> >> >> > >> >> >> >> > The code mimics similar code in head.S. >> >> >> >> >> >> >> >> I can't see a similar off-by-1 in head.S. >> >> >> > >> >> >> > 662 /* >> >> >> > 663 * Update frame addresses in page tables excluding l2_identmap >> >> >> > 664 * without its first entry which points to l1_identmap. >> >> >> > 665 */ >> >> >> > 666 mov $((__page_tables_end-__page_tables_start)/8),%ecx >> >> >> > 667 mov $(((l2_identmap-__page_tables_start)/8)+1),%edx >> >> >> > 668 1: cmp $((l2_identmap+l2_identmap_sizeof-__page_tables_start)/8),%ecx >> >> >> > 669 cmove %edx,%ecx >> >> >> > 670 testl $_PAGE_PRESENT,sym_fs(__page_tables_start)-8(,%ecx,8) >> >> >> > 671 jz 2f >> >> >> > 672 add %esi,sym_fs(__page_tables_start)-8(,%ecx,8) >> >> >> > 673 2: loop 1b >> >> >> >> >> >> Well - this is the code in question, but you fail to point out where >> >> >> the off-by-1 is. >> >> > >> >> > Line 667, 668 and 669. >> >> >> >> I don't think so, no. Note the -8 in lines 670 and 672. >> > >> > However, you are missing +1 in line 667. >> >> I don't think I am: The first entry of l2_identmap actually needs >> processing afaics (and as the comment says), as that's the only one >> with non-absolute contents. IOW - part of my original comment was >> wrong, but the other half (you skipping one entry) still seems >> applicable, as does the part concerning the missing comment. > > It seems correct to me. l2_identmap[0] gets updated and then > we move to l3_bootmap[0]. Am I missing something? Hmm, yes, I think you're right. But the way you've coded it it's less obvious than in the assembly variant, and typically it should be the other way around. I'd really like you to make this a closer match. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/8] xen/x86: manually build xen.mb.efi binary 2018-07-11 12:26 ` Jan Beulich @ 2018-07-12 10:52 ` Daniel Kiper [not found] ` <5B4733110200007F03CC3F14@prv1-mh.provo.novell.com> 0 siblings, 1 reply; 50+ messages in thread From: Daniel Kiper @ 2018-07-12 10:52 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel On Wed, Jul 11, 2018 at 06:26:23AM -0600, Jan Beulich wrote: > >>> On 11.07.18 at 13:41, <daniel.kiper@oracle.com> wrote: > > On Tue, Jul 10, 2018 at 07:54:51AM -0600, Jan Beulich wrote: > >> >>> On 10.07.18 at 12:48, <daniel.kiper@oracle.com> wrote: > >> > On Fri, Jul 06, 2018 at 09:08:29AM -0600, Jan Beulich wrote: > >> >> >>> On 06.07.18 at 16:02, <daniel.kiper@oracle.com> wrote: > >> >> > On Thu, Jul 05, 2018 at 02:18:03AM -0600, Jan Beulich wrote: > >> >> >> >>> On 04.07.18 at 18:35, <daniel.kiper@oracle.com> wrote: > >> >> >> > On Wed, Jul 04, 2018 at 09:27:43AM -0600, Jan Beulich wrote: > >> >> >> >> >>> On 04.07.18 at 16:01, <daniel.kiper@oracle.com> wrote: > >> >> >> >> > On Mon, Jun 25, 2018 at 09:36:07AM -0600, Jan Beulich wrote: > >> >> >> >> >> >>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: > >> >> >> >> >> > @@ -582,6 +587,12 @@ static void __init efi_arch_memory_setup(void) > >> >> >> >> >> > if ( !efi_enabled(EFI_LOADER) ) > >> >> >> >> >> > return; > >> >> >> >> >> > > >> >> >> >> >> > + if ( efi_enabled(EFI_MB_LOADER) ) > >> >> >> >> >> > + for ( pte = __page_tables_start; pte < __page_tables_end; > >> >> >> >> >> > + pte += ( pte != (intpte_t *)l2_identmap ) ? 1 : 4 * L2_PAGETABLE_ENTRIES ) > >> >> >> >> >> > >> >> >> >> >> Please avoid explicit casts - &l2e_get_intpte(l2_identmap[0]) or > >> >> >> >> >> something along those lines ought to work here. Same for > >> >> >> >> >> 4 * L2_PAGETABLE_ENTRIES - you mean ARRAY_SIZE() there. > >> >> >> >> > > >> >> >> >> > OK. > >> >> >> >> > > >> >> >> >> >> Also this whole code block needs a comment, to explain what it > >> >> >> >> >> does and also why l2_identmap needs skipping. > >> >> >> >> >> > >> >> >> >> >> Furthermore - isn't this off by one, and you process l2_identmap[0] > >> >> >> >> >> this way, skipping the rest _plus_ the first following entry? I think > >> >> >> >> > > >> >> >> >> > The code mimics similar code in head.S. > >> >> >> >> > >> >> >> >> I can't see a similar off-by-1 in head.S. > >> >> >> > > >> >> >> > 662 /* > >> >> >> > 663 * Update frame addresses in page tables excluding l2_identmap > >> >> >> > 664 * without its first entry which points to l1_identmap. > >> >> >> > 665 */ > >> >> >> > 666 mov $((__page_tables_end-__page_tables_start)/8),%ecx > >> >> >> > 667 mov $(((l2_identmap-__page_tables_start)/8)+1),%edx > >> >> >> > 668 1: cmp $((l2_identmap+l2_identmap_sizeof-__page_tables_start)/8),%ecx > >> >> >> > 669 cmove %edx,%ecx > >> >> >> > 670 testl $_PAGE_PRESENT,sym_fs(__page_tables_start)-8(,%ecx,8) > >> >> >> > 671 jz 2f > >> >> >> > 672 add %esi,sym_fs(__page_tables_start)-8(,%ecx,8) > >> >> >> > 673 2: loop 1b > >> >> >> > >> >> >> Well - this is the code in question, but you fail to point out where > >> >> >> the off-by-1 is. > >> >> > > >> >> > Line 667, 668 and 669. > >> >> > >> >> I don't think so, no. Note the -8 in lines 670 and 672. > >> > > >> > However, you are missing +1 in line 667. > >> > >> I don't think I am: The first entry of l2_identmap actually needs > >> processing afaics (and as the comment says), as that's the only one > >> with non-absolute contents. IOW - part of my original comment was > >> wrong, but the other half (you skipping one entry) still seems > >> applicable, as does the part concerning the missing comment. > > > > It seems correct to me. l2_identmap[0] gets updated and then > > we move to l3_bootmap[0]. Am I missing something? > > Hmm, yes, I think you're right. But the way you've coded it it's > less obvious than in the assembly variant, and typically it should > be the other way around. I'd really like you to make this a closer > match. Do you suggest that we should do that backwards as we do in ASM? Or anything else? Daniel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
[parent not found: <5B4733110200007F03CC3F14@prv1-mh.provo.novell.com>]
* Re: [PATCH v2 3/8] xen/x86: manually build xen.mb.efi binary [not found] ` <5B4733110200007F03CC3F14@prv1-mh.provo.novell.com> @ 2018-07-12 11:50 ` Jan Beulich 0 siblings, 0 replies; 50+ messages in thread From: Jan Beulich @ 2018-07-12 11:50 UTC (permalink / raw) To: Daniel Kiper Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel >>> On 12.07.18 at 12:52, <daniel.kiper@oracle.com> wrote: > On Wed, Jul 11, 2018 at 06:26:23AM -0600, Jan Beulich wrote: >> >>> On 11.07.18 at 13:41, <daniel.kiper@oracle.com> wrote: >> > On Tue, Jul 10, 2018 at 07:54:51AM -0600, Jan Beulich wrote: >> >> >>> On 10.07.18 at 12:48, <daniel.kiper@oracle.com> wrote: >> >> > On Fri, Jul 06, 2018 at 09:08:29AM -0600, Jan Beulich wrote: >> >> >> >>> On 06.07.18 at 16:02, <daniel.kiper@oracle.com> wrote: >> >> >> > On Thu, Jul 05, 2018 at 02:18:03AM -0600, Jan Beulich wrote: >> >> >> >> >>> On 04.07.18 at 18:35, <daniel.kiper@oracle.com> wrote: >> >> >> >> > On Wed, Jul 04, 2018 at 09:27:43AM -0600, Jan Beulich wrote: >> >> >> >> >> >>> On 04.07.18 at 16:01, <daniel.kiper@oracle.com> wrote: >> >> >> >> >> > On Mon, Jun 25, 2018 at 09:36:07AM -0600, Jan Beulich wrote: >> >> >> >> >> >> >>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: >> >> >> >> >> >> > @@ -582,6 +587,12 @@ static void __init efi_arch_memory_setup(void) >> >> >> >> >> >> > if ( !efi_enabled(EFI_LOADER) ) >> >> >> >> >> >> > return; >> >> >> >> >> >> > >> >> >> >> >> >> > + if ( efi_enabled(EFI_MB_LOADER) ) >> >> >> >> >> >> > + for ( pte = __page_tables_start; pte < __page_tables_end; >> >> >> >> >> >> > + pte += ( pte != (intpte_t *)l2_identmap ) ? 1 : 4 * > L2_PAGETABLE_ENTRIES ) >> >> >> >> >> >> >> >> >> >> >> >> Please avoid explicit casts - &l2e_get_intpte(l2_identmap[0]) or >> >> >> >> >> >> something along those lines ought to work here. Same for >> >> >> >> >> >> 4 * L2_PAGETABLE_ENTRIES - you mean ARRAY_SIZE() there. >> >> >> >> >> > >> >> >> >> >> > OK. >> >> >> >> >> > >> >> >> >> >> >> Also this whole code block needs a comment, to explain what it >> >> >> >> >> >> does and also why l2_identmap needs skipping. >> >> >> >> >> >> >> >> >> >> >> >> Furthermore - isn't this off by one, and you process l2_identmap[0] >> >> >> >> >> >> this way, skipping the rest _plus_ the first following entry? I think >> >> >> >> >> > >> >> >> >> >> > The code mimics similar code in head.S. >> >> >> >> >> >> >> >> >> >> I can't see a similar off-by-1 in head.S. >> >> >> >> > >> >> >> >> > 662 /* >> >> >> >> > 663 * Update frame addresses in page tables excluding > l2_identmap >> >> >> >> > 664 * without its first entry which points to l1_identmap. >> >> >> >> > 665 */ >> >> >> >> > 666 mov $((__page_tables_end-__page_tables_start)/8),%ecx >> >> >> >> > 667 mov $(((l2_identmap-__page_tables_start)/8)+1),%edx >> >> >> >> > 668 1: cmp > $((l2_identmap+l2_identmap_sizeof-__page_tables_start)/8),%ecx >> >> >> >> > 669 cmove %edx,%ecx >> >> >> >> > 670 testl > $_PAGE_PRESENT,sym_fs(__page_tables_start)-8(,%ecx,8) >> >> >> >> > 671 jz 2f >> >> >> >> > 672 add %esi,sym_fs(__page_tables_start)-8(,%ecx,8) >> >> >> >> > 673 2: loop 1b >> >> >> >> >> >> >> >> Well - this is the code in question, but you fail to point out where >> >> >> >> the off-by-1 is. >> >> >> > >> >> >> > Line 667, 668 and 669. >> >> >> >> >> >> I don't think so, no. Note the -8 in lines 670 and 672. >> >> > >> >> > However, you are missing +1 in line 667. >> >> >> >> I don't think I am: The first entry of l2_identmap actually needs >> >> processing afaics (and as the comment says), as that's the only one >> >> with non-absolute contents. IOW - part of my original comment was >> >> wrong, but the other half (you skipping one entry) still seems >> >> applicable, as does the part concerning the missing comment. >> > >> > It seems correct to me. l2_identmap[0] gets updated and then >> > we move to l3_bootmap[0]. Am I missing something? >> >> Hmm, yes, I think you're right. But the way you've coded it it's >> less obvious than in the assembly variant, and typically it should >> be the other way around. I'd really like you to make this a closer >> match. > > Do you suggest that we should do that backwards as we do in ASM? > Or anything else? The closer the match, the better imo. I wouldn't insist on it being done backwards though, unless there's a particular reason for that. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v2 4/8] xen/x86: add some addresses to the Multiboot header 2018-06-19 14:35 [PATCH v2 0/8] Change xen.efi build and add SHIM_LOCK verification into efi_multiboot2() Daniel Kiper ` (2 preceding siblings ...) 2018-06-19 14:35 ` [PATCH v2 3/8] xen/x86: manually build xen.mb.efi binary Daniel Kiper @ 2018-06-19 14:35 ` Daniel Kiper 2018-06-28 13:41 ` Jan Beulich 2018-06-19 14:35 ` [PATCH v2 5/8] xen/x86: add some addresses to the Multiboot2 header Daniel Kiper ` (3 subsequent siblings) 7 siblings, 1 reply; 50+ messages in thread From: Daniel Kiper @ 2018-06-19 14:35 UTC (permalink / raw) To: xen-devel Cc: sstabellini, wei.liu2, george.dunlap, andrew.cooper3, ian.jackson, tim, julien.grall, jbeulich, tamas.k.lengyel In comparison to ELF the PE format is not supported by the Multiboot protocol. So, if we wish to load xen.mb.efi using this protocol we have to put header_addr, load_addr, load_end_addr, bss_end_addr and entry_addr data into Multiboot header. The Multiboot protocol spec can be found at https://www.gnu.org/software/grub/manual/multiboot/ Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> --- v2 - suggestions/fixes: - add the pointer to the Multiboot protocol spec to the commit message (suggested by Konrad Rzeszutek Wilk). --- xen/arch/x86/boot/head.S | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index 47f254d..3ed43d9 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -201,13 +201,24 @@ ENTRY(start) .balign 4 multiboot1_header: /*** MULTIBOOT1 HEADER ****/ #define MULTIBOOT_HEADER_FLAGS (MULTIBOOT_HEADER_MODS_ALIGNED | \ - MULTIBOOT_HEADER_WANT_MEMORY) + MULTIBOOT_HEADER_WANT_MEMORY | \ + MULTIBOOT_HEADER_HAS_ADDR) /* Magic number indicating a Multiboot header. */ .long MULTIBOOT_HEADER_MAGIC /* Flags to bootloader (see Multiboot spec). */ .long MULTIBOOT_HEADER_FLAGS /* Checksum: must be the negated sum of the first two fields. */ .long -(MULTIBOOT_HEADER_MAGIC + MULTIBOOT_HEADER_FLAGS) + /* header_addr */ + .long sym_offs(multiboot1_header) + /* load_addr */ + .long sym_offs(start) + /* load_end_addr */ + .long sym_offs(__bss_start) + /* bss_end_addr */ + .long sym_offs(__2M_rwdata_end) + /* entry_addr */ + .long sym_offs(__start) .size multiboot1_header, . - multiboot1_header .type multiboot1_header, @object -- 1.7.10.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [PATCH v2 4/8] xen/x86: add some addresses to the Multiboot header 2018-06-19 14:35 ` [PATCH v2 4/8] xen/x86: add some addresses to the Multiboot header Daniel Kiper @ 2018-06-28 13:41 ` Jan Beulich 0 siblings, 0 replies; 50+ messages in thread From: Jan Beulich @ 2018-06-28 13:41 UTC (permalink / raw) To: Daniel Kiper Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel >>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: > In comparison to ELF the PE format is not supported by the Multiboot > protocol. So, if we wish to load xen.mb.efi using this protocol we > have to put header_addr, load_addr, load_end_addr, bss_end_addr and > entry_addr data into Multiboot header. > > The Multiboot protocol spec can be found at > https://www.gnu.org/software/grub/manual/multiboot/ > > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> Acked-by: Jan Beulich <jbeulich@suse.com> (but imo not to go in without further patches in this series actually needing this one) Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v2 5/8] xen/x86: add some addresses to the Multiboot2 header 2018-06-19 14:35 [PATCH v2 0/8] Change xen.efi build and add SHIM_LOCK verification into efi_multiboot2() Daniel Kiper ` (3 preceding siblings ...) 2018-06-19 14:35 ` [PATCH v2 4/8] xen/x86: add some addresses to the Multiboot header Daniel Kiper @ 2018-06-19 14:35 ` Daniel Kiper 2018-06-28 13:42 ` Jan Beulich 2018-06-19 14:35 ` [PATCH v2 6/8] efi: split out efi_shim_lock() Daniel Kiper ` (2 subsequent siblings) 7 siblings, 1 reply; 50+ messages in thread From: Daniel Kiper @ 2018-06-19 14:35 UTC (permalink / raw) To: xen-devel Cc: sstabellini, wei.liu2, george.dunlap, andrew.cooper3, ian.jackson, tim, julien.grall, jbeulich, tamas.k.lengyel In comparison to ELF the PE format is not supported by the Multiboot2 protocol. So, if we wish to load xen.mb.efi using this protocol we have to add MULTIBOOT2_HEADER_TAG_ADDRESS and MULTIBOOT2_HEADER_TAG_ENTRY_ADDRESS tags into Multiboot2 header. Additionally, put MULTIBOOT2_HEADER_TAG_ENTRY_ADDRESS and MULTIBOOT2_HEADER_TAG_ENTRY_ADDRESS_EFI64 tags close to each other to make the header more readable. The Multiboot2 protocol spec can be found at https://www.gnu.org/software/grub/manual/multiboot2/ Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> --- v2 - suggestions/fixes: - add the pointer to the Multiboot2 protocol spec to the commit message (suggested by Konrad Rzeszutek Wilk), - improve commit message (suggested by Konrad Rzeszutek Wilk). --- xen/arch/x86/boot/head.S | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index 3ed43d9..582dc51 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -245,6 +245,13 @@ multiboot2_header: /* Align modules at page boundry. */ mb2ht_init MB2_HT(MODULE_ALIGN), MB2_HT(REQUIRED) + /* The address tag. */ + mb2ht_init MB2_HT(ADDRESS), MB2_HT(REQUIRED), \ + sym_offs(multiboot2_header), /* header_addr */ \ + sym_offs(start), /* load_addr */ \ + sym_offs(__bss_start), /* load_end_addr */ \ + sym_offs(__2M_rwdata_end) /* bss_end_addr */ + /* Load address preference. */ mb2ht_init MB2_HT(RELOCATABLE), MB2_HT(OPTIONAL), \ sym_offs(start), /* Min load address. */ \ @@ -252,6 +259,14 @@ multiboot2_header: 0x200000, /* Load address alignment (2 MiB). */ \ MULTIBOOT2_LOAD_PREFERENCE_HIGH + /* Multiboot2 entry point. */ + mb2ht_init MB2_HT(ENTRY_ADDRESS), MB2_HT(REQUIRED), \ + sym_offs(__start) + + /* EFI64 Multiboot2 entry point. */ + mb2ht_init MB2_HT(ENTRY_ADDRESS_EFI64), MB2_HT(OPTIONAL), \ + sym_offs(__efi64_mb2_start) + /* Console flags tag. */ mb2ht_init MB2_HT(CONSOLE_FLAGS), MB2_HT(OPTIONAL), \ MULTIBOOT2_CONSOLE_FLAGS_EGA_TEXT_SUPPORTED @@ -265,10 +280,6 @@ multiboot2_header: /* Request that ExitBootServices() not be called. */ mb2ht_init MB2_HT(EFI_BS), MB2_HT(OPTIONAL) - /* EFI64 Multiboot2 entry point. */ - mb2ht_init MB2_HT(ENTRY_ADDRESS_EFI64), MB2_HT(OPTIONAL), \ - sym_offs(__efi64_mb2_start) - /* Multiboot2 header end tag. */ mb2ht_init MB2_HT(END), MB2_HT(REQUIRED) .Lmultiboot2_header_end: -- 1.7.10.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [PATCH v2 5/8] xen/x86: add some addresses to the Multiboot2 header 2018-06-19 14:35 ` [PATCH v2 5/8] xen/x86: add some addresses to the Multiboot2 header Daniel Kiper @ 2018-06-28 13:42 ` Jan Beulich 0 siblings, 0 replies; 50+ messages in thread From: Jan Beulich @ 2018-06-28 13:42 UTC (permalink / raw) To: Daniel Kiper Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel >>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: > In comparison to ELF the PE format is not supported by the Multiboot2 > protocol. So, if we wish to load xen.mb.efi using this protocol we have > to add MULTIBOOT2_HEADER_TAG_ADDRESS and MULTIBOOT2_HEADER_TAG_ENTRY_ADDRESS > tags into Multiboot2 header. > > Additionally, put MULTIBOOT2_HEADER_TAG_ENTRY_ADDRESS and > MULTIBOOT2_HEADER_TAG_ENTRY_ADDRESS_EFI64 tags close to each > other to make the header more readable. > > The Multiboot2 protocol spec can be found at > https://www.gnu.org/software/grub/manual/multiboot2/ > > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> Acked-by: Jan Beulich <jbeulich@suse.com> (but imo not to go in without further patches in this series actually needing this one) Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v2 6/8] efi: split out efi_shim_lock() 2018-06-19 14:35 [PATCH v2 0/8] Change xen.efi build and add SHIM_LOCK verification into efi_multiboot2() Daniel Kiper ` (4 preceding siblings ...) 2018-06-19 14:35 ` [PATCH v2 5/8] xen/x86: add some addresses to the Multiboot2 header Daniel Kiper @ 2018-06-19 14:35 ` Daniel Kiper 2018-06-28 13:43 ` Jan Beulich 2018-06-19 14:35 ` [PATCH v2 7/8] xen/x86/efi: Verify dom0 kernel with SHIM_LOCK protocol in efi_multiboot2() Daniel Kiper 2018-06-19 14:35 ` [PATCH v2 8/8] efi: drop original xen.efi code and build mechanism Daniel Kiper 7 siblings, 1 reply; 50+ messages in thread From: Daniel Kiper @ 2018-06-19 14:35 UTC (permalink / raw) To: xen-devel Cc: sstabellini, wei.liu2, george.dunlap, andrew.cooper3, ian.jackson, tim, julien.grall, jbeulich, tamas.k.lengyel ..which verifies PE signatures with SHIM_LOCK protocol. We want to re-use this code in subsequent patch in efi_multiboot2(). Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> --- xen/common/efi/boot.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 64d1268..06bfadc 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -124,6 +124,7 @@ 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_shim_lock(VOID *Buffer, UINT32 Size); static void efi_tables(void); static void setup_efi_pci(void); static void efi_variables(void); @@ -797,6 +798,17 @@ static UINTN __init efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, return gop_mode; } +static void __init efi_shim_lock(VOID *Buffer, UINT32 Size) +{ + static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID; + EFI_SHIM_LOCK_PROTOCOL *shim_lock; + EFI_STATUS status; + + if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL, (void **)&shim_lock)) && + (status = shim_lock->Verify(Buffer, Size)) != EFI_SUCCESS ) + PrintErrMesg(L"Dom0 kernel image could not be verified", status); +} + static void __init efi_tables(void) { unsigned int i; @@ -1062,13 +1074,11 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) { static EFI_GUID __initdata loaded_image_guid = LOADED_IMAGE_PROTOCOL; - static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID; EFI_LOADED_IMAGE *loaded_image; EFI_STATUS status; unsigned int i, argc; CHAR16 **argv, *file_name, *cfg_file_name = NULL, *options = NULL; UINTN gop_mode = ~0; - EFI_SHIM_LOCK_PROTOCOL *shim_lock; EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL; union string section = { NULL }, name; bool base_video = false; @@ -1225,10 +1235,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) read_file(dir_handle, s2w(&name), &kernel, option_str); efi_bs->FreePool(name.w); - if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL, - (void **)&shim_lock)) && - (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS ) - PrintErrMesg(L"Dom0 kernel image could not be verified", status); + efi_shim_lock(kernel.ptr, kernel.size); name.s = get_value(&cfg, section.s, "ramdisk"); if ( name.s ) -- 1.7.10.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [PATCH v2 6/8] efi: split out efi_shim_lock() 2018-06-19 14:35 ` [PATCH v2 6/8] efi: split out efi_shim_lock() Daniel Kiper @ 2018-06-28 13:43 ` Jan Beulich 0 siblings, 0 replies; 50+ messages in thread From: Jan Beulich @ 2018-06-28 13:43 UTC (permalink / raw) To: Daniel Kiper Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel >>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: > ..which verifies PE signatures with SHIM_LOCK protocol. We want > to re-use this code in subsequent patch in efi_multiboot2(). > > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> I think this would better be merged with the patch actually needing it, but anyway Acked-by: Jan Beulich <jbeulich@suse.com> Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v2 7/8] xen/x86/efi: Verify dom0 kernel with SHIM_LOCK protocol in efi_multiboot2() 2018-06-19 14:35 [PATCH v2 0/8] Change xen.efi build and add SHIM_LOCK verification into efi_multiboot2() Daniel Kiper ` (5 preceding siblings ...) 2018-06-19 14:35 ` [PATCH v2 6/8] efi: split out efi_shim_lock() Daniel Kiper @ 2018-06-19 14:35 ` Daniel Kiper 2018-06-28 13:48 ` Jan Beulich 2018-06-19 14:35 ` [PATCH v2 8/8] efi: drop original xen.efi code and build mechanism Daniel Kiper 7 siblings, 1 reply; 50+ messages in thread From: Daniel Kiper @ 2018-06-19 14:35 UTC (permalink / raw) To: xen-devel Cc: sstabellini, wei.liu2, george.dunlap, andrew.cooper3, ian.jackson, tim, julien.grall, jbeulich, tamas.k.lengyel Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> --- v2 - suggestions/fixes: - add const to *dom0_kernel efi_multiboot2() argument, (suggested by Jan Beulich), - improve comments (suggested by Konrad Rzeszutek Wilk). --- xen/arch/x86/boot/head.S | 23 +++++++++++++++++++++-- xen/arch/x86/efi/efi-boot.h | 10 +++++++++- xen/arch/x86/efi/stub.c | 6 +++++- 3 files changed, 35 insertions(+), 4 deletions(-) diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index 582dc51..48f1b00 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -395,9 +395,13 @@ __efi64_mb2_start: jmp x86_32_switch .Lefi_multiboot2_proto: - /* Zero EFI SystemTable and EFI ImageHandle addresses. */ + /* + * Zero EFI SystemTable, EFI ImageHandle and + * dom0 kernel module struct addresses. + */ xor %esi,%esi xor %edi,%edi + xor %r14d,%r14d /* Skip Multiboot2 information fixed part. */ lea (MB2_fixed_sizeof+MULTIBOOT2_TAG_ALIGN-1)(%rbx),%ecx @@ -435,6 +439,18 @@ __efi64_mb2_start: cmove MB2_efi64_ih(%rcx),%rdi je .Lefi_mb2_next_tag + /* + * Get dom0 kernel module struct address from Multiboot2 + * information and ignore the rest of modules. + */ + cmpl $MULTIBOOT2_TAG_TYPE_MODULE,MB2_tag_type(%rcx) + jne .Lefi_mb2_end + + test %r14d,%r14d + cmovz %ecx,%r14d + jmp .Lefi_mb2_next_tag + +.Lefi_mb2_end: /* Is it the end of Multiboot2 information? */ cmpl $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx) je .Lrun_bs @@ -496,9 +512,12 @@ __efi64_mb2_start: /* Keep the stack aligned. Do not pop a single item off it. */ mov (%rsp),%rdi + mov %r14d,%edx + /* * efi_multiboot2() is called according to System V AMD64 ABI: - * - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable. + * - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable, + * %rdx - if passed, dom0 kernel module struct address. */ call efi_multiboot2 diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index 5f0e821..f8aaa37 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -3,6 +3,8 @@ * is intended to be included by common/efi/boot.c _only_, and * therefore can define arch specific global variables. */ +#include <xen/types.h> +#include <xen/multiboot2.h> #include <xen/vga.h> #include <asm/e820.h> #include <asm/edd.h> @@ -694,7 +696,9 @@ efi_mb_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) efi_start(ImageHandle, SystemTable); } -void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) +void __init efi_multiboot2(EFI_HANDLE ImageHandle, + EFI_SYSTEM_TABLE *SystemTable, + const multiboot2_tag_module_t *dom0_kernel) { EFI_GRAPHICS_OUTPUT_PROTOCOL *gop; UINTN cols, gop_mode = ~0, rows; @@ -710,6 +714,10 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable &cols, &rows) == EFI_SUCCESS ) efi_arch_console_init(cols, rows); + if ( dom0_kernel && dom0_kernel->mod_end > dom0_kernel->mod_start ) + efi_shim_lock((VOID *)(unsigned long)dom0_kernel->mod_start, + dom0_kernel->mod_end - dom0_kernel->mod_start); + gop = efi_get_gop(); if ( gop ) diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.c index 2142932..5918536 100644 --- a/xen/arch/x86/efi/stub.c +++ b/xen/arch/x86/efi/stub.c @@ -1,7 +1,9 @@ +#include <xen/types.h> #include <xen/efi.h> #include <xen/errno.h> #include <xen/init.h> #include <xen/lib.h> +#include <xen/multiboot2.h> #include <asm/page.h> #include <asm/efibind.h> #include <efi/efidef.h> @@ -24,11 +26,13 @@ asm ( "efi_mb_start: \n" " mov %rcx,%rdi \n" " mov %rdx,%rsi \n" + " xor %rdx,%rdx \n" " call efi_multiboot2 \n" ); void __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle, - EFI_SYSTEM_TABLE *SystemTable) + EFI_SYSTEM_TABLE *SystemTable, + const multiboot2_tag_module_t *dom0_kernel) { static const CHAR16 __initconst err[] = L"Xen does not have EFI code build in!\r\nSystem halted!\r\n"; -- 1.7.10.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [PATCH v2 7/8] xen/x86/efi: Verify dom0 kernel with SHIM_LOCK protocol in efi_multiboot2() 2018-06-19 14:35 ` [PATCH v2 7/8] xen/x86/efi: Verify dom0 kernel with SHIM_LOCK protocol in efi_multiboot2() Daniel Kiper @ 2018-06-28 13:48 ` Jan Beulich 0 siblings, 0 replies; 50+ messages in thread From: Jan Beulich @ 2018-06-28 13:48 UTC (permalink / raw) To: Daniel Kiper Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel >>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: > @@ -435,6 +439,18 @@ __efi64_mb2_start: > cmove MB2_efi64_ih(%rcx),%rdi > je .Lefi_mb2_next_tag > > + /* > + * Get dom0 kernel module struct address from Multiboot2 > + * information and ignore the rest of modules. > + */ > + cmpl $MULTIBOOT2_TAG_TYPE_MODULE,MB2_tag_type(%rcx) > + jne .Lefi_mb2_end > + > + test %r14d,%r14d > + cmovz %ecx,%r14d > + jmp .Lefi_mb2_next_tag > + > +.Lefi_mb2_end: Especially here, but perhaps also applicable to other hunks: Despite surrounding code being like this, can we please stop the bad habit of not having blanks after the commas in insn operand lists? Also personally I'd prefer if you uppercased the d in Dom0 everywhere - we commonly use something like d%d or otherwise Dom%d (or DomU). With these purely mechanical things taken care of Acked-by: Jan Beulich <jbeulich@suse.com> Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v2 8/8] efi: drop original xen.efi code and build mechanism 2018-06-19 14:35 [PATCH v2 0/8] Change xen.efi build and add SHIM_LOCK verification into efi_multiboot2() Daniel Kiper ` (6 preceding siblings ...) 2018-06-19 14:35 ` [PATCH v2 7/8] xen/x86/efi: Verify dom0 kernel with SHIM_LOCK protocol in efi_multiboot2() Daniel Kiper @ 2018-06-19 14:35 ` Daniel Kiper 2018-06-28 13:51 ` Jan Beulich 7 siblings, 1 reply; 50+ messages in thread From: Daniel Kiper @ 2018-06-19 14:35 UTC (permalink / raw) To: xen-devel Cc: sstabellini, wei.liu2, george.dunlap, andrew.cooper3, ian.jackson, tim, julien.grall, jbeulich, tamas.k.lengyel Then rename xen.mb.efi to xen.efi and drop all related differentiators in the code. Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> --- xen/Makefile | 16 +- xen/arch/arm/efi/efi-boot.h | 4 - xen/arch/x86/Makefile | 87 +-------- xen/arch/x86/boot/head.S | 2 +- xen/arch/x86/efi/Makefile | 17 +- xen/arch/x86/efi/buildid.ihex | 3 - xen/arch/x86/efi/check.c | 4 - xen/arch/x86/efi/efi-boot.h | 88 +-------- xen/arch/x86/efi/mkreloc.c | 384 --------------------------------------- xen/arch/x86/efi/relocs-dummy.S | 11 -- xen/arch/x86/efi/stub.c | 97 ---------- xen/arch/x86/xen.lds.S | 81 +-------- xen/common/efi/boot.c | 2 - xen/common/version.c | 51 ------ xen/include/xen/efi.h | 1 - 15 files changed, 21 insertions(+), 827 deletions(-) delete mode 100644 xen/arch/x86/efi/buildid.ihex delete mode 100644 xen/arch/x86/efi/check.c delete mode 100644 xen/arch/x86/efi/mkreloc.c delete mode 100644 xen/arch/x86/efi/relocs-dummy.S delete mode 100644 xen/arch/x86/efi/stub.c diff --git a/xen/Makefile b/xen/Makefile index a49b9b7..0c03914 100644 --- a/xen/Makefile +++ b/xen/Makefile @@ -61,20 +61,17 @@ _install: $(TARGET)$(CONFIG_XEN_INSTALL_SUFFIX) ln -f -s $(T)-$(XEN_FULLVERSION)$(Z) $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION).$(XEN_SUBVERSION)$(Z) ln -f -s $(T)-$(XEN_FULLVERSION)$(Z) $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION)$(Z) ln -f -s $(T)-$(XEN_FULLVERSION)$(Z) $(D)$(BOOT_DIR)/$(T)$(Z) - $(INSTALL_DATA) $(TARGET).mb.efi $(D)$(BOOT_DIR)/$(T)-$(XEN_FULLVERSION).mb.efi - ln -f -s $(T)-$(XEN_FULLVERSION).mb.efi $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION).$(XEN_SUBVERSION).mb.efi - ln -f -s $(T)-$(XEN_FULLVERSION).mb.efi $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION).mb.efi - ln -f -s $(T)-$(XEN_FULLVERSION).mb.efi $(D)$(BOOT_DIR)/$(T).mb.efi + $(INSTALL_DATA) $(TARGET).efi $(D)$(BOOT_DIR)/$(T)-$(XEN_FULLVERSION).efi + ln -f -s $(T)-$(XEN_FULLVERSION).efi $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION).$(XEN_SUBVERSION).efi + ln -f -s $(T)-$(XEN_FULLVERSION).efi $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION).efi + ln -f -s $(T)-$(XEN_FULLVERSION).efi $(D)$(BOOT_DIR)/$(T).efi [ -d "$(D)$(DEBUG_DIR)" ] || $(INSTALL_DIR) $(D)$(DEBUG_DIR) $(INSTALL_DATA) $(TARGET)-syms $(D)$(DEBUG_DIR)/$(T)-syms-$(XEN_FULLVERSION) $(INSTALL_DATA) $(TARGET)-syms.map $(D)$(DEBUG_DIR)/$(T)-syms-$(XEN_FULLVERSION).map $(INSTALL_DATA) $(KCONFIG_CONFIG) $(D)$(BOOT_DIR)/$(T)-$(XEN_FULLVERSION).config - if [ -r $(TARGET).efi -a -n '$(EFI_DIR)' ]; then \ + if [ -n '$(EFI_DIR)' ]; then \ [ -d $(D)$(EFI_DIR) ] || $(INSTALL_DIR) $(D)$(EFI_DIR); \ $(INSTALL_DATA) $(TARGET).efi $(D)$(EFI_DIR)/$(T)-$(XEN_FULLVERSION).efi; \ - if [ -e $(TARGET).efi.map ]; then \ - $(INSTALL_DATA) $(TARGET).efi.map $(D)$(DEBUG_DIR)/$(T)-$(XEN_FULLVERSION).efi.map; \ - fi; \ ln -sf $(T)-$(XEN_FULLVERSION).efi $(D)$(EFI_DIR)/$(T)-$(XEN_VERSION).$(XEN_SUBVERSION).efi; \ ln -sf $(T)-$(XEN_FULLVERSION).efi $(D)$(EFI_DIR)/$(T)-$(XEN_VERSION).efi; \ ln -sf $(T)-$(XEN_FULLVERSION).efi $(D)$(EFI_DIR)/$(T).efi; \ @@ -103,7 +100,6 @@ _uninstall: rm -f $(D)$(DEBUG_DIR)/$(T)-syms-$(XEN_FULLVERSION).map rm -f $(D)$(EFI_DIR)/$(T)-$(XEN_FULLVERSION).efi rm -f $(D)$(EFI_DIR)/$(T)-$(XEN_VERSION).$(XEN_SUBVERSION).efi - rm -f $(D)$(DEBUG_DIR)/$(T)-$(XEN_FULLVERSION).efi.map rm -f $(D)$(EFI_DIR)/$(T)-$(XEN_VERSION).efi rm -f $(D)$(EFI_DIR)/$(T).efi rm -f $(D)$(EFI_MOUNTPOINT)/efi/$(EFI_VENDOR)/$(T)-$(XEN_FULLVERSION).efi @@ -125,7 +121,7 @@ _clean: delete-unfresh-files $(MAKE) -f $(BASEDIR)/Rules.mk -C test clean $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) SRCARCH=$(SRCARCH) clean find . \( -name "*.o" -o -name ".*.d" -o -name "*.gcno" \) -exec rm -f {} \; - rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi $(TARGET).mb.efi $(TARGET).efi.map $(TARGET)-syms $(TARGET)-syms.map *~ core + rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi $(TARGET)-syms $(TARGET)-syms.map *~ core rm -f include/asm-*/asm-offsets.h rm -f .banner diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h index ca655ff..0495150 100644 --- a/xen/arch/arm/efi/efi-boot.h +++ b/xen/arch/arm/efi/efi-boot.h @@ -341,10 +341,6 @@ static void __init *fdt_increase_size(struct file *fdtfile, int add_size) return new_fdt; } -static void __init efi_arch_relocate_image(unsigned long delta) -{ -} - static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable, void *map, UINTN map_size, diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index ef3fb51..92c38fe 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -74,10 +74,6 @@ obj-y += xstate.o x86_emulate.o: x86_emulate/x86_emulate.c x86_emulate/x86_emulate.h -efi-y := $(shell if [ ! -r $(BASEDIR)/include/xen/compile.h -o \ - -O $(BASEDIR)/include/xen/compile.h ]; then \ - echo '$(TARGET).efi'; fi) - ifneq ($(build_id_linker),) notes_phdrs = --notes else @@ -98,12 +94,12 @@ endif syms-warn-dup-y := --warn-dup syms-warn-dup-$(CONFIG_SUPPRESS_DUPLICATE_SYMBOL_WARNINGS) := -$(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32 +$(TARGET): $(TARGET)-syms boot/mkelf32 ./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET) $(XEN_IMG_OFFSET) \ `$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . __2M_rwdata_end$$/0x\1/p'` $(OBJCOPY) -O binary -S --change-section-address \ ".efi.pe.header-`$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . __image_base__$$/0x\1/p'`" \ - $(TARGET)-syms $(TARGET).mb.efi + $(TARGET)-syms $(TARGET).efi ALL_OBJS := $(BASEDIR)/arch/x86/boot/built_in.o $(BASEDIR)/arch/x86/efi/built_in.o $(ALL_OBJS) @@ -112,21 +108,12 @@ ifeq ($(CONFIG_LTO),y) prelink_lto.o: $(ALL_OBJS) $(LD_LTO) -r -o $@ $^ -prelink-efi_lto.o: $(ALL_OBJS) efi/runtime.o efi/compat.o - $(guard) $(LD_LTO) -r -o $@ $(filter-out %/efi/built_in.o,$^) - # Link it with all the binary objects prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o $(LD) $(LDFLAGS) -r -o $@ $^ - -prelink-efi.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink-efi_lto.o efi/boot.init.o - $(guard) $(LD) $(LDFLAGS) -r -o $@ $^ else prelink.o: $(ALL_OBJS) $(LD) $(LDFLAGS) -r -o $@ $^ - -prelink-efi.o: $(ALL_OBJS) efi/boot.init.o efi/runtime.o efi/compat.o - $(guard) $(LD) $(LDFLAGS) -r -o $@ $(filter-out %/efi/built_in.o,$^) endif $(BASEDIR)/common/symbols-dummy.o: @@ -152,66 +139,6 @@ $(TARGET)-syms: prelink.o xen.lds $(BASEDIR)/common/symbols-dummy.o >$(@D)/$(@F).map rm -f $(@D)/.$(@F).[0-9]* -note.o: $(TARGET)-syms - $(OBJCOPY) -O binary --only-section=.note.gnu.build-id $(BASEDIR)/xen-syms $@.bin - $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 \ - --rename-section=.data=.note.gnu.build-id -S $@.bin $@ - rm -f $@.bin - -EFI_LDFLAGS = $(patsubst -m%,-mi386pep,$(LDFLAGS)) --subsystem=10 -EFI_LDFLAGS += --image-base=$(1) --stack=0,0 --heap=0,0 --strip-debug -EFI_LDFLAGS += --section-alignment=0x200000 --file-alignment=0x20 -EFI_LDFLAGS += --major-image-version=$(XEN_VERSION) -EFI_LDFLAGS += --minor-image-version=$(XEN_SUBVERSION) -EFI_LDFLAGS += --major-os-version=2 --minor-os-version=0 -EFI_LDFLAGS += --major-subsystem-version=2 --minor-subsystem-version=0 - -$(TARGET).efi: VIRT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A VIRT_START$$,,p') -$(TARGET).efi: ALT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A ALT_START$$,,p') -# Don't use $(wildcard ...) here - at least make 3.80 expands this too early! -$(TARGET).efi: guard = $(if $(shell echo efi/dis* | grep disabled),:) - -ifneq ($(build_id_linker),) -ifeq ($(call ld-ver-build-id,$(LD) $(filter -m%,$(EFI_LDFLAGS))),y) -CFLAGS += -DBUILD_ID_EFI -EFI_LDFLAGS += $(build_id_linker) -note_file := efi/buildid.o -# NB: this must be the last input in the linker call, because inputs following -# the -b option will all be treated as being in the specified format. -note_file_option := -b pe-x86-64 $(note_file) -else -note_file := note.o -endif -else -note_file := -endif -note_file_option ?= $(note_file) - -$(TARGET).efi: prelink-efi.o $(note_file) efi.lds efi/relocs-dummy.o $(BASEDIR)/common/symbols-dummy.o efi/mkreloc - $(foreach base, $(VIRT_BASE) $(ALT_BASE), \ - $(guard) $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds -N $< efi/relocs-dummy.o \ - $(BASEDIR)/common/symbols-dummy.o $(note_file_option) -o $(@D)/.$(@F).$(base).0 &&) : - $(guard) efi/mkreloc $(foreach base,$(VIRT_BASE) $(ALT_BASE),$(@D)/.$(@F).$(base).0) >$(@D)/.$(@F).0r.S - $(guard) $(NM) -pa --format=sysv $(@D)/.$(@F).$(VIRT_BASE).0 \ - | $(guard) $(BASEDIR)/tools/symbols $(all_symbols) --sysv --sort >$(@D)/.$(@F).0s.S - $(guard) $(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).0r.o $(@D)/.$(@F).0s.o - $(foreach base, $(VIRT_BASE) $(ALT_BASE), \ - $(guard) $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds -N $< \ - $(@D)/.$(@F).0r.o $(@D)/.$(@F).0s.o $(note_file_option) -o $(@D)/.$(@F).$(base).1 &&) : - $(guard) efi/mkreloc $(foreach base,$(VIRT_BASE) $(ALT_BASE),$(@D)/.$(@F).$(base).1) >$(@D)/.$(@F).1r.S - $(guard) $(NM) -pa --format=sysv $(@D)/.$(@F).$(VIRT_BASE).1 \ - | $(guard) $(BASEDIR)/tools/symbols $(all_symbols) --sysv --sort >$(@D)/.$(@F).1s.S - $(guard) $(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1r.o $(@D)/.$(@F).1s.o - $(guard) $(LD) $(call EFI_LDFLAGS,$(VIRT_BASE)) -T efi.lds -N $< \ - $(@D)/.$(@F).1r.o $(@D)/.$(@F).1s.o $(note_file_option) -o $@ - if $(guard) false; then rm -f $@; echo 'EFI support disabled'; \ - else $(NM) -pa --format=sysv $(@D)/$(@F) \ - | $(BASEDIR)/tools/symbols --xensyms --sysv --sort >$(@D)/$(@F).map; fi - rm -f $(@D)/.$(@F).[0-9]* - -efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o: $(BASEDIR)/arch/x86/efi/built_in.o -efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o: ; - asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c $(CC) $(filter-out -Wa$(comma)% -flto,$(CFLAGS)) -S -o $@ $< @@ -220,21 +147,11 @@ xen.lds: xen.lds.S sed -e 's/xen\.lds\.o:/xen\.lds:/g' <.xen.lds.d >.xen.lds.d.new mv -f .xen.lds.d.new .xen.lds.d -efi.lds: xen.lds.S - $(CC) -P -E -Ui386 -DEFI $(filter-out -Wa$(comma)%,$(AFLAGS)) -o $@ $< - sed -e 's/efi\.lds\.o:/efi\.lds:/g' <.$(@F).d >.$(@F).d.new - mv -f .$(@F).d.new .$(@F).d - boot/mkelf32: boot/mkelf32.c $(HOSTCC) $(HOSTCFLAGS) -o $@ $< -efi/mkreloc: efi/mkreloc.c - $(HOSTCC) $(HOSTCFLAGS) -g -o $@ $< - .PHONY: clean 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/cmdline.S boot/reloc.S boot/*.lnk boot/*.bin - rm -f note.o diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index 48f1b00..578461b 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -139,7 +139,7 @@ optional_header: .long __2M_rwdata_end - efi_pe_head_end /* SizeOfCode. */ .long 0 /* SizeOfInitializedData. */ .long 0 /* SizeOfUninitializedData. */ - .long sym_offs(efi_mb_start) /* AddressOfEntryPoint. */ + .long sym_offs(efi_start) /* AddressOfEntryPoint. */ .long sym_offs(start) /* BaseOfCode. */ .quad sym_offs(__image_base__) /* ImageBase. */ .long XEN_LOAD_ALIGN /* SectionAlignment. */ diff --git a/xen/arch/x86/efi/Makefile b/xen/arch/x86/efi/Makefile index 3be9661..61cb8f6 100644 --- a/xen/arch/x86/efi/Makefile +++ b/xen/arch/x86/efi/Makefile @@ -1,16 +1,5 @@ CFLAGS += -fshort-wchar -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) - -%.o: %.ihex - $(OBJCOPY) -I ihex -O binary $< $@ - -boot.init.o: buildid.o - -obj-y := stub.o -obj-$(efi) := boot.init.o compat.o relocs-dummy.o runtime.o -extra-$(efi) += buildid.o -nocov-$(efi) += stub.o +obj-y += boot.init.o +obj-y += compat.o +obj-y += runtime.o diff --git a/xen/arch/x86/efi/buildid.ihex b/xen/arch/x86/efi/buildid.ihex deleted file mode 100644 index a89046d..0000000 --- a/xen/arch/x86/efi/buildid.ihex +++ /dev/null @@ -1,3 +0,0 @@ -:10000000648600004D8DAD57140000000000000014 -:0400100000000000EC -:00000001FF diff --git a/xen/arch/x86/efi/check.c b/xen/arch/x86/efi/check.c deleted file mode 100644 index 7fedd5a..0000000 --- a/xen/arch/x86/efi/check.c +++ /dev/null @@ -1,4 +0,0 @@ -int __attribute__((__ms_abi__)) test(int i) -{ - return i; -} diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index f8aaa37..25a82f4 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -35,75 +35,6 @@ static void __init edd_put_string(u8 *dst, size_t n, const char *src) extern intpte_t __page_tables_start[], __page_tables_end[]; -#define in_page_tables(v) ((intpte_t *)(v) >= __page_tables_start && \ - (intpte_t *)(v) < __page_tables_end) - -#define PE_BASE_RELOC_ABS 0 -#define PE_BASE_RELOC_HIGHLOW 3 -#define PE_BASE_RELOC_DIR64 10 - -extern const struct pe_base_relocs { - u32 rva; - u32 size; - u16 entries[]; -} __base_relocs_start[], __base_relocs_end[]; - -static void __init efi_arch_relocate_image(unsigned long delta) -{ - const struct pe_base_relocs *base_relocs; - - if ( efi_enabled(EFI_MB_LOADER) ) - return; - - for ( base_relocs = __base_relocs_start; base_relocs < __base_relocs_end; ) - { - unsigned int i = 0, n; - - n = (base_relocs->size - sizeof(*base_relocs)) / - sizeof(*base_relocs->entries); - - /* - * Relevant l{2,3}_bootmap entries get initialized explicitly in - * efi_arch_memory_setup(), so we must not apply relocations there. - * l2_identmap's first slot, otoh, should be handled normally, as - * efi_arch_memory_setup() won't touch it (xen_phys_start should - * never be zero). - */ - if ( xen_phys_start + base_relocs->rva == (unsigned long)l3_bootmap || - xen_phys_start + base_relocs->rva == (unsigned long)l2_bootmap ) - i = n; - - for ( ; i < n; ++i ) - { - unsigned long addr = xen_phys_start + base_relocs->rva + - (base_relocs->entries[i] & 0xfff); - - switch ( base_relocs->entries[i] >> 12 ) - { - case PE_BASE_RELOC_ABS: - break; - case PE_BASE_RELOC_HIGHLOW: - if ( delta ) - { - *(u32 *)addr += delta; - if ( in_page_tables(addr) ) - *(u32 *)addr += xen_phys_start; - } - break; - case PE_BASE_RELOC_DIR64: - if ( in_page_tables(addr) ) - blexit(L"Unexpected relocation type"); - if ( delta ) - *(u64 *)addr += delta; - break; - default: - blexit(L"Unsupported relocation type"); - } - } - base_relocs = (const void *)(base_relocs->entries + i + (i & 1)); - } -} - extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[]; extern const s32 __trampoline_seg_start[], __trampoline_seg_stop[]; @@ -237,7 +168,6 @@ static void __init noreturn efi_arch_post_exit_boot(void) { u64 cr4 = XEN_MINIMAL_CR4 & ~X86_CR4_PGE, efer; - efi_arch_relocate_image(__XEN_VIRT_START - xen_phys_start); memcpy((void *)trampoline_phys, trampoline_start, cfg.size); /* Set system registers and transfer control. */ @@ -589,11 +519,10 @@ static void __init efi_arch_memory_setup(void) if ( !efi_enabled(EFI_LOADER) ) return; - if ( efi_enabled(EFI_MB_LOADER) ) - for ( pte = __page_tables_start; pte < __page_tables_end; - pte += ( pte != (intpte_t *)l2_identmap ) ? 1 : 4 * L2_PAGETABLE_ENTRIES ) - if ( get_pte_flags(*pte) & _PAGE_PRESENT ) - *pte += xen_phys_start; + for ( pte = __page_tables_start; pte < __page_tables_end; + pte += ( pte != (intpte_t *)l2_identmap ) ? 1 : 4 * L2_PAGETABLE_ENTRIES ) + if ( get_pte_flags(*pte) & _PAGE_PRESENT ) + *pte += xen_phys_start; /* Initialise L2 identity-map and boot-map page table entries (16MB). */ for ( i = 0; i < 8; ++i ) @@ -687,15 +616,6 @@ static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable) static void __init efi_arch_flush_dcache_area(const void *vaddr, UINTN size) { } -void EFIAPI efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable); - -void EFIAPI __init noreturn -efi_mb_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) -{ - __set_bit(EFI_MB_LOADER, &efi_flags); - efi_start(ImageHandle, SystemTable); -} - void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable, const multiboot2_tag_module_t *dom0_kernel) diff --git a/xen/arch/x86/efi/mkreloc.c b/xen/arch/x86/efi/mkreloc.c deleted file mode 100644 index 1aca796..0000000 --- a/xen/arch/x86/efi/mkreloc.c +++ /dev/null @@ -1,384 +0,0 @@ -#include <fcntl.h> -#include <inttypes.h> -#include <limits.h> -#include <stddef.h> -#include <stdint.h> -#include <stdio.h> -#include <stdlib.h> -#include <string.h> -#include <sys/mman.h> -#include <unistd.h> - -struct mz_hdr { - uint16_t signature; -#define MZ_SIGNATURE 0x5a4d - uint16_t last_page_size; - uint16_t page_count; - uint16_t relocation_count; - uint16_t header_paras; - uint16_t min_paras; - uint16_t max_paras; - uint16_t entry_ss; - uint16_t entry_sp; - uint16_t checksum; - uint16_t entry_ip; - uint16_t entry_cs; - uint16_t relocations; - uint16_t overlay; - uint8_t reserved[32]; - uint32_t extended_header_base; -}; - -struct pe_hdr { - uint32_t signature; -#define PE_SIGNATURE 0x00004550 - uint16_t cpu; - uint16_t section_count; - int32_t timestamp; - uint32_t symbols_file_offset; - uint32_t symbol_count; - uint16_t opt_hdr_size; - uint16_t flags; - struct { - uint16_t magic; -#define PE_MAGIC_EXE32 0x010b -#define PE_MAGIC_EXE32PLUS 0x020b - uint8_t linker_major, linker_minor; - uint32_t code_size, data_size, bss_size; - uint32_t entry_rva, code_rva, data_rva; - } opt_hdr; -}; - -#define PE_PAGE_SIZE 0x1000 - -#define PE_BASE_RELOC_ABS 0 -#define PE_BASE_RELOC_HIGHLOW 3 -#define PE_BASE_RELOC_DIR64 10 - -struct coff_section { - char name[8]; - uint32_t size; - uint32_t rva; - uint32_t file_size; - uint32_t file_offset; - uint32_t relocation_file_offset; - uint32_t line_number_file_offset; - uint16_t relocation_count; - uint16_t line_number_count; - uint32_t flags; -#define COFF_SECTION_BSS 0x00000080U -#define COFF_SECTION_DISCARDABLE 0x02000000U -#define COFF_SECTION_WRITEABLE 0x80000000U -}; - -static void usage(const char *cmd, int rc) -{ - fprintf(rc ? stderr : stdout, - "Usage: %s <image1> <image2>\n", - cmd); - exit(rc); -} - -static unsigned int load(const char *name, int *handle, - struct coff_section **sections, - uint_fast64_t *image_base, - uint32_t *image_size, - unsigned int *width) -{ - int in = open(name, O_RDONLY); - struct mz_hdr mz_hdr; - struct pe_hdr pe_hdr; - uint32_t base; - - if ( in < 0 || - read(in, &mz_hdr, sizeof(mz_hdr)) != sizeof(mz_hdr) ) - { - perror(name); - exit(2); - } - if ( mz_hdr.signature != MZ_SIGNATURE || - mz_hdr.relocations < sizeof(mz_hdr) || - !mz_hdr.extended_header_base ) - { - fprintf(stderr, "%s: Wrong DOS file format\n", name); - exit(2); - } - - if ( lseek(in, mz_hdr.extended_header_base, SEEK_SET) < 0 || - read(in, &pe_hdr, sizeof(pe_hdr)) != sizeof(pe_hdr) || - read(in, &base, sizeof(base)) != sizeof(base) || - /* - * Luckily the image size field lives at the - * same offset for both formats. - */ - lseek(in, 24, SEEK_CUR) < 0 || - read(in, image_size, sizeof(*image_size)) != sizeof(*image_size) ) - { - perror(name); - exit(3); - } - switch ( (pe_hdr.signature == PE_SIGNATURE && - pe_hdr.opt_hdr_size > sizeof(pe_hdr.opt_hdr)) * - pe_hdr.opt_hdr.magic ) - { - case PE_MAGIC_EXE32: - *width = 32; - *image_base = base; - break; - case PE_MAGIC_EXE32PLUS: - *width = 64; - *image_base = ((uint64_t)base << 32) | pe_hdr.opt_hdr.data_rva; - break; - default: - fprintf(stderr, "%s: Wrong PE file format\n", name); - exit(3); - } - - *sections = malloc(pe_hdr.section_count * sizeof(**sections)); - if ( !*sections ) - { - perror(NULL); - exit(4); - } - if ( lseek(in, - mz_hdr.extended_header_base + offsetof(struct pe_hdr, opt_hdr) + - pe_hdr.opt_hdr_size, - SEEK_SET) < 0 || - read(in, *sections, pe_hdr.section_count * sizeof(**sections)) != - pe_hdr.section_count * sizeof(**sections) ) - { - perror(name); - exit(4); - } - - *handle = in; - - return pe_hdr.section_count; -} - -static long page_size; - -static const void *map_section(const struct coff_section *sec, int in, - const char *name) -{ - const char *ptr; - unsigned long offs; - - if ( !page_size ) - page_size = sysconf(_SC_PAGESIZE); - offs = sec->file_offset & (page_size - 1); - - ptr = mmap(0, offs + sec->file_size, PROT_READ, MAP_PRIVATE, in, - sec->file_offset - offs); - if ( ptr == MAP_FAILED ) - { - perror(name); - exit(6); - } - - return ptr + offs; -} - -static void unmap_section(const void *ptr, const struct coff_section *sec) -{ - unsigned long offs = sec->file_offset & (page_size - 1); - - munmap((char *)ptr - offs, offs + sec->file_size); -} - -static void diff_sections(const unsigned char *ptr1, const unsigned char *ptr2, - const struct coff_section *sec, - int_fast64_t diff, unsigned int width, - uint_fast64_t base, uint_fast64_t end) -{ - static uint_fast32_t cur_rva, reloc_size; - unsigned int disp = 0; - uint_fast32_t i; - - if ( !sec ) - { - reloc_size += reloc_size & 2; - if ( reloc_size ) - printf("\t.balign 4\n" - "\t.equ rva_%08" PRIxFAST32 "_relocs, %#08" PRIxFAST32 "\n", - cur_rva, reloc_size); - return; - } - - while ( !(diff & (((int_fast64_t)1 << ((disp + 1) * CHAR_BIT)) - 1)) ) - ++disp; - - for ( i = 0; i < sec->file_size; ++i ) - { - uint_fast32_t rva; - union { - uint32_t u32; - uint64_t u64; - } val1, val2; - int_fast64_t delta; - unsigned int reloc = (width == 4 ? PE_BASE_RELOC_HIGHLOW : - PE_BASE_RELOC_DIR64); - - if ( ptr1[i] == ptr2[i] ) - continue; - - if ( i < disp || i + width - disp > sec->file_size ) - { - fprintf(stderr, - "Bogus difference at %.8s:%08" PRIxFAST32 "\n", - sec->name, i); - exit(3); - } - - memcpy(&val1, ptr1 + i - disp, width); - memcpy(&val2, ptr2 + i - disp, width); - delta = width == 4 ? val2.u32 - val1.u32 : val2.u64 - val1.u64; - if ( delta != diff ) - { - fprintf(stderr, - "Difference at %.8s:%08" PRIxFAST32 " is %#" PRIxFAST64 - " (expected %#" PRIxFAST64 ")\n", - sec->name, i, delta, diff); - continue; - } - if ( width == 8 && (val1.u64 < base || val1.u64 > end) ) - reloc = PE_BASE_RELOC_HIGHLOW; - - rva = (sec->rva + i - disp) & ~(PE_PAGE_SIZE - 1); - if ( rva > cur_rva ) - { - reloc_size += reloc_size & 2; - if ( reloc_size ) - printf("\t.equ rva_%08" PRIxFAST32 "_relocs," - " %#08" PRIxFAST32 "\n", - cur_rva, reloc_size); - printf("\t.balign 4\n" - "\t.long %#08" PRIxFAST32 "," - " rva_%08" PRIxFAST32 "_relocs\n", - rva, rva); - cur_rva = rva; - reloc_size = 8; - } - else if ( rva != cur_rva ) - { - fprintf(stderr, - "Cannot handle decreasing RVA (at %.8s:%08" PRIxFAST32 ")\n", - sec->name, i); - exit(3); - } - - if ( !(sec->flags & COFF_SECTION_WRITEABLE) ) - fprintf(stderr, - "Warning: relocation to r/o section %.8s:%08" PRIxFAST32 "\n", - sec->name, i); - - printf("\t.word (%u << 12) | 0x%03" PRIxFAST32 "\n", - reloc, sec->rva + i - disp - rva); - reloc_size += 2; - i += width - disp - 1; - } -} - -int main(int argc, char *argv[]) -{ - int in1, in2; - unsigned int i, nsec, width1, width2; - uint_fast64_t base1, base2; - uint32_t size1, size2; - struct coff_section *sec1, *sec2; - - if ( argc == 1 || - !strcmp(argv[1], "-?") || - !strcmp(argv[1], "-h") || - !strcmp(argv[1], "--help") ) - usage(*argv, argc == 1); - - if ( argc != 3 ) - usage(*argv, 1); - - nsec = load(argv[1], &in1, &sec1, &base1, &size1, &width1); - if ( nsec != load(argv[2], &in2, &sec2, &base2, &size2, &width2) ) - { - fputs("Mismatched section counts\n", stderr); - return 5; - } - if ( width1 != width2 ) - { - fputs("Mismatched image types\n", stderr); - return 5; - } - width1 >>= 3; - if ( base1 == base2 ) - { - fputs("Images must have different base addresses\n", stderr); - return 5; - } - if ( size1 != size2 ) - { - fputs("Images must have identical sizes\n", stderr); - return 5; - } - - puts("\t.section .reloc, \"a\", @progbits\n" - "\t.balign 4\n" - "\t.globl __base_relocs_start, __base_relocs_end\n" - "__base_relocs_start:"); - - for ( i = 0; i < nsec; ++i ) - { - const void *ptr1, *ptr2; - - if ( memcmp(sec1[i].name, sec2[i].name, sizeof(sec1[i].name)) || - sec1[i].rva != sec2[i].rva || - sec1[i].size != sec2[i].size || - sec1[i].file_size != sec2[i].file_size || - sec1[i].flags != sec2[i].flags ) - { - fprintf(stderr, "Mismatched section %u parameters\n", i); - return 5; - } - - if ( !sec1[i].size || - (sec1[i].flags & (COFF_SECTION_DISCARDABLE|COFF_SECTION_BSS)) ) - continue; - - /* - * Don't generate relocations for sections that definitely - * aren't used by the boot loader code. - */ - if ( memcmp(sec1[i].name, ".initcal", sizeof(sec1[i].name)) == 0 || - memcmp(sec1[i].name, ".init.se", sizeof(sec1[i].name)) == 0 || - memcmp(sec1[i].name, ".buildid", sizeof(sec1[i].name)) == 0 || - memcmp(sec1[i].name, ".lockpro", sizeof(sec1[i].name)) == 0 ) - continue; - - if ( !sec1[i].rva ) - { - fprintf(stderr, "Can't handle section %u with zero RVA\n", i); - return 3; - } - - if ( sec1[i].file_size > sec1[i].size ) - { - sec1[i].file_size = sec1[i].size; - sec2[i].file_size = sec2[i].size; - } - ptr1 = map_section(sec1 + i, in1, argv[1]); - ptr2 = map_section(sec2 + i, in2, argv[2]); - - diff_sections(ptr1, ptr2, sec1 + i, base2 - base1, width1, - base1, base1 + size1); - - unmap_section(ptr1, sec1 + i); - unmap_section(ptr2, sec2 + i); - } - - diff_sections(NULL, NULL, NULL, 0, 0, 0, 0); - - puts("__base_relocs_end:"); - - close(in1); - close(in2); - - return 0; -} diff --git a/xen/arch/x86/efi/relocs-dummy.S b/xen/arch/x86/efi/relocs-dummy.S deleted file mode 100644 index d928a82..0000000 --- a/xen/arch/x86/efi/relocs-dummy.S +++ /dev/null @@ -1,11 +0,0 @@ - - .section .reloc, "a", @progbits - .balign 4 -GLOBAL(__base_relocs_start) - .long 0 - .long 8 -GLOBAL(__base_relocs_end) - - .globl VIRT_START, ALT_START - .equ VIRT_START, XEN_VIRT_START - .equ ALT_START, XEN_VIRT_END diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.c deleted file mode 100644 index 5918536..0000000 --- a/xen/arch/x86/efi/stub.c +++ /dev/null @@ -1,97 +0,0 @@ -#include <xen/types.h> -#include <xen/efi.h> -#include <xen/errno.h> -#include <xen/init.h> -#include <xen/lib.h> -#include <xen/multiboot2.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_mb_start() and efi_multiboot2() are the exceptions. - * Please look below for more details. - */ - -asm ( - " .text \n" - " .globl efi_mb_start \n" - "efi_mb_start: \n" - " mov %rcx,%rdi \n" - " mov %rdx,%rsi \n" - " xor %rdx,%rdx \n" - " call efi_multiboot2 \n" - ); - -void __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle, - EFI_SYSTEM_TABLE *SystemTable, - const multiboot2_tag_module_t *dom0_kernel) -{ - static const CHAR16 __initconst 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 *%3 \n" - "0: hlt \n" - " jmp 0b \n" - : "+c" (StdErr), "=d" (StdErr) : "1" (err), "rm" (StdErr->OutputString) - : "rax", "r8", "r9", "r10", "r11", "memory"); - - unreachable(); -} - -bool efi_enabled(unsigned int feature) -{ - return false; -} - -void __init efi_init_memory(void) { } - -void efi_update_l4_pgtable(unsigned int l4idx, l4_pgentry_t l4e) { } - -bool efi_rs_using_pgtables(void) -{ - return false; -} - -unsigned long efi_get_time(void) -{ - BUG(); - return 0; -} - -void efi_halt_system(void) { } -void efi_reset_system(bool warm) { } - -int efi_get_info(uint32_t idx, union xenpf_efi_info *info) -{ - return -ENOSYS; -} - -int efi_compat_get_info(uint32_t idx, union compat_pf_efi_info *) - __attribute__((__alias__("efi_get_info"))); - -int efi_runtime_call(struct xenpf_efi_runtime_call *op) -{ - return -ENOSYS; -} - -int efi_compat_runtime_call(struct compat_pf_efi_runtime_call *) - __attribute__((__alias__("efi_runtime_call"))); diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index 1e5233a..43e0799 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -7,26 +7,12 @@ #undef ENTRY #undef ALIGN -#ifdef EFI - -#define FORMAT "pei-x86-64" -#undef __XEN_VIRT_START -#define __XEN_VIRT_START __image_base__ -#define SECTION_ALIGN MB(2) -#define DECL_SECTION(x) x : - -ENTRY(efi_start) - -#else /* !EFI */ - #define FORMAT "elf64-x86-64" #define SECTION_ALIGN PAGE_SIZE #define DECL_SECTION(x) x : AT(ADDR(x) - __XEN_VIRT_START) ENTRY(start_pa) -#endif /* EFI */ - OUTPUT_FORMAT(FORMAT, FORMAT, FORMAT) OUTPUT_ARCH(i386:x86-64) @@ -34,18 +20,14 @@ OUTPUT_ARCH(i386:x86-64) PHDRS { text PT_LOAD ; -#if (defined(BUILD_ID) || defined (CONFIG_PVH_GUEST)) && !defined(EFI) +#if defined(BUILD_ID) || defined (CONFIG_PVH_GUEST) note PT_NOTE ; #endif } SECTIONS { -#if !defined(EFI) . = __XEN_VIRT_START; __image_base__ = .; -#else - . = __image_base__; -#endif #if 0 /* @@ -60,9 +42,6 @@ SECTIONS start_pa = ABSOLUTE(start - __XEN_VIRT_START); -#ifdef EFI - . = __XEN_VIRT_START + XEN_IMG_OFFSET; -#else /* * The PE header must be followed by .text section which * starts at __XEN_VIRT_START + XEN_IMG_OFFSET address. @@ -72,7 +51,6 @@ SECTIONS DECL_SECTION(.efi.pe.header) { *(.efi.pe.header) } :NONE -#endif _start = .; DECL_SECTION(.text) { @@ -117,16 +95,6 @@ SECTIONS *(.data.rel.ro) *(.data.rel.ro.*) -#if defined(BUILD_ID) && defined(EFI) && !defined(BUILD_ID_EFI) -/* - * No mechanism to put an PT_NOTE in the EFI file - so put - * it in .rodata section. (notes.o supplies us with .note.gnu.build-id). - */ - . = ALIGN(4); - __note_gnu_build_id_start = .; - *(.note.gnu.build-id) - __note_gnu_build_id_end = .; -#endif . = ALIGN(8); /* Exception table */ __start___ex_table = .; @@ -157,32 +125,23 @@ SECTIONS #endif } :text -#if defined(CONFIG_PVH_GUEST) && !defined(EFI) +#if defined(CONFIG_PVH_GUEST) DECL_SECTION(.note.Xen) { *(.note.Xen) } :note :text #endif #if defined(BUILD_ID) -#if !defined(EFI) /* - * What a strange section name. The reason is that on ELF builds this section - * is extracted to notes.o (which then is ingested in the EFI file). But the - * compiler may want to inject other things in the .note which we don't care - * about - hence this unique name. + * What a strange section name. The reason is that the compiler may want to + * inject other things in the .note which we don't care about - hence this + * unique name. */ DECL_SECTION(.note.gnu.build-id) { __note_gnu_build_id_start = .; *(.note.gnu.build-id) __note_gnu_build_id_end = .; } :note :text -#elif defined(BUILD_ID_EFI) - DECL_SECTION(.buildid) { - __note_gnu_build_id_start = .; - *(.buildid) - __note_gnu_build_id_end = .; - } :text -#endif #endif _erodata = .; @@ -192,11 +151,7 @@ SECTIONS __2M_init_start = .; /* Start of 2M superpages, mapped RWX (boot only). */ . = ALIGN(PAGE_SIZE); /* Init code and data */ __init_begin = .; -#ifdef EFI /* EFI wants to merge all of .init.* ELF doesn't. */ - DECL_SECTION(.init) { -#else DECL_SECTION(.init.text) { -#endif _sinittext = .; *(.init.text) /* @@ -207,12 +162,8 @@ SECTIONS *(.altinstr_replacement) _einittext = .; -#ifdef EFI /* EFI wants to merge all of .init.* ELF doesn't. */ - . = ALIGN(SMP_CACHE_BYTES); -#else } :text DECL_SECTION(.init.data) { -#endif *(.init.rodata) *(.init.rodata.rel) @@ -310,20 +261,6 @@ SECTIONS __pe_SizeOfImage = ALIGN(. - __image_base__, MB(16)); -#ifdef EFI - . = ALIGN(4); - .reloc : { - *(.reloc) - } :text - /* Trick the linker into setting the image size to exactly 16Mb. */ - . = ALIGN(__section_alignment__); - .pad : { - . = ALIGN(MB(16)); - } :text -#endif - - efi = DEFINED(efi) ? efi : .; - /* Sections to be discarded */ /DISCARD/ : { *(.exit.text) @@ -332,12 +269,6 @@ SECTIONS *(.discard) *(.discard.*) *(.eh_frame) -#ifdef EFI - *(.efi.pe.header) - *(.comment) - *(.comment.*) - *(.note.Xen) -#endif } /* Stabs debugging sections. */ @@ -381,11 +312,9 @@ ASSERT((trampoline_end - trampoline_start) < TRAMPOLINE_SPACE - MBI_SPACE_MIN, ASSERT((wakeup_stack - wakeup_stack_start) >= WAKEUP_STACK_MIN, "wakeup stack too small") -#ifndef EFI ASSERT(efi_pe_head_end == _start, "PE header does not end at the beginning of .text section") ASSERT(_start == __XEN_VIRT_START + XEN_IMG_OFFSET, ".text section begins at wrong address") ASSERT(IS_ALIGNED(_start, XEN_FILE_ALIGN), "_start misaligned") ASSERT(IS_ALIGNED(__bss_start, XEN_FILE_ALIGN), "__bss_start misaligned") ASSERT(IS_ALIGNED(__pe_SizeOfImage, XEN_LOAD_ALIGN), "__pe_SizeOfImage is not multiple of XEN_LOAD_ALIGN") ASSERT(XEN_LOAD_ALIGN >= XEN_FILE_ALIGN, "XEN_LOAD_ALIGN < XEN_FILE_ALIGN") -#endif diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 06bfadc..a11f97b 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -1160,8 +1160,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) PrintStr(L"Xen " __stringify(XEN_VERSION) "." __stringify(XEN_SUBVERSION) XEN_EXTRAVERSION " (c/s " XEN_CHANGESET ") EFI loader\r\n"); - efi_arch_relocate_image(0); - if ( use_cfg_file ) { EFI_FILE_HANDLE dir_handle; diff --git a/xen/common/version.c b/xen/common/version.c index 223cb52..3b4ff9a 100644 --- a/xen/common/version.c +++ b/xen/common/version.c @@ -4,7 +4,6 @@ #include <xen/lib.h> #include <xen/string.h> #include <xen/types.h> -#include <xen/efi.h> #include <xen/elf.h> #include <xen/version.h> @@ -118,28 +117,6 @@ int xen_build_id_check(const Elf_Note *n, unsigned int n_sz, return 0; } -struct pe_external_debug_directory -{ - uint32_t characteristics; - uint32_t time_stamp; - uint16_t major_version; - uint16_t minor_version; -#define PE_IMAGE_DEBUG_TYPE_CODEVIEW 2 - uint32_t type; - uint32_t size; - uint32_t rva_of_data; - uint32_t filepos_of_data; -}; - -struct cv_info_pdb70 -{ -#define CVINFO_PDB70_CVSIGNATURE 0x53445352 /* "RSDS" */ - uint32_t cv_signature; - unsigned char signature[16]; - uint32_t age; - char pdb_filename[]; -}; - static int __init xen_build_init(void) { const Elf_Note *n = __note_gnu_build_id_start; @@ -158,34 +135,6 @@ static int __init xen_build_init(void) rc = xen_build_id_check(n, sz, &build_id_p, &build_id_len); -#ifdef CONFIG_X86 - /* Alternatively we may have a CodeView record from an EFI build. */ - if ( rc && efi_enabled(EFI_LOADER) ) - { - const struct pe_external_debug_directory *dir = (const void *)n; - - /* - * Validate that the full-note-header check above won't prevent - * fall-through to the CodeView case here. - */ - BUILD_BUG_ON(sizeof(*n) > sizeof(*dir)); - - if ( sz > sizeof(*dir) + sizeof(struct cv_info_pdb70) && - dir->type == PE_IMAGE_DEBUG_TYPE_CODEVIEW && - dir->size > sizeof(struct cv_info_pdb70) && - XEN_VIRT_START + dir->rva_of_data == (unsigned long)(dir + 1) ) - { - const struct cv_info_pdb70 *info = (const void *)(dir + 1); - - if ( info->cv_signature == CVINFO_PDB70_CVSIGNATURE ) - { - build_id_p = info->signature; - build_id_len = sizeof(info->signature); - rc = 0; - } - } - } -#endif if ( !rc ) printk(XENLOG_INFO "build-id: %*phN\n", build_id_len, build_id_p); diff --git a/xen/include/xen/efi.h b/xen/include/xen/efi.h index 73f83c1..44b7d3e 100644 --- a/xen/include/xen/efi.h +++ b/xen/include/xen/efi.h @@ -11,7 +11,6 @@ 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? */ -#define EFI_MB_LOADER 4 /* xen.mb.efi booted directly from EFI loader? */ /* Add fields here only if they need to be referenced from non-EFI code. */ struct efi { -- 1.7.10.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [PATCH v2 8/8] efi: drop original xen.efi code and build mechanism 2018-06-19 14:35 ` [PATCH v2 8/8] efi: drop original xen.efi code and build mechanism Daniel Kiper @ 2018-06-28 13:51 ` Jan Beulich 2018-07-04 14:25 ` Daniel Kiper 0 siblings, 1 reply; 50+ messages in thread From: Jan Beulich @ 2018-06-28 13:51 UTC (permalink / raw) To: Daniel Kiper Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel >>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: > Then rename xen.mb.efi to xen.efi and drop all related > differentiators in the code. For this you'll first of all need to convince me that the binary you build is a drop-in replacement for xen.efi. As noted in the replies to earlier patches, I'm getting the impression of this not being the case. A further hint towards this is the outright deletion of xen/arch/x86/efi/mkreloc.c: How is the Xen image going to be relocated that way, when loaded from the EFI shell or boot loader? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 8/8] efi: drop original xen.efi code and build mechanism 2018-06-28 13:51 ` Jan Beulich @ 2018-07-04 14:25 ` Daniel Kiper 2018-07-04 15:34 ` Jan Beulich 0 siblings, 1 reply; 50+ messages in thread From: Daniel Kiper @ 2018-07-04 14:25 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel On Thu, Jun 28, 2018 at 07:51:52AM -0600, Jan Beulich wrote: > >>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: > > Then rename xen.mb.efi to xen.efi and drop all related > > differentiators in the code. > > For this you'll first of all need to convince me that the binary you build is > a drop-in replacement for xen.efi. As noted in the replies to earlier > patches, I'm getting the impression of this not being the case. A further > hint towards this is the outright deletion of xen/arch/x86/efi/mkreloc.c: > How is the Xen image going to be relocated that way, when loaded from > the EFI shell or boot loader? It works because all addressing is relative to %rip. To be precise, new xen.efi, earlier xen.mb.efi, contains exactly the same code as ELF does. So, if ELF works without any relocations why PE should not. Especially on x86-64. Additionally, my tests showed that in general UEFI implementations just require Base Relocation Table entry in PE Data Directories to relocate the image. Even it can be empty. As it is in current patchset. Though I am afraid about more picky UEFI stuff and considering addition of at least one .reloc entry, fake one, as Linux kernel does. And this rises another question: should not we add .bss section into PE header? Right now it is embedded/hidden in PE .text section. Daniel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 8/8] efi: drop original xen.efi code and build mechanism 2018-07-04 14:25 ` Daniel Kiper @ 2018-07-04 15:34 ` Jan Beulich 2018-07-04 16:48 ` Daniel Kiper 0 siblings, 1 reply; 50+ messages in thread From: Jan Beulich @ 2018-07-04 15:34 UTC (permalink / raw) To: Daniel Kiper Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel >>> On 04.07.18 at 16:25, <daniel.kiper@oracle.com> wrote: > On Thu, Jun 28, 2018 at 07:51:52AM -0600, Jan Beulich wrote: >> >>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: >> > Then rename xen.mb.efi to xen.efi and drop all related >> > differentiators in the code. >> >> For this you'll first of all need to convince me that the binary you build is >> a drop-in replacement for xen.efi. As noted in the replies to earlier >> patches, I'm getting the impression of this not being the case. A further >> hint towards this is the outright deletion of xen/arch/x86/efi/mkreloc.c: >> How is the Xen image going to be relocated that way, when loaded from >> the EFI shell or boot loader? > > It works because all addressing is relative to %rip. To be precise, new > xen.efi, earlier xen.mb.efi, contains exactly the same code as ELF does. > So, if ELF works without any relocations why PE should not. Especially on > x86-64. Additionally, my tests showed that in general UEFI implementations > just require Base Relocation Table entry in PE Data Directories to relocate > the image. The thing I'm missing in the series is the generation of the relocations to go into the section pointed to by this Data Directory entry. > Even it can be empty. As it is in current patchset. No, it can't be empty. Just try removing the relocations from xen.efi and see what you get. > Though I am > afraid about more picky UEFI stuff and considering addition of at least > one .reloc entry, fake one, as Linux kernel does. And this rises another > question: should not we add .bss section into PE header? Right now it is > embedded/hidden in PE .text section. My xen.efi does have a .bss section. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 8/8] efi: drop original xen.efi code and build mechanism 2018-07-04 15:34 ` Jan Beulich @ 2018-07-04 16:48 ` Daniel Kiper 2018-07-05 8:35 ` Jan Beulich 0 siblings, 1 reply; 50+ messages in thread From: Daniel Kiper @ 2018-07-04 16:48 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel On Wed, Jul 04, 2018 at 09:34:09AM -0600, Jan Beulich wrote: > >>> On 04.07.18 at 16:25, <daniel.kiper@oracle.com> wrote: > > On Thu, Jun 28, 2018 at 07:51:52AM -0600, Jan Beulich wrote: > >> >>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: > >> > Then rename xen.mb.efi to xen.efi and drop all related > >> > differentiators in the code. > >> > >> For this you'll first of all need to convince me that the binary you build is > >> a drop-in replacement for xen.efi. As noted in the replies to earlier > >> patches, I'm getting the impression of this not being the case. A further > >> hint towards this is the outright deletion of xen/arch/x86/efi/mkreloc.c: > >> How is the Xen image going to be relocated that way, when loaded from > >> the EFI shell or boot loader? > > > > It works because all addressing is relative to %rip. To be precise, new > > xen.efi, earlier xen.mb.efi, contains exactly the same code as ELF does. > > So, if ELF works without any relocations why PE should not. Especially on > > x86-64. Additionally, my tests showed that in general UEFI implementations > > just require Base Relocation Table entry in PE Data Directories to relocate > > the image. > > The thing I'm missing in the series is the generation of the relocations to > go into the section pointed to by this Data Directory entry. There is no such thing because xen.mb.efi does not need that. > > Even it can be empty. As it is in current patchset. > > No, it can't be empty. Just try removing the relocations from xen.efi and > see what you get. I am pretty sure that current xen.efi will not work without .reloc section. However, xen.mb.efi works without any issue. So, I am not sure where is the problem. +/- fake .reloc entry which I was talking about earlier. > > Though I am > > afraid about more picky UEFI stuff and considering addition of at least > > one .reloc entry, fake one, as Linux kernel does. And this rises another > > question: should not we add .bss section into PE header? Right now it is > > embedded/hidden in PE .text section. > > My xen.efi does have a .bss section. It is completely valid to have BSS embedded in .text section or living as a separate entity in PE section table. So, the question is: which one do we prefer? Currently existing xen.mb.efi layout is similar to ELF layout. Daniel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 8/8] efi: drop original xen.efi code and build mechanism 2018-07-04 16:48 ` Daniel Kiper @ 2018-07-05 8:35 ` Jan Beulich 2018-07-06 14:46 ` Daniel Kiper 0 siblings, 1 reply; 50+ messages in thread From: Jan Beulich @ 2018-07-05 8:35 UTC (permalink / raw) To: Daniel Kiper Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel >>> On 04.07.18 at 18:48, <daniel.kiper@oracle.com> wrote: > On Wed, Jul 04, 2018 at 09:34:09AM -0600, Jan Beulich wrote: >> >>> On 04.07.18 at 16:25, <daniel.kiper@oracle.com> wrote: >> > On Thu, Jun 28, 2018 at 07:51:52AM -0600, Jan Beulich wrote: >> >> >>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: >> >> > Then rename xen.mb.efi to xen.efi and drop all related >> >> > differentiators in the code. >> >> >> >> For this you'll first of all need to convince me that the binary you build is >> >> a drop-in replacement for xen.efi. As noted in the replies to earlier >> >> patches, I'm getting the impression of this not being the case. A further >> >> hint towards this is the outright deletion of xen/arch/x86/efi/mkreloc.c: >> >> How is the Xen image going to be relocated that way, when loaded from >> >> the EFI shell or boot loader? >> > >> > It works because all addressing is relative to %rip. To be precise, new >> > xen.efi, earlier xen.mb.efi, contains exactly the same code as ELF does. >> > So, if ELF works without any relocations why PE should not. Especially on >> > x86-64. Additionally, my tests showed that in general UEFI implementations >> > just require Base Relocation Table entry in PE Data Directories to relocate >> > the image. >> >> The thing I'm missing in the series is the generation of the relocations to >> go into the section pointed to by this Data Directory entry. > > There is no such thing because xen.mb.efi does not need that. Well, fine. But you still owe me an answer to the "why" part here. >> > Even it can be empty. As it is in current patchset. >> >> No, it can't be empty. Just try removing the relocations from xen.efi and >> see what you get. > > I am pretty sure that current xen.efi will not work without .reloc section. > However, xen.mb.efi works without any issue. So, I am not sure where is the > problem. +/- fake .reloc entry which I was talking about earlier. As per above: If this is the case - fine. But I want this to be explained, not the least because I'd like to understand if I wasted effort back when I added to code to produce and handle the relocations. >> > Though I am >> > afraid about more picky UEFI stuff and considering addition of at least >> > one .reloc entry, fake one, as Linux kernel does. And this rises another >> > question: should not we add .bss section into PE header? Right now it is >> > embedded/hidden in PE .text section. >> >> My xen.efi does have a .bss section. > > It is completely valid to have BSS embedded in .text section or living > as a separate entity in PE section table. So, the question is: which one > do we prefer? Currently existing xen.mb.efi layout is similar to ELF layout. I'd prefer any binary to look as natural as possible. However, our ELF binary doesn't, and hence I wouldn't insist on this to be the case for the PE one. There's a reason for the ELF one to be the way it is, though. If no similar reason exists for the PE one, then please have it have a "normal" set of sections. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 8/8] efi: drop original xen.efi code and build mechanism 2018-07-05 8:35 ` Jan Beulich @ 2018-07-06 14:46 ` Daniel Kiper 2018-07-06 15:16 ` Jan Beulich 0 siblings, 1 reply; 50+ messages in thread From: Daniel Kiper @ 2018-07-06 14:46 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel On Thu, Jul 05, 2018 at 02:35:32AM -0600, Jan Beulich wrote: > >>> On 04.07.18 at 18:48, <daniel.kiper@oracle.com> wrote: > > On Wed, Jul 04, 2018 at 09:34:09AM -0600, Jan Beulich wrote: > >> >>> On 04.07.18 at 16:25, <daniel.kiper@oracle.com> wrote: > >> > On Thu, Jun 28, 2018 at 07:51:52AM -0600, Jan Beulich wrote: > >> >> >>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: > >> >> > Then rename xen.mb.efi to xen.efi and drop all related > >> >> > differentiators in the code. > >> >> > >> >> For this you'll first of all need to convince me that the binary you build is > >> >> a drop-in replacement for xen.efi. As noted in the replies to earlier > >> >> patches, I'm getting the impression of this not being the case. A further > >> >> hint towards this is the outright deletion of xen/arch/x86/efi/mkreloc.c: > >> >> How is the Xen image going to be relocated that way, when loaded from > >> >> the EFI shell or boot loader? > >> > > >> > It works because all addressing is relative to %rip. To be precise, new > >> > xen.efi, earlier xen.mb.efi, contains exactly the same code as ELF does. > >> > So, if ELF works without any relocations why PE should not. Especially on > >> > x86-64. Additionally, my tests showed that in general UEFI implementations > >> > just require Base Relocation Table entry in PE Data Directories to relocate > >> > the image. > >> > >> The thing I'm missing in the series is the generation of the relocations to > >> go into the section pointed to by this Data Directory entry. > > > > There is no such thing because xen.mb.efi does not need that. > > Well, fine. But you still owe me an answer to the "why" part here. > > >> > Even it can be empty. As it is in current patchset. > >> > >> No, it can't be empty. Just try removing the relocations from xen.efi and > >> see what you get. > > > > I am pretty sure that current xen.efi will not work without .reloc section. > > However, xen.mb.efi works without any issue. So, I am not sure where is the > > problem. +/- fake .reloc entry which I was talking about earlier. > > As per above: If this is the case - fine. But I want this to be explained, > not the least because I'd like to understand if I wasted effort back when > I added to code to produce and handle the relocations. OK, xen.mb.efi does not need relocs because: - we generate PE file from xen-syms file like we do with ELF output; so, the code in the PE file is the same like in the ELF file; hence, if ELF works why PE should not, - all addressing is relative to %rip as it is in ELF file, - page tables content is updated by Xen EFI boot code and does not depend on .reloc section. Hmmm... And after some thinking I realized that maybe you used relocs because probably it is not possible, or at least difficult, to have relocatable PE EFI ia32 binary without them. IIRC you did that work when 32bit Xen was supported. However, I am not sure Xen PE EFI ia32 was build or not. Another factor maybe was that, IIRC, you derived Xen EFI boot from another internal SUSE EFI project. Hence, there is a chance that it is more or less copy from it. > >> > Though I am > >> > afraid about more picky UEFI stuff and considering addition of at least > >> > one .reloc entry, fake one, as Linux kernel does. And this rises another > >> > question: should not we add .bss section into PE header? Right now it is > >> > embedded/hidden in PE .text section. > >> > >> My xen.efi does have a .bss section. > > > > It is completely valid to have BSS embedded in .text section or living > > as a separate entity in PE section table. So, the question is: which one > > do we prefer? Currently existing xen.mb.efi layout is similar to ELF layout. > > I'd prefer any binary to look as natural as possible. However, our ELF > binary doesn't, and hence I wouldn't insist on this to be the case for > the PE one. There's a reason for the ELF one to be the way it is, though. My guess is that it is related, at least to some extent, to the compatibility with the Muliboot protocol. Commit b199c44 (efi: build xen.gz with EFI code) explains the problem quite well. However, AFAICT, it is possible to build ELF with .text and .bss sections which can be properly digested by the Multiboot protocols. > If no similar reason exists for the PE one, then please have it have a > "normal" set of sections. OK, I will add .bss and fake .reloc section. Daniel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 8/8] efi: drop original xen.efi code and build mechanism 2018-07-06 14:46 ` Daniel Kiper @ 2018-07-06 15:16 ` Jan Beulich 2018-07-10 11:35 ` Daniel Kiper 0 siblings, 1 reply; 50+ messages in thread From: Jan Beulich @ 2018-07-06 15:16 UTC (permalink / raw) To: Daniel Kiper Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel >>> On 06.07.18 at 16:46, <daniel.kiper@oracle.com> wrote: > On Thu, Jul 05, 2018 at 02:35:32AM -0600, Jan Beulich wrote: >> >>> On 04.07.18 at 18:48, <daniel.kiper@oracle.com> wrote: >> > On Wed, Jul 04, 2018 at 09:34:09AM -0600, Jan Beulich wrote: >> >> >>> On 04.07.18 at 16:25, <daniel.kiper@oracle.com> wrote: >> >> > On Thu, Jun 28, 2018 at 07:51:52AM -0600, Jan Beulich wrote: >> >> >> >>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: >> >> >> > Then rename xen.mb.efi to xen.efi and drop all related >> >> >> > differentiators in the code. >> >> >> >> >> >> For this you'll first of all need to convince me that the binary you build is >> >> >> a drop-in replacement for xen.efi. As noted in the replies to earlier >> >> >> patches, I'm getting the impression of this not being the case. A further >> >> >> hint towards this is the outright deletion of xen/arch/x86/efi/mkreloc.c: >> >> >> How is the Xen image going to be relocated that way, when loaded from >> >> >> the EFI shell or boot loader? >> >> > >> >> > It works because all addressing is relative to %rip. To be precise, new >> >> > xen.efi, earlier xen.mb.efi, contains exactly the same code as ELF does. >> >> > So, if ELF works without any relocations why PE should not. Especially on >> >> > x86-64. Additionally, my tests showed that in general UEFI implementations >> >> > just require Base Relocation Table entry in PE Data Directories to relocate >> >> > the image. >> >> >> >> The thing I'm missing in the series is the generation of the relocations to >> >> go into the section pointed to by this Data Directory entry. >> > >> > There is no such thing because xen.mb.efi does not need that. >> >> Well, fine. But you still owe me an answer to the "why" part here. >> >> >> > Even it can be empty. As it is in current patchset. >> >> >> >> No, it can't be empty. Just try removing the relocations from xen.efi and >> >> see what you get. >> > >> > I am pretty sure that current xen.efi will not work without .reloc section. >> > However, xen.mb.efi works without any issue. So, I am not sure where is the >> > problem. +/- fake .reloc entry which I was talking about earlier. >> >> As per above: If this is the case - fine. But I want this to be explained, >> not the least because I'd like to understand if I wasted effort back when >> I added to code to produce and handle the relocations. > > OK, xen.mb.efi does not need relocs because: > - we generate PE file from xen-syms file like we do with ELF output; > so, the code in the PE file is the same like in the ELF file; > hence, if ELF works why PE should not, > - all addressing is relative to %rip as it is in ELF file, What are the several hundred base relocs in xen.efi doing then? Sure some of them wouldn't really be needed, but I doubt that's true for all of them. The first and foremost case of non-RIP-relative addressing is data with static initializers pointing elsewhere in the image. These need relocations applied to work. Once again - a fundamental criteria is whether your binary can be used in place of the current xen.efi. I can't convince myself that you've actually tried that out. At the very least I'd expect the static array in PrintErrMesg() to present problems here. > - page tables content is updated by Xen EFI boot code and > does not depend on .reloc section. > > Hmmm... And after some thinking I realized that maybe you used relocs because > probably it is not possible, or at least difficult, to have relocatable PE > EFI ia32 binary without them. IIRC you did that work when 32bit Xen was > supported. However, I am not sure Xen PE EFI ia32 was build or not. We've never built a xen.efi that would work on a 32-bit EFI. > Another > factor maybe was that, IIRC, you derived Xen EFI boot from another internal > SUSE EFI project. Hence, there is a chance that it is more or less copy > from it. There was no need for base relocations in that original project, because there the binary really was just the OS loader, not the OS kernel itself. >> >> > Though I am >> >> > afraid about more picky UEFI stuff and considering addition of at least >> >> > one .reloc entry, fake one, as Linux kernel does. And this rises another >> >> > question: should not we add .bss section into PE header? Right now it is >> >> > embedded/hidden in PE .text section. >> >> >> >> My xen.efi does have a .bss section. >> > >> > It is completely valid to have BSS embedded in .text section or living >> > as a separate entity in PE section table. So, the question is: which one >> > do we prefer? Currently existing xen.mb.efi layout is similar to ELF layout. >> >> I'd prefer any binary to look as natural as possible. However, our ELF >> binary doesn't, and hence I wouldn't insist on this to be the case for >> the PE one. There's a reason for the ELF one to be the way it is, though. > > My guess is that it is related, at least to some extent, to the > compatibility with the Muliboot protocol. Commit b199c44 (efi: build xen.gz > with EFI code) explains the problem quite well. However, AFAICT, it is > possible to build ELF with .text and .bss sections which can be properly > digested by the Multiboot protocols. > >> If no similar reason exists for the PE one, then please have it have a >> "normal" set of sections. > > OK, I will add .bss and fake .reloc section. As per above - minus "fake". Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 8/8] efi: drop original xen.efi code and build mechanism 2018-07-06 15:16 ` Jan Beulich @ 2018-07-10 11:35 ` Daniel Kiper 2018-07-10 14:05 ` Jan Beulich 0 siblings, 1 reply; 50+ messages in thread From: Daniel Kiper @ 2018-07-10 11:35 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel On Fri, Jul 06, 2018 at 09:16:38AM -0600, Jan Beulich wrote: > >>> On 06.07.18 at 16:46, <daniel.kiper@oracle.com> wrote: > > On Thu, Jul 05, 2018 at 02:35:32AM -0600, Jan Beulich wrote: > >> >>> On 04.07.18 at 18:48, <daniel.kiper@oracle.com> wrote: > >> > On Wed, Jul 04, 2018 at 09:34:09AM -0600, Jan Beulich wrote: > >> >> >>> On 04.07.18 at 16:25, <daniel.kiper@oracle.com> wrote: > >> >> > On Thu, Jun 28, 2018 at 07:51:52AM -0600, Jan Beulich wrote: > >> >> >> >>> On 19.06.18 at 16:35, <daniel.kiper@oracle.com> wrote: > >> >> >> > Then rename xen.mb.efi to xen.efi and drop all related > >> >> >> > differentiators in the code. > >> >> >> > >> >> >> For this you'll first of all need to convince me that the binary you build is > >> >> >> a drop-in replacement for xen.efi. As noted in the replies to earlier > >> >> >> patches, I'm getting the impression of this not being the case. A further > >> >> >> hint towards this is the outright deletion of xen/arch/x86/efi/mkreloc.c: > >> >> >> How is the Xen image going to be relocated that way, when loaded from > >> >> >> the EFI shell or boot loader? > >> >> > > >> >> > It works because all addressing is relative to %rip. To be precise, new > >> >> > xen.efi, earlier xen.mb.efi, contains exactly the same code as ELF does. > >> >> > So, if ELF works without any relocations why PE should not. Especially on > >> >> > x86-64. Additionally, my tests showed that in general UEFI implementations > >> >> > just require Base Relocation Table entry in PE Data Directories to relocate > >> >> > the image. > >> >> > >> >> The thing I'm missing in the series is the generation of the relocations to > >> >> go into the section pointed to by this Data Directory entry. > >> > > >> > There is no such thing because xen.mb.efi does not need that. > >> > >> Well, fine. But you still owe me an answer to the "why" part here. > >> > >> >> > Even it can be empty. As it is in current patchset. > >> >> > >> >> No, it can't be empty. Just try removing the relocations from xen.efi and > >> >> see what you get. > >> > > >> > I am pretty sure that current xen.efi will not work without .reloc section. > >> > However, xen.mb.efi works without any issue. So, I am not sure where is the > >> > problem. +/- fake .reloc entry which I was talking about earlier. > >> > >> As per above: If this is the case - fine. But I want this to be explained, > >> not the least because I'd like to understand if I wasted effort back when > >> I added to code to produce and handle the relocations. > > > > OK, xen.mb.efi does not need relocs because: > > - we generate PE file from xen-syms file like we do with ELF output; > > so, the code in the PE file is the same like in the ELF file; > > hence, if ELF works why PE should not, > > - all addressing is relative to %rip as it is in ELF file, > > What are the several hundred base relocs in xen.efi doing then? Sure > some of them wouldn't really be needed, but I doubt that's true for > all of them. The first and foremost case of non-RIP-relative addressing > is data with static initializers pointing elsewhere in the image. These > need relocations applied to work. > > Once again - a fundamental criteria is whether your binary can be used > in place of the current xen.efi. I can't convince myself that you've > actually tried that out. At the very least I'd expect the static array in > PrintErrMesg() to present problems here. Ugh... You are right. I forgot about that. Sadly the problem applies to the EFI boot code in the xen.gz too. So, both things have to be fixed. At first sight it seems to me that we can leave relocs in the xen-syms and then attach them to the xen.mb.efi/xen.gz somehow. It would be nice to do that just only for the EFI boot code. Should not we put it in separate section then? Another question is the size of the .reloc section. We do not know it in advance. So, probably we should build the code in two steps as we do now. Or prealloc a static place and fill it later. This way we would have just one phase build. Or another option. Add Xen mappings in the early EFI boot code. However, it seems crazy and may not work on all EFI implementations. Hmmm... Have to check the UEFI spec... By the way, I am not sure why mkreloc is executed twice. Could you explain that? Daniel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 8/8] efi: drop original xen.efi code and build mechanism 2018-07-10 11:35 ` Daniel Kiper @ 2018-07-10 14:05 ` Jan Beulich 2018-07-11 11:57 ` Daniel Kiper 0 siblings, 1 reply; 50+ messages in thread From: Jan Beulich @ 2018-07-10 14:05 UTC (permalink / raw) To: Daniel Kiper Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel >>> On 10.07.18 at 13:35, <daniel.kiper@oracle.com> wrote: > On Fri, Jul 06, 2018 at 09:16:38AM -0600, Jan Beulich wrote: >> >>> On 06.07.18 at 16:46, <daniel.kiper@oracle.com> wrote: >> > OK, xen.mb.efi does not need relocs because: >> > - we generate PE file from xen-syms file like we do with ELF output; >> > so, the code in the PE file is the same like in the ELF file; >> > hence, if ELF works why PE should not, >> > - all addressing is relative to %rip as it is in ELF file, >> >> What are the several hundred base relocs in xen.efi doing then? Sure >> some of them wouldn't really be needed, but I doubt that's true for >> all of them. The first and foremost case of non-RIP-relative addressing >> is data with static initializers pointing elsewhere in the image. These >> need relocations applied to work. >> >> Once again - a fundamental criteria is whether your binary can be used >> in place of the current xen.efi. I can't convince myself that you've >> actually tried that out. At the very least I'd expect the static array in >> PrintErrMesg() to present problems here. > > Ugh... You are right. I forgot about that. Sadly the problem applies to > the EFI boot code in the xen.gz too. So, both things have to be fixed. > At first sight it seems to me that we can leave relocs in the xen-syms > and then attach them to the xen.mb.efi/xen.gz somehow. It would be nice > to do that just only for the EFI boot code. Should not we put it in > separate section then? Another question is the size of the .reloc section. > We do not know it in advance. So, probably we should build the code in > two steps as we do now. Or prealloc a static place and fill it later. > This way we would have just one phase build. Any static allocation/reservation scheme is wasteful at first and eventually not allocating/reserving enough space. Unless there's a way to reasonably well estimate the size ahead of time, I'd be opposed to such a model. As to a separate section - sure, why not? Relocations are in a separate section in xen.efi as well. > Or another option. Add Xen mappings in the early EFI boot code. However, > it seems crazy and may not work on all EFI implementations. Hmmm... > Have to check the UEFI spec... I'm afraid I don't understand anyway what you think of here. > By the way, I am not sure why mkreloc is executed twice. Could you > explain that? Its needed as many times as we link a binary, minus the very first time, where stub (dummy) objects are used. I don't see how you think we could get away with just one invocation. Are you thinking we could get away with fewer linking steps? - Link step 0 produces a binary without kallsyms table, but with all symbols. Hence a symbol table generated from it will have the correct number of entries and hence the correct size. - Link step 1 produces a binary with kallsyms table taken from the step 0 binary. This results in all addresses in the resulting binary now being correct (no more code/data is going to be added), but the symbol table itself is wrong (as coming from step 0). - Link step 2 produces a binary with a _correct_ kallsyms table. If we omitted relocations from step 1, we'd risk other addresses changing in step 2 (maybe this is just a theoretical risk, but anyway). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 8/8] efi: drop original xen.efi code and build mechanism 2018-07-10 14:05 ` Jan Beulich @ 2018-07-11 11:57 ` Daniel Kiper [not found] ` <5B45F0CD0200007F03CB4495@prv1-mh.provo.novell.com> 0 siblings, 1 reply; 50+ messages in thread From: Daniel Kiper @ 2018-07-11 11:57 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel On Tue, Jul 10, 2018 at 08:05:56AM -0600, Jan Beulich wrote: > >>> On 10.07.18 at 13:35, <daniel.kiper@oracle.com> wrote: > > On Fri, Jul 06, 2018 at 09:16:38AM -0600, Jan Beulich wrote: > >> >>> On 06.07.18 at 16:46, <daniel.kiper@oracle.com> wrote: > >> > OK, xen.mb.efi does not need relocs because: > >> > - we generate PE file from xen-syms file like we do with ELF output; > >> > so, the code in the PE file is the same like in the ELF file; > >> > hence, if ELF works why PE should not, > >> > - all addressing is relative to %rip as it is in ELF file, > >> > >> What are the several hundred base relocs in xen.efi doing then? Sure > >> some of them wouldn't really be needed, but I doubt that's true for > >> all of them. The first and foremost case of non-RIP-relative addressing > >> is data with static initializers pointing elsewhere in the image. These > >> need relocations applied to work. > >> > >> Once again - a fundamental criteria is whether your binary can be used > >> in place of the current xen.efi. I can't convince myself that you've > >> actually tried that out. At the very least I'd expect the static array in > >> PrintErrMesg() to present problems here. > > > > Ugh... You are right. I forgot about that. Sadly the problem applies to > > the EFI boot code in the xen.gz too. So, both things have to be fixed. > > At first sight it seems to me that we can leave relocs in the xen-syms > > and then attach them to the xen.mb.efi/xen.gz somehow. It would be nice > > to do that just only for the EFI boot code. Should not we put it in > > separate section then? Another question is the size of the .reloc section. > > We do not know it in advance. So, probably we should build the code in > > two steps as we do now. Or prealloc a static place and fill it later. > > This way we would have just one phase build. > > Any static allocation/reservation scheme is wasteful at first and eventually > not allocating/reserving enough space. Unless there's a way to reasonably > well estimate the size ahead of time, I'd be opposed to such a model. As I have the same concerns in regards to that. > to a separate section - sure, why not? Relocations are in a separate section > in xen.efi as well. I was thinking about separate section for EFI boot code itself, e.g. .text.efi. Then probably it will be much easier to identify and use/get relocs needed only for that code. > > Or another option. Add Xen mappings in the early EFI boot code. However, > > it seems crazy and may not work on all EFI implementations. Hmmm... > > Have to check the UEFI spec... > > I'm afraid I don't understand anyway what you think of here. All non-rip-relative addresses are in Xen address space. So, I was thinking about adding required mappings to avoid .reloc section requirement. Though UEFI spec may not allow such play with page tables before ExitBootServices() call. > > By the way, I am not sure why mkreloc is executed twice. Could you > > explain that? > > Its needed as many times as we link a binary, minus the very first time, > where stub (dummy) objects are used. I don't see how you think we > could get away with just one invocation. Are you thinking we could get > away with fewer linking steps? That would be nice. At least I will try... > - Link step 0 produces a binary without kallsyms table, but with all > symbols. Hence a symbol table generated from it will have the correct > number of entries and hence the correct size. > - Link step 1 produces a binary with kallsyms table taken from the > step 0 binary. This results in all addresses in the resulting binary now > being correct (no more code/data is going to be added), but the > symbol table itself is wrong (as coming from step 0). > - Link step 2 produces a binary with a _correct_ kallsyms table. > If we omitted relocations from step 1, we'd risk other addresses > changing in step 2 (maybe this is just a theoretical risk, but anyway). OK, thanks for explanation. Daniel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
[parent not found: <5B45F0CD0200007F03CB4495@prv1-mh.provo.novell.com>]
* Re: [PATCH v2 8/8] efi: drop original xen.efi code and build mechanism [not found] ` <5B45F0CD0200007F03CB4495@prv1-mh.provo.novell.com> @ 2018-07-11 12:33 ` Jan Beulich 2018-07-12 10:57 ` Daniel Kiper 0 siblings, 1 reply; 50+ messages in thread From: Jan Beulich @ 2018-07-11 12:33 UTC (permalink / raw) To: Daniel Kiper Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel >>> On 11.07.18 at 13:57, <daniel.kiper@oracle.com> wrote: > On Tue, Jul 10, 2018 at 08:05:56AM -0600, Jan Beulich wrote: >> >>> On 10.07.18 at 13:35, <daniel.kiper@oracle.com> wrote: >> > On Fri, Jul 06, 2018 at 09:16:38AM -0600, Jan Beulich wrote: >> >> >>> On 06.07.18 at 16:46, <daniel.kiper@oracle.com> wrote: >> >> > OK, xen.mb.efi does not need relocs because: >> >> > - we generate PE file from xen-syms file like we do with ELF output; >> >> > so, the code in the PE file is the same like in the ELF file; >> >> > hence, if ELF works why PE should not, >> >> > - all addressing is relative to %rip as it is in ELF file, >> >> >> >> What are the several hundred base relocs in xen.efi doing then? Sure >> >> some of them wouldn't really be needed, but I doubt that's true for >> >> all of them. The first and foremost case of non-RIP-relative addressing >> >> is data with static initializers pointing elsewhere in the image. These >> >> need relocations applied to work. >> >> >> >> Once again - a fundamental criteria is whether your binary can be used >> >> in place of the current xen.efi. I can't convince myself that you've >> >> actually tried that out. At the very least I'd expect the static array in >> >> PrintErrMesg() to present problems here. >> > >> > Ugh... You are right. I forgot about that. Sadly the problem applies to >> > the EFI boot code in the xen.gz too. So, both things have to be fixed. >> > At first sight it seems to me that we can leave relocs in the xen-syms >> > and then attach them to the xen.mb.efi/xen.gz somehow. It would be nice >> > to do that just only for the EFI boot code. Should not we put it in >> > separate section then? Another question is the size of the .reloc section. >> > We do not know it in advance. So, probably we should build the code in >> > two steps as we do now. Or prealloc a static place and fill it later. >> > This way we would have just one phase build. >> >> Any static allocation/reservation scheme is wasteful at first and eventually >> not allocating/reserving enough space. Unless there's a way to reasonably >> well estimate the size ahead of time, I'd be opposed to such a model. As > > I have the same concerns in regards to that. > >> to a separate section - sure, why not? Relocations are in a separate section >> in xen.efi as well. > > I was thinking about separate section for EFI boot code itself, e.g. .text.efi. > Then probably it will be much easier to identify and use/get relocs needed only > for that code. How would you constrain which other code can be called from code in this section? While things like memcpy() won't need relocations, there would be no separation between code that can and be called here and code that must not be. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 8/8] efi: drop original xen.efi code and build mechanism 2018-07-11 12:33 ` Jan Beulich @ 2018-07-12 10:57 ` Daniel Kiper 0 siblings, 0 replies; 50+ messages in thread From: Daniel Kiper @ 2018-07-12 10:57 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel On Wed, Jul 11, 2018 at 06:33:15AM -0600, Jan Beulich wrote: > >>> On 11.07.18 at 13:57, <daniel.kiper@oracle.com> wrote: > > On Tue, Jul 10, 2018 at 08:05:56AM -0600, Jan Beulich wrote: > >> >>> On 10.07.18 at 13:35, <daniel.kiper@oracle.com> wrote: > >> > On Fri, Jul 06, 2018 at 09:16:38AM -0600, Jan Beulich wrote: > >> >> >>> On 06.07.18 at 16:46, <daniel.kiper@oracle.com> wrote: > >> >> > OK, xen.mb.efi does not need relocs because: > >> >> > - we generate PE file from xen-syms file like we do with ELF output; > >> >> > so, the code in the PE file is the same like in the ELF file; > >> >> > hence, if ELF works why PE should not, > >> >> > - all addressing is relative to %rip as it is in ELF file, > >> >> > >> >> What are the several hundred base relocs in xen.efi doing then? Sure > >> >> some of them wouldn't really be needed, but I doubt that's true for > >> >> all of them. The first and foremost case of non-RIP-relative addressing > >> >> is data with static initializers pointing elsewhere in the image. These > >> >> need relocations applied to work. > >> >> > >> >> Once again - a fundamental criteria is whether your binary can be used > >> >> in place of the current xen.efi. I can't convince myself that you've > >> >> actually tried that out. At the very least I'd expect the static array in > >> >> PrintErrMesg() to present problems here. > >> > > >> > Ugh... You are right. I forgot about that. Sadly the problem applies to > >> > the EFI boot code in the xen.gz too. So, both things have to be fixed. > >> > At first sight it seems to me that we can leave relocs in the xen-syms > >> > and then attach them to the xen.mb.efi/xen.gz somehow. It would be nice > >> > to do that just only for the EFI boot code. Should not we put it in > >> > separate section then? Another question is the size of the .reloc section. > >> > We do not know it in advance. So, probably we should build the code in > >> > two steps as we do now. Or prealloc a static place and fill it later. > >> > This way we would have just one phase build. > >> > >> Any static allocation/reservation scheme is wasteful at first and eventually > >> not allocating/reserving enough space. Unless there's a way to reasonably > >> well estimate the size ahead of time, I'd be opposed to such a model. As > > > > I have the same concerns in regards to that. > > > >> to a separate section - sure, why not? Relocations are in a separate section > >> in xen.efi as well. > > > > I was thinking about separate section for EFI boot code itself, e.g. .text.efi. > > Then probably it will be much easier to identify and use/get relocs needed only > > for that code. > > How would you constrain which other code can be called from code in this > section? While things like memcpy() won't need relocations, there would be > no separation between code that can and be called here and code that > must not be. I think that it is difficult or even impossible. However, the EFI boot code is quite well self contained, so, there is chance that this way we can reduce the number of generated relocs. Daniel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 50+ messages in thread
end of thread, other threads:[~2018-07-12 11:50 UTC | newest]
Thread overview: 50+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-19 14:35 [PATCH v2 0/8] Change xen.efi build and add SHIM_LOCK verification into efi_multiboot2() Daniel Kiper
2018-06-19 14:35 ` [PATCH v2 1/8] xen: calculate XEN_BUILD_TIME using XEN_BUILD_DATE value Daniel Kiper
2018-06-25 13:48 ` Jan Beulich
2018-07-04 12:06 ` Daniel Kiper
2018-07-04 13:58 ` Ian Jackson
2018-07-04 14:39 ` Daniel Kiper
2018-07-04 15:41 ` Ian Jackson
2018-07-04 15:51 ` Roger Pau Monné
2018-07-04 16:07 ` Daniel Kiper
2018-07-04 16:30 ` Ian Jackson
2018-07-04 16:55 ` Daniel Kiper
2018-06-19 14:35 ` [PATCH v2 2/8] xen: introduce XEN_COMPILE_POSIX_TIME Daniel Kiper
2018-06-25 13:54 ` Jan Beulich
2018-06-25 14:00 ` Andrew Cooper
2018-07-04 12:19 ` Daniel Kiper
2018-06-19 14:35 ` [PATCH v2 3/8] xen/x86: manually build xen.mb.efi binary Daniel Kiper
2018-06-25 15:36 ` Jan Beulich
2018-07-04 14:01 ` Daniel Kiper
2018-07-04 15:27 ` Jan Beulich
2018-07-04 16:35 ` Daniel Kiper
2018-07-05 8:18 ` Jan Beulich
2018-07-06 14:02 ` Daniel Kiper
2018-07-06 15:08 ` Jan Beulich
2018-07-10 10:48 ` Daniel Kiper
2018-07-10 13:54 ` Jan Beulich
2018-07-11 11:41 ` Daniel Kiper
[not found] ` <5B45ECF8020000FA04BD1FEB@prv1-mh.provo.novell.com>
2018-07-11 12:26 ` Jan Beulich
2018-07-12 10:52 ` Daniel Kiper
[not found] ` <5B4733110200007F03CC3F14@prv1-mh.provo.novell.com>
2018-07-12 11:50 ` Jan Beulich
2018-06-19 14:35 ` [PATCH v2 4/8] xen/x86: add some addresses to the Multiboot header Daniel Kiper
2018-06-28 13:41 ` Jan Beulich
2018-06-19 14:35 ` [PATCH v2 5/8] xen/x86: add some addresses to the Multiboot2 header Daniel Kiper
2018-06-28 13:42 ` Jan Beulich
2018-06-19 14:35 ` [PATCH v2 6/8] efi: split out efi_shim_lock() Daniel Kiper
2018-06-28 13:43 ` Jan Beulich
2018-06-19 14:35 ` [PATCH v2 7/8] xen/x86/efi: Verify dom0 kernel with SHIM_LOCK protocol in efi_multiboot2() Daniel Kiper
2018-06-28 13:48 ` Jan Beulich
2018-06-19 14:35 ` [PATCH v2 8/8] efi: drop original xen.efi code and build mechanism Daniel Kiper
2018-06-28 13:51 ` Jan Beulich
2018-07-04 14:25 ` Daniel Kiper
2018-07-04 15:34 ` Jan Beulich
2018-07-04 16:48 ` Daniel Kiper
2018-07-05 8:35 ` Jan Beulich
2018-07-06 14:46 ` Daniel Kiper
2018-07-06 15:16 ` Jan Beulich
2018-07-10 11:35 ` Daniel Kiper
2018-07-10 14:05 ` Jan Beulich
2018-07-11 11:57 ` Daniel Kiper
[not found] ` <5B45F0CD0200007F03CB4495@prv1-mh.provo.novell.com>
2018-07-11 12:33 ` Jan Beulich
2018-07-12 10:57 ` Daniel Kiper
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.