Messages in this thread | | | From | Tom Murphy <> | Date | Thu, 25 Apr 2019 00:47:32 +0100 | Subject | Re: [PATCH v2 3/7] iommu/vt-d: Expose ISA direct mapping region via iommu_get_resv_regions |
| |
I can see two potential problems with these patches that should be addressed:
The default domain of a group can be changed to type IOMMU_DOMAIN_IDENTITY via the command line. With these patches we are returning the si_domain for type IOMMU_DOMAIN_IDENTITY. There's a chance the shared si_domain could be freed while it is still being used when a group is freed. For example here in the iommu_group_release: https://github.com/torvalds/linux/blob/cd8dead0c39457e58ec1d36db93aedca811d48f1/drivers/iommu/iommu.c#L376 "if (group->default_domain) iommu_domain_free(group->default_domain);"
Also now that a domain is attached to a device earlier we should implement the is_attach_deferred call-back and use it to defer the domain attach from iommu driver init to device driver init when iommu is pre-enabled in kdump kernel. like in this commit: https://github.com/torvalds/linux/commit/df3f7a6e8e855e4ff533508807cd7c3723faa51f
On Tue, Apr 16, 2019 at 3:24 AM Lu Baolu <baolu.lu@linux.intel.com> wrote: > > Hi James, > > On 4/15/19 10:16 PM, James Sewart wrote: > > Hey Lu, > > > >> On 10 Apr 2019, at 06:22, Lu Baolu <baolu.lu@linux.intel.com> wrote: > >> > >> Hi James, > >> > >> On 4/6/19 2:02 AM, James Sewart wrote: > >>> Hey Lu, > >>> My bad, did some debugging on my end. The issue was swapping out > >>> find_domain for iommu_get_domain_for_dev. It seems in some situations the > >>> domain is not attached to the group but the device is expected to have the > >>> domain still stored in its archdata. > >>> I’ve attached the final patch with find_domain unremoved which seems to > >>> work in my testing. > >> > >> Just looked into your v3 patch set and some thoughts from my end posted > >> here just for your information. > >> > >> Let me post the problems we want to address. > >> > >> 1. When allocating a new group for a device, how should we determine the > >> type of the default domain? > >> > >> 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. > >> > >> My new thought is letting the iommu generic code to determine the > >> default domain type (hence my proposed vendor specific default domain > >> type patches could be dropped). > >> > >> If the default domain type is dynamical mapping, and later in iommu_no_mapping(), we determines that we must use an identity domain, > >> we then call iommu_request_dm_for_dev(dev). > >> > >> If the default domain type is identity mapping, and later in > >> iommu_no_mapping(), we determined that we must use a dynamical domain, > >> we then call iommu_request_dma_domain_for_dev(dev). > >> > >> We already have iommu_request_dm_for_dev() in iommu.c. We only need to > >> implement iommu_request_dma_domain_for_dev(). > >> > >> With this done, your patch titled "Create an IOMMU group for devices > >> that require an identity map" could also be dropped. > >> > >> Any thoughts? > > > > Theres a couple issues I can think of. iommu_request_dm_for_dev changes > > the domain for all devices within the devices group, if we may have > > devices with different domain requirements in the same group then only the > > last initialised device will get the domain it wants. If we want to add > > iommu_request_dma_domain_for_dev then we would still need another IOMMU > > group for identity domain devices. > > Current solution (a.k.a. v3) for this is to assign a new group to the > device which requires an identity mapped domain. This will break the > functionality of device direct pass-through (to user level). The iommu > group represents the minimum granularity of iommu isolation and > protection. The requirement of identity mapped domain should not be a > factor for a new group. > > Both iomm_request_dma_domain_for_dev() or iommu_request_dm_for_dev() > only support groups with a single device in it. > > The users could choose between domains of DMA type or IDENTITY type for > a group. If multiple devices share a single group and both don't work > for them, they have to disable the IOMMU. This isn't a valid > configuration from IOMMU's point of view. > > > > > Both with v3 and the proposed method here, iommu_should_identity_map is > > determining whether the device requires an identity map. In v3 this is > > called once up front by the generic code to determine for the IOMMU group > > which domain type to use. In the proposed method I think this would happen > > after initial domain allocation and would trigger the domain to be > > replaced with a different domain. > > > > I prefer the solution in v3. What do you think? > > The only concern of solution in v3 from my point of view is some kind of > duplication. Even we can ask the Intel IOMMU driver to tell the default > domain type, we might change it after boot up. For example, for 32-bit > pci devices, we don't know whether it's 64-bit or 32-bit capable during > IOMMU initialization, so we might tell iommu.c to use identity mapped > domain. After boot up, we check it again, and find out that it's only > 32-bit capable (hence only can access physical memory below 4G) and the > default identity domain doesn't work. And we have to change it to DMA > domain via iommu_request_dma_domain_for_dev(). > > So to make it simple and straight-forward, we can just let iommu.c to > determine the default domain type and after that the Intel IOMMU driver > could tweak it according to the quirk bits in the driver. > > Best regards, > Lu Baolu
| |