Patchwork [4/4] iommu/vt-d: Remove lazy allocation of domains

login
register
mail settings
Submitter Lu Baolu
Date March 8, 2019, 3:09 a.m.
Message ID <bb9c941b-05b6-27aa-d0ca-1d9c56f8f277@linux.intel.com>
Download mbox | patch
Permalink /patch/744235/
State New
Headers show

Comments

Lu Baolu - March 8, 2019, 3:09 a.m.
Hi James,

On 3/7/19 6:21 PM, James Sewart wrote:
> Hey Lu,
> 
>> On 7 Mar 2019, at 06:31, Lu Baolu <baolu.lu@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.

----------------

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.

----------------

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.

----------------

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.

----------------

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
Benjamin Serebrin via iommu - March 8, 2019, 4:57 p.m.
Hey Lu,

> On 8 Mar 2019, at 03:09, Lu Baolu <baolu.lu@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.lu@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.
Lu Baolu - March 10, 2019, 2:51 a.m.
Hi,

On 3/9/19 7:49 PM, James Sewart wrote:
> Hey Lu,
> 
>> On 9 Mar 2019, at 01:53, Lu Baolu <baolu.lu@linux.intel.com> wrote:
>>
>> Hi James,
>>
>> On 3/9/19 12:57 AM, James Sewart wrote:
>>> Hey Lu,
>>>> On 8 Mar 2019, at 03:09, Lu Baolu<baolu.lu@linux.intel.com>  wrote:
>>>>>>
>>>>>> 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?
>>
>> Good catch!
>>
>> This is supposed to be handled in iommu_no_mapping(). But, obviously
>> current code sticks to lazy domain allocation. I'm not sure whether
>> there are any real such cases, but we should handle it in a clean way.
>> My idea is that we could downgrade to multiple domains per group (just
>> like what we have now) in this case and print a kernel message for this.
> 
> I think if a device requires an identity domain, then it should ignore
> attempts to attach to something else. A print to warn a user about this
> would be a good idea.
> 
> I figure during attach: if iommu_no_mapping() then attach to si_domain and
> print, else continue with the given domain.

Yes, agree.

Best regards,
Lu Baolu

Patch

From 3d02365161a3b1441aaf1d36940758df2b4f2b1c Mon Sep 17 00:00:00 2001
From: Lu Baolu <baolu.lu@linux.intel.com>
Date: Fri, 8 Mar 2019 09:34:39 +0800
Subject: [PATCH 12/12] iommu/vt-d: remove prepare identity map during boot

---
 drivers/iommu/intel-iommu.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index aa33f65b32cc..e7a94bb927c4 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2839,18 +2839,10 @@  static int __init dev_prepare_static_identity_mapping(struct device *dev, int hw
 
 static int __init iommu_prepare_static_identity_mapping(int hw)
 {
-	struct pci_dev *pdev = NULL;
 	struct dmar_drhd_unit *drhd;
 	struct intel_iommu *iommu;
 	struct device *dev;
-	int i;
-	int ret = 0;
-
-	for_each_pci_dev(pdev) {
-		ret = dev_prepare_static_identity_mapping(&pdev->dev, hw);
-		if (ret)
-			return ret;
-	}
+	int i, ret = 0;
 
 	for_each_active_iommu(iommu, drhd)
 		for_each_active_dev_scope(drhd->devices, drhd->devices_cnt, i, dev) {
-- 
2.17.1