Subject: Re: emulation-specific page fault handling
To: Emmanuel Dreyfus <manu@netbsd.org>
From: Jaromir Dolecek <jdolecek@netbsd.org>
List: tech-kern
Date: 09/07/2002 22:03:48
> We have a remaining problem, because the vm_space can be modified by
> memory fault traps, in particular when we have a COW. Chuck Silvers
> suggested to introduce a e_fault in struct emul that points to uvm_fault
> for natives processes and to irix_vm_fault for IRIX processes. 

Since there isn't any compat_irix LKM, you could always use the
old-style blurbs ;)

#ifdef COMPAT_IRIX
	if (p->p_emul == &emul_irix)
		irix_vm_fault(p, ...);
	else
#endif
		uvm_fault(...);

Seriously, though - your proposal seems fine, as far as the change affects
only mips code (where it's needed).

> In sys/arch/mips/mips/trap.c, we would replace occurences of uvm_fault
> by (*p->p_emul->e_fault). 

It might be better to use something along:
	if (__predict_false(p->p_emul->e_fault != NULL))
		(*p->p_emul->e_fault)(...)
	else
		uvm_fault(...)

Primarily so that it wouldn't be necessary to set emul struct to anything
unless you want to use it, i.e. that it wouldn't be necessary to change
any sys/compat/ code besides compat/irix.
 
Since it only appears on two places in mips/trap.c, this should
be feasible.

> Other ports trap handlers would remain unaffected for now, but e_fault
> would be in the MI part of struct emul, just in case we need a similar
> mecanism for another emulation later.

I'd probably mark e_fault as either #ifdef __mips__  or with comment saying
where exactly it is currently used and for what purpose. Basically just
to avoid any confusion on whether it is or isn't needed for other compat code.
 
> Another problem is that I need a pointer to struct proc in irix_vm_fault
> in order to walk the share group list (it is in p_emuldata). I suggest
> to introduct struct proc * as an argument to uvm_fault. 

I think it's not generally good to add unneeded arguments to frequently
used functions. Perhaps just use curproc in irix_vm_fault() ?
 
> Opinions about this?
> 
> Last problem: declaring e_fault in struct proc uses vm_fault_t, which
> forces me to include <uvm/uvm_extern.h> in <sys/proc.h>. Unfortunately,
> <uvm/extern.h> needs to include <sys/proc.h> in order to get ltsleep
> definition, which is after the include <uvm/uvm_extern.h> in
> <sys/proc.h>. I end up with duplicated declaration of ltsleep in
> <sys/proc.h> before inlcuding <uvm/uvm_extern.h>, is it the way it
> should be done?

In this case, you could perhaps just cheat and use 'int' instead of vm_fault_t,
i.e. defining e_fault as:
	int	(*e_fault) __P((struct vm_map *, vaddr_t, int, vm_prot_t));

Doesn't seem to cause any warning from compiler (not in a tiny
program I tested this with).

Jaromir
-- 
Jaromir Dolecek <jdolecek@NetBSD.org>            http://www.NetBSD.org/
-=- We should be mindful of the potential goal, but as the tantric    -=-
-=- Buddhist masters say, ``You may notice during meditation that you -=-
-=- sometimes levitate or glow.   Do not let this distract you.''     -=-