Page MenuHomeFreeBSD

riscv: Fix size reserved for the devmap region in pmap_bootstrap.
ClosedPublic

Authored by alfredo.mazzinghi_cl.cam.ac.uk on May 15 2023, 3:31 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, May 2, 2:05 AM
Unknown Object (File)
Thu, May 2, 12:11 AM
Unknown Object (File)
Wed, May 1, 11:52 PM
Unknown Object (File)
Wed, May 1, 11:20 PM
Unknown Object (File)
Wed, May 1, 11:19 PM
Unknown Object (File)
Tue, Apr 23, 4:58 PM
Unknown Object (File)
Mar 7 2024, 1:00 PM
Unknown Object (File)
Feb 9 2024, 5:29 PM
Subscribers

Details

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

mhorne requested changes to this revision.May 15 2023, 3:50 PM
mhorne added a subscriber: mhorne.

In order for this to work, you will need to fix the assembly in locore.S where we actually create the devmap mapping. There, we create it as a single 2MiB page beginning at (VM_MAX_KERNEL_ADDRESS - L2_SIZE).

The relevant snippet, beginning at line 160:

	/* Create an L1 table entry for early devmap */
	lla	s1, pagetable_l1
	lla	s2, pagetable_l2_devmap	/* Link to next level PN */
	srli	s2, s2, PAGE_SHIFT

	li	a5, (VM_MAX_KERNEL_ADDRESS - L2_SIZE)
	srli	a5, a5, L1_SHIFT	/* >> L1_SHIFT */
	andi	a5, a5, Ln_ADDR_MASK	/* & Ln_ADDR_MASK */
	li	t4, PTE_V
	slli	t5, s2, PTE_PPN0_S	/* (s2 << PTE_PPN0_S) */
	or	t6, t4, t5
This revision now requires changes to proceed.May 15 2023, 3:50 PM

@mhorne I'm not sure how this currently is supposed to work, because the PMAP_MAPDEV_EARLY_SIZE constant is already larger than L2_SIZE in vmparam.h. So what we really want is also the DTB to be mapped at VM_EARLY_DTB_ADDRESS, which currently seems to be assumed to correspond to the second-to-last L2 entry in the devmap table. Is the PMAP_MAPDEV_EARLY_SIZE constant supposed to contain VM_EARLY_DTB_ADDRESS?

@mhorne I'm not sure how this currently is supposed to work, because the PMAP_MAPDEV_EARLY_SIZE constant is already larger than L2_SIZE in vmparam.h. So what we really want is also the DTB to be mapped at VM_EARLY_DTB_ADDRESS, which currently seems to be assumed to correspond to the second-to-last L2 entry in the devmap table. Is the PMAP_MAPDEV_EARLY_SIZE constant supposed to contain VM_EARLY_DTB_ADDRESS?

Yeah the situation here is indeed confusing. The PMAP_MAPDEV_EARLY_SIZE constant was inherited from arm64, but as you have discovered, we do not use it. Instead, the implicit size of the devmap on riscv is L2_SIZE; this is what locore.S creates and what pmap_bootstrap() assumes.

VM_EARLY_DTB_ADDRESS is not supposed to overlap with the devmap, they must be separate. The exact address we choose for either is arbitrary, but we have (so far) chosen the second-to-last L2 page of KVA for mapping the DTB, and the last L2 page for the devmap.

So you have two options for how to proceed, as I see it:

  1. Shrink the size of PMAP_MAPDEV_EARLY_SIZE to be L2_SIZE, and start using this define in places we currently hard-code as L2_SIZE. This is simpler, but more fragile as we can't grow the devmap size without overlapping the DTB mapping.
  2. Fix locore.S assembly to create a devmap of the correct size, and adjust the value of VM_EARLY_DTB_ADDRESS so that it will still precede the devmap by one L2 page.

In either case, I would be happy if you could add a compile-time assertion to verify that the two regions do not overlap.

In the longer term, I have some WIP changes to eliminate the early DTB mapping completely, and delay the construction of the devmap until pmap_bootstrap(), essentially making either solution moot. I cannot say how soon that will be ready to land, it could be a couple of months or many more.

I hope this helps. Let me know if you have further questions!

Shrink the size of PMAP_MAPDEV_EARLY_SIZE to a single L2_SIZE.
This should be acceptable given the fact that it does not seem that we were
trampling the DTB mapping before.
While this solution is less robust, it avoids adding complexity to asm code
unless it is absolutely required.
Added assertions that verify that both the DTB and early DEVMAP are aligned to
L2 boundary and that the DTB mapping does not overlap the DEVMAP.
This check assumes that the DTB only consumes a single L2 entry.

LGTM, thanks. I can push this to the main branch.

Can you update the title+summary to reflect the final diff? (We didn't end up changing the devmap size.) Even easier would be if you could just send the git format-patch output to me over email: mhorne@freebsd.org.

This revision is now accepted and ready to land.May 25 2023, 2:25 PM

LGTM, thanks. I can push this to the main branch.

Can you update the title+summary to reflect the final diff? (We didn't end up changing the devmap size.) Even easier would be if you could just send the git format-patch output to me over email: mhorne@freebsd.org.

Sent patch email. Thanks!