Re: [RFC PATCH v3 2/5] powerpc: Exception hooks for context tracking subsystem

[Li Zhong <zhong@linux.vnet.ibm.com>]

On Mon, 2013-05-13 at 15:57 +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2013-05-13 at 13:21 +0800, Li Zhong wrote:
> >  	int recover = 0;
> > +	enum ctx_state prev_state;
> > +
> > +	prev_state = exception_enter();
> 
> Please make it nicer:
> 
> 	enum ctx_state prev_state = exception_enter();
> 	int recover = 0;

OK, I'll fix it, and all the others.

> >  	__get_cpu_var(irq_stat).mce_exceptions++;
> >  
> > @@ -683,7 +687,7 @@ void machine_check_exception(struct pt_regs *regs)
> >  		recover = cur_cpu_spec->machine_check(regs);
> >  
> >  	if (recover > 0)
> > -		return;
> > +		goto exit;
> 
> I'm no fan of "exit" as a label as it's otherwise a libc function (I
> know there is no relationship here but I just find that annoying).
> 
> I usually use "bail"

OK, I'll fix it, and all the others.

> 
> >  #if defined(CONFIG_8xx) && defined(CONFIG_PCI)
> >  	/* the qspan pci read routines can cause machine checks -- Cort
> > @@ -693,20 +697,23 @@ void machine_check_exception(struct pt_regs *regs)
> >  	 * -- BenH
> >  	 */
> >  	bad_page_fault(regs, regs->dar, SIGBUS);
> > -	return;
> > +	goto exit;
> >  #endif
> >  
> >  	if (debugger_fault_handler(regs))
> > -		return;
> > +		goto exit;
> >  
> >  	if (check_io_access(regs))
> > -		return;
> > +		goto exit;
> >  
> >  	die("Machine check", regs, SIGBUS);
> >  
> >  	/* Must die if the interrupt is not recoverable */
> >  	if (!(regs->msr & MSR_RI))
> >  		panic("Unrecoverable Machine check");
> > +
> > +exit:
> > +	exception_exit(prev_state);
> >  }
> >  
> >  void SMIException(struct pt_regs *regs)
> > @@ -716,20 +723,31 @@ void SMIException(struct pt_regs *regs)
> >  
> >  void unknown_exception(struct pt_regs *regs)
> >  {
> > +	enum ctx_state prev_state;
> > +	prev_state = exception_enter();
> > +
> 
> Same cosmetic comment for all other cases
> 
>  ..../...
> 
> >  #include <asm/firmware.h>
> >  #include <asm/page.h>
> > @@ -193,8 +194,8 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr, int fault)
> >   * The return value is 0 if the fault was handled, or the signal
> >   * number if this is a kernel fault that can't be handled here.
> >   */
> > -int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
> > -			    unsigned long error_code)
> > +static int __kprobes __do_page_fault(struct pt_regs *regs,
> > +				unsigned long address, unsigned long error_code)
> >  {
> >  	struct vm_area_struct * vma;
> >  	struct mm_struct *mm = current->mm;
> > @@ -475,6 +476,17 @@ bad_area_nosemaphore:
> >  
> >  }
> >  
> > +int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
> > +			    unsigned long error_code)
> > +{
> > +	int ret;
> > +	enum ctx_state prev_state;
> > +	prev_state = exception_enter();
> > +	ret = __do_page_fault(regs, address, error_code);
> > +	exception_exit(prev_state);
> > +	return ret;
> > +}
> 
> Any reason why you didn't use the same wrapping trick in traps.c ? I'd
> rather we stay consistent in the way we do things between the two files.

OK, I'll make it consistent with those in traps.c

