Hi Daniel, sorry for the delay
On Fri, 2020-02-14 at 17:02 +0800, Daniel Drake wrote:
> From: Jon Derrick <jonathan.derr...@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 <dr...@endlessm.com>
> Fixes: b0140c69637e ("iommu/vt-d: Use pci_real_dma_dev() for mapping")
> Signed-off-by: Daniel Drake <dr...@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
