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

[Leon Romanovsky <leon@kernel.org>]

On Mon, Jul 05, 2021 at 10:18:59AM +0000, Shameerali Kolothum Thodi wrote:
> 
> 
> > -----Original Message-----
> > From: Max Gurtovoy [mailto:mgurtovoy@nvidia.com]
> > Sent: 05 July 2021 10:42
> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> > Leon Romanovsky <leon@kernel.org>
> > Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org;
> > linux-crypto@vger.kernel.org; alex.williamson@redhat.com; jgg@nvidia.com;
> > Linuxarm <linuxarm@huawei.com>; liulongfang <liulongfang@huawei.com>;
> > Zengtao (B) <prime.zeng@hisilicon.com>; yuzenghui
> > <yuzenghui@huawei.com>; Jonathan Cameron
> > <jonathan.cameron@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>
> > 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:
> > >
> > >> -----Original Message-----
> > >> From: Leon Romanovsky [mailto:leon@kernel.org]
> > >> Sent: 04 July 2021 08:04
> > >> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > >> Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org;
> > >> linux-crypto@vger.kernel.org; alex.williamson@redhat.com;
> > jgg@nvidia.com;
> > >> mgurtovoy@nvidia.com; Linuxarm <linuxarm@huawei.com>; liulongfang
> > >> <liulongfang@huawei.com>; Zengtao (B) <prime.zeng@hisilicon.com>;
> > >> yuzenghui <yuzenghui@huawei.com>; Jonathan Cameron
> > >> <jonathan.cameron@huawei.com>; Wangzhou (B)
> > <wangzhou1@hisilicon.com>
> > >> Subject: Re: [RFC v2 1/4] hisi-acc-vfio-pci: add new vfio_pci driver for
> > HiSilicon
> > >> ACC devices
> > >>
> > >> On Fri, Jul 02, 2021 at 10:58:46AM +0100, Shameer Kolothum wrote:
> > >>> 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
> > >> <shameerali.kolothum.thodi@huawei.com>
> > >>> ---
> > >>>   drivers/vfio/pci/Kconfig             |   9 +++
> > >>>   drivers/vfio/pci/Makefile            |   2 +
> > >>>   drivers/vfio/pci/hisi_acc_vfio_pci.c | 100
> > +++++++++++++++++++++++++++
> > >>>   3 files changed, 111 insertions(+)
> > >>>   create mode 100644 drivers/vfio/pci/hisi_acc_vfio_pci.c
> > >> <...>
> > >>
> > >>> +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
> > >> 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
> > > 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 
> 
>  
> > 
> > 
> > >
> > > Thanks,
> > > Shameer