Re: [PATCH] powerpc/pseries: Fix MCE handling on pseries

[Ganesh <ganeshgr@linux.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 4256 bytes --]



On 3/20/20 8:11 AM, Nicholas Piggin wrote:
> Ganesh's on March 18, 2020 12:35 am:
>>
>> On 3/17/20 3:31 PM, Nicholas Piggin wrote:
>>> Ganesh's on March 16, 2020 9:47 pm:
>>>> On 3/14/20 9:18 AM, Nicholas Piggin wrote:
>>>>> Ganesh Goudar's on March 14, 2020 12:04 am:
>>>>>> MCE handling on pSeries platform fails as recent rework to use common
>>>>>> code for pSeries and PowerNV in machine check error handling tries to
>>>>>> access per-cpu variables in realmode. The per-cpu variables may be
>>>>>> outside the RMO region on pSeries platform and needs translation to be
>>>>>> enabled for access. Just moving these per-cpu variable into RMO region
>>>>>> did'nt help because we queue some work to workqueues in real mode, which
>>>>>> again tries to touch per-cpu variables.
>>>>> Which queues are these? We should not be using Linux workqueues, but the
>>>>> powerpc mce code which uses irq_work.
>>>> Yes, irq work queues accesses memory outside RMO.
>>>> irq_work_queue()->__irq_work_queue_local()->[this_cpu_ptr(&lazy_list) | this_cpu_ptr(&raised_list)]
>>> Hmm, okay.
>>>
>>>>>> Also fwnmi_release_errinfo()
>>>>>> cannot be called when translation is not enabled.
>>>>> Why not?
>>>> It crashes when we try to get RTAS token for "ibm, nmi-interlock" device
>>>> tree node. But yes we can avoid it by storing it rtas_token somewhere but haven't
>>>> tried it, here is the backtrace I got when fwnmi_release_errinfo() called from
>>>> realmode handler.
>>> Okay, I actually had problems with that messing up soft-irq state too
>>> and so I sent a patch to get rid of it, but that's the least of your
>>> problems really.
>>>
>>>>>> This patch fixes this by enabling translation in the exception handler
>>>>>> when all required real mode handling is done. This change only affects
>>>>>> the pSeries platform.
>>>>> Not supposed to do this, because we might not be in a state
>>>>> where the MMU is ready to be turned on at this point.
>>>>>
>>>>> I'd like to understand better which accesses are a problem, and whether
>>>>> we can fix them all to be in the RMO.
>>>> I faced three such access problems,
>>>>     * accessing per-cpu data (like mce_event,mce_event_queue and mce_event_queue),
>>>>       we can move this inside RMO.
>>>>     * calling fwnmi_release_errinfo().
>>>>     * And queuing work to irq_work_queue, not sure how to fix this.
>>> Yeah. The irq_work_queue one is the biggest problem.
>>>
>>> This code "worked" prior to the series unifying pseries and powernv
>>> machine check handlers, 9ca766f9891d ("powerpc/64s/pseries: machine
>>> check convert to use common event code") and friends. But it does in
>>> basically the same way as your fix (i.e., it runs this early handler
>>> in virtual mode), but that's not really the right fix.
>>>
>>> Consider: you get a SLB multi hit on a kernel address due to hardware or
>>> software error. That access causes a MCE, but before the error can be
>>> decode to save and flush the SLB, you turn on relocation and that
>>> causes another SLB multi hit...
>> We turn on relocation only after all the realmode handling/recovery is done
>> like SLB flush and reload, All we do after we turn relocation on is saving
>> mce event to array and queuing the work to irq_workqueue.
> Oh I see, fwnmi_release_errinfo is done after mce_handle_error, I didnt
> read your comment closely!
>
> That means the recovery is done with MSR[ME]=0, which means saving the
> SLB entries can take a machine check which will turn into a checkstop,
> or walking user page tables and loading memory to handle memory
> failures.
>
> We really should release that immediately so we get ME back on.
>
>> So we are good to turn it on here.
> Possibly. I don't think it's generally a good idea enable relocation
> from an interrupted relocation off context, but yeah this might be okay.
>
> I think FWNMI mce needs to be fixed to not do this, and do the
> nmi-interlock earlier, but for now your patch I guess improves things
> significantly. So, okay let's go with it.
>
> You should be able to just use mtmsrd to switch to virtual mode, so no
> need for the asm code.
>
>    mtmsr(mfmsr()|MSR_IR|MSR_DR);

Sure, Thanks

>
> Otherwise,
>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
> Thanks,
> Nick


[-- Attachment #2: Type: text/html, Size: 6357 bytes --]