diff mbox series

[v3,1/8] vfio/pci: Invalidate mmaps and block the access in D3hot power state

Message ID 20220425092615.10133-2-abhsahu@nvidia.com (mailing list archive)
State Superseded
Headers show
Series vfio/pci: power management changes | expand

Commit Message

Abhishek Sahu April 25, 2022, 9:26 a.m. UTC
According to [PCIe v5 5.3.1.4.1] for D3hot state

 "Configuration and Message requests are the only TLPs accepted by a
  Function in the D3Hot state. All other received Requests must be
  handled as Unsupported Requests, and all received Completions may
  optionally be handled as Unexpected Completions."

Currently, if the vfio PCI device has been put into D3hot state and if
user makes non-config related read/write request in D3hot state, these
requests will be forwarded to the host and this access may cause
issues on a few systems.

This patch leverages the memory-disable support added in commit
'abafbc551fdd ("vfio-pci: Invalidate mmaps and block MMIO access on
disabled memory")' to generate page fault on mmap access and
return error for the direct read/write. If the device is D3hot state,
then the error will be returned for MMIO access. The IO access generally
does not make the system unresponsive so the IO access can still happen
in D3hot state. The default value should be returned in this case
without bringing down the complete system.

Also, the power related structure fields need to be protected so
we can use the same 'memory_lock' to protect these fields also.
This protection is mainly needed when user changes the PCI
power state by writing into PCI_PM_CTRL register.
vfio_lock_and_set_power_state() wrapper function will take the
required locks and then it will invoke the vfio_pci_set_power_state().

Signed-off-by: Abhishek Sahu <abhsahu@nvidia.com>
---
 drivers/vfio/pci/vfio_pci_config.c | 19 ++++++++++++++++++-
 drivers/vfio/pci/vfio_pci_core.c   |  4 +++-
 drivers/vfio/pci/vfio_pci_rdwr.c   |  6 ++++--
 include/linux/vfio_pci_core.h      |  1 +
 4 files changed, 26 insertions(+), 4 deletions(-)

Comments

