| Message ID | 20130530183855.14612.15585.stgit@bling.home |
|---|---|
| State | Not Applicable |
| Headers | show |
> -----Original Message----- > From: iommu-bounces@lists.linux-foundation.org [mailto:iommu- > bounces@lists.linux-foundation.org] On Behalf Of Alex Williamson > Sent: Friday, May 31, 2013 12:09 AM > To: iommu@lists.linux-foundation.org; dwmw2@infradead.org; > joro@8bytes.org > Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: [PATCH] iommu: amd/intel: Remove multifunction assumption around > grouping > > If a device is multifunction and does not have ACS enabled then we assume > that the entire package lacks ACS and use function 0 as the base of the > group. The PCIe spec however states that components are permitted to > implement ACS on some, none, or all of their applicable functions. It's > therefore conceivable that function 0 may be fully independent and > support ACS while other functions do not. Instead use the lowest > function of the slot that does not have ACS enabled as the base of the > group. This may be the current device, which is intentional. So long as > we use a consistent algorithm, all the non-ACS functions will be grouped > together and ACS functions will get separate groups. > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > --- > drivers/iommu/amd_iommu.c | 25 +++++++++++++++++++------ > drivers/iommu/intel-iommu.c | 25 +++++++++++++++++++------ > 2 files changed, 38 insertions(+), 12 deletions(-) > > diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c index > 1d84be1..565c745 100644 > --- a/drivers/iommu/amd_iommu.c > +++ b/drivers/iommu/amd_iommu.c > @@ -287,14 +287,27 @@ static struct pci_dev *get_isolation_root(struct > pci_dev *pdev) > > /* > * If it's a multifunction device that does not support our > - * required ACS flags, add to the same group as function 0. > + * required ACS flags, add to the same group as lowest numbered > + * function that also does not suport the required ACS flags. > */ > if (dma_pdev->multifunction && > - !pci_acs_enabled(dma_pdev, REQ_ACS_FLAGS)) > - swap_pci_ref(&dma_pdev, > - pci_get_slot(dma_pdev->bus, > - PCI_DEVFN(PCI_SLOT(dma_pdev->devfn), > - 0))); > + !pci_acs_enabled(dma_pdev, REQ_ACS_FLAGS)) { > + u8 i, slot = PCI_SLOT(dma_pdev->devfn); > + > + for (i = 0; i < 8; i++) { > + struct pci_dev *tmp; > + > + tmp = pci_get_slot(dma_pdev->bus, PCI_DEVFN(slot, i)); > + if (!tmp) > + continue; > + > + if (!pci_acs_enabled(tmp, REQ_ACS_FLAGS)) { > + swap_pci_ref(&dma_pdev, tmp); > + break; > + } > + pci_dev_put(tmp); > + } > + } > > /* > * Devices on the root bus go through the iommu. If that's not us, > diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c > index b4f0e28..eec0d3e 100644 > --- a/drivers/iommu/intel-iommu.c > +++ b/drivers/iommu/intel-iommu.c > @@ -4182,14 +4182,27 @@ static int intel_iommu_add_device(struct device > *dev) > > /* > * If it's a multifunction device that does not support our > - * required ACS flags, add to the same group as function 0. > + * required ACS flags, add to the same group as lowest numbered > + * function that also does not suport the required ACS flags. > */ > if (dma_pdev->multifunction && > - !pci_acs_enabled(dma_pdev, REQ_ACS_FLAGS)) > - swap_pci_ref(&dma_pdev, > - pci_get_slot(dma_pdev->bus, > - PCI_DEVFN(PCI_SLOT(dma_pdev->devfn), > - 0))); > + !pci_acs_enabled(dma_pdev, REQ_ACS_FLAGS)) { > + u8 i, slot = PCI_SLOT(dma_pdev->devfn); > + > + for (i = 0; i < 8; i++) { [Sethi Varun-B16395] A macro like PCI_MAX_FUNCTIONS would improve code readability. > + struct pci_dev *tmp; > + > + tmp = pci_get_slot(dma_pdev->bus, PCI_DEVFN(slot, i)); > + if (!tmp) > + continue; > + > + if (!pci_acs_enabled(tmp, REQ_ACS_FLAGS)) { > + swap_pci_ref(&dma_pdev, tmp); > + break; > + } > + pci_dev_put(tmp); > + } > + } It would be nice if this code could be represented as a function in a common file like iommu/pci.c. -Varun -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2013-06-03 at 07:28 +0000, Sethi Varun-B16395 wrote: > > -----Original Message----- > > From: iommu-bounces@lists.linux-foundation.org [mailto:iommu- > > bounces@lists.linux-foundation.org] On Behalf Of Alex Williamson > > Sent: Friday, May 31, 2013 12:09 AM > > To: iommu@lists.linux-foundation.org; dwmw2@infradead.org; > > joro@8bytes.org > > Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org > > Subject: [PATCH] iommu: amd/intel: Remove multifunction assumption around > > grouping > > > > If a device is multifunction and does not have ACS enabled then we assume > > that the entire package lacks ACS and use function 0 as the base of the > > group. The PCIe spec however states that components are permitted to > > implement ACS on some, none, or all of their applicable functions. It's > > therefore conceivable that function 0 may be fully independent and > > support ACS while other functions do not. Instead use the lowest > > function of the slot that does not have ACS enabled as the base of the > > group. This may be the current device, which is intentional. So long as > > we use a consistent algorithm, all the non-ACS functions will be grouped > > together and ACS functions will get separate groups. > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > --- > > drivers/iommu/amd_iommu.c | 25 +++++++++++++++++++------ > > drivers/iommu/intel-iommu.c | 25 +++++++++++++++++++------ > > 2 files changed, 38 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c index > > 1d84be1..565c745 100644 > > --- a/drivers/iommu/amd_iommu.c > > +++ b/drivers/iommu/amd_iommu.c > > @@ -287,14 +287,27 @@ static struct pci_dev *get_isolation_root(struct > > pci_dev *pdev) > > > > /* > > * If it's a multifunction device that does not support our > > - * required ACS flags, add to the same group as function 0. > > + * required ACS flags, add to the same group as lowest numbered > > + * function that also does not suport the required ACS flags. > > */ > > if (dma_pdev->multifunction && > > - !pci_acs_enabled(dma_pdev, REQ_ACS_FLAGS)) > > - swap_pci_ref(&dma_pdev, > > - pci_get_slot(dma_pdev->bus, > > - PCI_DEVFN(PCI_SLOT(dma_pdev->devfn), > > - 0))); > > + !pci_acs_enabled(dma_pdev, REQ_ACS_FLAGS)) { > > + u8 i, slot = PCI_SLOT(dma_pdev->devfn); > > + > > + for (i = 0; i < 8; i++) { > > + struct pci_dev *tmp; > > + > > + tmp = pci_get_slot(dma_pdev->bus, PCI_DEVFN(slot, i)); > > + if (!tmp) > > + continue; > > + > > + if (!pci_acs_enabled(tmp, REQ_ACS_FLAGS)) { > > + swap_pci_ref(&dma_pdev, tmp); > > + break; > > + } > > + pci_dev_put(tmp); > > + } > > + } > > > > /* > > * Devices on the root bus go through the iommu. If that's not us, > > diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c > > index b4f0e28..eec0d3e 100644 > > --- a/drivers/iommu/intel-iommu.c > > +++ b/drivers/iommu/intel-iommu.c > > @@ -4182,14 +4182,27 @@ static int intel_iommu_add_device(struct device > > *dev) > > > > /* > > * If it's a multifunction device that does not support our > > - * required ACS flags, add to the same group as function 0. > > + * required ACS flags, add to the same group as lowest numbered > > + * function that also does not suport the required ACS flags. > > */ > > if (dma_pdev->multifunction && > > - !pci_acs_enabled(dma_pdev, REQ_ACS_FLAGS)) > > - swap_pci_ref(&dma_pdev, > > - pci_get_slot(dma_pdev->bus, > > - PCI_DEVFN(PCI_SLOT(dma_pdev->devfn), > > - 0))); > > + !pci_acs_enabled(dma_pdev, REQ_ACS_FLAGS)) { > > + u8 i, slot = PCI_SLOT(dma_pdev->devfn); > > + > > + for (i = 0; i < 8; i++) { > [Sethi Varun-B16395] A macro like PCI_MAX_FUNCTIONS would improve code readability. Ok, I see a couple defines of PCI_MAX_FUNCTION, I guess we could add one to iommu/pci.h too. > > + struct pci_dev *tmp; > > + > > + tmp = pci_get_slot(dma_pdev->bus, PCI_DEVFN(slot, i)); > > + if (!tmp) > > + continue; > > + > > + if (!pci_acs_enabled(tmp, REQ_ACS_FLAGS)) { > > + swap_pci_ref(&dma_pdev, tmp); > > + break; > > + } > > + pci_dev_put(tmp); > > + } > > + } > > It would be nice if this code could be represented as a function in a common file like iommu/pci.c. I agree, but I haven't gotten any feedback on iommu/pci.c yet and didn't want this patch to depend on that series. There are probably more opportunities to consolidate there. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 30, 2013 at 12:39:18PM -0600, Alex Williamson wrote: > If a device is multifunction and does not have ACS enabled then we > assume that the entire package lacks ACS and use function 0 as the > base of the group. The PCIe spec however states that components are > permitted to implement ACS on some, none, or all of their applicable > functions. It's therefore conceivable that function 0 may be fully > independent and support ACS while other functions do not. Instead > use the lowest function of the slot that does not have ACS enabled > as the base of the group. This may be the current device, which is > intentional. So long as we use a consistent algorithm, all the > non-ACS functions will be grouped together and ACS functions will > get separate groups. > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > --- > drivers/iommu/amd_iommu.c | 25 +++++++++++++++++++------ > drivers/iommu/intel-iommu.c | 25 +++++++++++++++++++------ Didn't really fit into any of my existing branches. Applied it to x86/vt-d because it is probably not worth creating a new x86-specific branch just for this patch. Thanks, Joerg -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c index 1d84be1..565c745 100644 --- a/drivers/iommu/amd_iommu.c +++ b/drivers/iommu/amd_iommu.c @@ -287,14 +287,27 @@ static struct pci_dev *get_isolation_root(struct pci_dev *pdev) /* * If it's a multifunction device that does not support our - * required ACS flags, add to the same group as function 0. + * required ACS flags, add to the same group as lowest numbered + * function that also does not suport the required ACS flags. */ if (dma_pdev->multifunction && - !pci_acs_enabled(dma_pdev, REQ_ACS_FLAGS)) - swap_pci_ref(&dma_pdev, - pci_get_slot(dma_pdev->bus, - PCI_DEVFN(PCI_SLOT(dma_pdev->devfn), - 0))); + !pci_acs_enabled(dma_pdev, REQ_ACS_FLAGS)) { + u8 i, slot = PCI_SLOT(dma_pdev->devfn); + + for (i = 0; i < 8; i++) { + struct pci_dev *tmp; + + tmp = pci_get_slot(dma_pdev->bus, PCI_DEVFN(slot, i)); + if (!tmp) + continue; + + if (!pci_acs_enabled(tmp, REQ_ACS_FLAGS)) { + swap_pci_ref(&dma_pdev, tmp); + break; + } + pci_dev_put(tmp); + } + } /* * Devices on the root bus go through the iommu. If that's not us, diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c index b4f0e28..eec0d3e 100644 --- a/drivers/iommu/intel-iommu.c +++ b/drivers/iommu/intel-iommu.c @@ -4182,14 +4182,27 @@ static int intel_iommu_add_device(struct device *dev) /* * If it's a multifunction device that does not support our - * required ACS flags, add to the same group as function 0. + * required ACS flags, add to the same group as lowest numbered + * function that also does not suport the required ACS flags. */ if (dma_pdev->multifunction && - !pci_acs_enabled(dma_pdev, REQ_ACS_FLAGS)) - swap_pci_ref(&dma_pdev, - pci_get_slot(dma_pdev->bus, - PCI_DEVFN(PCI_SLOT(dma_pdev->devfn), - 0))); + !pci_acs_enabled(dma_pdev, REQ_ACS_FLAGS)) { + u8 i, slot = PCI_SLOT(dma_pdev->devfn); + + for (i = 0; i < 8; i++) { + struct pci_dev *tmp; + + tmp = pci_get_slot(dma_pdev->bus, PCI_DEVFN(slot, i)); + if (!tmp) + continue; + + if (!pci_acs_enabled(tmp, REQ_ACS_FLAGS)) { + swap_pci_ref(&dma_pdev, tmp); + break; + } + pci_dev_put(tmp); + } + } /* * Devices on the root bus go through the iommu. If that's not us,
If a device is multifunction and does not have ACS enabled then we assume that the entire package lacks ACS and use function 0 as the base of the group. The PCIe spec however states that components are permitted to implement ACS on some, none, or all of their applicable functions. It's therefore conceivable that function 0 may be fully independent and support ACS while other functions do not. Instead use the lowest function of the slot that does not have ACS enabled as the base of the group. This may be the current device, which is intentional. So long as we use a consistent algorithm, all the non-ACS functions will be grouped together and ACS functions will get separate groups. Signed-off-by: Alex Williamson <alex.williamson@redhat.com> --- drivers/iommu/amd_iommu.c | 25 +++++++++++++++++++------ drivers/iommu/intel-iommu.c | 25 +++++++++++++++++++------ 2 files changed, 38 insertions(+), 12 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html