On Fri, 7 Jul 2023 at 19:12, Borislav Petkov <bp@xxxxxxxxx> wrote: > > On Fri, Jul 07, 2023 at 10:25:15AM -0500, Michael Roth wrote: > > ... > > It would be unfortunate if we finally abandoned this path because of the > > issue being hit here though. I think the patch posted here is the proper > > resolution to the issue being hit, and I'm hoping at this point we've > > identified all the similar cases where EFI/setup_data-related structures > > were missing explicit mappings. But if we still think it's too much of a > > liability to access the EFI config table outside of SEV-enabled guests, > > then I can work on re-implementing things based on the above logic. > > Replying here to Tom's note too... > > So, I like the idea of rechecking CPUID. Yes, let's do the sev_status > check. As a result, we either fail the guest - no problem - or we boot > and we recheck. Thus, we don't run AMD code on !AMD machines, if the HV > is not a lying bastard. > > Now, if we've gotten a valid setup_data SETUP_EFI entry with a valid > pointer to an EFI config table, then that should happen in the generic > path - initialize_identity_maps(), for example - like you've done in > b57feed2cc26 - not in the kexec code because kexec *happens* to need it. > > We want to access the EFI config table? Sure, by all means, but make > that generic for all code. > OK, so in summary, what seems to be happening here is that the SEV init code in the decompressor looks for the cc blob table before the on-demand mapping code is up, which normally ensures that any RAM address is accessible even if it hasn't been mapped explicitly. This is why the fix happens to work: the code only maps the array of (guid, phys_addr) tuples that describes the list of configuration tables that have been provided by the firmware. The actual configuration tables themselves could be anywhere in physical memory, and without prior knowledge of a particular GUID value, there is no way to know the size of the table, and so they cannot be mapped upfront like this. However, the cc blob table does not exist on this machine, and so whether the EFI config tables themselves are mapped or not is irrelevant. But it does mean the fix is incomplete, and certainly does not belong in generic kexec code. If anything, we should be fixing the decompressor code to defer the cc blob table check until after the demand mapping code is up. If this is problematic, we might instead disable SEV for kexec, and rely on the fact that SEV firmware enters with a complete 1:1 map (as we seem to be doing currently). If kexec for SEV is needed at some point, we can re-enable it by having it provide a mapping for the config table array and the cc blob table explicitly.
