Re: [PATCH] efi: Always map EfiRuntimeServicesCode and EfiRuntimeServicesData

[Sergei Temerkhanov <s.temerkhanov@xxxxxxxxx>]

On Fri, Sep 4, 2020 at 12:47 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 04.09.2020 01:24, Sergey Temerkhanov wrote:
> > --- a/xen/common/efi/boot.c
> > +++ b/xen/common/efi/boot.c
> > @@ -1521,7 +1521,9 @@ void __init efi_init_memory(void)
>
> Looking at the line numbers - is this patch against the master
> or staging branch? I ask because about as far away from the line
> number above as the chunk of cose you mean to change there's a
> very similar conditional, which has caused some slight confusion
> over here.

it was the latest tag, AFAIR.

>
> >          }
> >
> >          if ( !efi_enabled(EFI_RS) ||
> > -             (!(desc->Attribute & EFI_MEMORY_RUNTIME) &&
> > +             ((!(desc->Attribute & EFI_MEMORY_RUNTIME) &&
> > +                (desc->Type != EfiRuntimeServicesCode &&
> > +                 desc->Type != EfiRuntimeServicesData)) &&
> >                (!map_bs ||
> >                 (desc->Type != EfiBootServicesCode &&
> >                  desc->Type != EfiBootServicesData))) )
>
> I'm in principle okay with a workaround like this, but I don't
> think it should go silently. I'd therefore like to suggest you
> add a new if() ahead of this one and then set
> EFI_MEMORY_RUNTIME in affected descriptors (to keep things
> consistent with other consumers of the memory map without
> having to update every one of those checking for the flag)
> alongside issuing a log message.
>
> There's nevertheless another piece of code you need to adjust,
> inside a CONFIG_EFI_SET_VIRTUAL_ADDRESS_MAP conditional in
> efi_exit_boot(). But you shouldn't adjust the descriptor
> there, yet - this should happen only after its logging in
> efi_init_memory().
>
> Additionally I'd like it to be at least considered to also
> check that EFI_MEMORY_WB (or at the very least one of the
> cachability flags) is set, so that we won't run into the
> path further down complaining about a lack thereof in this
> case.

Makes sense. I'm making it set the UC for data and WP for code as the most
conservative option in such a case.


>
> Jan