Re: [PATCH v2 1/3] powerpc/mce: Bug fixes for MCE handling in kernel space

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 05/04/2018 à 09:14, Balbir Singh a écrit :
> The code currently assumes PAGE_SHIFT as the shift value of
> the pfn, this works correctly (mostly) for user space pages,
> but the correct thing to do is
> 
> 1. Extract the shift value returned via the pte-walk API's
> 2. Use the shift value to access the instruction address.
> 
> Note, the final physical address still use PAGE_SHIFT for
> computation. handle_ierror() is not modified and handle_derror()
> is modified just for extracting the correct instruction
> address.
> 
> This is largely due to __find_linux_pte() returning pfn's
> shifted by pdshift. The code is much more generic and can
> handle shift values returned.
> 
> Fixes: ba41e1e1ccb9 ("powerpc/mce: Hookup derror (load/store) UE errors")
> 
> Signed-off-by: Balbir Singh <bsingharora@gmail.com>

Series superseded by 
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=126132&state=*

> ---
>   arch/powerpc/kernel/mce_power.c | 26 ++++++++++++++++----------
>   1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/mce_power.c b/arch/powerpc/kernel/mce_power.c
> index fe6fc63251fe..bd9754def479 100644
> --- a/arch/powerpc/kernel/mce_power.c
> +++ b/arch/powerpc/kernel/mce_power.c
> @@ -36,7 +36,8 @@
>    * Convert an address related to an mm to a PFN. NOTE: we are in real
>    * mode, we could potentially race with page table updates.
>    */
> -static unsigned long addr_to_pfn(struct pt_regs *regs, unsigned long addr)
> +static unsigned long addr_to_pfn(struct pt_regs *regs, unsigned long addr,
> +		unsigned int *shift)
>   {
>   	pte_t *ptep;
>   	unsigned long flags;
> @@ -49,13 +50,15 @@ static unsigned long addr_to_pfn(struct pt_regs *regs, unsigned long addr)
>   
>   	local_irq_save(flags);
>   	if (mm == current->mm)
> -		ptep = find_current_mm_pte(mm->pgd, addr, NULL, NULL);
> +		ptep = find_current_mm_pte(mm->pgd, addr, NULL, shift);
>   	else
> -		ptep = find_init_mm_pte(addr, NULL);
> +		ptep = find_init_mm_pte(addr, shift);
>   	local_irq_restore(flags);
>   	if (!ptep || pte_special(*ptep))
>   		return ULONG_MAX;
> -	return pte_pfn(*ptep);
> +	if (!*shift)
> +		*shift = PAGE_SHIFT;
> +	return (pte_val(*ptep) & PTE_RPN_MASK) >> *shift;
>   }
>   
>   /* flush SLBs and reload */
> @@ -353,15 +356,16 @@ static int mce_find_instr_ea_and_pfn(struct pt_regs *regs, uint64_t *addr,
>   	unsigned long pfn, instr_addr;
>   	struct instruction_op op;
>   	struct pt_regs tmp = *regs;
> +	unsigned int shift;
>   
> -	pfn = addr_to_pfn(regs, regs->nip);
> +	pfn = addr_to_pfn(regs, regs->nip, &shift);
>   	if (pfn != ULONG_MAX) {
> -		instr_addr = (pfn << PAGE_SHIFT) + (regs->nip & ~PAGE_MASK);
> +		instr_addr = (pfn << shift) + (regs->nip & ((1 << shift) - 1));
>   		instr = *(unsigned int *)(instr_addr);
>   		if (!analyse_instr(&op, &tmp, instr)) {
> -			pfn = addr_to_pfn(regs, op.ea);
> +			pfn = addr_to_pfn(regs, op.ea, &shift);
>   			*addr = op.ea;
> -			*phys_addr = (pfn << PAGE_SHIFT);
> +			*phys_addr = (pfn << shift);
>   			return 0;
>   		}
>   		/*
> @@ -435,12 +439,14 @@ static int mce_handle_ierror(struct pt_regs *regs,
>   			if (mce_err->severity == MCE_SEV_ERROR_SYNC &&
>   				table[i].error_type == MCE_ERROR_TYPE_UE) {
>   				unsigned long pfn;
> +				unsigned int shift;
>   
>   				if (get_paca()->in_mce < MAX_MCE_DEPTH) {
> -					pfn = addr_to_pfn(regs, regs->nip);
> +					pfn = addr_to_pfn(regs, regs->nip,
> +							&shift);
>   					if (pfn != ULONG_MAX) {
>   						*phys_addr =
> -							(pfn << PAGE_SHIFT);
> +							(pfn << shift);
>   						handled = 1;
>   					}
>   				}