Re: [PATCH v7 1/3] efi/x86: Fix EFI memory map corruption with kexec

["Kalra, Ashish" <ashish.kalra@amd.com>]

Re-sending this, last response got garbled.

On 6/3/2024 11:48 AM, Kalra, Ashish wrote:
> On 6/3/2024 10:31 AM, Mike Rapoport wrote:
>
>> On Mon, Jun 03, 2024 at 04:46:39PM +0200, Borislav Petkov wrote:
>>> On Mon, Jun 03, 2024 at 09:01:49AM -0500, Kalra, Ashish wrote:
>>>> If we skip efi_arch_mem_reserve() (which should probably be anyway 
>>>> skipped
>>>> for kexec case), then for kexec boot, EFI memmap is memremapped in 
>>>> the same
>>>> virtual address as the first kernel and not the allocated memblock 
>>>> address.
>>> Are you saying that we should simply do
>>>
>>> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
>>> index fdf07dd6f459..410cb0743289 100644
>>> --- a/drivers/firmware/efi/efi.c
>>> +++ b/drivers/firmware/efi/efi.c
>>> @@ -577,6 +577,9 @@ void __init efi_mem_reserve(phys_addr_t addr, 
>>> u64 size)
>>>       if (WARN_ON_ONCE(efi_enabled(EFI_PARAVIRT)))
>>>           return;
>>>   +    if (kexec_in_progress)
>>> +        return;
>>> +
>>>       if (!memblock_is_region_reserved(addr, size))
>>>           memblock_reserve(addr, size);
>>>   and skip that whole call?
>> I think Ashish suggested rather
>>
>> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
>> index fdf07dd6f459..eccc10ab15a4 100644
>> --- a/drivers/firmware/efi/efi.c
>> +++ b/drivers/firmware/efi/efi.c
>> @@ -580,6 +580,9 @@ void __init efi_mem_reserve(phys_addr_t addr, u64 
>> size)
>>       if (!memblock_is_region_reserved(addr, size))
>>           memblock_reserve(addr, size);
>>   +    if (kexec_in_progress)
>> +        return;
>> +
>>       /*
>>        * Some architectures (x86) reserve all boot services ranges
>>        * until efi_free_boot_services() because of buggy firmware
> Yes, something similar as above, as efi_mem_reserve() is used to 
> reserve boot service memory and is not necessary for kexec boot.
>
> So, Dave Young (dyoung@redhat.com) had suggested that we skip 
> efi_arch_mem_reserve() for kexec by checking the set 
> EFI_MEMORY_RUNTIME attribute as below:
>
diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index f0cc00032751..6f398c59278a 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -255,15 +255,39 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, 
u64 size)
         struct efi_memory_map_data data = { 0 };
         struct efi_mem_range mr;
         efi_memory_desc_t md;
-       int num_entries;
+       int num_entries, ret;
         void *new;

-       if (efi_mem_desc_lookup(addr, &md) ||
-           md.type != EFI_BOOT_SERVICES_DATA) {
+       /*
+        * efi_mem_reserve() is used to reserve boot service memory, eg. 
bgrt,
+        * but it is not neccasery for kexec, as there are no boot 
services in
+        * kexec reboot at all after the first kernel's ExitBootServices().
+        *
+        * Therefore, skip efi_mem_reserve for kexec booting by checking the
+        * EFI_MEMORY_RUNTIME attribute which indicates boot service memory
+        * ranges reserved by the first kernel using efi_mem_reserve and 
marked
+        * with EFI_MEMORY_RUNTIME attribute.
+        */
+
+       ret = efi_mem_desc_lookup(addr, &md);

+       if (ret) {

                 pr_err("Failed to lookup EFI memory descriptor for 
%pa\n", &addr);
                 return;
         }

+       if (md.type != EFI_BOOT_SERVICES_DATA) {
+               pr_err("Skip reserving non EFI Boot Service Data memory 
for %pa\n", &addr);
+               return;
+       }
+
+       /* Kexec copied the efi memmap from the first kernel, thus skip 
the case */
+       if (md.attribute & EFI_MEMORY_RUNTIME)
+               return;
+
         if (addr + size > md.phys_addr + (md.num_pages << 
EFI_PAGE_SHIFT)) {
                 pr_err("Region spans EFI memory descriptors, %pa\n", 
&addr);
                 return;

Thanks, Ashish