Extra newline.

This var is write-only.

Write-only.

The same condition may occur in pmap_enter_object().

Shouldn't this condition also block superpage promotion?

Is ppr leaked if rangeset_insert() fails?

Why not do the TLB invalidation in this function?

What if eva > VM_MAXUSER_ADDRESS?

If you split alloc/initialization and insertion of the pkru, you can use M_WAITOK for the allocation. I believe we generally try to avoid syscall failures due to transient ENOMEM conditions.

Hmm, I see that the rangeset operations may fail with ENOMEM anyway. This is a design flaw IMHO.

Should we have a dedicated ucode for this case?

What if a user application triggers a kernel mode protection key violation with copyin/out?

Extra newline.

It is used by PG_PTE_PROMOTE.

No, it is used by PG_PTE_PROMOTE definition.

Yes, I missed pmap_enter_object().

Promotion is handled by the updated definition of PG_PTE_PROMOTE.

Yes, should be fixed.

In fact nothing too bad would happen. But indeed the intent was to check the eva and not sva.

It is more complicated than just split of the new entry. Problem is that removal might need to split and then it also needs to allocate.

During the initial stages of the development, I tried to use per-pmap sx lock to protect the pkru rangeset, but it is too inconsistent and I decided that the only sane approach is to protect it with the same lock as the page tables, ie. pmap mutex.

I will think about providing pre-allocated elements for rangeset operations, but it even brief evaluation shows that rangeset interface will become extra-ugly. I do not like M_NOWAIT failures as well, sure.

I do not have an opinion there. After all, it is page protection. Not adding MD code might be good.

Such access must only occur with pcb_onfault set to non-NULL, and then the condition below would jump to the handler after return from the fault. If kernel is tricked to access usermode without pcb_onfault, then we panic as should.

Why is it NOFREE?

Extra braces.

From context it looks like we should be able to use M_WAITOK here.

Extra braces.

Why not move the check into pmap_enter_pde()?

The comparison with VM_MAXUSER_ADDRESS is redundant.

ctx is __unused.

ctx is __unused.

Perhaps assert that eva < VM_MAXUSER_ADDRESS?

sva is modified during the loop, the original value needs to be saved.

Oops, right.

It does not need to, before ENOMEM restart I tried to minimize the chances of failing allocations.

What do you mean by extra braces? {} are optional by style, and I put them around multiline statements.

The flag is not used during rangeset_init(), it is saved and used during the rangeset operations like _remove() when we execute under pmap lock.

This is very good question, because pmap_enter_pde() might sleep waiting for new pdp page and then the pmap_pkru_same() check is invalidated. So the check must be moved to pmap_enter_pde() after pdp allocation, and pmap_enter_object() can avoid the call.

You mean, in addition to the sva check above ?

This was the reason why invalidate_range was done outside the function.

I would avoid using NOFREE for zones whose usage can be easily influenced by userspace. The fragmentation that they cause is quite problematic, and their use should be strongly discouraged IMO.

Hmm, ok. I thought we tried to strictly avoid redundant braces in VM/pmap code (even for multiline statements). I'll stop commenting on that.

Hrm. What if the user changes the keyid for the range during the sleep?

In general it seems that pmap functions that operate on a range of VAs will handle invalidation themselves, so I suggested the change to be consistent.

I don't see why msva shouldn't just be called va.

Yes, I already removed the NOFREE.

I moved pmap_pkru_get() down into pmap_enter_pde(). Then it allowed to remove the cleaning of the key bits for small page insertion.

I wonder if we should use a write lock here. With the ENOMEM handling it's possible that multiple threads are racing to modify the same VA range. It might be preferable to use a dedicated sx lock instead of the vm map lock, to avoid blocking unrelated operations.

Doesn't vm_fault_soft_fast() need the same treatment? Maybe this should be handled internally by pmap_enter().

Why not use vm_map_unlock()?

I should have added a comment there. The consistency of single VA is handled by the pmap lock. With the additional guarantee of the rangeset_remove() to not modify the rangeset if an error is returned, the ENOMEM retry still should be fine.

This lock prevents parallel vmspace_fork() from receiving inconsistent protection settings if one thread forks and another modifies pmap protection keys for some range.

vm_fault_soft_fast() returns error to vm_fault() and then the fault handler executes 'slow' non-superpage pmap_enter().

First I thought that indeed inserting 4k entries in pmap_enter() and returning success would be fine, but really it is hard to distinguish other failures from the specific configuration (which is also not real failure). More, current contract for pmap_enter() is that the function that just does what directed; if pmap_enter(psind = 1) inserts non-superpage mapping it looks somewhat broken.

For the same reason why plain sx_xunlock() is used at the end of the function, see comment (we cannot process deferred free entries until both maps are unlocked). That said, the code should call the processing function even if formally.

Style: missing newline.

Why _NOFREE?

I would call this rangeset_lookup(). To me at least, _get() implies that the range is known to be present, i.e., the lookup cannot fail. Same with pmap_pkru_get().

I believe this will fail if an earlier attempt returned ENOMEM and we did a partial copy.

Fair enough. I agree that it would be odd for pmap_enter(psind = 1) to implement a 4KB fallback.

FIxed by removing everything on copy failure.