Re: [PATCH] iommu/vt-d: consider real PCI device when checking if mapping is needed

[Robin Murphy]

On 12/02/2020 3:17 am, Daniel Drake wrote:

On Wed, Feb 12, 2020 at 12:03 AM Derrick, Jonathan
 wrote:

On Tue, 2020-02-11 at 17:13 +0800, Daniel Drake wrote:

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, when allocating memory through (e.g.) intel_map_page().
Confusion ensues if the iommu domain type for the subdevice does not match
the iommu domain type for the real DMA device.

Is there a way to force this situation for my testing?


I think you could hack device_def_domain_type() to return
IOMMU_DOMAIN_IDENTITY for the real device, and IOMMU_DOMAIN_DMA for
the subdevice.


As far as I'm aware that should only be possible if the subdevice has a 
distinct physical requester ID from the real device, which given the 
nomenclature I assume is not the case.


(well, technically you *can* start routing different logical streams 
from a single requester ID to multiple domains if you have PASIDs, but 
that's a whole other ball game)


Robin.


But I got curious as to why my subdevice might be IOMMU_DOMAIN_DMA, so
I checked, and found out that my assumptions weren't quite correct.
The subdevice has no iommu domain recorded at all. Before applying any
patches here, what's actually happening is:

1. Real DMA device gets registered with the iommu as
IOMMU_DOMAIN_IDENTITY using si_domain.
2. When the subdevice gets registered, the relevant code flow is
inside dmar_insert_one_dev_info():
  - it creates a new device_domain_info and domain->domain.type == IDENTITY, but
  - it then calls find_domain(dev) which successfully defers to the
real DMA device and returns the real DMA device's dmar_domain
  - since found != NULL (dmar_domain was found for this device) the
function bails out before setting dev->archdata.iommu

The results at this point are that the real DMA device is fully
registered as IOMMU_DOMAIN_IDENTITY using si_domain, but all of the
subdevices will always have dev->archdata.iommu == NULL.

Then when intel_map_page() is reached for the subdevice, it calls
iommu_need_mapping() for the subdevice.
This calls identity_mapping() on the subdevice, but that will always
return 0 because dev->archdata.iommu == NULL.
Following on from there, iommu_need_mapping() will then *always*
return true (mapping needed) for subdevices.

That will then lead to the situation described in my last mail, where
later down the allocation chain the request for creating a mapping
will be handed towards the real DMA dev, but that will then fail
because the real DMA dev is using IOMMU_DOMAIN_IDENTITY where no
mapping is needed.

Unless I missed anything that seems pretty clear to me now, and I
guess the only reason why you may not have already faced this in the
vmd case is if the real DMA device is not using IOMMU_DOMAIN_IDENTITY.
(To check this, you could log the value of the real dev
domain->domain.type in dmar_insert_one_dev_info(), and/or observe the
return value of identity_mapping() in iommu_need_mapping for the real
dev).

In any case it seems increasingly clear to me that
iommu_need_mapping() should be consulting the real DMA device in the
identity_mapping check, and your patch likewise solves the problem
faced here.

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


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

Re: [PATCH] iommu/vt-d: consider real PCI device when checking if mapping is needed

[Daniel Drake]

On Wed, Feb 12, 2020 at 12:03 AM Derrick, Jonathan
 wrote:
> On Tue, 2020-02-11 at 17:13 +0800, Daniel Drake wrote:
> > 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, when allocating memory through (e.g.) intel_map_page().
> > Confusion ensues if the iommu domain type for the subdevice does not match
> > the iommu domain type for the real DMA device.
> Is there a way to force this situation for my testing?

I think you could hack device_def_domain_type() to return
IOMMU_DOMAIN_IDENTITY for the real device, and IOMMU_DOMAIN_DMA for
the subdevice.

But I got curious as to why my subdevice might be IOMMU_DOMAIN_DMA, so
I checked, and found out that my assumptions weren't quite correct.
The subdevice has no iommu domain recorded at all. Before applying any
patches here, what's actually happening is:

1. Real DMA device gets registered with the iommu as
IOMMU_DOMAIN_IDENTITY using si_domain.
2. When the subdevice gets registered, the relevant code flow is
inside dmar_insert_one_dev_info():
 - it creates a new device_domain_info and domain->domain.type == IDENTITY, but
 - it then calls find_domain(dev) which successfully defers to the
real DMA device and returns the real DMA device's dmar_domain
 - since found != NULL (dmar_domain was found for this device) the
function bails out before setting dev->archdata.iommu

The results at this point are that the real DMA device is fully
registered as IOMMU_DOMAIN_IDENTITY using si_domain, but all of the
subdevices will always have dev->archdata.iommu == NULL.

Then when intel_map_page() is reached for the subdevice, it calls
iommu_need_mapping() for the subdevice.
This calls identity_mapping() on the subdevice, but that will always
return 0 because dev->archdata.iommu == NULL.
Following on from there, iommu_need_mapping() will then *always*
return true (mapping needed) for subdevices.

That will then lead to the situation described in my last mail, where
later down the allocation chain the request for creating a mapping
will be handed towards the real DMA dev, but that will then fail
because the real DMA dev is using IOMMU_DOMAIN_IDENTITY where no
mapping is needed.

Unless I missed anything that seems pretty clear to me now, and I
guess the only reason why you may not have already faced this in the
vmd case is if the real DMA device is not using IOMMU_DOMAIN_IDENTITY.
(To check this, you could log the value of the real dev
domain->domain.type in dmar_insert_one_dev_info(), and/or observe the
return value of identity_mapping() in iommu_need_mapping for the real
dev).

In any case it seems increasingly clear to me that
iommu_need_mapping() should be consulting the real DMA device in the
identity_mapping check, and your patch likewise solves the problem
faced here.

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

Re: [PATCH] iommu/vt-d: consider real PCI device when checking if mapping is needed

[Derrick, Jonathan]

Hi Daniel

On Tue, 2020-02-11 at 17:13 +0800, Daniel Drake wrote:
> 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, when allocating memory through (e.g.) intel_map_page().
> Confusion ensues if the iommu domain type for the subdevice does not match
> the iommu domain type for the real DMA device.
Is there a way to force this situation for my testing? 

> 
> For example, take the case of the subdevice handled by intel_map_page()
> in a IOMMU_DOMAIN_DMA, with the real DMA device in a
> IOMMU_DOMAIN_IDENTITY:
> 
> 1. intel_map_page() checks if an IOMMU mapping is needed by calling
>iommu_need_mapping() on the subdevice. Result: mapping is needed.
> 2. __intel_map_single() is called to create the mapping:
>   - __intel_map_single() calls find_domain(). This function now returns
> the IDENTITY domain corresponding to the real DMA device.
>   - __intel_map_single() then 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. The above case will then directly fall back on
> dma_direct_map_page().
> 
> Fixes: 2b0140c69637 ("iommu/vt-d: Use pci_real_dma_dev() for mapping")
> Signed-off-by: Daniel Drake 
> ---
> 
> Notes:
> This problem was detected with a non-upstream patch
> "PCI: Add Intel remapped NVMe device support"
> (https://marc.info/?l=linux-ide=156015271021615=2)
> 
> This patch creates PCI devices in the same way as VMD, and hence
> I believe VMD would hit this class of problem for any cases where
> iommu domain type may mismatch between subdevice and real device,
> which we have run into here.
> 
> 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 | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 9dc37672bf89..713810f8350c 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -3587,6 +3587,9 @@ static bool iommu_need_mapping(struct device *dev)
>   if (iommu_dummy(dev))
>   return false;
>  
> + if (dev_is_pci(dev))
> + dev = _real_dma_dev(to_pci_dev(dev))->dev;
> +
>   ret = identity_mapping(dev);
>   if (ret) {
>   u64 dma_mask = *dev->dma_mask;
This will be a problem. We really want to use the subdevice's dma mask
in case there's a situation where the subdevice only supports 32-bit
dma (with the real dma requester having a 64-bit dma mask)



Would this work?

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 9dc3767..8f35e6b 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -3582,19 +3582,24 @@ 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;
 
-   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 (dma_mask >= dma_direct_get_required_mask(dev))
+   if (dev_is_pci(dev))
+   dev = _real_dma_dev(to_pci_dev(dev))->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

[PATCH] iommu/vt-d: consider real PCI device when checking if mapping is needed

[Daniel Drake]

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, when allocating memory through (e.g.) intel_map_page().
Confusion ensues if the iommu domain type for the subdevice does not match
the iommu domain type for the real DMA device.

For example, take the case of the subdevice handled by intel_map_page()
in a IOMMU_DOMAIN_DMA, with the real DMA device in a
IOMMU_DOMAIN_IDENTITY:

1. intel_map_page() checks if an IOMMU mapping is needed by calling
   iommu_need_mapping() on the subdevice. Result: mapping is needed.
2. __intel_map_single() is called to create the mapping:
  - __intel_map_single() calls find_domain(). This function now returns
the IDENTITY domain corresponding to the real DMA device.
  - __intel_map_single() then 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. The above case will then directly fall back on
dma_direct_map_page().

Fixes: 2b0140c69637 ("iommu/vt-d: Use pci_real_dma_dev() for mapping")
Signed-off-by: Daniel Drake 
---

Notes:
This problem was detected with a non-upstream patch
"PCI: Add Intel remapped NVMe device support"
(https://marc.info/?l=linux-ide=156015271021615=2)

This patch creates PCI devices in the same way as VMD, and hence
I believe VMD would hit this class of problem for any cases where
iommu domain type may mismatch between subdevice and real device,
which we have run into here.

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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 9dc37672bf89..713810f8350c 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -3587,6 +3587,9 @@ static bool iommu_need_mapping(struct device *dev)
if (iommu_dummy(dev))
return false;
 
+   if (dev_is_pci(dev))
+   dev = _real_dma_dev(to_pci_dev(dev))->dev;
+
ret = identity_mapping(dev);
if (ret) {
u64 dma_mask = *dev->dma_mask;
-- 
2.20.1

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