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