From: Jinliang Zheng <alexjlzheng@tencent.com>

According to the hardware manual, when the Poll command is issued, the
byte returned by the I/O read is 1 in Bit 7 when there is an interrupt,
and the highest priority binary code in Bits 2:0. The current pic
simulation code is not implemented strictly according to the above
expression.

Fix the implementation of pic_poll_read():
1. Set Bit 7 when there is an interrupt
2. Return 0 when there is no interrupt

Signed-off-by: Jinliang Zheng <alexjlzheng@tencent.com>
---
Changes since Version V2:
- Keep the logic of pic_poll_read(), only fix the return value
---
 arch/x86/kvm/i8259.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index 4756bcb5724f..6627f8a52f23 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -411,8 +411,9 @@ static u32 pic_poll_read(struct kvm_kpic_state *s, u32 addr1)
 		pic_clear_isr(s, ret);
 		if (addr1 >> 7 || ret != 2)
 			pic_update_irq(s->pics_state);
+		ret |= 0x80;
 	} else {
-		ret = 0x07;
+		ret = 0x00;
 		pic_update_irq(s->pics_state);
 	}
 
-- 
2.31.1

On Thu, Apr 13, 2023, alexjlzheng@gmail.com wrote:
> From: Jinliang Zheng <alexjlzheng@tencent.com>
> 
> According to the hardware manual, when the Poll command is issued, the

Please add "8259", i.e. "According to the 8259 hardware manual".

> byte returned by the I/O read is 1 in Bit 7 when there is an interrupt,
> and the highest priority binary code in Bits 2:0. The current pic
> simulation code is not implemented strictly according to the above
> expression.
> 
> Fix the implementation of pic_poll_read():
> 1. Set Bit 7 when there is an interrupt
> 2. Return 0 when there is no interrupt

I don't think #2 is justified.  The spec says:

  The interrupt requests are ordered in priority from 0 through 7 (0 highest).

I.e. the current code enumerates the _lowest_ priority when there is no interrupt,
which seems more correct than reporting the highest priority possible.

On Fri, 14 Apr 2023, Sean Christopherson <seanjc@google.com> wrote:
> On Thu, Apr 13, 2023, alexjlzheng@gmail.com wrote:
> > From: Jinliang Zheng <alexjlzheng@tencent.com>
> > 
> > According to the hardware manual, when the Poll command is issued, the
> 
> Please add "8259", i.e. "According to the 8259 hardware manual".

Ok, I will pay attention next time.

> 
> > byte returned by the I/O read is 1 in Bit 7 when there is an interrupt,
> > and the highest priority binary code in Bits 2:0. The current pic
> > simulation code is not implemented strictly according to the above
> > expression.
> > 
> > Fix the implementation of pic_poll_read():
> > 1. Set Bit 7 when there is an interrupt
> > 2. Return 0 when there is no interrupt
> 
> I don't think #2 is justified.  The spec says:
> 
>   The interrupt requests are ordered in priority from 0 through 7 (0 highest).

This is only true when don't use rotation for priority or just reset the 8259a.
It's prossible to change priorities, i.e. Specific Rotation Mode or Automatic
Rotation Mode.

> 
> I.e. the current code enumerates the _lowest_ priority when there is no interrupt,
> which seems more correct than reporting the highest priority possible.

The practice and interpretation of returning to the lowest priority interrupt
when there are no active interrupts in the PIC doesn't seem reasonable, as far as I
understand. For #2, in my opinion, the correct interpretation of the current code
may be that a spurious interrupt is returned(IRQ 7 is used for that according to
the 8259 hardware manual).

For #2, the main purpose of returning 0 is to set Bit 7 of the return value to 0
to indicate that there is no interrupt.

Thank you very much.
Jinliang Zheng

