Received-SPF: pass (google.com: domain of linux-kernel+bounces-109448-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <4d9fe26b-cf0e-4381-9dee-20f43f371710@intel.com>
Date: Thu, 21 Mar 2024 10:56:20 +1300
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] x86/kexec: do not update E820 kexec table for setup_data
Content-Language: en-US
To: Dave Young <dyoung@redhat.com>, <x86@kernel.org>
CC: Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
	Borislav Petkov <bp@alien8.de>, Dave Hansen <dave.hansen@linux.intel.com>,
	"H. Peter Anvin" <hpa@zytor.com>, <linux-kernel@vger.kernel.org>,
	<kexec@lists.infradead.org>, Baoquan He <bhe@redhat.com>, Eric Biederman
	<ebiederm@xmission.com>
References: <ZeZ2Kos-OOZNSrmO@darkstar.users.ipa.redhat.com>
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <ZeZ2Kos-OOZNSrmO@darkstar.users.ipa.redhat.com>
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?VFZubWZGWEFUbitnKzRiN3VlY0krT0JGdlY1dTZWTFdKUEt2ai9aZGk5d1Y3?=
 =?utf-8?B?QzhlSjFyYlpqVjFJTi9YM2pXb3FjeUgwSG1UcUE0aklPdmk4RlFsc004Q0ww?=
 =?utf-8?B?RXV0dGxiL1dPeDhuQ3dSNjBnUndqV0NhVUtNaldQRDBvQ2dFL2N2ZXZnY3lj?=
 =?utf-8?B?S1dxcjRUb2VhbTRndG83bkxJNGRabkdNQmFGNjR0ZzVIMU9ENVRiY25jSTlH?=
 =?utf-8?B?S2pOWlEwQXdSYmR3bC91UjBKYkNhMXNEWkpWTWtvazExM2FEa1Nxcmswc2FM?=
 =?utf-8?B?Nkh5Q0F4QUd2b3QrSC9QU0lwYzgvNFZUanBWQ2h1dFhSK2dSVG94ZnZRSmNF?=
 =?utf-8?B?ZE8waDN6STRhQ1ZacUhWeVI1aGVDSUNGeXZ1VGNYdXJjYlVTZFZtNkJUd2U0?=
 =?utf-8?B?MWhGbXhvejdNeHpxVzZ4NjE5a21jZm5sL2lTWUJVNHUwVERiN3VlU25iSGNy?=
 =?utf-8?B?NzZiZnZlandSWldmK0lKV0cxb2J6R1o4MFBsMEFuZnRwZ1NwV1NuYjY1cGxp?=
 =?utf-8?B?bHFzTm5COVBBZ3diS2M4N004Tk1DK21xZy9tRWJwU1RieGh2QWlMKy9wZng2?=
 =?utf-8?B?K2VpeWNoOVJBU2tYZDJFdGNpMEpuN21TYy90TUF3UzNYbzRWT3ZjdDRUVDgz?=
 =?utf-8?B?WjZJWlArRU1iMmtNa09keStaZkxyblcxWkVWa1JVZVZ6YVNJQTI2dGprWkJp?=
 =?utf-8?B?SFB6QSttZy92T3YrV0NwTVNvbUJPajJ4ay9LYllVdmJvdVByYWUxNlhTa2Yr?=
 =?utf-8?B?N1A3N2ErNnRoYzlGbUZvcjZrVnhpMDhraXFQQldRaXRid05MTFhMUTlzcVR5?=
 =?utf-8?B?WnNnTEtiY0g2QjY2SHdLY0ljNHhCcFI1TGU1QlRxZHBiekUwWWlJM05EY3cw?=
 =?utf-8?B?QXBWOHRHRVR1UlNCMVFMQmJpa0RtbFo1eDM2czdxR0kyYVlGWlZHZTVwa092?=
 =?utf-8?B?eTBvVURINkEyTEtkcS8xR2dMRmFQRktzbVNNb3QzY3hKTlNXVS9sU3FDbVYx?=
 =?utf-8?B?Zy80S3dTVHJqeE12YUIwTVc2TVpUdXR2L3NrYURuQkdwY3l4TWhFSWxpU1F4?=
 =?utf-8?B?MnhydVIvS0cyc1hndGs1MnVCMVNrdWxnL25rekszVUZSVWxBMGx6bmVVUG9J?=
 =?utf-8?B?SWY5alNXREprZUpoRmx2MjNKZEZHYjc1L1VVS20zak5ZZWNHTUc3UWd2ZXYw?=
 =?utf-8?B?dVNxMk4vS2pnd0FWSFhwaUVmOXpuM2FyRnE1NXBoUk94Z2drN1I1S0dPYytI?=
 =?utf-8?B?TzVKSWtLRndSS29XVlFuQ0UzQU1wcDg3WXdJN1VNbWI4ME1zUkZYSDlvd3d6?=
 =?utf-8?B?aXdWRUQ4NVBHbStDUjFUTkxDZ3dGdjlOYTlNS2RZbjdGSGFEQ1lhaWtVZERX?=
 =?utf-8?B?am1uQXd5VWF1ZFlOSUVkaTVucVJ0aHQvN0plVVJGRW9OV2x6aUVRNVo3NXdr?=
 =?utf-8?B?RDRVb0QySm9reTFVZXF6WnhVL1lmMzVZNW5pbzJ1SjB2RUlxOWZPYnVmRjMz?=
 =?utf-8?B?Mi9waVp2VjNSamtPcGJCOWoycDhQb0xnUTRtYVZuMEZlZUNXRUZxcUJFdW5r?=
 =?utf-8?B?dUFuTEhpN1diWUpHRy9MSlZUY1E1NStRem45RTNEOFlpcDVwVW8yK3FFNUV2?=
 =?utf-8?B?Z3hjV1lUODhQNVJRU1hpeVFSWkVOVHh6SGx4YkI1V05hVnk0QitoSXR5TlJP?=
 =?utf-8?B?MjI3azlhZExBbTlxS3d6OXBwL0ovR1M3ZDNUc3o2MGg4WjhOUlhNT3h1eThq?=
 =?utf-8?B?REhQNFo5SzhkM0xJVFF0eDVac210Y05NTy94QjJQSStPaU5MWFV2V244K0gr?=
 =?utf-8?B?ZlBPcTFlN2VtR3dvczJ0WEFrdFV6V0FBY2FGbWJuNFJqWVZiTE82UnhJMVQz?=
 =?utf-8?B?ZkFnNVlTVkFaeGljRU1MczZMMms5YWxlcG5JcXB6Q2FrdkZIdUs5RlhVYm9U?=
 =?utf-8?B?YjlyQTBRTEtDaE9hOXNWVWI0eGdiaDBQdFF0ejFxVnNXZTlDSVk2TUFTY0ll?=
 =?utf-8?B?dmFDNEhrMGFieFI2cUNGYno4eXA1U1Mrc2RKM1RESFJ5czdGOWdYamtzSmpn?=
 =?utf-8?B?UTJERy8veEw4SkNjeTNQN3lqeFJvQUw5eVBnYkN6MHBhY2ZldHBBSXFyUHhy?=
 =?utf-8?Q?mqg8Z4YNkhtaLE/uGVbVq2XvA?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2bad6151-1370-4be4-be4a-08dc492899e5
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Mar 2024 21:56:31.0246
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: pysZkq8F29K1JCtTgr4YpkbHI+r4VRMf5FX81OjcvO4xhHWkataM6fnaQ+ycJBJLajdMTzF7Msp1Er2uS70Fzw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW5PR11MB5930
X-OriginatorOrg: intel.com

