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

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

Le 04/01/2020 à 03:33, Emmanuel Dreyfus a écrit :
> On Tue, Dec 31, 2019 at 09:32:05AM +0100, Maxime Villard wrote:
> > I think max-page-size=0x1000 is the right thing to do, but someone needs to
> > verify that the resulting binary is correct and that the resulting in-memory
> > layout is correct too.
>
> Attached is an updated patch with this approach. I tested at mine and
> it seems fine.

Come on... "I tested and it seems fine"... Whatever.

I have now verified that the resulting binary layout is correct, that
the in-memory layout is correct, and that libsa doesn't do crazy things
with this binary. As far as my concerns were concerned, the max-page-size
change is good to go.

The rest is confused:

+.section multiboot,"",@note

Why @note? It will be in the .text anyway. Also why no dot in the section
name? That's supposed to be the naming convention.

+EXTRA_LINKFLAGS=	--split-by-file=0x100000 -z max-page-size=0x1000 -r -d

KASLR kernels to not have a PHDR, so this is not relevant -- there was a
reason this parameter wasn't getting passed in the first place.

+options 	MULTIBOOT	# Multiboot support (see multiboot(8))

As said repeatedly, the option should be enabled only _after_ the garbage
has been cleaned up.

In fact, why don't you revert your change, fix it correctly locally, and
then re-submit it? I don't know if you realize, but you landed a huge pile
of crap in the middle of the amd64 locore, not only does this crap not
work but it also breaks EFI boot, and for two weeks you've been wondering
what's wrong in it and you have consistently proposed absurd workarounds.

Please revert your change entirely and put back the code in a clean and
functional state. Thanks.

Maxime