> -----Original Message----- > From: Li, Jiongxi > Sent: Friday, September 14, 2012 10:16 PM > To: 'Avi Kivity' > Cc: kvm@xxxxxxxxxxxxxxx > Subject: RE: [PATCH 2/5]KVM:x86, apicv: adjust for virtual interrupt delivery > > Sorry for the late response > > > -----Original Message----- > > From: Avi Kivity [mailto:avi@xxxxxxxxxx] > > Sent: Friday, September 07, 2012 12:22 AM > > To: Li, Jiongxi > > Cc: kvm@xxxxxxxxxxxxxxx > > Subject: Re: [PATCH 2/5]KVM:x86, apicv: adjust for virtual interrupt > > delivery > > > > On 09/05/2012 08:41 AM, Li, Jiongxi wrote: > > > Virtual interrupt delivery avoids KVM to inject vAPIC interrupts > > > manually, which is fully taken care of by the hardware. This needs > > > some special awareness into existing interrupr injection path: > > > > > > - for pending interrupt, instead of direct injection, we may need > > > update architecture specific indicators before resuming to guest. > > > > > > - A pending interrupt, which is masked by ISR, should be also > > > considered in above update action, since hardware will decide > > > when to inject it at right time. Current has_interrupt and > > > get_interrupt only returns a valid vector from injection p.o.v. > > > > > > > > > --- a/arch/x86/kvm/x86.c > > > +++ b/arch/x86/kvm/x86.c > > > @@ -5194,6 +5194,13 @@ static void inject_pending_event(struct > > kvm_vcpu *vcpu) > > > vcpu->arch.nmi_injected = true; > > > kvm_x86_ops->set_nmi(vcpu); > > > } > > > + } else if (kvm_apic_vid_enabled(vcpu)) { > > > + if (kvm_cpu_has_interrupt_apic_vid(vcpu) && > > > + kvm_x86_ops->interrupt_allowed(vcpu)) { > > > + kvm_queue_interrupt(vcpu, > > > + kvm_cpu_get_interrupt_apic_vid(vcpu), false); > > > + kvm_x86_ops->set_irq(vcpu); > > > + } > > > > It may be simpler to change kvm_cpu_{has,get}_interrupt to ignore the > > apic if virtual interrupt delivery is enabled. > OKs Kvm_cpu_has_interrupt is also called in other place, no just used to judge whether to inject interrupt manually. For instance, it is called in kvm_arch_vcpu_runnable. In that case, apic can't be ingored. So for safety, I think it is better to use another function here other than change the original kvm_cpu_has_interrupt function. > > > > > @@ -5293,16 +5300,27 @@ static int vcpu_enter_guest(struct kvm_vcpu > > *vcpu) > > > } > > > > > > if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) { > > > + /* update archtecture specific hints for APIC virtual interrupt > > > +delivery > > */ > > > + if (kvm_apic_vid_enabled(vcpu)) > > > + kvm_x86_ops->update_irq(vcpu); > > > + > > > > Not defined. > This function is defined in patch 3/5. Because virtual interrupt delivery is not > enabled in this patch. So this function is not called. Since we will enable this > feature by default, so maybe we can merge PATCH 2,3,4 together into one > patch. > > > > > inject_pending_event(vcpu); > > > > > > /* enable NMI/IRQ window open exits if needed */ > > > if (vcpu->arch.nmi_pending) > > > kvm_x86_ops->enable_nmi_window(vcpu); > > > - else if (kvm_cpu_has_interrupt(vcpu) || req_int_win) > > > + else if (kvm_apic_vid_enabled(vcpu)) { > > > + if (kvm_cpu_has_interrupt_apic_vid(vcpu)) > > > + kvm_x86_ops->enable_irq_window(vcpu); > > > + } else if (kvm_cpu_has_interrupt(vcpu) || req_int_win) > > > kvm_x86_ops->enable_irq_window(vcpu); > > > > > > if (kvm_lapic_enabled(vcpu)) { > > > - update_cr8_intercept(vcpu); > > > + /* no need for tpr_threshold update if APIC virtual > > > + * interrupt delivery is enabled > > > + */ > > > + if (!kvm_apic_vid_enabled(vcpu)) > > > + update_cr8_intercept(vcpu); > > > > Perhaps the arch function should do the ignoring. > You means putting the 'vid_enabled' judgement in > 'kvm_x86_ops->update_cr8_intercept'? Is it just out of the reason that > reducing the code change in common code? > > > > > kvm_lapic_sync_to_vapic(vcpu); > > > } > > > } > > > > > > > > > -- > > error compiling committee.c: too many arguments to function -- 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
