arm64/vmm: Enable 16-bit VMIDs when in use by pmap
Needs ReviewPublic
Actions

Authored by jrtc27 on Sat, Mar 14, 11:43 PM.

Details

Reviewers

andrew
markj
manu

Group Reviewers

arm64
cheri

Summary

pmap_init always uses 16-bit VMIDs when supported, but we never enable
them in VTCR_EL2 (for ASIDs, locore enables them in TCR_EL1 and
pmap_init keys off whether they've been enabled, but the order in which
pmap_init and vmmops_modinit run is reversed). As a result, although the
full 16-bit value can be stored to VTTBR_EL2 and read back, the upper 8
bits are treated as 0, and so VMIDs that our VMID allocation believes
are distinct end up aliasing.

Obtained from: CheriBSD
Fixes: 47e073941f4e ("Import the kernel parts of bhyve/arm64")
MFC after: 1 week

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 71407
Build 68290: arc lint + arc unit

Event Timeline

jrtc27 created this revision.Sat, Mar 14, 11:43 PM

Herald added a reviewer: manu. · View Herald TranscriptSat, Mar 14, 11:43 PM

Herald added subscribers: emaste, imp. · View Herald Transcript

jrtc27 requested review of this revision.Sat, Mar 14, 11:43 PM

Harbormaster completed remote builds in B71407: Diff 173704.Sat, Mar 14, 11:43 PM

Herald added a reviewer: cheri. · View Herald TranscriptSat, Mar 14, 11:43 PM

Herald added a reviewer: cheri. · View Herald Transcript

Is there a reason to not just read the ID register in the vmm code?

In D55860#1278404, @andrew wrote:

Is there a reason to not just read the ID register in the vmm code?

Several reasons:

I didn't like duplicating the logic
If ever some new extension comes along that adds another toggle for bigger VMIDs, this will blow up (and is more likely to be noticed when grep'ing pmap.c) - I want to make sure this error never happens again, and having two parallel bits of code that don't interact with each other is still at risk of that (DS on the following lines is also a bit at risk of that and ideally would instead have been pmap_[stage2_]ds_enabled rather than peeking at TCR_EL1 to infer the expectations of pmap.c)
This way you can test with VS disabled on a system that supports 16-bit VMIDs by only modifying the one place in pmap.c that chooses vmids.asid_bits

My initial patch was to do just that, read the ID register and enable VS if supported, but that didn't give me warm fuzzy feelings.

jhb added a subscriber: jhb.Thu, Mar 19, 2:48 PM

jhb added inline comments.

sys/arm64/arm64/pmap.c
800	Hmm, is this function saying that VS is already enabled, or that VS needs to be enabled? Given that we use this to set the bit, I wonder if `pmap_vs_needed` might be a clearer name?

jrtc27 added inline comments.Thu, Mar 19, 3:09 PM

sys/arm64/arm64/pmap.c
800	I guess in my head the name is "pmap has logically enabled using it and vmm will realise that in VTCR_EL2", and it matched the naming for pmap_ps_enabled, though that's an x86-ism that got used as the MI name for superpages. Perhaps pmap_vmid16_enabled (c.f. FEAT_VMID16 as the extension), which more closely matches the fact that pmap has already put 16 in vmids.asid_bits (and if there were a tunable to disable it, it would just override that back to 8), and then vmm uses that to determine it needs to set VTCR_EL2.VS?

jhb added inline comments.Thu, Mar 19, 4:02 PM

sys/arm64/arm64/pmap.c
800	Yes, I think that would be perfect.

andrew added inline comments.Thu, Mar 19, 4:21 PM

sys/arm64/arm64/pmap.c
800	I was wondering if we should move the `FEAT_VMID16` detection logic to vmm & have it tell pmap how many vmid bits to support.

jrtc27 added inline comments.Thu, Mar 19, 4:25 PM

sys/arm64/arm64/pmap.c
800	You could defer the pmap_init_asids until vmm is loaded, but then you'd need to think about what it would mean to unload vmm.ko and reload it, potentially with a different number of VMID bits requested. Which is perhaps a better longer-term design, but I don't particularly want to have to deal with that right now to fix this bug (that we hit in a few hours of a demo that loops booting a Morello seL4 VM).