Page MenuHomeFreeBSD
Differential D20347

efirt: Add efi_memory_attribute
AbandonedPublic
Actions

Authored by manu on May 22 2019, 5:06 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 14, 12:23 PM
Unknown Object (File)
Thu, Nov 14, 12:23 PM
Unknown Object (File)
Sat, Nov 9, 6:51 PM
Unknown Object (File)
Thu, Nov 7, 2:48 PM
Unknown Object (File)
Wed, Nov 6, 2:59 PM
Unknown Object (File)
Tue, Nov 5, 3:14 PM
Unknown Object (File)
Sat, Nov 2, 4:44 AM
Unknown Object (File)
Mon, Oct 28, 11:24 AM
View All Files
Subscribers
emaste
imp
tuexen
val_packett.cool

Details

Reviewers
kib
andrew
Group Reviewers
arm64
Summary

This function looks in the efi memory map for a physical address
and returns the attribute.
This is needed when we want to correctly map some region.

Sponsored by: Ampere Computing, LLC

Test Plan

On the Ampere eMAG where there is ~150 efi region in the map this is the distribution or the bsearch rounds :

37 num=2
87 num=7
24 num=8
37 num=9

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

manu created this revision.May 22 2019, 5:06 AM
Herald added a subscriber: imp. · View Herald TranscriptMay 22 2019, 5:06 AM
manu added a reviewer: andrew.May 22 2019, 5:07 AM
manu added a child revision: D20348: arm64: pmap: Try to find the correct attribute.
kib added inline comments.May 22 2019, 8:59 AM
sys/dev/efidev/efirt.c
148

This should be in a helper function.

266–268

See note above about a helper.

308

Perhaps put this under bootverbose.

444

Some debugging left ?

467

And this.

638

And this.

manu added inline comments.May 22 2019, 9:08 AM
sys/dev/efidev/efirt.c
308

Sorry it's not needed I just commited part of some debug code by mistake, will remove it.

kib added inline comments.May 22 2019, 9:20 AM
sys/dev/efidev/efirt.c
308

I find it useful, but it should not clutter normal boot messages.

manu updated this revision to Diff 57686.May 22 2019, 10:14 AM

Add helper function that get the memory map from efi.
Remove some debugs printf that weren't supposed to be there.

manu marked 6 inline comments as done.May 22 2019, 10:15 AM
manu added inline comments.
sys/dev/efidev/efirt.c
308

Ok I'll leave this line and remove the ones that prints the runtime function addresses (unless you want those too).

kib added inline comments.May 22 2019, 10:26 AM
sys/dev/efidev/efirt.c
63

You do not need = NULL;.

147

Extra blank line.

155

Another extra.

308

I find addresses are useful too.

andrew added inline comments.May 22 2019, 11:13 AM
sys/dev/efidev/efirt.c
169

It looks like this could be replaced with

roundup2(sizeof(struct efi_map_header), 16)

emaste added a subscriber: emaste.May 22 2019, 1:54 PM
manu updated this revision to Diff 57746.May 23 2019, 3:00 AM
manu marked 5 inline comments as done.
kib added inline comments.May 23 2019, 7:23 PM
sys/dev/efidev/efirt.c
176

No need for ().

181

Is it reasonable to return 0 if the address is not covered by the map ?

Could EFI_MEMORY_UC be a better return value in this case ?

314

I think %p would be a better format specifier.

manu added inline comments.May 23 2019, 7:35 PM
sys/dev/efidev/efirt.c
176

Indeed, that was just a copy/paste from the old code.

181

This is something I'm really not sure about.
We really need to have a consistent way of returning the attr for every arch, maybe it would be better to just return 0/-1 and write the attr in a pointer passed as arg. Then the caller could do different action based on the arch.

314

Ok, I'll change that.

manu updated this revision to Diff 57952.May 27 2019, 2:22 PM

Address kib comments.
Note that based on some comment on D20348 I might change this revision so efi_memory_attribute won't use the efi map directly.

kib accepted this revision.May 27 2019, 4:20 PM
kib added inline comments.
sys/dev/efidev/efirt.c
154

return (efihdr != NULL);

309

The format specifiers there are still very LP64-specific. I would use %jx/uintmax_t for all non-pointers.

This revision is now accepted and ready to land.May 27 2019, 4:20 PM
manu updated this revision to Diff 58045.May 29 2019, 3:14 PM
manu edited the test plan for this revision. (Show Details)

Use bsearch and address kib's comments.

This revision now requires review to proceed.May 29 2019, 3:14 PM
manu marked 5 inline comments as done.May 29 2019, 3:14 PM
manu updated this revision to Diff 58048.May 29 2019, 4:14 PM

return (0) -> return (false)

kib added a comment.May 29 2019, 7:53 PM

This all looks fine, except one detail. I reviewed UEFI 2.8 description of EFI_BOOT_SERVICES.GetMemoryMap() but did not found a mention that they require the map ordered by phys address. Did you miss the code to sort the map ?

For the task of DMAP creation with correct attributes, I definitely do not want to call efi_memory_attribute() for each 4K page. Perhaps add optional out argument which returns amount of memory with the same attribute as pa, after pa.

manu added a comment.May 30 2019, 11:17 AM
In D20347#441757, @kib wrote:

