Messages in this thread | | | Subject | Re: [RFC PATCH v3 2/5] powerpc: Exception hooks for context tracking subsystem | From | Li Zhong <> | Date | Mon, 13 May 2013 16:44:40 +0800 |
| |
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. > >
| |