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

[Cao jin <caoj.fnst@cn.fujitsu.com>]

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