| Message ID | 87dd603bfecae702fc24207300b047937933618b.1422433767.git.chen.fan.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
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; > 3333 > 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.fnst@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
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; >> 3333 >> 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.fnst@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 > > . >
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; > >> 3333 > >> 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.fnst@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
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;
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; 3333 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.fnst@cn.fujitsu.com> --- hw/vfio/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)