<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<br>
<br>
<div class="moz-cite-prefix">On 3/20/20 8:11 AM, Nicholas Piggin
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:1584670525.n2ybablt2y.astroid@bobo.none">
<pre class="moz-quote-pre" wrap="">Ganesh's on March 18, 2020 12:35 am:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
On 3/17/20 3:31 PM, Nicholas Piggin wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Ganesh's on March 16, 2020 9:47 pm:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
On 3/14/20 9:18 AM, Nicholas Piggin wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Ganesh Goudar's on March 14, 2020 12:04 am:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Which queues are these? We should not be using Linux workqueues, but the
powerpc mce code which uses irq_work.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">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)]
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Hmm, okay.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Also fwnmi_release_errinfo()
cannot be called when translation is not enabled.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Why not?
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">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.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">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...
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">So we are good to turn it on here.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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);</pre>
</blockquote>
<pre>Sure, Thanks
</pre>
<blockquote type="cite"
cite="mid:1584670525.n2ybablt2y.astroid@bobo.none">
<pre class="moz-quote-pre" wrap="">
Otherwise,
Reviewed-by: Nicholas Piggin <a class="moz-txt-link-rfc2396E" href="mailto:npiggin@gmail.com"><npiggin@gmail.com></a>
Thanks,
Nick
</pre>
</blockquote>
<br>
</body>
</html>