Inspired by the split irqchip patches, this series limits the
IOAPIC<->LAPIC to the EOI exit bitmap that is inferred from the
redirection table.
TMR is entirely handled within the local APIC, and no global copy
of the IOAPIC-handled vectors is necessary anymore. This makes
it simpler to introduce a userspace IOAPIC.
Tested with ioapic.flat for now, planning to do more complete tests
tomorrow. The most interesting test to do here is an assigned device
that uses INTX, so I am CCing Alex Williamson for a heads up.
Paolo
Paolo Bonzini (2):
KVM: x86: set TMR when the interrupt is accepted
KVM: x86: store IOAPIC-handled vectors in each VCPU
arch/x86/include/asm/kvm_host.h | 3 ++-
arch/x86/kvm/ioapic.c | 27 ++++-----------------------
arch/x86/kvm/ioapic.h | 11 +----------
arch/x86/kvm/lapic.c | 29 ++++++++++++++++++-----------
arch/x86/kvm/lapic.h | 1 -
arch/x86/kvm/svm.c | 2 +-
arch/x86/kvm/vmx.c | 3 ++-
arch/x86/kvm/x86.c | 11 +++--------
8 files changed, 31 insertions(+), 56 deletions(-)
--
1.8.3.1
Do not compute TMR in advance. Instead, set the TMR just before the interrupt
is accepted into the IRR. This limits the coupling between IOAPIC and LAPIC.
Signed-off-by: Paolo Bonzini <[email protected]>
---
arch/x86/kvm/ioapic.c | 9 ++-------
arch/x86/kvm/ioapic.h | 3 +--
arch/x86/kvm/lapic.c | 19 ++++++++++---------
arch/x86/kvm/lapic.h | 1 -
arch/x86/kvm/x86.c | 5 +----
5 files changed, 14 insertions(+), 23 deletions(-)
diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
index 856f79105bb5..eaf4ec38d980 100644
--- a/arch/x86/kvm/ioapic.c
+++ b/arch/x86/kvm/ioapic.c
@@ -246,8 +246,7 @@ static void update_handled_vectors(struct kvm_ioapic *ioapic)
smp_wmb();
}
-void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap,
- u32 *tmr)
+void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap)
{
struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic;
union kvm_ioapic_redirect_entry *e;
@@ -260,13 +259,9 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap,
kvm_irq_has_notifier(ioapic->kvm, KVM_IRQCHIP_IOAPIC, index) ||
index == RTC_GSI) {
if (kvm_apic_match_dest(vcpu, NULL, 0,
- e->fields.dest_id, e->fields.dest_mode)) {
+ e->fields.dest_id, e->fields.dest_mode))
__set_bit(e->fields.vector,
(unsigned long *)eoi_exit_bitmap);
- if (e->fields.trig_mode == IOAPIC_LEVEL_TRIG)
- __set_bit(e->fields.vector,
- (unsigned long *)tmr);
- }
}
}
spin_unlock(&ioapic->lock);
diff --git a/arch/x86/kvm/ioapic.h b/arch/x86/kvm/ioapic.h
index ca0b0b4e6256..3dbd0e2aac4e 100644
--- a/arch/x86/kvm/ioapic.h
+++ b/arch/x86/kvm/ioapic.h
@@ -120,7 +120,6 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
struct kvm_lapic_irq *irq, unsigned long *dest_map);
int kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
int kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
-void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap,
- u32 *tmr);
+void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap);
#endif
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 2a5ca97c263b..9be64c77d6db 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -551,15 +551,6 @@ static void pv_eoi_clr_pending(struct kvm_vcpu *vcpu)
__clear_bit(KVM_APIC_PV_EOI_PENDING, &vcpu->arch.apic_attention);
}
-void kvm_apic_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr)
-{
- struct kvm_lapic *apic = vcpu->arch.apic;
- int i;
-
- for (i = 0; i < 8; i++)
- apic_set_reg(apic, APIC_TMR + 0x10 * i, tmr[i]);
-}
-
static void apic_update_ppr(struct kvm_lapic *apic)
{
u32 tpr, isrv, ppr, old_ppr;
@@ -781,6 +772,9 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
case APIC_DM_LOWEST:
vcpu->arch.apic_arb_prio++;
case APIC_DM_FIXED:
+ if (unlikely(trig_mode && !level))
+ break;
+
/* FIXME add logic for vcpu on reset */
if (unlikely(!apic_enabled(apic)))
break;
@@ -790,6 +784,13 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
if (dest_map)
__set_bit(vcpu->vcpu_id, dest_map);
+ if (apic_test_vector(vector, apic->regs + APIC_TMR) != !!trig_mode) {
+ if (trig_mode)
+ apic_set_vector(vector, apic->regs + APIC_TMR);
+ else
+ apic_clear_vector(vector, apic->regs + APIC_TMR);
+ }
+
if (kvm_x86_ops->deliver_posted_interrupt)
kvm_x86_ops->deliver_posted_interrupt(vcpu, vector);
else {
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 764037991d26..eb46d6bcaa75 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -57,7 +57,6 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value);
u64 kvm_lapic_get_base(struct kvm_vcpu *vcpu);
void kvm_apic_set_version(struct kvm_vcpu *vcpu);
-void kvm_apic_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr);
void __kvm_apic_update_irr(u32 *pir, void *regs);
void kvm_apic_update_irr(struct kvm_vcpu *vcpu, u32 *pir);
int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 23e47a0b054b..48dc954542db 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6149,17 +6149,14 @@ static void process_smi(struct kvm_vcpu *vcpu)
static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
{
u64 eoi_exit_bitmap[4];
- u32 tmr[8];
if (!kvm_apic_hw_enabled(vcpu->arch.apic))
return;
memset(eoi_exit_bitmap, 0, 32);
- memset(tmr, 0, 32);
- kvm_ioapic_scan_entry(vcpu, eoi_exit_bitmap, tmr);
+ kvm_ioapic_scan_entry(vcpu, eoi_exit_bitmap);
kvm_x86_ops->load_eoi_exitmap(vcpu, eoi_exit_bitmap);
- kvm_apic_update_tmr(vcpu, tmr);
}
static void kvm_vcpu_flush_tlb(struct kvm_vcpu *vcpu)
--
1.8.3.1
We can reuse the algorithm that computes the EOI exit bitmap to figure
out which vectors are handled by the IOAPIC. The only difference
between the two is for edge-triggered interrupts other than IRQ8
that have no notifiers active; however, the IOAPIC does not have to
do anything special for these interrupts anyway.
This again limits the interactions between the IOAPIC and the LAPIC,
making it easier to move the former to userspace.
Inspired by a patch from Steve Rutherford.
Signed-off-by: Paolo Bonzini <[email protected]>
---
arch/x86/include/asm/kvm_host.h | 3 ++-
arch/x86/kvm/ioapic.c | 18 ++----------------
arch/x86/kvm/ioapic.h | 8 --------
arch/x86/kvm/lapic.c | 10 ++++++++--
arch/x86/kvm/svm.c | 2 +-
arch/x86/kvm/vmx.c | 3 ++-
arch/x86/kvm/x86.c | 8 +++-----
7 files changed, 18 insertions(+), 34 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 2f9e504f9f0c..d0e991ef6ef0 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -383,6 +383,7 @@ struct kvm_vcpu_arch {
u64 efer;
u64 apic_base;
struct kvm_lapic *apic; /* kernel irqchip context */
+ u64 eoi_exit_bitmap[4];
unsigned long apic_attention;
int32_t apic_arb_prio;
int mp_state;
@@ -808,7 +809,7 @@ struct kvm_x86_ops {
int (*vm_has_apicv)(struct kvm *kvm);
void (*hwapic_irr_update)(struct kvm_vcpu *vcpu, int max_irr);
void (*hwapic_isr_update)(struct kvm *kvm, int isr);
- void (*load_eoi_exitmap)(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap);
+ void (*load_eoi_exitmap)(struct kvm_vcpu *vcpu);
void (*set_virtual_x2apic_mode)(struct kvm_vcpu *vcpu, bool set);
void (*set_apic_access_page_addr)(struct kvm_vcpu *vcpu, hpa_t hpa);
void (*deliver_posted_interrupt)(struct kvm_vcpu *vcpu, int vector);
diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
index eaf4ec38d980..2dcda0f188ba 100644
--- a/arch/x86/kvm/ioapic.c
+++ b/arch/x86/kvm/ioapic.c
@@ -233,19 +233,6 @@ static void kvm_ioapic_inject_all(struct kvm_ioapic *ioapic, unsigned long irr)
}
-static void update_handled_vectors(struct kvm_ioapic *ioapic)
-{
- DECLARE_BITMAP(handled_vectors, 256);
- int i;
-
- memset(handled_vectors, 0, sizeof(handled_vectors));
- for (i = 0; i < IOAPIC_NUM_PINS; ++i)
- __set_bit(ioapic->redirtbl[i].fields.vector, handled_vectors);
- memcpy(ioapic->handled_vectors, handled_vectors,
- sizeof(handled_vectors));
- smp_wmb();
-}
-
void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap)
{
struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic;
@@ -310,7 +297,6 @@ static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
e->bits |= (u32) val;
e->fields.remote_irr = 0;
}
- update_handled_vectors(ioapic);
mask_after = e->fields.mask;
if (mask_before != mask_after)
kvm_fire_mask_notifiers(ioapic->kvm, KVM_IRQCHIP_IOAPIC, index, mask_after);
@@ -594,7 +580,6 @@ static void kvm_ioapic_reset(struct kvm_ioapic *ioapic)
ioapic->id = 0;
memset(ioapic->irq_eoi, 0x00, IOAPIC_NUM_PINS);
rtc_irq_eoi_tracking_reset(ioapic);
- update_handled_vectors(ioapic);
}
static const struct kvm_io_device_ops ioapic_mmio_ops = {
@@ -623,8 +608,10 @@ int kvm_ioapic_init(struct kvm *kvm)
if (ret < 0) {
kvm->arch.vioapic = NULL;
kfree(ioapic);
+ return ret;
}
+ kvm_vcpu_request_scan_ioapic(kvm);
return ret;
}
@@ -661,7 +648,6 @@ int kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state)
memcpy(ioapic, state, sizeof(struct kvm_ioapic_state));
ioapic->irr = 0;
ioapic->irr_delivered = 0;
- update_handled_vectors(ioapic);
kvm_vcpu_request_scan_ioapic(kvm);
kvm_ioapic_inject_all(ioapic, state->irr);
spin_unlock(&ioapic->lock);
diff --git a/arch/x86/kvm/ioapic.h b/arch/x86/kvm/ioapic.h
index 3dbd0e2aac4e..bf36d66a1951 100644
--- a/arch/x86/kvm/ioapic.h
+++ b/arch/x86/kvm/ioapic.h
@@ -73,7 +73,6 @@ struct kvm_ioapic {
struct kvm *kvm;
void (*ack_notifier)(void *opaque, int irq);
spinlock_t lock;
- DECLARE_BITMAP(handled_vectors, 256);
struct rtc_status rtc_status;
struct delayed_work eoi_inject;
u32 irq_eoi[IOAPIC_NUM_PINS];
@@ -98,13 +97,6 @@ static inline struct kvm_ioapic *ioapic_irqchip(struct kvm *kvm)
return kvm->arch.vioapic;
}
-static inline bool kvm_ioapic_handles_vector(struct kvm *kvm, int vector)
-{
- struct kvm_ioapic *ioapic = kvm->arch.vioapic;
- smp_rmb();
- return test_bit(vector, ioapic->handled_vectors);
-}
-
void kvm_rtc_eoi_tracking_restore_one(struct kvm_vcpu *vcpu);
bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
int short_hand, unsigned int dest, int dest_mode);
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 9be64c77d6db..c158f4667aa3 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -869,14 +869,20 @@ int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcpu2)
return vcpu1->arch.apic_arb_prio - vcpu2->arch.apic_arb_prio;
}
+static bool kvm_ioapic_handles_vector(struct kvm_lapic *apic, int vector)
+{
+ return test_bit(vector, (ulong *)apic->vcpu->arch.eoi_exit_bitmap);
+}
+
static void kvm_ioapic_send_eoi(struct kvm_lapic *apic, int vector)
{
- if (kvm_ioapic_handles_vector(apic->vcpu->kvm, vector)) {
+ if (kvm_ioapic_handles_vector(apic, vector)) {
int trigger_mode;
if (apic_test_vector(vector, apic->regs + APIC_TMR))
trigger_mode = IOAPIC_LEVEL_TRIG;
else
trigger_mode = IOAPIC_EDGE_TRIG;
+
kvm_ioapic_update_eoi(apic->vcpu, vector, trigger_mode);
}
}
@@ -1922,7 +1928,7 @@ static void apic_sync_pv_eoi_to_guest(struct kvm_vcpu *vcpu,
/* Cache not set: could be safe but we don't bother. */
apic->highest_isr_cache == -1 ||
/* Need EOI to update ioapic. */
- kvm_ioapic_handles_vector(vcpu->kvm, apic->highest_isr_cache)) {
+ kvm_ioapic_handles_vector(apic, apic->highest_isr_cache)) {
/*
* PV EOI was disabled by apic_sync_pv_eoi_from_guest
* so we need not do anything here.
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index d1a114d8d22b..676c9eb10174 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -3765,7 +3765,7 @@ static int svm_vm_has_apicv(struct kvm *kvm)
return 0;
}
-static void svm_load_eoi_exitmap(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap)
+static void svm_load_eoi_exitmap(struct kvm_vcpu *vcpu)
{
return;
}
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 4014a82ae846..7323ea8fba88 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -8111,8 +8111,9 @@ static void vmx_hwapic_irr_update(struct kvm_vcpu *vcpu, int max_irr)
}
}
-static void vmx_load_eoi_exitmap(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap)
+static void vmx_load_eoi_exitmap(struct kvm_vcpu *vcpu)
{
+ u64 *eoi_exit_bitmap = vcpu->arch.eoi_exit_bitmap;
if (!vmx_vm_has_apicv(vcpu->kvm))
return;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 48dc954542db..4ad7334aa604 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6148,15 +6148,13 @@ static void process_smi(struct kvm_vcpu *vcpu)
static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
{
- u64 eoi_exit_bitmap[4];
-
if (!kvm_apic_hw_enabled(vcpu->arch.apic))
return;
- memset(eoi_exit_bitmap, 0, 32);
+ memset(vcpu->arch.eoi_exit_bitmap, 0, 256 / 8);
- kvm_ioapic_scan_entry(vcpu, eoi_exit_bitmap);
- kvm_x86_ops->load_eoi_exitmap(vcpu, eoi_exit_bitmap);
+ kvm_ioapic_scan_entry(vcpu, vcpu->arch.eoi_exit_bitmap);
+ kvm_x86_ops->load_eoi_exitmap(vcpu);
}
static void kvm_vcpu_flush_tlb(struct kvm_vcpu *vcpu)
--
1.8.3.1
On Wed, 2015-07-29 at 15:37 +0200, Paolo Bonzini wrote:
> Inspired by the split irqchip patches, this series limits the
> IOAPIC<->LAPIC to the EOI exit bitmap that is inferred from the
> redirection table.
>
> TMR is entirely handled within the local APIC, and no global copy
> of the IOAPIC-handled vectors is necessary anymore. This makes
> it simpler to introduce a userspace IOAPIC.
>
> Tested with ioapic.flat for now, planning to do more complete tests
> tomorrow. The most interesting test to do here is an assigned device
> that uses INTX, so I am CCing Alex Williamson for a heads up.
Tested Windows and Linux guests with assigned devices restricted to INTx
mode, no obvious regression. Thanks,
Alex
> Paolo Bonzini (2):
> KVM: x86: set TMR when the interrupt is accepted
> KVM: x86: store IOAPIC-handled vectors in each VCPU
>
> arch/x86/include/asm/kvm_host.h | 3 ++-
> arch/x86/kvm/ioapic.c | 27 ++++-----------------------
> arch/x86/kvm/ioapic.h | 11 +----------
> arch/x86/kvm/lapic.c | 29 ++++++++++++++++++-----------
> arch/x86/kvm/lapic.h | 1 -
> arch/x86/kvm/svm.c | 2 +-
> arch/x86/kvm/vmx.c | 3 ++-
> arch/x86/kvm/x86.c | 11 +++--------
> 8 files changed, 31 insertions(+), 56 deletions(-)
>
On Wed, Jul 29, 2015 at 03:37:34PM +0200, Paolo Bonzini wrote:
> Do not compute TMR in advance. Instead, set the TMR just before the interrupt
> is accepted into the IRR. This limits the coupling between IOAPIC and LAPIC.
>
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> arch/x86/kvm/ioapic.c | 9 ++-------
> arch/x86/kvm/ioapic.h | 3 +--
> arch/x86/kvm/lapic.c | 19 ++++++++++---------
> arch/x86/kvm/lapic.h | 1 -
> arch/x86/kvm/x86.c | 5 +----
> 5 files changed, 14 insertions(+), 23 deletions(-)
>
> diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
> index 856f79105bb5..eaf4ec38d980 100644
> --- a/arch/x86/kvm/ioapic.c
> +++ b/arch/x86/kvm/ioapic.c
> @@ -246,8 +246,7 @@ static void update_handled_vectors(struct kvm_ioapic *ioapic)
> smp_wmb();
> }
>
> -void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap,
> - u32 *tmr)
> +void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap)
> {
> struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic;
> union kvm_ioapic_redirect_entry *e;
> @@ -260,13 +259,9 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap,
> kvm_irq_has_notifier(ioapic->kvm, KVM_IRQCHIP_IOAPIC, index) ||
> index == RTC_GSI) {
> if (kvm_apic_match_dest(vcpu, NULL, 0,
> - e->fields.dest_id, e->fields.dest_mode)) {
> + e->fields.dest_id, e->fields.dest_mode))
> __set_bit(e->fields.vector,
> (unsigned long *)eoi_exit_bitmap);
> - if (e->fields.trig_mode == IOAPIC_LEVEL_TRIG)
> - __set_bit(e->fields.vector,
> - (unsigned long *)tmr);
> - }
> }
> }
> spin_unlock(&ioapic->lock);
> diff --git a/arch/x86/kvm/ioapic.h b/arch/x86/kvm/ioapic.h
> index ca0b0b4e6256..3dbd0e2aac4e 100644
> --- a/arch/x86/kvm/ioapic.h
> +++ b/arch/x86/kvm/ioapic.h
> @@ -120,7 +120,6 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> struct kvm_lapic_irq *irq, unsigned long *dest_map);
> int kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
> int kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
> -void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap,
> - u32 *tmr);
> +void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap);
>
> #endif
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 2a5ca97c263b..9be64c77d6db 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -551,15 +551,6 @@ static void pv_eoi_clr_pending(struct kvm_vcpu *vcpu)
> __clear_bit(KVM_APIC_PV_EOI_PENDING, &vcpu->arch.apic_attention);
> }
>
> -void kvm_apic_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr)
> -{
> - struct kvm_lapic *apic = vcpu->arch.apic;
> - int i;
> -
> - for (i = 0; i < 8; i++)
> - apic_set_reg(apic, APIC_TMR + 0x10 * i, tmr[i]);
> -}
> -
> static void apic_update_ppr(struct kvm_lapic *apic)
> {
> u32 tpr, isrv, ppr, old_ppr;
> @@ -781,6 +772,9 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
> case APIC_DM_LOWEST:
> vcpu->arch.apic_arb_prio++;
> case APIC_DM_FIXED:
> + if (unlikely(trig_mode && !level))
> + break;
> +
> /* FIXME add logic for vcpu on reset */
> if (unlikely(!apic_enabled(apic)))
> break;
> @@ -790,6 +784,13 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
> if (dest_map)
> __set_bit(vcpu->vcpu_id, dest_map);
>
> + if (apic_test_vector(vector, apic->regs + APIC_TMR) != !!trig_mode) {
> + if (trig_mode)
> + apic_set_vector(vector, apic->regs + APIC_TMR);
> + else
> + apic_clear_vector(vector, apic->regs + APIC_TMR);
> + }
> +
> if (kvm_x86_ops->deliver_posted_interrupt)
> kvm_x86_ops->deliver_posted_interrupt(vcpu, vector);
> else {
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index 764037991d26..eb46d6bcaa75 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -57,7 +57,6 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value);
> u64 kvm_lapic_get_base(struct kvm_vcpu *vcpu);
> void kvm_apic_set_version(struct kvm_vcpu *vcpu);
>
> -void kvm_apic_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr);
> void __kvm_apic_update_irr(u32 *pir, void *regs);
> void kvm_apic_update_irr(struct kvm_vcpu *vcpu, u32 *pir);
> int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 23e47a0b054b..48dc954542db 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6149,17 +6149,14 @@ static void process_smi(struct kvm_vcpu *vcpu)
> static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
> {
> u64 eoi_exit_bitmap[4];
> - u32 tmr[8];
>
> if (!kvm_apic_hw_enabled(vcpu->arch.apic))
> return;
>
> memset(eoi_exit_bitmap, 0, 32);
> - memset(tmr, 0, 32);
>
> - kvm_ioapic_scan_entry(vcpu, eoi_exit_bitmap, tmr);
> + kvm_ioapic_scan_entry(vcpu, eoi_exit_bitmap);
> kvm_x86_ops->load_eoi_exitmap(vcpu, eoi_exit_bitmap);
> - kvm_apic_update_tmr(vcpu, tmr);
> }
>
> static void kvm_vcpu_flush_tlb(struct kvm_vcpu *vcpu)
> --
> 1.8.3.1
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for writing this. This makes TMR handling far cleaner.
On Wed, Jul 29, 2015 at 03:37:35PM +0200, Paolo Bonzini wrote:
> We can reuse the algorithm that computes the EOI exit bitmap to figure
> out which vectors are handled by the IOAPIC. The only difference
> between the two is for edge-triggered interrupts other than IRQ8
> that have no notifiers active; however, the IOAPIC does not have to
> do anything special for these interrupts anyway.
>
> This again limits the interactions between the IOAPIC and the LAPIC,
> making it easier to move the former to userspace.
>
> Inspired by a patch from Steve Rutherford.
>
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> arch/x86/include/asm/kvm_host.h | 3 ++-
> arch/x86/kvm/ioapic.c | 18 ++----------------
> arch/x86/kvm/ioapic.h | 8 --------
> arch/x86/kvm/lapic.c | 10 ++++++++--
> arch/x86/kvm/svm.c | 2 +-
> arch/x86/kvm/vmx.c | 3 ++-
> arch/x86/kvm/x86.c | 8 +++-----
> 7 files changed, 18 insertions(+), 34 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 2f9e504f9f0c..d0e991ef6ef0 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -383,6 +383,7 @@ struct kvm_vcpu_arch {
> u64 efer;
> u64 apic_base;
> struct kvm_lapic *apic; /* kernel irqchip context */
> + u64 eoi_exit_bitmap[4];
> unsigned long apic_attention;
> int32_t apic_arb_prio;
> int mp_state;
> @@ -808,7 +809,7 @@ struct kvm_x86_ops {
> int (*vm_has_apicv)(struct kvm *kvm);
> void (*hwapic_irr_update)(struct kvm_vcpu *vcpu, int max_irr);
> void (*hwapic_isr_update)(struct kvm *kvm, int isr);
> - void (*load_eoi_exitmap)(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap);
> + void (*load_eoi_exitmap)(struct kvm_vcpu *vcpu);
> void (*set_virtual_x2apic_mode)(struct kvm_vcpu *vcpu, bool set);
> void (*set_apic_access_page_addr)(struct kvm_vcpu *vcpu, hpa_t hpa);
> void (*deliver_posted_interrupt)(struct kvm_vcpu *vcpu, int vector);
> diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
> index eaf4ec38d980..2dcda0f188ba 100644
> --- a/arch/x86/kvm/ioapic.c
> +++ b/arch/x86/kvm/ioapic.c
> @@ -233,19 +233,6 @@ static void kvm_ioapic_inject_all(struct kvm_ioapic *ioapic, unsigned long irr)
> }
>
>
> -static void update_handled_vectors(struct kvm_ioapic *ioapic)
> -{
> - DECLARE_BITMAP(handled_vectors, 256);
> - int i;
> -
> - memset(handled_vectors, 0, sizeof(handled_vectors));
> - for (i = 0; i < IOAPIC_NUM_PINS; ++i)
> - __set_bit(ioapic->redirtbl[i].fields.vector, handled_vectors);
> - memcpy(ioapic->handled_vectors, handled_vectors,
> - sizeof(handled_vectors));
> - smp_wmb();
> -}
> -
> void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap)
> {
> struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic;
> @@ -310,7 +297,6 @@ static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
> e->bits |= (u32) val;
> e->fields.remote_irr = 0;
> }
> - update_handled_vectors(ioapic);
> mask_after = e->fields.mask;
> if (mask_before != mask_after)
> kvm_fire_mask_notifiers(ioapic->kvm, KVM_IRQCHIP_IOAPIC, index, mask_after);
> @@ -594,7 +580,6 @@ static void kvm_ioapic_reset(struct kvm_ioapic *ioapic)
> ioapic->id = 0;
> memset(ioapic->irq_eoi, 0x00, IOAPIC_NUM_PINS);
> rtc_irq_eoi_tracking_reset(ioapic);
> - update_handled_vectors(ioapic);
> }
>
> static const struct kvm_io_device_ops ioapic_mmio_ops = {
> @@ -623,8 +608,10 @@ int kvm_ioapic_init(struct kvm *kvm)
> if (ret < 0) {
> kvm->arch.vioapic = NULL;
> kfree(ioapic);
> + return ret;
> }
>
> + kvm_vcpu_request_scan_ioapic(kvm);
> return ret;
> }
>
> @@ -661,7 +648,6 @@ int kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state)
> memcpy(ioapic, state, sizeof(struct kvm_ioapic_state));
> ioapic->irr = 0;
> ioapic->irr_delivered = 0;
> - update_handled_vectors(ioapic);
> kvm_vcpu_request_scan_ioapic(kvm);
> kvm_ioapic_inject_all(ioapic, state->irr);
> spin_unlock(&ioapic->lock);
> diff --git a/arch/x86/kvm/ioapic.h b/arch/x86/kvm/ioapic.h
> index 3dbd0e2aac4e..bf36d66a1951 100644
> --- a/arch/x86/kvm/ioapic.h
> +++ b/arch/x86/kvm/ioapic.h
> @@ -73,7 +73,6 @@ struct kvm_ioapic {
> struct kvm *kvm;
> void (*ack_notifier)(void *opaque, int irq);
> spinlock_t lock;
> - DECLARE_BITMAP(handled_vectors, 256);
> struct rtc_status rtc_status;
> struct delayed_work eoi_inject;
> u32 irq_eoi[IOAPIC_NUM_PINS];
> @@ -98,13 +97,6 @@ static inline struct kvm_ioapic *ioapic_irqchip(struct kvm *kvm)
> return kvm->arch.vioapic;
> }
>
> -static inline bool kvm_ioapic_handles_vector(struct kvm *kvm, int vector)
> -{
> - struct kvm_ioapic *ioapic = kvm->arch.vioapic;
> - smp_rmb();
> - return test_bit(vector, ioapic->handled_vectors);
> -}
> -
> void kvm_rtc_eoi_tracking_restore_one(struct kvm_vcpu *vcpu);
> bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
> int short_hand, unsigned int dest, int dest_mode);
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 9be64c77d6db..c158f4667aa3 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -869,14 +869,20 @@ int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcpu2)
> return vcpu1->arch.apic_arb_prio - vcpu2->arch.apic_arb_prio;
> }
>
> +static bool kvm_ioapic_handles_vector(struct kvm_lapic *apic, int vector)
> +{
> + return test_bit(vector, (ulong *)apic->vcpu->arch.eoi_exit_bitmap);
> +}
> +
> static void kvm_ioapic_send_eoi(struct kvm_lapic *apic, int vector)
> {
> - if (kvm_ioapic_handles_vector(apic->vcpu->kvm, vector)) {
> + if (kvm_ioapic_handles_vector(apic, vector)) {
> int trigger_mode;
> if (apic_test_vector(vector, apic->regs + APIC_TMR))
> trigger_mode = IOAPIC_LEVEL_TRIG;
> else
> trigger_mode = IOAPIC_EDGE_TRIG;
> +
Nit: Not sure if you meant to add this.
> kvm_ioapic_update_eoi(apic->vcpu, vector, trigger_mode);
> }
> }
> @@ -1922,7 +1928,7 @@ static void apic_sync_pv_eoi_to_guest(struct kvm_vcpu *vcpu,
> /* Cache not set: could be safe but we don't bother. */
> apic->highest_isr_cache == -1 ||
> /* Need EOI to update ioapic. */
> - kvm_ioapic_handles_vector(vcpu->kvm, apic->highest_isr_cache)) {
> + kvm_ioapic_handles_vector(apic, apic->highest_isr_cache)) {
> /*
> * PV EOI was disabled by apic_sync_pv_eoi_from_guest
> * so we need not do anything here.
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index d1a114d8d22b..676c9eb10174 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -3765,7 +3765,7 @@ static int svm_vm_has_apicv(struct kvm *kvm)
> return 0;
> }
>
> -static void svm_load_eoi_exitmap(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap)
> +static void svm_load_eoi_exitmap(struct kvm_vcpu *vcpu)
> {
> return;
> }
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 4014a82ae846..7323ea8fba88 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -8111,8 +8111,9 @@ static void vmx_hwapic_irr_update(struct kvm_vcpu *vcpu, int max_irr)
> }
> }
>
> -static void vmx_load_eoi_exitmap(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap)
> +static void vmx_load_eoi_exitmap(struct kvm_vcpu *vcpu)
> {
> + u64 *eoi_exit_bitmap = vcpu->arch.eoi_exit_bitmap;
> if (!vmx_vm_has_apicv(vcpu->kvm))
> return;
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 48dc954542db..4ad7334aa604 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6148,15 +6148,13 @@ static void process_smi(struct kvm_vcpu *vcpu)
>
> static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
> {
> - u64 eoi_exit_bitmap[4];
> -
> if (!kvm_apic_hw_enabled(vcpu->arch.apic))
> return;
>
> - memset(eoi_exit_bitmap, 0, 32);
> + memset(vcpu->arch.eoi_exit_bitmap, 0, 256 / 8);
>
> - kvm_ioapic_scan_entry(vcpu, eoi_exit_bitmap);
> - kvm_x86_ops->load_eoi_exitmap(vcpu, eoi_exit_bitmap);
> + kvm_ioapic_scan_entry(vcpu, vcpu->arch.eoi_exit_bitmap);
Nit: Does scan entry still need to carry around the pointer to the bitmap?
> + kvm_x86_ops->load_eoi_exitmap(vcpu);
> }
>
> static void kvm_vcpu_flush_tlb(struct kvm_vcpu *vcpu)
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Looks good. Just picked a few nits.
On 30/07/2015 05:55, Steve Rutherford wrote:
> > static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
> > {
> > - u64 eoi_exit_bitmap[4];
> > -
> > if (!kvm_apic_hw_enabled(vcpu->arch.apic))
> > return;
> >
> > - memset(eoi_exit_bitmap, 0, 32);
> > + memset(vcpu->arch.eoi_exit_bitmap, 0, 256 / 8);
> >
> > - kvm_ioapic_scan_entry(vcpu, eoi_exit_bitmap);
> > - kvm_x86_ops->load_eoi_exitmap(vcpu, eoi_exit_bitmap);
> > + kvm_ioapic_scan_entry(vcpu, vcpu->arch.eoi_exit_bitmap);
> Nit: Does scan entry still need to carry around the pointer to the bitmap?
IIRC I was copying what you did in your patch, but I also think it's
better this way. The IOAPIC has no business in the vcpu structure's
fields; it only needs it in order to pass vcpu to kvm_apic_match_dest.
Paolo
Paolo Bonzini wrote on 2015-07-29:
> Do not compute TMR in advance. Instead, set the TMR just before the
> interrupt is accepted into the IRR. This limits the coupling between
> IOAPIC and LAPIC.
>
Uh.., it back to original way which is wrong. You cannot modify the apic page(here is the TMR reg) directly when the corresponding VMCS may be used at same time.
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> arch/x86/kvm/ioapic.c | 9 ++-------
> arch/x86/kvm/ioapic.h | 3 +--
> arch/x86/kvm/lapic.c | 19 ++++++++++---------
> arch/x86/kvm/lapic.h | 1 -
> arch/x86/kvm/x86.c | 5 +----
> 5 files changed, 14 insertions(+), 23 deletions(-)
> diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
> index 856f79105bb5..eaf4ec38d980 100644
> --- a/arch/x86/kvm/ioapic.c
> +++ b/arch/x86/kvm/ioapic.c
> @@ -246,8 +246,7 @@ static void update_handled_vectors(struct kvm_ioapic
> *ioapic)
> smp_wmb();
> }
> -void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap,
> - u32 *tmr)
> +void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap)
> {
> struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic;
> union kvm_ioapic_redirect_entry *e;
> @@ -260,13 +259,9 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu,
> u64 *eoi_exit_bitmap,
> kvm_irq_has_notifier(ioapic->kvm, KVM_IRQCHIP_IOAPIC, index) ||
> index == RTC_GSI) { if (kvm_apic_match_dest(vcpu, NULL, 0,
> - e->fields.dest_id, e->fields.dest_mode)) {
> + e->fields.dest_id, e->fields.dest_mode))
> __set_bit(e->fields.vector,
> (unsigned long *)eoi_exit_bitmap);
> - if (e->fields.trig_mode == IOAPIC_LEVEL_TRIG)
> - __set_bit(e->fields.vector, - (unsigned long *)tmr); - }
> }
> }
> spin_unlock(&ioapic->lock);
> diff --git a/arch/x86/kvm/ioapic.h b/arch/x86/kvm/ioapic.h
> index ca0b0b4e6256..3dbd0e2aac4e 100644
> --- a/arch/x86/kvm/ioapic.h
> +++ b/arch/x86/kvm/ioapic.h
> @@ -120,7 +120,6 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct
> kvm_lapic *src,
> struct kvm_lapic_irq *irq, unsigned long *dest_map);
> int kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
> int kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
> -void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap,
> - u32 *tmr);
> +void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap);
>
> #endif
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 2a5ca97c263b..9be64c77d6db 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -551,15 +551,6 @@ static void pv_eoi_clr_pending(struct kvm_vcpu
> *vcpu)
> __clear_bit(KVM_APIC_PV_EOI_PENDING, &vcpu->arch.apic_attention);
> }
> -void kvm_apic_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr)
> -{
> - struct kvm_lapic *apic = vcpu->arch.apic;
> - int i;
> -
> - for (i = 0; i < 8; i++)
> - apic_set_reg(apic, APIC_TMR + 0x10 * i, tmr[i]);
> -}
> -
> static void apic_update_ppr(struct kvm_lapic *apic)
> {
> u32 tpr, isrv, ppr, old_ppr;
> @@ -781,6 +772,9 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int
> delivery_mode,
> case APIC_DM_LOWEST:
> vcpu->arch.apic_arb_prio++;
> case APIC_DM_FIXED:
> + if (unlikely(trig_mode && !level))
> + break;
> +
> /* FIXME add logic for vcpu on reset */
> if (unlikely(!apic_enabled(apic)))
> break;
> @@ -790,6 +784,13 @@ static int __apic_accept_irq(struct kvm_lapic *apic,
> int delivery_mode,
> if (dest_map)
> __set_bit(vcpu->vcpu_id, dest_map);
> + if (apic_test_vector(vector, apic->regs + APIC_TMR) != !!trig_mode) {
> + if (trig_mode)
> + apic_set_vector(vector, apic->regs + APIC_TMR);
> + else
> + apic_clear_vector(vector, apic->regs + APIC_TMR);
> + }
> +
> if (kvm_x86_ops->deliver_posted_interrupt)
> kvm_x86_ops->deliver_posted_interrupt(vcpu, vector);
> else {
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index 764037991d26..eb46d6bcaa75 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -57,7 +57,6 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64
> value);
> u64 kvm_lapic_get_base(struct kvm_vcpu *vcpu);
> void kvm_apic_set_version(struct kvm_vcpu *vcpu);
> -void kvm_apic_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr);
> void __kvm_apic_update_irr(u32 *pir, void *regs);
> void kvm_apic_update_irr(struct kvm_vcpu *vcpu, u32 *pir);
> int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 23e47a0b054b..48dc954542db 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6149,17 +6149,14 @@ static void process_smi(struct kvm_vcpu *vcpu)
> static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
> {
> u64 eoi_exit_bitmap[4];
> - u32 tmr[8];
>
> if (!kvm_apic_hw_enabled(vcpu->arch.apic))
> return;
>
> memset(eoi_exit_bitmap, 0, 32);
> - memset(tmr, 0, 32);
>
> - kvm_ioapic_scan_entry(vcpu, eoi_exit_bitmap, tmr);
> + kvm_ioapic_scan_entry(vcpu, eoi_exit_bitmap);
> kvm_x86_ops->load_eoi_exitmap(vcpu, eoi_exit_bitmap);
> - kvm_apic_update_tmr(vcpu, tmr); }
>
> static void kvm_vcpu_flush_tlb(struct kvm_vcpu *vcpu)
Best regards,
Yang
On Thu, Jul 30, 2015 at 11:26:28PM +0000, Zhang, Yang Z wrote:
> Paolo Bonzini wrote on 2015-07-29:
> > Do not compute TMR in advance. Instead, set the TMR just before the
> > interrupt is accepted into the IRR. This limits the coupling between
> > IOAPIC and LAPIC.
> >
>
> Uh.., it back to original way which is wrong. You cannot modify the apic page(here is the TMR reg) directly when the corresponding VMCS may be used at same time.
Oh... Yeah. That's a damn good point, given that the interrupt can be injected from another thread while one is in that guest vcpu.
Easiest time to update the TMR should be on guest entry through vcpu_scan_ioapic, as before.
The best way to go is probably to ditch the new per vcpu EOI exit bitmap, and just update/use the TMR. There's no reason to duplicate that data in the representation of the apic (I suspect that the duplication was inspired by my mistaken notion of the TMR). The IOAPIC exit check can use the TMR instead.
Based upon my reading of the SDM, the only reason that the eoi exit bitmaps are not the exact same as the TMR is that it is possible to have virtual-interrupt delivery enabled /without/ an apic access page (Note: V-ID => EOI exit bitmap enabled).
Yang, do you happen to know if that is the case?
[Note: Just looked back at the old code for updating the EOI exit bitmaps, which for some reason was configured to trigger EOI exits for all IOAPIC interrupts, not just level-triggered IOAPIC interrupts. Which is weird, and I believe totally unecessary.]
>
>
> > Signed-off-by: Paolo Bonzini <[email protected]>
> > ---
> > arch/x86/kvm/ioapic.c | 9 ++-------
> > arch/x86/kvm/ioapic.h | 3 +--
> > arch/x86/kvm/lapic.c | 19 ++++++++++---------
> > arch/x86/kvm/lapic.h | 1 -
> > arch/x86/kvm/x86.c | 5 +----
> > 5 files changed, 14 insertions(+), 23 deletions(-)
> > diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
> > index 856f79105bb5..eaf4ec38d980 100644
> > --- a/arch/x86/kvm/ioapic.c
> > +++ b/arch/x86/kvm/ioapic.c
> > @@ -246,8 +246,7 @@ static void update_handled_vectors(struct kvm_ioapic
> > *ioapic)
> > smp_wmb();
> > }
> > -void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap,
> > - u32 *tmr)
> > +void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap)
> > {
> > struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic;
> > union kvm_ioapic_redirect_entry *e;
> > @@ -260,13 +259,9 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu,
> > u64 *eoi_exit_bitmap,
> > kvm_irq_has_notifier(ioapic->kvm, KVM_IRQCHIP_IOAPIC, index) ||
> > index == RTC_GSI) { if (kvm_apic_match_dest(vcpu, NULL, 0,
> > - e->fields.dest_id, e->fields.dest_mode)) {
> > + e->fields.dest_id, e->fields.dest_mode))
> > __set_bit(e->fields.vector,
> > (unsigned long *)eoi_exit_bitmap);
> > - if (e->fields.trig_mode == IOAPIC_LEVEL_TRIG)
> > - __set_bit(e->fields.vector, - (unsigned long *)tmr); - }
> > }
> > }
> > spin_unlock(&ioapic->lock);
> > diff --git a/arch/x86/kvm/ioapic.h b/arch/x86/kvm/ioapic.h
> > index ca0b0b4e6256..3dbd0e2aac4e 100644
> > --- a/arch/x86/kvm/ioapic.h
> > +++ b/arch/x86/kvm/ioapic.h
> > @@ -120,7 +120,6 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct
> > kvm_lapic *src,
> > struct kvm_lapic_irq *irq, unsigned long *dest_map);
> > int kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
> > int kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
> > -void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap,
> > - u32 *tmr);
> > +void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap);
> >
> > #endif
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index 2a5ca97c263b..9be64c77d6db 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -551,15 +551,6 @@ static void pv_eoi_clr_pending(struct kvm_vcpu
> > *vcpu)
> > __clear_bit(KVM_APIC_PV_EOI_PENDING, &vcpu->arch.apic_attention);
> > }
> > -void kvm_apic_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr)
> > -{
> > - struct kvm_lapic *apic = vcpu->arch.apic;
> > - int i;
> > -
> > - for (i = 0; i < 8; i++)
> > - apic_set_reg(apic, APIC_TMR + 0x10 * i, tmr[i]);
> > -}
> > -
> > static void apic_update_ppr(struct kvm_lapic *apic)
> > {
> > u32 tpr, isrv, ppr, old_ppr;
> > @@ -781,6 +772,9 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int
> > delivery_mode,
> > case APIC_DM_LOWEST:
> > vcpu->arch.apic_arb_prio++;
> > case APIC_DM_FIXED:
> > + if (unlikely(trig_mode && !level))
> > + break;
> > +
> > /* FIXME add logic for vcpu on reset */
> > if (unlikely(!apic_enabled(apic)))
> > break;
> > @@ -790,6 +784,13 @@ static int __apic_accept_irq(struct kvm_lapic *apic,
> > int delivery_mode,
> > if (dest_map)
> > __set_bit(vcpu->vcpu_id, dest_map);
> > + if (apic_test_vector(vector, apic->regs + APIC_TMR) != !!trig_mode) {
> > + if (trig_mode)
> > + apic_set_vector(vector, apic->regs + APIC_TMR);
> > + else
> > + apic_clear_vector(vector, apic->regs + APIC_TMR);
> > + }
> > +
> > if (kvm_x86_ops->deliver_posted_interrupt)
> > kvm_x86_ops->deliver_posted_interrupt(vcpu, vector);
> > else {
> > diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> > index 764037991d26..eb46d6bcaa75 100644
> > --- a/arch/x86/kvm/lapic.h
> > +++ b/arch/x86/kvm/lapic.h
> > @@ -57,7 +57,6 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64
> > value);
> > u64 kvm_lapic_get_base(struct kvm_vcpu *vcpu);
> > void kvm_apic_set_version(struct kvm_vcpu *vcpu);
> > -void kvm_apic_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr);
> > void __kvm_apic_update_irr(u32 *pir, void *regs);
> > void kvm_apic_update_irr(struct kvm_vcpu *vcpu, u32 *pir);
> > int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 23e47a0b054b..48dc954542db 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -6149,17 +6149,14 @@ static void process_smi(struct kvm_vcpu *vcpu)
> > static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
> > {
> > u64 eoi_exit_bitmap[4];
> > - u32 tmr[8];
> >
> > if (!kvm_apic_hw_enabled(vcpu->arch.apic))
> > return;
> >
> > memset(eoi_exit_bitmap, 0, 32);
> > - memset(tmr, 0, 32);
> >
> > - kvm_ioapic_scan_entry(vcpu, eoi_exit_bitmap, tmr);
> > + kvm_ioapic_scan_entry(vcpu, eoi_exit_bitmap);
> > kvm_x86_ops->load_eoi_exitmap(vcpu, eoi_exit_bitmap);
> > - kvm_apic_update_tmr(vcpu, tmr); }
> >
> > static void kvm_vcpu_flush_tlb(struct kvm_vcpu *vcpu)
>
>
> Best regards,
> Yang
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On 31/07/2015 04:49, Steve Rutherford wrote:
> Oh... Yeah. That's a damn good point, given that the interrupt can be
> injected from another thread while one is in that guest vcpu.
>
> Easiest time to update the TMR should be on guest entry through
> vcpu_scan_ioapic, as before.
>
> The best way to go is probably to ditch the new per vcpu EOI exit
> bitmap, and just update/use the TMR. There's no reason to duplicate
> that data in the representation of the apic (I suspect that the
> duplication was inspired by my mistaken notion of the TMR). The
> IOAPIC exit check can use the TMR instead.
>
> Based upon my reading of the SDM, the only reason that the eoi exit
> bitmaps are not the exact same as the TMR is that it is possible to
> have virtual-interrupt delivery enabled /without/ an apic access page
> (Note: V-ID => EOI exit bitmap enabled).
>
> Yang, do you happen to know if that is the case?
>
> [Note: Just looked back at the old code for updating the EOI exit
> bitmaps, which for some reason was configured to trigger EOI exits
> for all IOAPIC interrupts, not just level-triggered IOAPIC
> interrupts. Which is weird, and I believe totally unecessary.]
The RTC interrupt needs to trigger an EOI exit with the in-kernel
IOAPIC, in order to detect coalesced interrupts. This is necessary to
avoid severe time drift in Windows guest.
Paolo
On 31/07/2015 01:26, Zhang, Yang Z wrote:
>>> Do not compute TMR in advance. Instead, set the TMR just before
>>> the interrupt is accepted into the IRR. This limits the coupling
>>> between IOAPIC and LAPIC.
>
> Uh.., it back to original way which is wrong. You cannot modify the
> apic page(here is the TMR reg) directly when the corresponding VMCS
> may be used at same time.
Where is this documented? The TMR is not part of the set of virtualized
APIC registers (TPR, PPR, EOI, ISR, IRR, ICR+ICR2; SDM 29.1.1).
Only virtualized APIC register reads use the virtual TMR registers (SDM
29.4.2 or 29.5), but these just read data from the corresponding field
in the virtual APIC page.
Paolo
Paolo Bonzini wrote on 2015-07-31:
>
>
> On 31/07/2015 01:26, Zhang, Yang Z wrote:
>>>> Do not compute TMR in advance. Instead, set the TMR just before
>>>> the interrupt is accepted into the IRR. This limits the coupling
>>>> between IOAPIC and LAPIC.
>>
>> Uh.., it back to original way which is wrong. You cannot modify the
>> apic page(here is the TMR reg) directly when the corresponding VMCS
>> may be used at same time.
>
> Where is this documented? The TMR is not part of the set of virtualized
> APIC registers (TPR, PPR, EOI, ISR, IRR, ICR+ICR2; SDM 29.1.1).
>
> Only virtualized APIC register reads use the virtual TMR registers (SDM
> 29.4.2 or 29.5), but these just read data from the corresponding field
> in the virtual APIC page.
It's not only for virtual apic page. All data structures that is used by VMCS(either directly inside in VMCS or referenced by pointer in a VMCS) need follow this rule:
24.11.4 Software Access to Related Structures
In addition to data in the VMCS region itself, VMX non-root operation can be controlled by data structures that are
referenced by pointers in a VMCS (for example, the I/O bitmaps). While the pointers to these data structures are
parts of the VMCS, the data structures themselves are not. They are not accessible using VMREAD and VMWRITE
but by ordinary memory writes.
Software should ensure that each such data structure is modified only when no logical processor with a current
VMCS that references it is in VMX non-root operation. Doing otherwise may lead to unpredictable behavior
(including behaviors identified in Section 24.11.1).
>
> Paolo
Best regards,
Yang
Paolo Bonzini wrote on 2015-07-31:
>
>
> On 31/07/2015 04:49, Steve Rutherford wrote:
>> Oh... Yeah. That's a damn good point, given that the interrupt can be
>> injected from another thread while one is in that guest vcpu.
>>
>> Easiest time to update the TMR should be on guest entry through
>> vcpu_scan_ioapic, as before.
>>
>> The best way to go is probably to ditch the new per vcpu EOI exit
>> bitmap, and just update/use the TMR. There's no reason to duplicate
>> that data in the representation of the apic (I suspect that the
>> duplication was inspired by my mistaken notion of the TMR). The
>> IOAPIC exit check can use the TMR instead.
>>
>> Based upon my reading of the SDM, the only reason that the eoi exit
>> bitmaps are not the exact same as the TMR is that it is possible to
>> have virtual-interrupt delivery enabled /without/ an apic access page
>> (Note: V-ID => EOI exit bitmap enabled).
>>
>> Yang, do you happen to know if that is the case?
>>
>> [Note: Just looked back at the old code for updating the EOI exit
>> bitmaps, which for some reason was configured to trigger EOI exits
>> for all IOAPIC interrupts, not just level-triggered IOAPIC
>> interrupts. Which is weird, and I believe totally unecessary.]
>
> The RTC interrupt needs to trigger an EOI exit with the in-kernel
> IOAPIC, in order to detect coalesced interrupts. This is necessary to
> avoid severe time drift in Windows guest.
Correct! EOI exit bitmap is not absolutely same with TMR. Some interrupts (especially timer interrupt) which is edge trigger but need a notification when it got injected into guest.
>
> Paolo
Best regards,
Yang
On 03/08/2015 04:37, Zhang, Yang Z wrote:
>> > Only virtualized APIC register reads use the virtual TMR registers (SDM
>> > 29.4.2 or 29.5), but these just read data from the corresponding field
>> > in the virtual APIC page.
>
> 24.11.4 Software Access to Related Structures
> In addition to data in the VMCS region itself, VMX non-root operation
> can be controlled by data structures that are referenced by pointers
> in a VMCS (for example, the I/O bitmaps). While the pointers to these
> data structures are parts of the VMCS, the data structures themselves
> are not. They are not accessible using VMREAD and VMWRITE but by
> ordinary memory writes.
I don't think the TMR fields of the virtual-APIC page are among the data
structures that _controls_ VMX non-root operations, because:
* it is not part of the virtualized APIC state is listed in 29.1.1
* read accesses from the APIC-access page simply return data from the
corresponding page offset on the virtual-APIC page using the memory
access type stored in IA32_VMX_BASIC_MSR. I think this explicitly says
that the effects of 24.11.1 (especially non-deterministic behavior after
a write) do not apply here.
In any case, the TMR behavior introduced by the APICv patches is
completely different from the hardware behavior, so it has to be fixed.
The alternative is to inject level-triggered interrupts synchronously,
without using posted interrupts.
I'll write some testcases to understand the functioning of TMR in the
virtual-APIC page, but the manual seems clear to me.
Paolo
Paolo Bonzini wrote on 2015-08-03:
>
>
> On 03/08/2015 04:37, Zhang, Yang Z wrote:
>>>> Only virtualized APIC register reads use the virtual TMR
>>>> registers (SDM
>>>> 29.4.2 or 29.5), but these just read data from the corresponding
>>>> field in the virtual APIC page.
>>
>> 24.11.4 Software Access to Related Structures In addition to data in
>> the VMCS region itself, VMX non-root operation can be controlled by
>> data structures that are referenced by pointers in a VMCS (for
>> example, the I/O bitmaps). While the pointers to these data
>> structures are parts of the VMCS, the data structures themselves are
>> not. They are not accessible using VMREAD and VMWRITE but by
>> ordinary memory writes.
>
> I don't think the TMR fields of the virtual-APIC page are among the
> data structures that _controls_ VMX non-root operations, because:
>
> * it is not part of the virtualized APIC state is listed in 29.1.1
>
> * read accesses from the APIC-access page simply return data from the
> corresponding page offset on the virtual-APIC page using the memory
> access type stored in IA32_VMX_BASIC_MSR. I think this explicitly
> says that the effects of 24.11.1 (especially non-deterministic
> behavior after a write) do not apply here.
>
> In any case, the TMR behavior introduced by the APICv patches is
> completely different from the hardware behavior, so it has to be fixed.
But any real problem with it?
> The alternative is to inject level-triggered interrupts
> synchronously, without using posted interrupts.
>
> I'll write some testcases to understand the functioning of TMR in the
> virtual-APIC page, but the manual seems clear to me.
Currently, no existing hardware will use TMR and will not cause any problem.(That's the reason why we leave it in Xen).But we don't know whether future hardware will use it or not(SDM always keeps changing :)).And per 24.11.4's description, the perfect solution is don't modify it.
btw, IIRC, only TMR doesn't follow the rule. All other VMCS accesses are issued in right VMCS context.
>
> Paolo
Best regards,
Yang
On 03/08/2015 12:23, Zhang, Yang Z wrote:
> > In any case, the TMR behavior introduced by the APICv patches is
> > completely different from the hardware behavior, so it has to be fixed.
>
> But any real problem with it?
It is a problem for split irqchip, where the EOI exit bitmap can be
inferred from the IOAPIC routes but the TMR cannot. The hardware
behavior on the other hand can be implemented purely within the LAPIC.
> > The alternative is to inject level-triggered interrupts
> > synchronously, without using posted interrupts.
> >
> > I'll write some testcases to understand the functioning of TMR in the
> > virtual-APIC page, but the manual seems clear to me.
>
> Currently, no existing hardware will use TMR and will not cause any
> problem.(That's the reason why we leave it in Xen).But we don't know
> whether future hardware will use it or not(SDM always keeps changing
> :)).
But that would be covered by a different execution control (for
backwards compatibility). We'll get there when such a feature is
introduced.
> And per 24.11.4's description, the perfect solution is don't
> modify it. btw, IIRC, only TMR doesn't follow the rule. All other
> VMCS accesses are issued in right VMCS context.
Yes, that's correct. It's just the TMR.
Paolo
Paolo Bonzini wrote on 2015-08-03:
>
>
> On 03/08/2015 12:23, Zhang, Yang Z wrote:
>>> In any case, the TMR behavior introduced by the APICv patches is
>>> completely different from the hardware behavior, so it has to be fixed.
>>
>> But any real problem with it?
>
> It is a problem for split irqchip, where the EOI exit bitmap can be
> inferred from the IOAPIC routes but the TMR cannot. The hardware
> behavior on the other hand can be implemented purely within the LAPIC.
So updating the TMR within LAPIC is the only solution to handle it?
>
>>> The alternative is to inject level-triggered interrupts
>>> synchronously, without using posted interrupts.
>>>
>>> I'll write some testcases to understand the functioning of TMR in
>>> the virtual-APIC page, but the manual seems clear to me.
>>
>> Currently, no existing hardware will use TMR and will not cause any
>> problem.(That's the reason why we leave it in Xen).But we don't know
>> whether future hardware will use it or not(SDM always keeps changing
>> :)).
>
> But that would be covered by a different execution control (for
> backwards compatibility). We'll get there when such a feature is introduced.
Yes, we can leave it in future. But one concern is that it may hard to handle it at that time if someone also develops feature which rely on it (like current patch to split irqchip).
>
>> And per 24.11.4's description, the perfect solution is don't modify
>> it. btw, IIRC, only TMR doesn't follow the rule. All other VMCS
>> accesses are issued in right VMCS context.
>
> Yes, that's correct. It's just the TMR.
>
> Paolo
Best regards,
Yang
On 04/08/2015 02:46, Zhang, Yang Z wrote:
> > It is a problem for split irqchip, where the EOI exit bitmap can be
> > inferred from the IOAPIC routes but the TMR cannot. The hardware
> > behavior on the other hand can be implemented purely within the LAPIC.
>
> So updating the TMR within LAPIC is the only solution to handle it?
It's the simplest and the one that makes most sense. Considering that
TMR is a pretty obscure feature, it's unlikely that it will be
accelerated in the future.
Paolo
Paolo Bonzini wrote on 2015-08-04:
>
>
> On 04/08/2015 02:46, Zhang, Yang Z wrote:
>>> It is a problem for split irqchip, where the EOI exit bitmap can be
>>> inferred from the IOAPIC routes but the TMR cannot. The hardware
>>> behavior on the other hand can be implemented purely within the LAPIC.
>>
>> So updating the TMR within LAPIC is the only solution to handle it?
>
> It's the simplest and the one that makes most sense. Considering that
> TMR is a pretty obscure feature, it's unlikely that it will be
> accelerated in the future.
You may be right. It is safe if no future hardware plans to use it. Let me check with our hardware team to see whether it will be used or not in future.
>
> Paolo
Best regards,
Yang
Zhang, Yang Z wrote on 2015-08-04:
> Paolo Bonzini wrote on 2015-08-04:
>>
>>
>> On 04/08/2015 02:46, Zhang, Yang Z wrote:
>>>> It is a problem for split irqchip, where the EOI exit bitmap can
>>>> be inferred from the IOAPIC routes but the TMR cannot. The
>>>> hardware behavior on the other hand can be implemented purely within the LAPIC.
>>>
>>> So updating the TMR within LAPIC is the only solution to handle it?
>>
>> It's the simplest and the one that makes most sense. Considering
>> that TMR is a pretty obscure feature, it's unlikely that it will be
>> accelerated in the future.
>
> You may be right. It is safe if no future hardware plans to use it.
> Let me check with our hardware team to see whether it will be used or not in future.
After checking with Jun, there is no guarantee that the guest running on another CPU will operate properly if hypervisor modify the vTMR from another CPU. So the hypervisor should not to do it.
>
>>
>> Paolo
>
>
> Best regards,
> Yang
>
Best regards,
Yang
On 13/08/2015 08:35, Zhang, Yang Z wrote:
>> You may be right. It is safe if no future hardware plans to use
>> it. Let me check with our hardware team to see whether it will be
>> used or not in future.
>
> After checking with Jun, there is no guarantee that the guest running
> on another CPU will operate properly if hypervisor modify the vTMR
> from another CPU. So the hypervisor should not to do it.
I guess I can cause a vmexit on level-triggered interrupts, it's not a
big deal, but no weasel words, please.
What's going to break, and where is it documented?
Paolo