Gleb Natapov wrote on 2012-11-22:
> On Wed, Nov 21, 2012 at 04:09:36PM +0800, Yang Zhang 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.
>> Signed-off-by: Yang Zhang <yang.z.zhang@xxxxxxxxx>
>> Signed-off-by: Kevin Tian <kevin.tian@xxxxxxxxx>
>> ---
>> arch/x86/include/asm/kvm_host.h | 4 + arch/x86/include/asm/vmx.h
>> | 11 ++++ arch/x86/kvm/irq.c | 44 ++++++++++++++
>> arch/x86/kvm/lapic.c | 44 +++++++++++++-
>> arch/x86/kvm/lapic.h | 13 ++++ arch/x86/kvm/svm.c
>> | 6 ++ arch/x86/kvm/vmx.c | 125
>> ++++++++++++++++++++++++++++++++++++++- arch/x86/kvm/x86.c
>> | 16 +++++- virt/kvm/ioapic.c | 1 + 9 files
>> changed, 260 insertions(+), 4 deletions(-)
>> diff --git a/arch/x86/include/asm/kvm_host.h
>> b/arch/x86/include/asm/kvm_host.h index b2e11f4..8e07a86 100644 ---
>> a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -682,6 +682,10 @@ 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);
>> + void (*set_eoi_exitmap)(struct kvm_vcpu *vcpu, int vector,
>> + int need_eoi, int global);
>> 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/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
>> index 21101b6..1003341 100644
>> --- a/arch/x86/include/asm/vmx.h
>> +++ b/arch/x86/include/asm/vmx.h
>> @@ -62,6 +62,7 @@
>> #define EXIT_REASON_MCE_DURING_VMENTRY 41 #define
>> EXIT_REASON_TPR_BELOW_THRESHOLD 43 #define EXIT_REASON_APIC_ACCESS
>> 44 +#define EXIT_REASON_EOI_INDUCED 45 #define
>> EXIT_REASON_EPT_VIOLATION 48 #define EXIT_REASON_EPT_MISCONFIG
>> 49 #define EXIT_REASON_WBINVD 54 @@ -143,6 +144,7 @@
>> #define SECONDARY_EXEC_WBINVD_EXITING 0x00000040 #define
>> SECONDARY_EXEC_UNRESTRICTED_GUEST 0x00000080 #define
>> SECONDARY_EXEC_APIC_REGISTER_VIRT 0x00000100 +#define
>> SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY 0x00000200 #define
>> SECONDARY_EXEC_PAUSE_LOOP_EXITING 0x00000400 #define
>> SECONDARY_EXEC_ENABLE_INVPCID 0x00001000
>> @@ -180,6 +182,7 @@ enum vmcs_field {
>> GUEST_GS_SELECTOR = 0x0000080a, GUEST_LDTR_SELECTOR
>> = 0x0000080c, GUEST_TR_SELECTOR = 0x0000080e,
>> + GUEST_INTR_STATUS = 0x00000810, HOST_ES_SELECTOR
>> = 0x00000c00, HOST_CS_SELECTOR = 0x00000c02,
>> HOST_SS_SELECTOR = 0x00000c04, @@ -207,6 +210,14 @@
>> enum vmcs_field { APIC_ACCESS_ADDR_HIGH = 0x00002015, EPT_POINTER
>> = 0x0000201a, EPT_POINTER_HIGH =
>> 0x0000201b,
>> + EOI_EXIT_BITMAP0 = 0x0000201c,
>> + EOI_EXIT_BITMAP0_HIGH = 0x0000201d,
>> + EOI_EXIT_BITMAP1 = 0x0000201e,
>> + EOI_EXIT_BITMAP1_HIGH = 0x0000201f,
>> + EOI_EXIT_BITMAP2 = 0x00002020,
>> + EOI_EXIT_BITMAP2_HIGH = 0x00002021,
>> + EOI_EXIT_BITMAP3 = 0x00002022,
>> + EOI_EXIT_BITMAP3_HIGH = 0x00002023,
>> GUEST_PHYSICAL_ADDRESS = 0x00002400,
>> GUEST_PHYSICAL_ADDRESS_HIGH = 0x00002401,
>> VMCS_LINK_POINTER = 0x00002800,
>> diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
>> index 7e06ba1..c7356a3 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_extint(struct kvm_vcpu *v)
>> +{
>> + struct kvm_pic *s;
>> +
>> + if (!irqchip_in_kernel(v->kvm))
>> + return v->arch.interrupt.pending;
>> +
> This does not belong here. If !irqchip_in_kernel() the function will not
> be called. Hmm actually with !irqchip_in_kernel() kernel will oops in
> kvm_apic_vid_enabled() since it dereference vcpu->arch.apic without
> checking if it is NULL.
Right. Will remove it in next version and add the check in kvm_apic_vid_enabled.
>
>> + if (kvm_apic_accept_pic_intr(v)) {
>> + s = pic_irqchip(v->kvm); /* PIC */
>> + return s->output;
>> + } else
>> + return 0;
> This is code duplication from kvm_cpu_has_interrupt(). Write common
> function and call it from kvm_cpu_has_interrupt(), but even that is
> not needed, see below.
Why it is not needed?
>> +}
>> +
>> +/*
>> * 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_extint(struct kvm_vcpu *v)
>> +{
>> + struct kvm_pic *s;
>> + int vector = -1;
>> +
>> + if (!irqchip_in_kernel(v->kvm))
>> + return v->arch.interrupt.nr;
> Same as above.
>
>> +
>> + if (kvm_apic_accept_pic_intr(v)) {
>> + s = pic_irqchip(v->kvm);
>> + s->output = 0; /* PIC */
>> + vector = kvm_pic_read_irq(v->kvm);
> Ditto about code duplication.
>
>> + }
>> + 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 a63ffdc..af48361 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -643,6 +643,12 @@ out:
>> return ret;
>> }
>> +void kvm_set_eoi_exitmap(struct kvm_vcpu *vcpu, int vector,
>> + int need_eoi, int global)
>> +{
>> + kvm_x86_ops->set_eoi_exitmap(vcpu, vector, need_eoi, global);
>> +}
>> +
>> /*
>> * Add a pending IRQ into lapic.
>> * Return 1 if successfully added and 0 if discarded.
>> @@ -664,8 +670,11 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int
> delivery_mode,
>> if (trig_mode) {
>> apic_debug("level trig mode for vector %d", vector);
>> apic_set_vector(vector, apic->regs + APIC_TMR);
>> - } else
>> + kvm_set_eoi_exitmap(vcpu, vector, 1, 0);
>> + } else {
>> apic_clear_vector(vector, apic->regs + APIC_TMR);
>> + kvm_set_eoi_exitmap(vcpu, vector, 0, 0);
> Why not use APIC_TMR directly instead of kvm_set_eoi_exitmap() logic?
Good idea. It seems more reasonable.
>> + }
>>
>> result = !apic_test_and_set_irr(vector, apic);
>> trace_kvm_apic_accept_irq(vcpu->vcpu_id, delivery_mode, @@ -769,6
>> +778,26 @@ static int apic_set_eoi(struct kvm_lapic *apic) return
>> vector; }
>> +/*
>> + * this interface assumes a trap-like exit, which has already finished
>> + * desired side effect including vISR and vPPR update.
>> + */
>> +void kvm_apic_set_eoi_accelerated(struct kvm_vcpu *vcpu, int vector)
>> +{
>> + struct kvm_lapic *apic = vcpu->arch.apic;
>> + int trigger_mode;
>> +
>> + if (apic_test_and_clear_vector(vector, apic->regs + APIC_TMR))
>> + trigger_mode = IOAPIC_LEVEL_TRIG;
>> + else
>> + trigger_mode = IOAPIC_EDGE_TRIG;
>> +
>> + if (!(kvm_apic_get_reg(apic, APIC_SPIV) & APIC_SPIV_DIRECTED_EOI))
>> + kvm_ioapic_update_eoi(apic->vcpu->kvm, vector, trigger_mode);
>> + kvm_make_request(KVM_REQ_EVENT, apic->vcpu);
> More code duplication. Why not call apic_set_eoi() and skip isr/ppr
> logic there if vid is enabled, or put the logic in common function and
> call from both places.
Ok, will change it in next patch.
>> +}
>> +EXPORT_SYMBOL_GPL(kvm_apic_set_eoi_accelerated);
>> +
>> static void apic_send_ipi(struct kvm_lapic *apic) { u32 icr_low =
>> kvm_apic_get_reg(apic, APIC_ICR); @@ -1510,6 +1539,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;
> What do you have vid_enabled for. This is global, not per apic, state.
When inject interrupt to guest, we need this to check whether vid is enabled. If not, use old way to handle the interrupt.
I thing put it in apic is reasonable. Though all vcpu use same configuration, APICv feature is per vcpu too.
>
>> return 0; nomem_free_apic: kfree(apic); @@ -1533,6 +1564,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 = kvm_apic_get_reg(vcpu->arch.apic, APIC_LVT0);
>> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
>> index c42f111..2503a64 100644
>> --- a/arch/x86/kvm/lapic.h
>> +++ b/arch/x86/kvm/lapic.h
>> @@ -20,6 +20,7 @@ struct kvm_lapic {
>> u32 divide_count; struct kvm_vcpu *vcpu; bool irr_pending; + bool
>> vid_enabled; /* Number of bits set in ISR. */ s16 isr_count; /* The
>> highest vector set in ISR; if -1 - invalid, must scan ISR. */ @@ -39,6
>> +40,9 @@ void kvm_free_lapic(struct kvm_vcpu *vcpu); int
>> kvm_apic_has_interrupt(struct kvm_vcpu *vcpu); int
>> kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu); int
>> kvm_get_apic_interrupt(struct kvm_vcpu *vcpu);
>> +int kvm_cpu_has_extint(struct kvm_vcpu *v);
>> +int kvm_cpu_get_extint(struct kvm_vcpu *v);
>> +int kvm_apic_get_highest_irr(struct kvm_vcpu *vcpu);
>> void kvm_lapic_reset(struct kvm_vcpu *vcpu); u64
>> kvm_lapic_get_cr8(struct kvm_vcpu *vcpu); void
>> kvm_lapic_set_tpr(struct kvm_vcpu *vcpu, unsigned long cr8); @@ -50,6
>> +54,8 @@ void kvm_apic_set_version(struct kvm_vcpu *vcpu); int
>> kvm_apic_match_physical_addr(struct kvm_lapic *apic, u16 dest); int
>> kvm_apic_match_logical_addr(struct kvm_lapic *apic, u8 mda); int
>> kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq);
>> +void kvm_set_eoi_exitmap(struct kvm_vcpu *vcpu, int vector,
>> + int need_eoi, int global);
>> int kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type);
>>
>> bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
>> @@ -65,6 +71,7 @@ u64 kvm_get_lapic_tscdeadline_msr(struct kvm_vcpu
> *vcpu);
>> void kvm_set_lapic_tscdeadline_msr(struct kvm_vcpu *vcpu, u64 data);
>>
>> int kvm_apic_write_nodecode(struct kvm_vcpu *vcpu, u32 offset);
>> +void kvm_apic_set_eoi_accelerated(struct kvm_vcpu *vcpu, int vector);
>>
>> void kvm_lapic_set_vapic_addr(struct kvm_vcpu *vcpu, gpa_t vapic_addr);
>> void kvm_lapic_sync_from_vapic(struct kvm_vcpu *vcpu);
>> @@ -81,6 +88,12 @@ static inline bool
> kvm_hv_vapic_assist_page_enabled(struct kvm_vcpu *vcpu)
>> return vcpu->arch.hv_vapic & HV_X64_MSR_APIC_ASSIST_PAGE_ENABLE;
>> }
>> +static inline bool kvm_apic_vid_enabled(struct kvm_vcpu *vcpu)
>> +{
>> + struct kvm_lapic *apic = vcpu->arch.apic;
>> + return apic->vid_enabled;
>> +}
> vcpu->arch.apic can be NULL from where this is called.
>
>> +
>> int kvm_lapic_enable_pv_eoi(struct kvm_vcpu *vcpu, u64 data);
>> void kvm_lapic_init(void);
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index d017df3..b290aba 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -3564,6 +3564,11 @@ static void update_cr8_intercept(struct kvm_vcpu
> *vcpu, int tpr, int irr)
>> set_cr_intercept(svm, INTERCEPT_CR8_WRITE);
>> }
>> +static int svm_has_virtual_interrupt_delivery(struct kvm_vcpu *vcpu)
>> +{
>> + return 0;
>> +}
>> +
>> static int svm_nmi_allowed(struct kvm_vcpu *vcpu) { struct vcpu_svm
>> *svm = to_svm(vcpu); @@ -4283,6 +4288,7 @@ static struct kvm_x86_ops
>> svm_x86_ops = { .enable_nmi_window = enable_nmi_window,
>> .enable_irq_window = enable_irq_window, .update_cr8_intercept =
>> update_cr8_intercept,
>> + .has_virtual_interrupt_delivery = svm_has_virtual_interrupt_delivery,
>>
>> .set_tss_addr = svm_set_tss_addr,
>> .get_tdp_level = get_npt_level,
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index e9287aa..c0d74ce 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -86,6 +86,9 @@ module_param(fasteoi, bool, S_IRUGO);
>> static bool __read_mostly enable_apicv_reg = 0;
>> module_param(enable_apicv_reg, bool, S_IRUGO);
>> +static bool __read_mostly enable_apicv_vid = 0;
>> +module_param(enable_apicv_vid, bool, S_IRUGO);
>> +
>> /*
>> * If nested=1, nested virtualization is supported, i.e., guests may use
>> * VMX and be a hypervisor for its own guests. If nested=0, guests may not
>> @@ -432,6 +435,10 @@ struct vcpu_vmx {
>>
>> bool rdtscp_enabled;
>> + u8 eoi_exitmap_changed;
>> + u64 eoi_exit_bitmap[4];
>> + u64 eoi_exit_bitmap_global[4];
>> +
>> /* Support for a guest hypervisor (nested VMX) */
>> struct nested_vmx nested;
>> };
>> @@ -770,6 +777,12 @@ static inline bool
> cpu_has_vmx_apic_register_virt(void)
>> SECONDARY_EXEC_APIC_REGISTER_VIRT;
>> }
>> +static inline bool cpu_has_vmx_virtual_intr_delivery(void)
>> +{
>> + return vmcs_config.cpu_based_2nd_exec_ctrl &
>> + SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY;
>> +}
>> +
>> static inline bool cpu_has_vmx_flexpriority(void)
>> {
>> return cpu_has_vmx_tpr_shadow() &&
>> @@ -2480,7 +2493,8 @@ static __init int setup_vmcs_config(struct
> vmcs_config *vmcs_conf)
>> SECONDARY_EXEC_PAUSE_LOOP_EXITING |
>> SECONDARY_EXEC_RDTSCP |
>> SECONDARY_EXEC_ENABLE_INVPCID |
>> - SECONDARY_EXEC_APIC_REGISTER_VIRT;
>> + SECONDARY_EXEC_APIC_REGISTER_VIRT |
>> + SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY;
>> if (adjust_vmx_controls(min2, opt2,
>> MSR_IA32_VMX_PROCBASED_CTLS2,
>> &_cpu_based_2nd_exec_control) < 0)
>> @@ -2494,7 +2508,8 @@ static __init int setup_vmcs_config(struct
>> vmcs_config *vmcs_conf)
>>
>> if (!(_cpu_based_exec_control & CPU_BASED_TPR_SHADOW))
>> _cpu_based_2nd_exec_control &= ~(
>> - SECONDARY_EXEC_APIC_REGISTER_VIRT);
>> + SECONDARY_EXEC_APIC_REGISTER_VIRT |
>> + SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
>>
>> if (_cpu_based_2nd_exec_control & SECONDARY_EXEC_ENABLE_EPT) { /*
>> CR3 accesses and invlpg don't need to cause VM Exits when EPT @@
>> -2696,6 +2711,9 @@ static __init int hardware_setup(void) if
>> (!cpu_has_vmx_apic_register_virt()) enable_apicv_reg = 0;
>> + if (!cpu_has_vmx_virtual_intr_delivery())
>> + enable_apicv_vid = 0;
>> +
>> if (nested)
>> nested_vmx_setup_ctls_msrs();
>> @@ -3811,6 +3829,8 @@ static u32 vmx_secondary_exec_control(struct
> vcpu_vmx *vmx)
>> exec_control &= ~SECONDARY_EXEC_PAUSE_LOOP_EXITING;
>> if (!enable_apicv_reg)
>> exec_control &= ~SECONDARY_EXEC_APIC_REGISTER_VIRT;
>> + if (!enable_apicv_vid)
>> + exec_control &= ~SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY;
>> return exec_control;
>> }
>> @@ -3855,6 +3875,15 @@ static int vmx_vcpu_setup(struct vcpu_vmx *vmx)
>> vmx_secondary_exec_control(vmx));
>> }
>> + if (enable_apicv_vid) {
>> + vmcs_write64(EOI_EXIT_BITMAP0, 0);
>> + vmcs_write64(EOI_EXIT_BITMAP1, 0);
>> + vmcs_write64(EOI_EXIT_BITMAP2, 0);
>> + vmcs_write64(EOI_EXIT_BITMAP3, 0);
>> +
>> + vmcs_write16(GUEST_INTR_STATUS, 0);
>> + }
>> +
>> if (ple_gap) {
>> vmcs_write32(PLE_GAP, ple_gap);
>> vmcs_write32(PLE_WINDOW, ple_window);
>> @@ -4770,6 +4799,16 @@ static int handle_apic_access(struct kvm_vcpu
> *vcpu)
>> return emulate_instruction(vcpu, 0) == EMULATE_DONE;
>> }
>> +static int handle_apic_eoi_induced(struct kvm_vcpu *vcpu)
>> +{
>> + unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
>> + int vector = exit_qualification & 0xff;
>> +
>> + /* EOI-induced VM exit is trap-like and thus no need to adjust IP */
>> + kvm_apic_set_eoi_accelerated(vcpu, vector);
>> + return 1;
>> +}
>> +
>> static int handle_apic_write(struct kvm_vcpu *vcpu)
>> {
>> unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
>> @@ -5719,6 +5758,7 @@ static int (*const kvm_vmx_exit_handlers[])(struct
> kvm_vcpu *vcpu) = {
>> [EXIT_REASON_TPR_BELOW_THRESHOLD] = handle_tpr_below_threshold,
>> [EXIT_REASON_APIC_ACCESS] = handle_apic_access,
>> [EXIT_REASON_APIC_WRITE] = handle_apic_write,
>> + [EXIT_REASON_EOI_INDUCED] = handle_apic_eoi_induced,
>> [EXIT_REASON_WBINVD] = handle_wbinvd,
>> [EXIT_REASON_XSETBV] = handle_xsetbv,
>> [EXIT_REASON_TASK_SWITCH] = handle_task_switch,
>> @@ -6049,6 +6089,11 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu)
>>
>> static void update_cr8_intercept(struct kvm_vcpu *vcpu, int tpr, int irr)
>> {
>> + /* no need for tpr_threshold update if APIC virtual
>> + * interrupt delivery is enabled */
>> + if (!enable_apicv_vid)
>> + return ;
>
> Just set kvm_x86_ops->update_cr8_intercept to NULL if !enable_apicv_vid
> and the function will not be called.
Sure.
>> +
>> if (irr == -1 || tpr < irr) {
>> vmcs_write32(TPR_THRESHOLD, 0);
>> return;
>> @@ -6057,6 +6102,79 @@ static void update_cr8_intercept(struct kvm_vcpu
> *vcpu, int tpr, int irr)
>> vmcs_write32(TPR_THRESHOLD, irr);
>> }
>> +static int vmx_has_virtual_interrupt_delivery(struct kvm_vcpu *vcpu)
>> +{
>> + return irqchip_in_kernel(vcpu->kvm) && enable_apicv_vid;
>> +}
>> +
>> +static void vmx_update_irq(struct kvm_vcpu *vcpu)
>> +{
>> + u16 status;
>> + u8 old;
>> + int vector;
>> + struct vcpu_vmx *vmx = to_vmx(vcpu);
>> +
>> + if (!enable_apicv_vid)
>> + return ;
> Ditto. Set kvm_x86_ops->update_irq to a function that does nothing if
> !enable_apicv_vid. BTW you do not set this callback in SVM code and call
> it unconditionally.
>
>> +
>> + vector = kvm_apic_get_highest_irr(vcpu);
>> + if (vector == -1)
>> + return;
>> +
>> + status = vmcs_read16(GUEST_INTR_STATUS);
>> + old = (u8)status & 0xff;
>> + if ((u8)vector != old) {
>> + status &= ~0xff;
>> + status |= (u8)vector;
>> + vmcs_write16(GUEST_INTR_STATUS, status);
>> + }
> Please write RVI assessor functions.
Sure.
>> +
>> + if (vmx->eoi_exitmap_changed) {
>> +#define UPDATE_EOI_EXITMAP(v, e) { \
>> + if ((v)->eoi_exitmap_changed & (1 << (e))) \
>> + vmcs_write64(EOI_EXIT_BITMAP##e, \
>> + (v)->eoi_exit_bitmap[e] | (v)->eoi_exit_bitmap_global[e]); }
> Inline function would do. But why calculate this on each entry? We want
> EOI exits only for level IOAPIC interrupts and edge IOAPIC interrupt
> with registered notifiers. This configuration rarely changes.
eoi_exitmap_changed is used to track whether the trig mode is changed. As you said, it changes rarely, so this codes seldom will be executed.
>
>
>> +
>> + UPDATE_EOI_EXITMAP(vmx, 0);
>> + UPDATE_EOI_EXITMAP(vmx, 1);
>> + UPDATE_EOI_EXITMAP(vmx, 2);
>> + UPDATE_EOI_EXITMAP(vmx, 3);
>> + vmx->eoi_exitmap_changed = 0;
>> + }
>> +}
>> +
>> +static void vmx_set_eoi_exitmap(struct kvm_vcpu *vcpu,
>> + int vector,
>> + int need_eoi, int global)
>> +{
>> + struct vcpu_vmx *vmx = to_vmx(vcpu);
>> + int index, offset, changed;
>> + unsigned long *eoi_exitmap;
>> +
>> + if (!enable_apicv_vid)
>> + return ;
>> +
>> + if (WARN_ONCE((vector < 0) || (vector > 255),
>> + "KVM VMX: vector (%d) out of range\n", vector))
>> + return;
>> +
>> + index = vector >> 6;
>> + offset = vector & 63;
>> + if (global)
>> + eoi_exitmap =
>> + (unsigned long *)&vmx->eoi_exit_bitmap_global[index];
>> + else
>> + eoi_exitmap = (unsigned long *)&vmx->eoi_exit_bitmap[index];
>> +
>> + if (need_eoi)
>> + changed = !test_and_set_bit(offset, eoi_exitmap);
>> + else
>> + changed = test_and_clear_bit(offset, eoi_exitmap);
>> +
>> + if (changed)
>> + vmx->eoi_exitmap_changed |= 1 << index;
>> +}
>> +
>> static void vmx_complete_atomic_exit(struct vcpu_vmx *vmx) { u32
>> exit_intr_info; @@ -7320,6 +7438,9 @@ static struct kvm_x86_ops
>> vmx_x86_ops = { .enable_nmi_window = enable_nmi_window,
>> .enable_irq_window = enable_irq_window, .update_cr8_intercept =
>> update_cr8_intercept,
>> + .has_virtual_interrupt_delivery = vmx_has_virtual_interrupt_delivery,
>> + .update_irq = vmx_update_irq,
> You need to initialize this one in svm.c too.
>
>> + .set_eoi_exitmap = vmx_set_eoi_exitmap,
>>
>> .set_tss_addr = vmx_set_tss_addr,
>> .get_tdp_level = get_ept_level,
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 4f76417..8b8de3b 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -5190,6 +5190,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_extint(vcpu) &&
>> + kvm_x86_ops->interrupt_allowed(vcpu)) {
>> + kvm_queue_interrupt(vcpu,
>> + kvm_cpu_get_extint(vcpu), false);
>> + kvm_x86_ops->set_irq(vcpu);
>> + }
> Drop all this and modify kvm_cpu_has_interrupt()/kvm_cpu_get_interrupt()
> to consider apic interrupts only if vid is enabled then the if below
> will just work.
Ok.
>
>> } else if (kvm_cpu_has_interrupt(vcpu)) {
>> if (kvm_x86_ops->interrupt_allowed(vcpu)) {
>> kvm_queue_interrupt(vcpu, kvm_cpu_get_interrupt(vcpu),
>> @@ -5289,12 +5296,19 @@ 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 */
>> + kvm_x86_ops->update_irq(vcpu);
>> +
>> 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_extint(vcpu))
>> + kvm_x86_ops->enable_irq_window(vcpu);
> Same as above. With proper kvm_cpu_has_interrupt() implementation this
> id is not needed.
>
>> + } else if (kvm_cpu_has_interrupt(vcpu) || req_int_win)
>> kvm_x86_ops->enable_irq_window(vcpu);
>>
>> if (kvm_lapic_enabled(vcpu)) {
>> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
>> index 166c450..898aa62 100644
>> --- a/virt/kvm/ioapic.c
>> +++ b/virt/kvm/ioapic.c
>> @@ -186,6 +186,7 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int
> irq)
>> /* need to read apic_id from apic regiest since * it can be
>> rewritten */ irqe.dest_id = ioapic->kvm->bsp_vcpu_id;
>> + kvm_set_eoi_exitmap(ioapic->kvm->vcpus[0], irqe.vector, 1, 1); }
>> #endif return kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe);
>> --
>> 1.7.1
>
> --
> Gleb.
Best regards,
Yang
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
