On 12/21/21, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> On Mon, Dec 20, 2021 at 05:27:00PM -0300, Martin Fernandez wrote:
>> On 12/20/21, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> 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.
