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

[David Gibson]

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 <address@hidden>
> >>>> ---
> >>>>  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.

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

signature.asc
Description: PGP signature