IOMMU Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] iommu/vt-d: consider real PCI device when checking if mapping is needed
@ 2020-02-14  9:02 Daniel Drake
  2020-02-14 12:16 ` Lu Baolu
  2020-02-14 16:43 ` Derrick, Jonathan
  0 siblings, 2 replies; 3+ messages in thread
From: Daniel Drake @ 2020-02-14  9:02 UTC (permalink / raw)
  To: dwmw2, baolu.lu, joro; +Cc: bhelgaas, iommu, linux, jonathan.derrick

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.

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;
 
 		/*
-- 
2.20.1

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] iommu/vt-d: consider real PCI device when checking if mapping is needed
  2020-02-14  9:02 [PATCH v2] iommu/vt-d: consider real PCI device when checking if mapping is needed Daniel Drake
@ 2020-02-14 12:16 ` Lu Baolu
  2020-02-14 16:43 ` Derrick, Jonathan
  1 sibling, 0 replies; 3+ messages in thread
From: Lu Baolu @ 2020-02-14 12:16 UTC (permalink / raw)
  To: Daniel Drake, dwmw2, joro; +Cc: bhelgaas, iommu, linux, jonathan.derrick

Hi,

On 2020/2/14 17:02, 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.
> 
> 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>

Why not have the patch author's signed-off-by?

Best regards,
baolu

> ---
> 
> 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;
>   
>   		/*
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] iommu/vt-d: consider real PCI device when checking if mapping is needed
  2020-02-14  9:02 [PATCH v2] iommu/vt-d: consider real PCI device when checking if mapping is needed Daniel Drake
  2020-02-14 12:16 ` Lu Baolu
@ 2020-02-14 16:43 ` Derrick, Jonathan
  1 sibling, 0 replies; 3+ messages in thread
From: Derrick, Jonathan @ 2020-02-14 16:43 UTC (permalink / raw)
  To: baolu.lu, joro, drake, dwmw2; +Cc: bhelgaas, iommu, linux

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-14  9:02 [PATCH v2] iommu/vt-d: consider real PCI device when checking if mapping is needed Daniel Drake
2020-02-14 12:16 ` Lu Baolu
2020-02-14 16:43 ` Derrick, Jonathan

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