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

[Keith Busch <kbusch@kernel.org>]

On Wed, Nov 13, 2024 at 05:58:12AM +0100, Christoph Hellwig wrote:
> 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?

Ack on all suggestions.