kernel test robot April 26, 2022, 1:42 a.m. UTC | #1
Hi Abhishek,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on v5.18-rc4]
[also build test WARNING on next-20220422]
[cannot apply to awilliam-vfio/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Abhishek-Sahu/vfio-pci-power-management-changes/20220425-173224
base:    af2d861d4cd2a4da5137f795ee3509e6f944a25b
config: x86_64-rhel-8.3-kselftests (https://download.01.org/0day-ci/archive/20220426/202204260928.TsUAxMD3-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.2.0-20) 11.2.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-dirty
        # https://github.com/intel-lab-lkp/linux/commit/1d48b86a17606c483f200c1734085ab415dbfc3c
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Abhishek-Sahu/vfio-pci-power-management-changes/20220425-173224
        git checkout 1d48b86a17606c483f200c1734085ab415dbfc3c
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/vfio/pci/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> drivers/vfio/pci/vfio_pci_config.c:703:13: sparse: sparse: restricted pci_power_t degrades to integer
   drivers/vfio/pci/vfio_pci_config.c:703:22: sparse: sparse: restricted pci_power_t degrades to integer

vim +703 drivers/vfio/pci/vfio_pci_config.c

   694	
   695	/*
   696	 * It takes all the required locks to protect the access of power related
   697	 * variables and then invokes vfio_pci_set_power_state().
   698	 */
   699	static void
   700	vfio_lock_and_set_power_state(struct vfio_pci_core_device *vdev,
   701				      pci_power_t state)
   702	{
 > 703		if (state >= PCI_D3hot)
   704			vfio_pci_zap_and_down_write_memory_lock(vdev);
   705		else
   706			down_write(&vdev->memory_lock);
   707	
   708		vfio_pci_set_power_state(vdev, state);
   709		up_write(&vdev->memory_lock);
   710	}
   711
Bjorn Helgaas April 26, 2022, 2:14 p.m. UTC | #2
On Tue, Apr 26, 2022 at 09:42:45AM +0800, kernel test robot wrote:
> ...

> sparse warnings: (new ones prefixed by >>)
> >> drivers/vfio/pci/vfio_pci_config.c:703:13: sparse: sparse: restricted pci_power_t degrades to integer
>    drivers/vfio/pci/vfio_pci_config.c:703:22: sparse: sparse: restricted pci_power_t degrades to integer

I dunno what Alex thinks, but we have several of these warnings in
drivers/pci/.  I'd like to get rid of them, but we haven't figured out
a good way yet.  So this might be something we just live with for now.

> vim +703 drivers/vfio/pci/vfio_pci_config.c
> 
>    694	
>    695	/*
>    696	 * It takes all the required locks to protect the access of power related
>    697	 * variables and then invokes vfio_pci_set_power_state().
>    698	 */
>    699	static void
>    700	vfio_lock_and_set_power_state(struct vfio_pci_core_device *vdev,
>    701				      pci_power_t state)
>    702	{
>  > 703		if (state >= PCI_D3hot)
>    704			vfio_pci_zap_and_down_write_memory_lock(vdev);
>    705		else
>    706			down_write(&vdev->memory_lock);
>    707	
>    708		vfio_pci_set_power_state(vdev, state);
>    709		up_write(&vdev->memory_lock);
>    710	}
>    711	
> 
> -- 
> 0-DAY CI Kernel Test Service
> https://01.org/lkp
diff mbox series

Patch

diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
index 6e58b4bf7a60..dd557edae6e1 100644
--- a/drivers/vfio/pci/vfio_pci_config.c
+++ b/drivers/vfio/pci/vfio_pci_config.c
@@ -692,6 +692,23 @@  static int __init init_pci_cap_basic_perm(struct perm_bits *perm)
 	return 0;
 }
 
+/*
+ * It takes all the required locks to protect the access of power related
+ * variables and then invokes vfio_pci_set_power_state().
+ */
+static void
+vfio_lock_and_set_power_state(struct vfio_pci_core_device *vdev,
+			      pci_power_t state)
+{
+	if (state >= PCI_D3hot)
+		vfio_pci_zap_and_down_write_memory_lock(vdev);
+	else
+		down_write(&vdev->memory_lock);
+
+	vfio_pci_set_power_state(vdev, state);
+	up_write(&vdev->memory_lock);
+}
+
 static int vfio_pm_config_write(struct vfio_pci_core_device *vdev, int pos,
 				int count, struct perm_bits *perm,
 				int offset, __le32 val)
@@ -718,7 +735,7 @@  static int vfio_pm_config_write(struct vfio_pci_core_device *vdev, int pos,
 			break;
 		}
 
-		vfio_pci_set_power_state(vdev, state);
+		vfio_lock_and_set_power_state(vdev, state);
 	}
 
 	return count;
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 06b6f3594a13..f3dfb033e1c4 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -230,6 +230,8 @@  int vfio_pci_set_power_state(struct vfio_pci_core_device *vdev, pci_power_t stat
 	ret = pci_set_power_state(pdev, state);
 
 	if (!ret) {
+		vdev->power_state_d3 = (pdev->current_state >= PCI_D3hot);
+
 		/* D3 might be unsupported via quirk, skip unless in D3 */
 		if (needs_save && pdev->current_state >= PCI_D3hot) {
 			/*
@@ -1398,7 +1400,7 @@  static vm_fault_t vfio_pci_mmap_fault(struct vm_fault *vmf)
 	mutex_lock(&vdev->vma_lock);
 	down_read(&vdev->memory_lock);
 
-	if (!__vfio_pci_memory_enabled(vdev)) {
+	if (!__vfio_pci_memory_enabled(vdev) || vdev->power_state_d3) {
 		ret = VM_FAULT_SIGBUS;
 		goto up_out;
 	}
diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
index 82ac1569deb0..fac6bb40a4ce 100644
--- a/drivers/vfio/pci/vfio_pci_rdwr.c
+++ b/drivers/vfio/pci/vfio_pci_rdwr.c
@@ -43,7 +43,8 @@  static int vfio_pci_iowrite##size(struct vfio_pci_core_device *vdev,		\
 {									\
 	if (test_mem) {							\
 		down_read(&vdev->memory_lock);				\
-		if (!__vfio_pci_memory_enabled(vdev)) {			\
+		if (!__vfio_pci_memory_enabled(vdev) ||			\
+		    vdev->power_state_d3) {				\
 			up_read(&vdev->memory_lock);			\
 			return -EIO;					\
 		}							\
@@ -70,7 +71,8 @@  static int vfio_pci_ioread##size(struct vfio_pci_core_device *vdev,		\
 {									\
 	if (test_mem) {							\
 		down_read(&vdev->memory_lock);				\
-		if (!__vfio_pci_memory_enabled(vdev)) {			\
+		if (!__vfio_pci_memory_enabled(vdev) ||			\
+		    vdev->power_state_d3) {				\
 			up_read(&vdev->memory_lock);			\
 			return -EIO;					\
 		}							\
diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h
index 48f2dd3c568c..505b2a74a479 100644
--- a/include/linux/vfio_pci_core.h
+++ b/include/linux/vfio_pci_core.h
@@ -124,6 +124,7 @@  struct vfio_pci_core_device {
 	bool			needs_reset;
 	bool			nointx;
 	bool			needs_pm_restore;
+	bool			power_state_d3;
 	struct pci_saved_state	*pci_saved_state;
 	struct pci_saved_state	*pm_save;
 	int			ioeventfds_nr;