On Mon, 22 May 2023 at 14:48, Joerg Roedel <jroedel@xxxxxxx> wrote: > > On Fri, May 19, 2023 at 09:04:46AM -0500, Tom Lendacky wrote: > > Deferring the checks is probably the safest thing to do, since that would > > match the way things are done today and known to work. I'm not sure what > > other things might pop up if we stay with this approach, for example, page > > state change calls using the GHCB MSR protocol that also don't save/restore > > the MSR value. > > > > It is possible to audit these areas and stay with this approach, but I'm > > wondering if that wouldn't be better done as a separate patch series. > > > > Adding @Joerg for any additional thoughts he might have around this area, too. > > If I got it correctly the patch actually moves two things before > ExitBootServices: > > 1) SEV features check > > 2) SEV initialization > > I think it makes a lot of sense to have 1) before ExitBootServices. It > allows to soft-fail in case the kernel does not support all required > SEV-SNP features and move on to a kernel which does. This check also only > needs the SEV_STATUS MSR and not any GHCB calls. > > The problem is the GHCB protocol negotiation with the HV, but the GHCB > protocol is downward-compatible, so an older kernel can work with a > newer HV. > > But 2) needs to stay after ExitBootServices, as it needs resources owned > by UEFI, e.g. the GHCB MSR and potentially the configured GHCB itself. > Fiddling around with the GHCB MSR while it is still owned by UEFI will > bite us in one or the other way (e.g. UEFI, before ExitBootServices, is > free to take IRQs with handlers that rely on the GHCB MSR content). > Thanks for the insight. Note that I have sent a v3 in the mean time that moves all of this *after* ExitBootServices() [0], but I failed to cc you - apologies. So IIUC, we could just read sev_status much earlier just to perform the SNP feature check, and fail the boot gracefully on a mismatch. And the sev_enable() call needs to move after ExitBootServices(), right? That would result in only very minor duplication, afaict. I'll have a stab at implementing this for v4. Thanks, [0] https://lore.kernel.org/all/20230522071415.501717-21-ardb@xxxxxxxxxx/
