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

[Martin Fernandez <martin.fernandez@eclypsium.com>]

On 12/21/21, Greg KH <gregkh@linuxfoundation.org> wrote:
> 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.
>

I prefer that Andy answers.

Either way, I think that the enum will be the best option.

>> >> 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?
>

Oh I see. No, that's not a problem. That if guard is to decide if you
need to start/close a region. e820_nomerge is to prevent merging
certain region types. In any case, the new check will cause less
merging, complying with what e820_nomerge says.