From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 44C2ED68BCD for ; Fri, 15 Nov 2024 17:30:40 +0000 (UTC) Received: from list by lists.xenproject.org with outflank-mailman.837941.1253848 (Exim 4.92) (envelope-from ) id 1tC09E-0008Gu-PY; Fri, 15 Nov 2024 17:30:20 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 837941.1253848; Fri, 15 Nov 2024 17:30:20 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1tC09E-0008Gn-Mt; Fri, 15 Nov 2024 17:30:20 +0000 Received: by outflank-mailman (input) for mailman id 837941; Fri, 15 Nov 2024 17:30:18 +0000 Received: from se1-gles-sth1-in.inumbo.com ([159.253.27.254] helo=se1-gles-sth1.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1tC09C-0008Gh-DH for xen-devel@lists.xenproject.org; Fri, 15 Nov 2024 17:30:18 +0000 Received: from NAM02-BN1-obe.outbound.protection.outlook.com (mail-bn1nam02on2060d.outbound.protection.outlook.com [2a01:111:f403:2407::60d]) by se1-gles-sth1.inumbo.com (Halon) with ESMTPS id 4485e88e-a377-11ef-a0c7-8be0dac302b0; Fri, 15 Nov 2024 18:30:12 +0100 (CET) Received: from SJ0PR13CA0206.namprd13.prod.outlook.com (2603:10b6:a03:2c3::31) by CH3PR12MB8936.namprd12.prod.outlook.com (2603:10b6:610:179::17) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.8158.20; Fri, 15 Nov 2024 17:30:07 +0000 Received: from SJ1PEPF00001CE4.namprd03.prod.outlook.com (2603:10b6:a03:2c3:cafe::77) by SJ0PR13CA0206.outlook.office365.com (2603:10b6:a03:2c3::31) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.8182.10 via Frontend Transport; Fri, 15 Nov 2024 17:30:06 +0000 Received: from SATLEXMB04.amd.com (165.204.84.17) by SJ1PEPF00001CE4.mail.protection.outlook.com (10.167.242.20) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.20.8158.14 via Frontend Transport; Fri, 15 Nov 2024 17:30:06 +0000 Received: from SATLEXMB04.amd.com (10.181.40.145) by SATLEXMB04.amd.com (10.181.40.145) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.39; Fri, 15 Nov 2024 11:30:05 -0600 Received: from [172.17.143.135] (10.180.168.240) by SATLEXMB04.amd.com (10.181.40.145) with Microsoft SMTP Server id 15.1.2507.39 via Frontend Transport; Fri, 15 Nov 2024 11:30:05 -0600 X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list Sender: "Xen-devel" X-Inumbo-ID: 4485e88e-a377-11ef-a0c7-8be0dac302b0 X-Custom-Connection: eyJyZW1vdGVpcCI6IjJhMDE6MTExOmY0MDM6MjQwNzo6NjBkIiwiaGVsbyI6Ik5BTTAyLUJOMS1vYmUub3V0Ym91bmQucHJvdGVjdGlvbi5vdXRsb29rLmNvbSJ9 X-Custom-Transaction: eyJpZCI6IjQ0ODVlODhlLWEzNzctMTFlZi1hMGM3LThiZTBkYWMzMDJiMCIsInRzIjoxNzMxNjkxODEyLjc3MzAxMSwic2VuZGVyIjoiamFzb24uYW5kcnl1a0BhbWQuY29tIiwicmVjaXBpZW50IjoieGVuLWRldmVsQGxpc3RzLnhlbnByb2plY3Qub3JnIn0= ARC-Seal: i=1; a=rsa-sha256; s=arcselector10001; d=microsoft.com; cv=none; b=FEyi4ylgk5/YFomFPmFVbZcUw8tKtaZlB2fu81KeTD5zWaN3LLNoG64i7SeOkZqsghQsdO+fCB6n09fRf9Q885BILctnJERreY9ei4oeiFHkaeE0ajxtGf9hpwvTkNFyo/Vlo9xOw40yjxlbhWSA9TSduS5HQ5B7VlwMtLS1NwH1b0fsxsZl6IIaezUdiwz/YK8AxFLgR6Bu2mXXm6hmxfsD8maFsZ5L2gq10sdJZjA7IpO0q1JV8KZT1B3CD/vYMKmUCaOSLRip6MrSQeNXjeK7+IaY68XNqFwfZZzoDtn0I0/bKkK48vFJXxNNObXElKb5tAahRu/KMtSRveQuQQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector10001; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=Spl4V/nqqoU6Vg9mo4P+0Of8dR0qSG9zK57CRV0edPU=; b=EWGoBLfJfrb3QXwUnjVPDAV/6jAsI5vv/cLY/5RE10F1ydnHhvq6qjGuUv0EDTS3+InZXdLA+84Kkha9UF476sb7HVrbPqnnYK9Vi7DP28w4kzOAc+f/ACviQm0WIm5UYXzbuZ5lmzS0KeMmLQeKWIWu8NjJg/b3y0WLWvPXqAkXC5tabXRdiAgacMSK7lkUSpI0FWf8ZBOT7zeLdsWqDP44v8Ew/OFZVeS7a4619HSunaNWrQ5MBVEHrY4Rv2qM0fMLTAWQIsMV3fmF6xKervfenFTi7tu2vJYsCQVUOP8V37TQVcBoe2d50aIv89VanVKbSOfivM9elr2v17Sa2Q== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 165.204.84.17) smtp.rcpttodomain=apertussolutions.com smtp.mailfrom=amd.com; dmarc=pass (p=quarantine sp=quarantine pct=100) action=none header.from=amd.com; dkim=none (message not signed); arc=none (0) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amd.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=Spl4V/nqqoU6Vg9mo4P+0Of8dR0qSG9zK57CRV0edPU=; b=M16bK7vFC6VVl84JPVx6My3JHIbiZvmfhDRKi72HFlnt4gRWJdTp6O1G8u9wF3K3/p8dsQnv7FZ3wk/u84qNZ8qhSxaYh5DdoC/mKhgJjKH/gpADpNO8JxcQEQlWakYGzvjoeKOrjUOwd7tjcjllBYTLj6IItYEu3k++487TkUE= X-MS-Exchange-Authentication-Results: spf=pass (sender IP is 165.204.84.17) smtp.mailfrom=amd.com; dkim=none (message not signed) header.d=none;dmarc=pass action=none header.from=amd.com; Received-SPF: Pass (protection.outlook.com: domain of amd.com designates 165.204.84.17 as permitted sender) receiver=protection.outlook.com; client-ip=165.204.84.17; helo=SATLEXMB04.amd.com; pr=C Message-ID: Date: Fri, 15 Nov 2024 12:18:49 -0500 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v9 2/6] x86/boot: introduce module release To: "Daniel P. Smith" , CC: , , Jan Beulich , Andrew Cooper , =?UTF-8?Q?Roger_Pau_Monn=C3=A9?= References: <20241115131204.32135-1-dpsmith@apertussolutions.com> <20241115131204.32135-3-dpsmith@apertussolutions.com> <374d2387-6f8c-4a2b-a979-7066675dd247@amd.com> <3f61cc43-c29a-40e0-ac40-4f273fd4461e@apertussolutions.com> Content-Language: en-US From: Jason Andryuk In-Reply-To: <3f61cc43-c29a-40e0-ac40-4f273fd4461e@apertussolutions.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit Received-SPF: None (SATLEXMB04.amd.com: jason.andryuk@amd.com does not designate permitted sender hosts) X-EOPAttributedMessage: 0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: SJ1PEPF00001CE4:EE_|CH3PR12MB8936:EE_ X-MS-Office365-Filtering-Correlation-Id: 34cdacb8-efef-4e83-d2ef-08dd059b25b4 X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0;ARA:13230040|1800799024|36860700013|376014|82310400026; X-Microsoft-Antispam-Message-Info: =?utf-8?B?dXRVNjBXK3FkUU9nbTArQVY1T3BKY3hEeGEyK0tIMzczTnJiUm1HOTBjZENY?= =?utf-8?B?K1YvUkE5SUlaQ1pDUlBrSEVQUFU2ODF5LzFvMkUvSkNUalRiVFkvVW1aWERs?= =?utf-8?B?OHBCelp6RFdPUGdaR0h1bVBSQW4zYUVmTmR5WjJ5aHJlcjA0MnBoUFVJY1hY?= =?utf-8?B?LzRXU2VTcnRIQmFhSm95RVhyQnhqN3ZHNjRpSFV3ZjhHY3IyejZwM05za1Jq?= =?utf-8?B?ZTRXcStFdTRTRXFlRUNnY3RqcExpaFZ2S0hWWkU5T2FOdkFmR2xoNDlGRW9s?= =?utf-8?B?bW4zbEdTYVVMNWhFRm8rU3hXNjJ1WG93bjdQdFk5ZUtObk13TS83SHhaK0dQ?= =?utf-8?B?SDhzUmJpbDhieWVRbGxlTTlpeWNhQk5HSnlPQjlPQWV1VzFkbytOQUVKaFYx?= =?utf-8?B?dlArSGl3TjBYUkVUdElhdmEzekNXblN6TUlLcG1WUkhXUlhidFlZek1Qclli?= =?utf-8?B?MGVtMElvamlKNEFKMmRYeGhJcmNKUUVBMU9RRkVWT2o3VlhBaUgvaEJGbjk1?= =?utf-8?B?U0xIZXVXczN3dkkvSnVxaVUxTmczeG9qc3JFY09kNWNOZ1A4U05EbUlqcFdW?= =?utf-8?B?RUdIOHNWTk9JNGNBc29UZGRDcWJlRE9NMUd0TkQ4Q0hoU3R3YzVDb3BUR1pp?= =?utf-8?B?M2RXdXF6VUplRFE3czVwd1RzSWZGNVVwc0t6T2FYZnU3ZWNKWnNCYzgxYWRL?= =?utf-8?B?SDl4WGhCZXA1UHlHUXdoTW53cXpTUk53eGRHclZ4S3pyV0hBZVdVcjBOMVJu?= =?utf-8?B?ZmxuQmtmNXhGZndvWS9sZ2kyNlZubCs0UnRVaGkwdnNubmF3dDJqbDZOQ1RQ?= =?utf-8?B?a0JEYTFUem8vNVhBNFNHQytSbmZxdGdoK3p1aFRPQ2pkZ0EwSys3RS9GUUJ5?= =?utf-8?B?WWdSakY0Zm93b1VYL1VsM0ZQTTF1ci9JTlRWR1E0ejkrTmtFWHNPeXV5bjND?= =?utf-8?B?VllYTHF5NkpYNVRHN1g0SHk4M1JnWm5LaWdjdHVMeU1zc05iOWo2RS91RlE2?= =?utf-8?B?ZS9LTno5QUFkZ1hHa2swWCtCQkpwRHlRTUhKY2IxOEt4aElLMDg1bVgzS0pX?= =?utf-8?B?N2pWcFhqT28wNjNHSXhJT1hTSEY0WmU2bVN4amR5KytlYTZiMG9DT0NMQlg4?= =?utf-8?B?NVNHakxSMk0xSHZxcVoyZHpUbW4vWGVldzFqQU9FNk1YSFVWMFZvY0oxWFhT?= =?utf-8?B?YU9mQjVpenY4cEZqQnlqN0xvOXF5VkRYdjBOZnF3a0VIRTUrUHNLUmVGSWx1?= =?utf-8?B?R0hyaDNOSXcyWEd5elNkSG5YYkZTZUZjMkJpbjVlQkszV3ZRRHNrb2ZkUVps?= =?utf-8?B?RFV5cU1tNjJ3aHh3MG9EWTNmcmRVY21wK0N4dG1vOFBITGo4blJPbXFzOFdk?= =?utf-8?B?SkRSN3hKVjhlZU5hWTV5NlFSZm5NRTN1bTlac2UxVkZlOFFDelFHQTJtNTdJ?= =?utf-8?B?cWIrSjZLZE1hZ016bnFuWGI4dW5RdUowcEtiajhkVWtMam1TNlNNNWhDa0gw?= =?utf-8?B?emVMTGhPKzRXSmROaTZGVXNiSVNGeEpkOFZiN2dHTktNWHAyTkdKNWNuY0lD?= =?utf-8?B?d0tqaThPclpUSkRxQkRiTVhCZHliNnVZekxqSXdNWXpCbnNKQzRHZ0hKcXZR?= =?utf-8?B?K0Y4YWl6MTlIYVBmOWlxdUl2Z3JtamJUZjhyajh3d29hRHhuWGJwZ0tKeXdT?= =?utf-8?B?Q2F2QjRrT29hMmZXeWRLazVhZUNnRGkwbFlBSVBseFJaTTNGam5UNzZVdi9x?= =?utf-8?B?Zm8ybDhGY2svQno5VGc3c0RXWlg1WS9TcHV1SHNieDhyTldDWVNrTmQ2aXVy?= =?utf-8?B?NXRIQ1ExR3phVmNoRm5rM1diMS92NkFDd3R1Y0daaXRENEdZYkJhUjFYRmxB?= =?utf-8?B?Z1Y3ck9wdUhleDVxZGptK1NLUk9Ldk53ajRCOHNhWkZER1E9PQ==?= X-Forefront-Antispam-Report: CIP:165.204.84.17;CTRY:US;LANG:en;SCL:1;SRV:;IPV:CAL;SFV:NSPM;H:SATLEXMB04.amd.com;PTR:InfoDomainNonexistent;CAT:NONE;SFS:(13230040)(1800799024)(36860700013)(376014)(82310400026);DIR:OUT;SFP:1101; X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Nov 2024 17:30:06.5436 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: 34cdacb8-efef-4e83-d2ef-08dd059b25b4 X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d;Ip=[165.204.84.17];Helo=[SATLEXMB04.amd.com] X-MS-Exchange-CrossTenant-AuthSource: SJ1PEPF00001CE4.namprd03.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Anonymous X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR12MB8936 On 2024-11-15 12:16, Daniel P. Smith wrote: > On 11/15/24 11:50, Jason Andryuk wrote: >> On 2024-11-15 08:12, Daniel P. Smith wrote: >>> A precarious approach was used to release the pages used to hold a >>> boot module. >>> The precariousness stemmed from the fact that in the case of PV dom0, >>> the >>> initrd module pages may be either mapped or copied into the dom0 >>> address space. >>> In the former case, the PV dom0 construction code will set the size >>> of the >>> module to zero, relying on discard_initial_images() to skip any >>> modules with a >>> size of zero. In the latter case, the pages are freed by the PV dom0 >>> construction code. This freeing of pages is done so that in either >>> case, the >>> initrd variable can be reused for tracking the initrd location in >>> dom0 memory >>> through the remaining dom0 construction code. >>> >>> To encapsulate the logical action of releasing a boot module, the >>> function >>> release_boot_module() is introduced along with the `released` flag >>> added to >>> boot module. The boot module flag `released` allows the tracking of >>> when a boot >>> module has been released by release_boot_module(). >>> >>> As part of adopting release_boot_module() the function >>> discard_initial_images() >>> is renamed to free_boot_modules(), a name that better reflects the >>> functions >>> actions. >>> >>> Signed-off-by: Daniel P. Smith >>> --- >>> Changes since v8: >>> - completely reworked the commit >>>    - switch backed to a releasing all but pv initrd approach >>>    - renamed discard_initial_images to free_boot_modules >>> --- >>>   xen/arch/x86/hvm/dom0_build.c       |  2 +- >>>   xen/arch/x86/include/asm/bootinfo.h |  2 ++ >>>   xen/arch/x86/include/asm/setup.h    |  4 +++- >>>   xen/arch/x86/pv/dom0_build.c        | 27 +++++++++++++-------------- >>>   xen/arch/x86/setup.c                | 27 +++++++++++++++------------ >>>   5 files changed, 34 insertions(+), 28 deletions(-) >>> >>> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/ >>> dom0_build.c >>> index d1bdf1b14601..d1410e1a02b0 100644 >>> --- a/xen/arch/x86/hvm/dom0_build.c >>> +++ b/xen/arch/x86/hvm/dom0_build.c >>> @@ -755,7 +755,7 @@ static int __init pvh_load_kernel( >>>       } >>>       /* Free temporary buffers. */ >>> -    discard_initial_images(); >>> +    free_boot_modules(); >> >> This... >> >>>       if ( cmdline != NULL ) >>>       { >> >>> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c >>> index 6be3d7745fab..2580162f3df4 100644 >>> --- a/xen/arch/x86/pv/dom0_build.c >>> +++ b/xen/arch/x86/pv/dom0_build.c >> >>> @@ -875,7 +874,7 @@ static int __init dom0_construct(struct boot_info >>> *bi, struct domain *d) >>>       } >>>       /* Free temporary buffers. */ >>> -    discard_initial_images(); >>> +    free_boot_modules(); >> >> ...and this.  I think Andrew requested/suggested moving to a single >> free_boot_modules call: >>      They're both right at the end of construction, so it would >>      make far more sense for __start_xen() to do this after >>      create_dom0().   That also avoids needing to export the function. > > I wanted to do this and had it written this way. Then I started testing > it and the pvhshim test failed due to not enough ram to build the domU > inside pvshim. I started splitting this commit to see where it broke the > test case, and for an unknown reason, replacing these two calls with a > single call in __start_xen() just after create_dom0() is the cause. > Instead of trying to tear apart the construction logic to determine why, > I backed this part of the change out for the time being. Ah, ok. Thanks for the info. >>>       /* Set up start info area. */ >>>       si = (start_info_t *)vstartinfo_start; >>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c >>> index 495e90a7e132..0bda1326a485 100644 >>> --- a/xen/arch/x86/setup.c >>> +++ b/xen/arch/x86/setup.c >> >>> +void __init free_boot_modules(void) >>>   { >>>       struct boot_info *bi = &xen_boot_info; >>>       unsigned int i; >>>       for ( i = 0; i < bi->nr_modules; ++i ) >>>       { >>> -        uint64_t start = pfn_to_paddr(bi->mods[i].mod->mod_start); >>> -        uint64_t size  = bi->mods[i].mod->mod_end; >>> - >>> -        /* >>> -         * Sometimes the initrd is mapped, rather than copied, into >>> dom0. >>> -         * Size being 0 is how we're instructed to leave the module >>> alone. >>> -         */ >>> -        if ( size == 0 ) >>> +        if ( bi->mods[i].released ) >>>               continue; >>> -        init_domheap_pages(start, start + PAGE_ALIGN(size)); >>> +        release_boot_module(&bi->mods[i]); >>>       } >>> - >>> -    bi->nr_modules = 0; >> >> IIUC, zero-ing here was a safety feature to ensure boot modules could >> not be used after this point.  Should it be retained? > > The released flag displaced the need for this, but I realized it would > make it stronger if in bootstrap_map_bm() we add a check that the > released flag is not set before mapping. I think this is a stronger > approach without loosing information like the number of boot modules > were passed. Andrew> Clobbering this prevents the loop constructs from working. I thought the boot modules are unusable after free_boot_modules() is called, so I'm not clear on the utility of keeping the boot modules around and/or keeping the loop constructs working. I wondered about, but didn't write, clearing the boot_module info in release_boot_module() to eliminate stale data hanging around. Yes, a bootstrap_map_bm() check is a good idea. Having said that, there is a lack of checking the return value of bootstrap_map_bm(), so would you panic? Regards, Jason