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

[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 <sheng@linux.intel.com>
> | Date: Wednesday, July 14, 2010 06:31 pm
> | 
> | On Thursday 15 July 2010 02:01:29 Casey Leedom wrote:
> | > | From: Sheng Yang <sheng@linux.intel.com>
> | > | 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

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

[Casey Leedom]

| From: Sheng Yang <sheng@linux.intel.com>
| Date: Thursday, July 15, 2010 05:56 pm
| 
| 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.

  I'm not familiar enough with the KVM/QEmu to know where how to make these 
changes easily.  I'd be happy to test such changes or even take a whack at 
making the changes if someone could point me at the relevant repositories/files.  
Alternatively, I could file a bug if KVM has its own bug database ...

| >   [Calling pci_reset_function() in our driver->probe() routine] 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.
| 
| 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. :)

  Okay, I'll ask Yu Zhao why he added the lock in pci_dev_reset() (changeset 
8c1c699fec9e9021bf6ff0285dee086bb27aec90) and if he feels that adding a non-
locking path would make sense -- this is an area of the kernel which I've only 
perused so I don't want to propose random changes ... :-)

Casey