Change various kernel APIs to work with pointers instead of vm_offset_t by bsdjhb · Pull Request #2068 · freebsd/freebsd-src

bsdjhb · 2026-03-09T14:19:32Z

In CHERI kernels, pointers are not the same size as addresses, and vm_offset_t represents a virtual address (ptraddr_t). To handle places that use vm_offset_t to hold pointers that can be dereferenced by the CPU, CheriBSD adds a new vm_pointer_t type (uintptr_t). However, using vm_pointer_t can be a bit fragile, as C compilers will silently accept conversions between uintptr_t and ptraddr_t. The resulting errors can only be found at runtime. However, if pointers are stored as a pointer type such as void *, then mismatches between addresses and pointers can be caught at compile time including when compiling on non-CHERI architectures. As such, this series converts various kernel pmap/virtual memory kernel APIs that currently use vm_offset_t to use pointer types (either void * or char *) instead. Normally void * is used, but char * is preferred if the API or structure member is commonly used in pointer arithmetic. In practice, these changes seem to be cleaner as most kernel consumers for many of these APIs had to cast between pointer types and vm_offset_t anyway and now those casts can be removed.

Note, when landing this series, I will add a commit to bump __FreeBSD_version so that kmods in ports can handle these changes, but it's pointless to add that commit until this is actually landed. This would also not be a MFC candidate.

Also, this seems too unwieldy to review in phabricator, hence reviewing here.

…ters

No functional change, but this is friendlier for CHERI.

This removes the need for several casts to pointer in callers.

Add explicit uintptr_t casts to the arguments to these macros so that the work both with virtual addresses (e.g. vm_offset_t) and pointers. Drop no-longer-needed casts in various invocations of DMAP_TO_PHYS.

Consistently use vm_paddr_t for the type returned from moea64_bootstrap_alloc and avoid temporarily smuggling it via a pointer. Instead, be explicit in the places that assume a 1:1 mapping.

github-actions · 2026-03-09T14:19:42Z

Thank you for taking the time to contribute to FreeBSD!

Some of files have special handling:

Important

@bsdjhb @bryanv @markjdb @khng300 @markpeek wants to review changes to sys/amd64/vmm/

Important

@zxombie wants to review changes to sys/arm64/arm64

mchoo7 · 2026-03-11T03:09:32Z

The justification makes sense to me. What I worry is mixing void * and char * part. From the PR description and mailing list I get that char * is for pointer arithmetic, but it could be confusing for people who don't know the context (e.g. they might wonder rationale behind char *vaddr). This could end up like unsigned long devid for linux tasklet API where most people pass pointers instead of integers (I guess CHERI linux WG needs to handle this?)

My suggestion is creating a typedef to char *. In that case pointer arithmetic works and the type name makes more sense.

kostikbel · 2026-03-11T09:58:45Z

sys/amd64/amd64/pmap.c


 static int
-pmap_change_props_locked(vm_offset_t va, vm_size_t size, vm_prot_t prot,
+pmap_change_props_locked(void *addr, vm_size_t size, vm_prot_t prot,


I am not sure about this change. pmap_change_props() operates on address/page table, not on the pointer.

Yes, this one was more about a tradeoff of casts in callers vs just one here, and yes it is a bit more squirrely.

kostikbel · 2026-03-11T09:59:30Z

sys/amd64/amd64/pmap.c

 	int error;
 	bool changed;

+	va = (vm_offset_t)addr;


And the fact that you have to immediately cast the address to vm_offset_t aligns with my doubts.

kostikbel · 2026-03-11T10:02:11Z

sys/amd64/include/vmparam.h

 	    ("physical address %#jx not covered by the DMAP",		\
 	    (uintmax_t)x));						\
-	(x) + kva_layout.dmap_low; })
+	(void *)((x) + kva_layout.dmap_low); })


I think this change is fine. But I suggest to provide a macro, lets call it PHYS_TO_DMAP_ADDR(), which is identical to the old PHYS_TO_DMAP(), which returns the address. Then PHYS_TO_DMAP() becomes ((void *)PHYS_TO_DMAP_ADDR()).

PHYS_TO_DMA_ADDR() should be used where you immediately cast PHYS_TO_DMAP() result back to address.

kostikbel · 2026-03-11T10:04:05Z

sys/amd64/include/vmparam.h

