Page MenuHomeFreeBSD
Differential D24154

RISC-V: use physmem to manage physical memory
ClosedPublic
Actions

Authored by mhorne on Mar 22 2020, 6:38 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 16, 3:52 PM
Unknown Object (File)
Sat, Apr 12, 7:18 AM
Unknown Object (File)
Sat, Apr 12, 5:05 AM
Unknown Object (File)
Thu, Apr 3, 6:53 PM
Unknown Object (File)
Mar 5 2025, 11:39 AM
Unknown Object (File)
Mar 4 2025, 8:38 PM
Unknown Object (File)
Feb 26 2025, 8:10 PM
Unknown Object (File)
Feb 26 2025, 7:55 PM
View All Files
Subscribers
imp
kp
nick

Details

Reviewers
br
markj
Group Reviewers
riscv
Commits
rS360083: RISC-V: use physmem to manage physical memory
Summary

Replace our hand-rolled functions with the generic ones provided by
kern/subr_physmem.c. This greatly simplifies the initialization of
physical memory regions and kernel globals.

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 30045
Build 27856: arc lint + arc unit

Event Timeline

mhorne created this revision.Mar 22 2020, 6:38 PM
Harbormaster completed remote builds in B30045: Diff 69774.Mar 22 2020, 6:39 PM
mhorne added a parent revision: D24153: Convert arm's physmem interface to MI code.Mar 22 2020, 6:41 PM
mhorne added a child revision: D24155: RISC-V: exclude reserved memory regions.
kp added a subscriber: kp.Mar 27 2020, 5:26 PM

I've found this to fail to boot, because it didn't reserve memory for the DTB (supplied by BBL). I'm not entirely sure if that's a result of an issue with your patch, or if there's something wrong in our local tree.

How is the DTB memory supposed to be reserved?

The following fixes the immediate problem for me:

@@ -806,9 +808,11 @@ initriscv(struct riscv_bootparams *rvbp)
 #ifdef FDT
        try_load_dtb(kmdp, rvbp->dtbp_virt);
 #endif
+       /* Reserve memory for the DTB. */
+       physmem_exclude_region(rvbp->dtbp_phys, L2_SIZE, EXFLAG_NOALLOC);
mhorne added a comment.Mar 27 2020, 5:42 PM
In D24154#532404, @kp wrote:

I've found this to fail to boot, because it didn't reserve memory for the DTB (supplied by BBL). I'm not entirely sure if that's a result of an issue with your patch, or if there's something wrong in our local tree.

How is the DTB memory supposed to be reserved?

The following fixes the immediate problem for me:

@@ -806,9 +808,11 @@ initriscv(struct riscv_bootparams *rvbp)
 #ifdef FDT
        try_load_dtb(kmdp, rvbp->dtbp_virt);
 #endif
+       /* Reserve memory for the DTB. */
+       physmem_exclude_region(rvbp->dtbp_phys, L2_SIZE, EXFLAG_NOALLOC);

Looks like I missed linking one of the parents. This patch also depends on D24152.

mhorne added a parent revision: D24152: RISC-V: copy the DTB to early KVA.Mar 27 2020, 5:42 PM
mhorne updated this revision to Diff 69943.Mar 27 2020, 11:49 PM

Rebase.

Harbormaster completed remote builds in B30144: Diff 69943.Mar 27 2020, 11:49 PM
kp added a comment.Mar 28 2020, 9:43 AM
In D24154#532415, @mhorne wrote:

Looks like I missed linking one of the parents. This patch also depends on D24152.

Thanks! I should have realised. I'd already seen the other patch, but somehow neglected to include it in my tests.

br added inline comments.Mar 30 2020, 4:49 PM
sys/riscv/riscv/machdep.c
856

Do you prefer to use square brackets for a single line statements?

mhorne updated this revision to Diff 70211.Apr 4 2020, 10:11 PM

Rebase

Harbormaster completed remote builds in B30293: Diff 70211.Apr 4 2020, 10:11 PM
mhorne marked an inline comment as done.Apr 4 2020, 10:11 PM
mhorne added a comment.Apr 11 2020, 6:00 PM

