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