IOMMU Archive on lore.kernel.org
 help / color / Atom feed
From: "Derrick, Jonathan" <jonathan.derrick@intel.com>
To: "baolu.lu@linux.intel.com" <baolu.lu@linux.intel.com>,
	"joro@8bytes.org" <joro@8bytes.org>,
	"drake@endlessm.com" <drake@endlessm.com>,
	"dwmw2@infradead.org" <dwmw2@infradead.org>
Cc: "bhelgaas@google.com" <bhelgaas@google.com>,
	"iommu@lists.linux-foundation.org"
	<iommu@lists.linux-foundation.org>,
	"linux@endlessm.com" <linux@endlessm.com>
Subject: Re: [PATCH v2] iommu/vt-d: consider real PCI device when checking if mapping is needed
Date: Fri, 14 Feb 2020 16:43:20 +0000
Message-ID: <0750dfaa5772e102df83b099926623f8378e6cc4.camel@intel.com> (raw)
In-Reply-To: <20200214090234.5746-1-drake@endlessm.com>

Hi Daniel, sorry for the delay


On Fri, 2020-02-14 at 17:02 +0800, Daniel Drake wrote:
> From: Jon Derrick <jonathan.derrick@intel.com>
> 
> The PCI devices handled by intel-iommu may have a DMA requester on
> another bus, such as VMD subdevices needing to use the VMD endpoint.
> 
> The real DMA device is now used for the DMA mapping, but one case was
> missed earlier: if the VMD device (and hence subdevices too) are under
> IOMMU_DOMAIN_IDENTITY, mappings do not work.
> 
> Codepaths like intel_map_page() handle the IOMMU_DOMAIN_DMA case by
> creating an iommu DMA mapping, and fall back on dma_direct_map_page()
> for the IOMMU_DOMAIN_IDENTITY case. However, handling of the IDENTITY
> case is broken when intel_page_page() handles a subdevice.
intel_map_page?


