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 <address@hidden>
> > 
> > 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
> >