Re: [PATCH v3 5/8] iommu/vt-d: Implement def_domain_type iommu ops entry
Hi,
On 5/6/19 11:25 PM, Tom Murphy wrote:
It looks like there is a bug in this code.
The behavior before this patch in __intel_map_single was that
iommu_no_mapping would call remove the attached si_domain for 32 bit
devices (in the dmar_remove_one_dev_info(dev) call in
iommu_no_mapping) and then allocate a new domain in
get_valid_domain_for_dev
old:
if (iommu_no_mapping(dev))
return paddr;
domain = get_valid_domain_for_dev(dev);
if (!domain)
return DMA_MAPPING_ERROR;
but in the new code we remove the attached si_domain but we WON'T
allocate a new domain and instead just return an error when we call
find_domain
new:
if (iommu_no_mapping(dev))
return paddr;
domain = find_domain(dev);
if (!domain)
return DMA_MAPPING_ERROR;
This is a bug, right?
When we use the old lazy creation of iommu domain, we can change the
domain for a 32bit device from identity to dma by pulling it out of the
si_domain and allocating a new one for it.
When we switch to default domain in iommu generic layer, we can't do
this anymore. The logic in above code is if we find this case (32bit
device using an identity domain), we simple return error for dma api
and warn the user "hey, this is a 32bit device, don't use the default
pass-through mode".
I believe there should be better solutions, for example, how about
letting pci core to call iommu_request_dma_map_for_dev() when it
finds a 32bit device.
Best regards,
Lu Baolu
On Tue, Apr 30, 2019 at 3:18 AM Lu Baolu wrote:
Hi Christoph,
On 4/30/19 4:03 AM, Christoph Hellwig wrote:
@@ -3631,35 +3607,30 @@ static int iommu_no_mapping(struct device *dev)
if (iommu_dummy(dev))
return 1;
-if (!iommu_identity_mapping)
-return 0;
-
FYI, iommu_no_mapping has been refactored in for-next:
https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git/commit/?h=x86/vt-d&id=48b2c937ea37a3bece0094b46450ed5267525289
Oh, yes! Thanks for letting me know this. Will rebase the code.
found = identity_mapping(dev);
if (found) {
+/*
+ * If the device's dma_mask is less than the system's memory
+ * size then this is not a candidate for identity mapping.
+ */
+u64 dma_mask = *dev->dma_mask;
+
+if (dev->coherent_dma_mask &&
+dev->coherent_dma_mask < dma_mask)
+dma_mask = dev->coherent_dma_mask;
+
+if (dma_mask < dma_get_required_mask(dev)) {
I know this is mostly existing code moved around, but it really needs
some fixing. For one dma_get_required_mask is supposed to return the
required to not bounce mask for the given device. E.g. for a device
behind an iommu it should always just return 32-bit. If you really
want to check vs system memory please call dma_direct_get_required_mask
without the dma_ops indirection.
Second I don't even think we need to check the coherent_dma_mask,
dma_direct is pretty good at always finding memory even without
an iommu.
Third this doesn't take take the bus_dma_mask into account.
This probably should just be:
if (min(*dev->dma_mask, dev->bus_dma_mask) <
dma_direct_get_required_mask(dev)) {
Agreed and will add this in the next version.
Best regards,
Lu Baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 5/8] iommu/vt-d: Implement def_domain_type iommu ops entry
It looks like there is a bug in this code.
The behavior before this patch in __intel_map_single was that
iommu_no_mapping would call remove the attached si_domain for 32 bit
devices (in the dmar_remove_one_dev_info(dev) call in
iommu_no_mapping) and then allocate a new domain in
get_valid_domain_for_dev
old:
if (iommu_no_mapping(dev))
return paddr;
domain = get_valid_domain_for_dev(dev);
if (!domain)
return DMA_MAPPING_ERROR;
but in the new code we remove the attached si_domain but we WON'T
allocate a new domain and instead just return an error when we call
find_domain
new:
if (iommu_no_mapping(dev))
return paddr;
domain = find_domain(dev);
if (!domain)
return DMA_MAPPING_ERROR;
This is a bug, right?
On Tue, Apr 30, 2019 at 3:18 AM Lu Baolu wrote:
>
> Hi Christoph,
>
> On 4/30/19 4:03 AM, Christoph Hellwig wrote:
> >> @@ -3631,35 +3607,30 @@ static int iommu_no_mapping(struct device *dev)
> >> if (iommu_dummy(dev))
> >> return 1;
> >>
> >> -if (!iommu_identity_mapping)
> >> -return 0;
> >> -
> >
> > FYI, iommu_no_mapping has been refactored in for-next:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git/commit/?h=x86/vt-d&id=48b2c937ea37a3bece0094b46450ed5267525289
>
> Oh, yes! Thanks for letting me know this. Will rebase the code.
>
> >
> >> found = identity_mapping(dev);
> >> if (found) {
> >> +/*
> >> + * If the device's dma_mask is less than the system's memory
> >> + * size then this is not a candidate for identity mapping.
> >> + */
> >> +u64 dma_mask = *dev->dma_mask;
> >> +
> >> +if (dev->coherent_dma_mask &&
> >> +dev->coherent_dma_mask < dma_mask)
> >> +dma_mask = dev->coherent_dma_mask;
> >> +
> >> +if (dma_mask < dma_get_required_mask(dev)) {
> >
> > I know this is mostly existing code moved around, but it really needs
> > some fixing. For one dma_get_required_mask is supposed to return the
> > required to not bounce mask for the given device. E.g. for a device
> > behind an iommu it should always just return 32-bit. If you really
> > want to check vs system memory please call dma_direct_get_required_mask
> > without the dma_ops indirection.
> >
> > Second I don't even think we need to check the coherent_dma_mask,
> > dma_direct is pretty good at always finding memory even without
> > an iommu.
> >
> > Third this doesn't take take the bus_dma_mask into account.
> >
> > This probably should just be:
> >
> > if (min(*dev->dma_mask, dev->bus_dma_mask) <
> > dma_direct_get_required_mask(dev)) {
>
> Agreed and will add this in the next version.
>
> Best regards,
> Lu Baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 5/8] iommu/vt-d: Implement def_domain_type iommu ops entry
Hi Christoph,
On 4/30/19 4:03 AM, Christoph Hellwig wrote:
@@ -3631,35 +3607,30 @@ static int iommu_no_mapping(struct device *dev)
if (iommu_dummy(dev))
return 1;
- if (!iommu_identity_mapping)
- return 0;
-
FYI, iommu_no_mapping has been refactored in for-next:
https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git/commit/?h=x86/vt-d&id=48b2c937ea37a3bece0094b46450ed5267525289
Oh, yes! Thanks for letting me know this. Will rebase the code.
found = identity_mapping(dev);
if (found) {
+ /*
+* If the device's dma_mask is less than the system's memory
+* size then this is not a candidate for identity mapping.
+*/
+ u64 dma_mask = *dev->dma_mask;
+
+ if (dev->coherent_dma_mask &&
+ dev->coherent_dma_mask < dma_mask)
+ dma_mask = dev->coherent_dma_mask;
+
+ if (dma_mask < dma_get_required_mask(dev)) {
I know this is mostly existing code moved around, but it really needs
some fixing. For one dma_get_required_mask is supposed to return the
required to not bounce mask for the given device. E.g. for a device
behind an iommu it should always just return 32-bit. If you really
want to check vs system memory please call dma_direct_get_required_mask
without the dma_ops indirection.
Second I don't even think we need to check the coherent_dma_mask,
dma_direct is pretty good at always finding memory even without
an iommu.
Third this doesn't take take the bus_dma_mask into account.
This probably should just be:
if (min(*dev->dma_mask, dev->bus_dma_mask) <
dma_direct_get_required_mask(dev)) {
Agreed and will add this in the next version.
Best regards,
Lu Baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 5/8] iommu/vt-d: Implement def_domain_type iommu ops entry
> @@ -3631,35 +3607,30 @@ static int iommu_no_mapping(struct device *dev)
> if (iommu_dummy(dev))
> return 1;
>
> - if (!iommu_identity_mapping)
> - return 0;
> -
FYI, iommu_no_mapping has been refactored in for-next:
https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git/commit/?h=x86/vt-d&id=48b2c937ea37a3bece0094b46450ed5267525289
> found = identity_mapping(dev);
> if (found) {
> + /*
> + * If the device's dma_mask is less than the system's memory
> + * size then this is not a candidate for identity mapping.
> + */
> + u64 dma_mask = *dev->dma_mask;
> +
> + if (dev->coherent_dma_mask &&
> + dev->coherent_dma_mask < dma_mask)
> + dma_mask = dev->coherent_dma_mask;
> +
> + if (dma_mask < dma_get_required_mask(dev)) {
I know this is mostly existing code moved around, but it really needs
some fixing. For one dma_get_required_mask is supposed to return the
required to not bounce mask for the given device. E.g. for a device
behind an iommu it should always just return 32-bit. If you really
want to check vs system memory please call dma_direct_get_required_mask
without the dma_ops indirection.
Second I don't even think we need to check the coherent_dma_mask,
dma_direct is pretty good at always finding memory even without
an iommu.
Third this doesn't take take the bus_dma_mask into account.
This probably should just be:
if (min(*dev->dma_mask, dev->bus_dma_mask) <
dma_direct_get_required_mask(dev)) {
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v3 5/8] iommu/vt-d: Implement def_domain_type iommu ops entry
It will be used by the iommu generic framework to check the
default domain types that Intel IOMMU driver supports for a
specific device. Return IOMMU_DOMAIN_DMA if device requires
a non-identity domain; Return IOMMU_DOMAIN_IDENTITY if the
device requires to use an identity domain. Otherwise return
IOMMU_DOMAIN_ANY which indicates that both domain types are
supported, hence the static default domain type defined in
iommu generic layer will be used.
Signed-off-by: Lu Baolu
---
drivers/iommu/intel-iommu.c | 86 +
1 file changed, 29 insertions(+), 57 deletions(-)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 847e4a326d29..d2b51e045603 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2858,9 +2858,6 @@ static int identity_mapping(struct device *dev)
{
struct device_domain_info *info;
- if (likely(!iommu_identity_mapping))
- return 0;
-
info = dev->archdata.iommu;
if (info && info != DUMMY_DEVICE_DOMAIN_INFO)
return (info->domain == si_domain);
@@ -2945,29 +2942,26 @@ static bool device_is_rmrr_locked(struct device *dev)
return true;
}
-static int iommu_should_identity_map(struct device *dev, int startup)
+static int intel_iommu_def_domain_type(struct device *dev)
{
if (dev_is_pci(dev)) {
struct pci_dev *pdev = to_pci_dev(dev);
if (device_is_rmrr_locked(dev))
- return 0;
+ return IOMMU_DOMAIN_DMA;
/*
* Prevent any device marked as untrusted from getting
* placed into the statically identity mapping domain.
*/
if (pdev->untrusted)
- return 0;
+ return IOMMU_DOMAIN_DMA;
if ((iommu_identity_mapping & IDENTMAP_AZALIA) &&
IS_AZALIA(pdev))
- return 1;
+ return IOMMU_DOMAIN_IDENTITY;
if ((iommu_identity_mapping & IDENTMAP_GFX) &&
IS_GFX_DEVICE(pdev))
- return 1;
-
- if (!(iommu_identity_mapping & IDENTMAP_ALL))
- return 0;
+ return IOMMU_DOMAIN_IDENTITY;
/*
* We want to start off with all devices in the 1:1 domain, and
@@ -2988,43 +2982,25 @@ static int iommu_should_identity_map(struct device
*dev, int startup)
*/
if (!pci_is_pcie(pdev)) {
if (!pci_is_root_bus(pdev->bus))
- return 0;
+ return IOMMU_DOMAIN_DMA;
if (pdev->class >> 8 == PCI_CLASS_BRIDGE_PCI)
- return 0;
+ return IOMMU_DOMAIN_DMA;
} else if (pci_pcie_type(pdev) == PCI_EXP_TYPE_PCI_BRIDGE)
- return 0;
+ return IOMMU_DOMAIN_DMA;
} else {
if (device_has_rmrr(dev))
- return 0;
- }
-
- /*
-* At boot time, we don't yet know if devices will be 64-bit capable.
-* Assume that they will — if they turn out not to be, then we can
-* take them out of the 1:1 domain later.
-*/
- if (!startup) {
- /*
-* If the device's dma_mask is less than the system's memory
-* size then this is not a candidate for identity mapping.
-*/
- u64 dma_mask = *dev->dma_mask;
-
- if (dev->coherent_dma_mask &&
- dev->coherent_dma_mask < dma_mask)
- dma_mask = dev->coherent_dma_mask;
-
- return dma_mask >= dma_get_required_mask(dev);
+ return IOMMU_DOMAIN_DMA;
}
- return 1;
+ return (iommu_identity_mapping & IDENTMAP_ALL) ?
+ IOMMU_DOMAIN_IDENTITY : IOMMU_DOMAIN_ANY;
}
static int __init dev_prepare_static_identity_mapping(struct device *dev, int
hw)
{
int ret;
- if (!iommu_should_identity_map(dev, 1))
+ if (intel_iommu_def_domain_type(dev) != IOMMU_DOMAIN_IDENTITY)
return 0;
ret = domain_add_dev_info(si_domain, dev);
@@ -3631,35 +3607,30 @@ static int iommu_no_mapping(struct device *dev)
if (iommu_dummy(dev))
return 1;
- if (!iommu_identity_mapping)
- return 0;
-
found = identity_mapping(dev);
if (found) {
- if (iommu_should_identity_map(dev, 0))
- return 1;
- else {
+ /*
+* If the device's dma_mask is less than the system's memory
+* size then this is not a candidate for identity mapping.
+*/
+ u64 dma_mask = *dev->dma_mas