Ping. Any opposition to this one? I would like to commit the set of patches soon.

nick added a subscriber: nick.Apr 15 2020, 10:28 PM

These changes look good. I can confirm that this works.

mhorne updated this revision to Diff 70716.Apr 17 2020, 9:27 PM

Keep physmap globals in machdep.c.

Harbormaster completed remote builds in B30571: Diff 70716.Apr 17 2020, 9:27 PM
This revision was not accepted when it landed; it landed in state Needs Review.Apr 19 2020, 12:18 AM
Closed by commit rS360083: RISC-V: use physmem to manage physical memory (authored by mhorne). · Explain Why
This revision was automatically updated to reflect the committed changes.
mhorne added a commit: rS360083: RISC-V: use physmem to manage physical memory.
Herald added a subscriber: imp. · View Herald TranscriptApr 19 2020, 12:18 AM

Revision Contents
Changeset List

PathSize
sys/
conf/
1 line
riscv/
include/
3 lines
riscv/
92 lines
52 lines

Diff 69774

View Options

sys/conf/files.riscv

Show First 20 Lines • Show All 530 Lines • ▼ Show 20 Lines if (vs2 != NULL)
TAILQ_INSERT_BEFORE(vs2, vs, link); TAILQ_INSERT_BEFORE(vs2, vs, link);
else else
TAILQ_INSERT_TAIL(&vnet_constructors, vs, link); TAILQ_INSERT_TAIL(&vnet_constructors, vs, link);
/* /*
* Invoke the constructor on all the existing vnets when it is * Invoke the constructor on all the existing vnets when it is
* registered. * registered.
*/ */
VNET_LIST_RLOCK();
VNET_FOREACH(vnet) { VNET_FOREACH(vnet) {
CURVNET_SET_QUIET(vnet); CURVNET_SET_QUIET(vnet);
vs->func(vs->arg); vs->func(vs->arg);
CURVNET_RESTORE(); CURVNET_RESTORE();
} }
VNET_LIST_RUNLOCK();
VNET_SYSINIT_WUNLOCK(); VNET_SYSINIT_WUNLOCK();
} }
void void
vnet_deregister_sysinit(void *arg) vnet_deregister_sysinit(void *arg)
{ {
struct vnet_sysinit *vs; struct vnet_sysinit *vs;
Show All 34 Linesvnet_deregister_sysuninit(void *arg)
struct vnet *vnet; struct vnet *vnet;
vs = arg; vs = arg;
/* /*
* Invoke the destructor on all the existing vnets when it is * Invoke the destructor on all the existing vnets when it is
* deregistered. * deregistered.
*/ */
VNET_LIST_RLOCK();
zleiUnsubmitted
Not Done
Inline Actions

In vnet_register_sysinit() we have lock order

VNET_SYSINIT_WLOCK();
VNET_LIST_RLOCK;

I think it is best to make lock order consistent with that in vnet_register_sysinit(), although dead lock is not possible.

zlei: In `vnet_register_sysinit()` we have lock order ``` VNET_SYSINIT_WLOCK(); VNET_LIST_RLOCK; ```…
VNET_SYSINIT_WLOCK(); VNET_SYSINIT_WLOCK();
VNET_FOREACH(vnet) { VNET_FOREACH(vnet) {
CURVNET_SET_QUIET(vnet); CURVNET_SET_QUIET(vnet);
vs->func(vs->arg); vs->func(vs->arg);
CURVNET_RESTORE(); CURVNET_RESTORE();
} }
/* Remove the destructor from the global list of vnet destructors. */ /* Remove the destructor from the global list of vnet destructors. */
TAILQ_REMOVE(&vnet_destructors, vs, link); TAILQ_REMOVE(&vnet_destructors, vs, link);
VNET_LIST_RUNLOCK();
VNET_SYSINIT_WUNLOCK(); VNET_SYSINIT_WUNLOCK();
} }
mjgUnsubmitted
Not Done
Inline Actions

