svn commit: r253135 - in projects/bhyve_npt_pmap/sys/amd64: include vmm

[Neel Natu]

Hi Konstantin,

On Wed, Jul 10, 2013 at 12:20 AM, Konstantin Belousov
<kostikbel at gmail.com> wrote:
> On Wed, Jul 10, 2013 at 07:12:55AM +0000, Neel Natu wrote:
>> Author: neel
>> Date: Wed Jul 10 07:12:55 2013
>> New Revision: 253135
>> URL: http://svnweb.freebsd.org/changeset/base/253135
>>
>> Log:
>>   Replace vm_gpa2hpa() with a pair of functions vm_gpa_hold()/vm_gpa_release().
>>
>>   We guarantee that the vm_page backing the 'gpa' is not reclaimed by
>>   the page daemon until the caller indicates that they are done using it
>>   by calling 'vm_gpa_release()'.
>>
>> Modified:
>>   projects/bhyve_npt_pmap/sys/amd64/include/vmm.h
>>   projects/bhyve_npt_pmap/sys/amd64/vmm/vmm.c
>>   projects/bhyve_npt_pmap/sys/amd64/vmm/vmm_dev.c
>>   projects/bhyve_npt_pmap/sys/amd64/vmm/vmm_instruction_emul.c
>>
>> Modified: projects/bhyve_npt_pmap/sys/amd64/include/vmm.h
>> ==============================================================================
>> --- projects/bhyve_npt_pmap/sys/amd64/include/vmm.h   Wed Jul 10 06:46:46 2013        (r253134)
>> +++ projects/bhyve_npt_pmap/sys/amd64/include/vmm.h   Wed Jul 10 07:12:55 2013        (r253135)
>> @@ -93,6 +93,9 @@ const char *vm_name(struct vm *vm);
>>  int vm_malloc(struct vm *vm, vm_paddr_t gpa, size_t len);
>>  int vm_map_mmio(struct vm *vm, vm_paddr_t gpa, size_t len, vm_paddr_t hpa);
>>  int vm_unmap_mmio(struct vm *vm, vm_paddr_t gpa, size_t len);
>> +void *vm_gpa_hold(struct vm *, vm_paddr_t gpa, size_t len, int prot,
>> +               void **cookie);
>> +void vm_gpa_release(void *cookie);
>>  vm_paddr_t vm_gpa2hpa(struct vm *vm, vm_paddr_t gpa, size_t size);
>>  int vm_gpabase2memseg(struct vm *vm, vm_paddr_t gpabase,
>>             struct vm_memory_segment *seg);
>>
>> Modified: projects/bhyve_npt_pmap/sys/amd64/vmm/vmm.c
>> ==============================================================================
>> --- projects/bhyve_npt_pmap/sys/amd64/vmm/vmm.c       Wed Jul 10 06:46:46 2013        (r253134)
>> +++ projects/bhyve_npt_pmap/sys/amd64/vmm/vmm.c       Wed Jul 10 07:12:55 2013        (r253135)
>> @@ -439,16 +439,48 @@ vm_malloc(struct vm *vm, vm_paddr_t gpa,
>>       return (0);
>>  }
>>
>> -vm_paddr_t
>> -vm_gpa2hpa(struct vm *vm, vm_paddr_t gpa, size_t len)
>> +void *
>> +vm_gpa_hold(struct vm *vm, vm_paddr_t gpa, size_t len, int reqprot,
>> +         void **cookie)
>>  {
>> -     vm_paddr_t nextpage;
>> +     int rv, pageoff;
>> +     vm_page_t m;
>> +     struct proc *p;
>> +
>> +     pageoff = gpa & PAGE_MASK;
>> +     if (len > PAGE_SIZE - pageoff)
>> +             panic("vm_gpa_hold: invalid gpa/len: 0x%016lx/%lu", gpa, len);
>> +
>> +     p = curthread->td_proc;
>> +
>> +     PROC_LOCK(p);
>> +     p->p_lock++;
>> +     PROC_UNLOCK(p);
> Why do you need to hold the process there ?

I was following the idiom in trap_pfault() - the comment in trap.c
about swapout was enough to convince me :-)

> I do not think that hold in the trap handler is really useful, probably
> the reverse.

I am happy to remove the lock if isn't needed.

Could you explain a bit more on why it may be detrimental or point me
in the right direction so I can look for myself.

best
Neel