Hey Lu, > On 8 Mar 2019, at 03:09, Lu Baolu <baolu...@linux.intel.com> wrote: > > Hi James, > > On 3/7/19 6:21 PM, James Sewart wrote: >> Hey Lu, >>> On 7 Mar 2019, at 06:31, Lu Baolu <baolu...@linux.intel.com> wrote: >>> >>> Hi James, >>> >>> On 3/7/19 2:08 AM, James Sewart wrote: >>>>>>>> - /* >>>>>>>> - * For each rmrr >>>>>>>> - * for each dev attached to rmrr >>>>>>>> - * do >>>>>>>> - * locate drhd for dev, alloc domain for dev >>>>>>>> - * allocate free domain >>>>>>>> - * allocate page table entries for rmrr >>>>>>>> - * if context not allocated for bus >>>>>>>> - * allocate and init context >>>>>>>> - * set present in root table for this bus >>>>>>>> - * init context with domain, translation etc >>>>>>>> - * endfor >>>>>>>> - * endfor >>>>>>>> - */ >>>>>>>> - pr_info("Setting RMRR:\n"); >>>>>>>> - for_each_rmrr_units(rmrr) { >>>>>>>> - /* some BIOS lists non-exist devices in DMAR table. */ >>>>>>>> - for_each_active_dev_scope(rmrr->devices, >>>>>>>> rmrr->devices_cnt, >>>>>>>> - i, dev) { >>>>>>>> - ret = iommu_prepare_rmrr_dev(rmrr, dev); >>>>>>>> - if (ret) >>>>>>>> - pr_err("Mapping reserved region >>>>>>>> failed\n"); >>>>>>>> - } >>>>>>>> - } >>>>>>>> - >>>>>>>> - iommu_prepare_isa(); >>>>>>> Why do you want to remove this segment of code? >>>>>> This will only work if the lazy allocation of domains exists, these >>>>>> mappings will disappear once a default domain is attached to a device and >>>>>> then remade by iommu_group_create_direct_mappings. This code is redundant >>>>>> and removing it allows us to remove all the lazy allocation logic. >>>>> No exactly. >>>>> >>>>> We need to setup the rmrr mapping before enabling dma remapping, >>>>> otherwise, there will be a window after dma remapping enabling and >>>>> rmrr getting mapped, during which people might see dma faults. >>>> Do you think this patch instead should be a refactoring to simplify this >>>> initial domain setup before the default domain takes over? It seems like >>>> duplicated effort to have both lazy allocated domains and externally >>>> managed domains. We could allocate a domain here for any devices with RMRR >>>> and call get_resv_regions to avoid duplicating RMRR loop. >>> >>> Agree. We should replace the lazy allocated domains with default domain >>> in a clean way. Actually, your patches look great to me. But I do think >>> we need further cleanups. The rmrr code is one example, and the identity >>> domain (si_domain) is another. >>> >>> Do you mind if I work on top of your patches for further cleanups and >>> sign off a v2 together with you? >> Sure, sounds good. I’ll fixup patch 3 and have a go at integrating >> iommu_prepare_isa into get_resv_regions. This should make the initial >> domain logic here quite concise. > > Here attached three extra patches which I think should be added before > PATCH 3/4, and some further cleanup changes which you can merge with > PATCH 4/4. > > ---------------- > > 0006-iommu-Add-ops-entry-for-vendor-specific-default-doma.patch > 0007-iommu-vt-d-Add-is_identity_map-ops-entry.patch > > These two patches aim to add a generic method for vendor specific iommu > drivers to specify the type of the default domain for a device. Intel > iommu driver will register an ops for this since it already has its own > identity map adjudicator for a long time.
This seems like a good idea, but as domain alloc is only called for the default domain on first device attached to a group, we may miss checking whether a device added later should have an identity domain. Should there be paths to downgrade a groups domain if one of the devices requires one? > > ---------------- > > 0008-iommu-vt-d-Enable-DMA-remapping-after-rmrr-mapped.patch > > This aims to address the requirement of rmrr identity map before > enabling DMA remapping. With default domain mechanism, the default > domain will be allocated and attached to the device in bus_set_iommu() > during boot. Move enabling DMA remapping engines after bus_set_iommu() > will fix the rmrr issue. Thats pretty neat, avoids any temporary domain allocation, nice! > > ---------------- > > 0009-return-si_domain-directly.patch > > I suggest that we should keep current si_domain implementation since > changing si_domain is not the purpose of this patch set. Please merge > this with PATCH 3/4 if you like it. Seems reasonable. > > ---------------- > > 0010-iommu-vt-d-remove-floopy-workaround.patch > 0011-iommu-vt-d-remove-prepare-rmrr-helpers.patch > 0012-iommu-vt-d-remove-prepare-identity-map-during-boot.patch > > Above patches are further cleanups as the result of switching to default > domain. Please consider to merge them with PATCH 4/4. Nice, good catch. > > ---------------- > > I have done some sanity checks on my local machine against all patches. > I can do more tests after you submit a v2. > > > Best regards, > Lu Baolu > > > <0006-iommu-Add-ops-entry-for-vendor-specific-default-doma.patch><0007-iommu-vt-d-Add-is_identity_map-ops-entry.patch><0008-iommu-vt-d-Enable-DMA-remapping-after-rmrr-mapped.patch><0009-return-si_domain-directly.patch><0010-iommu-vt-d-remove-floopy-workaround.patch><0011-iommu-vt-d-remove-prepare-rmrr-helpers.patch><0012-iommu-vt-d-remove-prepare-identity-map-during-boot.patch> I have revised my patches and hope to do some testing before reposting. Cheers, James. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu