Received-SPF: pass (google.com: domain of linux-kernel+bounces-199483-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <1d03466a-052c-4152-9dc4-0b72d95cde6d@amd.com>
Date: Mon, 3 Jun 2024 12:05:45 -0500
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v7 1/3] efi/x86: Fix EFI memory map corruption with kexec
Content-Language: en-US
From: "Kalra, Ashish" <ashish.kalra@amd.com>
To: Mike Rapoport <rppt@kernel.org>, Borislav Petkov <bp@alien8.de>
Cc: tglx@linutronix.de, mingo@redhat.com, dave.hansen@linux.intel.com,
 x86@kernel.org, rafael@kernel.org, hpa@zytor.com, peterz@infradead.org,
 adrian.hunter@intel.com, sathyanarayanan.kuppuswamy@linux.intel.com,
 jun.nakajima@intel.com, rick.p.edgecombe@intel.com, thomas.lendacky@amd.com,
 michael.roth@amd.com, seanjc@google.com, kai.huang@intel.com,
 bhe@redhat.com, kirill.shutemov@linux.intel.com, bdas@redhat.com,
 vkuznets@redhat.com, dionnaglaze@google.com, anisinha@redhat.com,
 jroedel@suse.de, ardb@kernel.org, kexec@lists.infradead.org,
 linux-coco@lists.linux.dev, linux-kernel@vger.kernel.org,
 Dave Young <dyoung@redhat.com>
References: <20240528095522.509667-1-kirill.shutemov@linux.intel.com>
 <cover.1717111180.git.ashish.kalra@amd.com>
 <f4be03b8488665f56a1e5c6e6459f447352dfcf5.1717111180.git.ashish.kalra@amd.com>
 <20240603085654.GBZl2FVjPd-gagt-UA@fat_crate.local>
 <8e3dfc15-f609-4839-85c7-1cc8cefd7acc@amd.com> <Zl3HfiQ6oHdTdOdA@kernel.org>
 <1ef36309-8d7f-447b-a54a-3cdafeccca64@amd.com>
 <20240603144639.GCZl3XTwmFHwi-KUZW@fat_crate.local>
 <Zl3hwYL2GDiyUfGo@kernel.org> <7eb4ca99-679c-4f23-9562-a39b966c28a0@amd.com>
