Re: [RFT PATCH v3 00/21] x86: strict separation of startup code

[Ingo Molnar <mingo@kernel.org>]

* Ard Biesheuvel <ardb@kernel.org> wrote:

> On Tue, 13 May 2025 at 15:17, Borislav Petkov <bp@alien8.de> wrote:
> >
> > On Tue, May 13, 2025 at 01:22:16PM +0200, Ingo Molnar wrote:
> ...
> > > Note that two of those fixes were from Ard who is working on further
> > > robustifying the startup code - a much needed change.
> >
> > Really? Much needed huh?
> >
> > Please do explain why is it much needed?
> >
> > Because the reason Ard is doing it is a different one but maybe
> > I misunderstood him...
> >
> 
> I will refrain from inserting myself into the intra-tip review and
> testing policy debate, but let me at least provide a quick recap of
> what I am doing here and why.
> 
> Since commit
> 
>    c88d71508e36 x86/boot/64: Rewrite startup_64() in C
> 
> dated Jun 6 2017, we have been using C code on the boot path in a way
> that is not supported by the toolchain, i.e., to execute non-PIC C
> code from a mapping of memory that is different from the one provided
> to the linker. It should have been obvious at the time that this was a
> bad idea, given the need to sprinkle fixup_pointer() calls left and
> right to manipulate global variables (including non-pointer variables)
> without crashing.
> 
> This C startup code has been expanding, and in particular, the SEV-SNP
> startup code has been expanding over the past couple of years, and
> grown many of these warts, where the C code needs to use special
> annotations or helpers to access global objects.
> 
> Google uses Clang internally, and as expected, it does not behave
> quite like GCC in this regard either. The result is that the SEV-SNP
> boot tended to break in cryptic ways with Clang built kernels, due to
> absolute references in the startup code that runs before those
> absolute references are mapped.
> 
> I've done a preliminary pass upstream with RIP_REL_REF() and
> rip_rel_ptr() and the use of the .head.text section for startup code
> to ensure that we detect such issues at build time, and it has already
> resulted in a number of true positives where the code in question
> would have failed at boot time. At this point, I'm not aware of any
> issues caused by absolute references going undetected.
> 
> However, Linus kindly indicated that the resulting diagnostics
> produced by the relocs tool do not meet his high standards, and so I
> proposed another approach, which I am implementing now (see cover
> letter for details). Note that this approach is also much more robust,
> as annotating things as __head by hand to get it emitted into the
> section to which the diagnostics are applied is obviously not
> foolproof.

Exactly.

> Fixing the existing 5-level paging and kernel mapping code was rather 
> straight-forward. However, splitting up the SEV-SNP code has been 
> rather challenging due to the way it was put together, i.e., as a 
> single source file used everywhere, and to which additional 
> functionality has been added piecemeal (i.e., the SVSM support).

Yeah.

> It is obvious that these changes should be tested before being merged,
> hence the RFT in the subject. And I have been struggling a bit to get
> access to usable hardware. (I do have access to internal development
> systems, but those do not fit the 'usable' description by any measure,
> given that I have to go through the cloud VM orchestration APIs to
> test boot a simple kernel image).

:-/ This is one of the reasons why bugs have such long latencies here.

For example it appears nobody has run kdump on SEV-SNP since last 
August:

  d2062cc1b1c3 x86/sev: Do not touch VMSA pages during SNP guest memory kdump

  ...
    It then results in unrecoverable #NPF/RMP faults as the VMSA page is
    marked busy/in-use when the vCPU is running and subsequently a causes
    guest softlockup/hang.
  ...

  1 file changed, 158 insertions(+), 86 deletions(-)

Yet lack of testability of your SEV-SNP series is still somehow 
blocking ongoing development work.

> What Boris might allude to is the fact that some of these changes also
> form a prerequisite for being able to construct a generic EFI zboot
> image for x86, which is a long-term objective that I am working on in
> the background. But this is not the main reason.
> 
> In any case, there is no urgency wrt these changes as far as I am
> concerned, and given that I already found an issue myself with v3,
> perhaps it is better if we disregard it for the time being, and we can
> come back to it for the next cycle. In the mean time, I can compare
> notes with Boris and Tom directly to ensure that this is in the right
> shape, and perhaps we could at least fix the pgtable_l5_enabled() mess
> as well (for which I sent out a RFC/v3 today).

You are being exceedingly generous here, but obviously boot code 
changes need quite a bit of testing, and v6.17 (or later) is perfectly 
fine too.

We could perhaps do the mechanical code movement to 
arch/x86/boot/startup/ alone, without any of the followup functional 
changes. This would reduce the cross section of the riskiest part of 
your series substantially. If that sounds good to you, please send a 
series for review.

Thanks,

	Ingo