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
144

This should be in a helper function.

228–230

See note above about a helper.

274

Perhaps put this under bootverbose.

410

Some debugging left ?

433

And this.

604

And this.

sys/dev/efidev/efirt.c
274

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

sys/dev/efidev/efirt.c
274

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
274

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

143

Extra blank line.

151

Another extra.

274

I find addresses are useful too.

sys/dev/efidev/efirt.c
165

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
173

No need for ().

178

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 ?

281

I think %p would be a better format specifier.

sys/dev/efidev/efirt.c
173

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

178

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.

281

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
150

return (efihdr != NULL);

275

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
137

efi_map_bsearch_compare()

139

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

202

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.

206

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
161

trailing ws

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