| Message ID | CACZJ9cV2gv+A_2wCXowzi9M-HrySeBxNLKfK+bXRLffwR94=fA@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM:x86:Fix an interrupt injection logic error during PIC interrupt simulation | expand |
On Thu, Jul 25, 2024 at 4:00 PM Liam Ni <zhiguangni01@gmail.com> wrote: > > The input parameter level to the pic_irq_request function indicates > whether there are interrupts to be injected, > a level value of 1 indicates that there are interrupts to be injected, > and a level value of 0 indicates that there are no interrupts to be injected. > And the value of level will be assigned to s->output, > so we should set s->wakeup_needed to true when s->output is true. > > Signed-off-by: Liam Ni <zhiguangni01@gmail.com> > --- > arch/x86/kvm/i8259.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c > index 8dec646e764b..ec9d6ee7d33d 100644 > --- a/arch/x86/kvm/i8259.c > +++ b/arch/x86/kvm/i8259.c > @@ -567,7 +567,7 @@ static void pic_irq_request(struct kvm *kvm, int level) > { > struct kvm_pic *s = kvm->arch.vpic; > > - if (!s->output) > + if (s->output) This is the old value of s->output. wakeup is needed if you have a 0->1 transition, so what you're looking for is either if (level) s->wakeup_needed = true; or if (!s->output && level) s->wakeup_needed = true; but your version is incorrect because it would look for a 1->1 transition instead. Thanks, Paolo
On Thu, 25 Jul 2024 at 22:17, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On Thu, Jul 25, 2024 at 4:00 PM Liam Ni <zhiguangni01@gmail.com> wrote: > > > > The input parameter level to the pic_irq_request function indicates > > whether there are interrupts to be injected, > > a level value of 1 indicates that there are interrupts to be injected, > > and a level value of 0 indicates that there are no interrupts to be injected. > > And the value of level will be assigned to s->output, > > so we should set s->wakeup_needed to true when s->output is true. > > > > Signed-off-by: Liam Ni <zhiguangni01@gmail.com> > > --- > > arch/x86/kvm/i8259.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c > > index 8dec646e764b..ec9d6ee7d33d 100644 > > --- a/arch/x86/kvm/i8259.c > > +++ b/arch/x86/kvm/i8259.c > > @@ -567,7 +567,7 @@ static void pic_irq_request(struct kvm *kvm, int level) > > { > > struct kvm_pic *s = kvm->arch.vpic; > > > > - if (!s->output) > > + if (s->output) > > This is the old value of s->output. wakeup is needed if you have a > 0->1 transition, so what you're looking for is either I would like to know the reason why we monitor the 0->1 transformations? > > if (level) > s->wakeup_needed = true; This solution seems more appropriate with level=true, indicating that there is a pending interrupt in the PIC Thanks Liam Ni > > or > > if (!s->output && level) > s->wakeup_needed = true; > > but your version is incorrect because it would look for a 1->1 > transition instead. > > Thanks, > > Paolo >
diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c index 8dec646e764b..ec9d6ee7d33d 100644 --- a/arch/x86/kvm/i8259.c +++ b/arch/x86/kvm/i8259.c @@ -567,7 +567,7 @@ static void pic_irq_request(struct kvm *kvm, int level) { struct kvm_pic *s = kvm->arch.vpic; - if (!s->output) + if (s->output) s->wakeup_needed = true; s->output = level; }
The input parameter level to the pic_irq_request function indicates whether there are interrupts to be injected, a level value of 1 indicates that there are interrupts to be injected, and a level value of 0 indicates that there are no interrupts to be injected. And the value of level will be assigned to s->output, so we should set s->wakeup_needed to true when s->output is true. Signed-off-by: Liam Ni <zhiguangni01@gmail.com> --- arch/x86/kvm/i8259.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)