linux-kernel - Re: [PATCH 12/16] nvme-pci: Check DMA ops when indicating support for PCI P2PDMA

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 2 May 2021 18:29:29 -0700
From:   John Hubbard <jhubbard@...dia.com>
To:     Logan Gunthorpe <logang@...tatee.com>,
        <linux-kernel@...r.kernel.org>, <linux-nvme@...ts.infradead.org>,
        <linux-block@...r.kernel.org>, <linux-pci@...r.kernel.org>,
        <linux-mm@...ck.org>, <iommu@...ts.linux-foundation.org>
CC:     Stephen Bates <sbates@...thlin.com>,
        Christoph Hellwig <hch@....de>,
        Dan Williams <dan.j.williams@...el.com>,
        Jason Gunthorpe <jgg@...pe.ca>,
        Christian König <christian.koenig@....com>,
        Don Dutile <ddutile@...hat.com>,
        Matthew Wilcox <willy@...radead.org>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Jakowski Andrzej <andrzej.jakowski@...el.com>,
        Minturn Dave B <dave.b.minturn@...el.com>,
        Jason Ekstrand <jason@...kstrand.net>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Xiong Jianxin <jianxin.xiong@...el.com>,
        Bjorn Helgaas <helgaas@...nel.org>,
        Ira Weiny <ira.weiny@...el.com>,
        Robin Murphy <robin.murphy@....com>
Subject: Re: [PATCH 12/16] nvme-pci: Check DMA ops when indicating support for
 PCI P2PDMA

On 4/8/21 10:01 AM, Logan Gunthorpe wrote:
> Introduce a supports_pci_p2pdma() operation in nvme_ctrl_ops to
> replace the fixed NVME_F_PCI_P2PDMA flag such that the dma_map_ops
> flags can be checked for PCI P2PDMA support.
> 
> Signed-off-by: Logan Gunthorpe <logang@...tatee.com>
> ---
>   drivers/nvme/host/core.c |  3 ++-
>   drivers/nvme/host/nvme.h |  2 +-
>   drivers/nvme/host/pci.c  | 11 +++++++++--
>   3 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 0896e21642be..223419454516 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3907,7 +3907,8 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
>   		blk_queue_flag_set(QUEUE_FLAG_STABLE_WRITES, ns->queue);
>   
>   	blk_queue_flag_set(QUEUE_FLAG_NONROT, ns->queue);
> -	if (ctrl->ops->flags & NVME_F_PCI_P2PDMA)
> +	if (ctrl->ops->supports_pci_p2pdma &&
> +	    ctrl->ops->supports_pci_p2pdma(ctrl))

This is a little excessive, as I suspected. How about providing a
default .supports_pci_p2pdma routine that returns false, so that
the op is always available (non-null)? By "default", maybe that
means either requiring an init_the_ops_struct() routine to be
used, and/or checking all the users of struct nvme_ctrl_ops.

Another idea: maybe you don't really need a bool .supports_pci_p2pdma()
routine at all, because the existing .flags really is about right.
You just need the flags to be filled in dynamically. So, do that
during nvme_pci setup/init time: that's when this module would call
dma_pci_p2pdma_supported().

Actually, I think that second idea simplifies things quite a
bit, but only if it's possible. I haven't worked through the
startup order of calls in nvme_pci.

thanks,
-- 
John Hubbard
NVIDIA

>   		blk_queue_flag_set(QUEUE_FLAG_PCI_P2PDMA, ns->queue);
>   
>   	ns->queue->queuedata = ns;
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 07b34175c6ce..9c04df982d2c 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -473,7 +473,6 @@ struct nvme_ctrl_ops {
>   	unsigned int flags;
>   #define NVME_F_FABRICS			(1 << 0)
>   #define NVME_F_METADATA_SUPPORTED	(1 << 1)
> -#define NVME_F_PCI_P2PDMA		(1 << 2)
>   	int (*reg_read32)(struct nvme_ctrl *ctrl, u32 off, u32 *val);
>   	int (*reg_write32)(struct nvme_ctrl *ctrl, u32 off, u32 val);
>   	int (*reg_read64)(struct nvme_ctrl *ctrl, u32 off, u64 *val);
> @@ -481,6 +480,7 @@ struct nvme_ctrl_ops {
>   	void (*submit_async_event)(struct nvme_ctrl *ctrl);
>   	void (*delete_ctrl)(struct nvme_ctrl *ctrl);
>   	int (*get_address)(struct nvme_ctrl *ctrl, char *buf, int size);
> +	bool (*supports_pci_p2pdma)(struct nvme_ctrl *ctrl);
>   };
>   
>   #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 7249ae74f71f..14f092973792 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -2759,17 +2759,24 @@ static int nvme_pci_get_address(struct nvme_ctrl *ctrl, char *buf, int size)
>   	return snprintf(buf, size, "%s\n", dev_name(&pdev->dev));
>   }
>   
> +static bool nvme_pci_supports_pci_p2pdma(struct nvme_ctrl *ctrl)
> +{
> +	struct nvme_dev *dev = to_nvme_dev(ctrl);
> +
> +	return dma_pci_p2pdma_supported(dev->dev);
> +}
> +
>   static const struct nvme_ctrl_ops nvme_pci_ctrl_ops = {
>   	.name			= "pcie",
>   	.module			= THIS_MODULE,
> -	.flags			= NVME_F_METADATA_SUPPORTED |
> -				  NVME_F_PCI_P2PDMA,
> +	.flags			= NVME_F_METADATA_SUPPORTED,
>   	.reg_read32		= nvme_pci_reg_read32,
>   	.reg_write32		= nvme_pci_reg_write32,
>   	.reg_read64		= nvme_pci_reg_read64,
>   	.free_ctrl		= nvme_pci_free_ctrl,
>   	.submit_async_event	= nvme_pci_submit_async_event,
>   	.get_address		= nvme_pci_get_address,
> +	.supports_pci_p2pdma	= nvme_pci_supports_pci_p2pdma,
>   };
>   
>   static int nvme_dev_map(struct nvme_dev *dev)
>