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

[Avi Kivity <avi@redhat.com>]

On 04/08/2012 05:42 PM, Michael S. Tsirkin wrote:
> On Sun, Apr 08, 2012 at 05:01:36PM +0300, Avi Kivity wrote:
> > On 04/08/2012 04:53 PM, Michael S. Tsirkin wrote:
> > > On Sun, Apr 08, 2012 at 04:41:27PM +0300, Avi Kivity wrote:
> > > > On 04/08/2012 04:30 PM, Michael S. Tsirkin wrote:
> > > > > On Sun, Apr 08, 2012 at 04:24:11PM +0300, Avi Kivity wrote:
> > > > > > On 04/08/2012 04:21 PM, Michael S. Tsirkin wrote:
> > > > > > > On Sun, Apr 08, 2012 at 04:18:29PM +0300, Avi Kivity wrote:
> > > > > > > > On 04/08/2012 04:17 PM, Michael S. Tsirkin wrote:
> > > > > > > > > > 
> > > > > > > > > > Don't we FLR the device, which ought to disable MSI on the real device?
> > > > > > > > >
> > > > > > > > > AFAIK we call pci_reset, which saves device state, does an FLR
> > > > > > > > > and then restores the state. I think this might include msi as well.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > 
> > > > > > > > Then that is wrong as well, no?
> > > > > > >
> > > > > > > Not as such assuming we disable msi/msix first :)
> > > > > > 
> > > > > > I think we need to fix both, no?
> > > > >
> > > > > Isn't this what this patch does?
> > > > 
> > > > If we change pci_reset() (or a variant that we call) to reset MSI, and
> > > > update qemu to synchronize from the device after pci_reset(), then we
> > > > achieve the same result, in a different way.
> > >
> > > MSI vectors are set up by kvm in the host. So we should not
> > > abruptly drop that by a sysfs write: would need to
> > > synchronise with kvm. Once we do, there's nothing left
> > > for pci_reset to do.
> > 
> > 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.

> >         set emulated register to hardware value
>
> Yes, I see what you are trying to say now.
> Unfortunately that's not enough: we also
> need to restore the registers afterwards for
> device to become useful again.

I guess this is correct for the MSIX BAR.  But is it correct for MSIX
enable/disable?

> 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?


> > > > Since reset can change other config space registers, we achieve
> > > > correctness for more of them.
> > >
> > > Which other registers do you have in mind?
> > 
> > BARs for example.  We may have our own reset for this, but isn't copying
> > the hardware values more trustworthy?
>
> BAR values in host and guest are unrelated.
> If pci_reset didn't restore BAR values we won't
> be able to operate the device.
>

Right.

I guess candidates are those that are initialized with
assigned_dev_emulate_config_*()?  Hard to see which ones because they're
mass initialized.



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