In-Reply-To: <7eb4ca99-679c-4f23-9562-a39b966c28a0@amd.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?UUk0RGlQWlNmY0dDUDVwMVZjaW1PbS8yMUZOdSs3dXZNMUp0RFk1eVlOZTFW?=
 =?utf-8?B?ck9RSXFrQjNLWUhOejBqQ1hTdmFYaHM0ZjNpSnJ6ZnhpV1RIRTVyK0dnTi9J?=
 =?utf-8?B?RVpKU1hzaDYxYUJmRU1iYWJuZnp0NWJQbEF6RFNvME5KSEpTK2I3am5JQURn?=
 =?utf-8?B?RERMVE5hV2lseXc1ZUZTdUpia1FDdlBJc2pXV3ljOXVtQmdTT2FDZTY2eXpy?=
 =?utf-8?B?WmdKbklpd0w5RCtsU0h6RjkvK1gxTTFkNk1XUC9LaDFlSDllOWxKNTI2YmRO?=
 =?utf-8?B?RW9YYUcydUE4OTl6OXptQTNQVCszdjBrQzEzNmFHbVY3bG9RL0RWN0hDbkxG?=
 =?utf-8?B?dTNmN3ErN0ZQMlNKUlppelRCZm10VGROaUJiSzE1WERTTmVudjlkSVZ2MGtY?=
 =?utf-8?B?RitmVTVyUTM5R3VuSG5aUlMzTDFKZGltMS9PcTdEaFgvcmI1RmV1SVlpZU9X?=
 =?utf-8?B?eitCK0xyT1hLcXVnaG1Bd1pReEZ5clZVYnQrM2ZxZVl5U0E2S0ZML1pRclJU?=
 =?utf-8?B?akpMa2ZYMDVFVTFRUzRXVnNqMUE4ZHdidzgvYTYwaFRiYkU4L3RpL0t1cDlv?=
 =?utf-8?B?ZzdHdkZBK01BcjZzR3NmYVdWb2JTNGpZY283aG5yL3RjNkFvYjRYTzNDdCtX?=
 =?utf-8?B?WlNyZGdiSGF2YlEzRzlnNmR1UHhJMkVSQjlVVTBwRUhLbTJnVThFbmgwM3Fj?=
 =?utf-8?B?eUNkRFFZZGVzdUtUVHJQcElkYnRKTm94dHQ4eVdqVk5RSlVKR3BvNU96ckN2?=
 =?utf-8?B?eFUrckRsWkxJcTNzVGQwL3dkOThLbmNiN3pWVTg1NnhwNzNXMktidUk3VmFV?=
 =?utf-8?B?VjJQYUdxMW1vcHpsRHlYSVcvWC9PWHAxb2VwcVZqMjlHQ01BQU93ZDBqVmti?=
 =?utf-8?B?RURqY0JZSUdta3hnMDVXM1d0K0Mrbi9BbTBlUmtvMzJVaWQ4Mll5Mi9EUXRw?=
 =?utf-8?B?UHk1bnY0NEVxRUNScmE1NmNjbE1TL3ROY3RPMSt1TFRyS3REQXMrZm43dWhV?=
 =?utf-8?B?SUU4NGI4OElMSy9wVHd3U2dwNTQwQ2dXWHk0YU1ka213cWo3cERoWW02QXRx?=
 =?utf-8?B?cC8zNUxReFBvMURUR3E5S0JCRFgzUk1BeklKQ2pZSzVINWlybWlzMnBJWmZC?=
 =?utf-8?B?TkpHN3VRU1dQY1pmRWVGVk1iS1RJblVrMWpCUWJwUDN3THpneTdjSlc2MWY4?=
 =?utf-8?B?SlQxb1hLMzR6QUtHdVk0Lyt1d2t4cTdXdzMvdlFTRGpINHE4bUlFV2JHSUl0?=
 =?utf-8?B?aVJRVEdJa0FXZWwrS2lmanZON09ESzFjM0NFVHVzSVREQ1M1MHplanBTMTlQ?=
 =?utf-8?B?U1B2TVhxTEZFWkFnL084cGovY1Bta1RQczdtRUQvT0xyZkx0dFUyaTdKNUlW?=
 =?utf-8?B?WkdkaWNITHNwRy9hRzJTbUFNK2s4TmYzVTMyNnorL1liY1ZZOFhZWVZCWUFp?=
 =?utf-8?B?THNZNXllUHFMQmJYb1VFNUplOTBrNnNXUE5INzFCU0xZc0Q1cGNqaUdLU1Qz?=
 =?utf-8?B?MVN4RGNOZzNFTGtuZlhXNGFYNnd0eUVRK2pLa1lhRjRmNDRwMWFXQ3Yzclp4?=
 =?utf-8?B?L3RmeXYxL3JvUWo2MW5iUUZLaFRCM3FmNTVQZG5pSThPckhUd2w4aUZDUE9r?=
 =?utf-8?B?MUlDeml0MFVaeDB0b0ZJSVpGWThWajlUZUpiaXdoVGZhMUgySVpqSk95TG4v?=
 =?utf-8?B?d1VIbEsxb0tOQWxVU0pJc1R1VXFtTnRTcUlUWVQ4MHZqTFhoUUJ1NWhJRUw0?=
 =?utf-8?B?RmhLNHRDQ2xFRUxsRFVYZHdtTm9vKzF3ZzdQa0ZKSHBybENvVkR0RHhaUVJ2?=
 =?utf-8?B?REV5R3FXUDliMkNwVTlFaVhVSEJSK1NYVUgvcXlsSmZhSUdLZFVvTC9rVHg0?=
 =?utf-8?B?M0Jpb1BobWk0bGtZdjVpZHBWajFrcFhRc2t3cjZSRCsyZW9tZ01vc0VvWkxO?=
 =?utf-8?B?b1NsSWg5RzE4M2xEZ3pZWUVDeE1iWmJZcjNvakU0UE5BdU54dnd0UUMzeE5O?=
 =?utf-8?B?dzF3UzFtVjdYOXczOVJiVTVYc2hxZmhKRWVwdTcrZis3TVBIRXhnT0w1aG04?=
 =?utf-8?B?eWlLMGJEdlp5amxNZFhiQnZURzdOYWhibGpUbUhkNWo5dzFNTE9JV0FIZStE?=
 =?utf-8?Q?6pGUsF9gbLgsW5wDdDuLxt8+N?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5cd4209c-0ffc-4a36-df15-08dc83ef6c19
X-MS-Exchange-CrossTenant-AuthSource: SN6PR12MB2767.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Jun 2024 17:05:51.4867
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: tw7ydBLh+gTxOlPyipovyGShVDYDzz5g1MQv5HJawJ3OKuYnG72+qC7ALxvxDm2DoAnw+dq3nbOq/LxXVN/7WA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR12MB6549

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