On Tue, Nov 11, 2014 at 12:14:31PM +0530, Aravinda Prasad wrote: > On Tuesday 11 November 2014 08:46 AM, David Gibson wrote: > > On Wed, Nov 05, 2014 at 12:43:15PM +0530, Aravinda Prasad wrote: [snip] > >> + . = 0x200 > >> + /* > >> + * Trampoline saves r3 in sprg2 and issues private hcall > >> + * to request qemu to build error log. QEMU builds the > >> + * error log, copies to rtas-blob and returns the address. > >> + * The initial 16 bytes in return adress consist of saved > >> + * srr0 and srr1 which we restore and pass on the actual error > >> + * log address to OS handled mcachine check notification > >> + * routine > >> + * > >> + * All the below instructions are copied to interrupt vector > >> + * 0x200 at the time of handling ibm,nmi-register rtas call. > >> + */ > >> + mtsprg 2,3 > >> + li 3,0 > >> + /* > >> + * ori r3,r3,KVMPPC_H_REPORT_MC_ERR. The KVMPPC_H_REPORT_MC_ERR > >> + * value is patched below > >> + */ > >> +1: ori 3,3,0 > >> + sc 1 /* Issue H_CALL */ > >> + cmpdi cr0,3,0 > >> + beq cr0,1b /* retry KVMPPC_H_REPORT_MC_ERR */ > > > > Having to retry the hcall from here seems very awkward. This is a > > private hcall, so you can define it to do whatever retries are > > necessary internally (and I don't think your current implementation > > can fail anyway). > > Retrying is required in the cases when multi-processors experience > machine check at or about the same time. As per PAPR, subsequent > processors should serialize and wait for the first processor to issue > the ibm,nmi-interlock call. The second processor retries if the first > processor which received a machine check is still reading the error log > and is yet to issue ibm,nmi-interlock call.
Hmm.. ok. But I don't see any mechanism in the patches by which
H_REPORT_MC_ERR will report failure if another CPU has an MC in
progress.
> Retrying cannot be done internally in h_report_mc_err hcall: only one
> thread can succeed entering qemu upon parallel hcall and hence retrying
> inside the hcall will not allow the ibm,nmi-interlock from first CPU to
> succeed.
It's possible, but would require some fiddling inside the h_call to
unlock and wait for the other CPUs to finish, so yes, it might be more
trouble than it's worth.
>
> >
> >> + mtsprg 2,4
> >
> > Um.. doesn't this clobber the value of r3 you saved in SPRG2 just above.
>
> The r3 saved in SPRG2 is moved to rtas area in the private hcall and
> hence it is fine to clobber r3 here
Ok, if you're going to do some magic register saving inside the HCALL,
why not do the SRR[01] and CR restoration inside there as well.
> >
> >> + ld 4,0(3)
> >> + mtsrr0 4 /* Restore srr0 */
> >> + ld 4,8(3)
> >> + mtsrr1 4 /* Restore srr1 */
> >> + ld 4,16(3)
> >> + mtcrf 0,4 /* Restore cr */
> >
> > mtcrf? aren't you restoring the whole CR?
>
> No. I am moving only cr0. The 0 in mtcrf 0,4 represents the cr field
> mask that is replaced.
Uh, yes it is. In which case a value of 0 means *no* condition
register fields are transferred.
>
> >
> >> + addi 3,3,24
> >> + mfsprg 4,2
> >> + /*
> >> + * Branch to address registered by OS. The branch address is
> >> + * patched in the ibm,nmi-register rtas call.
> >> + */
> >> + ba 0x0
> >> + b .
> >
> > The branch to self is pointless. Even if the instruction above is
> > not patched, or patched incorrectly, it's a ba, so you're not likely
> > to end up at the instruction underneath.
>
> I added it to avoid speculative execution. Based on how it is used in
> arch/powerpc/kernel/exceptions-64s.S
Ah, I guess that makes sense, although surely any ba instruction would
also have to inhibit speculative execution.
> > Actually, what would probably make more sense would be to just have a
> > "b ." *instead* of the ba, and have the qemu patching replace it with
> > the correct ba instruction. That will limit the damage if it somehow
> > gets executed without being patched.
>
> good idea. Will do that.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
pgpZH5cqDwS7c.pgp
Description: PGP signature
