Re: [PATCH 2/2][RFC] KVM: Emulate MSI-X table and PBA in kernel

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Dec 31, 2010 at 11:05:28AM +0800, Sheng Yang wrote:
> On Thursday 30 December 2010 18:32:56 Michael S. Tsirkin wrote:
> > On Thu, Dec 30, 2010 at 11:30:12AM +0200, Avi Kivity wrote:
> > > On 12/30/2010 09:47 AM, Michael S. Tsirkin wrote:
> > > >I am not really suggesting this. What I say is PBA is unimplemented
> > > >let us not commit to an interface yet.
> > > 
> > > What happens to a guest that tries to use PBA?
> > > It's a mandatory part of MSI-X, no?
> > 
> > Yes. Unfortunately the pending bit is in fact a communication channel
> > used for function specific purposes when mask bit is set,
> > and 0 when unset. The spec even seems to *require* this use:
> > 
> > I refer to this:
> > 
> > 	For MSI and MSI-X, while a vector is masked, the function is prohibited
> > 	from sending the associated message, and the function must set the
> > 	associated Pending bit whenever the function would otherwise send the
> > 	message. When software unmasks a vector whose associated Pending bit is
> > 	set, the function must schedule sending the associated message, and
> > 	clear the Pending bit as soon as the message has been sent. Note that
> > 	clearing the MSI-X Function Mask bit may result in many messages needing
> > 	to be sent.
> > 
> > 
> > 	If a masked vector has its Pending bit set, and the associated
> > 	underlying interrupt events are somehow satisfied (usually by software
> > 	though the exact manner is function-specific), the function must clear
> > 	the Pending bit, to avoid sending a spurious interrupt message later
> > 	when software unmasks the vector. However, if a subsequent interrupt
> > 	event occurs while the vector is still masked, the function must again
> > 	set the Pending bit.
> > 
> > 
> > 	Software is permitted to mask one or more vectors indefinitely, and
> > 	service their associated interrupt events strictly based on polling
> > 	their Pending bits. A function must set and clear its Pending bits as
> > 	necessary to support this “pure polling” mode of operation.
> > 
> > For assigned devices, supporting this would require
> > that the mask bits on the device are set if the mask bit in
> > guest is set (otherwise pending bits are disabled).
> 
> For assigned device, I think the result we should return is IRQ_PENDING bit of 
> related IRQ. Seems it perfectly fits the meaning of pending bit definition here - 
> set when masked, and if we didn't clean it, one interrupt would be retriggered 
> after unmask.

Well, it doesn't seem to fit this part of the definition 
> > 	If a masked vector has its Pending bit set, and the associated
> > 	underlying interrupt events are somehow satisfied (usually by software
> > 	though the exact manner is function-specific), the function must clear
> > 	the Pending bit, to avoid sending a spurious interrupt message later
> > 	when software unmasks the vector. However, if a subsequent interrupt
> > 	event occurs while the vector is still masked, the function must again
> > 	set the Pending bit.
> > 
> > 	Software is permitted to mask one or more vectors indefinitely, and
> > 	service their associated interrupt events strictly based on polling
> > 	their Pending bits. A function must set and clear its Pending bits as
> > 	necessary to support this “pure polling” mode of operation.
looking at IRQ_PENDING will make the pending bit *never* clear while
the vector is masked.


> But it's a internal flag, and use it would lead to some core 
> change(more need to be considered if we want to operate the flag bit outside core 
> kernel part). 
> > 
> > Existing code does not support PBA in assigned devices, so at least it's
> > not a regression there, and the virtio spec says nothing about this so
> > we should be fine.
> 
> I agree. At least it's not a regression. And in fact we haven't seen any device 
> driver use this. I've checked Linux kernel code, found no one used PCI_MSIX_PBA or 
> msix_pba_offset_reg().
> 
> I guess it's fine to get MSI-X mask part in first, then deal with PBA part if 
> necessary - though we haven't seen any driver use it so far. It won't be worse 
> with this patch anyway...
> 
> --
> regards
> Yang, Sheng