diff mbox series

efi: Always map EfiRuntimeServicesCode and EfiRuntimeServicesData

Message ID 20200903232458.16551-1-s.temerkhanov@gmail.com
State Superseded
Headers show
Series efi: Always map EfiRuntimeServicesCode and EfiRuntimeServicesData | expand

Commit Message

Sergey Temerkhanov Sept. 3, 2020, 11:24 p.m. UTC
This helps overcome problems observed with some UEFI implementations
which don't set the Attributes field in memery descriptors properly

Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
---
 xen/common/efi/boot.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Jan Beulich Sept. 4, 2020, 9:47 a.m. UTC | #1
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.

>          }
>  
>          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.

Jan
Sergey Temerkhanov Sept. 4, 2020, 9:03 p.m. UTC | #2
On Fri, Sep 4, 2020 at 12:47 PM Jan Beulich <jbeulich@suse.com> 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
Jan Beulich Sept. 7, 2020, 6:16 a.m. UTC | #3
On 04.09.2020 23:03, Sergei Temerkhanov wrote:
> On Fri, Sep 4, 2020 at 12:47 PM Jan Beulich <jbeulich@suse.com> 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.

That's definitely not sufficient for a patch submission, or - if
you absolutely can't work with master / staging for some reason -
should be explicitly pointed out in the submission.

>>
>>>          }
>>>
>>>          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.

Please don't: I intentionally said "check", not "correct".
Unless of course you have proof of both aspects being got wrong
on a single piece of firmware at the same time.

Jan
diff mbox series

Patch

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 5a520bf21d..4644ce2525 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1521,7 +1521,9 @@  void __init efi_init_memory(void)
         }
 
         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))) )