Page MenuHomeFreeBSD

kern: physmem: add new KPIs for amd64
AcceptedPublic

Authored by kevans on Nov 6 2021, 7:28 PM.
Tags
None
Referenced Files
F103016226: D32874.diff
Tue, Nov 19, 8:56 PM
Unknown Object (File)
Tue, Nov 5, 4:39 AM
Unknown Object (File)
Sat, Nov 2, 4:52 AM
Unknown Object (File)
Sat, Oct 26, 7:03 AM
Unknown Object (File)
Sat, Oct 26, 7:03 AM
Unknown Object (File)
Sat, Oct 26, 7:02 AM
Unknown Object (File)
Sat, Oct 26, 7:02 AM
Unknown Object (File)
Sat, Oct 26, 6:44 AM
Subscribers

Details

Reviewers
kib
markj
Summary

amd64 needs to calculate Maxmem earlier than we init globals, so provide
a KPI to do that. The value it returns is technically inaccurate
because it doesn't take into account excluded regions, but it's a close
enough heuristic for our purposes. It will be readjusted after placing
further constraints on phys_avail once that's been populated, anyways.

amd64 may also be configured to do a memtest on startup, so provide a
page iterator. The iterator function takes flags so that it can operate
over any of: avail pages, noalloc pages, nodump pages, but amd64 will
only use it for iterating over avail pages. We cannot allow mutation of
the region set we're iterating over, but we can allow mutation of the
other region set.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 42637
Build 39525: arc lint + arc unit

Event Timeline

sys/kern/subr_physmem.c
501

return ()

What are assumptions about hwregions[] ? It is ordered by addr, addr + size < next->addr, what else? I suggest to add some checker that verifies the assumptions, and is executed after each op in DIAGNOSTICS builds.

542

So if you have partial page at the end, is it possible that we never iterate over it?

Is it needed for this KPI to operate on partial pages, at all?

sys/kern/subr_physmem.c
501

OK, will do; yeah:

  1. Ordered by addr
  2. addr + size < next->addr
  3. Strictly flags == 0 for hwregions, flags != 0 for exregions
542

Right, so in hindsight, this behavior is correct for hwregions but probably wrong for exregions based on how amd64 was previously building phys_avail. You don't want the partial page for hwregions, but I'd think you do want the partial page (presented as a full page) for exregions.

IMO if we end up keeping the 'chop off partial pages into phys_avail' logic, then we have no need for partial pages here. As I write all of this, though, I think i'm convinced that it'd be better to include the partial pages and just ignore them in amd64's memtest. I can drop a comment somewhere to explain partial pages and how they come into play (or don't) in the final phys_avail/dump_avail.

Parenthesis and DIAGNOSTIC check

The page iterator will now return partial pages at the front and back so that
caller gets a full view; it may ignore partial page at its discretion, of course.

sys/kern/subr_physmem.c
166

Does it make sense to check for overflow, i.e. assert that start < end?

171

This is same as need_flags ^ (flags == 0) but it might be somewhat obscure.

193

If you checked for overflow, then it is enough to assert that pend < start

200

I am confused by this. I would think that you allow pend <= start for pidx == idx - 1.

kevans added inline comments.
sys/kern/subr_physmem.c
200

For non-matching flags we can only check the sort criteria; a previous non-matching extent could extend beyond the start of this one legally.

kib added inline comments.
sys/kern/subr_physmem.c
171
This revision is now accepted and ready to land.Nov 21 2021, 3:16 PM