PCI device not properly reset after VFIO

Hi Alex,

I've been playing around with VFIO and megasas (of course). What I did now was switching between VFIO and 'normal' operation, ie emulated access.

megasas is happily running under VFIO, but when I do an emergency stop like killing the Qemu session the PCI device is not properly reset. IE when I load 'megaraid_sas' after unbinding the vfio_pci module the driver cannot initialize the card and waits forever for the firmware state to change.

I need to do a proper pci reset via
echo 1 > /sys/bus/pci/device/XXXX/reset
to get it into a working state again.

Looking at vfio_pci_disable() pci reset is called before the config state and BARs are restored.
Seeing that vfio_pci_enable() calls pci reset right at the start, too, before modifying anything I do wonder whether the pci reset is at the correct location for disable.

I would have expected to call pci reset in vfio_pci_disable() _after_ we have restored the configuration, to ensure a sane state after reset.
And, as experience show, we do need to call it there.

So what is the rationale for the pci reset?
Can we move it to the end of vfio_pci_disable() or do we need to call pci reset twice?

Hi Alex,

I've been playing around with VFIO and megasas (of course). What I did now was switching between VFIO and 'normal' operation, ie emulated access.

megasas is happily running under VFIO, but when I do an emergency stop like killing the Qemu session the PCI device is not properly reset. IE when I load 'megaraid_sas' after unbinding the vfio_pci module the driver cannot initialize the card and waits forever for the firmware state to change.

I need to do a proper pci reset via
echo 1 > /sys/bus/pci/device/XXXX/reset
to get it into a working state again.

Looking at vfio_pci_disable() pci reset is called before the config state and BARs are restored.
Seeing that vfio_pci_enable() calls pci reset right at the start, too, before modifying anything I do wonder whether the pci reset is at the correct location for disable.

I would have expected to call pci reset in vfio_pci_disable() _after_ we have restored the configuration, to ensure a sane state after reset.
And, as experience show, we do need to call it there.

So what is the rationale for the pci reset?
Can we move it to the end of vfio_pci_disable() or do we need to call pci reset twice?

Hi Hannes,

Thanks for testing vfio

Quoted 22 lines · Hannes Reinecke

Hi Alex,

I've been playing around with VFIO and megasas (of course). What I did now was switching between VFIO and 'normal' operation, ie emulated access.

megasas is happily running under VFIO, but when I do an emergency stop like killing the Qemu session the PCI device is not properly reset. IE when I load 'megaraid_sas' after unbinding the vfio_pci module the driver cannot initialize the card and waits forever for the firmware state to change.

I need to do a proper pci reset via
echo 1 > /sys/bus/pci/device/XXXX/reset
to get it into a working state again.

Looking at vfio_pci_disable() pci reset is called before the config state and BARs are restored.
Seeing that vfio_pci_enable() calls pci reset right at the start, too, before modifying anything I do wonder whether the pci reset is at the correct location for disable.

Quoted 8 lines

I would have expected to call pci reset in vfio_pci_disable() _after_ we have restored the configuration, to ensure a sane state after reset.
And, as experience show, we do need to call it there.

So what is the rationale for the pci reset?
Can we move it to the end of vfio_pci_disable() or do we need to call pci reset twice?

I believe the rationale was that by resetting the device before we restore the state we stop anything that the device was doing. Restoring the saved state on a running device seems like it could cause problems, so you may be right and we actually need to do reset, load, restore, reset. Does adding another call to pci_reset_function in the pci_restore_state (as below) solve the problem? Traditional KVM device assignment has a nearly identical path, does it have this same bug?

Thanks,

Alex

drivers/vfio/pci/vfio_pci.c +0 -0

index 6c11994..d07a45c 100644
@@ -107,9 +107,10 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev)
         pci_reset_function(vdev->pdev);
 

Hi Hannes,

Thanks for testing vfio

Quoted 22 lines · Hannes Reinecke

Hi Alex,

I've been playing around with VFIO and megasas (of course). What I did now was switching between VFIO and 'normal' operation, ie emulated access.

