riscv: eliminate physmap global
ClosedPublic
Actions

Authored by mhorne on Apr 4 2022, 5:51 PM.

Details

Reviewers

markj
jrtc27

Commits

rGaec1262c5ef0: riscv: eliminate physmap global
rG8a0339e679aa: riscv: eliminate physmap global

Summary

Since physical memory management is now handled by subr_physmem.c, the
need to keep this global array has diminished. It is not referenced
outside of early boot-time, and is populated by physmem_avail() in
pmap_bootstrap(). Just allocate the array on the stack for the duration
of its lifetime.

The check against physmap[0] in initriscv() can be dropped altogether,
as there is no consequence for excluding a memory range twice.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 44996
Build 41884: arc lint + arc unit

Event Timeline

mhorne created this revision.Apr 4 2022, 5:51 PM

Herald added a subscriber: imp. · View Herald TranscriptApr 4 2022, 5:51 PM

mhorne requested review of this revision.Apr 4 2022, 5:51 PM

Harbormaster completed remote builds in B44996: Diff 104578.Apr 4 2022, 5:51 PM

mhorne added inline comments.Apr 4 2022, 5:55 PM

sys/riscv/riscv/pmap.c
654–665	It seems like we could eliminate this block, but I need to check the guarantees of `physmem_avail()`. But I believe it should become: min_pa = physmap[0]; max_pa = physmap[physmap_idx]; where we don't divide `physmap_idx` by two.

markj added inline comments.Apr 6 2022, 3:04 PM

sys/riscv/riscv/machdep.c
561	Last time I looked at this code, I wondered why we do this after pmap_bootstrap(). Isn't it possible that pmap_bootstrap_dmap() will use pages below 2MB for the direct map?

mhorne added inline comments.Apr 6 2022, 4:35 PM

sys/riscv/riscv/machdep.c
561	This is a good question, and I'm not sure now if there was a reason. At the moment `pmap_bootstrap_dmap()` is quite simple and will construct the dmap using 1G superpages only, not really respecting any excluded ranges. So it will always map pages below 2MB, but never use them. So for now, I think it is incorrect but inconsequential that this is called after `pmap_bootstrap()`. I have some half-baked patches to fix this.
561	And by 'fix this', I mean make the dmap construction smarter.

markj accepted this revision.Apr 6 2022, 5:42 PM

markj added inline comments.

sys/riscv/riscv/machdep.c
561	Oh, my mistake is that I read pmap_bootstrap() as allocating page table pages themselves starting from `min_pa`. I think it's probably okay to map the bottom 2MB. (Though, I believe we effectively ignore the "no-map" attribute in device trees, which as I understand it means "don't map this physical memory region at all.")
sys/riscv/riscv/pmap.c
654–665	I think you're right, but we should probably map each region separately rather than assuming that the physical address space doesn't contain large holes. That is, the existing code is a bit simplistic. Maybe that's what you mean by "make dmap construction smarter"? (And if we don't expect the PA space to ever contain large holes, riscv's vmparam.h should define VM_PHYSSEG_DENSE rather than VM_PHYSSEG_SPARSE. The former enables a cheaper implementation of PHYS_TO_VM_PAGE(). But I doubt we can make this assertion about all riscv platforms.)

This revision is now accepted and ready to land.Apr 6 2022, 5:42 PM

mhorne added inline comments.Apr 6 2022, 6:06 PM

sys/riscv/riscv/pmap.c
654–665	We definitely expect a segmented physical address space, I have seen it already with the beagle-v. And yes, most likely we will end up iterating over/passing the physmap array directly to `pmap_bootstrap_dmap()`.