Re: [Qemu-devel] [PATCH 4/4] target-ppc: Handle NMI guest exit

[Aravinda Prasad <aravinda@linux.vnet.ibm.com>]

On Friday 13 November 2015 11:27 AM, David Gibson wrote:
> On Fri, Nov 13, 2015 at 10:23:20AM +0530, Aravinda Prasad wrote:
>>
>>
>> On Friday 13 November 2015 07:28 AM, David Gibson wrote:
>>> On Thu, Nov 12, 2015 at 11:53:45PM +0530, Aravinda Prasad wrote:
>>>>
>>>>
>>>> On Thursday 12 November 2015 01:39 PM, Thomas Huth wrote:
>>>>> On 11/11/15 18:16, Aravinda Prasad wrote:
>>>>>> Memory error such as bit flips that cannot be corrected
>>>>>> by hardware are passed on to the kernel for handling.
>>>>>> If the memory address in error belongs to guest then
>>>>>> guest kernel is responsible for taking suitable action.
>>>>>> Patch [1] enhances KVM to exit guest with exit reason
>>>>>> set to KVM_EXIT_NMI in such cases.
>>>>>>
>>>>>> This patch handles KVM_EXIT_NMI exit. If the guest OS
>>>>>> has registered the machine check handling routine by
>>>>>> calling "ibm,nmi-register", then the handler builds
>>>>>> the error log and invokes the registered handler else
>>>>>> invokes the handler at 0x200.
>>>>>>
>>>>>> [1] http://marc.info/?l=kvm-ppc&m=144726114408289
>>>>>>
>>>>>> Signed-off-by: Aravinda Prasad <aravinda@linux.vnet.ibm.com>
>>>>>> ---
>>>>>>  target-ppc/kvm.c     |   69 +++++++++++++++++++++++++++++++++++++++++++
>>>>>>  target-ppc/kvm_ppc.h |   81 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>  2 files changed, 150 insertions(+)
>>>>>>
>>>>>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>>>>>> index 110436d..e2e5170 100644
>>>>>> --- a/target-ppc/kvm.c
>>>>>> +++ b/target-ppc/kvm.c
>>>>>> @@ -1665,6 +1665,11 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
>>>>>>          ret = 0;
>>>>>>          break;
>>>>>>  
>>>>>> +    case KVM_EXIT_NMI:
>>>>>> +        DPRINTF("handle NMI exception\n");
>>>>>> +        ret = kvm_handle_nmi(cpu);
>>>>>> +        break;
>>>>>> +
>>>>>>      default:
>>>>>>          fprintf(stderr, "KVM: unknown exit reason %d\n", run->exit_reason);
>>>>>>          ret = -1;
>>>>>> @@ -2484,3 +2489,67 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
>>>>>>  {
>>>>>>      return data & 0xffff;
>>>>>>  }
>>>>>> +
>>>>>> +int kvm_handle_nmi(PowerPCCPU *cpu)
>>>>>> +{
>>>>>> +    struct rtas_mc_log mc_log;
>>>>>> +    CPUPPCState *env = &cpu->env;
>>>>>> +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>>>>>> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>>>>>> +
>>>>>> +    cpu_synchronize_state(CPU(ppc_env_get_cpu(env)));
>>>>>> +
>>>>>> +    /* Properly set bits in MSR before we invoke the handler */
>>>>>> +    env->msr = 0;
>>>>>> +
>>>>>> +    if (!(*pcc->interrupts_big_endian)(cpu)) {
>>>>>> +        env->msr |= (1ULL << MSR_LE);
>>>>>> +    }
>>>>>> +
>>>>>> +#ifdef TARGET_PPC64
>>>>>> +    env->msr |= (1ULL << MSR_SF);
>>>>>> +#endif
>>>>>> +
>>>>>> +    if (!spapr->guest_machine_check_addr) {
>>>>>> +        /*
>>>>>> +         * If OS has not registered with "ibm,nmi-register"
>>>>>> +         * jump to 0x200
>>>>>> +         */
>>>>>
>>>>> Shouldn't you also check MSR_ME here first and enter checkstop when
>>>>> machine checks are disabled?
>>>>
>>>> Yes, MSR_ME should be checked first.
>>>>
>>>>> Also I think you have to set up some more registers for machine check
>>>>> interrupts, like SRR0 and SRR1?
>>>>
>>>> SRRO and SRR1 of vcpu are properly set in KVM in kvmppc_interrupt_hv. I
>>>> am not sure if any other registers need to be set.
>>>
>>> DAR and DSISR are the obvious ones you need to consider, although I
>>> suspect they're already set up correctly by the kernel, too.
>>
>> Yes, they are also setup properly by KVM.
> 
> Ok, good.  Might be worth throwing a comment in the qemu code noting
> that the kernel has already set up that state properly.

sure.

> 

-- 
Regards,
Aravinda