Re: [Qemu-ppc] [Qemu-devel] [PATCH v3 4/4] target-ppc: Handle ibm, nmi-register RTAS call

[David Gibson]

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

pgpE4kHPZVjoW.pgp
Description: PGP signature