Page MenuHomeFreeBSD

Map ACPI memory in ARM64 pmap
ClosedPublic

Authored by mst_semihalf.com on Apr 13 2018, 5:54 PM.

Details

Summary

Previously DMAP VA range was used for this purpose but the address range of ACPI tables may not be covered by DMAP.
This change is a prerequisite before switching to L2-block based DMAP.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Can you provide more context. If you are uploading a patch via the web interface there are instructions on the wiki to ensure this is the case at https://wiki.freebsd.org/Phabricator#Create_a_Revision_via_Web_Interface

sys/arm64/arm64/pmap.c
4343 ↗(On Diff #41437)

Does this rely on the change to pmap_bootstrap_dmap? If not we should split this into two patches. One for pmap_mapbios/pmap_unmapbios and any related changes, and another to further restrict the DMAP region.

I'd prefer to handle the DMAP change as it will need acknowledgements to the funding agency the work was carried out under.

4437 ↗(On Diff #41437)

This doesn't look right. What will happen if we have a mapping open across setting vm_initialized?

4442 ↗(On Diff #41437)

You should call kva_free after removing the maps, another CPU may allocate the same virtual address creating a race.

Can you provide more context. If you are uploading a patch via the web interface there are instructions on the wiki to ensure this is the case at https://wiki.freebsd.org/Phabricator#Create_a_Revision_via_Web_Interface

Sorry, wrong option during export, I will upload again.

sys/arm64/arm64/pmap.c
4343 ↗(On Diff #41437)

Yes, I think we can split it. I'll remove pmap_bootstrap_dmap changes from this patch so that you can commit it separately after mapbios/unmapbios changes have gone in.

sys/arm64/arm64/pmap.c
4442 ↗(On Diff #41437)

Good point, I'll reverse the order. I wonder if it's necessary to remove the maps at all - on amd64 for example there's only kva_free() and return.

sys/arm64/arm64/pmap.c
4437 ↗(On Diff #41437)

That's a caveat I forgot to mention - it wouldn't work if the mapping is held for so long. I wanted to keep the code simple and didn't see such usage of mapbios currently in the kernel so I concluded it would be invalid to use this interface in this way. Also there's one more problem with holding a mapping across setting vm_initialized. If the mapped physical memory is included in phys_avail (shouldn't be the case for ACPI tables but that's solely dependent on what EFI provides) then 'vm_initialized=1' means VM has already added it to its pool and may overwrite data in those pages at any time. The pre-init mapping is no longer valid in such a case, when vm_initialized is set - which is done in pmap_init.

To add support for mappings held across VM initialization while keeping it simple, I suppose it would be enough to store original PA and size in the pre-init array, maybe increasing the size of the array, and always map new L2 block each time. This could lead to duplicate mappings (two VA blocks -> same PA block) but I think arm64 mandates PIPT d-caches so it shouldn't be a problem.
I can implement this if you think we should support it. That said, the caveat with VM possibly reclaiming the physical memory in between mapbios/unmapbios calls would still hold (however unlikely).

mst_semihalf.com edited the summary of this revision. (Show Details)

I wrote an alternative, more complete implementation of mapbios/unmapbios functions without any of the previous caveats.
Of course the code got a little more complicated but I think it's still comprehensible.
@andrew let me know what you think.

I also removed the changes to pmap_bootstrap_dmap as @andrew will commit it separately, after this patch.

Add fix for race condition in pmap_unmapbios() which I forgot to add earlier.

Can you update the proposed commit message. I'm happy with the change, other than we should be clearing the mapping in pmap_unmapbios.

sys/arm64/arm64/pmap.c
4392 ↗(On Diff #41943)

I'd prefer we remove the mapping here. It's undefined behaviour to map the same physical address twice with different attributes.

mst_semihalf.com retitled this revision from Create L2 DMAP pages on ARM64 to Map ACPI memory in ARM64 pmap.
mst_semihalf.com edited the summary of this revision. (Show Details)

Clear the mapping in pmap_unmapbios as suggested.

This revision was not accepted when it landed; it landed in state Needs Review.May 22 2018, 11:16 AM
This revision was automatically updated to reflect the committed changes.