Page MenuHomeFreeBSD

Clean up initialization of phys_avail.
AcceptedPublic

Authored by markj on Thu, Dec 6, 10:01 PM.

Details

Reviewers
br
jhb
Summary
  • Get rid of kern_delta. Its definition makes no sense: KERNBASE is a virtual address and kernstart is a physical address.
  • Don't use avail_slot to index into physmap[], that just appears to be bug.
  • Simplify the first stage of phys_avail[] initialization by avoiding the extra external if statement.
  • Clean up and fix some comments for the second stage of phys_avail[] initialization.
  • Rmeove some dead stores.
Test Plan

Booted under QEMU and spike. Used EARLY_PRINTF to verify
some values.

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 21418
Build 20740: arc lint + arc unit

Event Timeline

markj created this revision.Thu, Dec 6, 10:01 PM
markj edited the test plan for this revision. (Show Details)Thu, Dec 6, 10:03 PM
markj added reviewers: br, jhb.
jhb added a comment.Thu, Dec 6, 11:08 PM

This is inspired by PR 231515?

Also, I wonder if in general it would be simpler if we had a single loop to populate phys_avail[] at the end that just split regions as necessary to skip over '[kernstart, freemempos (but PA instead of VA)]'?

sys/riscv/riscv/pmap.c
578–579

I think used_map_slot can be removed. I think the result of these lines is:

used_map_slot = map_slot;

/* Various bits that don't change map_slot. */

map_slot = used_map_slot;
579

What is the purpose of this section of code? The loop that walks L2 slots results in 'va' being set, but then 'va' isn't used in the rest of the function AFAICT. The old version set 'pa' that wasn't used either. The arm64 pmap_bootstrap() uses 'va' and 'pa' in a call to pmap_bootstrap_l2() before the call to pmap_bootstrap_l3(), but riscv doesn't have a pmap_bootstrap_l2()?

markj updated this revision to Diff 51699.Fri, Dec 7, 2:21 AM
markj marked 3 inline comments as done.
  • Further cleanup based on feedback.
markj added a comment.Fri, Dec 7, 2:21 AM
In D18464#393352, @jhb wrote:

This is inspired by PR 231515?

Yes.

> Also, I wonder if in general it would be simpler if we had a single loop to populate phys_avail[] at the end that just split regions as necessary to skip over '[kernstart, freemempos (but PA instead of VA)]'?

It seems to be simpler, I did that in the latest upload.

sys/riscv/riscv/pmap.c
579

I agree that it doesn't do anything, so I removed it for now.

jhb accepted this revision.Thu, Dec 13, 10:03 PM

Sorry, I missed the last upload of this.

This revision is now accepted and ready to land.Thu, Dec 13, 10:03 PM