I do not think that you need a new variable for this. Compare pg_nx with 0 and set EFER_NXE bit if not zero.

kernelprot seems to be too generic. Might be, kernelnx ?

Look at the fpuinit_bsp1, particularly CPUID_EXTSTATE_XSAVEOPT block.

I believe page tables never need exec perms.

I do not see a need in the kernel option. This should work or not be done at all.

In fact, I do not see a need in the tunable. Perhaps only as the opt-out on machines where the feature appeared buggy. But it should be fixed.

Good point! I'll change it.

I think kernelnx is too specific, since it also changes the way the RW bits are set. I agree that kernelprot is too generic, but I struggled to come up with a better name. I'm open to suggestions. (Other similar suggestions could be kernelrwx or kernelnxrw.) I'm really open for something better.)

Thanks for pointing that out! I didn't know we actually had self-modifying code in the kernel.

Hmmm... My CPU supports XSAVE, but didn't fault on that. I'll need to dig deeper into why not.

If I read the Intel documentation correctly, setting NX on a PDE propagates the NX behavior to all pages mapped by PTEs referenced by the PDE. I don't think that's what we want.

I decided we might need a tunable for debugging purposes. In particular, one way to set breakpoints is to modify instructions. Or, there might be a need to modify random memory as part of debugging.

I'm perfectly fine with removing the kernel option and activating it by default.

It probably has XSAVE, but not XSAVEOPT.

This is mapping of the page tables itself, not the introduction of PDEs into the pt hierarchy. The KPTmap does not map kernel objects, but kernel *page tables*.

I did not added the option when I set NX on DMAP, see r316767.

Tweaking the description here (it's not just read-only) might also help with the tunable name. Maybe "Map the kernel with fine-grained permissions rather than RWX."

You could in fact invert the option to be 'vm.pmap.kernel_rwx' where a 1 setting is the legacy behavior and 0 is the default which is the new behavior. If you go that route than the description would be "Map the entire kernel RWX rather than using fine-grained permissions."

Does that imply we should defer the removal of write from kernel text until after doing any fixups? We could use a second pass to walk the page tables and clear W from executable pages?

Agreed on only adding a tunable (and defaulting to new behavior) and not adding the kernel option.

This works because we don't set cr0.wp until a little bit later in the boot process. So, we aren't enforcing write protection at this point.

I'm fairly certain I'm interpreting this correctly; however, I might not have stated it well. So, let me try again.

(I know you know what is below. I'm merely restating it to pass on my understanding of what this particular structure does. This will let you identify the element of my thinking that is incorrect.)

The kernel page table is rooted at KPML4phys, which contains the PML4Es.

The PDPEs are in KPDPphys. The code puts entries for the PDPEs in KPML4phys starting at KPML4BASE.

The PDEs are in KPDphys. The code puts entries for the PDEs into KPDPphys. Some of the PDEs end up mapping 2M pages. Some of the PDEs point to PTEs.

The PTEs are in KPTphys. The code puts entries for the PTEs into KPDphys. (However, some get over-written with 2M pages.)

According to the Intel SDM (Vol. 3a, section 5.13.2, beginning on page 5-30), if "the execute-disable bit is set in any of the paging-structure entries used to map [a] page", execution will be denied. Table 5-5 illustrates this.

So, my understanding is that if I set the NX bit on entries in KPDphys, the processor will disable execution for any PTEs mapped using that PDE.

(Note that this is distinct from the recursive mapping found my mapping KPML4phys[PML4PML4I] to itself, which vtopte() and vtopde() follow. For those, I agree that we could set the NX bit; however, we could do that at the top level. I'll open a separate review for that, since that is a bit different from what I'm trying to do here.)

I think you should perhaps extend the comment above this range with a note that writes to perform fixups in .text are still permitted until CR0.WP is set.

Nevermind, I misread the fragment, thinking that it is creation of PTmap. Ignore me.

Perhaps order alphabetically ?

Thanks! I changed it.

No problem. It took me multiple times through this code to fully grok it. :-)

Done. (I think. Please check if the new ordering looks OK.)

I am not sure why it is reasonable to check for CR0_WP. IMO it is good enough to always clear _WP before write, and always reload previously saved cr0 content below.

Also, if you do this, it removes the need for the tunable.

I was trying to cut down on writes to the control register. However, I take your point that this will be true > 99% of the time, so it makes sense to just unconditionally write it.

I agree it removes the need for the tunable. I'll take it out.

These should have -Wl, wrapping, and it probably makes sense to commit this separately, with a comment.

The parentheses are not necessary; they are just a coding habit (quirk?). I can take them out.

Thanks! Good catch! I can commit this separately.