On Sat, Apr 15, 2023, alexjlzheng@gmail.com wrote:
> On Fri, 14 Apr 2023, Sean Christopherson <seanjc@google.com> wrote:
> > On Thu, Apr 13, 2023, alexjlzheng@gmail.com wrote:
> > > Fix the implementation of pic_poll_read():
> > > 1. Set Bit 7 when there is an interrupt
> > > 2. Return 0 when there is no interrupt
> > 
> > I don't think #2 is justified.  The spec says:
> > 
> >   The interrupt requests are ordered in priority from 0 through 7 (0 highest).
> 
> This is only true when don't use rotation for priority or just reset the 8259a.
> It's prossible to change priorities, i.e. Specific Rotation Mode or Automatic
> Rotation Mode.
> 
> > 
> > I.e. the current code enumerates the _lowest_ priority when there is no interrupt,
> > which seems more correct than reporting the highest priority possible.
> 
> The practice and interpretation of returning to the lowest priority interrupt
> when there are no active interrupts in the PIC doesn't seem reasonable, as far as I
> understand. For #2, in my opinion, the correct interpretation of the current code
> may be that a spurious interrupt is returned(IRQ 7 is used for that according to
> the 8259 hardware manual).
> 
> For #2, the main purpose of returning 0 is to set Bit 7 of the return value to 0
> to indicate that there is no interrupt.

Is there an actual real world chunk of guest code that is broken by KVM's behavior
for the "no interrupt" case?  Because if not, my strong preference is to leave the
code as-is.

I have no objection to setting bit 7 when there is an interrupt, as that behavior
is explicitly called out and KVM is clearly in the wrong.

But for the "no interrupt" case, there are a lot of "mays" and "seems" in both of
our responses, i.e. it's not obvious that the current code is outright wrong, nor
that it is correct either.  Given the lack of clarity, unless there's a guest that's
actually broken by KVM's current implementation, I see no benefit to changing KVM's
behavior, only the potential for breaking existing KVM guests.

And if the "no interrupt" case really does need to be fixed, please split it to
a separate patch.

On Mon, 17 Apr 2023, Sean Christopherson <seanjc@google.com> wrote:
> On Sat, Apr 15, 2023, alexjlzheng@gmail.com wrote:
> > On Fri, 14 Apr 2023, Sean Christopherson <seanjc@google.com> wrote:
> > > On Thu, Apr 13, 2023, alexjlzheng@gmail.com wrote:
> > > > Fix the implementation of pic_poll_read():
> > > > 1. Set Bit 7 when there is an interrupt
> > > > 2. Return 0 when there is no interrupt
> > > 
> > > I don't think #2 is justified.  The spec says:
> > > 
> > >   The interrupt requests are ordered in priority from 0 through 7 (0 highest).
> > 
> > This is only true when don't use rotation for priority or just reset the 8259a.
> > It's prossible to change priorities, i.e. Specific Rotation Mode or Automatic
> > Rotation Mode.
> > 
> > > 
> > > I.e. the current code enumerates the _lowest_ priority when there is no interrupt,
> > > which seems more correct than reporting the highest priority possible.
> > 
> > The practice and interpretation of returning to the lowest priority interrupt
> > when there are no active interrupts in the PIC doesn't seem reasonable, as far as I
> > understand. For #2, in my opinion, the correct interpretation of the current code
> > may be that a spurious interrupt is returned(IRQ 7 is used for that according to
> > the 8259 hardware manual).
> > 
> > For #2, the main purpose of returning 0 is to set Bit 7 of the return value to 0
> > to indicate that there is no interrupt.
> 
> Is there an actual real world chunk of guest code that is broken by KVM's behavior
> for the "no interrupt" case?  Because if not, my strong preference is to leave the
> code as-is.
> 
> I have no objection to setting bit 7 when there is an interrupt, as that behavior
> is explicitly called out and KVM is clearly in the wrong.

Very happy that we have reached a consensus on #1.

> 
> But for the "no interrupt" case, there are a lot of "mays" and "seems" in both of
> our responses, i.e. it's not obvious that the current code is outright wrong, nor
> that it is correct either.  Given the lack of clarity, unless there's a guest that's
> actually broken by KVM's current implementation, I see no benefit to changing KVM's
> behavior, only the potential for breaking existing KVM guests.

For #2, neither returning 0 nor 7 will affect the behavior of interrupt handling in
the guest os. Because their Bit 7 are all 0, the guest os will interpret them as no
interrupt. However, keeping it as it is (return 7) will reduce the readability of
the pic_poll_read() code. When developers compare the code in kvm_pic_read_irq(),
they may think that what is returned in #2 is a spurious interrupt, but this is not.

> 
> And if the "no interrupt" case really does need to be fixed, please split it to
> a separate patch.

For the reasons above, I suggest fix #2. I will split it to a separate patch.

Thank you.
Jinliang Zheng