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

From: Jan Beulich <jbeulich@suse.com>
To: Sergei Temerkhanov <s.temerkhanov@gmail.com>
Cc: xen-devel@lists.xenproject.org
Subject: Re: [PATCH] efi: Always map EfiRuntimeServicesCode and EfiRuntimeServicesData
Date: Mon, 7 Sep 2020 08:16:07 +0200	[thread overview]
Message-ID: <3af10c84-8e97-8932-02fc-4bc654e31a39@suse.com> (raw)
In-Reply-To: <CAPEA6db-gNWhMU=Ex4OLFEB0HcFAy5GFs6Cjc6a4wupEpBReKw@mail.gmail.com>

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