Hi Dave,

Some nitpicking in changelog:

On 5/03/2024 2:32 pm, Dave Young wrote:
> crashkernel reservation failed on a Thinkpad t440s laptop recently,

',' -> '.' to make it as a standalone sentence.

> Actually the memblock reservation succeeded, but later insert_resource()
> failed.
> 
> Test step:
> kexec load ->
> 	kexec reboot ->
> 		check the crashkernel memory
> 		dmesg|grep "crashkernel reserved"; saw reserved suceeeded:

"suceeeded" -> "succeeded".

> 		0x00000000d0000000 - 0x00000000da000000
> 		grep Crash /proc/iomem: got nothing

And somehow I found it's not easy to read.  :-)

> 
> The background story is like below:

Better to have an blank line to make text more breathable.

> Currently E820 code reserves setup_data regions for both the current kernel
> and the kexec kernel, and it will also insert them into resources list.

"will insert" -> "inserts".

> Before the kexec kernel reboot nobody passes the old setup_data, kexec only

			  ^ "reboots"				  ^ and

> passes SETUP_EFI and SETUP_IMA if needed.  Thus the old setup data memory
> are not used at all. But due to old kernel updated the kexec e820 table

   ^ is					     ^ updates

> as well so kexec kernel see them as E820_TYPE_RESERVED_KERN regions, later

