From: Bjorn Helgaas <helgaas@kernel.org>
To: Logan Gunthorpe <logang@deltatee.com>
Cc: linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org,
linux-nvme@lists.infradead.org, linux-rdma@vger.kernel.org,
linux-nvdimm@lists.01.org, linux-block@vger.kernel.org,
"Stephen Bates" <sbates@raithlin.com>,
"Christoph Hellwig" <hch@lst.de>, "Jens Axboe" <axboe@kernel.dk>,
"Keith Busch" <keith.busch@intel.com>,
"Sagi Grimberg" <sagi@grimberg.me>,
"Bjorn Helgaas" <bhelgaas@google.com>,
"Jason Gunthorpe" <jgg@mellanox.com>,
"Max Gurtovoy" <maxg@mellanox.com>,
"Dan Williams" <dan.j.williams@intel.com>,
"Jérôme Glisse" <jglisse@redhat.com>,
"Benjamin Herrenschmidt" <benh@kernel.crashing.org>,
"Alex Williamson" <alex.williamson@redhat.com>
Subject: Re: [PATCH v2 04/10] PCI/P2PDMA: Clear ACS P2P flags for all devices behind switches
Date: Thu, 1 Mar 2018 12:02:57 -0600 [thread overview]
Message-ID: <20180301180257.GH13722@bhelgaas-glaptop.roam.corp.google.com> (raw)
In-Reply-To: <20180228234006.21093-5-logang@deltatee.com>
On Wed, Feb 28, 2018 at 04:40:00PM -0700, Logan Gunthorpe wrote:
> For peer-to-peer transactions to work the downstream ports in each
> switch must not have the ACS flags set. At this time there is no way
> to dynamically change the flags and update the corresponding IOMMU
> groups so this is done at enumeration time before the the groups are
> assigned.
s/the the/the/
> This effectively means that if CONFIG_PCI_P2PDMA is selected then
> all devices behind any switch will be in the same IOMMU group.
>
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> ---
> drivers/pci/Kconfig | 4 ++++
> drivers/pci/p2pdma.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
> drivers/pci/pci.c | 4 ++++
> include/linux/pci-p2pdma.h | 5 +++++
> 4 files changed, 57 insertions(+)
>
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index 840831418cbd..a430672f0ad4 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -138,6 +138,10 @@ config PCI_P2PDMA
> it's hard to tell which support it with good performance, so
> at this time you will need a PCIe switch.
>
> + Enabling this option will also disable ACS on all ports behind
> + any PCIe switch. This effictively puts all devices behind any
> + switch into the same IOMMU group.
s/effictively/effectively/
Does this really mean "all devices behind the same Root Port"?
What does this mean in terms of device security? I assume it means,
at least, that individual devices can't be assigned to separate VMs.
I don't mind admitting that this patch makes me pretty nervous, and I
don't have a clear idea of what the implications of this are, or how
to communicate those to end users. "The same IOMMU group" is a pretty
abstract idea.
> If unsure, say N.
>
> config PCI_LABEL
> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index 4e1c81f64b29..61af07acd21a 100644
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c
> @@ -255,6 +255,50 @@ static struct pci_dev *get_upstream_bridge_port(struct pci_dev *pdev)
> return up2;
> }
>
> +/*
> + * pci_p2pdma_disable_acs - disable ACS flags for ports in PCI
> + * bridges/switches
> + * @pdev: device to disable ACS flags for
> + *
> + * The ACS flags for P2P Request Redirect and P2P Completion Redirect need
> + * to be disabled on any downstream port in any switch in order for
> + * the TLPs to not be forwarded up to the RC which is not what we want
> + * for P2P.
> + *
> + * This function is called when the devices are first enumerated and
> + * will result in all devices behind any switch to be in the same IOMMU
> + * group. At this time there is no way to "hotplug" IOMMU groups so we rely
> + * on this largish hammer. If you need the devices to be in separate groups
> + * don't enable CONFIG_PCI_P2PDMA.
> + *
> + * Returns 1 if the ACS bits for this device were cleared, otherwise 0.
> + */
> +int pci_p2pdma_disable_acs(struct pci_dev *pdev)
> +{
> + struct pci_dev *up;
> + int pos;
> + u16 ctrl;
> +
> + up = get_upstream_bridge_port(pdev);
> + if (!up)
> + return 0;
> + pci_dev_put(up);
> +
> + pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ACS);
> + if (!pos)
> + return 0;
> +
> + dev_info(&pdev->dev, "disabling ACS flags for peer-to-peer DMA\n");
> +
> + pci_read_config_word(pdev, pos + PCI_ACS_CTRL, &ctrl);
> +
> + ctrl &= ~(PCI_ACS_RR | PCI_ACS_CR);
> +
> + pci_write_config_word(pdev, pos + PCI_ACS_CTRL, ctrl);
> +
> + return 1;
> +}
> +
> static bool __upstream_bridges_match(struct pci_dev *upstream,
> struct pci_dev *client)
> {
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index f6a4dd10d9b0..95ad3cf288c8 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -16,6 +16,7 @@
> #include <linux/of.h>
> #include <linux/of_pci.h>
> #include <linux/pci.h>
> +#include <linux/pci-p2pdma.h>
> #include <linux/pm.h>
> #include <linux/slab.h>
> #include <linux/module.h>
> @@ -2826,6 +2827,9 @@ static void pci_std_enable_acs(struct pci_dev *dev)
> */
> void pci_enable_acs(struct pci_dev *dev)
> {
> + if (pci_p2pdma_disable_acs(dev))
> + return;
This doesn't read naturally to me. I do see that when
CONFIG_PCI_P2PDMA is not set, pci_p2pdma_disable_acs() does nothing
and returns 0, so we'll go ahead and try to enable ACS as before.
But I think it would be clearer to have an #ifdef CONFIG_PCI_P2PDMA
right here so it's more obvious that we only disable ACS when it's
selected.
> if (!pci_acs_enable)
> return;
>
> diff --git a/include/linux/pci-p2pdma.h b/include/linux/pci-p2pdma.h
> index 126eca697ab3..f537f521f60c 100644
> --- a/include/linux/pci-p2pdma.h
> +++ b/include/linux/pci-p2pdma.h
> @@ -22,6 +22,7 @@ struct block_device;
> struct scatterlist;
>
> #ifdef CONFIG_PCI_P2PDMA
> +int pci_p2pdma_disable_acs(struct pci_dev *pdev);
> int pci_p2pdma_add_resource(struct pci_dev *pdev, int bar, size_t size,
> u64 offset);
> int pci_p2pdma_add_client(struct list_head *head, struct device *dev);
> @@ -41,6 +42,10 @@ int pci_p2pdma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
> void pci_p2pdma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
> enum dma_data_direction dir);
> #else /* CONFIG_PCI_P2PDMA */
> +static inline int pci_p2pdma_disable_acs(struct pci_dev *pdev)
> +{
> + return 0;
> +}
> static inline int pci_p2pdma_add_resource(struct pci_dev *pdev, int bar,
> size_t size, u64 offset)
> {
> --
> 2.11.0
>
next prev parent reply other threads:[~2018-03-01 18:03 UTC|newest]
Thread overview: 124+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-02-28 23:39 [PATCH v2 00/10] Copy Offload in NVMe Fabrics with P2P PCI Memory Logan Gunthorpe
2018-02-28 23:39 ` [PATCH v2 01/10] PCI/P2PDMA: Support peer to peer memory Logan Gunthorpe
2018-03-01 17:37 ` Bjorn Helgaas
2018-03-01 18:55 ` Logan Gunthorpe
2018-03-01 23:00 ` Bjorn Helgaas
2018-03-01 23:06 ` Logan Gunthorpe
2018-03-01 23:14 ` Stephen Bates
2018-03-01 23:45 ` Bjorn Helgaas
2018-02-28 23:39 ` [PATCH v2 02/10] PCI/P2PDMA: Add sysfs group to display p2pmem stats Logan Gunthorpe
2018-03-01 17:44 ` Bjorn Helgaas
2018-03-02 0:15 ` Logan Gunthorpe
2018-03-02 0:36 ` Dan Williams
2018-03-02 0:37 ` Logan Gunthorpe
2018-02-28 23:39 ` [PATCH v2 03/10] PCI/P2PDMA: Add PCI p2pmem dma mappings to adjust the bus offset Logan Gunthorpe
2018-03-01 17:49 ` Bjorn Helgaas
2018-03-01 19:36 ` Logan Gunthorpe
2018-02-28 23:40 ` [PATCH v2 04/10] PCI/P2PDMA: Clear ACS P2P flags for all devices behind switches Logan Gunthorpe
2018-03-01 18:02 ` Bjorn Helgaas [this message]
2018-03-01 18:54 ` Stephen Bates
2018-03-01 21:21 ` Alex Williamson
2018-03-01 21:26 ` Logan Gunthorpe
2018-03-01 21:32 ` Stephen Bates
2018-03-01 21:35 ` Jerome Glisse
2018-03-01 21:37 ` Logan Gunthorpe
2018-03-01 23:15 ` Bjorn Helgaas
2018-03-01 23:59 ` Logan Gunthorpe
2018-03-01 19:13 ` Logan Gunthorpe
2018-03-05 22:28 ` Bjorn Helgaas
2018-03-05 23:01 ` Logan Gunthorpe
2018-02-28 23:40 ` [PATCH v2 05/10] block: Introduce PCI P2P flags for request and request queue Logan Gunthorpe
2018-03-01 11:08 ` Sagi Grimberg
2018-02-28 23:40 ` [PATCH v2 06/10] IB/core: Add optional PCI P2P flag to rdma_rw_ctx_[init|destroy]() Logan Gunthorpe
2018-03-01 10:32 ` Sagi Grimberg
2018-03-01 17:16 ` Logan Gunthorpe
2018-02-28 23:40 ` [PATCH v2 07/10] nvme-pci: Use PCI p2pmem subsystem to manage the CMB Logan Gunthorpe
2018-03-05 1:33 ` Oliver
2018-03-05 16:00 ` Keith Busch
2018-03-05 17:10 ` Logan Gunthorpe
2018-03-05 18:02 ` Sinan Kaya
2018-03-05 18:09 ` Logan Gunthorpe
2018-03-06 0:49 ` Oliver
2018-03-06 1:14 ` Logan Gunthorpe
2018-03-06 10:40 ` Oliver
2018-03-05 19:57 ` Sagi Grimberg
2018-03-05 20:10 ` Jason Gunthorpe
2018-03-05 20:16 ` Logan Gunthorpe
2018-03-05 20:42 ` Keith Busch
2018-03-05 20:50 ` Jason Gunthorpe
2018-03-05 20:13 ` Logan Gunthorpe
2018-02-28 23:40 ` [PATCH v2 08/10] nvme-pci: Add support for P2P memory in requests Logan Gunthorpe
2018-03-01 11:07 ` Sagi Grimberg
2018-03-01 15:58 ` Stephen Bates
2018-03-09 5:08 ` Bart Van Assche
2018-02-28 23:40 ` [PATCH v2 09/10] nvme-pci: Add a quirk for a pseudo CMB Logan Gunthorpe
2018-03-01 11:03 ` Sagi Grimberg
2018-02-28 23:40 ` [PATCH v2 10/10] nvmet: Optionally use PCI P2P memory Logan Gunthorpe
2018-03-01 11:03 ` Sagi Grimberg
2018-03-01 16:15 ` Stephen Bates
2018-03-01 17:40 ` Logan Gunthorpe
2018-03-01 18:35 ` Sagi Grimberg
2018-03-01 18:42 ` Jason Gunthorpe
2018-03-01 19:01 ` Stephen Bates
2018-03-01 19:27 ` Logan Gunthorpe
2018-03-01 22:45 ` Jason Gunthorpe
2018-03-01 22:56 ` Logan Gunthorpe
2018-03-01 23:00 ` Stephen Bates
2018-03-01 23:20 ` Jason Gunthorpe
2018-03-01 23:29 ` Logan Gunthorpe
2018-03-01 23:32 ` Stephen Bates
2018-03-01 23:49 ` Keith Busch
2018-03-01 23:52 ` Logan Gunthorpe
2018-03-01 23:53 ` Stephen Bates
2018-03-02 15:53 ` Christoph Hellwig
2018-03-02 20:51 ` Stephen Bates
2018-03-01 23:57 ` Stephen Bates
2018-03-02 0:03 ` Logan Gunthorpe
2018-03-02 16:18 ` Jason Gunthorpe
2018-03-02 17:10 ` Logan Gunthorpe
2018-03-01 19:10 ` Logan Gunthorpe
2018-03-01 3:54 ` [PATCH v2 00/10] Copy Offload in NVMe Fabrics with P2P PCI Memory Benjamin Herrenschmidt
2018-03-01 3:56 ` Benjamin Herrenschmidt
2018-03-01 18:04 ` Logan Gunthorpe
2018-03-01 20:29 ` Benjamin Herrenschmidt
2018-03-01 20:55 ` Jerome Glisse
2018-03-01 21:03 ` Logan Gunthorpe
2018-03-01 21:10 ` Jerome Glisse
2018-03-01 21:15 ` Logan Gunthorpe
2018-03-01 21:25 ` Jerome Glisse
2018-03-01 21:37 ` Stephen Bates
2018-03-02 21:38 ` Stephen Bates
2018-03-02 22:09 ` Jerome Glisse
2018-03-05 20:36 ` Stephen Bates
2018-03-01 20:55 ` Logan Gunthorpe
2018-03-01 18:09 ` Stephen Bates
2018-03-01 20:32 ` Benjamin Herrenschmidt
2018-03-01 19:21 ` Dan Williams
2018-03-01 19:30 ` Logan Gunthorpe
2018-03-01 20:34 ` Benjamin Herrenschmidt
2018-03-01 20:40 ` Benjamin Herrenschmidt
2018-03-01 20:53 ` Jason Gunthorpe
2018-03-01 20:57 ` Logan Gunthorpe
2018-03-01 22:06 ` Benjamin Herrenschmidt
2018-03-01 22:31 ` Linus Torvalds
2018-03-01 22:34 ` Benjamin Herrenschmidt
2018-03-02 16:22 ` Kani, Toshi
2018-03-02 16:57 ` Linus Torvalds
2018-03-02 17:34 ` Linus Torvalds
2018-03-02 17:38 ` Kani, Toshi
2018-03-01 21:37 ` Dan Williams
2018-03-01 21:45 ` Logan Gunthorpe
2018-03-01 21:57 ` Logan Gunthorpe
2018-03-01 23:00 ` Benjamin Herrenschmidt
2018-03-01 23:19 ` Logan Gunthorpe
2018-03-01 23:25 ` Benjamin Herrenschmidt
2018-03-02 21:44 ` Benjamin Herrenschmidt
2018-03-02 22:24 ` Logan Gunthorpe
2018-03-01 23:26 ` Benjamin Herrenschmidt
2018-03-01 23:54 ` Logan Gunthorpe
2018-03-01 21:03 ` Benjamin Herrenschmidt
2018-03-01 21:11 ` Logan Gunthorpe
2018-03-01 21:18 ` Jerome Glisse
2018-03-01 21:22 ` Logan Gunthorpe
2018-03-01 10:31 ` Sagi Grimberg
2018-03-01 19:33 ` Logan Gunthorpe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180301180257.GH13722@bhelgaas-glaptop.roam.corp.google.com \
--to=helgaas@kernel.org \
--cc=alex.williamson@redhat.com \
--cc=axboe@kernel.dk \
--cc=benh@kernel.crashing.org \
--cc=bhelgaas@google.com \
--cc=dan.j.williams@intel.com \
--cc=hch@lst.de \
--cc=jgg@mellanox.com \
--cc=jglisse@redhat.com \
--cc=keith.busch@intel.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nvdimm@lists.01.org \
--cc=linux-nvme@lists.infradead.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux-rdma@vger.kernel.org \
--cc=logang@deltatee.com \
--cc=maxg@mellanox.com \
--cc=sagi@grimberg.me \
--cc=sbates@raithlin.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).