This all looks fine, except one detail. I reviewed UEFI 2.8 description of EFI_BOOT_SERVICES.GetMemoryMap() but did not found a mention that they require the map ordered by phys address. Did you miss the code to sort the map ?

In chapter 4.6, EFI_MEMORY_ATTRIBUTES_TABLE it is said : The list must be sorted by physical start address in ascending order.

For the task of DMAP creation with correct attributes, I definitely do not want to call efi_memory_attribute() for each 4K page. Perhaps add optional out argument which returns amount of memory with the same attribute as pa, after pa.

Ok I'll do that.

kib added a comment.May 30 2019, 11:39 AM
In D20347#441878, @manu wrote:
In D20347#441757, @kib wrote:

This all looks fine, except one detail. I reviewed UEFI 2.8 description of EFI_BOOT_SERVICES.GetMemoryMap() but did not found a mention that they require the map ordered by phys address. Did you miss the code to sort the map ?

In chapter 4.6, EFI_MEMORY_ATTRIBUTES_TABLE it is said : The list must be sorted by physical start address in ascending order.

I am even more confused. The description said that about EFI_MEMORY_ATTRIBUTE, not about memory map. And you correctly use memory map, because MEMORY_ATTRIBUTE seems to be some after-thought patch only applicable to EFI runtime code and data.

manu updated this revision to Diff 58107.May 31 2019, 8:32 AM

qsort the efi map
If out is supplied, search for the last page with the same attributes as the requested out and write the address in out.

manu added a comment.May 31 2019, 8:32 AM
In D20347#441879, @kib wrote:
In D20347#441878, @manu wrote:
In D20347#441757, @kib wrote:

This all looks fine, except one detail. I reviewed UEFI 2.8 description of EFI_BOOT_SERVICES.GetMemoryMap() but did not found a mention that they require the map ordered by phys address. Did you miss the code to sort the map ?

In chapter 4.6, EFI_MEMORY_ATTRIBUTES_TABLE it is said : The list must be sorted by physical start address in ascending order.

I am even more confused. The description said that about EFI_MEMORY_ATTRIBUTE, not about memory map. And you correctly use memory map, because MEMORY_ATTRIBUTE seems to be some after-thought patch only applicable to EFI runtime code and data.

Yeah sorry, I got confused and thought that this was describing the same map.

manu updated this revision to Diff 58108.May 31 2019, 9:15 AM

Forgot to write to out if no other pages were found.

kib added inline comments.Jun 1 2019, 11:47 AM
sys/dev/efidev/efirt.c
141

efi_map_bsearch_compare()

143

I believe you should use vm_paddr_t there. You pass a pointer to vm_paddr_t, which is uint64_t only by coincidense.

206

I think that the semantic of *out should be 'a byte after the end of the segment'.

If we assume that pa is exactly equal to the p->md_phys, then this means that -1 is not needed. If pa is greater than md_phys, then you need to adjust the formula to correctly return number of pages left after pa in the segment.

210

Why do you need bsearch ? After the map was sorted, we know for sure that the next segment in phys address order starts after the current one. You should check for a gap and last segment, but otherwise you could use the next segment without looking it up.

emaste added a comment.Jun 4 2019, 5:43 PM

Tested on X86? I had an earlier version of this in my WIP tree for testing on eMAG/ThunderX2 (where it worked) but encountered a panic on x86. I do have some other WIP in that tree though.

https://cirrus-ci.com/task/5146266359562240

KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xffffffff824d98e0
vpanic() at vpanic+0x19d/frame 0xffffffff824d9930
panic() at panic+0x43/frame 0xffffffff824d9990
trap_fatal() at trap_fatal+0x394/frame 0xffffffff824d99f0
trap_pfault() at trap_pfault+0x62/frame 0xffffffff824d9a40
trap() at trap+0x562/frame 0xffffffff824d9b50
calltrap() at calltrap+0x8/frame 0xffffffff824d9b50
--- trap 0xc, rip = 0xffffffff80605115, rsp = 0xffffffff824d9c20, rbp = 0xffffffff824d9c30 ---
efirt_modevents() at efirt_modevents+0x355/frame 0xffffffff824d9c30
module_register_init() at module_register_init+0xc6/frame 0xffffffff824d9c60
mi_startup() at mi_startup+0x216/frame 0xffffffff824d9cb0
btext() at btext+0x2c
KDB: enter: panic
[ thread pid 0 tid 100000 ]
Stopped at      kdb_enter+0x3b: movq    $0,kdb_why
db> qemu-system-x86_64: terminating on signal 15 from pid 52138 (timeout)
Did not boot successfully, see /tmp/ci-qemu-test-boot.log
sys/dev/efidev/efirt.c
165

trailing ws

val_packett.cool added a subscriber: val_packett.cool.Jul 24 2019, 4:22 PM
tuexen added a subscriber: tuexen.EditedAug 26 2019, 6:21 PM

Is it possible to get this in head? It seems to be needed to run FreeBSD on Ampere systems...

manu abandoned this revision.Aug 16 2022, 9:13 AM

So long ago and I don't know if this is still really needed as we boot fine on Ampere machines.

Revision Contents
Changeset List

PathSize
sys/
dev/
efidev/
126 lines
sys/
1 line

Diff 58107

View Options

sys/dev/efidev/efirt.c

Loading...
View Options

sys/sys/efi.h

Loading...