From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:32969) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1buAjU-0007Wz-HK for qemu-devel@nongnu.org; Wed, 12 Oct 2016 00:01:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1buAjQ-0002GQ-Ex for qemu-devel@nongnu.org; Wed, 12 Oct 2016 00:01:31 -0400 Received: from [59.151.112.132] (port=47288 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1buAjO-0002AQ-QM for qemu-devel@nongnu.org; Wed, 12 Oct 2016 00:01:28 -0400 References: <1476090763-25295-1-git-send-email-caoj.fnst@cn.fujitsu.com> <20161011202549.0af84928@t450s.home> From: Cao jin Message-ID: <57FDB5DF.2030809@cn.fujitsu.com> Date: Wed, 12 Oct 2016 12:02:39 +0800 MIME-Version: 1.0 In-Reply-To: <20161011202549.0af84928@t450s.home> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH RFC] vfio-pci: put device in INTx disable state in pre_reset List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson Cc: qemu-devel@nongnu.org On 10/12/2016 10:25 AM, Alex Williamson wrote: > On Mon, 10 Oct 2016 17:12:43 +0800 > Cao jin wrote: > >> Current code cleared the PCI_COMMAND_INTX_DISABLE, which indicates >> device/function could asserts its INTx# signal. >> >> PCI local spec says: >> A value of 0 enables the assertion of its INTx# signal. >> A value of 1 disables the assertion of its INTx# signal. > > The PCI spec also says that this bit's state is zero after reset and > we're about to perform a reset, so we expect it to be zero after > reset. I believe this is the reason a set it this way. If we want to > set it, we should OR it in, not AND it. Are you actually seeing any > issues with the current behavior or was this a code inspection > discovery? Thanks, > > Alex > Just code inspection discovery. I understand that the bit is 0 after reset. In pre reset, from what I understand, we disabled interrupts first, so I *guess*this bit maybe should indicate the current state(device can't assert to trigger INTx). If this bit is set to 1 in pre-reset, then cleared to 0 in post-reset, it will be more logical to me. But just clear it to 0 in pre-set seems not quite wrong, because we eventually want it to be 0. And yes, I made a mistake, we should OR it if want to set it. -- Yours Sincerely, Cao jin > >> Signed-off-by: Cao jin >> --- >> I guess it is a mistake, clearing the bit to enable INTx violate >> the intention of vfio_disable_interrupts above. >> >> hw/vfio/pci.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index a5a620a..cce3024 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -1898,8 +1898,8 @@ static void vfio_pci_pre_reset(VFIOPCIDevice *vdev) >> * Also put INTx Disable in known state. >> */ >> cmd = vfio_pci_read_config(pdev, PCI_COMMAND, 2); >> - cmd &= ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER | >> - PCI_COMMAND_INTX_DISABLE); >> + cmd &= ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER) | >> + PCI_COMMAND_INTX_DISABLE; >> vfio_pci_write_config(pdev, PCI_COMMAND, cmd, 2); >> } >> > > > >