lkml.org 
[lkml]   [2013]   [May]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [RFC PATCH v3 2/5] powerpc: Exception hooks for context tracking subsystem
From
Date
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.



\
 
 \ /
  Last update: 2013-05-13 08:41    [W:0.096 / U:0.264 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site