| Message ID | 1476090763-25295-1-git-send-email-caoj.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 10 Oct 2016 17:12:43 +0800 Cao jin <caoj.fnst@cn.fujitsu.com> 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 > Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> > --- > 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); > } >
On 10/12/2016 10:25 AM, Alex Williamson wrote: > On Mon, 10 Oct 2016 17:12:43 +0800 > Cao jin <caoj.fnst@cn.fujitsu.com> 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.
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); }
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. Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> --- 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(-)