From: Greg KH <gregkh@linuxfoundation.org>
To: Martin Fernandez <martin.fernandez@eclypsium.com>
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
Date: Tue, 21 Dec 2021 07:41:15 +0100 [thread overview]
Message-ID: <YcF3C9kfVoRqKamp@kroah.com> (raw)
In-Reply-To: <CAKgze5boi5h08ffpodqsKp5xNS=+u_zJWEVnExdbsXRgJ+eCTQ@mail.gmail.com>
On Mon, Dec 20, 2021 at 05:27:00PM -0300, Martin Fernandez wrote:
> On 12/20/21, Greg KH <gregkh@linuxfoundation.org> 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
next prev parent reply other threads:[~2021-12-21 6:41 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-16 19:22 [PATCH v4 0/5] x86: Show in sysfs if a memory node is able to do encryption Martin Fernandez
2021-12-16 19:22 ` [PATCH v4 1/5] mm/memblock: Tag memblocks with crypto capabilities Martin Fernandez
2021-12-16 19:22 ` [PATCH v4 2/5] mm/mmzone: Tag pg_data_t " Martin Fernandez
2021-12-16 19:22 ` [PATCH v4 3/5] x86/e820: Tag e820_entry " Martin Fernandez
2021-12-20 16:37 ` Greg KH
2021-12-20 20:27 ` Martin Fernandez
2021-12-21 6:41 ` Greg KH [this message]
2021-12-22 15:35 ` Martin Fernandez
2021-12-16 19:22 ` [PATCH v4 4/5] x86/efi: Tag e820_entries as crypto capable from EFI memmap Martin Fernandez
2021-12-16 19:22 ` [PATCH v4 5/5] drivers/node: Show in sysfs node's crypto capabilities Martin Fernandez
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YcF3C9kfVoRqKamp@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=akpm@linux-foundation.org \
--cc=alex.bazhaniuk@eclypsium.com \
--cc=alison.schofield@intel.com \
--cc=andy@infradead.org \
--cc=ardb@kernel.org \
--cc=bp@alien8.de \
--cc=daniel.gutson@eclypsium.com \
--cc=dave.hansen@linux.intel.com \
--cc=dvhart@infradead.org \
--cc=hpa@zytor.com \
--cc=hughsient@gmail.com \
--cc=linux-efi@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=martin.fernandez@eclypsium.com \
--cc=mingo@redhat.com \
--cc=platform-driver-x86@vger.kernel.org \
--cc=rafael@kernel.org \
--cc=rppt@kernel.org \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).