lkml.org 
[lkml]   [2012]   [Jul]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v5 3/4] kvm: Create kvm_clear_irq()
On Tue, Jul 17, 2012 at 10:17:03AM -0600, Alex Williamson wrote:
> > > > > > > > And current code looks buggy if yes we need to fix it somehow.
> > > > > > >
> > > > > > >
> > > > > > > Which to me seems to indicate this should be handled as a separate
> > > > > > > effort.
> > > > > >
> > > > > > A separate patchset, sure. But likely a prerequisite: we still need to
> > > > > > look at all the code. Let's not copy bugs, need to fix them.
> > > > >
> > > > > This looks tangential to me unless you can come up with an actual reason
> > > > > the above spinlock usage is incorrect or insufficient.
> > > >
> > > > You copy the same pattern that seems racy. So you double the
> > > > amount of code that woul need to be fixed.
> > >
> > >
> > > _Seems_ racy, or _is_ racy? Please identify the race.
> >
> > Look at this:
> >
> > static inline int 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);
> > else
> > clear_bit(irq_source_id, irq_state);
> >
> > return !!(*irq_state);
> > }
> >
> >
> > Now:
> > If other CPU changes some other bit after the atomic change,
> > it looks like !!(*irq_state) might return a stale value.
> >
> > CPU 0 clears bit 0. CPU 1 sets bit 1. CPU 1 sets level to 1.
> > If CPU 0 sees a stale value now it will return 0 here
> > and interrupt will get cleared.
> >
> >
> > Maybe this is not a problem. But in that case IMO it needs
> > a comment explaining why and why it's not a problem in
> > your code.
>
> So you want to close the door on anything that uses kvm_set_irq until
> this gets fixed... that's insane.

What does kvm_set_irq use have to do with it? You posted this patch:

+static int kvm_clear_pic_irq(struct kvm_kernel_irq_routing_entry *e,
+ struct kvm *kvm, int irq_source_id)
+{
+#ifdef CONFIG_X86
+ struct kvm_pic *pic = pic_irqchip(kvm);
+ int level =
kvm_clear_irq_line_state(&pic->irq_states[e->irqchip.pin],
+ irq_source_id);
+ if (level)
+ kvm_pic_set_irq(pic, e->irqchip.pin,
+ !!pic->irq_states[e->irqchip.pin]);
+ return level;
+#else
+ return -1;
+#endif
+}
+

it seems racy in the same way.

--
MST


\
 
 \ /
  Last update: 2012-07-17 19:01    [W:0.439 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site