-#define	VIRT_IN_DMAP(va)	\
-    ((va) >= kva_layout.dmap_low && (va) < kva_layout.dmap_low + dmaplimit)
+#define	VIRT_IN_DMAP(va)						\
+	((uintptr_t)(va) >= kva_layout.dmap_low &&			\


I would suggest to add a local of the uintptr_t type and evaluate and cast the 'va' arg only once.

kostikbel · 2026-03-11T10:05:35Z

sys/amd64/amd64/pmap.c

 	if (pmap->pm_pmltopu != NULL) {
-		m = PHYS_TO_VM_PAGE(DMAP_TO_PHYS((vm_offset_t)pmap->
-		    pm_pmltopu));
+		m = PHYS_TO_VM_PAGE(DMAP_TO_PHYS(pmap->pm_pmltopu));


Might be we need DMAP_TO_VM_PAGE() ?

Certainly this is a common pattern. Such a macro could also be MI as it would be spelled the same on all architectures.

bsdjhb · 2026-03-11T14:35:45Z

The justification makes sense to me. What I worry is mixing void * and char * part. From the PR description and mailing list I get that char * is for pointer arithmetic, but it could be confusing for people who don't know the context (e.g. they might wonder rationale behind char *vaddr). This could end up like unsigned long devid for linux tasklet API where most people pass pointers instead of integers (I guess CHERI linux WG needs to handle this?)

My suggestion is creating a typedef to char *. In that case pointer arithmetic works and the type name makes more sense.

BSD has a legacy typedef (caddr_t), but we generally avoid that in new code. Also, I think the only place that might be char * in this series is td_kstack?

mchoo7 · 2026-03-11T15:29:30Z

The justification makes sense to me. What I worry is mixing void * and char * part. From the PR description and mailing list I get that char * is for pointer arithmetic, but it could be confusing for people who don't know the context (e.g. they might wonder rationale behind char *vaddr). This could end up like unsigned long devid for linux tasklet API where most people pass pointers instead of integers (I guess CHERI linux WG needs to handle this?)
My suggestion is creating a typedef to char *. In that case pointer arithmetic works and the type name makes more sense.

BSD has a legacy typedef (caddr_t), but we generally avoid that in new code.

I would avoid caddr_t for parameters, return values, and struct fields but for local variables caddr_t seems ok. I don't mind sticking with char * but the name caddr_t sounds more intuitive.

Also, I think the only place that might be char * in this series is td_kstack?

I see char * in many places like in struct sync_list.

But actually, we can address this by adding a note to style(9). e.g.:

Usage of caddr_t is avoided and void * is preferred. If there is frequent pointer arithmetic, char * is acceptable.

and use char * over caddr_t everywhere including local variables. Because new contributors are encouraged to start by reading style(9) they'll get the context before they work on the code.

kostikbel

This one seems to be rather arbitrary. The patch adds more casts than it removes. Also, the kva is internal member which denotes the address which is used for page table manipulation (pmap_qenter()) and not a pointer that is operated upon. The single cast in sf_buf_kva() seems more proper to me.

jrtc27 · 2026-03-14T00:29:59Z

sf_buf_kva's returned value is used as a pointer, and in the non-DMAP + SFBUF case that pointer comes from the kva member. Yes, it's true that all supported CHERI architectures have a DMAP, but we should be consistent with types.

bsdjhb added 19 commits March 4, 2026 12:33

pmap_quick_(enter|remove)_page: Use void * instead of vm_offset_t

5de2fb5

busdma: Use char * instead of vm_offset_t for bounce page kernel poin…

9bd00b1

…ters

riscv: Switch the address argument to cpu_dcache_* to a pointer

96df5df

No functional change, but this is friendlier for CHERI.

pmap_change_attr: Use void * instead of vm_offset_t

6a5a886

pmap_change_prot: Use void * instead of vm_offset_t

e5e9c6a

sf_buf_kva: Return a pointer instead of a vm_offset_t

92cb505

This removes the need for several casts to pointer in callers.

pmap_map_io_transient: Use void * instead of vm_offset_t for vaddr array

6b3856f

pmap_qenter/qremove: Use void * instead of vm_offset_t

052724c

sf_buf: Convert kva member from vm_offset_t to void *

145db2a

gve: Convert a couple of structure members from vm_offset_t to char *

3eecdd0

kva_alloc/free: Use void * instead of vm_offset_t

2b5948f

vm_kstack: Use void * instead of vm_offset_t

39c3437

sys: Switch td_kstack from vm_offset_t to char *

8e02d8b

pmap_map: Use void * for the return value instead of vm_offset_t

c072c43

sys: Permit passing pointers to VIRT_IN_DMAP and DMAP_TO_PHYS

94d81cf

Add explicit uintptr_t casts to the arguments to these macros so that the work both with virtual addresses (e.g. vm_offset_t) and pointers. Drop no-longer-needed casts in various invocations of DMAP_TO_PHYS.

powerpc aim64: Return vm_paddr_t from moea64_bootstrap_alloc

9e4a07f

Consistently use vm_paddr_t for the type returned from moea64_bootstrap_alloc and avoid temporarily smuggling it via a pointer. Instead, be explicit in the places that assume a 1:1 mapping.

PHYS_TO_DMAP: Return a void * instead of a vm_offset_t

70b69a0

kmap_alloc_wait/kmap_free_wakeup: Use void * instead of vm_offset_t

5230f90

amd64/aarch64 pmap: Switch type of pmap_preinit_mapping.va to void *

1c094ef

bsdjhb requested review from brooksdavis, kostikbel and markjdb March 9, 2026 14:19

bsdjhb requested a review from bsdimp as a code owner March 9, 2026 14:19

kostikbel reviewed Mar 11, 2026

View reviewed changes

bsdjhb mentioned this pull request Mar 11, 2026

RFC: do we want to upstream vm_pointer_t? CTSRD-CHERI/cheribsd#2486

Open

kostikbel approved these changes Mar 14, 2026

View reviewed changes

kostikbel reviewed Mar 14, 2026

View reviewed changes

Conversation

bsdjhb commented Mar 9, 2026

Uh oh!

github-actions bot commented Mar 9, 2026

Uh oh!

mchoo7 commented Mar 11, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsdjhb commented Mar 11, 2026

Uh oh!

mchoo7 commented Mar 11, 2026

Uh oh!

kostikbel left a comment

Choose a reason for hiding this comment

Uh oh!

jrtc27 commented Mar 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants