On 22.01.2010, at 12:27, Liu Yu-B13201 wrote: > > >> -----Original Message----- >> From: kvm-ppc-owner@xxxxxxxxxxxxxxx >> [mailto:kvm-ppc-owner@xxxxxxxxxxxxxxx] On Behalf Of Alexander Graf >> Sent: Friday, January 22, 2010 7:13 PM >> To: Liu Yu-B13201 >> Cc: hollis@xxxxxxxxxxxxxx; kvm-ppc@xxxxxxxxxxxxxxx; >> kvm@xxxxxxxxxxxxxxx >> Subject: Re: [PATCH] kvmppc/booke: Set ESR and DEAR when >> inject interrupt to guest >> >> >> On 22.01.2010, at 11:54, Liu Yu wrote: >> >>> Old method prematurely sets ESR and DEAR. >>> Move this part after we decide to inject interrupt, >>> and make it more like hardware behave. >>> >>> Signed-off-by: Liu Yu <yu.liu@xxxxxxxxxxxxx> >>> --- >>> arch/powerpc/kvm/booke.c | 24 ++++++++++++++---------- >>> arch/powerpc/kvm/emulate.c | 2 -- >>> 2 files changed, 14 insertions(+), 12 deletions(-) >>> >>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c >>> index e283e44..a8ee148 100644 >>> --- a/arch/powerpc/kvm/booke.c >>> +++ b/arch/powerpc/kvm/booke.c >>> @@ -84,7 +84,8 @@ static void >> kvmppc_booke_queue_irqprio(struct kvm_vcpu *vcpu, >>> >>> void kvmppc_core_queue_program(struct kvm_vcpu *vcpu, ulong flags) >>> { >>> - /* BookE does flags in ESR, so ignore those we get here */ >>> + if (flags == SRR1_PROGTRAP) >>> + vcpu->arch.fault_esr = ESR_PTR; >> >> This looks odd. I was more thinking of "flags" being ESR in >> this context. So you'd save off flags to a field in the vcpu >> here and restore it later when the fault actually gets injected. > > See the end. > >> >>> kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_PROGRAM); >>> } >>> >>> @@ -115,14 +116,19 @@ static int >> kvmppc_booke_irqprio_deliver(struct kvm_vcpu *vcpu, >>> { >>> int allowed = 0; >>> ulong msr_mask; >>> + bool update_esr = false, update_dear = false; >>> >>> switch (priority) { >>> - case BOOKE_IRQPRIO_PROGRAM: >>> case BOOKE_IRQPRIO_DTLB_MISS: >>> - case BOOKE_IRQPRIO_ITLB_MISS: >>> - case BOOKE_IRQPRIO_SYSCALL: >> >> Is this correct? Was the previous order wrong according to the spec? > > what order? just make switch items share the code. Yikes. Yes. My fault. > >> >>> case BOOKE_IRQPRIO_DATA_STORAGE: >>> + update_dear = true; >>> + /* fall through */ >>> case BOOKE_IRQPRIO_INST_STORAGE: >>> + case BOOKE_IRQPRIO_PROGRAM: >>> + update_esr = true; >>> + /* fall through */ >>> + case BOOKE_IRQPRIO_ITLB_MISS: >>> + case BOOKE_IRQPRIO_SYSCALL: >>> case BOOKE_IRQPRIO_FP_UNAVAIL: >>> case BOOKE_IRQPRIO_SPE_UNAVAIL: >>> case BOOKE_IRQPRIO_SPE_FP_DATA: >>> @@ -157,6 +163,10 @@ static int >> kvmppc_booke_irqprio_deliver(struct kvm_vcpu *vcpu, >>> vcpu->arch.srr0 = vcpu->arch.pc; >>> vcpu->arch.srr1 = vcpu->arch.msr; >>> vcpu->arch.pc = vcpu->arch.ivpr | >> vcpu->arch.ivor[priority]; >>> + if (update_esr == true) >>> + vcpu->arch.esr = vcpu->arch.fault_esr; >> >> I'd rather like to see new fields used for that. >>> @@ -286,15 +295,12 @@ int kvmppc_handle_exit(struct kvm_run >> *run, struct kvm_vcpu *vcpu, >>> break; >>> >>> case BOOKE_INTERRUPT_DATA_STORAGE: >>> - vcpu->arch.dear = vcpu->arch.fault_dear; >>> - vcpu->arch.esr = vcpu->arch.fault_esr; >>> kvmppc_booke_queue_irqprio(vcpu, >> BOOKE_IRQPRIO_DATA_STORAGE); >> >> kvmppc_booke_queue_data_storage(vcpu, vcpu->arch.fault_esr, >> vcpu->arch.fault_dear); >> >>> kvmppc_account_exit(vcpu, DSI_EXITS); >>> r = RESUME_GUEST; >>> break; >>> >>> case BOOKE_INTERRUPT_INST_STORAGE: >>> - vcpu->arch.esr = vcpu->arch.fault_esr; >>> kvmppc_booke_queue_irqprio(vcpu, >> BOOKE_IRQPRIO_INST_STORAGE); >> >> kvmppc_booke_queue_inst_storage(vcpu, vcpu->arch.fault_esr); >> > > Not sure if this is redundant, as we already have fault_esr. > Or should we ignore what hareware is and create a new esr to guest? On Book3S I take the SRR1 we get from the host as "inspiration" of what to pass to the guest as SRR1. I think we should definitely be able to inject a fault that we didn't get in that exact form from the exit path. I'm also not sure if something could clobber fault_esr if another interrupt takes precedence. Say a #MC. > >> >>> - vcpu->arch.dear = vcpu->arch.fault_dear; >>> - vcpu->arch.esr = vcpu->arch.fault_esr; >>> kvmppc_mmu_dtlb_miss(vcpu); >>> kvmppc_account_exit(vcpu, DTLB_REAL_MISS_EXITS); >>> r = RESUME_GUEST; >>> diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c >>> index b905623..4f6b9df 100644 >>> --- a/arch/powerpc/kvm/emulate.c >>> +++ b/arch/powerpc/kvm/emulate.c >>> @@ -151,8 +151,6 @@ int kvmppc_emulate_instruction(struct >> kvm_run *run, struct kvm_vcpu *vcpu) >>> case OP_TRAP: >>> #ifdef CONFIG_PPC64 >>> case OP_TRAP_64: >>> -#else >>> - vcpu->arch.esr |= ESR_PTR; >>> #endif >>> kvmppc_core_queue_program(vcpu, SRR1_PROGTRAP); >> >> kvmppc_core_queue_program(vcpu, vcpu->arch.esr | ESR_PTR); >> > > This breaks book3s code. How so? For the handlers that don't take a "flags" parameter, just add one, stub it out for Book3S and I'll fix it. The queue_program function already does take one. And the semantics of "flags" is core specific. Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