you should unlock in the same order for consistency

the VNET_FOREACH macro should presumably assert on the lock?

mjg: you should unlock in the same order for consistency the VNET_FOREACH macro should presumably…
kpAuthorUnsubmitted
Not Done
Inline Actions

It should, but the locking for the vnet list uses two different locks for reasons I do not currently understand.
So we'd have to MPASS(sx_locked(&vnet_sxlock) || rw_rowned(&vnet_rwlock));. Which is doubly annoying because rw_rowned() doesn't exist.

kp: It should, but the locking for the vnet list uses two different locks for reasons I do not…
mjgUnsubmitted
Not Done
Inline Actions

that is a gap i'm going to plug

mjg: that is a gap i'm going to plug
zleiUnsubmitted
Not Done
Inline Actions

you should unlock in the same order for consistency

I'd prefer to unlock in the reverse order. That is more natural for me.
Anyway unlock order does not matter much.

the VNET_FOREACH macro should presumably assert on the lock?

ipreass_drain() in sys/netinet/ip_reass.c also miss read lock.
I think it is good to have read lock assert in macro VNET_FOREACH .

zlei: > you should unlock in the same order for consistency I'd prefer to unlock in the reverse…
/* /*
* Invoke all registered vnet constructors on the current vnet. Used during * Invoke all registered vnet constructors on the current vnet. Used during
* vnet construction. The caller is responsible for ensuring the new vnet is * vnet construction. The caller is responsible for ensuring the new vnet is
* the current vnet and that the vnet_sysinit_sxlock lock is locked. * the current vnet and that the vnet_sysinit_sxlock lock is locked.
*/ */
static void static void
vnet_sysinit(void) vnet_sysinit(void)
▲ Show 20 Lines • Show All 235 Lines • Show Last 20 Lines
View Options

sys/riscv/include/machdep.h

Show First 20 Lines • Show All 530 Lines • ▼ Show 20 Lines if (vs2 != NULL)
TAILQ_INSERT_BEFORE(vs2, vs, link); TAILQ_INSERT_BEFORE(vs2, vs, link);
else else
TAILQ_INSERT_TAIL(&vnet_constructors, vs, link); TAILQ_INSERT_TAIL(&vnet_constructors, vs, link);
/* /*
* Invoke the constructor on all the existing vnets when it is * Invoke the constructor on all the existing vnets when it is
* registered. * registered.
*/ */
VNET_LIST_RLOCK();
VNET_FOREACH(vnet) { VNET_FOREACH(vnet) {
CURVNET_SET_QUIET(vnet); CURVNET_SET_QUIET(vnet);
vs->func(vs->arg); vs->func(vs->arg);
CURVNET_RESTORE(); CURVNET_RESTORE();
} }
VNET_LIST_RUNLOCK();
VNET_SYSINIT_WUNLOCK(); VNET_SYSINIT_WUNLOCK();
} }
void void
vnet_deregister_sysinit(void *arg) vnet_deregister_sysinit(void *arg)
{ {
struct vnet_sysinit *vs; struct vnet_sysinit *vs;
Show All 34 Linesvnet_deregister_sysuninit(void *arg)
struct vnet *vnet; struct vnet *vnet;
vs = arg; vs = arg;
/* /*
* Invoke the destructor on all the existing vnets when it is * Invoke the destructor on all the existing vnets when it is
* deregistered. * deregistered.
*/ */
VNET_LIST_RLOCK();
zleiUnsubmitted
Not Done
Inline Actions

In vnet_register_sysinit() we have lock order

VNET_SYSINIT_WLOCK();
VNET_LIST_RLOCK;

I think it is best to make lock order consistent with that in vnet_register_sysinit(), although dead lock is not possible.

zlei: In `vnet_register_sysinit()` we have lock order ``` VNET_SYSINIT_WLOCK(); VNET_LIST_RLOCK; ```…
VNET_SYSINIT_WLOCK(); VNET_SYSINIT_WLOCK();
VNET_FOREACH(vnet) { VNET_FOREACH(vnet) {
CURVNET_SET_QUIET(vnet); CURVNET_SET_QUIET(vnet);
vs->func(vs->arg); vs->func(vs->arg);
CURVNET_RESTORE(); CURVNET_RESTORE();
} }
/* Remove the destructor from the global list of vnet destructors. */ /* Remove the destructor from the global list of vnet destructors. */
TAILQ_REMOVE(&vnet_destructors, vs, link); TAILQ_REMOVE(&vnet_destructors, vs, link);
VNET_LIST_RUNLOCK();
VNET_SYSINIT_WUNLOCK(); VNET_SYSINIT_WUNLOCK();
} }
mjgUnsubmitted
Not Done
Inline Actions

