Re: [Qemu-ppc] [PATCH v8 4/6] target/ppc: Build rtas error log upon an MCE

[Aravinda Prasad]

On Friday 10 May 2019 03:22 PM, David Gibson wrote:
> On Fri, May 10, 2019 at 12:35:13PM +0530, Aravinda Prasad wrote:
>>
>>
>> On Friday 10 May 2019 12:12 PM, David Gibson wrote:
>>> On Mon, Apr 22, 2019 at 12:33:26PM +0530, Aravinda Prasad wrote:

[...]

>>>> +    /* Save gpr[3] in the guest endian mode */
>>>> +    if ((*pcc->interrupts_big_endian)(cpu)) {
>>>> +        env->gpr[3] = cpu_to_be64(rtas_addr + RTAS_ERRLOG_OFFSET);
>>>
>>> I don't think this is right.  AIUI env->gpr[] are all stored in *host*
>>> endianness (for ease of doing arithmetic).
>>
>> env-gpr[3] is later used by guest to fetch the RTAS log. My guess is
>> that we will not do an endianness change of all the gprs during a switch
>> from host to guest (that will be costly).
> 
> There's no need to "change endianness".  In TCG the host needs to do
> arithmetic on the values and so they are in host endian.  With KVM the
> env values are only synchronized when we enter/exit KVM and they're
> going to registers, not memory and so have no endianness.

Ah.. ok.

> 
>> But let me cross check.
>>
>>>
>>>> +    } else {
>>>> +        env->gpr[3] = cpu_to_le64(rtas_addr + RTAS_ERRLOG_OFFSET);
>>>> +    }
>>>> +
>>>> +    env->nip = spapr->guest_machine_check_addr;
>>>> +}
>>>> +
>>>>  void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered)
>>>>  {
>>>>      SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>>>> @@ -640,6 +881,10 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool 
>>>> recovered)
>>>>          }
>>>>      }
>>>>      spapr->mc_status = cpu->vcpu_id;
>>>> +
>>>> +    spapr_mce_dispatch_elog(cpu, recovered);
>>>> +
>>>> +    return;
>>>>  }
>>>>  
>>>>  static void check_exception(PowerPCCPU *cpu, SpaprMachineState *spapr,
>>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>>>> index f7204d0..03f34bf 100644
>>>> --- a/include/hw/ppc/spapr.h
>>>> +++ b/include/hw/ppc/spapr.h
>>>> @@ -661,6 +661,9 @@ target_ulong spapr_hypercall(PowerPCCPU *cpu, 
>>>> target_ulong opcode,
>>>>  #define DIAGNOSTICS_RUN_MODE_IMMEDIATE 2
>>>>  #define DIAGNOSTICS_RUN_MODE_PERIODIC  3
>>>>  
>>>> +/* Offset from rtas-base where error log is placed */
>>>> +#define RTAS_ERRLOG_OFFSET       0x25
>>>
>>> Is this offset PAPR defined, or chosen here?  Using an entirely
>>> unaliged (odd) address seems a very strange choice.
>>
>> This is not PAPR defined. I will make it 0x30. Or do you prefer any
>> other offset?
> 
> 0x30 should be fine.

ok..

> 

-- 
Regards,
Aravinda