RE: [PATCH] vfio: Split migration ops from main device ops

[Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>]

> -----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
> >