you should unlock in the same order for consistency

the VNET_FOREACH macro should presumably assert on the lock?

mjg: you should unlock in the same order for consistency the VNET_FOREACH macro should presumably…
kpAuthorUnsubmitted
Not Done
Inline Actions

It should, but the locking for the vnet list uses two different locks for reasons I do not currently understand.
So we'd have to MPASS(sx_locked(&vnet_sxlock) || rw_rowned(&vnet_rwlock));. Which is doubly annoying because rw_rowned() doesn't exist.

kp: It should, but the locking for the vnet list uses two different locks for reasons I do not…
mjgUnsubmitted
Not Done
Inline Actions

that is a gap i'm going to plug

mjg: that is a gap i'm going to plug
zleiUnsubmitted
Not Done
Inline Actions

you should unlock in the same order for consistency

I'd prefer to unlock in the reverse order. That is more natural for me.
Anyway unlock order does not matter much.

the VNET_FOREACH macro should presumably assert on the lock?

ipreass_drain() in sys/netinet/ip_reass.c also miss read lock.
I think it is good to have read lock assert in macro VNET_FOREACH .

zlei: > you should unlock in the same order for consistency I'd prefer to unlock in the reverse…
/* /*
* Invoke all registered vnet constructors on the current vnet. Used during * Invoke all registered vnet constructors on the current vnet. Used during
* vnet construction. The caller is responsible for ensuring the new vnet is * vnet construction. The caller is responsible for ensuring the new vnet is
* the current vnet and that the vnet_sysinit_sxlock lock is locked. * the current vnet and that the vnet_sysinit_sxlock lock is locked.
*/ */
static void static void
vnet_sysinit(void) vnet_sysinit(void)
▲ Show 20 Lines • Show All 235 Lines • Show Last 20 Lines
View Options

sys/riscv/riscv/machdep.c

