| Message ID | 20220817012024.3251276-5-baolu.lu@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iommu: SVA and IOPF refactoring | expand |
On Wed, Aug 17, 2022 at 09:20:15AM +0800, Lu Baolu wrote: > Some configurations of the PCI fabric will route device originated TLP > packets based on the memory addresses. This makes it sound like a few unusual configurations will route TLPs based on memory addresses, but address routing is the default for all PCIe Memory Requests, and ACS provides a way to override that default. > These configurations are incompatible with PASID as the PASID > packets form a distinct address space. I would say "the Requester ID/PASID combination forms a distinct address space." > For instance, any configuration where switches are present > without ACS enabled is incompatible. > > This enhances the pci_enable_pasid() interface by requiring the ACS to > support Source Validation, Request Redirection, Completer Redirection, > and Upstream Forwarding. This effectively means that devices cannot > spoof their requester ID, requests and completions cannot be redirected, > and all transactions are forwarded upstream, even as it passes through a > bridge where the target device is downstream. I think your patch actually requires all those features to be not just "supported" but actually *enabled* for the entire path leading to the device. To use the terms from the spec: "P2P Request Redirect" "P2P Completion Redirect" "Requester ID, Requests, and Completions" and maybe something like: ... even if the TLP looks like a P2P Request because its memory address (ignoring the PASID) would fall in a bridge window and would normally be routed downstream. Does the PCIe spec really allow TLPs with PASID to be routed anywhere except upstream? It seems nonsensical to route them downstream, and hardware should be able to check that easily. But I took a quick look through the spec and didn't see anything about PASID by itself influencing routing. > Suggested-by: Jason Gunthorpe <jgg@nvidia.com> > Suggested-by: Kevin Tian <kevin.tian@intel.com> > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > --- > drivers/pci/ats.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c > index c967ad6e2626..0715e48e7973 100644 > --- a/drivers/pci/ats.c > +++ b/drivers/pci/ats.c > @@ -382,6 +382,11 @@ int pci_enable_pasid(struct pci_dev *pdev, int features) > if (!pasid) > return -EINVAL; > > + if (!pci_acs_path_enabled(pdev, NULL, > + PCI_ACS_SV | PCI_ACS_RR | > + PCI_ACS_CR | PCI_ACS_UF)) > + return -EINVAL; > + > pci_read_config_word(pdev, pasid + PCI_PASID_CAP, &supported); > supported &= PCI_PASID_CAP_EXEC | PCI_PASID_CAP_PRIV; > > -- > 2.25.1 >
On Wed, Aug 17, 2022 at 04:17:43PM -0500, Bjorn Helgaas wrote: > Does the PCIe spec really allow TLPs with PASID to be routed anywhere > except upstream? I think yes: 2.2.10.2 End-End TLP Prefix Processing: The presence of an End-End TLP Prefix does not alter the routing of a TLP. TLPs are routed based on the routing rules covered in Section 2.2.4 . Which I read as saying that routing is done after stripping all the prefixes. PASID is a prefix. Lu, you may want to quote the spec in the commit message to make it clear. A strange choice in my opinion, but there it is.. Jason
Hi Bjorn, On 2022/8/18 05:17, Bjorn Helgaas wrote: > On Wed, Aug 17, 2022 at 09:20:15AM +0800, Lu Baolu wrote: >> Some configurations of the PCI fabric will route device originated TLP >> packets based on the memory addresses. > This makes it sound like a few unusual configurations will route TLPs > based on memory addresses, but address routing is the default for all > PCIe Memory Requests, and ACS provides a way to override that default. > >> These configurations are incompatible with PASID as the PASID >> packets form a distinct address space. > I would say "the Requester ID/PASID combination forms a distinct > address space." > >> For instance, any configuration where switches are present >> without ACS enabled is incompatible. >> >> This enhances the pci_enable_pasid() interface by requiring the ACS to >> support Source Validation, Request Redirection, Completer Redirection, >> and Upstream Forwarding. This effectively means that devices cannot >> spoof their requester ID, requests and completions cannot be redirected, >> and all transactions are forwarded upstream, even as it passes through a >> bridge where the target device is downstream. > I think your patch actually requires all those features to be not just > "supported" but actually*enabled* for the entire path leading to the > device. > > To use the terms from the spec: > > "P2P Request Redirect" > "P2P Completion Redirect" > "Requester ID, Requests, and Completions" > > and maybe something like: > > ... even if the TLP looks like a P2P Request because its memory > address (ignoring the PASID) would fall in a bridge window and would > normally be routed downstream. Thank you for the suggestions. I will rephrase the commit message accordingly like this: PCI: Allow PASID only when ACS enforced on upstreaming path The PCIe fabric routes TLPs based on memory addresses for all PCIe Memory Requests regardless of whether TLPs have PASID prefixes. This is stated in section "2.2.10.2 End-End TLP Prefix Processing" of the specification: The presence of an End-End TLP Prefix does not alter the routing of a TLP. TLPs are routed based on the routing rules covered in Section 2.2.4 . As the Requester ID/PASID combination forms a distinct address space. The memory address based routing is not compatible for PASID TLPs anymore. Therefore we have to rely on ACS to override that default. This enhances pci_enable_pasid() interface by requiring the ACS features to be enabled for the entire path leading to the device. So that even if the TLP looks like a P2P Request because its memory address (ignoring the PASID) would fall in a bridge window and would normally be routed downstream. Best regards, baolu
On 2022/8/18 06:48, Jason Gunthorpe wrote: > On Wed, Aug 17, 2022 at 04:17:43PM -0500, Bjorn Helgaas wrote: > >> Does the PCIe spec really allow TLPs with PASID to be routed anywhere >> except upstream? > I think yes: > > 2.2.10.2 End-End TLP Prefix Processing: > > The presence of an End-End TLP Prefix does not alter the routing of a > TLP. TLPs are routed based on the routing rules covered in Section > 2.2.4 . > > Which I read as saying that routing is done after stripping all the > prefixes. PASID is a prefix. > > Lu, you may want to quote the spec in the commit message to make it > clear. Yes. Sure thing. Thank you! Best regards, baolu
On Wed, Aug 17, 2022 at 09:20:15AM +0800, Lu Baolu wrote: > Some configurations of the PCI fabric will route device originated TLP > packets based on the memory addresses. These configurations are > incompatible with PASID as the PASID packets form a distinct address > space. For instance, any configuration where switches are present > without ACS enabled is incompatible. > > This enhances the pci_enable_pasid() interface by requiring the ACS to > support Source Validation, Request Redirection, Completer Redirection, > and Upstream Forwarding. This effectively means that devices cannot > spoof their requester ID, requests and completions cannot be redirected, > and all transactions are forwarded upstream, even as it passes through a > bridge where the target device is downstream. > > Suggested-by: Jason Gunthorpe <jgg@nvidia.com> > Suggested-by: Kevin Tian <kevin.tian@intel.com> > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > --- > drivers/pci/ats.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c > index c967ad6e2626..0715e48e7973 100644 > --- a/drivers/pci/ats.c > +++ b/drivers/pci/ats.c > @@ -382,6 +382,11 @@ int pci_enable_pasid(struct pci_dev *pdev, int features) > if (!pasid) > return -EINVAL; > > + if (!pci_acs_path_enabled(pdev, NULL, > + PCI_ACS_SV | PCI_ACS_RR | > + PCI_ACS_CR | PCI_ACS_UF)) I think we only need RR and UF here? Source Validation causes the switch to validate the requestor RID in each TLP which has nothing to do with address based routing Completion Redirect changes how RID routing works, and has nothing to do with address based routing. Yes, both of those are usually set for virtualization scenarios but we shouldn't check it here as a basic requirement to enable PASID. Jason
On Thu, Aug 18, 2022 at 07:53:15PM +0800, Baolu Lu wrote: > On 2022/8/18 05:17, Bjorn Helgaas wrote: > > On Wed, Aug 17, 2022 at 09:20:15AM +0800, Lu Baolu wrote: > > > Some configurations of the PCI fabric will route device originated TLP > > > packets based on the memory addresses. > > This makes it sound like a few unusual configurations will route TLPs > > based on memory addresses, but address routing is the default for all > > PCIe Memory Requests, and ACS provides a way to override that default. > > > > > These configurations are incompatible with PASID as the PASID > > > packets form a distinct address space. > > I would say "the Requester ID/PASID combination forms a distinct > > address space." > > > > > For instance, any configuration where switches are present > > > without ACS enabled is incompatible. > > > > > > This enhances the pci_enable_pasid() interface by requiring the ACS to > > > support Source Validation, Request Redirection, Completer Redirection, > > > and Upstream Forwarding. This effectively means that devices cannot > > > spoof their requester ID, requests and completions cannot be redirected, > > > and all transactions are forwarded upstream, even as it passes through a > > > bridge where the target device is downstream. > > > > I think your patch actually requires all those features to be not just > > "supported" but actually*enabled* for the entire path leading to the > > device. > > > > To use the terms from the spec: > > > > "P2P Request Redirect" > > "P2P Completion Redirect" > > "Requester ID, Requests, and Completions" > > > > and maybe something like: > > > > ... even if the TLP looks like a P2P Request because its memory > > address (ignoring the PASID) would fall in a bridge window and would > > normally be routed downstream. > > Thank you for the suggestions. I will rephrase the commit message > accordingly like this: > > > PCI: Allow PASID only when ACS enforced on upstreaming path PCI: Enable PASID only when ACS RR & UF enabled on upstream path The Requester ID/Process Address Space ID (PASID) combination identifies an address space distinct from the PCI bus address space, e.g., an address space defined by an IOMMU. But the PCIe fabric routes Memory Requests based on the TLP address, ignoring any PASID (PCIe r6.0, sec 2.2.10.4), so a TLP with PASID that *should* go upstream to the IOMMU may instead be routed as a P2P Request if its address falls in a bridge window. To ensure that all Memory Requests with PASID are routed upstream, only enable PASID if ACS P2P Request Redirect and Upstream Forwarding are enabled for the path leading to the device. > The PCIe fabric routes TLPs based on memory addresses for all PCIe Memory > Requests regardless of whether TLPs have PASID prefixes. This is stated in > section "2.2.10.2 End-End TLP Prefix Processing" of the specification: > > The presence of an End-End TLP Prefix does not alter the routing of a > TLP. TLPs are routed based on the routing rules covered in Section > 2.2.4 . > > As the Requester ID/PASID combination forms a distinct address space. The > memory address based routing is not compatible for PASID TLPs anymore. > Therefore we have to rely on ACS to override that default. > > This enhances pci_enable_pasid() interface by requiring the ACS features > to be enabled for the entire path leading to the device. So that even if > the TLP looks like a P2P Request because its memory address (ignoring the > PASID) would fall in a bridge window and would normally be routed > downstream. > > Best regards, > baolu
在 2022/8/19 7:00, Bjorn Helgaas 写道: > On Thu, Aug 18, 2022 at 07:53:15PM +0800, Baolu Lu wrote: >> On 2022/8/18 05:17, Bjorn Helgaas wrote: >>> On Wed, Aug 17, 2022 at 09:20:15AM +0800, Lu Baolu wrote: >>>> Some configurations of the PCI fabric will route device originated TLP >>>> packets based on the memory addresses. >>> This makes it sound like a few unusual configurations will route TLPs >>> based on memory addresses, but address routing is the default for all >>> PCIe Memory Requests, and ACS provides a way to override that default. >>> >>>> These configurations are incompatible with PASID as the PASID >>>> packets form a distinct address space. >>> I would say "the Requester ID/PASID combination forms a distinct >>> address space." >>> >>>> For instance, any configuration where switches are present >>>> without ACS enabled is incompatible. >>>> >>>> This enhances the pci_enable_pasid() interface by requiring the ACS to >>>> support Source Validation, Request Redirection, Completer Redirection, >>>> and Upstream Forwarding. This effectively means that devices cannot >>>> spoof their requester ID, requests and completions cannot be redirected, >>>> and all transactions are forwarded upstream, even as it passes through a >>>> bridge where the target device is downstream. >>> I think your patch actually requires all those features to be not just >>> "supported" but actually*enabled* for the entire path leading to the >>> device. >>> >>> To use the terms from the spec: >>> >>> "P2P Request Redirect" >>> "P2P Completion Redirect" >>> "Requester ID, Requests, and Completions" >>> >>> and maybe something like: >>> >>> ... even if the TLP looks like a P2P Request because its memory >>> address (ignoring the PASID) would fall in a bridge window and would >>> normally be routed downstream. >> Thank you for the suggestions. I will rephrase the commit message >> accordingly like this: >> >> >> PCI: Allow PASID only when ACS enforced on upstreaming path > PCI: Enable PASID only when ACS RR & UF enabled on upstream path > > The Requester ID/Process Address Space ID (PASID) combination > identifies an address space distinct from the PCI bus address space, > e.g., an address space defined by an IOMMU. > > But the PCIe fabric routes Memory Requests based on the TLP address, > ignoring any PASID (PCIe r6.0, sec 2.2.10.4), so a TLP with PASID that > *should* go upstream to the IOMMU may instead be routed as a P2P > Request if its address falls in a bridge window. > > To ensure that all Memory Requests with PASID are routed upstream, > only enable PASID if ACS P2P Request Redirect and Upstream Forwarding > are enabled for the path leading to the device. Seeing these comments, my questions gone. Thanks Bjorn ! >> The PCIe fabric routes TLPs based on memory addresses for all PCIe Memory >> Requests regardless of whether TLPs have PASID prefixes. This is stated in >> section "2.2.10.2 End-End TLP Prefix Processing" of the specification: >> >> The presence of an End-End TLP Prefix does not alter the routing of a >> TLP. TLPs are routed based on the routing rules covered in Section >> 2.2.4 . >> >> As the Requester ID/PASID combination forms a distinct address space. The >> memory address based routing is not compatible for PASID TLPs anymore. >> Therefore we have to rely on ACS to override that default. >> >> This enhances pci_enable_pasid() interface by requiring the ACS features >> to be enabled for the entire path leading to the device. So that even if >> the TLP looks like a P2P Request because its memory address (ignoring the >> PASID) would fall in a bridge window and would normally be routed >> downstream. >> >> Best regards, >> baolu
On 2022/8/19 07:00, Bjorn Helgaas wrote: > On Thu, Aug 18, 2022 at 07:53:15PM +0800, Baolu Lu wrote: >> On 2022/8/18 05:17, Bjorn Helgaas wrote: >>> On Wed, Aug 17, 2022 at 09:20:15AM +0800, Lu Baolu wrote: >>>> Some configurations of the PCI fabric will route device originated TLP >>>> packets based on the memory addresses. >>> This makes it sound like a few unusual configurations will route TLPs >>> based on memory addresses, but address routing is the default for all >>> PCIe Memory Requests, and ACS provides a way to override that default. >>> >>>> These configurations are incompatible with PASID as the PASID >>>> packets form a distinct address space. >>> I would say "the Requester ID/PASID combination forms a distinct >>> address space." >>> >>>> For instance, any configuration where switches are present >>>> without ACS enabled is incompatible. >>>> >>>> This enhances the pci_enable_pasid() interface by requiring the ACS to >>>> support Source Validation, Request Redirection, Completer Redirection, >>>> and Upstream Forwarding. This effectively means that devices cannot >>>> spoof their requester ID, requests and completions cannot be redirected, >>>> and all transactions are forwarded upstream, even as it passes through a >>>> bridge where the target device is downstream. >>> I think your patch actually requires all those features to be not just >>> "supported" but actually*enabled* for the entire path leading to the >>> device. >>> >>> To use the terms from the spec: >>> >>> "P2P Request Redirect" >>> "P2P Completion Redirect" >>> "Requester ID, Requests, and Completions" >>> >>> and maybe something like: >>> >>> ... even if the TLP looks like a P2P Request because its memory >>> address (ignoring the PASID) would fall in a bridge window and would >>> normally be routed downstream. >> Thank you for the suggestions. I will rephrase the commit message >> accordingly like this: >> >> >> PCI: Allow PASID only when ACS enforced on upstreaming path > PCI: Enable PASID only when ACS RR & UF enabled on upstream path > > The Requester ID/Process Address Space ID (PASID) combination > identifies an address space distinct from the PCI bus address space, > e.g., an address space defined by an IOMMU. > > But the PCIe fabric routes Memory Requests based on the TLP address, > ignoring any PASID (PCIe r6.0, sec 2.2.10.4), so a TLP with PASID that > *should* go upstream to the IOMMU may instead be routed as a P2P > Request if its address falls in a bridge window. > > To ensure that all Memory Requests with PASID are routed upstream, > only enable PASID if ACS P2P Request Redirect and Upstream Forwarding > are enabled for the path leading to the device. Yours is clear and straight-forward. I will update the patch with above. Thank you and very appreciated! Best regards, baolu
On 2022/8/18 21:04, Jason Gunthorpe wrote: > On Wed, Aug 17, 2022 at 09:20:15AM +0800, Lu Baolu wrote: >> Some configurations of the PCI fabric will route device originated TLP >> packets based on the memory addresses. These configurations are >> incompatible with PASID as the PASID packets form a distinct address >> space. For instance, any configuration where switches are present >> without ACS enabled is incompatible. >> >> This enhances the pci_enable_pasid() interface by requiring the ACS to >> support Source Validation, Request Redirection, Completer Redirection, >> and Upstream Forwarding. This effectively means that devices cannot >> spoof their requester ID, requests and completions cannot be redirected, >> and all transactions are forwarded upstream, even as it passes through a >> bridge where the target device is downstream. >> >> Suggested-by: Jason Gunthorpe<jgg@nvidia.com> >> Suggested-by: Kevin Tian<kevin.tian@intel.com> >> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com> >> --- >> drivers/pci/ats.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c >> index c967ad6e2626..0715e48e7973 100644 >> --- a/drivers/pci/ats.c >> +++ b/drivers/pci/ats.c >> @@ -382,6 +382,11 @@ int pci_enable_pasid(struct pci_dev *pdev, int features) >> if (!pasid) >> return -EINVAL; >> >> + if (!pci_acs_path_enabled(pdev, NULL, >> + PCI_ACS_SV | PCI_ACS_RR | >> + PCI_ACS_CR | PCI_ACS_UF)) > I think we only need RR and UF here? > > Source Validation causes the switch to validate the requestor RID in > each TLP which has nothing to do with address based routing > > Completion Redirect changes how RID routing works, and has nothing to > do with address based routing. > > Yes, both of those are usually set for virtualization scenarios but we > shouldn't check it here as a basic requirement to enable PASID. Yes. Here only requires RR and UF. Best regards, baolu
On Tue, Aug 23, 2022 at 03:05:53PM +0800, Baolu Lu wrote: > On 2022/8/19 07:00, Bjorn Helgaas wrote: > > PCI: Enable PASID only when ACS RR & UF enabled on upstream path > > > > The Requester ID/Process Address Space ID (PASID) combination > > identifies an address space distinct from the PCI bus address space, > > e.g., an address space defined by an IOMMU. > > > > But the PCIe fabric routes Memory Requests based on the TLP address, > > ignoring any PASID (PCIe r6.0, sec 2.2.10.4), so a TLP with PASID that > > *should* go upstream to the IOMMU may instead be routed as a P2P > > Request if its address falls in a bridge window. > > > > To ensure that all Memory Requests with PASID are routed upstream, > > only enable PASID if ACS P2P Request Redirect and Upstream Forwarding > > are enabled for the path leading to the device. > > Yours is clear and straight-forward. I will update the patch with above. > Thank you and very appreciated! With the update to only require RR and UF and the commit log update, Acked-by: Bjorn Helgaas <bhelgaas@google.com> Thanks!
diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c index c967ad6e2626..0715e48e7973 100644 --- a/drivers/pci/ats.c +++ b/drivers/pci/ats.c @@ -382,6 +382,11 @@ int pci_enable_pasid(struct pci_dev *pdev, int features) if (!pasid) return -EINVAL; + if (!pci_acs_path_enabled(pdev, NULL, + PCI_ACS_SV | PCI_ACS_RR | + PCI_ACS_CR | PCI_ACS_UF)) + return -EINVAL; + pci_read_config_word(pdev, pasid + PCI_PASID_CAP, &supported); supported &= PCI_PASID_CAP_EXEC | PCI_PASID_CAP_PRIV;
Some configurations of the PCI fabric will route device originated TLP packets based on the memory addresses. These configurations are incompatible with PASID as the PASID packets form a distinct address space. For instance, any configuration where switches are present without ACS enabled is incompatible. This enhances the pci_enable_pasid() interface by requiring the ACS to support Source Validation, Request Redirection, Completer Redirection, and Upstream Forwarding. This effectively means that devices cannot spoof their requester ID, requests and completions cannot be redirected, and all transactions are forwarded upstream, even as it passes through a bridge where the target device is downstream. Suggested-by: Jason Gunthorpe <jgg@nvidia.com> Suggested-by: Kevin Tian <kevin.tian@intel.com> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> --- drivers/pci/ats.c | 5 +++++ 1 file changed, 5 insertions(+)