[Qemu-devel] [PATCH] virtio-pci: Clear IRQ at reset

[Qemu-devel] [PATCH] virtio-pci: Clear IRQ at reset

[Fam Zheng]

Currently we could leave PCI IRQ asserted even after reset, it is safer
to clear it.

In the case that a buggy driver has disabled MSI-X unintentially, we may
have already injected IRQ in previous virtio_pci_notify, which will not
be cleared by guest because it doesn't expect it (i.e. no irq handler).
However the driver may eventually notice the unresponsiveness and reset
the device, at that point, clearing the irq is meaningful.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/virtio/virtio-pci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index e7baf7b..2600f1e 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -988,6 +988,7 @@ static void virtio_pci_reset(DeviceState *qdev)
     virtio_pci_stop_ioeventfd(proxy);
     virtio_bus_reset(bus);
     msix_unuse_all_vectors(&proxy->pci_dev);
+    pci_irq_deassert(&proxy->pci_dev);
 }
 
 static Property virtio_pci_properties[] = {
-- 
1.9.3

Re: [Qemu-devel] [PATCH] virtio-pci: Clear IRQ at reset

[Michael S. Tsirkin]

On Thu, Mar 12, 2015 at 02:40:55PM +0800, Fam Zheng wrote:
> Currently we could leave PCI IRQ asserted even after reset, it is safer
> to clear it.
> 
> In the case that a buggy driver has disabled MSI-X unintentially, we may
> have already injected IRQ in previous virtio_pci_notify, which will not
> be cleared by guest because it doesn't expect it (i.e. no irq handler).
> However the driver may eventually notice the unresponsiveness and reset
> the device, at that point, clearing the irq is meaningful.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>

I don't get it. interrupts are de-asserted
in pci core:

static void pci_do_device_reset(PCIDevice *dev)
{   
    int r;
    
    pci_device_deassert_intx(dev);

...
}

why isn't this sufficient?

> ---
>  hw/virtio/virtio-pci.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index e7baf7b..2600f1e 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -988,6 +988,7 @@ static void virtio_pci_reset(DeviceState *qdev)
>      virtio_pci_stop_ioeventfd(proxy);
>      virtio_bus_reset(bus);
>      msix_unuse_all_vectors(&proxy->pci_dev);
> +    pci_irq_deassert(&proxy->pci_dev);
>  }
>  
>  static Property virtio_pci_properties[] = {
> -- 
> 1.9.3

Re: [Qemu-devel] [PATCH] virtio-pci: Clear IRQ at reset

[Fam Zheng]

On Thu, 03/12 08:22, Michael S. Tsirkin wrote:
> On Thu, Mar 12, 2015 at 02:40:55PM +0800, Fam Zheng wrote:
> > Currently we could leave PCI IRQ asserted even after reset, it is safer
> > to clear it.
> > 
> > In the case that a buggy driver has disabled MSI-X unintentially, we may
> > have already injected IRQ in previous virtio_pci_notify, which will not
> > be cleared by guest because it doesn't expect it (i.e. no irq handler).
> > However the driver may eventually notice the unresponsiveness and reset
> > the device, at that point, clearing the irq is meaningful.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> 
> I don't get it. interrupts are de-asserted
> in pci core:
> 
> static void pci_do_device_reset(PCIDevice *dev)
> {   
>     int r;
>     
>     pci_device_deassert_intx(dev);
> 
> ...
> }
> 
> why isn't this sufficient?

Becuase it's not called by virtio_pci_reset.

Fam

Re: [Qemu-devel] [PATCH] virtio-pci: Clear IRQ at reset

[Michael S. Tsirkin]

On Thu, Mar 12, 2015 at 02:40:55PM +0800, Fam Zheng wrote:
> Currently we could leave PCI IRQ asserted even after reset, it is safer
> to clear it.
> 
> In the case that a buggy driver has disabled MSI-X unintentially, we may
> have already injected IRQ in previous virtio_pci_notify, which will not
> be cleared by guest because it doesn't expect it (i.e. no irq handler).
> However the driver may eventually notice the unresponsiveness and reset
> the device, at that point, clearing the irq is meaningful.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>

I don't think we care about buggy drivers being able to recover, but the
same would apply to e.g. kdump when using virtio blk/scsi after a crash,
correct?

> ---
>  hw/virtio/virtio-pci.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index e7baf7b..2600f1e 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -988,6 +988,7 @@ static void virtio_pci_reset(DeviceState *qdev)
>      virtio_pci_stop_ioeventfd(proxy);
>      virtio_bus_reset(bus);
>      msix_unuse_all_vectors(&proxy->pci_dev);
> +    pci_irq_deassert(&proxy->pci_dev);
>  }
>  
>  static Property virtio_pci_properties[] = {
> -- 
> 1.9.3

Re: [Qemu-devel] [PATCH] virtio-pci: Clear IRQ at reset

[Fam Zheng]

On Thu, 03/12 10:41, Michael S. Tsirkin wrote:
> On Thu, Mar 12, 2015 at 02:40:55PM +0800, Fam Zheng wrote:
> > Currently we could leave PCI IRQ asserted even after reset, it is safer
> > to clear it.
> > 
> > In the case that a buggy driver has disabled MSI-X unintentially, we may
> > have already injected IRQ in previous virtio_pci_notify, which will not
> > be cleared by guest because it doesn't expect it (i.e. no irq handler).
> > However the driver may eventually notice the unresponsiveness and reset
> > the device, at that point, clearing the irq is meaningful.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> 
> I don't think we care about buggy drivers being able to recover, but the
> same would apply to e.g. kdump when using virtio blk/scsi after a crash,
> correct?

OK, yes, get your point. And this also relates to the patch you replied
yesterday:

http://www.spinics.net/lists/kernel/msg1943182.html

/*

The hanging problem, which is due to irq not being acknowledged, can only be
fixed by your patch if the irq is cleared at reset.

The uninterruptable process doesn't really matter, the system would still
shutdown, as long as the wrong irq is cleard.

See also RHBZ https://bugzilla.redhat.com/show_bug.cgi?id=1199155

*/

Fam

> 
> > ---
> >  hw/virtio/virtio-pci.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index e7baf7b..2600f1e 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -988,6 +988,7 @@ static void virtio_pci_reset(DeviceState *qdev)
> >      virtio_pci_stop_ioeventfd(proxy);
> >      virtio_bus_reset(bus);
> >      msix_unuse_all_vectors(&proxy->pci_dev);
> > +    pci_irq_deassert(&proxy->pci_dev);
> >  }
> >  
> >  static Property virtio_pci_properties[] = {
> > -- 
> > 1.9.3
> 

Re: [Qemu-devel] [PATCH] virtio-pci: Clear IRQ at reset

[Michael S. Tsirkin]

On Thu, Mar 12, 2015 at 06:00:28PM +0800, Fam Zheng wrote:
> On Thu, 03/12 10:41, Michael S. Tsirkin wrote:
> > On Thu, Mar 12, 2015 at 02:40:55PM +0800, Fam Zheng wrote:
> > > Currently we could leave PCI IRQ asserted even after reset, it is safer
> > > to clear it.
> > > 
> > > In the case that a buggy driver has disabled MSI-X unintentially, we may
> > > have already injected IRQ in previous virtio_pci_notify, which will not
> > > be cleared by guest because it doesn't expect it (i.e. no irq handler).
> > > However the driver may eventually notice the unresponsiveness and reset
> > > the device, at that point, clearing the irq is meaningful.
> > > 
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > 
> > I don't think we care about buggy drivers being able to recover, but the
> > same would apply to e.g. kdump when using virtio blk/scsi after a crash,
> > correct?
> 
> OK, yes, get your point. And this also relates to the patch you replied
> yesterday:
> 
> http://www.spinics.net/lists/kernel/msg1943182.html

About that one, I still don't know why it's necessary.
reboot causes a system reset and that will eventually
deassert an irq, no?


> /*
> 
> The hanging problem, which is due to irq not being acknowledged, can only be
> fixed by your patch if the irq is cleared at reset.
> 
> The uninterruptable process doesn't really matter, the system would still
> shutdown, as long as the wrong irq is cleard.
> 
> See also RHBZ https://bugzilla.redhat.com/show_bug.cgi?id=1199155
> 
> */
> 
> Fam
> 
> > 
> > > ---
> > >  hw/virtio/virtio-pci.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > index e7baf7b..2600f1e 100644
> > > --- a/hw/virtio/virtio-pci.c
> > > +++ b/hw/virtio/virtio-pci.c
> > > @@ -988,6 +988,7 @@ static void virtio_pci_reset(DeviceState *qdev)
> > >      virtio_pci_stop_ioeventfd(proxy);
> > >      virtio_bus_reset(bus);
> > >      msix_unuse_all_vectors(&proxy->pci_dev);
> > > +    pci_irq_deassert(&proxy->pci_dev);
> > >  }
> > >  
> > >  static Property virtio_pci_properties[] = {
> > > -- 
> > > 1.9.3
> > 

Re: [Qemu-devel] [PATCH] virtio-pci: Clear IRQ at reset

[Michael S. Tsirkin]

On Thu, Mar 12, 2015 at 06:00:28PM +0800, Fam Zheng wrote:
> On Thu, 03/12 10:41, Michael S. Tsirkin wrote:
> > On Thu, Mar 12, 2015 at 02:40:55PM +0800, Fam Zheng wrote:
> > > Currently we could leave PCI IRQ asserted even after reset, it is safer
> > > to clear it.
> > > 
> > > In the case that a buggy driver has disabled MSI-X unintentially, we may
> > > have already injected IRQ in previous virtio_pci_notify, which will not
> > > be cleared by guest because it doesn't expect it (i.e. no irq handler).
> > > However the driver may eventually notice the unresponsiveness and reset
> > > the device, at that point, clearing the irq is meaningful.
> > > 
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > 
> > I don't think we care about buggy drivers being able to recover, but the
> > same would apply to e.g. kdump when using virtio blk/scsi after a crash,
> > correct?
> 
> OK, yes, get your point.

So thinking about this more, by the time kdump tries to reset device,
linux has probably already disabled the IRQ at the APIC level.
Isn't that the case? If so, the patch won't help, will it?


> And this also relates to the patch you replied
> yesterday:
> 
> http://www.spinics.net/lists/kernel/msg1943182.html

OK can you please write a commit message that
explains the problem and how it's fixed,
so I can put it in git log?




> /*
> 
> The hanging problem, which is due to irq not being acknowledged, can only be
> fixed by your patch if the irq is cleared at reset.
> 
> The uninterruptable process doesn't really matter, the system would still
> shutdown, as long as the wrong irq is cleard.
> 
> See also RHBZ https://bugzilla.redhat.com/show_bug.cgi?id=1199155
> 
> */
> 
> Fam
> 
> > 
> > > ---
> > >  hw/virtio/virtio-pci.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > index e7baf7b..2600f1e 100644
> > > --- a/hw/virtio/virtio-pci.c
> > > +++ b/hw/virtio/virtio-pci.c
> > > @@ -988,6 +988,7 @@ static void virtio_pci_reset(DeviceState *qdev)
> > >      virtio_pci_stop_ioeventfd(proxy);
> > >      virtio_bus_reset(bus);
> > >      msix_unuse_all_vectors(&proxy->pci_dev);
> > > +    pci_irq_deassert(&proxy->pci_dev);
> > >  }
> > >  
> > >  static Property virtio_pci_properties[] = {
> > > -- 
> > > 1.9.3
> > 

Re: [Qemu-devel] [PATCH] virtio-pci: Clear IRQ at reset

[Peter Maydell]

On 12 March 2015 at 10:16, Michael S. Tsirkin <mst@redhat.com> wrote:
> So thinking about this more, by the time kdump tries to reset device,
> linux has probably already disabled the IRQ at the APIC level.
> Isn't that the case? If so, the patch won't help, will it?

Trying to deassert (or worse, assert) interrupt lines in
device reset functions is slightly bogus, yes. In general
the theory is that the interrupt controller the interrupt line
is connected to should have its own reset handling which
treats the line as going back to deasserted, because there's
no guarantee made about which of the two ends of the line
gets its reset handler called first.

Things are not really this neat in practice though. (There's
no good way to model a device which comes out of reset with
a line asserted, for instance.)

-- PMM

Re: [Qemu-devel] [PATCH] virtio-pci: Clear IRQ at reset

[Michael S. Tsirkin]

On Thu, Mar 12, 2015 at 10:21:47AM +0000, Peter Maydell wrote:
> On 12 March 2015 at 10:16, Michael S. Tsirkin <mst@redhat.com> wrote:
> > So thinking about this more, by the time kdump tries to reset device,
> > linux has probably already disabled the IRQ at the APIC level.
> > Isn't that the case? If so, the patch won't help, will it?
> 
> Trying to deassert (or worse, assert) interrupt lines in
> device reset functions is slightly bogus, yes. In general
> the theory is that the interrupt controller the interrupt line
> is connected to should have its own reset handling which
> treats the line as going back to deasserted, because there's
> no guarantee made about which of the two ends of the line
> gets its reset handler called first.
> 
> Things are not really this neat in practice though. (There's
> no good way to model a device which comes out of reset with
> a line asserted, for instance.)
> 
> -- PMM

This isn't a device reset though.
The function that Fam is touching is called
when a special "virtio reset" register to
poked by the driver.
It only resets part of the device, not all of it,
and it seems reasonable to ask that it clear the
interrupt.

So I think the patch is correct, even if just for
spec compliance reasons, but I would like to
find out and document the workloads that actually
benefit.

-- 
MST

Re: [Qemu-devel] [PATCH] virtio-pci: Clear IRQ at reset

[Peter Maydell]

On 12 March 2015 at 10:57, Michael S. Tsirkin <mst@redhat.com> wrote:
> This isn't a device reset though.
> The function that Fam is touching is called
> when a special "virtio reset" register to
> poked by the driver.
> It only resets part of the device, not all of it,
> and it seems reasonable to ask that it clear the
> interrupt.

Oh, right, sorry. Yes, that should clear the interrupt, then.
(Is there a similar bug on other virtio transports?)

-- PMM

Re: [Qemu-devel] [PATCH] virtio-pci: Clear IRQ at reset

[Michael S. Tsirkin]

On Thu, Mar 12, 2015 at 11:04:33AM +0000, Peter Maydell wrote:
> On 12 March 2015 at 10:57, Michael S. Tsirkin <mst@redhat.com> wrote:
> > This isn't a device reset though.
> > The function that Fam is touching is called
> > when a special "virtio reset" register to
> > poked by the driver.
> > It only resets part of the device, not all of it,
> > and it seems reasonable to ask that it clear the
> > interrupt.
> 
> Oh, right, sorry. Yes, that should clear the interrupt, then.
> (Is there a similar bug on other virtio transports?)
> 
> -- PMM

Hmm interesting.

I looked at virtio_reset and that one does:
    vdev->isr = 0;
    vdev->config_vector = VIRTIO_NO_VECTOR;
    virtio_notify_vector(vdev, vdev->config_vector);

which in turn would call
static void virtio_pci_notify(DeviceState *d, uint16_t vector)
{
    VirtIOPCIProxy *proxy = to_virtio_pci_proxy_fast(d);

    if (msix_enabled(&proxy->pci_dev))
        msix_notify(&proxy->pci_dev, vector);
    else {
        VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
        pci_set_irq(&proxy->pci_dev, vdev->isr & 1);
    }
}

since isr is 0, and msi is disabled, it looks like
pci_set_irq will get invoked automatically.

so at this point I stopped understanding how can
this patch help.

Fam, does your patch actually help some guests?
Could you pls investigate why isn't virtio_reset
sufficient?


-- 
MST

Re: [Qemu-devel] [PATCH] virtio-pci: Clear IRQ at reset

[Fam Zheng]

On Thu, 03/12 12:15, Michael S. Tsirkin wrote:
> On Thu, Mar 12, 2015 at 11:04:33AM +0000, Peter Maydell wrote:
> > On 12 March 2015 at 10:57, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > This isn't a device reset though.
> > > The function that Fam is touching is called
> > > when a special "virtio reset" register to
> > > poked by the driver.
> > > It only resets part of the device, not all of it,
> > > and it seems reasonable to ask that it clear the
> > > interrupt.
> > 
> > Oh, right, sorry. Yes, that should clear the interrupt, then.
> > (Is there a similar bug on other virtio transports?)
> > 
> > -- PMM
> 
> Hmm interesting.
> 
> I looked at virtio_reset and that one does:
>     vdev->isr = 0;
>     vdev->config_vector = VIRTIO_NO_VECTOR;
>     virtio_notify_vector(vdev, vdev->config_vector);
> 
> which in turn would call
> static void virtio_pci_notify(DeviceState *d, uint16_t vector)
> {
>     VirtIOPCIProxy *proxy = to_virtio_pci_proxy_fast(d);
> 
>     if (msix_enabled(&proxy->pci_dev))
>         msix_notify(&proxy->pci_dev, vector);
>     else {
>         VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
>         pci_set_irq(&proxy->pci_dev, vdev->isr & 1);
>     }
> }
> 
> since isr is 0, and msi is disabled, it looks like
> pci_set_irq will get invoked automatically.
> 
> so at this point I stopped understanding how can
> this patch help.
> 
> Fam, does your patch actually help some guests?
> Could you pls investigate why isn't virtio_reset
> sufficient?

Because virtio_reset doesn't disable msix, so here
"msix_notify(&proxy->pci_dev, 0)" is called.

I tested by applying your patch:

http://www.spinics.net/lists/kernel/msg1943182.html

and adding printf's in QEMU's virtio_reset and virtio_pci_notify.

It shows pci_set_irq is never called.

Fam

Re: [Qemu-devel] [PATCH] virtio-pci: Clear IRQ at reset

[Fam Zheng]

On Fri, 03/13 14:07, Fam Zheng wrote:
> On Thu, 03/12 12:15, Michael S. Tsirkin wrote:
> > On Thu, Mar 12, 2015 at 11:04:33AM +0000, Peter Maydell wrote:
> > > On 12 March 2015 at 10:57, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > This isn't a device reset though.
> > > > The function that Fam is touching is called
> > > > when a special "virtio reset" register to
> > > > poked by the driver.
> > > > It only resets part of the device, not all of it,
> > > > and it seems reasonable to ask that it clear the
> > > > interrupt.
> > > 
> > > Oh, right, sorry. Yes, that should clear the interrupt, then.
> > > (Is there a similar bug on other virtio transports?)
> > > 
> > > -- PMM
> > 
> > Hmm interesting.
> > 
> > I looked at virtio_reset and that one does:
> >     vdev->isr = 0;
> >     vdev->config_vector = VIRTIO_NO_VECTOR;
> >     virtio_notify_vector(vdev, vdev->config_vector);
> > 
> > which in turn would call
> > static void virtio_pci_notify(DeviceState *d, uint16_t vector)
> > {
> >     VirtIOPCIProxy *proxy = to_virtio_pci_proxy_fast(d);
> > 
> >     if (msix_enabled(&proxy->pci_dev))
> >         msix_notify(&proxy->pci_dev, vector);
> >     else {
> >         VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> >         pci_set_irq(&proxy->pci_dev, vdev->isr & 1);
> >     }
> > }
> > 
> > since isr is 0, and msi is disabled, it looks like
> > pci_set_irq will get invoked automatically.
> > 
> > so at this point I stopped understanding how can
> > this patch help.
> > 
> > Fam, does your patch actually help some guests?
> > Could you pls investigate why isn't virtio_reset
> > sufficient?
> 
> Because virtio_reset doesn't disable msix, so here
> "msix_notify(&proxy->pci_dev, 0)" is called.

s/0/VIRTIO_NO_VECTOR/

> 
> I tested by applying your patch:
> 
> http://www.spinics.net/lists/kernel/msg1943182.html
> 
> and adding printf's in QEMU's virtio_reset and virtio_pci_notify.
> 
> It shows pci_set_irq is never called.
> 
> Fam
> 
> 

Re: [Qemu-devel] [PATCH] virtio-pci: Clear IRQ at reset

[Michael S. Tsirkin]

On Fri, Mar 13, 2015 at 02:28:20PM +0800, Fam Zheng wrote:
> On Fri, 03/13 14:07, Fam Zheng wrote:
> > On Thu, 03/12 12:15, Michael S. Tsirkin wrote:
> > > On Thu, Mar 12, 2015 at 11:04:33AM +0000, Peter Maydell wrote:
> > > > On 12 March 2015 at 10:57, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > This isn't a device reset though.
> > > > > The function that Fam is touching is called
> > > > > when a special "virtio reset" register to
> > > > > poked by the driver.
> > > > > It only resets part of the device, not all of it,
> > > > > and it seems reasonable to ask that it clear the
> > > > > interrupt.
> > > > 
> > > > Oh, right, sorry. Yes, that should clear the interrupt, then.
> > > > (Is there a similar bug on other virtio transports?)
> > > > 
> > > > -- PMM
> > > 
> > > Hmm interesting.
> > > 
> > > I looked at virtio_reset and that one does:
> > >     vdev->isr = 0;
> > >     vdev->config_vector = VIRTIO_NO_VECTOR;
> > >     virtio_notify_vector(vdev, vdev->config_vector);
> > > 
> > > which in turn would call
> > > static void virtio_pci_notify(DeviceState *d, uint16_t vector)
> > > {
> > >     VirtIOPCIProxy *proxy = to_virtio_pci_proxy_fast(d);
> > > 
> > >     if (msix_enabled(&proxy->pci_dev))
> > >         msix_notify(&proxy->pci_dev, vector);
> > >     else {
> > >         VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> > >         pci_set_irq(&proxy->pci_dev, vdev->isr & 1);
> > >     }
> > > }
> > > 
> > > since isr is 0, and msi is disabled, it looks like
> > > pci_set_irq will get invoked automatically.
> > > 
> > > so at this point I stopped understanding how can
> > > this patch help.
> > > 
> > > Fam, does your patch actually help some guests?
> > > Could you pls investigate why isn't virtio_reset
> > > sufficient?
> > 
> > Because virtio_reset doesn't disable msix, so here
> > "msix_notify(&proxy->pci_dev, 0)" is called.
> 
> s/0/VIRTIO_NO_VECTOR/

My answer still holds - we don't need to clear msix
interrupts, and with msi enabled we know intx is low.

No?

> > 
> > I tested by applying your patch:
> > 
> > http://www.spinics.net/lists/kernel/msg1943182.html
> > 
> > and adding printf's in QEMU's virtio_reset and virtio_pci_notify.
> > 
> > It shows pci_set_irq is never called.
> > 
> > Fam
> > 
> > 

Re: [Qemu-devel] [PATCH] virtio-pci: Clear IRQ at reset

[Michael S. Tsirkin]

On Fri, Mar 13, 2015 at 02:07:19PM +0800, Fam Zheng wrote:
> On Thu, 03/12 12:15, Michael S. Tsirkin wrote:
> > On Thu, Mar 12, 2015 at 11:04:33AM +0000, Peter Maydell wrote:
> > > On 12 March 2015 at 10:57, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > This isn't a device reset though.
> > > > The function that Fam is touching is called
> > > > when a special "virtio reset" register to
> > > > poked by the driver.
> > > > It only resets part of the device, not all of it,
> > > > and it seems reasonable to ask that it clear the
> > > > interrupt.
> > > 
> > > Oh, right, sorry. Yes, that should clear the interrupt, then.
> > > (Is there a similar bug on other virtio transports?)
> > > 
> > > -- PMM
> > 
> > Hmm interesting.
> > 
> > I looked at virtio_reset and that one does:
> >     vdev->isr = 0;
> >     vdev->config_vector = VIRTIO_NO_VECTOR;
> >     virtio_notify_vector(vdev, vdev->config_vector);
> > 
> > which in turn would call
> > static void virtio_pci_notify(DeviceState *d, uint16_t vector)
> > {
> >     VirtIOPCIProxy *proxy = to_virtio_pci_proxy_fast(d);
> > 
> >     if (msix_enabled(&proxy->pci_dev))
> >         msix_notify(&proxy->pci_dev, vector);
> >     else {
> >         VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> >         pci_set_irq(&proxy->pci_dev, vdev->isr & 1);
> >     }
> > }
> > 
> > since isr is 0, and msi is disabled, it looks like
> > pci_set_irq will get invoked automatically.
> > 
> > so at this point I stopped understanding how can
> > this patch help.
> > 
> > Fam, does your patch actually help some guests?
> > Could you pls investigate why isn't virtio_reset
> > sufficient?
> 
> Because virtio_reset doesn't disable msix, so here
> "msix_notify(&proxy->pci_dev, 0)" is called.

Confused.  With msix enabled we know IRQ level is 0,
no need to clear it.


> I tested by applying your patch:
> 
> http://www.spinics.net/lists/kernel/msg1943182.html
> 
> and adding printf's in QEMU's virtio_reset and virtio_pci_notify.
> 
> It shows pci_set_irq is never called.
> 
> Fam