On Wed, Jul 18, 2012 at 04:40:38PM -0600, Alex Williamson wrote:
> 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 <m...@redhat.com>
> > ---
> >
> > 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) ||
>
Sure, go ahead. I just sent this to show how to address the locking with
EOI patches, don't intent to pursue it myself.
--
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
