Re: CVS commit: src/sys/arch/amd64
On Fri, Dec 27, 2019 at 06:24:07PM +0100, Maxime Villard wrote: > Now that I'm looking at i386 I see you've indeed made the same nonsensical > changes there, with all the unnecessary garbage in the code. Here I assume you refer to the starting at efi_multiboot2_loader, since most of the other significant multiboot stuff has been there for 13 years. It is copied from bootloader's startprog.S. How do you suggest to improve it? -- Emmanuel Dreyfus m...@netbsd.org
Re: CVS commit: src/sys/arch/amd64
On Fri, Dec 27, 2019 at 06:24:07PM +0100, Maxime Villard wrote:
> .text : AT (ADDR(.text) & 0x0fff)
> {
> + *(.multiboot)
> +
> . = ALIGN(__PAGE_SIZE);
> __text_user_start = . ;
> ...
>
> This guarantees that the structure is at the beginning of text.
That works. We can even make the multiboot section a note, for the sake
on cleanliness. (see attached patch. MULTIBOOT is enabled for testing).
> Regardless of whether it is needed in this specific case, cutting the 2MBs
> of zero in the binary is wanted. Unfortunately last I looked at this (two
> years ago) there were some non-obvious consequences, and it needs to be
> carefully done.
Any hints about the problems you encountered? Perhaps we can work it
around with an . = ALIGN(__LARGEE_PAGE_SIZE); before including .text.user ?
> Also, my previous remarks haven't been addressed entirely, and still stand.
Sure, it's just next in the todo list.
--
Emmanuel Dreyfus
m...@netbsd.org
? sys/arch/amd64/compile/obj
? sys/arch/amd64/stand/prekern/obj
Index: sys/arch/amd64/amd64/locore.S
===
RCS file: /cvsroot/src/sys/arch/amd64/amd64/locore.S,v
retrieving revision 1.195
diff -U4 -r1.195 locore.S
--- sys/arch/amd64/amd64/locore.S 15 Dec 2019 02:58:21 - 1.195
+++ sys/arch/amd64/amd64/locore.S 28 Dec 2019 01:41:03 -
@@ -431,10 +431,10 @@
.size tmpstk, tmpstk - .
.space 512
tmpstk:
-.section multiboot,"a"
#if defined(MULTIBOOT)
+.section multiboot,"",@note
.align 8
.globl Multiboot2_Header
_C_LABEL(Multiboot2_Header):
.intMULTIBOOT2_HEADER_MAGIC
Index: sys/arch/amd64/conf/GENERIC
===
RCS file: /cvsroot/src/sys/arch/amd64/conf/GENERIC,v
retrieving revision 1.551
diff -U4 -r1.551 GENERIC
--- sys/arch/amd64/conf/GENERIC 14 Dec 2019 07:45:20 - 1.551
+++ sys/arch/amd64/conf/GENERIC 28 Dec 2019 01:41:03 -
@@ -25,9 +25,9 @@
#ident "GENERIC-$Revision: 1.551 $"
maxusers 64 # estimated number of users
-#options MULTIBOOT # Multiboot support (see multiboot(8))
+optionsMULTIBOOT # Multiboot support (see multiboot(8))
# delay between "rebooting ..." message and hardware reset, in milliseconds
#options CPURESET_DELAY=2000
Index: sys/arch/amd64/conf/Makefile.amd64
===
RCS file: /cvsroot/src/sys/arch/amd64/conf/Makefile.amd64,v
retrieving revision 1.80
diff -U4 -r1.80 Makefile.amd64
--- sys/arch/amd64/conf/Makefile.amd64 14 Nov 2019 16:23:52 - 1.80
+++ sys/arch/amd64/conf/Makefile.amd64 28 Dec 2019 01:41:03 -
@@ -90,12 +90,12 @@
## (5) link settings
##
TEXTADDR?= 0x8020
.if defined(KASLR)
-EXTRA_LINKFLAGS= --split-by-file=0x10 -r -d
+EXTRA_LINKFLAGS= --split-by-file=0x10 -r -d -n
KERNLDSCRIPT?= ${AMD64}/conf/kern.ldscript.kaslr
.else
-EXTRA_LINKFLAGS= -z max-page-size=0x20
+EXTRA_LINKFLAGS= -z max-page-size=0x20 -n
KERNLDSCRIPT?= ${AMD64}/conf/kern.ldscript
.endif
LINKFLAGS_NORMAL= -X
Index: sys/arch/amd64/conf/kern.ldscript
===
RCS file: /cvsroot/src/sys/arch/amd64/conf/kern.ldscript,v
retrieving revision 1.30
diff -U4 -r1.30 kern.ldscript
--- sys/arch/amd64/conf/kern.ldscript 15 Dec 2019 02:56:40 - 1.30
+++ sys/arch/amd64/conf/kern.ldscript 28 Dec 2019 01:41:03 -
@@ -12,20 +12,11 @@
ENTRY(_start)
SECTIONS
{
- /*
-* multiboot (file_offset) : AT (load_address)
-* file_offset must be below 32k for multiboot 2 specification
-* BIOS boot requires load_address above 0x20
-*/
- multiboot 0x1000 : AT (0x20)
+ .text : AT (ADDR(.text) & 0x0fff)
{
- . = ALIGN(8);
KEEP(*(multiboot));
- }
- .text : AT (0x20 + SIZEOF(multiboot))
- {
. = ALIGN(__PAGE_SIZE);
__text_user_start = . ;
*(.text.user)
. = ALIGN(__PAGE_SIZE);
Re: CVS commit: src/usr.sbin/fstyp
Hi tkusumi, Thanks for the work you are doing. On 27/12/2019 11:06, Tomohiro Kusumi wrote: taken-from: FreeBSD (freebsd/freebsd@b4d7ad9f787e74e712423def67de8bd76f71943a) One minor nit, could you please cite the revision in the svn repo which is the source of truth and not the readonly git mirror, this is in case something happens to the mirror and hashes end up changing in the future. Sevan
Re: CVS commit: src/sys/arch/amd64
Le 27/12/2019 à 17:45, Emmanuel Dreyfus a écrit :
> On Fri, Dec 27, 2019 at 09:02:17AM +0100, Maxime Villard wrote:
>> Please stop with the nonsense... In this patch you are making the multiboot
>> header executable, and putting it in a section shared with userland under
>> SVS. Neither should be required; more than that, both are absolutely _not_
>> wanted.
>
> What are the actual drawbacks?
You are moving the structure to an area where it does not belong _at all_.
We don't want to map things in userland for no reason.
Now that I'm looking at the thing closely, I see why we are forced to make
it executable. So, that's fine for that part.
> FWIW, this is in line with how it was done on i386: it is just stored
> at the beginning of .text. Xen does the same. Of course it seems more
> natural to store that in a note section this is not loaded, but after
> experimenting a lot, I am not sure it can be done, since ld really
> want to push notes at the end of the file.
Now that I'm looking at i386 I see you've indeed made the same nonsensical
changes there, with all the unnecessary garbage in the code.
To me, you only need
.section .multiboot,"ax",@progbits
and
.text : AT (ADDR(.text) & 0x0fff)
{
+ *(.multiboot)
+
. = ALIGN(__PAGE_SIZE);
__text_user_start = . ;
...
This guarantees that the structure is at the beginning of text.
Regardless of whether it is needed in this specific case, cutting the 2MBs
of zero in the binary is wanted. Unfortunately last I looked at this (two
years ago) there were some non-obvious consequences, and it needs to be
carefully done.
Also, my previous remarks haven't been addressed entirely, and still stand.
Maxime
Re: CVS commit: src/sys/arch/amd64
On Fri, Dec 27, 2019 at 09:02:17AM +0100, Maxime Villard wrote: > Please stop with the nonsense... In this patch you are making the multiboot > header executable, and putting it in a section shared with userland under > SVS. Neither should be required; more than that, both are absolutely _not_ > wanted. What are the actual drawbacks? FWIW, this is in line with how it was done on i386: it is just stored at the beginning of .text. Xen does the same. Of course it seems more natural to store that in a note section this is not loaded, but after experimenting a lot, I am not sure it can be done, since ld really want to push notes at the end of the file. -- Emmanuel Dreyfus m...@netbsd.org
Re: CVS commit: src/sys/arch/amd64
Hi, Your patch works fine for my laptop too. Thank you. Masanobu SAITOH writes: > On 2019/12/27 1:55, Emmanuel Dreyfus wrote: >> On Wed, Dec 25, 2019 at 05:05:11PM +0900, Masanobu SAITOH wrote: > After this change, amd64 kernel does not boot on my HP Spectre x360 > 13-inch ae019TU laptop with pure UEFI boot mode. >>> I have a UEFI boot machine and it also doesn't boot well. >> >> Please try the attached patch. >> >> It adds the -n flag to ld, which disable auto-alignment of sections >> in the file. I undestand alignement is highly desirable for userland >> programs that may be mapped from file, but useless for the kernel, >> which is just readen once by the bootloader. >> >> Without auto-alignement, the .text segment starts right after the >> ELF headers. This means the multiboot header can go in .text and >> stay below 32k (as required by the multiboot specification). There >> is no need for a multiboot section for that, and therefore no >> need to modify the linker script. >> >> A side effect is that the kernel file shrinks of 2 MB, because there >> is not an alignement hole between ELF headers and the .text section >> anymore. >> >> My patch also enable the MULTIBOOT option so that we can check >> nothing gets broken with it. You can also try with the option >> disabled, of course. >> > > Both with and without MULTIBOOT works fine. No any hangup/panic. > > Thanks. > > -- > --- > SAITOH Masanobu (msai...@execsw.org > msai...@netbsd.org) -- Ryo ONODERA // r...@tetera.org PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB FD1B F404 27FA C7D1 15F3
Re: CVS commit: src/sys/arch/amd64
Le 26/12/2019 à 17:55, Emmanuel Dreyfus a écrit : > On Wed, Dec 25, 2019 at 05:05:11PM +0900, Masanobu SAITOH wrote: After this change, amd64 kernel does not boot on my HP Spectre x360 13-inch ae019TU laptop with pure UEFI boot mode. >> I have a UEFI boot machine and it also doesn't boot well. > > Please try the attached patch. > > It adds the -n flag to ld, which disable auto-alignment of sections > in the file. I undestand alignement is highly desirable for userland > programs that may be mapped from file, but useless for the kernel, > which is just readen once by the bootloader. > > Without auto-alignement, the .text segment starts right after the > ELF headers. This means the multiboot header can go in .text and > stay below 32k (as required by the multiboot specification). There > is no need for a multiboot section for that, and therefore no > need to modify the linker script. > > A side effect is that the kernel file shrinks of 2 MB, because there > is not an alignement hole between ELF headers and the .text section > anymore. > > My patch also enable the MULTIBOOT option so that we can check > nothing gets broken with it. You can also try with the option > disabled, of course. Please stop with the nonsense... In this patch you are making the multiboot header executable, and putting it in a section shared with userland under SVS. Neither should be required; more than that, both are absolutely _not_ wanted. Instead of trying to patch-work the thing over and over, you should probably revert it all, take an hour to peacefully write it correctly, and then submit it again. Given the way multiboot was written so far I don't see how we can accept to enable it by default. Maxime
