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

[Aravinda Prasad]

On Thursday 13 November 2014 09:22 AM, David Gibson wrote:
> 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.

h_report_mc_err returns 0 if another VCPU is processing machine check
and in that case we retry. h_report_mc_err returns error log address if
no other VCPU is processing machine check.

> 
>> 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.

SRR0/1 is clobbered while returning from HCALL and hence cannot be
restored in HCALL. For CR, we need to do the restoration here as we
clobber CR after returning from HCALL (the instruction checking the
return value of hcall clobbers CR).

> 
>>>
>>>> +  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.

Hmm.. yes.. I will fix this.

Regards,
Aravinda

> 
>>
>>>
>>>> +  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.
> 

-- 
Regards,
Aravinda