Don't fail changing props for unmapped DMAP memory
ClosedPublic
Actions

Authored by andrew on Dec 15 2021, 11:04 AM.

Details

Reviewers

markj
kib
alc
cperciva
manu

Group Reviewers

arm64

Commits

rG28e0603813cd: Don't fail changing props for unmapped DMAP memory
rG69cfbd669e34: Don't fail changing props for unmapped DMAP memory

Summary

When recursing in pmap_change_props_locked we may fail because there is
no pte. This shouldn't be considered a fail as it may happen in a few
cases, e.g. there are multiple normal memory ranges with device memory
between them.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

andrew created this revision.Dec 15 2021, 11:04 AM

Herald added a reviewer: manu. · View Herald TranscriptDec 15 2021, 11:04 AM

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

andrew requested review of this revision.Dec 15 2021, 11:04 AM

Harbormaster completed remote builds in B43366: Diff 100056.Dec 15 2021, 11:04 AM

Assuming this is the patch I saw yesterday, it unbreaks the ENA driver..

It's an updated version, so might break it again (but hopefully not). It will now skip over unmapped memory rather than return early.

Just tested the latest version; works fine.

This revision is now accepted and ready to land.Dec 15 2021, 3:45 PM

alc added inline comments.Dec 15 2021, 6:55 PM

sys/arm64/arm64/pmap.c
6027

markj added inline comments.Dec 16 2021, 3:54 PM

sys/arm64/arm64/pmap.c
555	The last sentence of this comment isn't accurate now.
558	There is a very similar function in arm64/iommu/iommu_pmap.c that still has the old behaviour w.r.t. setting `*level` when a PTE is missing. I think it would be better to keep them consistent.
587	The assertion `lvl == 3` in pmap_qremove() doesn't catch some erroneous cases now.

andrew added inline comments.Dec 16 2021, 4:28 PM

sys/arm64/arm64/pmap.c
558	I think I was confused and the code was correct, however it should start with `pmap_l0` so we can skip over the l0 entry if it's unmapped.
587	Should be fixed in D33509

alc added inline comments.Dec 16 2021, 4:59 PM

sys/arm64/arm64/pmap.c
558	I also think that the code was correct, and that that the proposed changes to pmap_pte() should be undone.

Extrace pmap_pte change to D33583

This revision now requires review to proceed.Dec 20 2021, 10:12 AM

Harbormaster completed remote builds in B43497: Diff 100336.Dec 20 2021, 10:12 AM

andrew marked an inline comment as done.Dec 20 2021, 10:12 AM

alc accepted this revision.Dec 21 2021, 4:58 AM

This revision is now accepted and ready to land.Dec 21 2021, 4:58 AM

So for the case of setting memory as uncacheable (when wbinv_range() is called), do you end up calling this function on unmapped range? Does it work on arm64?

We only ever call wbinv_range when ptep != NULL so a mapping will exist for the current virtual address. If the DMAP is unmapped the caller will perform the cache management. If the DMAP is mapped cpu_dcache_wbinv_range will be called twice, once for the non-DMAP, and once for the DMAP memory.

kib accepted this revision.Dec 21 2021, 11:32 AM

Closed by commit rG69cfbd669e34: Don't fail changing props for unmapped DMAP memory (authored by andrew). · Explain WhyDec 21 2021, 1:06 PM

This revision was automatically updated to reflect the committed changes.

andrew added a commit: rG69cfbd669e34: Don't fail changing props for unmapped DMAP memory.

andrew added a commit: rG28e0603813cd: Don't fail changing props for unmapped DMAP memory.Dec 29 2021, 10:17 AM