| Message ID | 20190322115702.10166-5-suravee.suthikulpanit@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM/x86: Add workaround to support ExtINT with AVIC | expand |
On 22/03/2019 12.57, Suthikulpanit, Suravee wrote: > Introduce a helper function for setting lapic parameters when > activate/deactivate apicv. > > Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> > --- > arch/x86/kvm/lapic.c | 23 ++++++++++++++++++----- > arch/x86/kvm/lapic.h | 1 + > 2 files changed, 19 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index 95295cf81283..9c5cd1a98928 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -2126,6 +2126,22 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value) > > } > > +void kvm_apic_update_apicv(struct kvm_vcpu *vcpu) > +{ > + struct kvm_lapic *apic = vcpu->arch.apic; > + bool active = vcpu->arch.apicv_active; > + > + if (active) { > + /* irr_pending is always true when apicv is activated. */ > + apic->irr_pending = true; > + apic->isr_count = 1; > + } else { > + apic->irr_pending = !!count_vectors(apic->regs + APIC_IRR); What about: apic->irr_pending = apic_search_irr(apic) != -1; instead? (more in line with the logic in apic_clear_irr()) Related to this, I wonder if we need to ensure to execute kvm_x86_ops->sync_pir_to_irr() just before apicv_active transitions from true to false, so that we don't miss an interrupt and irr_pending is set correctly in this function (on Intel at least). Hmm... there seems to be other stuff as well, that depends on vcpu->arch.apicv_active, which is not updated on a transition. For example: posted interrupts, CR8 intercept, and a potential asymmetry via avic_vcpu_load()/avic_vcpu_put() because the bottom half of just one of the two functions may be skipped... Do these need to be handled in some way? Regards Jan > + apic->isr_count = count_vectors(apic->regs + APIC_ISR); > + } > +} > +EXPORT_SYMBOL(kvm_apic_update_apicv); > + > void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event) > { > struct kvm_lapic *apic = vcpu->arch.apic; > @@ -2170,8 +2186,7 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event) > kvm_lapic_set_reg(apic, APIC_ISR + 0x10 * i, 0); > kvm_lapic_set_reg(apic, APIC_TMR + 0x10 * i, 0); > } > - apic->irr_pending = vcpu->arch.apicv_active; > - apic->isr_count = vcpu->arch.apicv_active ? 1 : 0; > + kvm_apic_update_apicv(vcpu); > apic->highest_isr_cache = -1; > update_divide_count(apic); > atomic_set(&apic->lapic_timer.pending, 0); > @@ -2433,9 +2448,7 @@ int kvm_apic_set_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s) > apic_manage_nmi_watchdog(apic, kvm_lapic_get_reg(apic, APIC_LVT0)); > update_divide_count(apic); > start_apic_timer(apic); > - apic->irr_pending = true; > - apic->isr_count = vcpu->arch.apicv_active ? > - 1 : count_vectors(apic->regs + APIC_ISR); > + kvm_apic_update_apicv(vcpu); > apic->highest_isr_cache = -1; > if (vcpu->arch.apicv_active) { > kvm_x86_ops->apicv_post_state_restore(vcpu); > diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h > index ff6ef9c3d760..b657605cfec0 100644 > --- a/arch/x86/kvm/lapic.h > +++ b/arch/x86/kvm/lapic.h > @@ -88,6 +88,7 @@ void kvm_apic_update_ppr(struct kvm_vcpu *vcpu); > int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq, > struct dest_map *dest_map); > int kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type); > +void kvm_apic_update_apicv(struct kvm_vcpu *vcpu); > > bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src, > struct kvm_lapic_irq *irq, int *r, struct dest_map *dest_map); >
Jan, On 5/8/2019 5:27 PM, Jan H. Schönherr wrote: > On 22/03/2019 12.57, Suthikulpanit, Suravee wrote: >> Introduce a helper function for setting lapic parameters when >> activate/deactivate apicv. >> >> Signed-off-by: Suravee Suthikulpanit<suravee.suthikulpanit@amd.com> >> --- >> arch/x86/kvm/lapic.c | 23 ++++++++++++++++++----- >> arch/x86/kvm/lapic.h | 1 + >> 2 files changed, 19 insertions(+), 5 deletions(-) >> >> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c >> index 95295cf81283..9c5cd1a98928 100644 >> --- a/arch/x86/kvm/lapic.c >> +++ b/arch/x86/kvm/lapic.c >> @@ -2126,6 +2126,22 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value) >> >> } >> >> +void kvm_apic_update_apicv(struct kvm_vcpu *vcpu) >> +{ >> + struct kvm_lapic *apic = vcpu->arch.apic; >> + bool active = vcpu->arch.apicv_active; >> + >> + if (active) { >> + /* irr_pending is always true when apicv is activated. */ >> + apic->irr_pending = true; >> + apic->isr_count = 1; >> + } else { >> + apic->irr_pending = !!count_vectors(apic->regs + APIC_IRR); > What about: > apic->irr_pending = apic_search_irr(apic) != -1; > instead? (more in line with the logic in apic_clear_irr()) Sure, that works also. > Related to this, I wonder if we need to ensure to execute > kvm_x86_ops->sync_pir_to_irr() just before apicv_active transitions > from true to false, so that we don't miss an interrupt and irr_pending > is set correctly in this function (on Intel at least). I'm not sure about the PIR on Intel, but AMD since AMD IOMMU hardware directly updates the vAPIC backing page, the driver should not need to worry. > Hmm... there seems to be other stuff as well, that depends on > vcpu->arch.apicv_active, which is not updated on a transition. For > example: posted interrupts, You are right. We need to also handle the posted-interrupt activate/deactivate. I am also doing this in V2. > CR8 intercept, When APICv is deactivated, the update_cr8_intercept() should handle updating of CR8 interception. > and a potential asymmetry via avic_vcpu_load()/avic_vcpu_put() > because the bottom half of just one of the two functions may be > skipped Not sure if I understand the concern that you mentioned here. However, once we add the IOMMU activation/deactivation code for posted-interrupt, we should not need to worry about calling avic_update_iommu_vcpu_affinity() at the end of the functions here. Thanks, Suravee > Regards > Jan >
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index 95295cf81283..9c5cd1a98928 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -2126,6 +2126,22 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value) } +void kvm_apic_update_apicv(struct kvm_vcpu *vcpu) +{ + struct kvm_lapic *apic = vcpu->arch.apic; + bool active = vcpu->arch.apicv_active; + + if (active) { + /* irr_pending is always true when apicv is activated. */ + apic->irr_pending = true; + apic->isr_count = 1; + } else { + apic->irr_pending = !!count_vectors(apic->regs + APIC_IRR); + apic->isr_count = count_vectors(apic->regs + APIC_ISR); + } +} +EXPORT_SYMBOL(kvm_apic_update_apicv); + void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event) { struct kvm_lapic *apic = vcpu->arch.apic; @@ -2170,8 +2186,7 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event) kvm_lapic_set_reg(apic, APIC_ISR + 0x10 * i, 0); kvm_lapic_set_reg(apic, APIC_TMR + 0x10 * i, 0); } - apic->irr_pending = vcpu->arch.apicv_active; - apic->isr_count = vcpu->arch.apicv_active ? 1 : 0; + kvm_apic_update_apicv(vcpu); apic->highest_isr_cache = -1; update_divide_count(apic); atomic_set(&apic->lapic_timer.pending, 0); @@ -2433,9 +2448,7 @@ int kvm_apic_set_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s) apic_manage_nmi_watchdog(apic, kvm_lapic_get_reg(apic, APIC_LVT0)); update_divide_count(apic); start_apic_timer(apic); - apic->irr_pending = true; - apic->isr_count = vcpu->arch.apicv_active ? - 1 : count_vectors(apic->regs + APIC_ISR); + kvm_apic_update_apicv(vcpu); apic->highest_isr_cache = -1; if (vcpu->arch.apicv_active) { kvm_x86_ops->apicv_post_state_restore(vcpu); diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h index ff6ef9c3d760..b657605cfec0 100644 --- a/arch/x86/kvm/lapic.h +++ b/arch/x86/kvm/lapic.h @@ -88,6 +88,7 @@ void kvm_apic_update_ppr(struct kvm_vcpu *vcpu); int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq, struct dest_map *dest_map); int kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type); +void kvm_apic_update_apicv(struct kvm_vcpu *vcpu); bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src, struct kvm_lapic_irq *irq, int *r, struct dest_map *dest_map);
Introduce a helper function for setting lapic parameters when activate/deactivate apicv. Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> --- arch/x86/kvm/lapic.c | 23 ++++++++++++++++++----- arch/x86/kvm/lapic.h | 1 + 2 files changed, 19 insertions(+), 5 deletions(-)