[Qemu-devel] Re: KVM vs. PCI-E Function Level Reset (FLR) ...

[Sheng Yang]

On Thursday 15 July 2010 23:39:36 Casey Leedom wrote:
> | From: Sheng Yang <address@hidden>
> | Date: Wednesday, July 14, 2010 06:31 pm
> | 
> | On Thursday 15 July 2010 02:01:29 Casey Leedom wrote:
> | > | From: Sheng Yang <address@hidden>
> | > | Date: Tuesday, July 13, 2010 05:53 pm
> | 
> | (Please use reply to all next time.)
> 
>   Sorry, old habit.
> 
> | > | On Wednesday 14 July 2010 04:41:01 Casey Leedom wrote:
> | >   Hhrrmmm, this seems like a semantic error.  "Resetting" the Vm should
> | > 
> | > be morally equivalent to resetting a real physical machine.  And when
> | > a real physical machine is reset, all of its buses, etc. get reset
> | > which propagates to Device Resets on those buses ...  I think that
> | > Assigned Devices should be reset for reboots and power off/on ...
> | 
> | Yes, it should be done when reset. And power on/off should be there,
> | because that's means the create and destroy the guest.
> 
>   Okay, cool.  I've looked through the kernel code and I don't see any
> likely places for putting such a VM Reboot/Reset feature in so I assume
> that this is also controlled by QEmu and it would need to be responsible
> for either doing a KVM_DEASSIGN_PCI_DEVICE/KVM_ASSIGN_PCI_DEVICE ioctl()
> pair for each assigned device or issuing a new
> KVM_RESET_ASSIGNED_PCI_DEVICE ioctl() for Reboot/Reset ...

Yeah, the detection of reset is not that straightforward... Maybe we need an 
ioctl 
for reset event in qemu-kvm kvm_reset_vcpu().

We don't need assign/deassign device when reboot/reset, we only need to notify 
KVM 
that the reset is happening, then KVM know what's to do.
 
> | > Assigned Devices.  For PCI-E SR-IOV Virtual Functions which are
> | > assigned to a VM, they need to be reset at reboot/power off/power on
> | > and the Configuration Space emulation needs to support the Guest OS/VM
> | > Device Driver issuing an FLR ...
> | 
> | You can add the FLR support for your device if you need to call it in the
> | guest. But I don't quite understand why you need to call it in the guest.
> | KVM should has already done that, and it's not necessary for native
> | device.
> 
>   This was mostly for device driver load/unload support.  I.e. being able
> to issue a Function Level Reset to a PCI Device Function (regardless of it
> being an SR-IOV Virtual Function or not) is a nice way to zap the device
> back to a canonical state.

OK.
> 
> | So you want to issue FLR(rather than probing the feature) when
> | driver->probe() called? Current seems KVM is the only user of
> | pci_reset_function(), so I think we can update it after your driver
> | posted to the mailing list. Also I am not sure why you want to reset
> | function when probing. KVM should cover them all I think.
> 
>   Remember, the "probe" _argument_ in pci_dev_reset() has nothing to do
> with whether it's being called from a device driver's probe() routine. 
> The "probe" _argument_ in pci_dev_reset() is used for the two-stage effort
> in
> pci_reset_function() which does:
> 
>  1. "try to see if it's possible to reset this function by itself"
>     Call pci_dev_reset(dev, probe=1);
> 
>  2. "go ahead and do the function reset"
>     Call pci_dev_reset(dev, probe=0);
> 
>   And yes, I'd noticed that KVM was the only current subscriber of the
> kernel interface but, as I noted above, it is useful for resetting the
> device function into a known state -- or at least it _was_ useful till the
> deadlock got introduced in 2.6.31 ... :-(  And again, the use of this
> actually has no dependency on KVM: I would want to be able to call
> pci_reset_function() in any device driver's probe() routine ...  It just
> happens that I ran into this need (and unfortunate 2.6.31 deadlock) with
> our PCI-E SR-IOV Virtual Function Driver.

What I meant was, before your driver, there is no such requirement in the code. 
And no one can predict every usage of the code in the future, so it's quite 
reasonable you called the "deadlock" is there. And I can't say it's a 
"deadlock" 
because there is no code in the current tree make it "deadlock" IIUR. 

So now you have this requirement, you can modify it to fit your need. That's 
quite 
straightforward...
> 
>   Oh, and the driver has been posted to net-next.  I'm guessing that it
> _should_ get merged into 2.6.35 ... or maybe 2.6.36 ... I'm really not
> sure of how the merge schedule between net-next and the core kernel runs
> ...

That's good. So you can modify the function to provide a lockless version. That 
make sense now. :)

--
regards
Yang, Sheng

> 
> Casey