Re: [PATCH] KVM: Allow host IRQ sharing for assigned PCI 2.3 devices

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

On Tue, Jan 10, 2012 at 07:21:01PM +0100, Jan Kiszka wrote:
> > ATM writes to msi/msix mask bit have no effect for assigned
> > devices. For virtio, they are implemented by deassigning irqfd
> > which is a very slow operation (rcu write side).
> > 
> > Instead, When guest writes to mask, qemu can set/clear by calling
> > this ioctl.
> 
> Isn't that effort better invested in proper in-kernel mask emulation for
> MSI-X?

This gives us a working implementation fo free.  Whether MSIX mask
writes are worth accelerating in kernel I'm not 100% sure.  But IMO this
shows it is a more generic interface.

> > 
> >>>
> >>>> As long as the
> >>>> +guest masks the legacy INTx, the kernel will refrain from unmasking it at
> >>>> +hardware level and will not assert the guest's IRQ line. User space is still
> >>>> +responsible for applying this state to the assigned device's real config space.
> >>>
> >>> Can this be made more explicit? You mean writing into 1st
> >>> byte of PCI control, right?
> >>
> >> For sure, I can state this.
> >>
> >>>
> >>>> +To avoid that the kernel overwrites the state user space wants to set,
> >>>> +KVM_ASSIGN_SET_INTX_MASK has to be called prior to updating the config space.
> >>>
> >>> This looks like a strange requirement, could you explain how
> >>> this helps avoid races?
> >>
> >> By declaring the target state of the INTx bit first to the kernel,
> >> concurrent changes of the kernel while user space performs a
> >> read-modify-write will not lead to an old mask state being written.
> > 
> > I note you don't require KVM_ASSIGN_SET_INTX_MASK before read though.
> > Further, userspace might cache the control byte. If we require
> > it not to do it, we probably need to be explicit?
> 
> User space can do with the control byte what it wants - kernel can't
> help this anyway. I should just tell the kernel ahead of time what the
> next INTx mask state will be. That particularly avoids that the kernel
> sets the mask when user space wants it cleared. The other way around is
> actually unproblematic as we check KVM_ASSIGN_SET_INTX_MASK before
> delivering the IRQ to the guest.
> 
> > 
> >>> This also raises questions about
> >>> what should be done to write a bit unrelated to masking.
> >>
> >> Just write it, using the INTx state user space maintains. In the worst
> >> case, some masking done by the kernel in the meantime will be
> >> overwritten, leading to a single spurious but harmless IRQ. That event
> >> won't be delivered to the guest unless it is ready to receive it - as we
> >> updated the mask state prior to writing to the config space. The point
> >> is that the kernel mechanism has to deal with crazy user space clearing
> >> the mask for whatever reason again.
> > 
> > I guess the point is that we need to avoid is this:
> > 
> > kernel masks bit
> > read
> > kernel unmasks bit
> > write
> > 
> > I'm not sure I understand how the text above suggests
> > doing this in a race free manner.
> 
> User space must not write INTx as read from the hardware but according
> to its own view. Then the above is harmless.
> 
> > 
> > 
> > A simple way would be to ask userspace to always clear
> > this bit on writes. What do you think?
> 
> That or - sounds more consistent - writing the state that user space
> exposes to the guest anyway. That (in addition to the ordering
> requirement) should be clearly stated in the doc, I agree.
> 
> Jan

Yes, I agree it all works, just needs clear documentation.
In summary, userspace must ignore the value of the bit
it reads from device.

> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux