linux-kernel - RE: [PATCH] iommu: amd/intel: Remove multifunction assumption around grouping

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]

Message-ID: <C5ECD7A89D1DC44195F34B25E172658D56A9F4@039-SN2MPN1-013.039d.mgd.msft.net>
Date:	Mon, 3 Jun 2013 07:28:26 +0000
From:	Sethi Varun-B16395 <B16395@...escale.com>
To:	Alex Williamson <alex.williamson@...hat.com>,
	"iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
	"dwmw2@...radead.org" <dwmw2@...radead.org>,
	"joro@...tes.org" <joro@...tes.org>
CC:	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] iommu: amd/intel: Remove multifunction assumption
 around	grouping


> -----Original Message-----
> From: iommu-bounces@...ts.linux-foundation.org [mailto:iommu-
> bounces@...ts.linux-foundation.org] On Behalf Of Alex Williamson
> Sent: Friday, May 31, 2013 12:09 AM
> To: iommu@...ts.linux-foundation.org; dwmw2@...radead.org;
> joro@...tes.org
> Cc: linux-pci@...r.kernel.org; linux-kernel@...r.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@...hat.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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/