Received-SPF: pass (google.com: domain of linux-crypto+bounces-3687-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 18 Apr 2024 16:54:34 -0600
From: Alex Williamson <alex.williamson@redhat.com>
To: Xin Zeng <xin.zeng@intel.com>
Cc: herbert@gondor.apana.org.au, 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,
 Yahui Cao <yahui.cao@intel.com>
Subject: Re: [PATCH v6 1/1] vfio/qat: Add vfio_pci driver for Intel QAT
 SR-IOV VF devices
Message-ID: <20240418165434.1da52cf0.alex.williamson@redhat.com>
In-Reply-To: <20240417143141.1909824-2-xin.zeng@intel.com>
References: <20240417143141.1909824-1-xin.zeng@intel.com>
	<20240417143141.1909824-2-xin.zeng@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: quoted-printable

On Wed, 17 Apr 2024 22:31:41 +0800
Xin Zeng <xin.zeng@intel.com> wrote:

> Add vfio pci variant driver for Intel QAT SR-IOV VF devices. This driver
> registers to the vfio subsystem through the interfaces exposed by the
> susbsystem. It follows the live migration protocol v2 defined in
> uapi/linux/vfio.h and interacts with Intel QAT PF driver through a set
> of interfaces defined in qat/qat_mig_dev.h to support live migration of
> Intel QAT VF devices.

=46rom here down could actually just be a comment towards the top of the
driver.

> The migration data of each Intel QAT GEN4 VF device is encapsulated into
> a 4096 bytes block. The data consists of two parts.
>=20
> The first is a pre-configured set of attributes of the VF being migrated,
> which are only set when it is created. This can be migrated during pre-co=
py
> stage and used for a device compatibility check.
>=20
> The second is the VF state. This includes the required MMIO regions and
> the shadow states maintained by the QAT PF driver. This part can only be
> saved when the VF is fully quiesced and be migrated during stop-copy stag=
e.
>=20
> Both these 2 parts of data are saved in hierarchical structures including
> a preamble section and several raw state sections.
>=20
> When the pre-configured part of the migration data is fully retrieved from
> user space, the preamble section are used to validate the correctness of
> the data blocks and check the version compatibility. The raw state
> sections are then used to do a device compatibility check.
>=20
> When the device transits from RESUMING state, the VF states are extracted
> from the raw state sections of the VF state part of the migration data and
> then loaded into the device.
>=20
> This version only covers migration for Intel QAT GEN4 VF devices.
>=20
> Co-developed-by: Yahui Cao <yahui.cao@intel.com>
> Signed-off-by: Yahui Cao <yahui.cao@intel.com>
> Signed-off-by: Xin Zeng <xin.zeng@intel.com>
> Reviewed-by: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> ---
>  MAINTAINERS                   |   8 +
>  drivers/vfio/pci/Kconfig      |   2 +
>  drivers/vfio/pci/Makefile     |   2 +
>  drivers/vfio/pci/qat/Kconfig  |  12 +
>  drivers/vfio/pci/qat/Makefile |   3 +
>  drivers/vfio/pci/qat/main.c   | 679 ++++++++++++++++++++++++++++++++++
>  6 files changed, 706 insertions(+)
>  create mode 100644 drivers/vfio/pci/qat/Kconfig
>  create mode 100644 drivers/vfio/pci/qat/Makefile
>  create mode 100644 drivers/vfio/pci/qat/main.c
...
> +static struct file *qat_vf_pci_step_device_state(struct qat_vf_core_devi=
ce *qat_vdev, u32 new)
> +{
> +	u32 cur =3D qat_vdev->mig_state;
> +	int ret;
> +
> +	/*
> +	 * As the device is not capable of just stopping P2P DMAs, suspend the
> +	 * device completely once any of the P2P states are reached.
> +	 * On the opposite direction, resume the device after transiting from
> +	 * the P2P state.
> +	 */
> +	if ((cur =3D=3D VFIO_DEVICE_STATE_RUNNING && new =3D=3D VFIO_DEVICE_STA=
TE_RUNNING_P2P) ||
> +	    (cur =3D=3D VFIO_DEVICE_STATE_PRE_COPY && new =3D=3D VFIO_DEVICE_ST=
ATE_PRE_COPY_P2P)) {
> +		ret =3D qat_vfmig_suspend(qat_vdev->mdev);
> +		if (ret)
> +			return ERR_PTR(ret);
> +		return NULL;
> +	}

This doesn't appear to be a valid way to support P2P, the P2P states
are defined as running states.  The guest driver may legitimately
access and modify the device state during P2P states.  Should this
device be advertising support for P2P?  Thanks,

Alex