megasas is happily running under VFIO, but when I do an emergency stop like killing the Qemu session the PCI device is not properly reset. IE when I load 'megaraid_sas' after unbinding the vfio_pci module the driver cannot initialize the card and waits forever for the firmware state to change.

I need to do a proper pci reset via
echo 1 > /sys/bus/pci/device/XXXX/reset
to get it into a working state again.

Looking at vfio_pci_disable() pci reset is called before the config state and BARs are restored.
Seeing that vfio_pci_enable() calls pci reset right at the start, too, before modifying anything I do wonder whether the pci reset is at the correct location for disable.

Quoted 8 lines

I would have expected to call pci reset in vfio_pci_disable() _after_ we have restored the configuration, to ensure a sane state after reset.
And, as experience show, we do need to call it there.

So what is the rationale for the pci reset?
Can we move it to the end of vfio_pci_disable() or do we need to call pci reset twice?

I believe the rationale was that by resetting the device before we restore the state we stop anything that the device was doing. Restoring the saved state on a running device seems like it could cause problems, so you may be right and we actually need to do reset, load, restore, reset. Does adding another call to pci_reset_function in the pci_restore_state (as below) solve the problem? Traditional KVM device assignment has a nearly identical path, does it have this same bug?

Thanks,

Alex

drivers/vfio/pci/vfio_pci.c +0 -0

index 6c11994..d07a45c 100644
@@ -107,9 +107,10 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev)
         pci_reset_function(vdev->pdev);
 

Quoted 36 lines · Alex Williamson

Hi Hannes,

Thanks for testing vfio

Quoted 22 lines · Hannes Reinecke

Hi Alex,

I've been playing around with VFIO and megasas (of course). What I did now was switching between VFIO and 'normal' operation, ie emulated access.

megasas is happily running under VFIO, but when I do an emergency stop like killing the Qemu session the PCI device is not properly reset. IE when I load 'megaraid_sas' after unbinding the vfio_pci module the driver cannot initialize the card and waits forever for the firmware state to change.

I need to do a proper pci reset via
echo 1 > /sys/bus/pci/device/XXXX/reset
to get it into a working state again.

Looking at vfio_pci_disable() pci reset is called before the config state and BARs are restored.
Seeing that vfio_pci_enable() calls pci reset right at the start, too, before modifying anything I do wonder whether the pci reset is at the correct location for disable.

Quoted 8 lines

I would have expected to call pci reset in vfio_pci_disable() _after_ we have restored the configuration, to ensure a sane state after reset.
And, as experience show, we do need to call it there.

So what is the rationale for the pci reset?
Can we move it to the end of vfio_pci_disable() or do we need to call pci reset twice?

Quoted 7 lines

I believe the rationale was that by resetting the device before we restore the state we stop anything that the device was doing. Restoring the saved state on a running device seems like it could cause problems, so you may be right and we actually need to do reset, load, restore, reset. Does adding another call to pci_reset_function in the pci_restore_state (as below) solve the problem? Traditional KVM device assignment has a nearly identical path, does it have this same bug?

It's actually the first time I've been able to test this (the hardware is a bit tricky to setup ...), so I cannot tell (yet) if KVM exhibited the same thing.

Quoted 11 lines

Thanks,

Alex

drivers/vfio/pci/vfio_pci.c +0 -0

index 6c11994..d07a45c 100644
@@ -107,9 +107,10 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev)
         pci_reset_function(vdev->pdev);
 

drivers/vfio/pci/vfio_pci.c static void vfio_pci_disable(struct vfio_pci_device *vdev) · line 116

Quoted 12 lines

drivers/vfio/pci/vfio_pci.c +3 -2

         if (pci_load_and_free_saved_state(vdev->pdev,
-                                         &vdev->pci_saved_state) == 0)
+                                         &vdev->pci_saved_state) == 0) {
                 pci_restore_state(vdev->pdev);
-       else
+               pci_reset_function(vdev->pdev);
+       } else
                 pr_info("%s: Couldn't reload %s saved state\n",
                         __func__, dev_name(&vdev->pdev->dev));
 

I would have called reset after unmapping the BARs; the HBA I'm working with does need to access the BARs, so the content of them might be relevant, too.