[Qemu-devel] [PATCH RFC] vfio-pci: put device in INTx disable state in pre_reset

[Qemu-devel] [PATCH RFC] vfio-pci: put device in INTx disable state in pre_reset

[Cao jin]

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(-)

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);
 }
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH RFC] vfio-pci: put device in INTx disable state in pre_reset

[Alex Williamson]

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);
>  }
>  

Re: [Qemu-devel] [PATCH RFC] vfio-pci: put device in INTx disable state in pre_reset

[Cao jin]

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.
-- 
Yours Sincerely,

Cao jin
>
>> 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);
>>   }
>>
>
>
>
>