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
