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.

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.

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.

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.

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.

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.

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().

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);
}