Re: [PATCHv2 1/2] nvme-pci: add support for sgl metadata

[Christoph Hellwig <hch@lst.de>]

On Tue, Nov 12, 2024 at 01:06:19PM -0800, Keith Busch wrote:
> Supporting this mode allows merging requests with metadata that wouldn't
> be possible otherwise, and creating user space requests that straddle
> physically discontiguous pages.

Not just merging, but also creating :)  I.e. it allows to transfer
multiple non-contiguous metadata segements.

> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index e119ba0f8ab8b..12e5064b9cba0 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1985,6 +1985,9 @@ static void nvme_set_ctrl_limits(struct nvme_ctrl *ctrl,
>  	lim->max_segments = min_t(u32, USHRT_MAX,
>  		min_not_zero(nvme_max_drv_segments(ctrl), ctrl->max_segments));
>  	lim->max_integrity_segments = ctrl->max_integrity_segments;
> +	if (lim->max_integrity_segments > 1 &&
> +	    !nvme_ctrl_meta_sgl_supported(ctrl))
> +		lim->max_integrity_segments = 1;

Despite the general mess about the SGLS field in the nvme spec, the meta
sgls bit really is a PCIe-only feasture, and this will break metadata
support on RDMA.  The nvme_ctrl_meta_sgl_supported needs to be in pci.c
to set ctrl->max_integrity_segments based on it.

> +static inline bool nvme_ctrl_meta_sgl_supported(struct nvme_ctrl *ctrl)
> +{
> +	return ctrl->sgls & NVME_CTRL_SGLS_MPTR;
> +}

.. and thus I'd move this to pci.c (or just drop the helper).

> +	NVME_CTRL_SGLS_MPTR                     = 1 << 19,

Maybe give the other fields in SGLS a name as well?