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.
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
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 | 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 | This doesn't look right. What will happen if we have a mapping open across setting vm_initialized? | |
| 4442 | You should call kva_free after removing the maps, another CPU may allocate the same virtual address creating a race. | |
| sys/arm64/arm64/pmap.c | ||
|---|---|---|
| 4343 | 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 | 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 | 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. | |
Comment Actions
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.
Comment Actions
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 | ||
|---|---|---|
| 4440 | I'd prefer we remove the mapping here. It's undefined behaviour to map the same physical address twice with different attributes. | |