RE: [PATCH V3 3/5] x86/efi: Permanently save the EFI_MEMORY_MAP passed by the firmware

[Prakhya, Sai Praneeth]

> >> I agree. Keep it simple. If the EFI crap fails, then assist with the
> >> reboot and otherwise just kill it.
> >
> > The reasons for saving old memory map are (in my view, these are the
> > less important ones because they are very unlikely to happen)
> >
> > 1. Make sure that a memory descriptor exists for the physical address
> > that was faulted on (EFI Memory Map could sometime have holes).
> > Assuming a case that the physical address that caused page fault
> > doesn't have a valid efi memory descriptor, the efi page fault handler shouldn't
> take any action because it hasn't triaged the problem yet.
> >
> > 2. Make sure that the faulted physical address is _not_ efi runtime service
> code/data region.
> > Efi runtime service code/data regions are always mapped by kernel in
> > efi_pgd and accesses to these regions should _never_ page fault.
> > Assuming that something like this happens, efi page fault handler
> > shouldn't take any action because it's not any illegal access by firmware but it's
> a kernel bug.
> >
> 
> What about attempts to modify code regions or attempts to execute data
> regions? What kind of fault will that trigger, and are they being handled at the
> moment?

AFAIK, at least in the x86 world, attempts to write to read only regions or attempts 
to execute XP (execute protected) pages will result in page fault and I don't think 
we are handling them.

> 
> As I pointed out, EFI runtime services code may legally access the stack or
> dereference pointer arguments, but could still contain bugs that result in out of
> bounds accesses or writes to read-only regions.

Yes, agreed. In fact, I did see these bugs.

> So I don't really care about the address of the illegal access, any fault that
> occurs while running in the firmware should be treated the same.

Ok.. makes sense.

> In fact, cross
> referencing the value of IP with RuntimeServicesCode regions may be more
> useful

This is to verify that firmware is indeed executing code from RuntimeServicesCode
regions when it faulted. Is that correct? Or did you mean something else?

> > That said, a more important reason (in my view) is to print out the
> > memory descriptor that we faulted on. This is a *proof* showing that
> > it's buggy firmware that caused page fault and hence is not a kernel
> > bug. This proof is important because whenever a stack trace is printed
> > with some efi function, kernel is the usual suspect and hence we need to show
> that it's not kernel fault. It could also help firmware engineers to fix the bug
> easily.
> >
> > dmesg would show something like this when buggy efi_reset_system()
> accesses reserved region:
> >
> > [  296.141511] efi: EFI Memory Descriptor for offending PA is:
> > [  296.141844] efi: [Reserved           |   |  |  |  |  |  |  |   |WB|WT|WC|UC]
> range=[0x000000007e915000-0x000000007e933fff] (0MB)
> > [  296.142522] efi: efi_reset_system() buggy! Reboot through BIOS
> >
> > So, I would be concerned if we miss this proof.
> >
> 
> You can dump the entire memory map by putting efi=debug on the kernel
> command line, so all we need to do is report the physical address, and you can
> easily figure out for yourself which memory map entry covers it.

That's true. In fact, that's how I debugged this issue and hence thought that it might be 
useful to have all that info at one place (i.e. in efi page fault handler).
But, as you said, to make the code look simpler, I will roll out a V4 without saving 
original memory map.

Regards,
Sai