Re: [PATCH 2/5]KVM:x86, apicv: adjust for virtual interrupt delivery
On 09/17/2012 02:28 PM, Li, Jiongxi wrote:
>> > > +} else if (kvm_apic_vid_enabled(vcpu)) {
>> > > +if (kvm_cpu_has_interrupt_apic_vid(vcpu) &&
>> > > +kvm_x86_ops->interrupt_allowed(vcpu)) {
>> > > +kvm_queue_interrupt(vcpu,
>> > > +kvm_cpu_get_interrupt_apic_vid(vcpu),
>> > > false);
>> > > +kvm_x86_ops->set_irq(vcpu);
>> > > +}
>> >
>> > It may be simpler to change kvm_cpu_{has,get}_interrupt to ignore the
>> > apic if virtual interrupt delivery is enabled.
>> OKs
>
> Kvm_cpu_has_interrupt is also called in other place, no just used to judge
> whether
> to inject interrupt manually. For instance, it is called in
kvm_arch_vcpu_runnable.
> In that case, apic can't be ingored. So for safety, I think it is
better to use another
> function here other than change the original kvm_cpu_has_interrupt
function.
Good point.
We can call them *_extint(), since that's what they're accepting.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/5]KVM:x86, apicv: adjust for virtual interrupt delivery
On 09/14/2012 05:15 PM, Li, Jiongxi wrote:
>
>>
>> > @@ -5293,16 +5300,27 @@ static int vcpu_enter_guest(struct kvm_vcpu
>> *vcpu)
>> >}
>> >
>> >if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) {
>> > + /* update archtecture specific hints for APIC virtual interrupt
>> > delivery
>> */
>> > + if (kvm_apic_vid_enabled(vcpu))
>> > + kvm_x86_ops->update_irq(vcpu);
>> > +
>>
>> Not defined.
> This function is defined in patch 3/5. Because virtual interrupt delivery is
> not enabled in this patch. So this function is not called. Since we will
> enable this feature by default, so maybe we can merge PATCH 2,3,4 together
> into one patch.
That will make it hard to review. You might try to build the eoi exit
bitmap in the first patch, and do the rest (including posted interrupts)
in a following patch. If you can split it further, it will be helpful.
>>
>> >inject_pending_event(vcpu);
>> >
>> >/* enable NMI/IRQ window open exits if needed */
>> >if (vcpu->arch.nmi_pending)
>> >kvm_x86_ops->enable_nmi_window(vcpu);
>> > - else if (kvm_cpu_has_interrupt(vcpu) || req_int_win)
>> > + else if (kvm_apic_vid_enabled(vcpu)) {
>> > + if (kvm_cpu_has_interrupt_apic_vid(vcpu))
>> > + kvm_x86_ops->enable_irq_window(vcpu);
>> > + } else if (kvm_cpu_has_interrupt(vcpu) || req_int_win)
>> >kvm_x86_ops->enable_irq_window(vcpu);
>> >
>> >if (kvm_lapic_enabled(vcpu)) {
>> > - update_cr8_intercept(vcpu);
>> > + /* no need for tpr_threshold update if APIC virtual
>> > + * interrupt delivery is enabled
>> > + */
>> > + if (!kvm_apic_vid_enabled(vcpu))
>> > + update_cr8_intercept(vcpu);
>>
>> Perhaps the arch function should do the ignoring.
> You means putting the 'vid_enabled' judgement in
> 'kvm_x86_ops->update_cr8_intercept'? Is it just out of the reason that
> reducing the code change in common code?
One option is to replace all the code above with
kvm_x86_ops->update_irq()
where
static void vmx_update_irq()
{
if (vid) {
... do vid stuff ...
} else
kvm_process_interrupt_queue(); // all the non-vid code above,
in a function
}
So instead of kvm_apic_vid_enabled() scattered throughout the code we
have just one check. svm_update_irq() can just call
kvm_process_interrupt_queue() and work as before, and later we can add
the AVIC code (the svm equivalent of APIC-V).
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2/5]KVM:x86, apicv: adjust for virtual interrupt delivery
> -Original Message-
> From: Li, Jiongxi
> Sent: Friday, September 14, 2012 10:16 PM
> To: 'Avi Kivity'
> Cc: kvm@vger.kernel.org
> Subject: RE: [PATCH 2/5]KVM:x86, apicv: adjust for virtual interrupt delivery
>
> Sorry for the late response
>
> > -Original Message-
> > From: Avi Kivity [mailto:a...@redhat.com]
> > Sent: Friday, September 07, 2012 12:22 AM
> > To: Li, Jiongxi
> > Cc: kvm@vger.kernel.org
> > Subject: Re: [PATCH 2/5]KVM:x86, apicv: adjust for virtual interrupt
> > delivery
> >
> > On 09/05/2012 08:41 AM, Li, Jiongxi wrote:
> > > Virtual interrupt delivery avoids KVM to inject vAPIC interrupts
> > > manually, which is fully taken care of by the hardware. This needs
> > > some special awareness into existing interrupr injection path:
> > >
> > > - for pending interrupt, instead of direct injection, we may need
> > > update architecture specific indicators before resuming to guest.
> > >
> > > - A pending interrupt, which is masked by ISR, should be also
> > > considered in above update action, since hardware will decide
> > > when to inject it at right time. Current has_interrupt and
> > > get_interrupt only returns a valid vector from injection p.o.v.
> > >
> > >
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -5194,6 +5194,13 @@ static void inject_pending_event(struct
> > kvm_vcpu *vcpu)
> > > vcpu->arch.nmi_injected = true;
> > > kvm_x86_ops->set_nmi(vcpu);
> > > }
> > > + } else if (kvm_apic_vid_enabled(vcpu)) {
> > > + if (kvm_cpu_has_interrupt_apic_vid(vcpu) &&
> > > + kvm_x86_ops->interrupt_allowed(vcpu)) {
> > > + kvm_queue_interrupt(vcpu,
> > > + kvm_cpu_get_interrupt_apic_vid(vcpu), false);
> > > + kvm_x86_ops->set_irq(vcpu);
> > > + }
> >
> > It may be simpler to change kvm_cpu_{has,get}_interrupt to ignore the
> > apic if virtual interrupt delivery is enabled.
> OKs
Kvm_cpu_has_interrupt is also called in other place, no just used to judge
whether to inject interrupt manually. For instance, it is called in
kvm_arch_vcpu_runnable. In that case, apic can't be ingored. So for safety, I
think it is better to use another function here other than change the original
kvm_cpu_has_interrupt function.
> >
> > > @@ -5293,16 +5300,27 @@ static int vcpu_enter_guest(struct kvm_vcpu
> > *vcpu)
> > > }
> > >
> > > if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) {
> > > + /* update archtecture specific hints for APIC virtual interrupt
> > > +delivery
> > */
> > > + if (kvm_apic_vid_enabled(vcpu))
> > > + kvm_x86_ops->update_irq(vcpu);
> > > +
> >
> > Not defined.
> This function is defined in patch 3/5. Because virtual interrupt delivery is
> not
> enabled in this patch. So this function is not called. Since we will enable
> this
> feature by default, so maybe we can merge PATCH 2,3,4 together into one
> patch.
> >
> > > inject_pending_event(vcpu);
> > >
> > > /* enable NMI/IRQ window open exits if needed */
> > > if (vcpu->arch.nmi_pending)
> > > kvm_x86_ops->enable_nmi_window(vcpu);
> > > - else if (kvm_cpu_has_interrupt(vcpu) || req_int_win)
> > > + else if (kvm_apic_vid_enabled(vcpu)) {
> > > + if (kvm_cpu_has_interrupt_apic_vid(vcpu))
> > > + kvm_x86_ops->enable_irq_window(vcpu);
> > > + } else if (kvm_cpu_has_interrupt(vcpu) || req_int_win)
> > > kvm_x86_ops->enable_irq_window(vcpu);
> > >
> > > if (kvm_lapic_enabled(vcpu)) {
> > > - update_cr8_intercept(vcpu);
> > > + /* no need for tpr_threshold update if APIC virtual
> > > + * interrupt delivery is enabled
> > > + */
> > > + if (!kvm_apic_vid_enabled(vcpu))
> > > + update_cr8_intercept(vcpu);
> >
> > Perhaps the arch function should do the ignoring.
> You means putting the 'vid_enabled' judgement in
> 'kvm_x86_ops->update_cr8_intercept'? Is it just out of the reason that
> reducing the code change in common code?
> >
> > > kvm_lapic_sync_to_vapic(vcpu);
> > > }
> > > }
> > >
> >
> >
> > --
> > error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2/5]KVM:x86, apicv: adjust for virtual interrupt delivery
Sorry for the late response
> -Original Message-
> From: Avi Kivity [mailto:a...@redhat.com]
> Sent: Friday, September 07, 2012 12:22 AM
> To: Li, Jiongxi
> Cc: kvm@vger.kernel.org
> Subject: Re: [PATCH 2/5]KVM:x86, apicv: adjust for virtual interrupt delivery
>
> On 09/05/2012 08:41 AM, Li, Jiongxi wrote:
> > Virtual interrupt delivery avoids KVM to inject vAPIC interrupts
> > manually, which is fully taken care of by the hardware. This needs
> > some special awareness into existing interrupr injection path:
> >
> > - for pending interrupt, instead of direct injection, we may need
> > update architecture specific indicators before resuming to guest.
> >
> > - A pending interrupt, which is masked by ISR, should be also
> > considered in above update action, since hardware will decide
> > when to inject it at right time. Current has_interrupt and
> > get_interrupt only returns a valid vector from injection p.o.v.
> >
> >
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -5194,6 +5194,13 @@ static void inject_pending_event(struct
> kvm_vcpu *vcpu)
> > vcpu->arch.nmi_injected = true;
> > kvm_x86_ops->set_nmi(vcpu);
> > }
> > + } else if (kvm_apic_vid_enabled(vcpu)) {
> > + if (kvm_cpu_has_interrupt_apic_vid(vcpu) &&
> > + kvm_x86_ops->interrupt_allowed(vcpu)) {
> > + kvm_queue_interrupt(vcpu,
> > + kvm_cpu_get_interrupt_apic_vid(vcpu), false);
> > + kvm_x86_ops->set_irq(vcpu);
> > + }
>
> It may be simpler to change kvm_cpu_{has,get}_interrupt to ignore the apic if
> virtual interrupt delivery is enabled.
OKs
>
> > @@ -5293,16 +5300,27 @@ static int vcpu_enter_guest(struct kvm_vcpu
> *vcpu)
> > }
> >
> > if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) {
> > + /* update archtecture specific hints for APIC virtual interrupt
> > delivery
> */
> > + if (kvm_apic_vid_enabled(vcpu))
> > + kvm_x86_ops->update_irq(vcpu);
> > +
>
> Not defined.
This function is defined in patch 3/5. Because virtual interrupt delivery is
not enabled in this patch. So this function is not called. Since we will enable
this feature by default, so maybe we can merge PATCH 2,3,4 together into one
patch.
>
> > inject_pending_event(vcpu);
> >
> > /* enable NMI/IRQ window open exits if needed */
> > if (vcpu->arch.nmi_pending)
> > kvm_x86_ops->enable_nmi_window(vcpu);
> > - else if (kvm_cpu_has_interrupt(vcpu) || req_int_win)
> > + else if (kvm_apic_vid_enabled(vcpu)) {
> > + if (kvm_cpu_has_interrupt_apic_vid(vcpu))
> > + kvm_x86_ops->enable_irq_window(vcpu);
> > + } else if (kvm_cpu_has_interrupt(vcpu) || req_int_win)
> > kvm_x86_ops->enable_irq_window(vcpu);
> >
> > if (kvm_lapic_enabled(vcpu)) {
> > - update_cr8_intercept(vcpu);
> > + /* no need for tpr_threshold update if APIC virtual
> > +* interrupt delivery is enabled
> > +*/
> > + if (!kvm_apic_vid_enabled(vcpu))
> > + update_cr8_intercept(vcpu);
>
> Perhaps the arch function should do the ignoring.
You means putting the 'vid_enabled' judgement in
'kvm_x86_ops->update_cr8_intercept'? Is it just out of the reason that reducing
the code change in common code?
>
> > kvm_lapic_sync_to_vapic(vcpu);
> > }
> > }
> >
>
>
> --
> error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/5]KVM:x86, apicv: adjust for virtual interrupt delivery
On 09/05/2012 08:41 AM, Li, Jiongxi wrote:
> Virtual interrupt delivery avoids KVM to inject vAPIC interrupts
> manually, which is fully taken care of by the hardware. This needs
> some special awareness into existing interrupr injection path:
>
> - for pending interrupt, instead of direct injection, we may need
> update architecture specific indicators before resuming to guest.
>
> - A pending interrupt, which is masked by ISR, should be also
> considered in above update action, since hardware will decide
> when to inject it at right time. Current has_interrupt and
> get_interrupt only returns a valid vector from injection p.o.v.
>
>
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5194,6 +5194,13 @@ static void inject_pending_event(struct kvm_vcpu *vcpu)
> vcpu->arch.nmi_injected = true;
> kvm_x86_ops->set_nmi(vcpu);
> }
> + } else if (kvm_apic_vid_enabled(vcpu)) {
> + if (kvm_cpu_has_interrupt_apic_vid(vcpu) &&
> + kvm_x86_ops->interrupt_allowed(vcpu)) {
> + kvm_queue_interrupt(vcpu,
> + kvm_cpu_get_interrupt_apic_vid(vcpu), false);
> + kvm_x86_ops->set_irq(vcpu);
> + }
It may be simpler to change kvm_cpu_{has,get}_interrupt to ignore the
apic if virtual interrupt delivery is enabled.
> @@ -5293,16 +5300,27 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> }
>
> if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) {
> + /* update archtecture specific hints for APIC virtual interrupt
> delivery */
> + if (kvm_apic_vid_enabled(vcpu))
> + kvm_x86_ops->update_irq(vcpu);
> +
Not defined.
> inject_pending_event(vcpu);
>
> /* enable NMI/IRQ window open exits if needed */
> if (vcpu->arch.nmi_pending)
> kvm_x86_ops->enable_nmi_window(vcpu);
> - else if (kvm_cpu_has_interrupt(vcpu) || req_int_win)
> + else if (kvm_apic_vid_enabled(vcpu)) {
> + if (kvm_cpu_has_interrupt_apic_vid(vcpu))
> + kvm_x86_ops->enable_irq_window(vcpu);
> + } else if (kvm_cpu_has_interrupt(vcpu) || req_int_win)
> kvm_x86_ops->enable_irq_window(vcpu);
>
> if (kvm_lapic_enabled(vcpu)) {
> - update_cr8_intercept(vcpu);
> + /* no need for tpr_threshold update if APIC virtual
> + * interrupt delivery is enabled
> + */
> + if (!kvm_apic_vid_enabled(vcpu))
> + update_cr8_intercept(vcpu);
Perhaps the arch function should do the ignoring.
> kvm_lapic_sync_to_vapic(vcpu);
> }
> }
>
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/5]KVM:x86, apicv: adjust for virtual interrupt delivery
Virtual interrupt delivery avoids KVM to inject vAPIC interrupts
manually, which is fully taken care of by the hardware. This needs
some special awareness into existing interrupr injection path:
- for pending interrupt, instead of direct injection, we may need
update architecture specific indicators before resuming to guest.
- A pending interrupt, which is masked by ISR, should be also
considered in above update action, since hardware will decide
when to inject it at right time. Current has_interrupt and
get_interrupt only returns a valid vector from injection p.o.v.
Three new interfaces are introduced accordingly:
kvm_apic_get_highest_irr
kvm_cpu_has_interrupt_apicv_vid
kvm_cpu_get_interrupt_apic_vid
Signed-off-by: Kevin Tian
Signed-off-by: Jiongxi Li
---
arch/x86/include/asm/kvm_host.h |2 +
arch/x86/kvm/irq.c | 44 +++
arch/x86/kvm/lapic.c| 13 +++
arch/x86/kvm/lapic.h| 10
arch/x86/kvm/svm.c |6 +
arch/x86/kvm/vmx.c |6 +
arch/x86/kvm/x86.c | 22 +-
7 files changed, 101 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 09155d6..ef74df5 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -669,6 +669,8 @@ struct kvm_x86_ops {
void (*enable_nmi_window)(struct kvm_vcpu *vcpu);
void (*enable_irq_window)(struct kvm_vcpu *vcpu);
void (*update_cr8_intercept)(struct kvm_vcpu *vcpu, int tpr, int irr);
+ int (*has_virtual_interrupt_delivery)(struct kvm_vcpu *vcpu);
+ void (*update_irq)(struct kvm_vcpu *vcpu);
int (*set_tss_addr)(struct kvm *kvm, unsigned int addr);
int (*get_tdp_level)(void);
u64 (*get_mt_mask)(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio);
diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
index 7e06ba1..abd3831 100644
--- a/arch/x86/kvm/irq.c
+++ b/arch/x86/kvm/irq.c
@@ -60,6 +60,29 @@ int kvm_cpu_has_interrupt(struct kvm_vcpu *v)
EXPORT_SYMBOL_GPL(kvm_cpu_has_interrupt);
/*
+ * check if there is pending interrupt without
+ * intack. This _apicv version is used when hardware
+ * supports APIC virtualization with virtual interrupt
+ * delivery support. In such case, KVM is not required
+ * to poll pending APIC interrupt, and thus this
+ * interface is used to poll pending interupts from
+ * non-APIC source.
+ */
+int kvm_cpu_has_interrupt_apic_vid(struct kvm_vcpu *v)
+{
+ struct kvm_pic *s;
+
+ if (!irqchip_in_kernel(v->kvm))
+ return v->arch.interrupt.pending;
+
+ if (kvm_apic_accept_pic_intr(v)) {
+ s = pic_irqchip(v->kvm);/* PIC */
+ return s->output;
+ } else
+ return 0;
+}
+
+/*
* Read pending interrupt vector and intack.
*/
int kvm_cpu_get_interrupt(struct kvm_vcpu *v)
@@ -82,6 +105,27 @@ int kvm_cpu_get_interrupt(struct kvm_vcpu *v)
}
EXPORT_SYMBOL_GPL(kvm_cpu_get_interrupt);
+/*
+ * Read pending interrupt vector and intack.
+ * Similar to kvm_cpu_has_interrupt_apicv, to get
+ * interrupts from non-APIC sources.
+ */
+int kvm_cpu_get_interrupt_apic_vid(struct kvm_vcpu *v)
+{
+ struct kvm_pic *s;
+ int vector = -1;
+
+ if (!irqchip_in_kernel(v->kvm))
+ return v->arch.interrupt.nr;
+
+ if (kvm_apic_accept_pic_intr(v)) {
+ s = pic_irqchip(v->kvm);
+ s->output = 0; /* PIC */
+ vector = kvm_pic_read_irq(v->kvm);
+ }
+ return vector;
+}
+
void kvm_inject_pending_timer_irqs(struct kvm_vcpu *vcpu)
{
kvm_inject_apic_timer_irqs(vcpu);
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 4a6d3a4..c47f3d3 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1310,6 +1310,8 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu)
kvm_lapic_reset(vcpu);
kvm_iodevice_init(&apic->dev, &apic_mmio_ops);
+ if (kvm_x86_ops->has_virtual_interrupt_delivery(vcpu))
+ apic->vid_enabled = true;
return 0;
nomem_free_apic:
kfree(apic);
@@ -1333,6 +1335,17 @@ int kvm_apic_has_interrupt(struct kvm_vcpu *vcpu)
return highest_irr;
}
+int kvm_apic_get_highest_irr(struct kvm_vcpu *vcpu)
+{
+struct kvm_lapic *apic = vcpu->arch.apic;
+
+if (!apic || !apic_enabled(apic))
+return -1;
+
+return apic_find_highest_irr(apic);
+}
+EXPORT_SYMBOL_GPL(kvm_apic_get_highest_irr);
+
int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu)
{
u32 lvt0 = apic_get_reg(vcpu->arch.apic, APIC_LVT0);
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index cd4875e..4e3b435 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -13,6 +13,7 @@ struct kvm_lapic {
u32 divide_count;
struct kvm_vcpu *vcpu;
