efirt: Add efi_memory_attribute

[manu]

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

[kib]

sys/dev/efidev/efirt.c

148

This should be in a helper function.

267–269

See note above about a helper.

309

Perhaps put this under bootverbose.

445

Some debugging left ?

468

And this.

639

And this.

[manu]

sys/dev/efidev/efirt.c

309

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

[kib]

sys/dev/efidev/efirt.c

309

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

[manu]

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

[manu]

sys/dev/efidev/efirt.c

309

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

[kib]

sys/dev/efidev/efirt.c

63

You do not need = NULL;.

147

Extra blank line.

155

Another extra.

309

I find addresses are useful too.

[andrew]

sys/dev/efidev/efirt.c

169

It looks like this could be replaced with

roundup2(sizeof(struct efi_map_header), 16)

[kib]

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 ?

315

I think %p would be a better format specifier.

[manu]

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.

315

Ok, I'll change that.

[manu]

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]

sys/dev/efidev/efirt.c

154

return (efihdr != NULL);

310

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

[manu]

Use bsearch and address kib's comments.

[manu]

return (0) -> return (false)

[kib]

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]

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]

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]

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]

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]

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

[kib]

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]

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

[tuexen]

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

[manu]

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