arm64: Fix pmap_copy()'s handling of 2MB mappings
ClosedPublic
Actions

Authored by markj on Jun 4 2021, 6:22 PM.

Details

Reviewers

manu
alc
kib

Group Reviewers

arm64

Commits

rGfa3cfd1a845a: arm64: Fix pmap_copy()'s handling of 2MB mappings
rG4e4035ef1fb5: arm64: Fix pmap_copy()'s handling of 2MB mappings

Summary

When copying mappings from parent to child, we clear the accessed and
dirty bits. This is done for both 4KB and 2MB PTEs. However,
pmap_demote_l2() asserts that writable superpages must be dirty. This
is to avoid races with the MMU setting the dirty bit during promotion
and demotion. pmap_copy() can create clean, writable superpage
mappings, so it violates this assertion.

Modify pmap_copy() to make new 2MB mappings read-only, like we do on
amd64. I am not sure though why we shouldn't simply copy the dirty bit
over to the child.

Fixes: ca2cae0b4dd
Reported by: Jenkins via mhorne

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 39748
Build 36637: arc lint + arc unit

Event Timeline

markj created this revision.Jun 4 2021, 6:22 PM

Herald added a reviewer: manu. · View Herald TranscriptJun 4 2021, 6:22 PM

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

markj requested review of this revision.Jun 4 2021, 6:22 PM

Harbormaster completed remote builds in B39711: Diff 90427.Jun 4 2021, 6:22 PM

markj added a child revision: D30644: riscv: Handle hardware-managed dirty bit updates in pmap_promote_l2().Jun 4 2021, 6:22 PM

markj added reviewers: alc, kib, arm64.Jun 4 2021, 6:23 PM

Don't we mark the copied mapping as clean to avoid unnecessary writes? Suppose that the source mapping is destroyed, and backing pages are marked dirty and written to the storage. Now, if the copied mapping is destroyed without ever being written to, we would re-dirty and write them again.

This revision is now accepted and ready to land.Jun 4 2021, 8:24 PM

In D30643#688425, @kib wrote:

Don't we mark the copied mapping as clean to avoid unnecessary writes? Suppose that the source mapping is destroyed, and backing pages are marked dirty and written to the storage. Now, if the copied mapping is destroyed without ever being written to, we would re-dirty and write them again.

When writing the dirty pages, won't we downgrade all writable mappings to RO and clear dirty bits from the copied mapping as well?

In D30643#688428, @markj wrote:

In D30643#688425, @kib wrote:

Don't we mark the copied mapping as clean to avoid unnecessary writes? Suppose that the source mapping is destroyed, and backing pages are marked dirty and written to the storage. Now, if the copied mapping is destroyed without ever being written to, we would re-dirty and write them again.

When writing the dirty pages, won't we downgrade all writable mappings to RO and clear dirty bits from the copied mapping as well?

But then system would need to demote and invalidate TLB. I still think that not marking mapping as dirty is the right thing to do, when we know that there were no write.

You wrote, "Modify pmap_copy() to make new 2MB mappings read-only, like we do on amd64. I am not sure though why we shouldn't simply copy the dirty bit over to the child."

I'm confused. :-) I'm looking at the amd64 source code right now, and for 2MB page mappings it copies PG_A and PG_M. Moreover, it copies PG_RW. However, for 4KB page mappings, it does clear PG_A and PG_M. In other words, we don't handle different page sizes uniformly.

In D30643#688707, @alc wrote:

You wrote, "Modify pmap_copy() to make new 2MB mappings read-only, like we do on amd64. I am not sure though why we shouldn't simply copy the dirty bit over to the child."

I'm confused. :-) I'm looking at the amd64 source code right now, and for 2MB page mappings it copies PG_A and PG_M. Moreover, it copies PG_RW. However, for 4KB page mappings, it does clear PG_A and PG_M. In other words, we don't handle different page sizes uniformly.

I see, I switched them around in my head when reading the code. The amd64 behaviour makes more sense to me now.

Preserve AF, DBM and RW flags when copying 2MB pages.

This revision now requires review to proceed.Jun 6 2021, 4:23 PM

Harbormaster completed remote builds in B39748: Diff 90486.Jun 6 2021, 4:23 PM

alc accepted this revision.Jun 6 2021, 6:20 PM

This revision is now accepted and ready to land.Jun 6 2021, 6:20 PM

I still think it is better to clear PG_A|PG_M|PG_RW on copy of 2M pages for amd64. Ideally we would only clear PG_A|PG_M, but this breaks known invariant.

In D30643#688425, @kib wrote:

Don't we mark the copied mapping as clean to avoid unnecessary writes? Suppose that the source mapping is destroyed, and backing pages are marked dirty and written to the storage. Now, if the copied mapping is destroyed without ever being written to, we would re-dirty and write them again.

No. To write the page(s) out, we would first perform a pmap_remove_write(), which clears PG_M from all of the mappings, including the copied ones.

Closed by commit rG4e4035ef1fb5: arm64: Fix pmap_copy()'s handling of 2MB mappings (authored by markj). · Explain WhyJun 6 2021, 8:47 PM

This revision was automatically updated to reflect the committed changes.

markj added a commit: rG4e4035ef1fb5: arm64: Fix pmap_copy()'s handling of 2MB mappings.

markj added a commit: rGfa3cfd1a845a: arm64: Fix pmap_copy()'s handling of 2MB mappings.Jun 14 2021, 8:27 PM