Re: [PATCH v2] kvm: Disable MSI/MSI-X in assigned device reset path

["Michael S. Tsirkin" <mst@redhat.com>]

On Sun, Apr 08, 2012 at 06:50:27PM +0300, Avi Kivity wrote:
> On 04/08/2012 06:46 PM, Michael S. Tsirkin wrote:
> > > > > 
> > > > > I'm thinking about this flow:
> > > > > 
> > > > >   FLR the device
> > > > >   for each emulated register
> > > > >      read it from the hardware
> > > > >      if different from emulated register:
> > > > >         update the internal model (for example, disabling MSI in kvm if
> > > > > needed)
> > > >
> > > > If we do it this way we get back the problem this patch
> > > > is trying to solve: MSIX assigned while device
> > > > memory is disabled would cause unsupported request errors.
> > > 
> > > Why is that?  FLR would presumably disable MSI in the device, and this
> > > line would disable it in kvm as well.
> >
> > The bug is that device memory is disabled (FLR would do that)
> > while MSI is enabled in kvm. The fix is to
> > disable MSI in kvm first.
> 
> Yes, no need to repeat.  My question is whether my pseudo-code does the
> same

It doesn't seem to: FLR (disabling memory) is followed
by MSI disable in kvm instead of the reverse.

> and whether or not if it is better (when applied to all emulated
> config space).

I'm not sure.
I would like to see an example of a register that you have
in mind.

> > > > Doing this in kernel seems more robust, otherwise
> > > > we risk losing the device if qemu gets killed
> > > > before it has restored the registers.
> > > 
> > > Doesn't the driver have to enable MSIX if it attaches to the device at
> > > that point, anyway?
> >
> > Yes. I'm talking about things like enabling memory, setting up irq register,
> > etc though. Most of this setup is done by bios.
> 
> I see.  So should we have a pci_reset_function() variant that limits
> itself to restoring just those bits?

We only need kernel to restore whatever qemu emulates, but
kernel doesn't know what that is.
What kind of interface do you have in mind?

> -- 
> error compiling committee.c: too many arguments to function