Page MenuHomeFreeBSD

riscv: Svpbmt extension support
ClosedPublic

Authored by mhorne on Jun 3 2024, 6:51 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Oct 26, 11:12 AM
Unknown Object (File)
Sun, Oct 20, 7:57 AM
Unknown Object (File)
Oct 7 2024, 2:13 AM
Unknown Object (File)
Sep 30 2024, 1:06 PM
Unknown Object (File)
Sep 27 2024, 3:16 PM
Unknown Object (File)
Sep 9 2024, 1:35 AM
Unknown Object (File)
Sep 8 2024, 10:42 PM
Unknown Object (File)
Sep 8 2024, 3:55 PM
Subscribers

Details

Summary

The Svpbmt extension provides specification of "Page-Based Memory
Types", or memory attributes (e.g. cacheability constraints).

Extend pmap code to apply memory attributes when creating/updating PTEs.
This is done in a way which should be non-hostile to CPUs lacking memory
attribute (Svpbmt) support, and applicable to alternate encodings of
this feature (T-HEAD CPUs, see next review).

Test Plan

Basic smoketest in a VM with 'Svpbmt' extension enabled: this only has a couple mappings marked "I/O" in sysctl vm.pmap.kernel_maps.

pmap_change_attr() is not widely used, so to test this I swapped some driver malloc() calls to allocate memory/KVA but map it as VM_MEMATTR_NOCACHE. This seemed to work fine and the DMAP was updated accordingly, forcing a demotion of the L1 page created by pmap_bootstrap_dmap().

Diff Detail

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

Event Timeline

mhorne requested review of this revision.Jun 3 2024, 6:51 PM
sys/riscv/riscv/pmap.c
4873

Could you pull memattr_mask down into this commit? We'll need it in CheriBSD, as our existing CHERI-RISC-V spec collides with Svpbmt (there was no space to do custom extensions at the time we wrote it, so picked the high bits thinking the standard bits would be allocated from the bottom not the top...). That will eventually change as we move towards standardising within RISC-V, but we haven't got a draft spec for PTEs yet, and even then it's going to be a big lift to switch everything over to the draft standard encodings, mnemonics, PTEs, etc.

sys/riscv/riscv/pmap.c
4873

Certainly, I can do that.

Generally looks good to me.

sys/riscv/riscv/pmap.c
544

One of my suggestions didn't get added and I have to add a dummy comment to add it.

sys/riscv/riscv/pmap.c
3417

You can remove the commented out line?

Finish implementation of pmap_change_attr_locked().

Bring in memattr_mask from the T-HEAD child review.

mhorne retitled this revision from [DRAFT/RFC] riscv: Svpbmt extension support to riscv: Svpbmt extension support.Jun 18 2024, 6:15 PM
mhorne edited the summary of this revision. (Show Details)
mhorne edited the test plan for this revision. (Show Details)
sys/riscv/riscv/pmap.c
4936

Note: I do think it is possible to collapse this into the first loop, but I had a difficult time expressing this cleanly. This is not a performance critical path.

4954–4959

This logic could be improved to batch updates to the DMAP, akin to amd64's implementation. For now I did the simple thing.

Apply memattr bits to mappings created at the end of pmap_bootstrap() (this was missed).

Ping. I am ready to land this soon. Any further comments?

jhb added inline comments.
sys/riscv/riscv/pmap.c
4895

amd64 uses PDPMASK, etc. which are like PAGE_MASK here, so yes, that means you need the -1 here and in the other case below.

The code from amd64:

			/*
			 * If the current 1GB page already has the required
			 * properties, then we need not demote this page.  Just
			 * increment tmpva to the next 1GB page frame.
			 */
			if ((*pdpe & pde_mask) == pde_bits) {
				tmpva = trunc_1gpage(tmpva) + NBPDP;
				continue;
			}

			/*
			 * If the current offset aligns with a 1GB page frame
			 * and there is at least 1GB left within the range, then
			 * we need not break down this page into 2MB pages.
			 */
			if ((tmpva & PDPMASK) == 0 &&
			    tmpva + PDPMASK < base + size) {
				tmpva += NBPDP;
				continue;
			}
			if (!pmap_demote_pdpe(kernel_pmap, pdpe, tmpva))
				return (ENOMEM);
4936

amd64 does two loops as well, one to do demotions, a second to do all the PTE updates

This revision is now accepted and ready to land.Jul 30 2024, 4:41 PM

tested, works to me on Allwinner D1S

sys/riscv/riscv/pmap.c
4895

Thanks, agreed. I've updated my local version committed version to express it as:

if ((tmpva & L1_OFFSET) == 0 &&
    tmpva + L1_SIZE <= base + size) {
sys/riscv/riscv/pmap.c
4954–4959

I think the simple version is fine.

This revision was automatically updated to reflect the committed changes.