> 
> >  /*
> >   * bad_page_fault is called when we have a bad access from the kernel.
> >   * It is called from the DSI and ISI handlers in head.S and from some
> > diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
> > index 88ac0ee..d92fb26 100644
> > --- a/arch/powerpc/mm/hash_utils_64.c
> > +++ b/arch/powerpc/mm/hash_utils_64.c
> > @@ -33,6 +33,7 @@
> >  #include <linux/init.h>
> >  #include <linux/signal.h>
> >  #include <linux/memblock.h>
> > +#include <linux/context_tracking.h>
> >  
> >  #include <asm/processor.h>
> >  #include <asm/pgtable.h>
> > @@ -962,6 +963,9 @@ int hash_page(unsigned long ea, unsigned long access, unsigned long trap)
> >  	const struct cpumask *tmp;
> >  	int rc, user_region = 0, local = 0;
> >  	int psize, ssize;
> > +	enum ctx_state prev_state;
> > +
> > +	prev_state = exception_enter();
> >  
> >  	DBG_LOW("hash_page(ea=%016lx, access=%lx, trap=%lx\n",
> >  		ea, access, trap);
> > @@ -973,7 +977,8 @@ int hash_page(unsigned long ea, unsigned long access, unsigned long trap)
> >  		mm = current->mm;
> >  		if (! mm) {
> >  			DBG_LOW(" user region with no mm !\n");
> > -			return 1;
> > +			rc = 1;
> > +			goto exit;
> >  		}
> >  		psize = get_slice_psize(mm, ea);
> >  		ssize = user_segment_size(ea);
> > @@ -992,19 +997,23 @@ int hash_page(unsigned long ea, unsigned long access, unsigned long trap)
> >  		/* Not a valid range
> >  		 * Send the problem up to do_page_fault 
> >  		 */
> > -		return 1;
> > +		rc = 1;
> > +		goto exit;
> >  	}
> >  	DBG_LOW(" mm=%p, mm->pgdir=%p, vsid=%016lx\n", mm, mm->pgd, vsid);
> >  
> >  	/* Bad address. */
> >  	if (!vsid) {
> >  		DBG_LOW("Bad address!\n");
> > -		return 1;
> > +		rc = 1;
> > +		goto exit;
> >  	}
> >  	/* Get pgdir */
> >  	pgdir = mm->pgd;
> > -	if (pgdir == NULL)
> > -		return 1;
> > +	if (pgdir == NULL) {
> > +		rc = 1;
> > +		goto exit;
> > +	}
> >  
> >  	/* Check CPU locality */
> >  	tmp = cpumask_of(smp_processor_id());
> > @@ -1027,7 +1036,8 @@ int hash_page(unsigned long ea, unsigned long access, unsigned long trap)
> >  	ptep = find_linux_pte_or_hugepte(pgdir, ea, &hugeshift);
> >  	if (ptep == NULL || !pte_present(*ptep)) {
> >  		DBG_LOW(" no PTE !\n");
> > -		return 1;
> > +		rc = 1;
> > +		goto exit;
> >  	}
> >  
> >  	/* Add _PAGE_PRESENT to the required access perm */
> > @@ -1038,13 +1048,16 @@ int hash_page(unsigned long ea, unsigned long access, unsigned long trap)
> >  	 */
> >  	if (access & ~pte_val(*ptep)) {
> >  		DBG_LOW(" no access !\n");
> > -		return 1;
> > +		rc = 1;
> > +		goto exit;
> >  	}
> >  
> >  #ifdef CONFIG_HUGETLB_PAGE
> > -	if (hugeshift)
> > -		return __hash_page_huge(ea, access, vsid, ptep, trap, local,
> > +	if (hugeshift) {
> > +		rc = __hash_page_huge(ea, access, vsid, ptep, trap, local,
> >  					ssize, hugeshift, psize);
> > +		goto exit;
> > +	}
> >  #endif /* CONFIG_HUGETLB_PAGE */
> >  
> >  #ifndef CONFIG_PPC_64K_PAGES
> > @@ -1124,6 +1137,8 @@ int hash_page(unsigned long ea, unsigned long access, unsigned long trap)
> >  		pte_val(*(ptep + PTRS_PER_PTE)));
> >  #endif
> >  	DBG_LOW(" -> rc=%d\n", rc);
> > +exit:
> > +	exception_exit(prev_state);
> >  	return rc;
> >  }
> >  EXPORT_SYMBOL_GPL(hash_page);
> > @@ -1259,6 +1274,9 @@ void flush_hash_range(unsigned long number, int local)
> >   */
> >  void low_hash_fault(struct pt_regs *regs, unsigned long address, int rc)
> >  {
> > +	enum ctx_state prev_state;
> > +	prev_state = exception_enter();
> > +
> >  	if (user_mode(regs)) {
> >  #ifdef CONFIG_PPC_SUBPAGE_PROT
> >  		if (rc == -2)
> > @@ -1268,6 +1286,8 @@ void low_hash_fault(struct pt_regs *regs, unsigned long address, int rc)
> >  			_exception(SIGBUS, regs, BUS_ADRERR, address);
> >  	} else
> >  		bad_page_fault(regs, address, SIGBUS);
> > +
> > +	exception_exit(prev_state);
> >  }
> 
> So the above comes from the return of hash page following an error, it's
> technically the same exception. I don't know if that matters however.
> Also some exceptions are directly handled in asm such as SLB misses,
> again I don't know if that matters as I'm not familiar with what the
> context tracing code is actually doing.

Yes, the above and hash_page() are two C functions for a same exception.
And the exception hooks enable RCU usage in those C codes. But for asm
codes, I think we could assume that there would be no RCU usage there,
so we don't need wrap them in the hooks.

Thanks, Zhong

> 
> >  long hpte_insert_repeating(unsigned long hash, unsigned long vpn,
> 
> Cheers,
> Ben.
> 
>