Re: [Qemu-devel] [RFC v2 6/8] vfio_pci: fix a wrong check in vfio_pci_reset

2015-02-04 Thread Alex Williamson 
On Wed, 2015-02-04 at 17:54 +0800, Chen Fan wrote:
 On 02/03/2015 04:16 AM, Alex Williamson wrote:
  On Wed, 2015-01-28 at 16:37 +0800, Chen Fan wrote:
  when vfio device support FLR, then when device reset,
  we call VFIO_DEVICE_RESET ioctl to reset the device first,
  at kernel side, we also can see the order of reset:
  3330 rc = pcie_flr(dev, probe);
  3331 if (rc != -ENOTTY)
  3332 goto done;
  
  3334 rc = pci_af_flr(dev, probe);
  3335 if (rc != -ENOTTY)
  3336 goto done;
  3337
  3338 rc = pci_pm_reset(dev, probe);
  3339 if (rc != -ENOTTY)
  3340 goto done;
 
  so when vfio has FLR, reset it directly.
 
  Signed-off-by: Chen Fan chen.fan.f...@cn.fujitsu.com
  ---
hw/vfio/pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
 
  diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
  index 8c81bb3..54eb6b4 100644
  --- a/hw/vfio/pci.c
  +++ b/hw/vfio/pci.c
  @@ -3455,7 +3455,7 @@ static void vfio_pci_reset(DeviceState *dev)
vfio_pci_pre_reset(vdev);

if (vdev-vbasedev.reset_works 
  -(vdev-has_flr || !vdev-has_pm_reset) 
  +vdev-has_flr 
!ioctl(vdev-vbasedev.fd, VFIO_DEVICE_RESET)) {
trace_vfio_pci_reset_flr(vdev-vbasedev.name);
goto post_reset;
  Does this actually fix anything?  QEMU shouldn't rely on a specific
  behavior of the kernel.  This test is de-prioritizing a PM reset because
  they're often non-effective.  If the device supports FLR, the second
  part of the OR is unreached, so what's the point of this change?
 For this change, when I tested the code on my own machine.
 I found the vfio device has neither flr nor pm reset (e.g. NoSoftRst+).
 this also trigger ioctl VFIO_DEVICE_RESET, is it right?

Yes, that means that the device has a device specific reset or that it's
a singleton device on the bus and we can use the simpler path of
VFIO_DEVICE_RESET.  Thanks,

Alex

Re: [Qemu-devel] [RFC v2 6/8] vfio_pci: fix a wrong check in vfio_pci_reset

2015-02-04 Thread Chen Fan 


On 02/03/2015 04:16 AM, Alex Williamson wrote:

On Wed, 2015-01-28 at 16:37 +0800, Chen Fan wrote:

when vfio device support FLR, then when device reset,
we call VFIO_DEVICE_RESET ioctl to reset the device first,
at kernel side, we also can see the order of reset:
3330 rc = pcie_flr(dev, probe);
3331 if (rc != -ENOTTY)
3332 goto done;

3334 rc = pci_af_flr(dev, probe);
3335 if (rc != -ENOTTY)
3336 goto done;
3337
3338 rc = pci_pm_reset(dev, probe);
3339 if (rc != -ENOTTY)
3340 goto done;

so when vfio has FLR, reset it directly.

Signed-off-by: Chen Fan chen.fan.f...@cn.fujitsu.com
---
  hw/vfio/pci.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 8c81bb3..54eb6b4 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3455,7 +3455,7 @@ static void vfio_pci_reset(DeviceState *dev)
  vfio_pci_pre_reset(vdev);
  
  if (vdev-vbasedev.reset_works 

-(vdev-has_flr || !vdev-has_pm_reset) 
+vdev-has_flr 
  !ioctl(vdev-vbasedev.fd, VFIO_DEVICE_RESET)) {
  trace_vfio_pci_reset_flr(vdev-vbasedev.name);
  goto post_reset;

Does this actually fix anything?  QEMU shouldn't rely on a specific
behavior of the kernel.  This test is de-prioritizing a PM reset because
they're often non-effective.  If the device supports FLR, the second
part of the OR is unreached, so what's the point of this change?

For this change, when I tested the code on my own machine.
I found the vfio device has neither flr nor pm reset (e.g. NoSoftRst+).
this also trigger ioctl VFIO_DEVICE_RESET, is it right?

Thanks,
Chen



Thanks,

Alex

.

Re: [Qemu-devel] [RFC v2 6/8] vfio_pci: fix a wrong check in vfio_pci_reset

2015-02-02 Thread Alex Williamson 
On Wed, 2015-01-28 at 16:37 +0800, Chen Fan wrote:
 when vfio device support FLR, then when device reset,
 we call VFIO_DEVICE_RESET ioctl to reset the device first,
 at kernel side, we also can see the order of reset:
 3330 rc = pcie_flr(dev, probe);
 3331 if (rc != -ENOTTY)
 3332 goto done;
 
 3334 rc = pci_af_flr(dev, probe);
 3335 if (rc != -ENOTTY)
 3336 goto done;
 3337
 3338 rc = pci_pm_reset(dev, probe);
 3339 if (rc != -ENOTTY)
 3340 goto done;
 
 so when vfio has FLR, reset it directly.
 
 Signed-off-by: Chen Fan chen.fan.f...@cn.fujitsu.com
 ---
  hw/vfio/pci.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
 index 8c81bb3..54eb6b4 100644
 --- a/hw/vfio/pci.c
 +++ b/hw/vfio/pci.c
 @@ -3455,7 +3455,7 @@ static void vfio_pci_reset(DeviceState *dev)
  vfio_pci_pre_reset(vdev);
  
  if (vdev-vbasedev.reset_works 
 -(vdev-has_flr || !vdev-has_pm_reset) 
 +vdev-has_flr 
  !ioctl(vdev-vbasedev.fd, VFIO_DEVICE_RESET)) {
  trace_vfio_pci_reset_flr(vdev-vbasedev.name);
  goto post_reset;

Does this actually fix anything?  QEMU shouldn't rely on a specific
behavior of the kernel.  This test is de-prioritizing a PM reset because
they're often non-effective.  If the device supports FLR, the second
part of the OR is unreached, so what's the point of this change?
Thanks,

Alex

[Qemu-devel] [RFC v2 6/8] vfio_pci: fix a wrong check in vfio_pci_reset

2015-01-28 Thread Chen Fan 
when vfio device support FLR, then when device reset,
we call VFIO_DEVICE_RESET ioctl to reset the device first,
at kernel side, we also can see the order of reset:
3330 rc = pcie_flr(dev, probe);
3331 if (rc != -ENOTTY)
3332 goto done;

3334 rc = pci_af_flr(dev, probe);
3335 if (rc != -ENOTTY)
3336 goto done;
3337
3338 rc = pci_pm_reset(dev, probe);
3339 if (rc != -ENOTTY)
3340 goto done;

so when vfio has FLR, reset it directly.

Signed-off-by: Chen Fan chen.fan.f...@cn.fujitsu.com
---
 hw/vfio/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 8c81bb3..54eb6b4 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3455,7 +3455,7 @@ static void vfio_pci_reset(DeviceState *dev)
 vfio_pci_pre_reset(vdev);
 
 if (vdev-vbasedev.reset_works 
-(vdev-has_flr || !vdev-has_pm_reset) 
+vdev-has_flr 
 !ioctl(vdev-vbasedev.fd, VFIO_DEVICE_RESET)) {
 trace_vfio_pci_reset_flr(vdev-vbasedev.name);
 goto post_reset;
-- 
1.9.3