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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id DA2EDC433EF for ; Tue, 21 Dec 2021 06:41:20 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4BD2A6B0073; Tue, 21 Dec 2021 01:41:20 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 46D736B0074; Tue, 21 Dec 2021 01:41:20 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 30F7B6B0075; Tue, 21 Dec 2021 01:41:20 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0075.hostedemail.com [216.40.44.75]) by kanga.kvack.org (Postfix) with ESMTP id 239AE6B0073 for ; Tue, 21 Dec 2021 01:41:20 -0500 (EST) Received: from smtpin25.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id B473B181AC9C6 for ; Tue, 21 Dec 2021 06:41:19 +0000 (UTC) X-FDA: 78940854678.25.4D52836 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf26.hostedemail.com (Postfix) with ESMTP id 40B08140028 for ; Tue, 21 Dec 2021 06:41:19 +0000 (UTC) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 2A7B06145C; Tue, 21 Dec 2021 06:41:18 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id DF834C36AE7; Tue, 21 Dec 2021 06:41:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1640068877; bh=ylv71IjLQIClbad2LwfXZJJ7QyLDq1p0XqsAvIfjF0E=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=qHJOk3sLAlFO7TaKjGyzKiY0cn6b7FUC2krnWAIRaPlVdE30eujy5kqtZvXwZM4j9 x1m6PDM73EQaCKeFPtatH5b5fcnpEmc1cm7U+fLUW48b9eNEqZAaUUkSYMc2Qeqkk5 uD7KEfIsnrosa6gkKzFyUOG/oUDmqEUxiS+ljIU0= Date: Tue, 21 Dec 2021 07:41:15 +0100 From: Greg KH To: Martin Fernandez Cc: linux-kernel@vger.kernel.org, linux-efi@vger.kernel.org, platform-driver-x86@vger.kernel.org, linux-mm@kvack.org, tglx@linutronix.de, mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com, x86@kernel.org, hpa@zytor.com, ardb@kernel.org, dvhart@infradead.org, andy@infradead.org, rafael@kernel.org, rppt@kernel.org, akpm@linux-foundation.org, daniel.gutson@eclypsium.com, hughsient@gmail.com, alex.bazhaniuk@eclypsium.com, alison.schofield@intel.com Subject: Re: [PATCH v4 3/5] x86/e820: Tag e820_entry with crypto capabilities Message-ID: References: <20211216192222.127908-1-martin.fernandez@eclypsium.com> <20211216192222.127908-4-martin.fernandez@eclypsium.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: 40B08140028 X-Stat-Signature: 5sscd1645ibdq67j8kcq9mz5jyqdazbs Authentication-Results: imf26.hostedemail.com; dkim=pass header.d=linuxfoundation.org header.s=korg header.b=qHJOk3sL; spf=pass (imf26.hostedemail.com: domain of gregkh@linuxfoundation.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=gregkh@linuxfoundation.org; dmarc=pass (policy=none) header.from=linuxfoundation.org X-HE-Tag: 1640068879-549873 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Mon, Dec 20, 2021 at 05:27:00PM -0300, Martin Fernandez wrote: > On 12/20/21, Greg KH wrote: > > On Thu, Dec 16, 2021 at 04:22:20PM -0300, Martin Fernandez wrote: > >> diff --git a/arch/x86/include/asm/e820/types.h > >> b/arch/x86/include/asm/e820/types.h > >> index 314f75d886d0..7b510dffd3b9 100644 > >> --- a/arch/x86/include/asm/e820/types.h > >> +++ b/arch/x86/include/asm/e820/types.h > >> @@ -56,6 +56,7 @@ struct e820_entry { > >> u64 addr; > >> u64 size; > >> enum e820_type type; > >> + u8 crypto_capable; > > > > Why isn't this a bool? > > It was a bool initially, but Andy Shevchenko told me that it couldn't > be that way because boolean may not be part of firmware ABIs. Where does this structure hit an "ABI"? Looks internal to me. If not, then something just broke anyway. > >> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c > >> index bc0657f0deed..001d64686938 100644 > >> --- a/arch/x86/kernel/e820.c > >> +++ b/arch/x86/kernel/e820.c > >> @@ -163,7 +163,7 @@ int e820__get_entry_type(u64 start, u64 end) > >> /* > >> * Add a memory region to the kernel E820 map. > >> */ > >> -static void __init __e820__range_add(struct e820_table *table, u64 start, > >> u64 size, enum e820_type type) > >> +static void __init __e820__range_add(struct e820_table *table, u64 start, > >> u64 size, enum e820_type type, u8 crypto_capable) > > > > Horrid api change, but it's internal to this file so oh well :( > > > > Hint, don't add flags to functions like this, it forces you to have to > > always remember what those flags are when you read the code. Right now > > you stuck "0" and "1" in the function call, which is not instructional > > at all. > > > > Heck, why not make it an enum to have it be self-describing? Like the > > type is here. that would make it much better and easier to understand > > and maintain over time. > > > > Yes, an enum will absolutely improve things. I'll do that. > > >> @@ -327,6 +330,7 @@ int __init e820__update_table(struct e820_table > >> *table) > >> unsigned long long last_addr; > >> u32 new_nr_entries, overlap_entries; > >> u32 i, chg_idx, chg_nr; > >> + u8 current_crypto, last_crypto; > >> > >> /* If there's only one memory region, don't bother: */ > >> if (table->nr_entries < 2) > >> @@ -367,6 +371,7 @@ int __init e820__update_table(struct e820_table > >> *table) > >> new_nr_entries = 0; /* Index for creating new map entries */ > >> last_type = 0; /* Start with undefined memory type */ > >> last_addr = 0; /* Start with 0 as last starting address */ > >> + last_crypto = 0; > >> > >> /* Loop through change-points, determining effect on the new map: */ > >> for (chg_idx = 0; chg_idx < chg_nr; chg_idx++) { > >> @@ -388,13 +393,17 @@ int __init e820__update_table(struct e820_table > >> *table) > >> * 1=usable, 2,3,4,4+=unusable) > >> */ > >> current_type = 0; > >> + current_crypto = 1; > >> for (i = 0; i < overlap_entries; i++) { > >> + current_crypto = current_crypto && overlap_list[i]->crypto_capable; > > > > Is it a u8 or not? You treat it as a boolean a lot :( > > > >> if (overlap_list[i]->type > current_type) > >> current_type = overlap_list[i]->type; > >> } > >> > >> /* Continue building up new map based on this information: */ > >> - if (current_type != last_type || e820_nomerge(current_type)) { > >> + if (current_type != last_type || > >> + current_crypto != last_crypto || > >> + e820_nomerge(current_type)) { > > > > Why check it before calling e820_nomerge()? Is that required? > > > > I don't see how the order of the checks matter, am I missing something? It might prevent this function from being called now when it previously was. Is that ok? thanks, greg k-h