> 
> We observe that at iommu attach time, dmar_insert_one_dev_info() for
> the subdevices will never set dev->archdata.iommu. This is because
> that function uses find_domain() to check if there is already an IOMMU
> for the device, and find_domain() then defers to the real DMA device
> which does have one. Thus dmar_insert_one_dev_info() returns without
> assigning dev->archdata.iommu.
> 
> Then, later:
> 
> 1. intel_map_page() checks if an IOMMU mapping is needed by calling
>    iommu_need_mapping() on the subdevice. identity_mapping() returns
>    false because dev->archdata.iommu is NULL, so this function
>    returns false indicating that mapping is needed.
> 2. __intel_map_single() is called to create the mapping.
> 3. __intel_map_single() calls find_domain(). This function now returns
>    the IDENTITY domain corresponding to the real DMA device.
> 4. __intel_map_single() calls domain_get_iommu() on this "real" domain.
>    A failure is hit and the entire operation is aborted, because this
>    codepath is not intended to handle IDENTITY mappings:
>        if (WARN_ON(domain->domain.type != IOMMU_DOMAIN_DMA))
>                    return NULL;
> 
> Fix this by using the real DMA device when checking if a mapping is
> needed, while also considering the subdevice DMA mask.
> The IDENTITY case will then directly fall back on dma_direct_map_page().
> 
> Reported-by: Daniel Drake <drake@endlessm.com>
> Fixes: b0140c69637e ("iommu/vt-d: Use pci_real_dma_dev() for mapping")
> Signed-off-by: Daniel Drake <drake@endlessm.com>
> ---
> 
> Notes:
>     v2: switch to Jon's approach instead.
>     
>     This problem was detected with a non-upstream patch
>     "PCI: Add Intel remapped NVMe device support"
>     (https://marc.info/?l=linux-ide&m=156015271021615&w=2)
>     
>     This patch creates PCI devices a bit like VMD, and hence
>     I believe VMD would hit this class of problem for any cases where
>     the VMD device is in the IDENTITY domain. (I presume the reason this
>     bug was not seen already there is that it is in a DMA iommu domain).
>     
>     However this hasn't actually been tested on VMD (don't have the hardware)
>     so if I've missed anything and/or it's not a real issue then feel free to
>     drop this patch.
> 
>  drivers/iommu/intel-iommu.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 9dc37672bf89..edbe2866b515 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -3582,19 +3582,23 @@ static struct dmar_domain *get_private_domain_for_dev(struct device *dev)
>  /* Check if the dev needs to go through non-identity map and unmap process.*/
>  static bool iommu_need_mapping(struct device *dev)
>  {
> +	u64 dma_mask, required_dma_mask;
>  	int ret;
>  
>  	if (iommu_dummy(dev))
>  		return false;
>  
> -	ret = identity_mapping(dev);
> -	if (ret) {
> -		u64 dma_mask = *dev->dma_mask;
> +	dma_mask = *dev->dma_mask;
> +	if (dev->coherent_dma_mask && dev->coherent_dma_mask < dma_mask)
> +		dma_mask = dev->coherent_dma_mask;
> +	required_dma_mask = dma_direct_get_required_mask(dev);
>  
> -		if (dev->coherent_dma_mask && dev->coherent_dma_mask < dma_mask)
> -			dma_mask = dev->coherent_dma_mask;
> +	if (dev_is_pci(dev))
> +		dev = &pci_real_dma_dev(to_pci_dev(dev))->dev;
>  
> -		if (dma_mask >= dma_direct_get_required_mask(dev))
> +	ret = identity_mapping(dev);
> +	if (ret) {
> +		if (dma_mask >= required_dma_mask)
>  			return false;
>  
>  		/*



I think this might work better since it shortcuts the mask check in the
non-identity case. Tests fine when VMD is forced into Identity domain.
Feel free to add my sign-off for either patch you go with.

Thanks,
Jon



diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 9dc3767..7ffd252 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -3582,12 +3582,16 @@ static struct dmar_domain
*get_private_domain_for_dev(struct device *dev)
 /* Check if the dev needs to go through non-identity map and unmap
process.*/
 static bool iommu_need_mapping(struct device *dev)
 {
+       struct device *dma_dev = dev;
        int ret;
 
        if (iommu_dummy(dev))
                return false;
 
-       ret = identity_mapping(dev);
+       if (dev_is_pci(dev))
+               dma_dev = &pci_real_dma_dev(to_pci_dev(dev))->dev;
+
+       ret = identity_mapping(dma_dev);
        if (ret) {
                u64 dma_mask = *dev->dma_mask;
 
@@ -3601,19 +3605,19 @@ static bool iommu_need_mapping(struct device
*dev)
                 * 32 bit DMA is removed from si_domain and fall back
to
                 * non-identity mapping.
                 */
-               dmar_remove_one_dev_info(dev);
-               ret = iommu_request_dma_domain_for_dev(dev);
+               dmar_remove_one_dev_info(dma_dev);
+               ret = iommu_request_dma_domain_for_dev(dma_dev);
                if (ret) {
                        struct iommu_domain *domain;
                        struct dmar_domain *dmar_domain;
 
-                       domain = iommu_get_domain_for_dev(dev);
+                       domain = iommu_get_domain_for_dev(dma_dev);
                        if (domain) {
                                dmar_domain = to_dmar_domain(domain);
                                dmar_domain->flags |=
DOMAIN_FLAG_LOSE_CHILDREN;
                        }
-                       dmar_remove_one_dev_info(dev);
-                       get_private_domain_for_dev(dev);
+                       dmar_remove_one_dev_info(dma_dev);
+                       get_private_domain_for_dev(dma_dev);
                }
 
                dev_info(dev, "32bit DMA uses non-identity mapping\n");

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

      parent reply index

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-14  9:02 Daniel Drake
2020-02-14 12:16 ` Lu Baolu
2020-02-14 16:43 ` Derrick, Jonathan [this message]

Reply instructions:

You may reply publically 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=0750dfaa5772e102df83b099926623f8378e6cc4.camel@intel.com \
    --to=jonathan.derrick@intel.com \
    --cc=baolu.lu@linux.intel.com \
    --cc=bhelgaas@google.com \
    --cc=drake@endlessm.com \
    --cc=dwmw2@infradead.org \
    --cc=iommu@lists.linux-foundation.org \
    --cc=joro@8bytes.org \
    --cc=linux@endlessm.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

IOMMU Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iommu/0 linux-iommu/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iommu linux-iommu/ https://lore.kernel.org/linux-iommu \
		iommu@lists.linux-foundation.org
	public-inbox-index linux-iommu

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linux-foundation.lists.iommu


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git