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

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

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;
 
>  	__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"

>  #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.

>  /*
>   * 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.
 
>  long hpte_insert_repeating(unsigned long hash, unsigned long vpn,

Cheers,
Ben.