Re: [RFC v2 1/4] hisi-acc-vfio-pci: add new vfio_pci driver for HiSilicon ACC devices

On Mon, Jul 05, 2021 at 10:18:59AM +0000, Shameerali Kolothum Thodi wrote:

quoted

-----Original Message-----
From: Max Gurtovoy [mailto:mgurtovoy@nvidia.com]
Sent: 05 July 2021 10:42
To: Shameerali Kolothum Thodi <redacted>;
Leon Romanovsky [off-list ref]
Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org;
linux-crypto@vger.kernel.org; alex.williamson@redhat.com; jgg@nvidia.com;
Linuxarm [off-list ref]; liulongfang [off-list ref];
Zengtao (B) [off-list ref]; yuzenghui
[off-list ref]; Jonathan Cameron
[off-list ref]; Wangzhou (B) [off-list ref]
Subject: Re: [RFC v2 1/4] hisi-acc-vfio-pci: add new vfio_pci driver for HiSilicon
ACC devices


On 7/5/2021 11:47 AM, Shameerali Kolothum Thodi wrote:

quoted

quoted

-----Original Message-----
From: Leon Romanovsky [mailto:leon@kernel.org]
Sent: 04 July 2021 08:04
To: Shameerali Kolothum Thodi <redacted>
Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org;
linux-crypto@vger.kernel.org; alex.williamson@redhat.com;

jgg@nvidia.com;

quoted

quoted

mgurtovoy@nvidia.com; Linuxarm [off-list ref]; liulongfang
[off-list ref]; Zengtao (B) [off-list ref];
yuzenghui [off-list ref]; Jonathan Cameron
[off-list ref]; Wangzhou (B)

[off-list ref]

quoted

quoted

Subject: Re: [RFC v2 1/4] hisi-acc-vfio-pci: add new vfio_pci driver for

HiSilicon

quoted

quoted

ACC devices

On Fri, Jul 02, 2021 at 10:58:46AM +0100, Shameer Kolothum wrote:

quoted

Add a vendor-specific vfio_pci driver for HiSilicon ACC devices.
This will be extended in follow-up patches to add support for
vfio live migration feature.

Signed-off-by: Shameer Kolothum

[off-list ref]

quoted

---
  drivers/vfio/pci/Kconfig             |   9 +++
  drivers/vfio/pci/Makefile            |   2 +
  drivers/vfio/pci/hisi_acc_vfio_pci.c | 100

+++++++++++++++++++++++++++

quoted

quoted

quoted

  3 files changed, 111 insertions(+)
  create mode 100644 drivers/vfio/pci/hisi_acc_vfio_pci.c

<...>

quoted

+static const struct vfio_device_ops hisi_acc_vfio_pci_ops = {
+	.name		= "hisi-acc-vfio-pci",
+	.open		= hisi_acc_vfio_pci_open,
+	.release	= vfio_pci_core_release,
+	.ioctl		= vfio_pci_core_ioctl,
+	.read		= vfio_pci_core_read,
+	.write		= vfio_pci_core_write,
+	.mmap		= vfio_pci_core_mmap,
+	.request	= vfio_pci_core_request,
+	.match		= vfio_pci_core_match,
+	.reflck_attach	= vfio_pci_core_reflck_attach,

I don't remember what was proposed in vfio-pci-core conversion patches,
but would expect that default behaviour is to fallback to vfio_pci_core_*

API

quoted

quoted

if ".release/.ioctl/e.t.c" are not redefined.

Yes, that would be nice, but don't think it does that in latest(v4).

Hi Max,
Could we please consider fall back to the core defaults, may be check and

assign defaults

quoted

in vfio_pci_core_register_device() ?

I don't see why we should do this.

vfio_pci_core.ko is just a library driver. It shouldn't decide for the
vendor driver ops.

If a vendor driver would like to use its helper functions - great.

If it wants to override it - great.

If it wants to leave some op as NULL - it can do it also.

Based on the documentation of the vfio_device_ops callbacks,
It looks like we already have a precedence in the case of reflck_attach
callback where it uses the vfio core default one, if it is not implemented.

The reflck_attach pattern is pretty common pattern in the kernel to provide fallback.

Also I would imagine that in majority use cases the vendor drivers will be
defaulting to core functions. 

Right, this is whole idea of having core functionality in one place, if
vendor wants/needs, he will overwrite.

I think, in any case, it would be good to update the Documentation based on
which way we end up doing this.

The request to update Documentation can be seen as an example of
choosing not-good API decisions. Expectation to see all drivers to
use same callbacks with same vfio-core function calls sounds strange
to me.

Thanks

Thanks,
Shameer 

 

quoted

quoted

Thanks,
Shameer