Re: [PATCH v4 3/5] x86/e820: Tag e820_entry with crypto capabilities

[Greg KH <gregkh@linuxfoundation.org>]

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