Re: [PATCH v5 00/10] crypto: qat - enable QAT GEN4 SRIOV VF live migration for QAT GEN4

From: Alex Williamson <alex.williamson@redhat.com>
To: "Cabiddu, Giovanni" <giovanni.cabiddu@intel.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>,
	Xin Zeng <xin.zeng@intel.com>, <jgg@nvidia.com>,
	<yishaih@nvidia.com>, <shameerali.kolothum.thodi@huawei.com>,
	<kevin.tian@intel.com>, <linux-crypto@vger.kernel.org>,
	<kvm@vger.kernel.org>, <qat-linux@intel.com>
Subject: Re: [PATCH v5 00/10] crypto: qat - enable QAT GEN4 SRIOV VF live migration for QAT GEN4
Date: Fri, 12 Apr 2024 16:59:59 -0600	[thread overview]
Message-ID: <20240412165959.4b7aefad.alex.williamson@redhat.com> (raw)
In-Reply-To: <ZhlC4lWg1ExOuNnl@gcabiddu-mobl.ger.corp.intel.com>

On Fri, 12 Apr 2024 15:19:14 +0100
"Cabiddu, Giovanni" <giovanni.cabiddu@intel.com> wrote:

> Hi Alex,
> 
> On Tue, Apr 02, 2024 at 10:52:06AM +0800, Herbert Xu wrote:
> > On Thu, Mar 28, 2024 at 09:03:49AM -0600, Alex Williamson wrote:  
> > >
> > > Would you mind making a branch available for those in anticipation of
> > > the qat vfio variant driver itself being merged through the vfio tree?
> > > Thanks,  
> > 
> > OK, I've just pushed out a vfio branch.  Please take a look to
> > see if I messed anything up.  
> What are the next steps here?
> 
> Shall we re-send the patch `vfio/qat: Add vfio_pci driver for Intel QAT
> VF devices` rebased against vfio-next?
> Or, wait for you to merge the branch from Herbert, then rebase and re-send?
> Or, are you going to take the patch that was sent to the mailing list as is
> and handle the rebase? (There is only a small conflict to sort on the
> makefiles).

Hi Giovanni,

The code itself looks fine to me, the Makefile conflict is trivial,
MAINTAINERS also requires a trivial re-ordering to keep it alphabetical
now that virtio-vfio-pci is merged.  The only thing I spot that could
use some attention is the documentation, where our acceptance criteria
requests:

  Additionally, drivers should make an attempt to provide sufficient
  documentation for reviewers to understand the device specific
  extensions, for example in the case of migration data, how is the
  device state composed and consumed, which portions are not otherwise
  available to the user via vfio-pci, what safeguards exist to validate
  the data, etc.

A lot of the code here is very similar in flow to the other migration
drivers, but I think it would be good to address some of the topics
above in comments throughout the driver.  For example, how does the
driver address P2P states, what information is provided in PRE_COPY,
how is versioning handled, is user sensitive data included in the
device migration data, typical ranges of device migration data size,
etc.

Kevin might have an edge in understanding the theory of operation
here already and documenting the interesting aspects of the driver in
comments might drive a little more engagement.  Thanks,

Alex