* RE: [PATCH] KVM: lapic: sync highest ISR to hardware apic on EOI
2014-05-23 14:51 [PATCH] KVM: lapic: sync highest ISR to hardware apic on EOI Paolo Bonzini
@ 2014-05-26 3:44 ` Zhang, Yang Z
2014-05-26 13:07 ` Paolo Bonzini
2014-05-26 14:28 ` Michael S. Tsirkin
2014-05-28 16:57 ` Marcelo Tosatti
2 siblings, 1 reply; 9+ messages in thread
From: Zhang, Yang Z @ 2014-05-26 3:44 UTC (permalink / raw)
To: Paolo Bonzini, linux-kernel@vger.kernel.org
Cc: mtosatti@redhat.com, stable@vger.kernel.org
Paolo Bonzini wrote on 2014-05-23:
> When Hyper-V enlightenments are in effect, Windows prefers to issue an
> Hyper-V MSR write to issue an EOI rather than an x2apic MSR write.
> The Hyper-V MSR write is not handled by the processor, and besides
> being slower, this also causes bugs with APIC virtualization. The
> reason is that on EOI the processor will modify the highest in-service
> interrupt (SVI) field of the VMCS, as explained in section 29.1.4 of
> the SDM.
>
Not only SVI update. It also includes ISR and PPR update. During PPR update, a new pending interrupt may be recognized and inject to guest.
> We need to do the same, and be careful not to muck with the isr_count
> and highest_isr_cache fields that are unused when virtual interrupt
> delivery is enabled.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> arch/x86/kvm/lapic.c | 62
> ++++++++++++++++++++++++++++++++++---------------
> 1 files changed, 43 insertions(+), 19 deletions(-)
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 9736529..0069118 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -360,6 +360,8 @@ static inline void apic_clear_irr(int vec, struct
> kvm_lapic *apic)
>
> static inline void apic_set_isr(int vec, struct kvm_lapic *apic)
> {
> + /* Note that we never get here with APIC virtualization enabled. */
> +
> if (!__apic_test_and_set_vector(vec, apic->regs + APIC_ISR))
> ++apic->isr_count;
> BUG_ON(apic->isr_count > MAX_APIC_VECTOR);
> @@ -371,12 +373,48 @@ static inline void apic_set_isr(int vec, struct
> kvm_lapic *apic)
> apic->highest_isr_cache = vec;
> }
>
> +static inline int apic_find_highest_isr(struct kvm_lapic *apic)
> +{
> + int result;
> +
> + /*
> + * Note that isr_count is always 1, and highest_isr_cache
> + * is always -1, with APIC virtualization enabled.
> + */
> + if (!apic->isr_count)
> + return -1;
> + if (likely(apic->highest_isr_cache != -1))
> + return apic->highest_isr_cache;
> +
> + result = find_highest_vector(apic->regs + APIC_ISR);
> + ASSERT(result == -1 || result >= 16);
> +
> + return result;
> +}
> +
> static inline void apic_clear_isr(int vec, struct kvm_lapic *apic)
> {
> - if (__apic_test_and_clear_vector(vec, apic->regs + APIC_ISR))
> + struct kvm_vcpu *vcpu;
> + if (!__apic_test_and_clear_vector(vec, apic->regs + APIC_ISR))
> + return;
> +
> + vcpu = apic->vcpu;
> +
> + /*
> + * We do get here for APIC virtualization enabled if the guest
> + * uses the Hyper-V APIC enlightenment. In this case we may need
> + * to trigger a new interrupt delivery by writing the SVI field;
> + * on the other hand isr_count and highest_isr_cache are unused
> + * and must be left alone.
> + */
> + if (unlikely(kvm_apic_vid_enabled(vcpu->kvm)))
> + kvm_x86_ops->hwapic_isr_update(vcpu->kvm,
> + apic_find_highest_isr(apic));
If there is a pending interrupt, will it be recognized? I am not looking into the Hyper-V enlightenments code, not sure whether it already covers interrupt recognition. But if it doesn't do it, then we need to do it.
> + else {
> --apic->isr_count;
> - BUG_ON(apic->isr_count < 0);
> - apic->highest_isr_cache = -1;
> + BUG_ON(apic->isr_count < 0);
> + apic->highest_isr_cache = -1;
> + }
> }
>
> int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
> @@ -456,22 +494,6 @@ static void pv_eoi_clr_pending(struct kvm_vcpu
> *vcpu)
> __clear_bit(KVM_APIC_PV_EOI_PENDING, &vcpu->arch.apic_attention);
> }
>
> -static inline int apic_find_highest_isr(struct kvm_lapic *apic)
> -{
> - int result;
> -
> - /* Note that isr_count is always 1 with vid enabled */
> - if (!apic->isr_count)
> - return -1;
> - if (likely(apic->highest_isr_cache != -1))
> - return apic->highest_isr_cache;
> -
> - result = find_highest_vector(apic->regs + APIC_ISR);
> - ASSERT(result == -1 || result >= 16);
> -
> - return result;
> -}
> -
> void kvm_apic_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr)
> {
> struct kvm_lapic *apic = vcpu->arch.apic;
> @@ -1605,6 +1627,8 @@ int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu)
> int vector = kvm_apic_has_interrupt(vcpu);
> struct kvm_lapic *apic = vcpu->arch.apic;
>
> + /* Note that we never get here with APIC virtualization enabled. */
> +
> if (vector == -1)
> return -1;
>
> --
> 1.7.1
Best regards,
Yang
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] KVM: lapic: sync highest ISR to hardware apic on EOI
2014-05-26 3:44 ` Zhang, Yang Z
@ 2014-05-26 13:07 ` Paolo Bonzini
2014-05-27 0:26 ` Zhang, Yang Z
0 siblings, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2014-05-26 13:07 UTC (permalink / raw)
To: Zhang, Yang Z, linux-kernel@vger.kernel.org
Cc: mtosatti@redhat.com, stable@vger.kernel.org
Il 26/05/2014 05:44, Zhang, Yang Z ha scritto:
> Paolo Bonzini wrote on 2014-05-23:
>> When Hyper-V enlightenments are in effect, Windows prefers to issue an
>> Hyper-V MSR write to issue an EOI rather than an x2apic MSR write.
>> The Hyper-V MSR write is not handled by the processor, and besides
>> being slower, this also causes bugs with APIC virtualization. The
>> reason is that on EOI the processor will modify the highest in-service
>> interrupt (SVI) field of the VMCS, as explained in section 29.1.4 of
>> the SDM.
>>
>
> Not only SVI update. It also includes ISR and PPR update. During PPR
> update, a new pending interrupt may be recognized and inject to guest.
Right, but SVI update is the only part that is missing. Writing VISR is
done by apic_clear_isr and PPR virtualization is done by
apic_update_ppr. PPR virtualization is also done anyway at any VM entry,
together with evaluating and delivering pending virtual interrupts.
We'll do two PPR virtualizations (one in KVM, one in the processor), but
that's ok because they're idempotent.
We also operate as if the EOI exit bitmap was all ones, but that's ok
because a useless kvm_ioapic_send_eoi is not harmful.
>> static inline void apic_clear_isr(int vec, struct kvm_lapic *apic)
>> {
>> - if (__apic_test_and_clear_vector(vec, apic->regs + APIC_ISR))
>> + struct kvm_vcpu *vcpu;
>> + if (!__apic_test_and_clear_vector(vec, apic->regs + APIC_ISR))
>> + return;
>> +
>> + vcpu = apic->vcpu;
>> +
>> + /*
>> + * We do get here for APIC virtualization enabled if the guest
>> + * uses the Hyper-V APIC enlightenment. In this case we may need
>> + * to trigger a new interrupt delivery by writing the SVI field;
>> + * on the other hand isr_count and highest_isr_cache are unused
>> + * and must be left alone.
>> + */
>> + if (unlikely(kvm_apic_vid_enabled(vcpu->kvm)))
>> + kvm_x86_ops->hwapic_isr_update(vcpu->kvm,
>> + apic_find_highest_isr(apic));
>
> If there is a pending interrupt, will it be recognized? I am not
> looking into the Hyper-V enlightenments code, not sure whether it
> already covers interrupt recognition. But if it doesn't do it, then we
> need to do it.
Yes, on the next VM entry the processor will do RVI to the PPR. Before
the VM entry KVM_REQ_EVENT will also be processed, which updates RVI in
hwapic_irr_update
.
Paolo
^ permalink raw reply [flat|nested] 9+ messages in thread* RE: [PATCH] KVM: lapic: sync highest ISR to hardware apic on EOI
2014-05-26 13:07 ` Paolo Bonzini
@ 2014-05-27 0:26 ` Zhang, Yang Z
0 siblings, 0 replies; 9+ messages in thread
From: Zhang, Yang Z @ 2014-05-27 0:26 UTC (permalink / raw)
To: Paolo Bonzini, linux-kernel@vger.kernel.org
Cc: mtosatti@redhat.com, stable@vger.kernel.org
Paolo Bonzini wrote on 2014-05-26:
> Il 26/05/2014 05:44, Zhang, Yang Z ha scritto:
>> Paolo Bonzini wrote on 2014-05-23:
>>> When Hyper-V enlightenments are in effect, Windows prefers to issue
>>> an Hyper-V MSR write to issue an EOI rather than an x2apic MSR write.
>>> The Hyper-V MSR write is not handled by the processor, and besides
>>> being slower, this also causes bugs with APIC virtualization. The
>>> reason is that on EOI the processor will modify the highest
>>> in-service interrupt (SVI) field of the VMCS, as explained in
>>> section
>>> 29.1.4 of the SDM.
>>>
>>
>> Not only SVI update. It also includes ISR and PPR update. During PPR
>> update, a new pending interrupt may be recognized and inject to guest.
>
> Right, but SVI update is the only part that is missing. Writing VISR
> is done by apic_clear_isr and PPR virtualization is done by
> apic_update_ppr. PPR virtualization is also done anyway at any VM
> entry, together with evaluating and delivering pending virtual interrupts.
>
> We'll do two PPR virtualizations (one in KVM, one in the processor),
> but that's ok because they're idempotent.
>
> We also operate as if the EOI exit bitmap was all ones, but that's ok
> because a useless kvm_ioapic_send_eoi is not harmful.
>
>>> static inline void apic_clear_isr(int vec, struct kvm_lapic *apic)
>>> {
>>> - if (__apic_test_and_clear_vector(vec, apic->regs + APIC_ISR))
>>> + struct kvm_vcpu *vcpu;
>>> + if (!__apic_test_and_clear_vector(vec, apic->regs + APIC_ISR))
>>> + return;
>>> +
>>> + vcpu = apic->vcpu;
>>> +
>>> + /*
>>> + * We do get here for APIC virtualization enabled if the guest
>>> + * uses the Hyper-V APIC enlightenment. In this case we may need
>>> + * to trigger a new interrupt delivery by writing the SVI field;
>>> + * on the other hand isr_count and highest_isr_cache are unused
>>> + * and must be left alone.
>>> + */
>>> + if (unlikely(kvm_apic_vid_enabled(vcpu->kvm)))
>>> + kvm_x86_ops->hwapic_isr_update(vcpu->kvm,
>>> + apic_find_highest_isr(apic));
>>
>> If there is a pending interrupt, will it be recognized? I am not
>> looking into the Hyper-V enlightenments code, not sure whether it
>> already covers interrupt recognition. But if it doesn't do it, then
>> we need to do it.
>
> Yes, on the next VM entry the processor will do RVI to the PPR.
> Before the VM entry KVM_REQ_EVENT will also be processed, which
> updates RVI in hwapic_irr_update .
Ok, thanks for explanation.
Reviewed-by: Yang Zhang <yang.z.zhang@intel.com>
>
> Paolo
Best regards,
Yang
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: lapic: sync highest ISR to hardware apic on EOI
2014-05-23 14:51 [PATCH] KVM: lapic: sync highest ISR to hardware apic on EOI Paolo Bonzini
2014-05-26 3:44 ` Zhang, Yang Z
@ 2014-05-26 14:28 ` Michael S. Tsirkin
2014-05-26 14:56 ` Paolo Bonzini
2014-05-28 16:57 ` Marcelo Tosatti
2 siblings, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2014-05-26 14:28 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: linux-kernel, yang.z.zhang, mtosatti, stable, kvm
On Fri, May 23, 2014 at 04:51:53PM +0200, Paolo Bonzini wrote:
> When Hyper-V enlightenments are in effect, Windows prefers to issue an
> Hyper-V MSR write to issue an EOI rather than an x2apic MSR write.
> The Hyper-V MSR write is not handled by the processor, and besides
> being slower, this also causes bugs with APIC virtualization. The
> reason is that on EOI the processor will modify the highest in-service
> interrupt (SVI) field of the VMCS, as explained in section 29.1.4 of
> the SDM.
>
> We need to do the same, and be careful not to muck with the isr_count
> and highest_isr_cache fields that are unused when virtual interrupt
> delivery is enabled.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
FWIW
Acked-by: Michael S. Tsirkin <mst@redhat.com>
some questions below.
> ---
> arch/x86/kvm/lapic.c | 62 ++++++++++++++++++++++++++++++++++---------------
> 1 files changed, 43 insertions(+), 19 deletions(-)
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 9736529..0069118 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -360,6 +360,8 @@ static inline void apic_clear_irr(int vec, struct kvm_lapic *apic)
>
> static inline void apic_set_isr(int vec, struct kvm_lapic *apic)
> {
> + /* Note that we never get here with APIC virtualization enabled. */
> +
> if (!__apic_test_and_set_vector(vec, apic->regs + APIC_ISR))
> ++apic->isr_count;
> BUG_ON(apic->isr_count > MAX_APIC_VECTOR);
> @@ -371,12 +373,48 @@ static inline void apic_set_isr(int vec, struct kvm_lapic *apic)
> apic->highest_isr_cache = vec;
> }
>
> +static inline int apic_find_highest_isr(struct kvm_lapic *apic)
> +{
> + int result;
> +
> + /*
> + * Note that isr_count is always 1, and highest_isr_cache
> + * is always -1, with APIC virtualization enabled.
> + */
> + if (!apic->isr_count)
> + return -1;
> + if (likely(apic->highest_isr_cache != -1))
> + return apic->highest_isr_cache;
> +
> + result = find_highest_vector(apic->regs + APIC_ISR);
> + ASSERT(result == -1 || result >= 16);
> +
> + return result;
> +}
> +
> static inline void apic_clear_isr(int vec, struct kvm_lapic *apic)
> {
> - if (__apic_test_and_clear_vector(vec, apic->regs + APIC_ISR))
> + struct kvm_vcpu *vcpu;
> + if (!__apic_test_and_clear_vector(vec, apic->regs + APIC_ISR))
> + return;
> +
> + vcpu = apic->vcpu;
> +
> + /*
> + * We do get here for APIC virtualization enabled if the guest
> + * uses the Hyper-V APIC enlightenment. In this case we may need
> + * to trigger a new interrupt delivery by writing the SVI field;
> + * on the other hand isr_count and highest_isr_cache are unused
> + * and must be left alone.
> + */
> + if (unlikely(kvm_apic_vid_enabled(vcpu->kvm)))
> + kvm_x86_ops->hwapic_isr_update(vcpu->kvm,
> + apic_find_highest_isr(apic));
I note that an indirect call on data path is mostly unnecessary here:
static int vmx_vm_has_apicv(struct kvm *kvm)
{
return enable_apicv && irqchip_in_kernel(kvm);
}
is all it does, and irqchip_in_kernel also has an rmb within it, which
is somewhat expensive on x86: and there's no way to reach this code
with irqchip disabled, correct?
So this is penalizing non-apicv boxes.
How about adding a bool flag in kvm_vcpu_arch, and testing that?
Or even move the enable_apicv module parameter to x86.
> + else {
> --apic->isr_count;
> - BUG_ON(apic->isr_count < 0);
> - apic->highest_isr_cache = -1;
> + BUG_ON(apic->isr_count < 0);
> + apic->highest_isr_cache = -1;
> + }
> }
>
> int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
> @@ -456,22 +494,6 @@ static void pv_eoi_clr_pending(struct kvm_vcpu *vcpu)
> __clear_bit(KVM_APIC_PV_EOI_PENDING, &vcpu->arch.apic_attention);
> }
>
> -static inline int apic_find_highest_isr(struct kvm_lapic *apic)
> -{
> - int result;
> -
> - /* Note that isr_count is always 1 with vid enabled */
> - if (!apic->isr_count)
> - return -1;
> - if (likely(apic->highest_isr_cache != -1))
> - return apic->highest_isr_cache;
> -
> - result = find_highest_vector(apic->regs + APIC_ISR);
> - ASSERT(result == -1 || result >= 16);
> -
> - return result;
> -}
> -
> void kvm_apic_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr)
> {
> struct kvm_lapic *apic = vcpu->arch.apic;
> @@ -1605,6 +1627,8 @@ int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu)
> int vector = kvm_apic_has_interrupt(vcpu);
> struct kvm_lapic *apic = vcpu->arch.apic;
>
> + /* Note that we never get here with APIC virtualization enabled. */
> +
> if (vector == -1)
> return -1;
>
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] KVM: lapic: sync highest ISR to hardware apic on EOI
2014-05-26 14:28 ` Michael S. Tsirkin
@ 2014-05-26 14:56 ` Paolo Bonzini
2014-05-26 15:39 ` Michael S. Tsirkin
0 siblings, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2014-05-26 14:56 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: linux-kernel, yang.z.zhang, mtosatti, stable, kvm
Il 26/05/2014 16:28, Michael S. Tsirkin ha scritto:
>> static inline void apic_clear_isr(int vec, struct kvm_lapic *apic)
>> {
>> - if (__apic_test_and_clear_vector(vec, apic->regs + APIC_ISR))
>> + struct kvm_vcpu *vcpu;
>> + if (!__apic_test_and_clear_vector(vec, apic->regs + APIC_ISR))
>> + return;
>> +
>> + vcpu = apic->vcpu;
>> +
>> + /*
>> + * We do get here for APIC virtualization enabled if the guest
>> + * uses the Hyper-V APIC enlightenment. In this case we may need
>> + * to trigger a new interrupt delivery by writing the SVI field;
>> + * on the other hand isr_count and highest_isr_cache are unused
>> + * and must be left alone.
>> + */
>> + if (unlikely(kvm_apic_vid_enabled(vcpu->kvm)))
>> + kvm_x86_ops->hwapic_isr_update(vcpu->kvm,
>> + apic_find_highest_isr(apic));
>
> I note that an indirect call on data path is mostly unnecessary here:
> static int vmx_vm_has_apicv(struct kvm *kvm)
> {
> return enable_apicv && irqchip_in_kernel(kvm);
> }
> is all it does, and irqchip_in_kernel also has an rmb within it, which
> is somewhat expensive on x86: and there's no way to reach this code
> with irqchip disabled, correct?
smp_rmb is just a compiler barrier, it's not expensive. The indirect
call is probably more expensive.
That said, other places in the paths (kvm_cpu_has_injectable_intr,
kvm_cpu_get_interrupt) already call kvm_apic_vid_enabled, so this patch
doesn't introduce something new.
> How about adding a bool flag in kvm_vcpu_arch, and testing that?
Yes, that's possible. It's also possible to test
kvm_x86_ops->hwapic_isr_update != NULL and zero the field in vmx.c's
hardware_setup. Can you make a patch?
Paolo
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] KVM: lapic: sync highest ISR to hardware apic on EOI
2014-05-26 14:56 ` Paolo Bonzini
@ 2014-05-26 15:39 ` Michael S. Tsirkin
0 siblings, 0 replies; 9+ messages in thread
From: Michael S. Tsirkin @ 2014-05-26 15:39 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: linux-kernel, yang.z.zhang, mtosatti, stable, kvm
On Mon, May 26, 2014 at 04:56:54PM +0200, Paolo Bonzini wrote:
> Il 26/05/2014 16:28, Michael S. Tsirkin ha scritto:
> >> static inline void apic_clear_isr(int vec, struct kvm_lapic *apic)
> >> {
> >>- if (__apic_test_and_clear_vector(vec, apic->regs + APIC_ISR))
> >>+ struct kvm_vcpu *vcpu;
> >>+ if (!__apic_test_and_clear_vector(vec, apic->regs + APIC_ISR))
> >>+ return;
> >>+
> >>+ vcpu = apic->vcpu;
> >>+
> >>+ /*
> >>+ * We do get here for APIC virtualization enabled if the guest
> >>+ * uses the Hyper-V APIC enlightenment. In this case we may need
> >>+ * to trigger a new interrupt delivery by writing the SVI field;
> >>+ * on the other hand isr_count and highest_isr_cache are unused
> >>+ * and must be left alone.
> >>+ */
> >>+ if (unlikely(kvm_apic_vid_enabled(vcpu->kvm)))
> >>+ kvm_x86_ops->hwapic_isr_update(vcpu->kvm,
> >>+ apic_find_highest_isr(apic));
> >
> >I note that an indirect call on data path is mostly unnecessary here:
> > static int vmx_vm_has_apicv(struct kvm *kvm)
> > {
> > return enable_apicv && irqchip_in_kernel(kvm);
> > }
> >is all it does, and irqchip_in_kernel also has an rmb within it, which
> >is somewhat expensive on x86: and there's no way to reach this code
> >with irqchip disabled, correct?
>
> smp_rmb is just a compiler barrier, it's not expensive. The indirect call
> is probably more expensive.
>
> That said, other places in the paths (kvm_cpu_has_injectable_intr,
> kvm_cpu_get_interrupt) already call kvm_apic_vid_enabled, so this patch
> doesn't introduce something new.
>
> >How about adding a bool flag in kvm_vcpu_arch, and testing that?
>
> Yes, that's possible. It's also possible to test
> kvm_x86_ops->hwapic_isr_update != NULL and zero the field in vmx.c's
> hardware_setup.
> Can you make a patch?
>
> Paolo
On top of this one? Sure.
--
MST
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: lapic: sync highest ISR to hardware apic on EOI
2014-05-23 14:51 [PATCH] KVM: lapic: sync highest ISR to hardware apic on EOI Paolo Bonzini
2014-05-26 3:44 ` Zhang, Yang Z
2014-05-26 14:28 ` Michael S. Tsirkin
@ 2014-05-28 16:57 ` Marcelo Tosatti
2014-05-28 17:11 ` Paolo Bonzini
2 siblings, 1 reply; 9+ messages in thread
From: Marcelo Tosatti @ 2014-05-28 16:57 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: linux-kernel, yang.z.zhang, stable
On Fri, May 23, 2014 at 04:51:53PM +0200, Paolo Bonzini wrote:
> When Hyper-V enlightenments are in effect, Windows prefers to issue an
> Hyper-V MSR write to issue an EOI rather than an x2apic MSR write.
> The Hyper-V MSR write is not handled by the processor, and besides
> being slower, this also causes bugs with APIC virtualization. The
> reason is that on EOI the processor will modify the highest in-service
> interrupt (SVI) field of the VMCS, as explained in section 29.1.4 of
> the SDM.
>
> We need to do the same, and be careful not to muck with the isr_count
> and highest_isr_cache fields that are unused when virtual interrupt
> delivery is enabled.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> arch/x86/kvm/lapic.c | 62 ++++++++++++++++++++++++++++++++++---------------
> 1 files changed, 43 insertions(+), 19 deletions(-)
Why not disable Hyper-V APIC enlightenment if APIC-V is available ?
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 9736529..0069118 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -360,6 +360,8 @@ static inline void apic_clear_irr(int vec, struct kvm_lapic *apic)
>
> static inline void apic_set_isr(int vec, struct kvm_lapic *apic)
> {
> + /* Note that we never get here with APIC virtualization enabled. */
> +
> if (!__apic_test_and_set_vector(vec, apic->regs + APIC_ISR))
> ++apic->isr_count;
> BUG_ON(apic->isr_count > MAX_APIC_VECTOR);
> @@ -371,12 +373,48 @@ static inline void apic_set_isr(int vec, struct kvm_lapic *apic)
> apic->highest_isr_cache = vec;
> }
>
> +static inline int apic_find_highest_isr(struct kvm_lapic *apic)
> +{
> + int result;
> +
> + /*
> + * Note that isr_count is always 1, and highest_isr_cache
> + * is always -1, with APIC virtualization enabled.
> + */
> + if (!apic->isr_count)
> + return -1;
> + if (likely(apic->highest_isr_cache != -1))
> + return apic->highest_isr_cache;
> +
> + result = find_highest_vector(apic->regs + APIC_ISR);
> + ASSERT(result == -1 || result >= 16);
> +
> + return result;
> +}
> +
> static inline void apic_clear_isr(int vec, struct kvm_lapic *apic)
> {
> - if (__apic_test_and_clear_vector(vec, apic->regs + APIC_ISR))
> + struct kvm_vcpu *vcpu;
> + if (!__apic_test_and_clear_vector(vec, apic->regs + APIC_ISR))
> + return;
> +
> + vcpu = apic->vcpu;
> +
> + /*
> + * We do get here for APIC virtualization enabled if the guest
> + * uses the Hyper-V APIC enlightenment. In this case we may need
> + * to trigger a new interrupt delivery by writing the SVI field;
> + * on the other hand isr_count and highest_isr_cache are unused
> + * and must be left alone.
> + */
> + if (unlikely(kvm_apic_vid_enabled(vcpu->kvm)))
> + kvm_x86_ops->hwapic_isr_update(vcpu->kvm,
> + apic_find_highest_isr(apic));
> + else {
> --apic->isr_count;
> - BUG_ON(apic->isr_count < 0);
> - apic->highest_isr_cache = -1;
> + BUG_ON(apic->isr_count < 0);
> + apic->highest_isr_cache = -1;
> + }
> }
>
> int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
> @@ -456,22 +494,6 @@ static void pv_eoi_clr_pending(struct kvm_vcpu *vcpu)
> __clear_bit(KVM_APIC_PV_EOI_PENDING, &vcpu->arch.apic_attention);
> }
>
> -static inline int apic_find_highest_isr(struct kvm_lapic *apic)
> -{
> - int result;
> -
> - /* Note that isr_count is always 1 with vid enabled */
> - if (!apic->isr_count)
> - return -1;
> - if (likely(apic->highest_isr_cache != -1))
> - return apic->highest_isr_cache;
> -
> - result = find_highest_vector(apic->regs + APIC_ISR);
> - ASSERT(result == -1 || result >= 16);
> -
> - return result;
> -}
> -
> void kvm_apic_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr)
> {
> struct kvm_lapic *apic = vcpu->arch.apic;
> @@ -1605,6 +1627,8 @@ int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu)
> int vector = kvm_apic_has_interrupt(vcpu);
> struct kvm_lapic *apic = vcpu->arch.apic;
>
> + /* Note that we never get here with APIC virtualization enabled. */
> +
> if (vector == -1)
> return -1;
>
> --
> 1.7.1
Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] KVM: lapic: sync highest ISR to hardware apic on EOI
2014-05-28 16:57 ` Marcelo Tosatti
@ 2014-05-28 17:11 ` Paolo Bonzini
0 siblings, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2014-05-28 17:11 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: linux-kernel, yang.z.zhang, stable
Il 28/05/2014 18:57, Marcelo Tosatti ha scritto:
> On Fri, May 23, 2014 at 04:51:53PM +0200, Paolo Bonzini wrote:
>> When Hyper-V enlightenments are in effect, Windows prefers to issue an
>> Hyper-V MSR write to issue an EOI rather than an x2apic MSR write.
>> The Hyper-V MSR write is not handled by the processor, and besides
>> being slower, this also causes bugs with APIC virtualization. The
>> reason is that on EOI the processor will modify the highest in-service
>> interrupt (SVI) field of the VMCS, as explained in section 29.1.4 of
>> the SDM.
>>
>> We need to do the same, and be careful not to muck with the isr_count
>> and highest_isr_cache fields that are unused when virtual interrupt
>> delivery is enabled.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>> arch/x86/kvm/lapic.c | 62 ++++++++++++++++++++++++++++++++++---------------
>> 1 files changed, 43 insertions(+), 19 deletions(-)
>
> Why not disable Hyper-V APIC enlightenment if APIC-V is available ?
That would be a good suggestion indeed, but it doesn't help if you just
keep your old configuration and get new hardware.
Paolo
^ permalink raw reply [flat|nested] 9+ messages in thread