Details

Reviewers

kib
andrew
markj
manu

Commits

rS367282: Add routines for ARM System MMU (SMMU) pmap management.

Summary

Add routines to manage SMMU page tables

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

br created this revision.Oct 20 2020, 9:52 AM

Herald added a reviewer: manu. · View Herald TranscriptOct 20 2020, 9:52 AM

Herald added a subscriber: emaste. · View Herald Transcript

br requested review of this revision.Oct 20 2020, 9:52 AM

markj added inline comments.Oct 21 2020, 1:33 PM

sys/arm64/arm64/pmap.c
3702 ↗	(On Diff #78499)	`pmap_sremove_pages()` would better match the names of the native routines. `pmap_remove_all()` removes all managed mappings of a given page.
3749 ↗	(On Diff #78499)	Can we have a block here? pmap_senter() does not create them.
3763 ↗	(On Diff #78499)	Debug printf Also don't we need to unwire the L3 page once for each valid entry?

o Rename pmap_sremove_all() to pmap_sremove_pages()
o Remove unneeded check for L2_BLOCK in pmap_sremove_pages()

br added inline comments.Oct 22 2020, 3:45 PM

sys/arm64/arm64/pmap.c
3702 ↗	(On Diff #78499)	Thanks, renamed!
3749 ↗	(On Diff #78499)	Not needed. So I removed it. Thanks!
3763 ↗	(On Diff #78499)	I could not find any debug printf macro, do we have any ? This function is called when no L3 pages are mapped, so if any valid L3 page present it just returns error and pmap will not be released by IOMMU code. (we can replace to panic, any other ideas ?)

markj added inline comments.Oct 27 2020, 1:52 PM

sys/arm64/arm64/pmap.c
3630 ↗	(On Diff #78592)	Why is the user bit being set?
3690 ↗	(On Diff #78592)	It's not necessary to do any kind of TLB invalidation here?
3713 ↗	(On Diff #78592)	These declarations of type `vm_page_t` can be in a single line.
3719 ↗	(On Diff #78592)	Can't we stop iterating once sva > VM_MAXUSER_ADDRESS?
3763 ↗	(On Diff #78499)	I see. If this condition is not expected to occur it should be a panic(). If you want to keep the printf() I'd put it under ifdef DIAGNOSTIC.

br added inline comments.Oct 27 2020, 3:06 PM

sys/arm64/arm64/pmap.c
3630 ↗	(On Diff #78592)	Maybe because iommu addresses are below VM_MAXUSER_ADDRESS ? I receive this event otherwise (i.e. without ATTR_S1_AP_USER bit): smmu0: Event F_PERMISSION (Permission fault occurred on page access.) received.
3690 ↗	(On Diff #78592)	SMMU has its own TLB that can be invalidated in the SMMU driver only, so we proceed invalidatation in the driver just after calling this function.
3713 ↗	(On Diff #78592)	Fixed.
3719 ↗	(On Diff #78592)	Sure!
3763 ↗	(On Diff #78499)	Ok, I've replaced the printf call with panic()

Address markj@ queries

Fix comment

br added a parent revision: D24618: ARM SMMU v3.2 support.Oct 30 2020, 12:20 PM

I have no objection to the change. I'm not really convinced it needs to be part of the pmap code, instead of a self-contained module in the SMMU driver, but I can see why it's convenient.

sys/arm64/arm64/pmap.c
3659 ↗	(On Diff #78837)	We should modify the pmap's resident count here.
3630 ↗	(On Diff #78592)	The bit controls access permission from EL0. I'm not sure how that meaning is translated when using the SMMU.

Manage resident count for L3 pages of SMMU pmap

br added inline comments.Nov 1 2020, 11:04 AM

sys/arm64/arm64/pmap.c
3659 ↗	(On Diff #78837)	Added here and in pmap_sremove().
3630 ↗	(On Diff #78592)	Yes, I think ATTR_S1_AP_USER instructs SMMU to use page table base register from TTB0 register. We only set pointer to page directory in TTB0 register of a context descriptor, and TTB1 is zero.