Hi James,
On 3/22/19 6:05 PM, James Sewart wrote:
Hey Lu,
On 20 Mar 2019, at 01:26, Lu Baolu <[email protected]> wrote:
Hi James,
On 3/19/19 9:35 PM, James Sewart wrote:
Hey Lu,
On 15 Mar 2019, at 03:13, Lu Baolu <[email protected]> wrote:
Hi James,
On 3/14/19 7:56 PM, James Sewart wrote:
Patches 1 and 2 are the same as v1.
v1-v2:
Refactored ISA direct mappings to be returned by iommu_get_resv_regions.
Integrated patch by Lu to defer turning on DMAR until iommu.c has mapped
reserved regions.
Integrated patches by Lu to remove more unused code in cleanup.
Lu: I didn't integrate your patch to set the default domain type as it
isn't directly related to the aim of this patchset. Instead patch 4
Without those patches, user experience will be affected and some devices
will not work on Intel platforms anymore.
For a long time, Intel IOMMU driver has its own logic to determine
whether a device requires an identity domain. For example, when user
specifies "iommu=pt" in kernel parameter, all device will be attached
with the identity domain. Further more, some quirky devices require
an identity domain to be used before enabling DMA remapping, otherwise,
it will not work. This was done by adding quirk bits in Intel IOMMU
driver.
So from my point of view, one way is porting all those quirks and kernel
parameters into IOMMU generic layer, or opening a door for vendor IOMMU
driver to determine the default domain type by their own. I prefer the
latter option since it will not impact any behaviors on other
architectures.
I see your point. I’m not confident that using the proposed door to set a
groups default domain has the desired behaviour. As discussed before the
default domain type will be set based on the desired type for only the
first device attached to a group. I think to change the default domain
type you would need a slightly different door that wasn’t conditioned on
device.
I think this as another problem. Just a summarize for the ease of
discussion. We saw two problems:
1. When allocating a new group for a device, how should we determine the
type of the default domain? This is what my proposal patches trying to
address.
This will work as expected only if all devices within a group get the same
result from is_identity_map. Otherwise wee see issue 2.
2. If we need to put a device into an existing group which uses a
different type of domain from what the device desires to use, we might
break the functionality of the device. For this problem I'd second your
proposal below if I get your point correctly.
For situations where individual devices require an identity domain because
of quirks then maybe calling is_identity_map per device in
iommu_group_get_for_dev is a better solution than the one I proposed.
Do you mean if we see a quirky device requires a different domain type
other than the default domain type of the group, we will assign a new
group to it? That looks good to me as far as I can see. I suppose this
should be done in vt-d's ops callback.
I have thought about this a bit and I think the cleanest approach is to
put devices that require an identity domain into their own group, this can
be done in the device_group callback, avoiding any situation where we have
to deal with creating a new group based on domain type in
iommu_group_get_for_dev. This way we shouldn’t ever be in a situation with
multiple different domain types per group. This way your patches will work
as expected. See below for a possible implementation.
Best regards,
Lu Baolu
Cheers,
James.
Devices that require an identity map because of quirks or other reasons
should be put in their own IOMMU group so as to not end up with multiple
different domains per group.
Yeah! This looks good to me.
Best regards,
Lu Baolu
Signed-off-by: James Sewart <[email protected]>
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 3cb8b36abf50..0f5a121d31a0 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -5421,6 +5421,22 @@ struct intel_iommu *intel_svm_device_to_iommu(struct
device *dev)
}
#endif /* CONFIG_INTEL_IOMMU_SVM */
+static struct iommu_group *intel_identity_group;
+
+struct iommu_group *intel_iommu_pci_device_group(struct device *dev)
+{
+ if (iommu_no_mapping(dev)) {
+ if (!intel_identity_group) {
+ intel_identity_group = iommu_group_alloc();
+ if (IS_ERR(intel_identity_group))
+ return NULL;
+ }
+ return intel_identity_group;
+ }
+
+ return pci_device_group(dev);
+}
+
const struct iommu_ops intel_iommu_ops = {
.capable = intel_iommu_capable,
.domain_alloc = intel_iommu_domain_alloc,
@@ -5435,7 +5451,7 @@ const struct iommu_ops intel_iommu_ops = {
.get_resv_regions = intel_iommu_get_resv_regions,
.put_resv_regions = intel_iommu_put_resv_regions,
.apply_resv_region = intel_iommu_apply_resv_region,
- .device_group = pci_device_group,
+ .device_group = intel_iommu_pci_device_group,
.pgsize_bitmap = INTEL_IOMMU_PGSIZES,
};
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