"so kexec kernel" -> ", the kexec kernel"

"see" -> "sees"

", later" -> ", and later"

> the old setup_data regions will be inserted into resources list in kexec

"will be" -> "are"

> kernel by e820__reserve_resources().
> 
> Note, due to no setup_data passed in for those old regions they are not

			    ^ is

> early reserved (by function early_reserve_memory), crashkernel memblock

						   ^ and the

> reservation will just regard them as usable memory and it could reserve
			
"regard" -> "treat"

> reserve crashkernel region overlaps with the old setup_data regions.

double "reserve".

"crashkernel region" -> "the crashkernel region"

"overlaps" -> "which overlaps"

> 
> Just like the bug I noticed here, kdump insert_resource failed because
> e820__reserve_resources added the overlapped chunks in /proc/iomem already.

			  ^ has added
> 
> Finally, looking at the code, the old setup_data regions are not used
> at all as no setup_data passed in by the kexec boot loader. Although

			  ^ is passed

> something like SETUP_PCI etc could be needed, kexec should pass
> the info as setup_data so that kexec kernel can take care of them.
> This should be taken care of in other separate patches if needed.
> 
> Thus drop the useless buggy code here.
> 
> Signed-off-by: Dave Young <dyoung@redhat.com>
> ---
>   arch/x86/kernel/e820.c |   16 +---------------
>   1 file changed, 1 insertion(+), 15 deletions(-)
> 
> Index: linux/arch/x86/kernel/e820.c
> ===================================================================
> --- linux.orig/arch/x86/kernel/e820.c
> +++ linux/arch/x86/kernel/e820.c
> @@ -1015,16 +1015,6 @@ void __init e820__reserve_setup_data(voi
>   		pa_next = data->next;
>   
>   		e820__range_update(pa_data, sizeof(*data)+data->len, E820_TYPE_RAM, E820_TYPE_RESERVED_KERN);
> -
> -		/*
> -		 * SETUP_EFI and SETUP_IMA are supplied by kexec and do not need
> -		 * to be reserved.
> -		 */
> -		if (data->type != SETUP_EFI && data->type != SETUP_IMA)
> -			e820__range_update_kexec(pa_data,
> -						 sizeof(*data) + data->len,
> -						 E820_TYPE_RAM, E820_TYPE_RESERVED_KERN);
> -
>   		if (data->type == SETUP_INDIRECT) {
>   			len += data->len;
>   			early_memunmap(data, sizeof(*data));
> @@ -1036,12 +1026,9 @@ void __init e820__reserve_setup_data(voi
>   
>   			indirect = (struct setup_indirect *)data->data;
>   
> -			if (indirect->type != SETUP_INDIRECT) {
> +			if (indirect->type != SETUP_INDIRECT)
>   				e820__range_update(indirect->addr, indirect->len,
>   						   E820_TYPE_RAM, E820_TYPE_RESERVED_KERN);
> -				e820__range_update_kexec(indirect->addr, indirect->len,
> -							 E820_TYPE_RAM, E820_TYPE_RESERVED_KERN);
> -			}
>   		}
>   
>   		pa_data = pa_next;
> @@ -1049,7 +1036,6 @@ void __init e820__reserve_setup_data(voi
>   	}
>   
>   	e820__update_table(e820_table);
> -	e820__update_table(e820_table_kexec);
>   
>   	pr_info("extended physical RAM map:\n");
>   	e820__print_table("reserve setup_data");
> 
>