Page MenuHomeFreeBSD

efirt: Add efi_memory_attribute
Needs ReviewPublic

Authored by manu on May 22 2019, 5:06 AM.

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
Unit Tests Skipped

Event Timeline

sys/dev/efidev/efirt.c
147

This should be in a helper function.

236–238

See note above about a helper.

277

Perhaps put this under bootverbose.

413

Some debugging left ?

436

And this.

607

And this.

sys/dev/efidev/efirt.c
277

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

sys/dev/efidev/efirt.c
277

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

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

manu added inline comments.
sys/dev/efidev/efirt.c
277

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

sys/dev/efidev/efirt.c
62

You do not need = NULL;.

146

Extra blank line.

154

Another extra.

277

I find addresses are useful too.

sys/dev/efidev/efirt.c
168

It looks like this could be replaced with

roundup2(sizeof(struct efi_map_header), 16)

manu marked 5 inline comments as done.
sys/dev/efidev/efirt.c
175

No need for ().

180

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 ?

283

I think %p would be a better format specifier.

sys/dev/efidev/efirt.c
175

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

180

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.

283

Ok, I'll change that.

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 added inline comments.
sys/dev/efidev/efirt.c
153

return (efihdr != NULL);

278

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

return (0) -> return (false)

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.

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.

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.

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.

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.

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

sys/dev/efidev/efirt.c
140

efi_map_bsearch_compare()

142

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

205

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.

209

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.

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
164

trailing ws

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