Re: [PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted
On 03/09/2015 07:18, Nakajima, Jun wrote: > On Wed, Sep 2, 2015 at 3:38 PM, Steve Rutherford > wrote: >> On Thu, Aug 13, 2015 at 09:31:48AM +0200, Paolo Bonzini wrote: >> Pinging this thread. >> >> Should I put together a patch to make split irqchip work properly with the >> old TMR behavior? > > Yes, please. > > Intel® 64 and IA-32 Architectures Software Developer’s Manual: > > 24.11.4 Software Access to Related Structures > > In addition to data in the VMCS region itself, VMX non-root operation > can be controlled by data structures that are > referenced by pointers in a VMCS (for example, the I/O bitmaps). The SDM does not list these data structures however. It also does not say that, whenever a page is pointed to by the VMCS, *the whole page* counts as a control data structure. In http://article.gmane.org/gmane.linux.kernel/2011131 I explained my reading of the manual and why the vTMR is IMO not part of the control data structures. In a nutshell, the vISR, vIRR, vTPR, vPPR etc. are control data structures, but the other fields look to me like they are just data. Jun, can you find anything wrong in the reasoning? Next week I'll write test cases for it, which are worthwhile anyway. In the meanwhile, if Steve wants to prepare a patch that injects level interrupts (those that have to set the vTMR to 1) while the VCPU is not running, that would also work great for me as I was going to look into that anyway. Paolo While > the pointers to these data structures are > parts of the VMCS, the data structures themselves are not. They are > not accessible using VMREAD and VMWRITE > but by ordinary memory writes. > Software should ensure that each such data structure is modified only > when no logical processor with a current > VMCS that references it is in VMX non-root operation. Doing otherwise > may lead to unpredictable behavior > (including behaviors identified in Section 24.11.1) > > > 29.6 POSTED-INTERRUPT PROCESSING > ... > Use of the posted-interrupt descriptor differs from that of other data > structures that are referenced by pointers in > a VMCS. There is a general requirement that software ensure that each > such data structure is modified only when > no logical processor with a current VMCS that references it is in VMX > non-root operation. That requirement does > not apply to the posted-interrupt descriptor. There is a requirement, > however, that such modifications be done > using locked read-modify-write instructions. > > >> >>> >>> >>> On 13/08/2015 08:35, Zhang, Yang Z wrote: > You may be right. It is safe if no future hardware plans to use > it. Let me check with our hardware team to see whether it will be > used or not in future. After checking with Jun, there is no guarantee that the guest running on another CPU will operate properly if hypervisor modify the vTMR from another CPU. So the hypervisor should not to do it. >>> >>> I guess I can cause a vmexit on level-triggered interrupts, it's not a >>> big deal, but no weasel words, please. >>> >>> What's going to break, and where is it documented? >>> >>> Paolo >>> -- >>> To unsubscribe from this list: send the line "unsubscribe kvm" in >>> the body of a message to majord...@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted
On Wed, Sep 2, 2015 at 3:38 PM, Steve Rutherford wrote: > On Thu, Aug 13, 2015 at 09:31:48AM +0200, Paolo Bonzini wrote: > Pinging this thread. > > Should I put together a patch to make split irqchip work properly with the > old TMR behavior? Yes, please. Intel® 64 and IA-32 Architectures Software Developer’s Manual: 24.11.4 Software Access to Related Structures In addition to data in the VMCS region itself, VMX non-root operation can be controlled by data structures that are referenced by pointers in a VMCS (for example, the I/O bitmaps). While the pointers to these data structures are parts of the VMCS, the data structures themselves are not. They are not accessible using VMREAD and VMWRITE but by ordinary memory writes. Software should ensure that each such data structure is modified only when no logical processor with a current VMCS that references it is in VMX non-root operation. Doing otherwise may lead to unpredictable behavior (including behaviors identified in Section 24.11.1) 29.6 POSTED-INTERRUPT PROCESSING ... Use of the posted-interrupt descriptor differs from that of other data structures that are referenced by pointers in a VMCS. There is a general requirement that software ensure that each such data structure is modified only when no logical processor with a current VMCS that references it is in VMX non-root operation. That requirement does not apply to the posted-interrupt descriptor. There is a requirement, however, that such modifications be done using locked read-modify-write instructions. > >> >> >> On 13/08/2015 08:35, Zhang, Yang Z wrote: >> >> You may be right. It is safe if no future hardware plans to use >> >> it. Let me check with our hardware team to see whether it will be >> >> used or not in future. >> > >> > After checking with Jun, there is no guarantee that the guest running >> > on another CPU will operate properly if hypervisor modify the vTMR >> > from another CPU. So the hypervisor should not to do it. >> >> I guess I can cause a vmexit on level-triggered interrupts, it's not a >> big deal, but no weasel words, please. >> >> What's going to break, and where is it documented? >> >> Paolo >> -- >> To unsubscribe from this list: send the line "unsubscribe kvm" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- Jun Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted
On Thu, Aug 13, 2015 at 09:31:48AM +0200, Paolo Bonzini wrote: Pinging this thread. Should I put together a patch to make split irqchip work properly with the old TMR behavior? > > > On 13/08/2015 08:35, Zhang, Yang Z wrote: > >> You may be right. It is safe if no future hardware plans to use > >> it. Let me check with our hardware team to see whether it will be > >> used or not in future. > > > > After checking with Jun, there is no guarantee that the guest running > > on another CPU will operate properly if hypervisor modify the vTMR > > from another CPU. So the hypervisor should not to do it. > > I guess I can cause a vmexit on level-triggered interrupts, it's not a > big deal, but no weasel words, please. > > What's going to break, and where is it documented? > > Paolo > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted
On 13/08/2015 08:35, Zhang, Yang Z wrote: >> You may be right. It is safe if no future hardware plans to use >> it. Let me check with our hardware team to see whether it will be >> used or not in future. > > After checking with Jun, there is no guarantee that the guest running > on another CPU will operate properly if hypervisor modify the vTMR > from another CPU. So the hypervisor should not to do it. I guess I can cause a vmexit on level-triggered interrupts, it's not a big deal, but no weasel words, please. What's going to break, and where is it documented? Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted
Zhang, Yang Z wrote on 2015-08-04: > Paolo Bonzini wrote on 2015-08-04: >> >> >> On 04/08/2015 02:46, Zhang, Yang Z wrote: It is a problem for split irqchip, where the EOI exit bitmap can be inferred from the IOAPIC routes but the TMR cannot. The hardware behavior on the other hand can be implemented purely within the LAPIC. >>> >>> So updating the TMR within LAPIC is the only solution to handle it? >> >> It's the simplest and the one that makes most sense. Considering >> that TMR is a pretty obscure feature, it's unlikely that it will be >> accelerated in the future. > > You may be right. It is safe if no future hardware plans to use it. > Let me check with our hardware team to see whether it will be used or not in > future. After checking with Jun, there is no guarantee that the guest running on another CPU will operate properly if hypervisor modify the vTMR from another CPU. So the hypervisor should not to do it. > >> >> Paolo > > > Best regards, > Yang > Best regards, Yang -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted
Paolo Bonzini wrote on 2015-08-04: > > > On 04/08/2015 02:46, Zhang, Yang Z wrote: >>> It is a problem for split irqchip, where the EOI exit bitmap can be >>> inferred from the IOAPIC routes but the TMR cannot. The hardware >>> behavior on the other hand can be implemented purely within the LAPIC. >> >> So updating the TMR within LAPIC is the only solution to handle it? > > It's the simplest and the one that makes most sense. Considering that > TMR is a pretty obscure feature, it's unlikely that it will be > accelerated in the future. You may be right. It is safe if no future hardware plans to use it. Let me check with our hardware team to see whether it will be used or not in future. > > Paolo Best regards, Yang -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted
On 04/08/2015 02:46, Zhang, Yang Z wrote: > > It is a problem for split irqchip, where the EOI exit bitmap can be > > inferred from the IOAPIC routes but the TMR cannot. The hardware > > behavior on the other hand can be implemented purely within the LAPIC. > > So updating the TMR within LAPIC is the only solution to handle it? It's the simplest and the one that makes most sense. Considering that TMR is a pretty obscure feature, it's unlikely that it will be accelerated in the future. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted
Paolo Bonzini wrote on 2015-08-03: > > > On 03/08/2015 12:23, Zhang, Yang Z wrote: >>> In any case, the TMR behavior introduced by the APICv patches is >>> completely different from the hardware behavior, so it has to be fixed. >> >> But any real problem with it? > > It is a problem for split irqchip, where the EOI exit bitmap can be > inferred from the IOAPIC routes but the TMR cannot. The hardware > behavior on the other hand can be implemented purely within the LAPIC. So updating the TMR within LAPIC is the only solution to handle it? > >>> The alternative is to inject level-triggered interrupts >>> synchronously, without using posted interrupts. >>> >>> I'll write some testcases to understand the functioning of TMR in >>> the virtual-APIC page, but the manual seems clear to me. >> >> Currently, no existing hardware will use TMR and will not cause any >> problem.(That's the reason why we leave it in Xen).But we don't know >> whether future hardware will use it or not(SDM always keeps changing >> :)). > > But that would be covered by a different execution control (for > backwards compatibility). We'll get there when such a feature is introduced. Yes, we can leave it in future. But one concern is that it may hard to handle it at that time if someone also develops feature which rely on it (like current patch to split irqchip). > >> And per 24.11.4's description, the perfect solution is don't modify >> it. btw, IIRC, only TMR doesn't follow the rule. All other VMCS >> accesses are issued in right VMCS context. > > Yes, that's correct. It's just the TMR. > > Paolo Best regards, Yang -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted
On 03/08/2015 12:23, Zhang, Yang Z wrote: > > In any case, the TMR behavior introduced by the APICv patches is > > completely different from the hardware behavior, so it has to be fixed. > > But any real problem with it? It is a problem for split irqchip, where the EOI exit bitmap can be inferred from the IOAPIC routes but the TMR cannot. The hardware behavior on the other hand can be implemented purely within the LAPIC. > > The alternative is to inject level-triggered interrupts > > synchronously, without using posted interrupts. > > > > I'll write some testcases to understand the functioning of TMR in the > > virtual-APIC page, but the manual seems clear to me. > > Currently, no existing hardware will use TMR and will not cause any > problem.(That's the reason why we leave it in Xen).But we don't know > whether future hardware will use it or not(SDM always keeps changing > :)). But that would be covered by a different execution control (for backwards compatibility). We'll get there when such a feature is introduced. > And per 24.11.4's description, the perfect solution is don't > modify it. btw, IIRC, only TMR doesn't follow the rule. All other > VMCS accesses are issued in right VMCS context. Yes, that's correct. It's just the TMR. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted
Paolo Bonzini wrote on 2015-08-03: > > > On 03/08/2015 04:37, Zhang, Yang Z wrote: Only virtualized APIC register reads use the virtual TMR registers (SDM 29.4.2 or 29.5), but these just read data from the corresponding field in the virtual APIC page. >> >> 24.11.4 Software Access to Related Structures In addition to data in >> the VMCS region itself, VMX non-root operation can be controlled by >> data structures that are referenced by pointers in a VMCS (for >> example, the I/O bitmaps). While the pointers to these data >> structures are parts of the VMCS, the data structures themselves are >> not. They are not accessible using VMREAD and VMWRITE but by >> ordinary memory writes. > > I don't think the TMR fields of the virtual-APIC page are among the > data structures that _controls_ VMX non-root operations, because: > > * it is not part of the virtualized APIC state is listed in 29.1.1 > > * read accesses from the APIC-access page simply return data from the > corresponding page offset on the virtual-APIC page using the memory > access type stored in IA32_VMX_BASIC_MSR. I think this explicitly > says that the effects of 24.11.1 (especially non-deterministic > behavior after a write) do not apply here. > > In any case, the TMR behavior introduced by the APICv patches is > completely different from the hardware behavior, so it has to be fixed. But any real problem with it? > The alternative is to inject level-triggered interrupts > synchronously, without using posted interrupts. > > I'll write some testcases to understand the functioning of TMR in the > virtual-APIC page, but the manual seems clear to me. Currently, no existing hardware will use TMR and will not cause any problem.(That's the reason why we leave it in Xen).But we don't know whether future hardware will use it or not(SDM always keeps changing :)).And per 24.11.4's description, the perfect solution is don't modify it. btw, IIRC, only TMR doesn't follow the rule. All other VMCS accesses are issued in right VMCS context. > > Paolo Best regards, Yang -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted
On 03/08/2015 04:37, Zhang, Yang Z wrote: >> > Only virtualized APIC register reads use the virtual TMR registers (SDM >> > 29.4.2 or 29.5), but these just read data from the corresponding field >> > in the virtual APIC page. > > 24.11.4 Software Access to Related Structures > In addition to data in the VMCS region itself, VMX non-root operation > can be controlled by data structures that are referenced by pointers > in a VMCS (for example, the I/O bitmaps). While the pointers to these > data structures are parts of the VMCS, the data structures themselves > are not. They are not accessible using VMREAD and VMWRITE but by > ordinary memory writes. I don't think the TMR fields of the virtual-APIC page are among the data structures that _controls_ VMX non-root operations, because: * it is not part of the virtualized APIC state is listed in 29.1.1 * read accesses from the APIC-access page simply return data from the corresponding page offset on the virtual-APIC page using the memory access type stored in IA32_VMX_BASIC_MSR. I think this explicitly says that the effects of 24.11.1 (especially non-deterministic behavior after a write) do not apply here. In any case, the TMR behavior introduced by the APICv patches is completely different from the hardware behavior, so it has to be fixed. The alternative is to inject level-triggered interrupts synchronously, without using posted interrupts. I'll write some testcases to understand the functioning of TMR in the virtual-APIC page, but the manual seems clear to me. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted
Paolo Bonzini wrote on 2015-07-31: > > > On 31/07/2015 04:49, Steve Rutherford wrote: >> Oh... Yeah. That's a damn good point, given that the interrupt can be >> injected from another thread while one is in that guest vcpu. >> >> Easiest time to update the TMR should be on guest entry through >> vcpu_scan_ioapic, as before. >> >> The best way to go is probably to ditch the new per vcpu EOI exit >> bitmap, and just update/use the TMR. There's no reason to duplicate >> that data in the representation of the apic (I suspect that the >> duplication was inspired by my mistaken notion of the TMR). The >> IOAPIC exit check can use the TMR instead. >> >> Based upon my reading of the SDM, the only reason that the eoi exit >> bitmaps are not the exact same as the TMR is that it is possible to >> have virtual-interrupt delivery enabled /without/ an apic access page >> (Note: V-ID => EOI exit bitmap enabled). >> >> Yang, do you happen to know if that is the case? >> >> [Note: Just looked back at the old code for updating the EOI exit >> bitmaps, which for some reason was configured to trigger EOI exits >> for all IOAPIC interrupts, not just level-triggered IOAPIC >> interrupts. Which is weird, and I believe totally unecessary.] > > The RTC interrupt needs to trigger an EOI exit with the in-kernel > IOAPIC, in order to detect coalesced interrupts. This is necessary to > avoid severe time drift in Windows guest. Correct! EOI exit bitmap is not absolutely same with TMR. Some interrupts (especially timer interrupt) which is edge trigger but need a notification when it got injected into guest. > > Paolo Best regards, Yang -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted
Paolo Bonzini wrote on 2015-07-31: > > > On 31/07/2015 01:26, Zhang, Yang Z wrote: Do not compute TMR in advance. Instead, set the TMR just before the interrupt is accepted into the IRR. This limits the coupling between IOAPIC and LAPIC. >> >> Uh.., it back to original way which is wrong. You cannot modify the >> apic page(here is the TMR reg) directly when the corresponding VMCS >> may be used at same time. > > Where is this documented? The TMR is not part of the set of virtualized > APIC registers (TPR, PPR, EOI, ISR, IRR, ICR+ICR2; SDM 29.1.1). > > Only virtualized APIC register reads use the virtual TMR registers (SDM > 29.4.2 or 29.5), but these just read data from the corresponding field > in the virtual APIC page. It's not only for virtual apic page. All data structures that is used by VMCS(either directly inside in VMCS or referenced by pointer in a VMCS) need follow this rule: 24.11.4 Software Access to Related Structures In addition to data in the VMCS region itself, VMX non-root operation can be controlled by data structures that are referenced by pointers in a VMCS (for example, the I/O bitmaps). While the pointers to these data structures are parts of the VMCS, the data structures themselves are not. They are not accessible using VMREAD and VMWRITE but by ordinary memory writes. Software should ensure that each such data structure is modified only when no logical processor with a current VMCS that references it is in VMX non-root operation. Doing otherwise may lead to unpredictable behavior (including behaviors identified in Section 24.11.1). > > Paolo Best regards, Yang -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted
On 31/07/2015 01:26, Zhang, Yang Z wrote: >>> Do not compute TMR in advance. Instead, set the TMR just before >>> the interrupt is accepted into the IRR. This limits the coupling >>> between IOAPIC and LAPIC. > > Uh.., it back to original way which is wrong. You cannot modify the > apic page(here is the TMR reg) directly when the corresponding VMCS > may be used at same time. Where is this documented? The TMR is not part of the set of virtualized APIC registers (TPR, PPR, EOI, ISR, IRR, ICR+ICR2; SDM 29.1.1). Only virtualized APIC register reads use the virtual TMR registers (SDM 29.4.2 or 29.5), but these just read data from the corresponding field in the virtual APIC page. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted
On 31/07/2015 04:49, Steve Rutherford wrote: > Oh... Yeah. That's a damn good point, given that the interrupt can be > injected from another thread while one is in that guest vcpu. > > Easiest time to update the TMR should be on guest entry through > vcpu_scan_ioapic, as before. > > The best way to go is probably to ditch the new per vcpu EOI exit > bitmap, and just update/use the TMR. There's no reason to duplicate > that data in the representation of the apic (I suspect that the > duplication was inspired by my mistaken notion of the TMR). The > IOAPIC exit check can use the TMR instead. > > Based upon my reading of the SDM, the only reason that the eoi exit > bitmaps are not the exact same as the TMR is that it is possible to > have virtual-interrupt delivery enabled /without/ an apic access page > (Note: V-ID => EOI exit bitmap enabled). > > Yang, do you happen to know if that is the case? > > [Note: Just looked back at the old code for updating the EOI exit > bitmaps, which for some reason was configured to trigger EOI exits > for all IOAPIC interrupts, not just level-triggered IOAPIC > interrupts. Which is weird, and I believe totally unecessary.] The RTC interrupt needs to trigger an EOI exit with the in-kernel IOAPIC, in order to detect coalesced interrupts. This is necessary to avoid severe time drift in Windows guest. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted
On Thu, Jul 30, 2015 at 11:26:28PM +, Zhang, Yang Z wrote: > Paolo Bonzini wrote on 2015-07-29: > > Do not compute TMR in advance. Instead, set the TMR just before the > > interrupt is accepted into the IRR. This limits the coupling between > > IOAPIC and LAPIC. > > > > Uh.., it back to original way which is wrong. You cannot modify the apic > page(here is the TMR reg) directly when the corresponding VMCS may be used at > same time. Oh... Yeah. That's a damn good point, given that the interrupt can be injected from another thread while one is in that guest vcpu. Easiest time to update the TMR should be on guest entry through vcpu_scan_ioapic, as before. The best way to go is probably to ditch the new per vcpu EOI exit bitmap, and just update/use the TMR. There's no reason to duplicate that data in the representation of the apic (I suspect that the duplication was inspired by my mistaken notion of the TMR). The IOAPIC exit check can use the TMR instead. Based upon my reading of the SDM, the only reason that the eoi exit bitmaps are not the exact same as the TMR is that it is possible to have virtual-interrupt delivery enabled /without/ an apic access page (Note: V-ID => EOI exit bitmap enabled). Yang, do you happen to know if that is the case? [Note: Just looked back at the old code for updating the EOI exit bitmaps, which for some reason was configured to trigger EOI exits for all IOAPIC interrupts, not just level-triggered IOAPIC interrupts. Which is weird, and I believe totally unecessary.] > > > > Signed-off-by: Paolo Bonzini > > --- > > arch/x86/kvm/ioapic.c | 9 ++--- > > arch/x86/kvm/ioapic.h | 3 +-- > > arch/x86/kvm/lapic.c | 19 ++- > > arch/x86/kvm/lapic.h | 1 - > > arch/x86/kvm/x86.c| 5 + > > 5 files changed, 14 insertions(+), 23 deletions(-) > > diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c > > index 856f79105bb5..eaf4ec38d980 100644 > > --- a/arch/x86/kvm/ioapic.c > > +++ b/arch/x86/kvm/ioapic.c > > @@ -246,8 +246,7 @@ static void update_handled_vectors(struct kvm_ioapic > > *ioapic) > > smp_wmb(); > > } > > -void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap, > > - u32 *tmr) > > +void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap) > > { > > struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic; > > union kvm_ioapic_redirect_entry *e; > > @@ -260,13 +259,9 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, > > u64 *eoi_exit_bitmap, > > kvm_irq_has_notifier(ioapic->kvm, KVM_IRQCHIP_IOAPIC, > > index) || > > index == RTC_GSI) { if > > (kvm_apic_match_dest(vcpu, NULL, 0, > > - e->fields.dest_id, e->fields.dest_mode)) { > > + e->fields.dest_id, e->fields.dest_mode)) > > __set_bit(e->fields.vector, > > (unsigned long *)eoi_exit_bitmap); > > - if (e->fields.trig_mode == IOAPIC_LEVEL_TRIG) > > - __set_bit(e->fields.vector, - > > (unsigned long *)tmr); - > > } > > } > > } > > spin_unlock(&ioapic->lock); > > diff --git a/arch/x86/kvm/ioapic.h b/arch/x86/kvm/ioapic.h > > index ca0b0b4e6256..3dbd0e2aac4e 100644 > > --- a/arch/x86/kvm/ioapic.h > > +++ b/arch/x86/kvm/ioapic.h > > @@ -120,7 +120,6 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct > > kvm_lapic *src, > > struct kvm_lapic_irq *irq, unsigned long *dest_map); > > int kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state); > > int kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state); > > -void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap, > > - u32 *tmr); > > +void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap); > > > > #endif > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > > index 2a5ca97c263b..9be64c77d6db 100644 > > --- a/arch/x86/kvm/lapic.c > > +++ b/arch/x86/kvm/lapic.c > > @@ -551,15 +551,6 @@ static void pv_eoi_clr_pending(struct kvm_vcpu > > *vcpu) > > __clear_bit(KVM_APIC_PV_EOI_PENDING, &vcpu->arch.apic_attention); > > } > > -void kvm_apic_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr) > > -{ > > - struct kvm_lapic *apic = vcpu->arch.apic; > > - int i; > > - > > - for (i = 0; i < 8; i++) > > - apic_set_reg(apic, APIC_TMR + 0x10 * i, tmr[i]); > > -} > > - > > static void apic_update_ppr(struct kvm_lapic *apic) > > { > > u32 tpr, isrv, ppr, old_ppr; > > @@ -781,6 +772,9 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int > > delivery_mode, > > case APIC_DM_LOWEST: > > vcpu->arch.apic_arb_prio++; > > case APIC_DM_FIXED: > > + if (unlikely(trig_mode && !level)) > > + break; > >
RE: [PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted
Paolo Bonzini wrote on 2015-07-29: > Do not compute TMR in advance. Instead, set the TMR just before the > interrupt is accepted into the IRR. This limits the coupling between > IOAPIC and LAPIC. > Uh.., it back to original way which is wrong. You cannot modify the apic page(here is the TMR reg) directly when the corresponding VMCS may be used at same time. > Signed-off-by: Paolo Bonzini > --- > arch/x86/kvm/ioapic.c | 9 ++--- > arch/x86/kvm/ioapic.h | 3 +-- > arch/x86/kvm/lapic.c | 19 ++- > arch/x86/kvm/lapic.h | 1 - > arch/x86/kvm/x86.c| 5 + > 5 files changed, 14 insertions(+), 23 deletions(-) > diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c > index 856f79105bb5..eaf4ec38d980 100644 > --- a/arch/x86/kvm/ioapic.c > +++ b/arch/x86/kvm/ioapic.c > @@ -246,8 +246,7 @@ static void update_handled_vectors(struct kvm_ioapic > *ioapic) > smp_wmb(); > } > -void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap, > - u32 *tmr) > +void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap) > { > struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic; > union kvm_ioapic_redirect_entry *e; > @@ -260,13 +259,9 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, > u64 *eoi_exit_bitmap, > kvm_irq_has_notifier(ioapic->kvm, KVM_IRQCHIP_IOAPIC, > index) || > index == RTC_GSI) { if > (kvm_apic_match_dest(vcpu, NULL, 0, > - e->fields.dest_id, e->fields.dest_mode)) { > + e->fields.dest_id, e->fields.dest_mode)) > __set_bit(e->fields.vector, > (unsigned long *)eoi_exit_bitmap); > - if (e->fields.trig_mode == IOAPIC_LEVEL_TRIG) > - __set_bit(e->fields.vector, - > (unsigned long *)tmr); - > } > } > } > spin_unlock(&ioapic->lock); > diff --git a/arch/x86/kvm/ioapic.h b/arch/x86/kvm/ioapic.h > index ca0b0b4e6256..3dbd0e2aac4e 100644 > --- a/arch/x86/kvm/ioapic.h > +++ b/arch/x86/kvm/ioapic.h > @@ -120,7 +120,6 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct > kvm_lapic *src, > struct kvm_lapic_irq *irq, unsigned long *dest_map); > int kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state); > int kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state); > -void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap, > - u32 *tmr); > +void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap); > > #endif > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index 2a5ca97c263b..9be64c77d6db 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -551,15 +551,6 @@ static void pv_eoi_clr_pending(struct kvm_vcpu > *vcpu) > __clear_bit(KVM_APIC_PV_EOI_PENDING, &vcpu->arch.apic_attention); > } > -void kvm_apic_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr) > -{ > - struct kvm_lapic *apic = vcpu->arch.apic; > - int i; > - > - for (i = 0; i < 8; i++) > - apic_set_reg(apic, APIC_TMR + 0x10 * i, tmr[i]); > -} > - > static void apic_update_ppr(struct kvm_lapic *apic) > { > u32 tpr, isrv, ppr, old_ppr; > @@ -781,6 +772,9 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int > delivery_mode, > case APIC_DM_LOWEST: > vcpu->arch.apic_arb_prio++; > case APIC_DM_FIXED: > + if (unlikely(trig_mode && !level)) > + break; > + > /* FIXME add logic for vcpu on reset */ > if (unlikely(!apic_enabled(apic))) > break; > @@ -790,6 +784,13 @@ static int __apic_accept_irq(struct kvm_lapic *apic, > int delivery_mode, > if (dest_map) > __set_bit(vcpu->vcpu_id, dest_map); > + if (apic_test_vector(vector, apic->regs + APIC_TMR) != > !!trig_mode) { > + if (trig_mode) > + apic_set_vector(vector, apic->regs + APIC_TMR); > + else > + apic_clear_vector(vector, apic->regs + > APIC_TMR); > + } > + > if (kvm_x86_ops->deliver_posted_interrupt) > kvm_x86_ops->deliver_posted_interrupt(vcpu, vector); > else { > diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h > index 764037991d26..eb46d6bcaa75 100644 > --- a/arch/x86/kvm/lapic.h > +++ b/arch/x86/kvm/lapic.h > @@ -57,7 +57,6 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 > value); > u64 kvm_lapic_get_base(struct kvm_vcpu *vcpu); > void kvm_apic_set_version(struct kvm_vcpu *vcpu); > -void kvm_apic_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr); > void __kvm_apic_update_irr(u32 *pir, void *regs); > vo
Re: [PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted
On Wed, Jul 29, 2015 at 03:37:34PM +0200, Paolo Bonzini wrote: > Do not compute TMR in advance. Instead, set the TMR just before the interrupt > is accepted into the IRR. This limits the coupling between IOAPIC and LAPIC. > > Signed-off-by: Paolo Bonzini > --- > arch/x86/kvm/ioapic.c | 9 ++--- > arch/x86/kvm/ioapic.h | 3 +-- > arch/x86/kvm/lapic.c | 19 ++- > arch/x86/kvm/lapic.h | 1 - > arch/x86/kvm/x86.c| 5 + > 5 files changed, 14 insertions(+), 23 deletions(-) > > diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c > index 856f79105bb5..eaf4ec38d980 100644 > --- a/arch/x86/kvm/ioapic.c > +++ b/arch/x86/kvm/ioapic.c > @@ -246,8 +246,7 @@ static void update_handled_vectors(struct kvm_ioapic > *ioapic) > smp_wmb(); > } > > -void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap, > - u32 *tmr) > +void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap) > { > struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic; > union kvm_ioapic_redirect_entry *e; > @@ -260,13 +259,9 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 > *eoi_exit_bitmap, > kvm_irq_has_notifier(ioapic->kvm, KVM_IRQCHIP_IOAPIC, > index) || > index == RTC_GSI) { > if (kvm_apic_match_dest(vcpu, NULL, 0, > - e->fields.dest_id, e->fields.dest_mode)) { > + e->fields.dest_id, e->fields.dest_mode)) > __set_bit(e->fields.vector, > (unsigned long *)eoi_exit_bitmap); > - if (e->fields.trig_mode == IOAPIC_LEVEL_TRIG) > - __set_bit(e->fields.vector, > - (unsigned long *)tmr); > - } > } > } > spin_unlock(&ioapic->lock); > diff --git a/arch/x86/kvm/ioapic.h b/arch/x86/kvm/ioapic.h > index ca0b0b4e6256..3dbd0e2aac4e 100644 > --- a/arch/x86/kvm/ioapic.h > +++ b/arch/x86/kvm/ioapic.h > @@ -120,7 +120,6 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct > kvm_lapic *src, > struct kvm_lapic_irq *irq, unsigned long *dest_map); > int kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state); > int kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state); > -void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap, > - u32 *tmr); > +void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap); > > #endif > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index 2a5ca97c263b..9be64c77d6db 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -551,15 +551,6 @@ static void pv_eoi_clr_pending(struct kvm_vcpu *vcpu) > __clear_bit(KVM_APIC_PV_EOI_PENDING, &vcpu->arch.apic_attention); > } > > -void kvm_apic_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr) > -{ > - struct kvm_lapic *apic = vcpu->arch.apic; > - int i; > - > - for (i = 0; i < 8; i++) > - apic_set_reg(apic, APIC_TMR + 0x10 * i, tmr[i]); > -} > - > static void apic_update_ppr(struct kvm_lapic *apic) > { > u32 tpr, isrv, ppr, old_ppr; > @@ -781,6 +772,9 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int > delivery_mode, > case APIC_DM_LOWEST: > vcpu->arch.apic_arb_prio++; > case APIC_DM_FIXED: > + if (unlikely(trig_mode && !level)) > + break; > + > /* FIXME add logic for vcpu on reset */ > if (unlikely(!apic_enabled(apic))) > break; > @@ -790,6 +784,13 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int > delivery_mode, > if (dest_map) > __set_bit(vcpu->vcpu_id, dest_map); > > + if (apic_test_vector(vector, apic->regs + APIC_TMR) != > !!trig_mode) { > + if (trig_mode) > + apic_set_vector(vector, apic->regs + APIC_TMR); > + else > + apic_clear_vector(vector, apic->regs + > APIC_TMR); > + } > + > if (kvm_x86_ops->deliver_posted_interrupt) > kvm_x86_ops->deliver_posted_interrupt(vcpu, vector); > else { > diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h > index 764037991d26..eb46d6bcaa75 100644 > --- a/arch/x86/kvm/lapic.h > +++ b/arch/x86/kvm/lapic.h > @@ -57,7 +57,6 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value); > u64 kvm_lapic_get_base(struct kvm_vcpu *vcpu); > void kvm_apic_set_version(struct kvm_vcpu *vcpu); > > -void kvm_apic_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr); > void __kvm_apic_update_irr(u32 *pir, void *regs); > void kvm_apic_update_irr(struct kvm_vcpu *vcpu, u32 *pir); > int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *
[PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted
Do not compute TMR in advance. Instead, set the TMR just before the interrupt is accepted into the IRR. This limits the coupling between IOAPIC and LAPIC. Signed-off-by: Paolo Bonzini --- arch/x86/kvm/ioapic.c | 9 ++--- arch/x86/kvm/ioapic.h | 3 +-- arch/x86/kvm/lapic.c | 19 ++- arch/x86/kvm/lapic.h | 1 - arch/x86/kvm/x86.c| 5 + 5 files changed, 14 insertions(+), 23 deletions(-) diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c index 856f79105bb5..eaf4ec38d980 100644 --- a/arch/x86/kvm/ioapic.c +++ b/arch/x86/kvm/ioapic.c @@ -246,8 +246,7 @@ static void update_handled_vectors(struct kvm_ioapic *ioapic) smp_wmb(); } -void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap, - u32 *tmr) +void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap) { struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic; union kvm_ioapic_redirect_entry *e; @@ -260,13 +259,9 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap, kvm_irq_has_notifier(ioapic->kvm, KVM_IRQCHIP_IOAPIC, index) || index == RTC_GSI) { if (kvm_apic_match_dest(vcpu, NULL, 0, - e->fields.dest_id, e->fields.dest_mode)) { + e->fields.dest_id, e->fields.dest_mode)) __set_bit(e->fields.vector, (unsigned long *)eoi_exit_bitmap); - if (e->fields.trig_mode == IOAPIC_LEVEL_TRIG) - __set_bit(e->fields.vector, - (unsigned long *)tmr); - } } } spin_unlock(&ioapic->lock); diff --git a/arch/x86/kvm/ioapic.h b/arch/x86/kvm/ioapic.h index ca0b0b4e6256..3dbd0e2aac4e 100644 --- a/arch/x86/kvm/ioapic.h +++ b/arch/x86/kvm/ioapic.h @@ -120,7 +120,6 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src, struct kvm_lapic_irq *irq, unsigned long *dest_map); int kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state); int kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state); -void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap, - u32 *tmr); +void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap); #endif diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index 2a5ca97c263b..9be64c77d6db 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -551,15 +551,6 @@ static void pv_eoi_clr_pending(struct kvm_vcpu *vcpu) __clear_bit(KVM_APIC_PV_EOI_PENDING, &vcpu->arch.apic_attention); } -void kvm_apic_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr) -{ - struct kvm_lapic *apic = vcpu->arch.apic; - int i; - - for (i = 0; i < 8; i++) - apic_set_reg(apic, APIC_TMR + 0x10 * i, tmr[i]); -} - static void apic_update_ppr(struct kvm_lapic *apic) { u32 tpr, isrv, ppr, old_ppr; @@ -781,6 +772,9 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, case APIC_DM_LOWEST: vcpu->arch.apic_arb_prio++; case APIC_DM_FIXED: + if (unlikely(trig_mode && !level)) + break; + /* FIXME add logic for vcpu on reset */ if (unlikely(!apic_enabled(apic))) break; @@ -790,6 +784,13 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, if (dest_map) __set_bit(vcpu->vcpu_id, dest_map); + if (apic_test_vector(vector, apic->regs + APIC_TMR) != !!trig_mode) { + if (trig_mode) + apic_set_vector(vector, apic->regs + APIC_TMR); + else + apic_clear_vector(vector, apic->regs + APIC_TMR); + } + if (kvm_x86_ops->deliver_posted_interrupt) kvm_x86_ops->deliver_posted_interrupt(vcpu, vector); else { diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h index 764037991d26..eb46d6bcaa75 100644 --- a/arch/x86/kvm/lapic.h +++ b/arch/x86/kvm/lapic.h @@ -57,7 +57,6 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value); u64 kvm_lapic_get_base(struct kvm_vcpu *vcpu); void kvm_apic_set_version(struct kvm_vcpu *vcpu); -void kvm_apic_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr); void __kvm_apic_update_irr(u32 *pir, void *regs); void kvm_apic_update_irr(struct kvm_vcpu *vcpu, u32 *pir); int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq, diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 23e47a0b054b..48dc954542db 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -6149,17 +6149,14 @@ static void process_smi(struct kvm_vc