The parens are not needed. Same comment applies in several places below.

We want to set PTE_D only if (flags & VM_PROT_WRITE) != 0. Otherwise this page will be marked dirty unnecessarily.

Don't we want PTE_R set too? Certainly this should be a leaf entry.

Note that this is for a kernel-only mapping, so not quite sure if PTE_D matters? OTOH, we can map swap-backed objects into KVA (e.g. shm_map in sys/kern/uipc_shm.c though there are currently no in-tree consumers, but also the nvidia driver maps VM objects shared with user space into KVA), so we probably don't want to set PTE_D even on kernel mappings of regular VM objects until they are actually written to.

Is pmap_enter() really the right place to be setting these? In the Alpha pmap (which also had software-managed A/D bits), the page fault handler would first check to see if the fault had occurred on a page that needed to set A or D and if it set the equivalent software bit and didn't call vm_fault at all. I think you really want to use similar logic to that in the page fault exception handler, not in pmap_enter. That is, when you get a load or instruction page fault, first look for the existing PTE. If it is valid and has the right permission enabled but doesn't have A set, set A and then retry without calling vm_fault at all. Similarly, for a store page fault, find the the existing PTE and check if either A or D are missing. If they are, set both and then return to retry without calling vm_fault.

Oops, I missed that. I thought we were eagerly setting PTE_D on user mappings here. There are some submaps (exec_map, pipe_map) where this is applicable, but in those cases we are pretty much always going to dirty any accessed page. Anyway, I think it's still right to avoid setting PTE_D unnecessarily.

FWIW, armv4 does this as well. It seems rather unfortunate that we can't avoid this lookup on riscv when the implementation handles setting PTE_A and PTE_D.

The manual actually says to only set A and D aggressively for memio and regions where the OS knows for certain it never needs the bits:

The A and D bits are never cleared by the implementation. If the supervisor software does not rely on accessed and/or dirty bits, e.g. if it does not swap memory pages to secondary storage or if the pages are being used to map I/O space, it should always set them to 1 in the PTE to improve performance.

(albeit this is in one of the italics sections that are just extra prose, not actual "spec")

I kind of think we should probably not be doing eager setting of A/D even for kernel mappings in pmap_enter() but perhaps only for things like the direct map or pmap_mapdev().

Yeah. I don't think there is any way to "know" which implementation is in place (e.g. no equivalent of a x86 cpuid bit). If we found this mattered we could perhaps "notice" implicit A/D settings and then set a global to disable the PTE fetch once we notice the first one. We could also perhaps temporarily map a page at a temporary KVA during boot and touch it and see if we got a fault or not or if A was implicitly set and use that to set the global.

You can trim the parens around '==' inside of '&&', for example:

if (((orig_l3 & (PTE_D | PTE_W)) == PTE_W &&
     (prot & PROT_WRITE != 0)

Also, the trailing \ is kind of odd. It seems like you maybe don't have parentheses around the entire expression currently?

Writes also need to set PTE_A, not just reads. I would just always set PTE_A in new_l3 without any condition.

We don't have to do this now, but I looked at amd64's pmap_enter() and it seems to assume that pmap_enter() is always in response to a fault so it sets PG_A (equivalent of PTE_A) always and it also sets PTE_M (equivalent of PTE_D) if flags (fault type) includes PROT_WRITE (N.B. not based on prot). amd64 does this here at the top of the function when constructing 'newpte' (same thing as 'new_l3' here).

For RISC-V processors that don't do hardware A/D bits that would seem to be an optimization to avoid an instant fault when retrying the instruction.

amd64's pmap also sets PG_NX here conditionally, so PTE_R | PTE_X is probably wanted?

We don't want to update l3 and invalite TLB entry without a reason I think

You've taken a fault, so if the PTE is valid the page is always going to be accessed and the 'orig != new' check means you only invalidate the TLB entry for this page if you are actually setting PTE_A. That said, this is probably fine as you would not want to set PTE_A for a write fault on a read-only mapping. The other way to handle that perhaps would be to verify permissions first and fail early just as you do for checking PTE_V, then the logic to set bits can be simpler:

    if ((orig_l3 & PTE_V) == 0 ||
        ((prot & VM_PROT_WRITE) != 0 && (orig_l3 & PTE_W) == 0) ||
        ((prot & VM_PROT_READ) != 0 && (orig_l3 & PTE_R) == 0))
            return (0);
    
    new_l3 = orig_l3 | PTE_A;
    if ((prot & VM_PROT_WRITE) != 0)
        new_l3 |= PTE_D;

    if (orig_l3 != new_l3) {
        pmap_load_store(l3, new_l3);
        pmap_invalidate_page(mmap, va);
        return (1);
    }

    /* XXX: This case should never happen since it means the PTE shouldn't have resulted in a fault .*/
    return (0);
}