Received-SPF: pass (google.com: domain of linux-kernel+bounces-199460-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <7eb4ca99-679c-4f23-9562-a39b966c28a0@amd.com>
Date: Mon, 3 Jun 2024 11:48:03 -0500
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v7 1/3] efi/x86: Fix EFI memory map corruption with kexec
Content-Language: en-US
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>
From: "Kalra, Ashish" <ashish.kalra@amd.com>
In-Reply-To: <Zl3hwYL2GDiyUfGo@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?OVpqMEkvR3N6T3RQMmxuQ0dWc1VTY3p1cVFnWDhCT1RQRG9aQ2NQdjZpTTB1?=
 =?utf-8?B?TXdDbkFJWEJFcFFOWGVudWpMSUZ2NDZsRm5FaUNEZXBZaDQyUXRLcXV6T0Jw?=
 =?utf-8?B?djZVMHZwcHVTMVdVRkVpaFEzUmRweHBmdzZpMFNPYXg2UzZlS29hSCsxVWh2?=
 =?utf-8?B?UkExd0UrMEtKNlQ1MllmV1J1dXc1SzZKRCs3bGxRVG5ic2FmeWNIeHpJSGUw?=
 =?utf-8?B?RE12b3hGM3JPODFXcDV0Q1FFOWs4NWhBTFhYMmpVaUlteFduZFdMeWVuL0h6?=
 =?utf-8?B?SjVReTZ4VDIrQm83YjNnRWpxN05ZOFV0ZUNaa0Y5WnFSQ0tzS09rQXBlSEd5?=
 =?utf-8?B?YVh6ZCtPMlA2OTJBREZsZVJOVWRWK1VYWXlDYUZXaE5YbmFEemtLai9LRU50?=
 =?utf-8?B?NGF6Zk93SFpXZnZSeEZWRXo2ZFoxSXFtUXIwUzVjSHpxdEFCSmhWbGZwTjh0?=
 =?utf-8?B?SzNZbFh6ZkpldzdRWHVIcm9BV212eUEvWUphK1VNZitQNmNzL2paM3RRWjRB?=
 =?utf-8?B?K1ROVWg1QkpxdDlqWWJzaFVJREZURVhhVUoybEYxYjBybUJVTUg5YjBPYnh2?=
 =?utf-8?B?bGF5ZmRlQ2FsUjJudmsxTWhJU3dNNUIyVE1ZdW0xbHY2ZHJNYTRDcnArK2FB?=
 =?utf-8?B?ckpGK3Bhbmp3K3pLRlZ5d1B1VE1lNmZqaENyZDA2MFMvWCtiNFpyL3F6V2d4?=
 =?utf-8?B?QjJkVkprNXdYbktMSHMzNy84V3BobWRvZ3JPbkpPdEtiQmRCVi9mWFlJNGNl?=
 =?utf-8?B?cGRMSlpEbkIyODF3aXk5d3pkdE16ZkRiWUF5dzJDRm9lQWx1Tnl0cktmQmhF?=
 =?utf-8?B?U3NDNzZhN1E0b2xNQ2lKWTNnMS9NRFVqYWVMTVpLRGtieGlHUUhuRkJ1aW9q?=
 =?utf-8?B?REFGRmdtL1VJb0xxcCt6d1JRSXJSZUJVRFFPSk9lMGMwL2lsZi9KWVlWY3I0?=
 =?utf-8?B?SnFmSEdOTG5aT0t6NVFwYUUwUXo0U2N2M2dPaVZlTmFENXpER3MvWGFua1h0?=
 =?utf-8?B?RWNoc0xSYVNQNHZNcXNDNWxld05NVEU5NjNsdEJ0em41Zm5Id0h4dDdDNGU2?=
 =?utf-8?B?SWsrdUFDR1hjSk5pbXNKTGtSNFJVdFJNOEVySDFYU3R1T1lJTWVyMDhlb0M3?=
 =?utf-8?B?aHN5OGF0OHRQRkhWN053M3poRitpRU9MT2ZxZ2JLcmh5OUo3Vkg1QmY1WTB5?=
 =?utf-8?B?NSs3QkUyWHFiSHM2L1dCeTdYV1Axd2lQTmNReFQvVmFOQTMwWjh0UnVrMm5C?=
 =?utf-8?B?RWVNTlBuZnZ4RGF4UEJuVDd0cTNrdy96QTdKand4VmxWWlRRNVdmT3kyT0hC?=
 =?utf-8?B?RFR0SDBGWGp2TTlCRlZVcFNuQVVFaTRobm4xUUc4SnpMWUdzdVdqNC9zSjUy?=
 =?utf-8?B?R09BMGRVN3drbk1kcFhMb1RFanhRQlJrK3l3QzdEUjlWYUthTEVHUnZqSTdD?=
 =?utf-8?B?VTJBZWVseFBoY29pMEtYak0rU0NRbXJmMnlxR0NjU09aVHA2UmNKcVU3ZDYv?=
 =?utf-8?B?WjJYeitqMkJQcmk4d1AzWU96QVQ3VmJSei80azI0d051bFBWd1JlYTJjTlRn?=
 =?utf-8?B?eDVjMW1Lckl6bW5ONERZVENCWnpxRkFMS0N2UU9SZHVyWnduejlSd2V3VWNB?=
 =?utf-8?B?RHI2RGU4OXBaMW5JUHA3U3RnbWxPRUt1RXdmemN6cEM4b2ZETTFJM3R5SXlR?=
 =?utf-8?B?bTJMbTNjenZJYmxYTjdYSkRZTHZNcDNkdnBQNWlyTy9DVk4xa0hER1R4RGxW?=
 =?utf-8?B?MDJIb1hCdkRNdmZIdmUzazlocnBWbmhJbUZubjNVV1N1Z0tKeEN4UTZYZXN0?=
 =?utf-8?B?UkJuQWEzYmZOU2IwMFFCS2kxaG54RTlCUCtaR0pUMXJsSVhmdktHQlYyT212?=
 =?utf-8?B?bVFmSkVWYUE3dDd5dkNLaTFYQTlHTDRvbDN2TFNjYkhDOGNpRlRQaVp3aEhV?=
 =?utf-8?B?ZWxtclRIM0c2UHNhNE1LWEh4aVQrTWsrVWt4Q2p2VUZJdVJhcFBLOGhQVzRY?=
 =?utf-8?B?S3pKd2pYNlpkYmMwamdoUGh1eXJLUmo1dGE0M3NxTUE1bmhXUlovV25mUG1F?=
 =?utf-8?B?YmlwYVQwZThKWkJlb2J4TjdqWTdFU2xmcmNZM1NRRXF6ZUpwdGVwRGpWQmR4?=
 =?utf-8?Q?WdjoFuUxhKO72S+3fZPNEG0hw?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9ad656c8-a988-4090-396f-08dc83ecf0f5
X-MS-Exchange-CrossTenant-AuthSource: SN6PR12MB2767.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Jun 2024 16:48:05.9224
 (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: BA75qaafOeAOS9fttu+5QmEsIl15vxLmjTbCKHBmqSaBnRPBz1jSUgLz3ilMe+eR2F6K+ixDK3Lv/f+nUKi50Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR12MB7707

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 
<https://lore.kernel.org/lkml/Zl3HfiQ6oHdTdOdA@kernel.org/T/#iZ2e.:..:f4be03b8488665f56a1e5c6e6459f447352dfcf5.1717111180.git.ashish.kalra::40amd.com:1arch:x86:platform:efi:quirks.c> 
--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;
-- 
2.34.1

>> -- 
>> Regards/Gruss,
>>      Boris.
>>
>> https://people.kernel.org/tglx/notes-about-netiquette