Show First 20 Lines • Show All 530 Lines • ▼ Show 20 Lines if (vs2 != NULL)
TAILQ_INSERT_BEFORE(vs2, vs, link); TAILQ_INSERT_BEFORE(vs2, vs, link);
else else
TAILQ_INSERT_TAIL(&vnet_constructors, vs, link); TAILQ_INSERT_TAIL(&vnet_constructors, vs, link);
/* /*
* Invoke the constructor on all the existing vnets when it is * Invoke the constructor on all the existing vnets when it is
* registered. * registered.
*/ */
VNET_LIST_RLOCK();
VNET_FOREACH(vnet) { VNET_FOREACH(vnet) {
CURVNET_SET_QUIET(vnet); CURVNET_SET_QUIET(vnet);
vs->func(vs->arg); vs->func(vs->arg);
CURVNET_RESTORE(); CURVNET_RESTORE();
} }
VNET_LIST_RUNLOCK();
VNET_SYSINIT_WUNLOCK(); VNET_SYSINIT_WUNLOCK();
} }
void void
vnet_deregister_sysinit(void *arg) vnet_deregister_sysinit(void *arg)
{ {
struct vnet_sysinit *vs; struct vnet_sysinit *vs;
Show All 34 Linesvnet_deregister_sysuninit(void *arg)
struct vnet *vnet; struct vnet *vnet;
vs = arg; vs = arg;
/* /*
* Invoke the destructor on all the existing vnets when it is * Invoke the destructor on all the existing vnets when it is
* deregistered. * deregistered.
*/ */
VNET_LIST_RLOCK();
zleiUnsubmitted
Not Done
Inline Actions

In vnet_register_sysinit() we have lock order

VNET_SYSINIT_WLOCK();
VNET_LIST_RLOCK;

I think it is best to make lock order consistent with that in vnet_register_sysinit(), although dead lock is not possible.

zlei: In `vnet_register_sysinit()` we have lock order ``` VNET_SYSINIT_WLOCK(); VNET_LIST_RLOCK; ```…
VNET_SYSINIT_WLOCK(); VNET_SYSINIT_WLOCK();
VNET_FOREACH(vnet) { VNET_FOREACH(vnet) {
CURVNET_SET_QUIET(vnet); CURVNET_SET_QUIET(vnet);
vs->func(vs->arg); vs->func(vs->arg);
CURVNET_RESTORE(); CURVNET_RESTORE();
} }
/* Remove the destructor from the global list of vnet destructors. */ /* Remove the destructor from the global list of vnet destructors. */
TAILQ_REMOVE(&vnet_destructors, vs, link); TAILQ_REMOVE(&vnet_destructors, vs, link);
VNET_LIST_RUNLOCK();
VNET_SYSINIT_WUNLOCK(); VNET_SYSINIT_WUNLOCK();
} }
mjgUnsubmitted
Not Done
Inline Actions

you should unlock in the same order for consistency

the VNET_FOREACH macro should presumably assert on the lock?

mjg: you should unlock in the same order for consistency the VNET_FOREACH macro should presumably…
kpAuthorUnsubmitted
Not Done
Inline Actions

It should, but the locking for the vnet list uses two different locks for reasons I do not currently understand.
So we'd have to MPASS(sx_locked(&vnet_sxlock) || rw_rowned(&vnet_rwlock));. Which is doubly annoying because rw_rowned() doesn't exist.

kp: It should, but the locking for the vnet list uses two different locks for reasons I do not…
mjgUnsubmitted
Not Done
Inline Actions

that is a gap i'm going to plug

mjg: that is a gap i'm going to plug
zleiUnsubmitted
Not Done
Inline Actions

you should unlock in the same order for consistency

I'd prefer to unlock in the reverse order. That is more natural for me.
Anyway unlock order does not matter much.

the VNET_FOREACH macro should presumably assert on the lock?

ipreass_drain() in sys/netinet/ip_reass.c also miss read lock.
I think it is good to have read lock assert in macro VNET_FOREACH .

zlei: > you should unlock in the same order for consistency I'd prefer to unlock in the reverse…
/* /*
* Invoke all registered vnet constructors on the current vnet. Used during * Invoke all registered vnet constructors on the current vnet. Used during
* vnet construction. The caller is responsible for ensuring the new vnet is * vnet construction. The caller is responsible for ensuring the new vnet is
* the current vnet and that the vnet_sysinit_sxlock lock is locked. * the current vnet and that the vnet_sysinit_sxlock lock is locked.
*/ */
static void static void
vnet_sysinit(void) vnet_sysinit(void)
▲ Show 20 Lines • Show All 235 Lines • Show Last 20 Lines
View Options

sys/riscv/riscv/pmap.c

