Re: [PATCH v2] KVM: x86: Fix poll command

[alexjlzheng]

On Mon, 17 Apr 2023, Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> On Sat, Apr 15, 2023, alexjlzheng@xxxxxxxxx wrote:
> > On Fri, 14 Apr 2023, Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > > On Thu, Apr 13, 2023, alexjlzheng@xxxxxxxxx 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