Page MenuHomeFreeBSD

Let arm64 pmap_qenter() and pmap_kenter() unconditionally set NX.
ClosedPublic

Authored by markj on Oct 24 2019, 3:25 PM.

Details

Test Plan

I booted a ThunderX with this diff applied.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markj created this revision.Oct 24 2019, 3:25 PM
markj edited the test plan for this revision. (Show Details)Oct 24 2019, 3:26 PM
markj added reviewers: alc, andrew.
alc accepted this revision.Oct 24 2019, 4:50 PM
This revision is now accepted and ready to land.Oct 24 2019, 4:50 PM
andrew accepted this revision.Oct 24 2019, 6:06 PM

Diff broken by rS354193 'Set the userspace execute never bit on kernel mappings' (looks like that sets UXN in the else case instead).

markj updated this revision to Diff 63930.Nov 4 2019, 4:59 PM

Rebase.

This revision now requires review to proceed.Nov 4 2019, 4:59 PM
andrew accepted this revision.Nov 4 2019, 5:18 PM
This revision is now accepted and ready to land.Nov 4 2019, 5:18 PM
alc accepted this revision.Nov 4 2019, 6:24 PM
alc added inline comments.
sys/arm64/arm64/pmap.c
1260 ↗(On Diff #63930)

While you're here, I'd like to suggest that it would good from a stylistic standpoint to add ATTR_AP(ATTR_AP_RW).

alc added inline comments.Nov 4 2019, 6:35 PM
sys/arm64/arm64/pmap.c
1378 ↗(On Diff #63930)

Just an FYI, and not a comment on this patch. I think that we should be implementing break-before-make here. Occasionally, we have used pmap_qenter() to replace a valid mapping. That is why I did not change this to the simpler pmap_store() when I introduced pmap_store().

markj updated this revision to Diff 63933.Nov 4 2019, 6:40 PM
markj marked an inline comment as done.

Explicitly specify ATTR_AP_RW for mappings created by pmap_kenter().

This revision now requires review to proceed.Nov 4 2019, 6:40 PM
alc accepted this revision.Nov 4 2019, 6:46 PM
This revision is now accepted and ready to land.Nov 4 2019, 6:46 PM
markj added inline comments.Nov 5 2019, 2:37 AM
sys/arm64/arm64/pmap.c
1378 ↗(On Diff #63930)

I think this can happen with some implementations of sf_buf_map() (though not on arm64). Are there other examples?

alc added inline comments.Nov 5 2019, 3:49 PM
sys/arm64/arm64/pmap.c
1378 ↗(On Diff #63930)

I won't swear to it, but I thought that I recently saw pmap_qenter() being used to remap a virtual page to a different physical page on a different NUMA node.

Can you commit this or shall I commit D22241 first? They conflict as they both change pmap_qenter() and pmap_kenter().

Can you commit this or shall I commit D22241 first? They conflict as they both change pmap_qenter() and pmap_kenter().

Please go ahead and commit your patch, I will come back to this next week.

This revision was automatically updated to reflect the committed changes.