Show First 20 Lines • Show All 530 Lines • ▼ Show 20 Lines if (vs2 != NULL)
TAILQ_INSERT_BEFORE(vs2, vs, link); TAILQ_INSERT_BEFORE(vs2, vs, link);
else else
TAILQ_INSERT_TAIL(&vnet_constructors, vs, link); TAILQ_INSERT_TAIL(&vnet_constructors, vs, link);
/* /*
* Invoke the constructor on all the existing vnets when it is * Invoke the constructor on all the existing vnets when it is
* registered. * registered.
*/ */
VNET_LIST_RLOCK();
VNET_FOREACH(vnet) { VNET_FOREACH(vnet) {
CURVNET_SET_QUIET(vnet); CURVNET_SET_QUIET(vnet);
vs->func(vs->arg); vs->func(vs->arg);
CURVNET_RESTORE(); CURVNET_RESTORE();
} }
VNET_LIST_RUNLOCK();
VNET_SYSINIT_WUNLOCK(); VNET_SYSINIT_WUNLOCK();
} }
void void
vnet_deregister_sysinit(void *arg) vnet_deregister_sysinit(void *arg)
{ {
struct vnet_sysinit *vs; struct vnet_sysinit *vs;
Show All 34 Linesvnet_deregister_sysuninit(void *arg)
struct vnet *vnet; struct vnet *vnet;
vs = arg; vs = arg;
/* /*
* Invoke the destructor on all the existing vnets when it is * Invoke the destructor on all the existing vnets when it is
* deregistered. * deregistered.
*/ */
VNET_LIST_RLOCK();
zleiUnsubmitted
Not Done
Inline Actions

In vnet_register_sysinit() we have lock order

VNET_SYSINIT_WLOCK();
VNET_LIST_RLOCK;

I think it is best to make lock order consistent with that in vnet_register_sysinit(), although dead lock is not possible.

zlei: In `vnet_register_sysinit()` we have lock order ``` VNET_SYSINIT_WLOCK(); VNET_LIST_RLOCK; ```…
VNET_SYSINIT_WLOCK(); VNET_SYSINIT_WLOCK();
VNET_FOREACH(vnet) { VNET_FOREACH(vnet) {
CURVNET_SET_QUIET(vnet); CURVNET_SET_QUIET(vnet);
vs->func(vs->arg); vs->func(vs->arg);
CURVNET_RESTORE(); CURVNET_RESTORE();
} }
/* Remove the destructor from the global list of vnet destructors. */ /* Remove the destructor from the global list of vnet destructors. */
TAILQ_REMOVE(&vnet_destructors, vs, link); TAILQ_REMOVE(&vnet_destructors, vs, link);
VNET_LIST_RUNLOCK();
VNET_SYSINIT_WUNLOCK(); VNET_SYSINIT_WUNLOCK();
} }
mjgUnsubmitted
Not Done
Inline Actions

you should unlock in the same order for consistency

the VNET_FOREACH macro should presumably assert on the lock?

mjg: you should unlock in the same order for consistency the VNET_FOREACH macro should presumably…
kpAuthorUnsubmitted
Not Done
Inline Actions

It should, but the locking for the vnet list uses two different locks for reasons I do not currently understand.
So we'd have to MPASS(sx_locked(&vnet_sxlock) || rw_rowned(&vnet_rwlock));. Which is doubly annoying because rw_rowned() doesn't exist.

kp: It should, but the locking for the vnet list uses two different locks for reasons I do not…
mjgUnsubmitted
Not Done
Inline Actions

that is a gap i'm going to plug

mjg: that is a gap i'm going to plug
zleiUnsubmitted
Not Done
Inline Actions

you should unlock in the same order for consistency

I'd prefer to unlock in the reverse order. That is more natural for me.
Anyway unlock order does not matter much.

the VNET_FOREACH macro should presumably assert on the lock?

ipreass_drain() in sys/netinet/ip_reass.c also miss read lock.
I think it is good to have read lock assert in macro VNET_FOREACH .

zlei: > you should unlock in the same order for consistency I'd prefer to unlock in the reverse…
/* /*
* Invoke all registered vnet constructors on the current vnet. Used during * Invoke all registered vnet constructors on the current vnet. Used during
* vnet construction. The caller is responsible for ensuring the new vnet is * vnet construction. The caller is responsible for ensuring the new vnet is
* the current vnet and that the vnet_sysinit_sxlock lock is locked. * the current vnet and that the vnet_sysinit_sxlock lock is locked.
*/ */
static void static void
vnet_sysinit(void) vnet_sysinit(void)
▲ Show 20 Lines • Show All 235 Lines • Show Last 20 Lines