From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9E72110EC for ; Mon, 11 Apr 2022 06:49:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1649659790; x=1681195790; h=message-id:date:mime-version:to:cc:references:from: subject:in-reply-to:content-transfer-encoding; bh=Im18RwYdvETKIFnmTzUmsr5SNvinw0XNRdaj7/G9hPU=; b=C4p1o3LGVJb12GtHbcy1LyyrGDeazfm6Lr8SfjBBgtGQmBDOUPO3AtT3 CnLeUIS6wAl5aHAuPQcmrPlmqn6WBofsDTWO9/TDmxFuxmbVt5FGw8Ux0 HLhf+4hlCT5jMvMU8i82A2jRLIkkQ1CCow/uOsLFtJ7UXlvoAPRyvjn2g gWsq5CEKGABdqIms7idIIqrvq5ubzrjbqjOotFURto8xc+m5pgybY11s8 EOxlaar/xVJHn97YqnUTx6rgAgf2O3Bc35NTRt42mdMCANukUZMdOZ/AI FiBix5MFzTgceBl48PNhfC4yIybF4VFi5cZUBAY5pRp909/ohyKkChRbv w==; X-IronPort-AV: E=McAfee;i="6400,9594,10313"; a="249327975" X-IronPort-AV: E=Sophos;i="5.90,251,1643702400"; d="scan'208";a="249327975" Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Apr 2022 23:49:49 -0700 X-IronPort-AV: E=Sophos;i="5.90,251,1643702400"; d="scan'208";a="525318292" Received: from srkondle-mobl.amr.corp.intel.com (HELO [10.212.113.6]) ([10.212.113.6]) by orsmga002-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Apr 2022 23:49:47 -0700 Message-ID: Date: Sun, 10 Apr 2022 23:49:52 -0700 Precedence: bulk X-Mailing-List: linux-coco@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Content-Language: en-US To: "Kirill A. Shutemov" Cc: "Kirill A. Shutemov" , Borislav Petkov , Andy Lutomirski , Sean Christopherson , Andrew Morton , Joerg Roedel , Ard Biesheuvel , Andi Kleen , Kuppuswamy Sathyanarayanan , David Rientjes , Vlastimil Babka , Tom Lendacky , Thomas Gleixner , Peter Zijlstra , Paolo Bonzini , Ingo Molnar , Varad Gautam , Dario Faggioli , Brijesh Singh , Mike Rapoport , David Hildenbrand , x86@kernel.org, linux-mm@kvack.org, linux-coco@lists.linux.dev, linux-efi@vger.kernel.org, linux-kernel@vger.kernel.org References: <20220405234343.74045-1-kirill.shutemov@linux.intel.com> <20220405234343.74045-5-kirill.shutemov@linux.intel.com> <043469ae-427c-b2bb-89ff-db8975894266@intel.com> <20220409202035.plaiekzuihov4kvq@box.shutemov.name> From: Dave Hansen Subject: Re: [PATCHv4 4/8] x86/boot/compressed: Handle unaccepted memory In-Reply-To: <20220409202035.plaiekzuihov4kvq@box.shutemov.name> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 4/9/22 13:20, Kirill A. Shutemov wrote: > On Fri, Apr 08, 2022 at 10:57:17AM -0700, Dave Hansen wrote: ... >> It's a real shame that we have to duplicate this code. Is there >> anything crazy we could do here like >> >> #include "../../../lib/find_bit.c" >> >> ? > > Well, it would require fracturing source files on the kernel side. > > __bitmap_set() and __bitmap_clear() are now in lib/bitmap.c. > > _find_next_bit() is in lib/find_bit.c. > > Both lib/bitmap.c and lib/find_bit.c have a lot of stuff that are not used > here. I guess we would need to split them into few pieces to make it in > sane way. Do you want me to go this path? I'd be curious if others have any sane ideas for how to do it. One idea would be to stick most of the implementation in a header that we can #include. Then, lib/find_bit.c #includes that header and does something simple like: #include "header.h" int _find_next_bit(...) { return _find_next_bit_from_header(); } EXPORT_SYMBOL(_find_next_bit); >>> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c >>> index fa8969fad011..c1d9d71a6615 100644 >>> --- a/arch/x86/boot/compressed/misc.c >>> +++ b/arch/x86/boot/compressed/misc.c >>> @@ -18,6 +18,7 @@ >>> #include "../string.h" >>> #include "../voffset.h" >>> #include >>> +#include >>> >>> /* >>> * WARNING!! >>> @@ -43,6 +44,9 @@ >>> void *memmove(void *dest, const void *src, size_t n); >>> #endif >>> >>> +#undef __pa >>> +#define __pa(x) ((unsigned long)(x)) >> >> Those #undef's always worry me. Why is this one needed? > > arch/x86/boot/compressed/misc.c:47:9: warning: '__pa' macro redefined [-Wmacro-redefined] > #define __pa(x) ((unsigned long)(x)) > ^ > arch/x86/include/asm/page.h:47:9: note: previous definition is here > #define __pa(x) __phys_addr((unsigned long)(x)) > > Note that sev.c does the same. At least we are consistent :) Ugh. Please do look into fixing this properly. The SEV folks will thank you. :) >>> +void accept_memory(phys_addr_t start, phys_addr_t end) >>> +{ >>> + unsigned long *unaccepted_memory; >>> + unsigned int rs, re; >>> + >>> + unaccepted_memory = (unsigned long *)boot_params->unaccepted_memory; >>> + rs = start / PMD_SIZE; >> >> OK, so start is a physical address, PMD_SIZE is 2^21, and 'rs' is an >> unsigned int. That means 'rs' can, at most, represent a physical >> address at 2^(21+32), or 2^53. That's cutting it a *bit* close, don't >> you think? >> >> Could we please just give 'rs' and 're' real names and make them >> 'unsigned long's, please? It will surely save at least one other person >> from doing math. The find_next_bit() functions seem to take ulongs anyway. > > Okay. 'range_start' and 'range_end' are good enough names? Yep, works for me.