| Message ID | 20220522094756.219881-1-yishaih@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio: Split migration ops from main device ops | expand |
Hi Yishai,
I love your patch! Yet something to improve:
[auto build test ERROR on awilliam-vfio/next]
[cannot apply to v5.18-rc7]
[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/Yishai-Hadas/vfio-Split-migration-ops-from-main-device-ops/20220522-174959
base: https://github.com/awilliam/linux-vfio.git next
config: arm64-allyesconfig (https://download.01.org/0day-ci/archive/20220522/202205222209.5JkbCwDa-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/f9fa522b20c805dbbb0907b0f90b2b7f1d260218
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Yishai-Hadas/vfio-Split-migration-ops-from-main-device-ops/20220522-174959
git checkout f9fa522b20c805dbbb0907b0f90b2b7f1d260218
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c: In function 'hisi_acc_vfio_pci_open_device':
>> drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c:1188:27: error: 'const struct vfio_device_ops' has no member named 'migration_set_state'
1188 | if (core_vdev->ops->migration_set_state) {
| ^~
At top level:
drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c:1201:13: warning: 'hisi_acc_vfio_pci_close_device' defined but not used [-Wunused-function]
1201 | static void hisi_acc_vfio_pci_close_device(struct vfio_device *core_vdev)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c:1138:13: warning: 'hisi_acc_vfio_pci_ioctl' defined but not used [-Wunused-function]
1138 | static long hisi_acc_vfio_pci_ioctl(struct vfio_device *core_vdev, unsigned int cmd,
| ^~~~~~~~~~~~~~~~~~~~~~~
drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c:1124:16: warning: 'hisi_acc_vfio_pci_read' defined but not used [-Wunused-function]
1124 | static ssize_t hisi_acc_vfio_pci_read(struct vfio_device *core_vdev,
| ^~~~~~~~~~~~~~~~~~~~~~
drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c:1110:16: warning: 'hisi_acc_vfio_pci_write' defined but not used [-Wunused-function]
1110 | static ssize_t hisi_acc_vfio_pci_write(struct vfio_device *core_vdev,
| ^~~~~~~~~~~~~~~~~~~~~~~
drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c:1086:12: warning: 'hisi_acc_vfio_pci_mmap' defined but not used [-Wunused-function]
1086 | static int hisi_acc_vfio_pci_mmap(struct vfio_device *core_vdev,
| ^~~~~~~~~~~~~~~~~~~~~~
vim +1188 drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
6abdce51af1a21 Shameer Kolothum 2022-03-08 1176
ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1177 static int hisi_acc_vfio_pci_open_device(struct vfio_device *core_vdev)
ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1178 {
b0eed085903e77 Longfang Liu 2022-03-08 1179 struct hisi_acc_vf_core_device *hisi_acc_vdev = container_of(core_vdev,
b0eed085903e77 Longfang Liu 2022-03-08 1180 struct hisi_acc_vf_core_device, core_device.vdev);
b0eed085903e77 Longfang Liu 2022-03-08 1181 struct vfio_pci_core_device *vdev = &hisi_acc_vdev->core_device;
ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1182 int ret;
ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1183
ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1184 ret = vfio_pci_core_enable(vdev);
ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1185 if (ret)
ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1186 return ret;
ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1187
b0eed085903e77 Longfang Liu 2022-03-08 @1188 if (core_vdev->ops->migration_set_state) {
b0eed085903e77 Longfang Liu 2022-03-08 1189 ret = hisi_acc_vf_qm_init(hisi_acc_vdev);
b0eed085903e77 Longfang Liu 2022-03-08 1190 if (ret) {
b0eed085903e77 Longfang Liu 2022-03-08 1191 vfio_pci_core_disable(vdev);
b0eed085903e77 Longfang Liu 2022-03-08 1192 return ret;
b0eed085903e77 Longfang Liu 2022-03-08 1193 }
b0eed085903e77 Longfang Liu 2022-03-08 1194 hisi_acc_vdev->mig_state = VFIO_DEVICE_STATE_RUNNING;
b0eed085903e77 Longfang Liu 2022-03-08 1195 }
ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1196
b0eed085903e77 Longfang Liu 2022-03-08 1197 vfio_pci_core_finish_enable(vdev);
ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1198 return 0;
ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1199 }
ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1200
Alex, This patch requires some extra handling in hisi driver to fix and encapsulate the migration specific handling in a single function per op, which at the end will call the matching vfio_pci_core_xxx function. This won't fit to current kernel cycle as the merge window is almost here, however your feedback on the below approach would be appreciated. Shameerali, Can you please review the below and send me some matching code in your driver ? I may put as part of V1, unless that you prefer that I'll do. Thanks, Yishai On 22/05/2022 17:21, kernel test robot wrote: > Hi Yishai, > > I love your patch! Yet something to improve: > > [auto build test ERROR on awilliam-vfio/next] > [cannot apply to v5.18-rc7] > [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/Yishai-Hadas/vfio-Split-migration-ops-from-main-device-ops/20220522-174959 > base: https://github.com/awilliam/linux-vfio.git next > config: arm64-allyesconfig (https://download.01.org/0day-ci/archive/20220522/202205222209.5JkbCwDa-lkp@intel.com/config) > compiler: aarch64-linux-gcc (GCC) 11.3.0 > reproduce (this is a W=1 build): > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # https://github.com/intel-lab-lkp/linux/commit/f9fa522b20c805dbbb0907b0f90b2b7f1d260218 > git remote add linux-review https://github.com/intel-lab-lkp/linux > git fetch --no-tags linux-review Yishai-Hadas/vfio-Split-migration-ops-from-main-device-ops/20220522-174959 > git checkout f9fa522b20c805dbbb0907b0f90b2b7f1d260218 > # save the config file > mkdir build_dir && cp config build_dir/.config > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash > > If you fix the issue, kindly add following tag where applicable > Reported-by: kernel test robot <lkp@intel.com> > > All errors (new ones prefixed by >>): > > drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c: In function 'hisi_acc_vfio_pci_open_device': >>> drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c:1188:27: error: 'const struct vfio_device_ops' has no member named 'migration_set_state' > 1188 | if (core_vdev->ops->migration_set_state) { > | ^~ > At top level: > drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c:1201:13: warning: 'hisi_acc_vfio_pci_close_device' defined but not used [-Wunused-function] > 1201 | static void hisi_acc_vfio_pci_close_device(struct vfio_device *core_vdev) > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c:1138:13: warning: 'hisi_acc_vfio_pci_ioctl' defined but not used [-Wunused-function] > 1138 | static long hisi_acc_vfio_pci_ioctl(struct vfio_device *core_vdev, unsigned int cmd, > | ^~~~~~~~~~~~~~~~~~~~~~~ > drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c:1124:16: warning: 'hisi_acc_vfio_pci_read' defined but not used [-Wunused-function] > 1124 | static ssize_t hisi_acc_vfio_pci_read(struct vfio_device *core_vdev, > | ^~~~~~~~~~~~~~~~~~~~~~ > drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c:1110:16: warning: 'hisi_acc_vfio_pci_write' defined but not used [-Wunused-function] > 1110 | static ssize_t hisi_acc_vfio_pci_write(struct vfio_device *core_vdev, > | ^~~~~~~~~~~~~~~~~~~~~~~ > drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c:1086:12: warning: 'hisi_acc_vfio_pci_mmap' defined but not used [-Wunused-function] > 1086 | static int hisi_acc_vfio_pci_mmap(struct vfio_device *core_vdev, > | ^~~~~~~~~~~~~~~~~~~~~~ > > > vim +1188 drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > > 6abdce51af1a21 Shameer Kolothum 2022-03-08 1176 > ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1177 static int hisi_acc_vfio_pci_open_device(struct vfio_device *core_vdev) > ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1178 { > b0eed085903e77 Longfang Liu 2022-03-08 1179 struct hisi_acc_vf_core_device *hisi_acc_vdev = container_of(core_vdev, > b0eed085903e77 Longfang Liu 2022-03-08 1180 struct hisi_acc_vf_core_device, core_device.vdev); > b0eed085903e77 Longfang Liu 2022-03-08 1181 struct vfio_pci_core_device *vdev = &hisi_acc_vdev->core_device; > ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1182 int ret; > ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1183 > ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1184 ret = vfio_pci_core_enable(vdev); > ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1185 if (ret) > ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1186 return ret; > ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1187 > b0eed085903e77 Longfang Liu 2022-03-08 @1188 if (core_vdev->ops->migration_set_state) { > b0eed085903e77 Longfang Liu 2022-03-08 1189 ret = hisi_acc_vf_qm_init(hisi_acc_vdev); > b0eed085903e77 Longfang Liu 2022-03-08 1190 if (ret) { > b0eed085903e77 Longfang Liu 2022-03-08 1191 vfio_pci_core_disable(vdev); > b0eed085903e77 Longfang Liu 2022-03-08 1192 return ret; > b0eed085903e77 Longfang Liu 2022-03-08 1193 } > b0eed085903e77 Longfang Liu 2022-03-08 1194 hisi_acc_vdev->mig_state = VFIO_DEVICE_STATE_RUNNING; > b0eed085903e77 Longfang Liu 2022-03-08 1195 } > ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1196 > b0eed085903e77 Longfang Liu 2022-03-08 1197 vfio_pci_core_finish_enable(vdev); > ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1198 return 0; > ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1199 } > ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1200 >
Hi Yishai,
I love your patch! Yet something to improve:
[auto build test ERROR on awilliam-vfio/next]
[also build test ERROR on next-20220520]
[cannot apply to v5.18-rc7]
[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/Yishai-Hadas/vfio-Split-migration-ops-from-main-device-ops/20220522-174959
base: https://github.com/awilliam/linux-vfio.git next
config: arm64-allmodconfig (https://download.01.org/0day-ci/archive/20220523/202205230157.hD7n0wig-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 1443dbaba6f0e57be066995db9164f89fb57b413)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/f9fa522b20c805dbbb0907b0f90b2b7f1d260218
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Yishai-Hadas/vfio-Split-migration-ops-from-main-device-ops/20220522-174959
git checkout f9fa522b20c805dbbb0907b0f90b2b7f1d260218
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/vfio/pci/hisilicon/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c:1188:22: error: no member named 'migration_set_state' in 'struct vfio_device_ops'
if (core_vdev->ops->migration_set_state) {
~~~~~~~~~~~~~~ ^
1 error generated.
vim +1188 drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
6abdce51af1a21 Shameer Kolothum 2022-03-08 1176
ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1177 static int hisi_acc_vfio_pci_open_device(struct vfio_device *core_vdev)
ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1178 {
b0eed085903e77 Longfang Liu 2022-03-08 1179 struct hisi_acc_vf_core_device *hisi_acc_vdev = container_of(core_vdev,
b0eed085903e77 Longfang Liu 2022-03-08 1180 struct hisi_acc_vf_core_device, core_device.vdev);
b0eed085903e77 Longfang Liu 2022-03-08 1181 struct vfio_pci_core_device *vdev = &hisi_acc_vdev->core_device;
ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1182 int ret;
ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1183
ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1184 ret = vfio_pci_core_enable(vdev);
ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1185 if (ret)
ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1186 return ret;
ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1187
b0eed085903e77 Longfang Liu 2022-03-08 @1188 if (core_vdev->ops->migration_set_state) {
b0eed085903e77 Longfang Liu 2022-03-08 1189 ret = hisi_acc_vf_qm_init(hisi_acc_vdev);
b0eed085903e77 Longfang Liu 2022-03-08 1190 if (ret) {
b0eed085903e77 Longfang Liu 2022-03-08 1191 vfio_pci_core_disable(vdev);
b0eed085903e77 Longfang Liu 2022-03-08 1192 return ret;
b0eed085903e77 Longfang Liu 2022-03-08 1193 }
b0eed085903e77 Longfang Liu 2022-03-08 1194 hisi_acc_vdev->mig_state = VFIO_DEVICE_STATE_RUNNING;
b0eed085903e77 Longfang Liu 2022-03-08 1195 }
ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1196
b0eed085903e77 Longfang Liu 2022-03-08 1197 vfio_pci_core_finish_enable(vdev);
ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1198 return 0;
ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1199 }
ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1200
> -----Original Message----- > From: Yishai Hadas [mailto:yishaih@nvidia.com] > Sent: 22 May 2022 16:04 > To: kernel test robot <lkp@intel.com>; alex.williamson@redhat.com; > jgg@nvidia.com; kvm@vger.kernel.org; Shameerali Kolothum Thodi > <shameerali.kolothum.thodi@huawei.com> > Cc: kbuild-all@lists.01.org; maorg@nvidia.com; cohuck@redhat.com; > kevin.tian@intel.com; liulongfang <liulongfang@huawei.com> > Subject: Re: [PATCH] vfio: Split migration ops from main device ops > > > Alex, > This patch requires some extra handling in hisi driver to fix and > encapsulate the migration specific handling in a single function per op, > which at the end will call the matching vfio_pci_core_xxx function. > This won't fit to current kernel cycle as the merge window is almost > here, however your feedback on the below approach would be appreciated. > > Shameerali, > Can you please review the below and send me some matching code in your > driver ? I may put as part of V1, unless that you prefer that I'll do. Ok. I think for HiSilicon driver you require the below changes instead, --x--- diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c index 4def43f5f7b6..b42964f5d7ee 100644 --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c @@ -1185,7 +1185,7 @@ static int hisi_acc_vfio_pci_open_device(struct vfio_device *core_vdev) if (ret) return ret; - if (core_vdev->ops->migration_set_state) { + if (core_vdev->mig_ops) { ret = hisi_acc_vf_qm_init(hisi_acc_vdev); if (ret) { vfio_pci_core_disable(vdev); @@ -1208,6 +1208,11 @@ static void hisi_acc_vfio_pci_close_device(struct vfio_device *core_vdev) vfio_pci_core_close_device(core_vdev); } +static const struct vfio_migration_ops hisi_acc_vfio_pci_migrn_state_ops = { + .migration_set_state = hisi_acc_vfio_pci_set_device_state, + .migration_get_state = hisi_acc_vfio_pci_get_device_state, +}; + static const struct vfio_device_ops hisi_acc_vfio_pci_migrn_ops = { .name = "hisi-acc-vfio-pci-migration", .open_device = hisi_acc_vfio_pci_open_device, @@ -1219,8 +1224,6 @@ static const struct vfio_device_ops hisi_acc_vfio_pci_migrn_ops = { .mmap = hisi_acc_vfio_pci_mmap, .request = vfio_pci_core_request, .match = vfio_pci_core_match, - .migration_set_state = hisi_acc_vfio_pci_set_device_state, - .migration_get_state = hisi_acc_vfio_pci_get_device_state, }; static const struct vfio_device_ops hisi_acc_vfio_pci_ops = { @@ -1272,6 +1275,8 @@ static int hisi_acc_vfio_pci_probe(struct pci_dev *pdev, const struct pci_device if (!ret) { vfio_pci_core_init_device(&hisi_acc_vdev->core_device, pdev, &hisi_acc_vfio_pci_migrn_ops); + hisi_acc_vdev->core_device.vdev.mig_ops = + &hisi_acc_vfio_pci_migrn_state_ops; } else { pci_warn(pdev, "migration support failed, continue with generic interface\n"); vfio_pci_core_init_device(&hisi_acc_vdev->core_device, pdev, --x--- Thanks, Shameer > On 22/05/2022 17:21, kernel test robot wrote: > > Hi Yishai, > > > > I love your patch! Yet something to improve: > > > > [auto build test ERROR on awilliam-vfio/next] > > [cannot apply to v5.18-rc7] > > [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/Yishai-Hadas/vfio-Split-migrati > on-ops-from-main-device-ops/20220522-174959 > > base: https://github.com/awilliam/linux-vfio.git next > > config: arm64-allyesconfig > (https://download.01.org/0day-ci/archive/20220522/202205222209.5JkbCw > Da-lkp@intel.com/config) > > compiler: aarch64-linux-gcc (GCC) 11.3.0 > > reproduce (this is a W=1 build): > > wget > https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O > ~/bin/make.cross > > chmod +x ~/bin/make.cross > > # > https://github.com/intel-lab-lkp/linux/commit/f9fa522b20c805dbbb0907b0f9 > 0b2b7f1d260218 > > git remote add linux-review https://github.com/intel-lab-lkp/linux > > git fetch --no-tags linux-review > Yishai-Hadas/vfio-Split-migration-ops-from-main-device-ops/20220522-17495 > 9 > > git checkout f9fa522b20c805dbbb0907b0f90b2b7f1d260218 > > # save the config file > > mkdir build_dir && cp config build_dir/.config > > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 > make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash > > > > If you fix the issue, kindly add following tag where applicable > > Reported-by: kernel test robot <lkp@intel.com> > > > > All errors (new ones prefixed by >>): > > > > drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c: In function > 'hisi_acc_vfio_pci_open_device': > >>> drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c:1188:27: error: 'const struct > vfio_device_ops' has no member named 'migration_set_state' > > 1188 | if (core_vdev->ops->migration_set_state) { > > | ^~ > > At top level: > > drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c:1201:13: warning: > 'hisi_acc_vfio_pci_close_device' defined but not used [-Wunused-function] > > 1201 | static void hisi_acc_vfio_pci_close_device(struct vfio_device > *core_vdev) > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c:1138:13: warning: > 'hisi_acc_vfio_pci_ioctl' defined but not used [-Wunused-function] > > 1138 | static long hisi_acc_vfio_pci_ioctl(struct vfio_device > *core_vdev, unsigned int cmd, > > | ^~~~~~~~~~~~~~~~~~~~~~~ > > drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c:1124:16: warning: > 'hisi_acc_vfio_pci_read' defined but not used [-Wunused-function] > > 1124 | static ssize_t hisi_acc_vfio_pci_read(struct vfio_device > *core_vdev, > > | ^~~~~~~~~~~~~~~~~~~~~~ > > drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c:1110:16: warning: > 'hisi_acc_vfio_pci_write' defined but not used [-Wunused-function] > > 1110 | static ssize_t hisi_acc_vfio_pci_write(struct vfio_device > *core_vdev, > > | ^~~~~~~~~~~~~~~~~~~~~~~ > > drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c:1086:12: warning: > 'hisi_acc_vfio_pci_mmap' defined but not used [-Wunused-function] > > 1086 | static int hisi_acc_vfio_pci_mmap(struct vfio_device > *core_vdev, > > | ^~~~~~~~~~~~~~~~~~~~~~ > > > > > > vim +1188 drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > > > > 6abdce51af1a21 Shameer Kolothum 2022-03-08 1176 > > ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1177 static int > hisi_acc_vfio_pci_open_device(struct vfio_device *core_vdev) > > ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1178 { > > b0eed085903e77 Longfang Liu 2022-03-08 1179 struct > hisi_acc_vf_core_device *hisi_acc_vdev = container_of(core_vdev, > > b0eed085903e77 Longfang Liu 2022-03-08 1180 > struct hisi_acc_vf_core_device, core_device.vdev); > > b0eed085903e77 Longfang Liu 2022-03-08 1181 struct > vfio_pci_core_device *vdev = &hisi_acc_vdev->core_device; > > ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1182 int ret; > > ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1183 > > ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1184 ret = > vfio_pci_core_enable(vdev); > > ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1185 if (ret) > > ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1186 return > ret; > > ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1187 > > b0eed085903e77 Longfang Liu 2022-03-08 @1188 if > (core_vdev->ops->migration_set_state) { > > b0eed085903e77 Longfang Liu 2022-03-08 1189 ret = > hisi_acc_vf_qm_init(hisi_acc_vdev); > > b0eed085903e77 Longfang Liu 2022-03-08 1190 if (ret) { > > b0eed085903e77 Longfang Liu 2022-03-08 1191 > vfio_pci_core_disable(vdev); > > b0eed085903e77 Longfang Liu 2022-03-08 1192 > return ret; > > b0eed085903e77 Longfang Liu 2022-03-08 1193 } > > b0eed085903e77 Longfang Liu 2022-03-08 1194 > hisi_acc_vdev->mig_state = VFIO_DEVICE_STATE_RUNNING; > > b0eed085903e77 Longfang Liu 2022-03-08 1195 } > > ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1196 > > b0eed085903e77 Longfang Liu 2022-03-08 1197 > vfio_pci_core_finish_enable(vdev); > > ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1198 return 0; > > ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1199 } > > ee3a5b2359e0e5 Shameer Kolothum 2022-03-08 1200 > >
On Sun, 22 May 2022 12:47:56 +0300 Yishai Hadas <yishaih@nvidia.com> wrote: > vfio core checks whether the driver sets some migration op (e.g. > set_state/get_state) and accordingly calls its op. > > However, currently mlx5 driver sets the above ops without regards to its > migration caps. > > This might lead to unexpected usage/Oops if user space may call to the > above ops even if the driver doesn't support migration. As for example, > the migration state_mutex is not initialized in that case. > > The cleanest way to manage that seems to split the migration ops from > the main device ops, this will let the driver setting them separately > from the main ops when it's applicable. > > As part of that, cleaned-up HISI driver to match this scheme. > > This scheme may enable down the road to come with some extra group of > ops (e.g. DMA log) that can be set without regards to the other options > based on driver caps. It seems like the hisi-acc driver already manages this by registering different structs based on the device migration capabilities, why is that not the default solution here? Or of course the mlx5 driver could test the migration capabilities before running into the weeds. We also have vfio_device.migration_flags which could factor in here as well. Thanks, Alex > Fixes: 6fadb021266d ("vfio/mlx5: Implement vfio_pci driver for mlx5 devices") > Signed-off-by: Yishai Hadas <yishaih@nvidia.com> > --- > .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 27 +++++-------------- > drivers/vfio/pci/mlx5/cmd.c | 4 ++- > drivers/vfio/pci/mlx5/cmd.h | 3 ++- > drivers/vfio/pci/mlx5/main.c | 9 ++++--- > drivers/vfio/vfio.c | 13 ++++----- > include/linux/vfio.h | 26 +++++++++++------- > 6 files changed, 40 insertions(+), 42 deletions(-) > > diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > index e92376837b29..cfe9c8925d68 100644 > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > @@ -1208,17 +1208,7 @@ static void hisi_acc_vfio_pci_close_device(struct vfio_device *core_vdev) > vfio_pci_core_close_device(core_vdev); > } > > -static const struct vfio_device_ops hisi_acc_vfio_pci_migrn_ops = { > - .name = "hisi-acc-vfio-pci-migration", > - .open_device = hisi_acc_vfio_pci_open_device, > - .close_device = hisi_acc_vfio_pci_close_device, > - .ioctl = hisi_acc_vfio_pci_ioctl, > - .device_feature = vfio_pci_core_ioctl_feature, > - .read = hisi_acc_vfio_pci_read, > - .write = hisi_acc_vfio_pci_write, > - .mmap = hisi_acc_vfio_pci_mmap, > - .request = vfio_pci_core_request, > - .match = vfio_pci_core_match, > +static const struct vfio_migration_ops hisi_acc_vfio_pci_migrn_ops = { > .migration_set_state = hisi_acc_vfio_pci_set_device_state, > .migration_get_state = hisi_acc_vfio_pci_get_device_state, > }; > @@ -1266,20 +1256,15 @@ static int hisi_acc_vfio_pci_probe(struct pci_dev *pdev, const struct pci_device > if (!hisi_acc_vdev) > return -ENOMEM; > > + vfio_pci_core_init_device(&hisi_acc_vdev->core_device, pdev, > + &hisi_acc_vfio_pci_ops); > pf_qm = hisi_acc_get_pf_qm(pdev); > if (pf_qm && pf_qm->ver >= QM_HW_V3) { > ret = hisi_acc_vfio_pci_migrn_init(hisi_acc_vdev, pdev, pf_qm); > - if (!ret) { > - vfio_pci_core_init_device(&hisi_acc_vdev->core_device, pdev, > - &hisi_acc_vfio_pci_migrn_ops); > - } else { > + if (!ret) > + hisi_acc_vdev->core_device.vdev.mig_ops = &hisi_acc_vfio_pci_migrn_ops; > + else > pci_warn(pdev, "migration support failed, continue with generic interface\n"); > - vfio_pci_core_init_device(&hisi_acc_vdev->core_device, pdev, > - &hisi_acc_vfio_pci_ops); > - } > - } else { > - vfio_pci_core_init_device(&hisi_acc_vdev->core_device, pdev, > - &hisi_acc_vfio_pci_ops); > } > > dev_set_drvdata(&pdev->dev, &hisi_acc_vdev->core_device); > diff --git a/drivers/vfio/pci/mlx5/cmd.c b/drivers/vfio/pci/mlx5/cmd.c > index 9b9f33ca270a..334806c024b1 100644 > --- a/drivers/vfio/pci/mlx5/cmd.c > +++ b/drivers/vfio/pci/mlx5/cmd.c > @@ -98,7 +98,8 @@ void mlx5vf_cmd_remove_migratable(struct mlx5vf_pci_core_device *mvdev) > destroy_workqueue(mvdev->cb_wq); > } > > -void mlx5vf_cmd_set_migratable(struct mlx5vf_pci_core_device *mvdev) > +void mlx5vf_cmd_set_migratable(struct mlx5vf_pci_core_device *mvdev, > + const struct vfio_migration_ops *mig_ops) > { > struct pci_dev *pdev = mvdev->core_device.pdev; > int ret; > @@ -139,6 +140,7 @@ void mlx5vf_cmd_set_migratable(struct mlx5vf_pci_core_device *mvdev) > mvdev->core_device.vdev.migration_flags = > VFIO_MIGRATION_STOP_COPY | > VFIO_MIGRATION_P2P; > + mvdev->core_device.vdev.mig_ops = mig_ops; > > end: > mlx5_vf_put_core_dev(mvdev->mdev); > diff --git a/drivers/vfio/pci/mlx5/cmd.h b/drivers/vfio/pci/mlx5/cmd.h > index 6c3112fdd8b1..7b9e3d56158e 100644 > --- a/drivers/vfio/pci/mlx5/cmd.h > +++ b/drivers/vfio/pci/mlx5/cmd.h > @@ -62,7 +62,8 @@ int mlx5vf_cmd_suspend_vhca(struct mlx5vf_pci_core_device *mvdev, u16 op_mod); > int mlx5vf_cmd_resume_vhca(struct mlx5vf_pci_core_device *mvdev, u16 op_mod); > int mlx5vf_cmd_query_vhca_migration_state(struct mlx5vf_pci_core_device *mvdev, > size_t *state_size); > -void mlx5vf_cmd_set_migratable(struct mlx5vf_pci_core_device *mvdev); > +void mlx5vf_cmd_set_migratable(struct mlx5vf_pci_core_device *mvdev, > + const struct vfio_migration_ops *mig_ops); > void mlx5vf_cmd_remove_migratable(struct mlx5vf_pci_core_device *mvdev); > int mlx5vf_cmd_save_vhca_state(struct mlx5vf_pci_core_device *mvdev, > struct mlx5_vf_migration_file *migf); > diff --git a/drivers/vfio/pci/mlx5/main.c b/drivers/vfio/pci/mlx5/main.c > index dd1009b5ff9c..73998e4778c8 100644 > --- a/drivers/vfio/pci/mlx5/main.c > +++ b/drivers/vfio/pci/mlx5/main.c > @@ -574,6 +574,11 @@ static void mlx5vf_pci_close_device(struct vfio_device *core_vdev) > vfio_pci_core_close_device(core_vdev); > } > > +static const struct vfio_migration_ops mlx5vf_pci_mig_ops = { > + .migration_set_state = mlx5vf_pci_set_device_state, > + .migration_get_state = mlx5vf_pci_get_device_state, > +}; > + > static const struct vfio_device_ops mlx5vf_pci_ops = { > .name = "mlx5-vfio-pci", > .open_device = mlx5vf_pci_open_device, > @@ -585,8 +590,6 @@ static const struct vfio_device_ops mlx5vf_pci_ops = { > .mmap = vfio_pci_core_mmap, > .request = vfio_pci_core_request, > .match = vfio_pci_core_match, > - .migration_set_state = mlx5vf_pci_set_device_state, > - .migration_get_state = mlx5vf_pci_get_device_state, > }; > > static int mlx5vf_pci_probe(struct pci_dev *pdev, > @@ -599,7 +602,7 @@ static int mlx5vf_pci_probe(struct pci_dev *pdev, > if (!mvdev) > return -ENOMEM; > vfio_pci_core_init_device(&mvdev->core_device, pdev, &mlx5vf_pci_ops); > - mlx5vf_cmd_set_migratable(mvdev); > + mlx5vf_cmd_set_migratable(mvdev, &mlx5vf_pci_mig_ops); > dev_set_drvdata(&pdev->dev, &mvdev->core_device); > ret = vfio_pci_core_register_device(&mvdev->core_device); > if (ret) > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > index cfcff7764403..5bc678547f1f 100644 > --- a/drivers/vfio/vfio.c > +++ b/drivers/vfio/vfio.c > @@ -1510,8 +1510,8 @@ vfio_ioctl_device_feature_mig_device_state(struct vfio_device *device, > struct file *filp = NULL; > int ret; > > - if (!device->ops->migration_set_state || > - !device->ops->migration_get_state) > + if (!device->mig_ops->migration_set_state || > + !device->mig_ops->migration_get_state) > return -ENOTTY; > > ret = vfio_check_feature(flags, argsz, > @@ -1527,7 +1527,8 @@ vfio_ioctl_device_feature_mig_device_state(struct vfio_device *device, > if (flags & VFIO_DEVICE_FEATURE_GET) { > enum vfio_device_mig_state curr_state; > > - ret = device->ops->migration_get_state(device, &curr_state); > + ret = device->mig_ops->migration_get_state(device, > + &curr_state); > if (ret) > return ret; > mig.device_state = curr_state; > @@ -1535,7 +1536,7 @@ vfio_ioctl_device_feature_mig_device_state(struct vfio_device *device, > } > > /* Handle the VFIO_DEVICE_FEATURE_SET */ > - filp = device->ops->migration_set_state(device, mig.device_state); > + filp = device->mig_ops->migration_set_state(device, mig.device_state); > if (IS_ERR(filp) || !filp) > goto out_copy; > > @@ -1558,8 +1559,8 @@ static int vfio_ioctl_device_feature_migration(struct vfio_device *device, > }; > int ret; > > - if (!device->ops->migration_set_state || > - !device->ops->migration_get_state) > + if (!device->mig_ops->migration_set_state || > + !device->mig_ops->migration_get_state) > return -ENOTTY; > > ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_GET, > diff --git a/include/linux/vfio.h b/include/linux/vfio.h > index 45b287826ce6..1a1f61803742 100644 > --- a/include/linux/vfio.h > +++ b/include/linux/vfio.h > @@ -32,6 +32,7 @@ struct vfio_device_set { > struct vfio_device { > struct device *dev; > const struct vfio_device_ops *ops; > + const struct vfio_migration_ops *mig_ops; > struct vfio_group *group; > struct vfio_device_set *dev_set; > struct list_head dev_set_list; > @@ -59,16 +60,6 @@ struct vfio_device { > * match, -errno for abort (ex. match with insufficient or incorrect > * additional args) > * @device_feature: Optional, fill in the VFIO_DEVICE_FEATURE ioctl > - * @migration_set_state: Optional callback to change the migration state for > - * devices that support migration. It's mandatory for > - * VFIO_DEVICE_FEATURE_MIGRATION migration support. > - * The returned FD is used for data transfer according to the FSM > - * definition. The driver is responsible to ensure that FD reaches end > - * of stream or error whenever the migration FSM leaves a data transfer > - * state or before close_device() returns. > - * @migration_get_state: Optional callback to get the migration state for > - * devices that support migration. It's mandatory for > - * VFIO_DEVICE_FEATURE_MIGRATION migration support. > */ > struct vfio_device_ops { > char *name; > @@ -85,6 +76,21 @@ struct vfio_device_ops { > int (*match)(struct vfio_device *vdev, char *buf); > int (*device_feature)(struct vfio_device *device, u32 flags, > void __user *arg, size_t argsz); > +}; > + > +/** > + * @migration_set_state: Optional callback to change the migration state for > + * devices that support migration. It's mandatory for > + * VFIO_DEVICE_FEATURE_MIGRATION migration support. > + * The returned FD is used for data transfer according to the FSM > + * definition. The driver is responsible to ensure that FD reaches end > + * of stream or error whenever the migration FSM leaves a data transfer > + * state or before close_device() returns. > + * @migration_get_state: Optional callback to get the migration state for > + * devices that support migration. It's mandatory for > + * VFIO_DEVICE_FEATURE_MIGRATION migration support. > + */ > +struct vfio_migration_ops { > struct file *(*migration_set_state)( > struct vfio_device *device, > enum vfio_device_mig_state new_state);
On Mon, May 23, 2022 at 11:25:00AM -0600, Alex Williamson wrote: > On Sun, 22 May 2022 12:47:56 +0300 > Yishai Hadas <yishaih@nvidia.com> wrote: > > > vfio core checks whether the driver sets some migration op (e.g. > > set_state/get_state) and accordingly calls its op. > > > > However, currently mlx5 driver sets the above ops without regards to its > > migration caps. > > > > This might lead to unexpected usage/Oops if user space may call to the > > above ops even if the driver doesn't support migration. As for example, > > the migration state_mutex is not initialized in that case. > > > > The cleanest way to manage that seems to split the migration ops from > > the main device ops, this will let the driver setting them separately > > from the main ops when it's applicable. > > > > As part of that, cleaned-up HISI driver to match this scheme. > > > > This scheme may enable down the road to come with some extra group of > > ops (e.g. DMA log) that can be set without regards to the other options > > based on driver caps. > > It seems like the hisi-acc driver already manages this by registering > different structs based on the device migration capabilities, why is > that not the default solution here? Or of course the mlx5 driver could > test the migration capabilities before running into the weeds. We also > have vfio_device.migration_flags which could factor in here as well. It starts to hit combinatoral explosion when the next patches add ops for dirty logging that may be optional too. This is simpler and simpifies the hisi driver to remove the 2nd ops too. Jason
> -----Original Message----- > From: Jason Gunthorpe [mailto:jgg@nvidia.com] > Sent: 24 May 2022 00:28 > To: Alex Williamson <alex.williamson@redhat.com> > Cc: Yishai Hadas <yishaih@nvidia.com>; kvm@vger.kernel.org; > maorg@nvidia.com; cohuck@redhat.com; kevin.tian@intel.com; Shameerali > Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; liulongfang > <liulongfang@huawei.com> > Subject: Re: [PATCH] vfio: Split migration ops from main device ops > > On Mon, May 23, 2022 at 11:25:00AM -0600, Alex Williamson wrote: > > On Sun, 22 May 2022 12:47:56 +0300 > > Yishai Hadas <yishaih@nvidia.com> wrote: > > > > > vfio core checks whether the driver sets some migration op (e.g. > > > set_state/get_state) and accordingly calls its op. > > > > > > However, currently mlx5 driver sets the above ops without regards to > > > its migration caps. > > > > > > This might lead to unexpected usage/Oops if user space may call to > > > the above ops even if the driver doesn't support migration. As for > > > example, the migration state_mutex is not initialized in that case. > > > > > > The cleanest way to manage that seems to split the migration ops > > > from the main device ops, this will let the driver setting them > > > separately from the main ops when it's applicable. > > > > > > As part of that, cleaned-up HISI driver to match this scheme. > > > > > > This scheme may enable down the road to come with some extra group > > > of ops (e.g. DMA log) that can be set without regards to the other > > > options based on driver caps. > > > > It seems like the hisi-acc driver already manages this by registering > > different structs based on the device migration capabilities, why is > > that not the default solution here? Or of course the mlx5 driver > > could test the migration capabilities before running into the weeds. > > We also have vfio_device.migration_flags which could factor in here as well. > > It starts to hit combinatoral explosion when the next patches add ops for dirty > logging that may be optional too. This is simpler and simpifies the hisi driver to > remove the 2nd ops too. Hmm..but for hisi driver we still need to have those mmap/ioctl/read/write override functions to restrict the BAR region exposure to Guest in case of migration support. So I am not sure we can get rid of two ops easily there(Though, we could add an explicit check for the migration support in those callbacks). Thanks, Shameer
diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c index e92376837b29..cfe9c8925d68 100644 --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c @@ -1208,17 +1208,7 @@ static void hisi_acc_vfio_pci_close_device(struct vfio_device *core_vdev) vfio_pci_core_close_device(core_vdev); } -static const struct vfio_device_ops hisi_acc_vfio_pci_migrn_ops = { - .name = "hisi-acc-vfio-pci-migration", - .open_device = hisi_acc_vfio_pci_open_device, - .close_device = hisi_acc_vfio_pci_close_device, - .ioctl = hisi_acc_vfio_pci_ioctl, - .device_feature = vfio_pci_core_ioctl_feature, - .read = hisi_acc_vfio_pci_read, - .write = hisi_acc_vfio_pci_write, - .mmap = hisi_acc_vfio_pci_mmap, - .request = vfio_pci_core_request, - .match = vfio_pci_core_match, +static const struct vfio_migration_ops hisi_acc_vfio_pci_migrn_ops = { .migration_set_state = hisi_acc_vfio_pci_set_device_state, .migration_get_state = hisi_acc_vfio_pci_get_device_state, }; @@ -1266,20 +1256,15 @@ static int hisi_acc_vfio_pci_probe(struct pci_dev *pdev, const struct pci_device if (!hisi_acc_vdev) return -ENOMEM; + vfio_pci_core_init_device(&hisi_acc_vdev->core_device, pdev, + &hisi_acc_vfio_pci_ops); pf_qm = hisi_acc_get_pf_qm(pdev); if (pf_qm && pf_qm->ver >= QM_HW_V3) { ret = hisi_acc_vfio_pci_migrn_init(hisi_acc_vdev, pdev, pf_qm); - if (!ret) { - vfio_pci_core_init_device(&hisi_acc_vdev->core_device, pdev, - &hisi_acc_vfio_pci_migrn_ops); - } else { + if (!ret) + hisi_acc_vdev->core_device.vdev.mig_ops = &hisi_acc_vfio_pci_migrn_ops; + else pci_warn(pdev, "migration support failed, continue with generic interface\n"); - vfio_pci_core_init_device(&hisi_acc_vdev->core_device, pdev, - &hisi_acc_vfio_pci_ops); - } - } else { - vfio_pci_core_init_device(&hisi_acc_vdev->core_device, pdev, - &hisi_acc_vfio_pci_ops); } dev_set_drvdata(&pdev->dev, &hisi_acc_vdev->core_device); diff --git a/drivers/vfio/pci/mlx5/cmd.c b/drivers/vfio/pci/mlx5/cmd.c index 9b9f33ca270a..334806c024b1 100644 --- a/drivers/vfio/pci/mlx5/cmd.c +++ b/drivers/vfio/pci/mlx5/cmd.c @@ -98,7 +98,8 @@ void mlx5vf_cmd_remove_migratable(struct mlx5vf_pci_core_device *mvdev) destroy_workqueue(mvdev->cb_wq); } -void mlx5vf_cmd_set_migratable(struct mlx5vf_pci_core_device *mvdev) +void mlx5vf_cmd_set_migratable(struct mlx5vf_pci_core_device *mvdev, + const struct vfio_migration_ops *mig_ops) { struct pci_dev *pdev = mvdev->core_device.pdev; int ret; @@ -139,6 +140,7 @@ void mlx5vf_cmd_set_migratable(struct mlx5vf_pci_core_device *mvdev) mvdev->core_device.vdev.migration_flags = VFIO_MIGRATION_STOP_COPY | VFIO_MIGRATION_P2P; + mvdev->core_device.vdev.mig_ops = mig_ops; end: mlx5_vf_put_core_dev(mvdev->mdev); diff --git a/drivers/vfio/pci/mlx5/cmd.h b/drivers/vfio/pci/mlx5/cmd.h index 6c3112fdd8b1..7b9e3d56158e 100644 --- a/drivers/vfio/pci/mlx5/cmd.h +++ b/drivers/vfio/pci/mlx5/cmd.h @@ -62,7 +62,8 @@ int mlx5vf_cmd_suspend_vhca(struct mlx5vf_pci_core_device *mvdev, u16 op_mod); int mlx5vf_cmd_resume_vhca(struct mlx5vf_pci_core_device *mvdev, u16 op_mod); int mlx5vf_cmd_query_vhca_migration_state(struct mlx5vf_pci_core_device *mvdev, size_t *state_size); -void mlx5vf_cmd_set_migratable(struct mlx5vf_pci_core_device *mvdev); +void mlx5vf_cmd_set_migratable(struct mlx5vf_pci_core_device *mvdev, + const struct vfio_migration_ops *mig_ops); void mlx5vf_cmd_remove_migratable(struct mlx5vf_pci_core_device *mvdev); int mlx5vf_cmd_save_vhca_state(struct mlx5vf_pci_core_device *mvdev, struct mlx5_vf_migration_file *migf); diff --git a/drivers/vfio/pci/mlx5/main.c b/drivers/vfio/pci/mlx5/main.c index dd1009b5ff9c..73998e4778c8 100644 --- a/drivers/vfio/pci/mlx5/main.c +++ b/drivers/vfio/pci/mlx5/main.c @@ -574,6 +574,11 @@ static void mlx5vf_pci_close_device(struct vfio_device *core_vdev) vfio_pci_core_close_device(core_vdev); } +static const struct vfio_migration_ops mlx5vf_pci_mig_ops = { + .migration_set_state = mlx5vf_pci_set_device_state, + .migration_get_state = mlx5vf_pci_get_device_state, +}; + static const struct vfio_device_ops mlx5vf_pci_ops = { .name = "mlx5-vfio-pci", .open_device = mlx5vf_pci_open_device, @@ -585,8 +590,6 @@ static const struct vfio_device_ops mlx5vf_pci_ops = { .mmap = vfio_pci_core_mmap, .request = vfio_pci_core_request, .match = vfio_pci_core_match, - .migration_set_state = mlx5vf_pci_set_device_state, - .migration_get_state = mlx5vf_pci_get_device_state, }; static int mlx5vf_pci_probe(struct pci_dev *pdev, @@ -599,7 +602,7 @@ static int mlx5vf_pci_probe(struct pci_dev *pdev, if (!mvdev) return -ENOMEM; vfio_pci_core_init_device(&mvdev->core_device, pdev, &mlx5vf_pci_ops); - mlx5vf_cmd_set_migratable(mvdev); + mlx5vf_cmd_set_migratable(mvdev, &mlx5vf_pci_mig_ops); dev_set_drvdata(&pdev->dev, &mvdev->core_device); ret = vfio_pci_core_register_device(&mvdev->core_device); if (ret) diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c index cfcff7764403..5bc678547f1f 100644 --- a/drivers/vfio/vfio.c +++ b/drivers/vfio/vfio.c @@ -1510,8 +1510,8 @@ vfio_ioctl_device_feature_mig_device_state(struct vfio_device *device, struct file *filp = NULL; int ret; - if (!device->ops->migration_set_state || - !device->ops->migration_get_state) + if (!device->mig_ops->migration_set_state || + !device->mig_ops->migration_get_state) return -ENOTTY; ret = vfio_check_feature(flags, argsz, @@ -1527,7 +1527,8 @@ vfio_ioctl_device_feature_mig_device_state(struct vfio_device *device, if (flags & VFIO_DEVICE_FEATURE_GET) { enum vfio_device_mig_state curr_state; - ret = device->ops->migration_get_state(device, &curr_state); + ret = device->mig_ops->migration_get_state(device, + &curr_state); if (ret) return ret; mig.device_state = curr_state; @@ -1535,7 +1536,7 @@ vfio_ioctl_device_feature_mig_device_state(struct vfio_device *device, } /* Handle the VFIO_DEVICE_FEATURE_SET */ - filp = device->ops->migration_set_state(device, mig.device_state); + filp = device->mig_ops->migration_set_state(device, mig.device_state); if (IS_ERR(filp) || !filp) goto out_copy; @@ -1558,8 +1559,8 @@ static int vfio_ioctl_device_feature_migration(struct vfio_device *device, }; int ret; - if (!device->ops->migration_set_state || - !device->ops->migration_get_state) + if (!device->mig_ops->migration_set_state || + !device->mig_ops->migration_get_state) return -ENOTTY; ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_GET, diff --git a/include/linux/vfio.h b/include/linux/vfio.h index 45b287826ce6..1a1f61803742 100644 --- a/include/linux/vfio.h +++ b/include/linux/vfio.h @@ -32,6 +32,7 @@ struct vfio_device_set { struct vfio_device { struct device *dev; const struct vfio_device_ops *ops; + const struct vfio_migration_ops *mig_ops; struct vfio_group *group; struct vfio_device_set *dev_set; struct list_head dev_set_list; @@ -59,16 +60,6 @@ struct vfio_device { * match, -errno for abort (ex. match with insufficient or incorrect * additional args) * @device_feature: Optional, fill in the VFIO_DEVICE_FEATURE ioctl - * @migration_set_state: Optional callback to change the migration state for - * devices that support migration. It's mandatory for - * VFIO_DEVICE_FEATURE_MIGRATION migration support. - * The returned FD is used for data transfer according to the FSM - * definition. The driver is responsible to ensure that FD reaches end - * of stream or error whenever the migration FSM leaves a data transfer - * state or before close_device() returns. - * @migration_get_state: Optional callback to get the migration state for - * devices that support migration. It's mandatory for - * VFIO_DEVICE_FEATURE_MIGRATION migration support. */ struct vfio_device_ops { char *name; @@ -85,6 +76,21 @@ struct vfio_device_ops { int (*match)(struct vfio_device *vdev, char *buf); int (*device_feature)(struct vfio_device *device, u32 flags, void __user *arg, size_t argsz); +}; + +/** + * @migration_set_state: Optional callback to change the migration state for + * devices that support migration. It's mandatory for + * VFIO_DEVICE_FEATURE_MIGRATION migration support. + * The returned FD is used for data transfer according to the FSM + * definition. The driver is responsible to ensure that FD reaches end + * of stream or error whenever the migration FSM leaves a data transfer + * state or before close_device() returns. + * @migration_get_state: Optional callback to get the migration state for + * devices that support migration. It's mandatory for + * VFIO_DEVICE_FEATURE_MIGRATION migration support. + */ +struct vfio_migration_ops { struct file *(*migration_set_state)( struct vfio_device *device, enum vfio_device_mig_state new_state);
vfio core checks whether the driver sets some migration op (e.g. set_state/get_state) and accordingly calls its op. However, currently mlx5 driver sets the above ops without regards to its migration caps. This might lead to unexpected usage/Oops if user space may call to the above ops even if the driver doesn't support migration. As for example, the migration state_mutex is not initialized in that case. The cleanest way to manage that seems to split the migration ops from the main device ops, this will let the driver setting them separately from the main ops when it's applicable. As part of that, cleaned-up HISI driver to match this scheme. This scheme may enable down the road to come with some extra group of ops (e.g. DMA log) that can be set without regards to the other options based on driver caps. Fixes: 6fadb021266d ("vfio/mlx5: Implement vfio_pci driver for mlx5 devices") Signed-off-by: Yishai Hadas <yishaih@nvidia.com> --- .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 27 +++++-------------- drivers/vfio/pci/mlx5/cmd.c | 4 ++- drivers/vfio/pci/mlx5/cmd.h | 3 ++- drivers/vfio/pci/mlx5/main.c | 9 ++++--- drivers/vfio/vfio.c | 13 ++++----- include/linux/vfio.h | 26 +++++++++++------- 6 files changed, 40 insertions(+), 42 deletions(-)