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 <address@hidden> 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 <address@hidden>
> > ---
> > 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);
> >   }