From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [PATCH] KVM: Allow host IRQ sharing for assigned PCI 2.3 devices Date: Tue, 10 Jan 2012 20:31:43 +0200 Message-ID: <20120110183143.GG17105@redhat.com> References: <4F0AF394.6000205@siemens.com> <20120110161748.GB17105@redhat.com> <4F0C758F.1060606@siemens.com> <20120110181014.GE17105@redhat.com> <4F0C818D.9@siemens.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Avi Kivity , Marcelo Tosatti , kvm , Alex Williamson , Jesse Barnes To: Jan Kiszka Return-path: Received: from mx1.redhat.com ([209.132.183.28]:4977 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756689Ab2AJS3h (ORCPT ); Tue, 10 Jan 2012 13:29:37 -0500 Content-Disposition: inline In-Reply-To: <4F0C818D.9@siemens.com> Sender: kvm-owner@vger.kernel.org List-ID: 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