From: Martin Pieuchot Subject: Re: km_alloc(9), UAREA and kv_pageable To: Mark Kettenis Cc: tech@openbsd.org Date: Sun, 20 Oct 2024 19:47:23 +0200 On 20/10/24(Sun) 14:36, Mark Kettenis wrote: > > Date: Sun, 20 Oct 2024 11:37:57 +0200 > > From: Martin Pieuchot > > Sorry, I didn't have time yet to investigate. But I'd rather not > change the km_alloc() API if it is not necessary, especially when the > it introduces a somewhat vague concept like "managed" that doesn't > even make sense in the context where it gets used. > > > On 10/10/24(Thu) 10:40, Martin Pieuchot wrote: > > > On 09/10/24(Wed) 13:39, Mark Kettenis wrote: > > > > > Date: Wed, 9 Oct 2024 13:16:25 +0200 > > > > > From: Martin Pieuchot > > > > > > > > > > On 02/10/24(Wed) 17:28, Martin Pieuchot wrote: > > > > > > It is currently impossible to use km_alloc(9) to get a managed mapping > > > > > > backed with physical pages allocated up front. I guess this is why > > > > > > uvm_km_kmemalloc_pla() is still used to allocate the UAREA. > > > > > > > > > > > > To fix that, I suggest the diff below which turns the "kp_pageable" flag > > > > > > of km_alloc(9) into a "kp_managed". This new flag no longer implies > > > > > > "kp_nomem" so I updated the description and fixed the remaining XXX to > > > > > > preserve existing behavior of the `kp_pageable' global. > > > > > > > > > > > > With this change I believe we could also get rid of uvm_km_zalloc() used > > > > > > in the i386 pmap. > > > > > > > > > > > > Comments? Oks? > > > > > > > > > > Anyone? > > > > > > > > I don't really understand why this needs to be "managed" memory. We > > > > stopped paging out the uarea ages ago, and I wonder if all this > > > > complexity is just from the time when we did. Can't this just be > > > > kp_zero memory? > > > > > > On amd64 using kp_zero results in the kernel faulting in loop from which > > > it cannot recover. Could you please look at it and tell me if there is a > > > bug somewhere else? > > Since the memory is no longer "managed", setguardpage() needs to use > pmap_kremove() instead of pmap_remove(). Nice found! > The following diff works for me on both amd64 and arm64. Lovely, should we test this on more architectures? Or are you confident with it as it is? > Index: arch/amd64/amd64/vm_machdep.c > =================================================================== > RCS file: /cvs/src/sys/arch/amd64/amd64/vm_machdep.c,v > diff -u -p -r1.47 vm_machdep.c > --- arch/amd64/amd64/vm_machdep.c 11 Apr 2023 00:45:07 -0000 1.47 > +++ arch/amd64/amd64/vm_machdep.c 20 Oct 2024 12:34:07 -0000 > @@ -135,8 +135,7 @@ cpu_exit(struct proc *p) > void > setguardpage(struct proc *p) > { > - pmap_remove(pmap_kernel(), (vaddr_t)p->p_addr + PAGE_SIZE, > - (vaddr_t)p->p_addr + 2 * PAGE_SIZE); > + pmap_kremove((vaddr_t)p->p_addr + PAGE_SIZE, PAGE_SIZE); > pmap_update(pmap_kernel()); > } > > Index: uvm/uvm_glue.c > =================================================================== > RCS file: /cvs/src/sys/uvm/uvm_glue.c,v > diff -u -p -r1.85 uvm_glue.c > --- uvm/uvm_glue.c 8 Oct 2024 02:29:10 -0000 1.85 > +++ uvm/uvm_glue.c 20 Oct 2024 12:34:07 -0000 > @@ -257,20 +257,18 @@ uvm_vsunlock_device(struct proc *p, void > uvm_km_free(kernel_map, kva, sz); > } > > +const struct kmem_va_mode kv_uarea = { > + .kv_map = &kernel_map, > + .kv_align = USPACE_ALIGN > +}; > + > /* > * uvm_uarea_alloc: allocate the u-area for a new thread > */ > vaddr_t > uvm_uarea_alloc(void) > { > - vaddr_t uaddr; > - > - uaddr = uvm_km_kmemalloc_pla(kernel_map, uvm.kernel_object, USPACE, > - USPACE_ALIGN, UVM_KMF_ZERO, > - no_constraint.ucr_low, no_constraint.ucr_high, > - 0, 0, USPACE/PAGE_SIZE); > - > - return (uaddr); > + return (vaddr_t)km_alloc(USPACE, &kv_uarea, &kp_zero, &kd_waitok); > } > > /* > @@ -282,7 +280,7 @@ uvm_uarea_alloc(void) > void > uvm_uarea_free(struct proc *p) > { > - uvm_km_free(kernel_map, (vaddr_t)p->p_addr, USPACE); > + km_free(p->p_addr, USPACE, &kv_uarea, &kp_zero); > p->p_addr = NULL; > } >