| Message ID | 20211216192222.127908-4-martin.fernandez@eclypsium.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86: Show in sysfs if a memory node is able to do encryption | expand |
On Thu, Dec 16, 2021 at 04:22:20PM -0300, Martin Fernandez wrote: > Add a new member in e820_entry to hold whether an entry is able to do > hardware memory encryption or not. > > Add a new argument to __e820__range_add to accept this new > crypto_capable. > > Add a new argument to __e820__update_range to be able to change a > region's crypto_capable member. Also, change its behavior a little, > before if you wanted to update a region with its same type it was a > BUG_ON; now if you call it with both old_type and new_type equals, > then the function won't change the types, just crypto_capable. > > Change e820__update_table to handle merging and overlap problems > taking into account crypto_capable. > > Add a function to mark a range as crypto, using __e820__range_update > in the background. This will be called when initializing EFI. > > Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com> > --- > arch/x86/include/asm/e820/api.h | 1 + > arch/x86/include/asm/e820/types.h | 1 + > arch/x86/kernel/e820.c | 59 ++++++++++++++++++++++++------- > 3 files changed, 49 insertions(+), 12 deletions(-) > > diff --git a/arch/x86/include/asm/e820/api.h b/arch/x86/include/asm/e820/api.h > index e8f58ddd06d9..677dcbabcc8b 100644 > --- a/arch/x86/include/asm/e820/api.h > +++ b/arch/x86/include/asm/e820/api.h > @@ -17,6 +17,7 @@ extern bool e820__mapped_all(u64 start, u64 end, enum e820_type type); > extern void e820__range_add (u64 start, u64 size, enum e820_type type); > extern u64 e820__range_update(u64 start, u64 size, enum e820_type old_type, enum e820_type new_type); > extern u64 e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool check_type); > +extern u64 e820__range_mark_as_crypto_capable(u64 start, u64 size); > > extern void e820__print_table(char *who); > extern int e820__update_table(struct e820_table *table); > 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? > } __attribute__((packed)); > > /* > 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. > { > int x = table->nr_entries; > > @@ -176,12 +176,13 @@ static void __init __e820__range_add(struct e820_table *table, u64 start, u64 si > table->entries[x].addr = start; > table->entries[x].size = size; > table->entries[x].type = type; > + table->entries[x].crypto_capable = crypto_capable; > table->nr_entries++; > } > > void __init e820__range_add(u64 start, u64 size, enum e820_type type) > { > - __e820__range_add(e820_table, start, size, type); > + __e820__range_add(e820_table, start, size, type, 0); > } > > static void __init e820_print_type(enum e820_type type) > @@ -211,6 +212,8 @@ void __init e820__print_table(char *who) > e820_table->entries[i].addr + e820_table->entries[i].size - 1); > > e820_print_type(e820_table->entries[i].type); > + if (e820_table->entries[i].crypto_capable) > + pr_cont("; crypto-capable"); > pr_cont("\n"); > } > } > @@ -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? > if (last_type != 0) { > new_entries[new_nr_entries].size = change_point[chg_idx]->addr - last_addr; > /* Move forward only if the new size was non-zero: */ > @@ -406,9 +415,12 @@ int __init e820__update_table(struct e820_table *table) > if (current_type != 0) { > new_entries[new_nr_entries].addr = change_point[chg_idx]->addr; > new_entries[new_nr_entries].type = current_type; > + new_entries[new_nr_entries].crypto_capable = current_crypto; > + > last_addr = change_point[chg_idx]->addr; > } > last_type = current_type; > + last_crypto = current_crypto; > } > } > > @@ -459,14 +471,20 @@ static int __init append_e820_table(struct boot_e820_entry *entries, u32 nr_entr > return __append_e820_table(entries, nr_entries); > } > > +/* > + * Update a memory range. > + * > + * If old_type and new_type are the same then ignore the types and > + * just change crypto_capable. > + */ > static u64 __init > -__e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_type old_type, enum e820_type new_type) > +__e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_type old_type, enum e820_type new_type, u8 crypto_capable) Same api comment here. > { > u64 end; > unsigned int i; > u64 real_updated_size = 0; > > - BUG_ON(old_type == new_type); No more bug? > + bool update_crypto = new_type == old_type; if statement? This looks really odd and is easy to overlook. > > if (size > (ULLONG_MAX - start)) > size = ULLONG_MAX - start; > @@ -476,6 +494,8 @@ __e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_ty > e820_print_type(old_type); > pr_cont(" ==> "); > e820_print_type(new_type); > + if (crypto_capable) > + pr_cont("; crypto-capable"); > pr_cont("\n"); > > for (i = 0; i < table->nr_entries; i++) { > @@ -483,22 +503,27 @@ __e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_ty > u64 final_start, final_end; > u64 entry_end; > > - if (entry->type != old_type) > + if (entry->type != old_type && !update_crypto) > continue; > > + if (update_crypto) > + new_type = entry->type; > + > entry_end = entry->addr + entry->size; > > /* Completely covered by new range? */ > if (entry->addr >= start && entry_end <= end) { > entry->type = new_type; > + entry->crypto_capable = crypto_capable; > real_updated_size += entry->size; > continue; > } > > /* New range is completely covered? */ > if (entry->addr < start && entry_end > end) { > - __e820__range_add(table, start, size, new_type); > - __e820__range_add(table, end, entry_end - end, entry->type); > + __e820__range_add(table, start, size, new_type, crypto_capable); > + __e820__range_add(table, end, entry_end - end, > + entry->type, entry->crypto_capable); > entry->size = start - entry->addr; > real_updated_size += size; > continue; > @@ -510,7 +535,8 @@ __e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_ty > if (final_start >= final_end) > continue; > > - __e820__range_add(table, final_start, final_end - final_start, new_type); > + __e820__range_add(table, final_start, final_end - final_start, > + new_type, crypto_capable); > > real_updated_size += final_end - final_start; > > @@ -527,14 +553,19 @@ __e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_ty > return real_updated_size; > } > > +u64 __init e820__range_mark_as_crypto_capable(u64 start, u64 size) > +{ > + return __e820__range_update(e820_table, start, size, 0, 0, true); > +} > + > u64 __init e820__range_update(u64 start, u64 size, enum e820_type old_type, enum e820_type new_type) > { > - return __e820__range_update(e820_table, start, size, old_type, new_type); > + return __e820__range_update(e820_table, start, size, old_type, new_type, false); See, what does "false" here mean? You have to now go look that up. thanks, greg k-h
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. >> 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? >> if (last_type != 0) { >> new_entries[new_nr_entries].size = change_point[chg_idx]->addr - >> last_addr; >> /* Move forward only if the new size was non-zero: */ >> @@ -406,9 +415,12 @@ int __init e820__update_table(struct e820_table >> *table) >> if (current_type != 0) { >> new_entries[new_nr_entries].addr = change_point[chg_idx]->addr; >> new_entries[new_nr_entries].type = current_type; >> + new_entries[new_nr_entries].crypto_capable = current_crypto; >> + >> last_addr = change_point[chg_idx]->addr; >> } >> last_type = current_type; >> + last_crypto = current_crypto; >> } >> } >> >> @@ -459,14 +471,20 @@ static int __init append_e820_table(struct >> boot_e820_entry *entries, u32 nr_entr >> return __append_e820_table(entries, nr_entries); >> } >> >> +/* >> + * Update a memory range. >> + * >> + * If old_type and new_type are the same then ignore the types and >> + * just change crypto_capable. >> + */ >> static u64 __init >> -__e820__range_update(struct e820_table *table, u64 start, u64 size, enum >> e820_type old_type, enum e820_type new_type) >> +__e820__range_update(struct e820_table *table, u64 start, u64 size, enum >> e820_type old_type, enum e820_type new_type, u8 crypto_capable) > > Same api comment here. > >> { >> u64 end; >> unsigned int i; >> u64 real_updated_size = 0; >> >> - BUG_ON(old_type == new_type); > > No more bug? > No, >> + bool update_crypto = new_type == old_type; > > if statement? This looks really odd and is easy to overlook. > because now I'm using those cases where new_type is equal to old_type to change crypto_capable, because I don't care about the type if I want to change crypto_capable. This is what I'm checking here, to ignore the types inside the for and just leave them as they are. I'm not really happy with this change, maybe a new function will be better, although it will be pretty similar to this one and its implementation is quite fragile, for checking the cases and adding/removing the regions, so I decided to just have only one ugly fragile function, instead of two fragile functions. I can do the new function if you prefer. >> >> if (size > (ULLONG_MAX - start)) >> size = ULLONG_MAX - start; >> @@ -476,6 +494,8 @@ __e820__range_update(struct e820_table *table, u64 >> start, u64 size, enum e820_ty >> e820_print_type(old_type); >> pr_cont(" ==> "); >> e820_print_type(new_type); >> + if (crypto_capable) >> + pr_cont("; crypto-capable"); >> pr_cont("\n"); >> >> for (i = 0; i < table->nr_entries; i++) { >> @@ -483,22 +503,27 @@ __e820__range_update(struct e820_table *table, u64 >> start, u64 size, enum e820_ty >> u64 final_start, final_end; >> u64 entry_end; >> >> - if (entry->type != old_type) >> + if (entry->type != old_type && !update_crypto) >> continue; >> >> + if (update_crypto) >> + new_type = entry->type; >> + >> entry_end = entry->addr + entry->size; >> >> /* Completely covered by new range? */ >> if (entry->addr >= start && entry_end <= end) { >> entry->type = new_type; >> + entry->crypto_capable = crypto_capable; >> real_updated_size += entry->size; >> continue; >> } >> >> /* New range is completely covered? */ >> if (entry->addr < start && entry_end > end) { >> - __e820__range_add(table, start, size, new_type); >> - __e820__range_add(table, end, entry_end - end, entry->type); >> + __e820__range_add(table, start, size, new_type, crypto_capable); >> + __e820__range_add(table, end, entry_end - end, >> + entry->type, entry->crypto_capable); >> entry->size = start - entry->addr; >> real_updated_size += size; >> continue; >> @@ -510,7 +535,8 @@ __e820__range_update(struct e820_table *table, u64 >> start, u64 size, enum e820_ty >> if (final_start >= final_end) >> continue; >> >> - __e820__range_add(table, final_start, final_end - final_start, >> new_type); >> + __e820__range_add(table, final_start, final_end - final_start, >> + new_type, crypto_capable); >> >> real_updated_size += final_end - final_start; >> >> @@ -527,14 +553,19 @@ __e820__range_update(struct e820_table *table, u64 >> start, u64 size, enum e820_ty >> return real_updated_size; >> } >> >> +u64 __init e820__range_mark_as_crypto_capable(u64 start, u64 size) >> +{ >> + return __e820__range_update(e820_table, start, size, 0, 0, true); >> +} >> + >> u64 __init e820__range_update(u64 start, u64 size, enum e820_type >> old_type, enum e820_type new_type) >> { >> - return __e820__range_update(e820_table, start, size, old_type, >> new_type); >> + return __e820__range_update(e820_table, start, size, old_type, new_type, >> false); > > See, what does "false" here mean? You have to now go look that up. > > thanks, > > greg k-h >
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
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.
diff --git a/arch/x86/include/asm/e820/api.h b/arch/x86/include/asm/e820/api.h index e8f58ddd06d9..677dcbabcc8b 100644 --- a/arch/x86/include/asm/e820/api.h +++ b/arch/x86/include/asm/e820/api.h @@ -17,6 +17,7 @@ extern bool e820__mapped_all(u64 start, u64 end, enum e820_type type); extern void e820__range_add (u64 start, u64 size, enum e820_type type); extern u64 e820__range_update(u64 start, u64 size, enum e820_type old_type, enum e820_type new_type); extern u64 e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool check_type); +extern u64 e820__range_mark_as_crypto_capable(u64 start, u64 size); extern void e820__print_table(char *who); extern int e820__update_table(struct e820_table *table); 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; } __attribute__((packed)); /* 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) { int x = table->nr_entries; @@ -176,12 +176,13 @@ static void __init __e820__range_add(struct e820_table *table, u64 start, u64 si table->entries[x].addr = start; table->entries[x].size = size; table->entries[x].type = type; + table->entries[x].crypto_capable = crypto_capable; table->nr_entries++; } void __init e820__range_add(u64 start, u64 size, enum e820_type type) { - __e820__range_add(e820_table, start, size, type); + __e820__range_add(e820_table, start, size, type, 0); } static void __init e820_print_type(enum e820_type type) @@ -211,6 +212,8 @@ void __init e820__print_table(char *who) e820_table->entries[i].addr + e820_table->entries[i].size - 1); e820_print_type(e820_table->entries[i].type); + if (e820_table->entries[i].crypto_capable) + pr_cont("; crypto-capable"); pr_cont("\n"); } } @@ -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; 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)) { if (last_type != 0) { new_entries[new_nr_entries].size = change_point[chg_idx]->addr - last_addr; /* Move forward only if the new size was non-zero: */ @@ -406,9 +415,12 @@ int __init e820__update_table(struct e820_table *table) if (current_type != 0) { new_entries[new_nr_entries].addr = change_point[chg_idx]->addr; new_entries[new_nr_entries].type = current_type; + new_entries[new_nr_entries].crypto_capable = current_crypto; + last_addr = change_point[chg_idx]->addr; } last_type = current_type; + last_crypto = current_crypto; } } @@ -459,14 +471,20 @@ static int __init append_e820_table(struct boot_e820_entry *entries, u32 nr_entr return __append_e820_table(entries, nr_entries); } +/* + * Update a memory range. + * + * If old_type and new_type are the same then ignore the types and + * just change crypto_capable. + */ static u64 __init -__e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_type old_type, enum e820_type new_type) +__e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_type old_type, enum e820_type new_type, u8 crypto_capable) { u64 end; unsigned int i; u64 real_updated_size = 0; - BUG_ON(old_type == new_type); + bool update_crypto = new_type == old_type; if (size > (ULLONG_MAX - start)) size = ULLONG_MAX - start; @@ -476,6 +494,8 @@ __e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_ty e820_print_type(old_type); pr_cont(" ==> "); e820_print_type(new_type); + if (crypto_capable) + pr_cont("; crypto-capable"); pr_cont("\n"); for (i = 0; i < table->nr_entries; i++) { @@ -483,22 +503,27 @@ __e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_ty u64 final_start, final_end; u64 entry_end; - if (entry->type != old_type) + if (entry->type != old_type && !update_crypto) continue; + if (update_crypto) + new_type = entry->type; + entry_end = entry->addr + entry->size; /* Completely covered by new range? */ if (entry->addr >= start && entry_end <= end) { entry->type = new_type; + entry->crypto_capable = crypto_capable; real_updated_size += entry->size; continue; } /* New range is completely covered? */ if (entry->addr < start && entry_end > end) { - __e820__range_add(table, start, size, new_type); - __e820__range_add(table, end, entry_end - end, entry->type); + __e820__range_add(table, start, size, new_type, crypto_capable); + __e820__range_add(table, end, entry_end - end, + entry->type, entry->crypto_capable); entry->size = start - entry->addr; real_updated_size += size; continue; @@ -510,7 +535,8 @@ __e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_ty if (final_start >= final_end) continue; - __e820__range_add(table, final_start, final_end - final_start, new_type); + __e820__range_add(table, final_start, final_end - final_start, + new_type, crypto_capable); real_updated_size += final_end - final_start; @@ -527,14 +553,19 @@ __e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_ty return real_updated_size; } +u64 __init e820__range_mark_as_crypto_capable(u64 start, u64 size) +{ + return __e820__range_update(e820_table, start, size, 0, 0, true); +} + u64 __init e820__range_update(u64 start, u64 size, enum e820_type old_type, enum e820_type new_type) { - return __e820__range_update(e820_table, start, size, old_type, new_type); + return __e820__range_update(e820_table, start, size, old_type, new_type, false); } static u64 __init e820__range_update_kexec(u64 start, u64 size, enum e820_type old_type, enum e820_type new_type) { - return __e820__range_update(e820_table_kexec, start, size, old_type, new_type); + return __e820__range_update(e820_table_kexec, start, size, old_type, new_type, false); } /* Remove a range of memory from the E820 table: */ @@ -572,7 +603,9 @@ u64 __init e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool /* Is the new range completely covered? */ if (entry->addr < start && entry_end > end) { - e820__range_add(end, entry_end - end, entry->type); + __e820__range_add(e820_table, end, entry_end - end, + entry->type, entry->crypto_capable); + entry->size = start - entry->addr; real_removed_size += size; continue; @@ -1322,6 +1355,8 @@ void __init e820__memblock_setup(void) continue; memblock_add(entry->addr, entry->size); + if (entry->crypto_capable) + memblock_mark_crypto_capable(entry->addr, entry->size); } /* Throw away partial pages: */
Add a new member in e820_entry to hold whether an entry is able to do hardware memory encryption or not. Add a new argument to __e820__range_add to accept this new crypto_capable. Add a new argument to __e820__update_range to be able to change a region's crypto_capable member. Also, change its behavior a little, before if you wanted to update a region with its same type it was a BUG_ON; now if you call it with both old_type and new_type equals, then the function won't change the types, just crypto_capable. Change e820__update_table to handle merging and overlap problems taking into account crypto_capable. Add a function to mark a range as crypto, using __e820__range_update in the background. This will be called when initializing EFI. Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com> --- arch/x86/include/asm/e820/api.h | 1 + arch/x86/include/asm/e820/types.h | 1 + arch/x86/kernel/e820.c | 59 ++++++++++++++++++++++++------- 3 files changed, 49 insertions(+), 12 deletions(-)