Re: [PATCH] KVM: lapic: sync highest ISR to hardware apic on EOI
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: sta...@vger.kernel.org Signed-off-by: Paolo Bonzini --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] KVM: lapic: sync highest ISR to hardware apic on EOI
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: sta...@vger.kernel.org
> Signed-off-by: Paolo Bonzini
> ---
> 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, >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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] KVM: lapic: sync highest ISR to hardware apic on EOI
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: sta...@vger.kernel.org
Signed-off-by: Paolo Bonzini pbonz...@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 mtosa...@redhat.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] KVM: lapic: sync highest ISR to hardware apic on EOI
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: sta...@vger.kernel.org Signed-off-by: Paolo Bonzini pbonz...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] KVM: lapic: sync highest ISR to hardware apic on EOI
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
>
> Paolo
Best regards,
Yang
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] KVM: lapic: sync highest ISR to hardware apic on EOI
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] KVM: lapic: sync highest ISR to hardware apic on EOI
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] KVM: lapic: sync highest ISR to hardware apic on EOI
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: sta...@vger.kernel.org
> Signed-off-by: Paolo Bonzini
FWIW
Acked-by: Michael S. Tsirkin
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, >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"
Re: [PATCH] KVM: lapic: sync highest ISR to hardware apic on EOI
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] KVM: lapic: sync highest ISR to hardware apic on EOI
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] KVM: lapic: sync highest ISR to hardware apic on EOI
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: sta...@vger.kernel.org
Signed-off-by: Paolo Bonzini pbonz...@redhat.com
FWIW
Acked-by: Michael S. Tsirkin m...@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 majord...@vger.kernel.org
More majordomo info at
Re: [PATCH] KVM: lapic: sync highest ISR to hardware apic on EOI
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] KVM: lapic: sync highest ISR to hardware apic on EOI
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] KVM: lapic: sync highest ISR to hardware apic on EOI
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.zh...@intel.com
Paolo
Best regards,
Yang
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] KVM: lapic: sync highest ISR to hardware apic on EOI
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: sta...@vger.kernel.org
> Signed-off-by: Paolo Bonzini
> ---
> 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, >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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] KVM: lapic: sync highest ISR to hardware apic on EOI
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: sta...@vger.kernel.org
Signed-off-by: Paolo Bonzini pbonz...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
