On Thu, 2012-07-19 at 01:11 +0300, Michael S. Tsirkin wrote:
> This creates a way to detect when kvm_set_irq(...,0) was run
> twice with the same source id by returning 0 in this case.
>
> Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
> ---
>
> This is on top of my bugfix patch. Uncompiled and untested. Alex, I
> think something like this patch will make it possible for you to simply
> do
> if (kvm_set_irq(...., 0))
> eventfd_signal()
>
> without need for further tricks.
>
> arch/x86/include/asm/kvm_host.h | 9 ++++-----
> arch/x86/kvm/i8259.c | 11 +++++++----
> virt/kvm/ioapic.c | 17 ++++++++++-------
> 3 files changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index fe6e9f1..6f5ed0a 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -802,16 +802,15 @@ int kvm_read_guest_page_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
> void kvm_propagate_fault(struct kvm_vcpu *vcpu, struct x86_exception *fault);
> bool kvm_require_cpl(struct kvm_vcpu *vcpu, int required_cpl);
>
> -static inline int kvm_irq_line_state(unsigned long *irq_state,
> +/* Tweak line state. Return true if state was changed. */
> +static inline bool kvm_irq_line_state(unsigned long *irq_state,
> int irq_source_id, int level)
> {
> /* Logical OR for level trig interrupt */
> if (level)
> - __set_bit(irq_source_id, irq_state);
> + return !__test_and_set_bit(irq_source_id, irq_state);
> else
> - __clear_bit(irq_source_id, irq_state);
> -
> - return !!(*irq_state);
> + return __test_and_clear_bit(irq_source_id, irq_state);
> }
>
> int kvm_pic_set_irq(struct kvm_pic *pic, int irq, int irq_source_id, int level);
> diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> index 95b504b..44e3635 100644
> --- a/arch/x86/kvm/i8259.c
> +++ b/arch/x86/kvm/i8259.c
> @@ -190,12 +190,15 @@ void kvm_pic_update_irq(struct kvm_pic *s)
>
> int kvm_pic_set_irq(struct kvm_pic *s, int irq, int src_id, int l)
> {
> - int ret = -1;
> - int level;
> + int ret, level;
>
> pic_lock(s);
> - if (irq >= 0 && irq < PIC_NUM_PINS) {
> - level = kvm_irq_line_state(&s->irq_states[irq], src_id, l);
> + if (irq < 0 || irq >= PIC_NUM_PINS) {
Why test this under lock? Return error before lock, then it becomes
int ret = 0;
if (irq < 0 || irq >= PIC_NUM_PINS)
return -1;
pic_lock(s);
if (kvm_irq_line_state(&s->irq_states[irq], irq_source_id, level) {
level = !!s->irq_states[irq];
...
}
pic_unlock(s);
return ret;
> + ret = -1;
> + } else if (!kvm_irq_line_state(&s->irq_states[irq], src_id, l)) {
> + ret = 0;
> + } else {
> + level = !!s->irq_states[irq];
> ret = pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);
> pic_update_irq(s);
> trace_kvm_pic_set_irq(irq >> 3, irq & 7, s->pics[irq >> 3].elcr,
> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> index 268b332..edc9445 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -196,18 +196,21 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int src_id, int l)
> u32 old_irr;
> u32 mask = 1 << irq;
> union kvm_ioapic_redirect_entry entry;
> - int ret = 1;
> - int level;
> + int ret, level;
>
> spin_lock(&ioapic->lock);
> old_irr = ioapic->irr;
> - if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
> + if (irq < 0 || irq >= IOAPIC_NUM_PINS) {
Same here
> + ret = -1;
> + } else if (!kvm_irq_line_state(&ioapic->irq_states[irq], src_id, l)) {
> + ret = 0;
> + } else {
> entry = ioapic->redirtbl[irq];
> - level = kvm_irq_line_state(&ioapic->irq_states[irq], src_id, l);
> - level ^= entry.fields.polarity;
> - if (!level)
> + level = (!!ioapic->irq_states[irq]) ^ entry.fields.polarity;
> + if (!level) {
> ioapic->irr &= ~mask;
> - else {
> + ret = 1;
> + } else {
> int edge = (entry.fields.trig_mode == IOAPIC_EDGE_TRIG);
> ioapic->irr |= mask;
> if ((edge && old_irr != ioapic->irr) ||
--
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
