On Sun, Apr 08, 2012 at 06:26:28PM +0300, Avi Kivity wrote: > 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. 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. > > > 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? Probably not. > > 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. > > > > > 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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
