Re: CVS commit: src/sys/arch/amd64

[Maxime Villard <max%m00nbsd.net@localhost>]

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) & 0x0fffffff)
	{
+		*(.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