Re: [PATCH 4/4] iommu/vt-d: Remove lazy allocation of domains
Hi, On 3/9/19 7:49 PM, James Sewart wrote: Hey Lu, On 9 Mar 2019, at 01:53, Lu Baolu wrote: Hi James, On 3/9/19 12:57 AM, James Sewart wrote: Hey Lu, On 8 Mar 2019, at 03:09, Lu Baolu 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
Re: [PATCH 4/4] iommu/vt-d: Remove lazy allocation of domains
Hey Lu, > On 9 Mar 2019, at 01:53, Lu Baolu wrote: > > Hi James, > > On 3/9/19 12:57 AM, James Sewart wrote: >> Hey Lu, >>> On 8 Mar 2019, at 03:09, Lu Baolu 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. > > Or any other thoughts? > > Best regards, > Lu Baolu Cheers, James.
Re: [PATCH 4/4] iommu/vt-d: Remove lazy allocation of domains
Hi James, On 3/9/19 12:57 AM, James Sewart wrote: Hey Lu, On 8 Mar 2019, at 03:09, Lu Baolu 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. Or any other thoughts? Best regards, Lu Baolu
Re: [PATCH 4/4] iommu/vt-d: Remove lazy allocation of domains
Hey Lu,
> On 8 Mar 2019, at 03:09, Lu Baolu wrote:
>
> Hi James,
>
> On 3/7/19 6:21 PM, James Sewart wrote:
>> Hey Lu,
>>> On 7 Mar 2019, at 06:31, Lu Baolu 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 hav
Re: [PATCH 4/4] iommu/vt-d: Remove lazy allocation of domains
Hi James,
On 3/7/19 6:21 PM, James Sewart wrote:
Hey Lu,
On 7 Mar 2019, at 06:31, Lu Baolu 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
>From d1625f1e8461cef23bc8697ec51382c47b92fa5a Mon Sep 17 00:00:00 2001
From: Lu Baolu
Date: Thu, 7 Mar 2019 10:50:27 +0800
Subject: [PATCH 06/12] iommu: Add ops entry for vendor specific default domain
type
This adds an iommu ops entry for iommu vendor driver to specify
the type of the default domain for each device. This is needed
for the vendor driver, which already has its own logic to
determine the use of identity domain for a long time, when it
switches to apply default domain.
Signed-off-by: Lu Baolu
---
drivers/iommu/iommu.c | 12 +---
include/linux/iommu.h | 4
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 374327018a11..4101f38a7844 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/i
Re: [PATCH 4/4] iommu/vt-d: Remove lazy allocation of domains
Hi,
On 3/7/19 6:21 PM, James Sewart wrote:
Hey Lu,
On 7 Mar 2019, at 06:31, Lu Baolu 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.
Very appreciated! Thank you!
Best regards,
Lu Baolu
Re: [PATCH 4/4] iommu/vt-d: Remove lazy allocation of domains
Hey Lu,
> On 7 Mar 2019, at 06:31, Lu Baolu 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.
>
> Best regards,
> Lu Baolu
Cheers,
James.
Re: [PATCH 4/4] iommu/vt-d: Remove lazy allocation of domains
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?
Best regards,
Lu Baolu
Re: [PATCH 4/4] iommu/vt-d: Remove lazy allocation of domains
Hey,
> On 6 Mar 2019, at 07:00, Lu Baolu wrote:
>
> Hi,
>
> On 3/5/19 7:46 PM, James Sewart wrote:
>> Hey Lu,
>>> On 5 Mar 2019, at 06:59, Lu Baolu wrote:
>>>
>>> Hi,
>>>
>>> It's hard for me to understand why do we remove the rmrr related
>>> code in this patch.
>> The RMRR code removed here requires the lazy allocation of domains to
>> exist, as it is run before iommu.c would assign IOMMU groups and attach a
>> domain. Before patch 3, removing this code would mean the RMRR regions are
>> never mapped for a domain: iommu.c will allocate a default domain for the
>> group that a device is about to be put in, it will attach the domain to
>> the device, then for each region returned by get_resv_regions it will
>> create an identity map, this is where the RMRRs are setup for the default
>> domain. >
>>>
>>> And, now we have two places to hold a domain for a device: group and
>>> dev->info. We can consider to optimize the use of per device iommu
>>> arch data. This should be later work anyway.
>>>
>>> More comments inline.
>>>
>>> On 3/4/19 11:47 PM, James Sewart wrote:
The generic IOMMU code will allocate and attach a dma ops domain to each
device that comes online, replacing any lazy allocated domain. Removes
RMRR application at iommu init time as we won't have a domain attached
to any device. iommu.c will do this after attaching a device using
create_direct_mappings.
Signed-off-by: James Sewart
---
drivers/iommu/intel-iommu.c | 202 ++--
1 file changed, 8 insertions(+), 194 deletions(-)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 71cd6bbfec05..282257e2628d 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2595,118 +2595,6 @@ static struct dmar_domain
*dmar_insert_one_dev_info(struct intel_iommu *iommu,
return domain;
}
-static int get_last_alias(struct pci_dev *pdev, u16 alias, void *opaque)
-{
- *(u16 *)opaque = alias;
- return 0;
-}
-
-static struct dmar_domain *find_or_alloc_domain(struct device *dev, int
gaw)
-{
- struct device_domain_info *info = NULL;
- struct dmar_domain *domain = NULL;
- struct intel_iommu *iommu;
- u16 dma_alias;
- unsigned long flags;
- u8 bus, devfn;
-
- iommu = device_to_iommu(dev, &bus, &devfn);
- if (!iommu)
- return NULL;
-
- if (dev_is_pci(dev)) {
- struct pci_dev *pdev = to_pci_dev(dev);
-
- pci_for_each_dma_alias(pdev, get_last_alias, &dma_alias);
-
- spin_lock_irqsave(&device_domain_lock, flags);
- info = dmar_search_domain_by_dev_info(pci_domain_nr(pdev->bus),
-PCI_BUS_NUM(dma_alias),
-dma_alias & 0xff);
- if (info) {
- iommu = info->iommu;
- domain = info->domain;
- }
- spin_unlock_irqrestore(&device_domain_lock, flags);
-
- /* DMA alias already has a domain, use it */
- if (info)
- goto out;
- }
-
- /* Allocate and initialize new domain for the device */
- domain = alloc_domain(0);
- if (!domain)
- return NULL;
- if (domain_init(domain, iommu, gaw)) {
- domain_exit(domain);
- return NULL;
- }
-
-out:
-
- return domain;
-}
-
-static struct dmar_domain *set_domain_for_dev(struct device *dev,
-struct dmar_domain *domain)
-{
- struct intel_iommu *iommu;
- struct dmar_domain *tmp;
- u16 req_id, dma_alias;
- u8 bus, devfn;
-
- iommu = device_to_iommu(dev, &bus, &devfn);
- if (!iommu)
- return NULL;
-
- req_id = ((u16)bus << 8) | devfn;
-
- if (dev_is_pci(dev)) {
- struct pci_dev *pdev = to_pci_dev(dev);
-
- pci_for_each_dma_alias(pdev, get_last_alias, &dma_alias);
-
- /* register PCI DMA alias device */
- if (req_id != dma_alias) {
- tmp = dmar_insert_one_dev_info(iommu,
PCI_BUS_NUM(dma_alias),
- dma_alias & 0xff, NULL, domain);
-
- if (!tmp || tmp != domain)
- return tmp;
- }
- }
-
- tmp = dmar_insert_one_dev_info(iommu, bus, devfn, dev, domain);
- if (!tmp || tmp != domain)
- return tmp;
-
- return domain;
-}
-
-static struct dmar_domain *get_domain_for_dev(struct device *dev, int gaw)
-{
- struct dmar_domain *domain, *tmp;
-
-
Re: [PATCH 4/4] iommu/vt-d: Remove lazy allocation of domains
Hi,
On 3/5/19 7:46 PM, James Sewart wrote:
Hey Lu,
On 5 Mar 2019, at 06:59, Lu Baolu wrote:
Hi,
It's hard for me to understand why do we remove the rmrr related
code in this patch.
The RMRR code removed here requires the lazy allocation of domains to
exist, as it is run before iommu.c would assign IOMMU groups and attach a
domain. Before patch 3, removing this code would mean the RMRR regions are
never mapped for a domain: iommu.c will allocate a default domain for the
group that a device is about to be put in, it will attach the domain to
the device, then for each region returned by get_resv_regions it will
create an identity map, this is where the RMRRs are setup for the default
domain. >
And, now we have two places to hold a domain for a device: group and
dev->info. We can consider to optimize the use of per device iommu
arch data. This should be later work anyway.
More comments inline.
On 3/4/19 11:47 PM, James Sewart wrote:
The generic IOMMU code will allocate and attach a dma ops domain to each
device that comes online, replacing any lazy allocated domain. Removes
RMRR application at iommu init time as we won't have a domain attached
to any device. iommu.c will do this after attaching a device using
create_direct_mappings.
Signed-off-by: James Sewart
---
drivers/iommu/intel-iommu.c | 202 ++--
1 file changed, 8 insertions(+), 194 deletions(-)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 71cd6bbfec05..282257e2628d 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2595,118 +2595,6 @@ static struct dmar_domain
*dmar_insert_one_dev_info(struct intel_iommu *iommu,
return domain;
}
-static int get_last_alias(struct pci_dev *pdev, u16 alias, void *opaque)
-{
- *(u16 *)opaque = alias;
- return 0;
-}
-
-static struct dmar_domain *find_or_alloc_domain(struct device *dev, int gaw)
-{
- struct device_domain_info *info = NULL;
- struct dmar_domain *domain = NULL;
- struct intel_iommu *iommu;
- u16 dma_alias;
- unsigned long flags;
- u8 bus, devfn;
-
- iommu = device_to_iommu(dev, &bus, &devfn);
- if (!iommu)
- return NULL;
-
- if (dev_is_pci(dev)) {
- struct pci_dev *pdev = to_pci_dev(dev);
-
- pci_for_each_dma_alias(pdev, get_last_alias, &dma_alias);
-
- spin_lock_irqsave(&device_domain_lock, flags);
- info = dmar_search_domain_by_dev_info(pci_domain_nr(pdev->bus),
- PCI_BUS_NUM(dma_alias),
- dma_alias & 0xff);
- if (info) {
- iommu = info->iommu;
- domain = info->domain;
- }
- spin_unlock_irqrestore(&device_domain_lock, flags);
-
- /* DMA alias already has a domain, use it */
- if (info)
- goto out;
- }
-
- /* Allocate and initialize new domain for the device */
- domain = alloc_domain(0);
- if (!domain)
- return NULL;
- if (domain_init(domain, iommu, gaw)) {
- domain_exit(domain);
- return NULL;
- }
-
-out:
-
- return domain;
-}
-
-static struct dmar_domain *set_domain_for_dev(struct device *dev,
- struct dmar_domain *domain)
-{
- struct intel_iommu *iommu;
- struct dmar_domain *tmp;
- u16 req_id, dma_alias;
- u8 bus, devfn;
-
- iommu = device_to_iommu(dev, &bus, &devfn);
- if (!iommu)
- return NULL;
-
- req_id = ((u16)bus << 8) | devfn;
-
- if (dev_is_pci(dev)) {
- struct pci_dev *pdev = to_pci_dev(dev);
-
- pci_for_each_dma_alias(pdev, get_last_alias, &dma_alias);
-
- /* register PCI DMA alias device */
- if (req_id != dma_alias) {
- tmp = dmar_insert_one_dev_info(iommu,
PCI_BUS_NUM(dma_alias),
- dma_alias & 0xff, NULL, domain);
-
- if (!tmp || tmp != domain)
- return tmp;
- }
- }
-
- tmp = dmar_insert_one_dev_info(iommu, bus, devfn, dev, domain);
- if (!tmp || tmp != domain)
- return tmp;
-
- return domain;
-}
-
-static struct dmar_domain *get_domain_for_dev(struct device *dev, int gaw)
-{
- struct dmar_domain *domain, *tmp;
-
- domain = find_domain(dev);
- if (domain)
- goto out;
-
- domain = find_or_alloc_domain(dev, gaw);
- if (!domain)
- goto out;
-
- tmp = set_domain_for_dev(dev, domain);
- if (!tmp || domain != tmp) {
- domain_exit(domain);
- domain = tmp;
- }
-
-out:
-
- return domain;
-}
-
stati
Re: [PATCH 4/4] iommu/vt-d: Remove lazy allocation of domains
Hey Lu,
> On 5 Mar 2019, at 06:59, Lu Baolu wrote:
>
> Hi,
>
> It's hard for me to understand why do we remove the rmrr related
> code in this patch.
The RMRR code removed here requires the lazy allocation of domains to
exist, as it is run before iommu.c would assign IOMMU groups and attach a
domain. Before patch 3, removing this code would mean the RMRR regions are
never mapped for a domain: iommu.c will allocate a default domain for the
group that a device is about to be put in, it will attach the domain to
the device, then for each region returned by get_resv_regions it will
create an identity map, this is where the RMRRs are setup for the default
domain.
>
> And, now we have two places to hold a domain for a device: group and
> dev->info. We can consider to optimize the use of per device iommu
> arch data. This should be later work anyway.
>
> More comments inline.
>
> On 3/4/19 11:47 PM, James Sewart wrote:
>> The generic IOMMU code will allocate and attach a dma ops domain to each
>> device that comes online, replacing any lazy allocated domain. Removes
>> RMRR application at iommu init time as we won't have a domain attached
>> to any device. iommu.c will do this after attaching a device using
>> create_direct_mappings.
>> Signed-off-by: James Sewart
>> ---
>> drivers/iommu/intel-iommu.c | 202 ++--
>> 1 file changed, 8 insertions(+), 194 deletions(-)
>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>> index 71cd6bbfec05..282257e2628d 100644
>> --- a/drivers/iommu/intel-iommu.c
>> +++ b/drivers/iommu/intel-iommu.c
>> @@ -2595,118 +2595,6 @@ static struct dmar_domain
>> *dmar_insert_one_dev_info(struct intel_iommu *iommu,
>> return domain;
>> }
>> -static int get_last_alias(struct pci_dev *pdev, u16 alias, void *opaque)
>> -{
>> -*(u16 *)opaque = alias;
>> -return 0;
>> -}
>> -
>> -static struct dmar_domain *find_or_alloc_domain(struct device *dev, int gaw)
>> -{
>> -struct device_domain_info *info = NULL;
>> -struct dmar_domain *domain = NULL;
>> -struct intel_iommu *iommu;
>> -u16 dma_alias;
>> -unsigned long flags;
>> -u8 bus, devfn;
>> -
>> -iommu = device_to_iommu(dev, &bus, &devfn);
>> -if (!iommu)
>> -return NULL;
>> -
>> -if (dev_is_pci(dev)) {
>> -struct pci_dev *pdev = to_pci_dev(dev);
>> -
>> -pci_for_each_dma_alias(pdev, get_last_alias, &dma_alias);
>> -
>> -spin_lock_irqsave(&device_domain_lock, flags);
>> -info = dmar_search_domain_by_dev_info(pci_domain_nr(pdev->bus),
>> - PCI_BUS_NUM(dma_alias),
>> - dma_alias & 0xff);
>> -if (info) {
>> -iommu = info->iommu;
>> -domain = info->domain;
>> -}
>> -spin_unlock_irqrestore(&device_domain_lock, flags);
>> -
>> -/* DMA alias already has a domain, use it */
>> -if (info)
>> -goto out;
>> -}
>> -
>> -/* Allocate and initialize new domain for the device */
>> -domain = alloc_domain(0);
>> -if (!domain)
>> -return NULL;
>> -if (domain_init(domain, iommu, gaw)) {
>> -domain_exit(domain);
>> -return NULL;
>> -}
>> -
>> -out:
>> -
>> -return domain;
>> -}
>> -
>> -static struct dmar_domain *set_domain_for_dev(struct device *dev,
>> - struct dmar_domain *domain)
>> -{
>> -struct intel_iommu *iommu;
>> -struct dmar_domain *tmp;
>> -u16 req_id, dma_alias;
>> -u8 bus, devfn;
>> -
>> -iommu = device_to_iommu(dev, &bus, &devfn);
>> -if (!iommu)
>> -return NULL;
>> -
>> -req_id = ((u16)bus << 8) | devfn;
>> -
>> -if (dev_is_pci(dev)) {
>> -struct pci_dev *pdev = to_pci_dev(dev);
>> -
>> -pci_for_each_dma_alias(pdev, get_last_alias, &dma_alias);
>> -
>> -/* register PCI DMA alias device */
>> -if (req_id != dma_alias) {
>> -tmp = dmar_insert_one_dev_info(iommu,
>> PCI_BUS_NUM(dma_alias),
>> -dma_alias & 0xff, NULL, domain);
>> -
>> -if (!tmp || tmp != domain)
>> -return tmp;
>> -}
>> -}
>> -
>> -tmp = dmar_insert_one_dev_info(iommu, bus, devfn, dev, domain);
>> -if (!tmp || tmp != domain)
>> -return tmp;
>> -
>> -return domain;
>> -}
>> -
>> -static struct dmar_domain *get_domain_for_dev(struct device *dev, int gaw)
>> -{
>> -struct dmar_domain *domain, *tmp;
>> -
>> -domain = find_domain(dev);
>> -if (domain)
>> -goto out;
>> -
>> -domain = find_or_alloc_domain(dev, gaw);
>> -if (!domain)
>> -goto out;
>> -
>> -tmp = set_domain_for_dev(dev, domain);
>> -if (!tmp || domain
Re: [PATCH 4/4] iommu/vt-d: Remove lazy allocation of domains
Hi,
It's hard for me to understand why do we remove the rmrr related
code in this patch.
And, now we have two places to hold a domain for a device: group and
dev->info. We can consider to optimize the use of per device iommu
arch data. This should be later work anyway.
More comments inline.
On 3/4/19 11:47 PM, James Sewart wrote:
The generic IOMMU code will allocate and attach a dma ops domain to each
device that comes online, replacing any lazy allocated domain. Removes
RMRR application at iommu init time as we won't have a domain attached
to any device. iommu.c will do this after attaching a device using
create_direct_mappings.
Signed-off-by: James Sewart
---
drivers/iommu/intel-iommu.c | 202 ++--
1 file changed, 8 insertions(+), 194 deletions(-)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 71cd6bbfec05..282257e2628d 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2595,118 +2595,6 @@ static struct dmar_domain
*dmar_insert_one_dev_info(struct intel_iommu *iommu,
return domain;
}
-static int get_last_alias(struct pci_dev *pdev, u16 alias, void *opaque)
-{
- *(u16 *)opaque = alias;
- return 0;
-}
-
-static struct dmar_domain *find_or_alloc_domain(struct device *dev, int gaw)
-{
- struct device_domain_info *info = NULL;
- struct dmar_domain *domain = NULL;
- struct intel_iommu *iommu;
- u16 dma_alias;
- unsigned long flags;
- u8 bus, devfn;
-
- iommu = device_to_iommu(dev, &bus, &devfn);
- if (!iommu)
- return NULL;
-
- if (dev_is_pci(dev)) {
- struct pci_dev *pdev = to_pci_dev(dev);
-
- pci_for_each_dma_alias(pdev, get_last_alias, &dma_alias);
-
- spin_lock_irqsave(&device_domain_lock, flags);
- info = dmar_search_domain_by_dev_info(pci_domain_nr(pdev->bus),
- PCI_BUS_NUM(dma_alias),
- dma_alias & 0xff);
- if (info) {
- iommu = info->iommu;
- domain = info->domain;
- }
- spin_unlock_irqrestore(&device_domain_lock, flags);
-
- /* DMA alias already has a domain, use it */
- if (info)
- goto out;
- }
-
- /* Allocate and initialize new domain for the device */
- domain = alloc_domain(0);
- if (!domain)
- return NULL;
- if (domain_init(domain, iommu, gaw)) {
- domain_exit(domain);
- return NULL;
- }
-
-out:
-
- return domain;
-}
-
-static struct dmar_domain *set_domain_for_dev(struct device *dev,
- struct dmar_domain *domain)
-{
- struct intel_iommu *iommu;
- struct dmar_domain *tmp;
- u16 req_id, dma_alias;
- u8 bus, devfn;
-
- iommu = device_to_iommu(dev, &bus, &devfn);
- if (!iommu)
- return NULL;
-
- req_id = ((u16)bus << 8) | devfn;
-
- if (dev_is_pci(dev)) {
- struct pci_dev *pdev = to_pci_dev(dev);
-
- pci_for_each_dma_alias(pdev, get_last_alias, &dma_alias);
-
- /* register PCI DMA alias device */
- if (req_id != dma_alias) {
- tmp = dmar_insert_one_dev_info(iommu,
PCI_BUS_NUM(dma_alias),
- dma_alias & 0xff, NULL, domain);
-
- if (!tmp || tmp != domain)
- return tmp;
- }
- }
-
- tmp = dmar_insert_one_dev_info(iommu, bus, devfn, dev, domain);
- if (!tmp || tmp != domain)
- return tmp;
-
- return domain;
-}
-
-static struct dmar_domain *get_domain_for_dev(struct device *dev, int gaw)
-{
- struct dmar_domain *domain, *tmp;
-
- domain = find_domain(dev);
- if (domain)
- goto out;
-
- domain = find_or_alloc_domain(dev, gaw);
- if (!domain)
- goto out;
-
- tmp = set_domain_for_dev(dev, domain);
- if (!tmp || domain != tmp) {
- domain_exit(domain);
- domain = tmp;
- }
-
-out:
-
- return domain;
-}
-
static int iommu_domain_identity_map(struct dmar_domain *domain,
unsigned long long start,
unsigned long long end)
@@ -2779,7 +2667,7 @@ static int iommu_prepare_identity_map(struct device *dev,
struct dmar_domain *domain;
int ret;
- domain = get_domain_for_dev(dev, DEFAULT_DOMAIN_ADDRESS_WIDTH);
+ domain = find_domain(dev);
if (!domain)
return -ENOMEM;
@@ -3301,11 +3189,9 @@ static int copy_translation_tables(struct intel_iommu *iommu)
static int __init init_dmars(void)
{
str
[PATCH 4/4] iommu/vt-d: Remove lazy allocation of domains
The generic IOMMU code will allocate and attach a dma ops domain to each
device that comes online, replacing any lazy allocated domain. Removes
RMRR application at iommu init time as we won't have a domain attached
to any device. iommu.c will do this after attaching a device using
create_direct_mappings.
Signed-off-by: James Sewart
---
drivers/iommu/intel-iommu.c | 202 ++--
1 file changed, 8 insertions(+), 194 deletions(-)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 71cd6bbfec05..282257e2628d 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2595,118 +2595,6 @@ static struct dmar_domain
*dmar_insert_one_dev_info(struct intel_iommu *iommu,
return domain;
}
-static int get_last_alias(struct pci_dev *pdev, u16 alias, void *opaque)
-{
- *(u16 *)opaque = alias;
- return 0;
-}
-
-static struct dmar_domain *find_or_alloc_domain(struct device *dev, int gaw)
-{
- struct device_domain_info *info = NULL;
- struct dmar_domain *domain = NULL;
- struct intel_iommu *iommu;
- u16 dma_alias;
- unsigned long flags;
- u8 bus, devfn;
-
- iommu = device_to_iommu(dev, &bus, &devfn);
- if (!iommu)
- return NULL;
-
- if (dev_is_pci(dev)) {
- struct pci_dev *pdev = to_pci_dev(dev);
-
- pci_for_each_dma_alias(pdev, get_last_alias, &dma_alias);
-
- spin_lock_irqsave(&device_domain_lock, flags);
- info = dmar_search_domain_by_dev_info(pci_domain_nr(pdev->bus),
- PCI_BUS_NUM(dma_alias),
- dma_alias & 0xff);
- if (info) {
- iommu = info->iommu;
- domain = info->domain;
- }
- spin_unlock_irqrestore(&device_domain_lock, flags);
-
- /* DMA alias already has a domain, use it */
- if (info)
- goto out;
- }
-
- /* Allocate and initialize new domain for the device */
- domain = alloc_domain(0);
- if (!domain)
- return NULL;
- if (domain_init(domain, iommu, gaw)) {
- domain_exit(domain);
- return NULL;
- }
-
-out:
-
- return domain;
-}
-
-static struct dmar_domain *set_domain_for_dev(struct device *dev,
- struct dmar_domain *domain)
-{
- struct intel_iommu *iommu;
- struct dmar_domain *tmp;
- u16 req_id, dma_alias;
- u8 bus, devfn;
-
- iommu = device_to_iommu(dev, &bus, &devfn);
- if (!iommu)
- return NULL;
-
- req_id = ((u16)bus << 8) | devfn;
-
- if (dev_is_pci(dev)) {
- struct pci_dev *pdev = to_pci_dev(dev);
-
- pci_for_each_dma_alias(pdev, get_last_alias, &dma_alias);
-
- /* register PCI DMA alias device */
- if (req_id != dma_alias) {
- tmp = dmar_insert_one_dev_info(iommu,
PCI_BUS_NUM(dma_alias),
- dma_alias & 0xff, NULL, domain);
-
- if (!tmp || tmp != domain)
- return tmp;
- }
- }
-
- tmp = dmar_insert_one_dev_info(iommu, bus, devfn, dev, domain);
- if (!tmp || tmp != domain)
- return tmp;
-
- return domain;
-}
-
-static struct dmar_domain *get_domain_for_dev(struct device *dev, int gaw)
-{
- struct dmar_domain *domain, *tmp;
-
- domain = find_domain(dev);
- if (domain)
- goto out;
-
- domain = find_or_alloc_domain(dev, gaw);
- if (!domain)
- goto out;
-
- tmp = set_domain_for_dev(dev, domain);
- if (!tmp || domain != tmp) {
- domain_exit(domain);
- domain = tmp;
- }
-
-out:
-
- return domain;
-}
-
static int iommu_domain_identity_map(struct dmar_domain *domain,
unsigned long long start,
unsigned long long end)
@@ -2779,7 +2667,7 @@ static int iommu_prepare_identity_map(struct device *dev,
struct dmar_domain *domain;
int ret;
- domain = get_domain_for_dev(dev, DEFAULT_DOMAIN_ADDRESS_WIDTH);
+ domain = find_domain(dev);
if (!domain)
return -ENOMEM;
@@ -3301,11 +3189,9 @@ static int copy_translation_tables(struct intel_iommu
*iommu)
static int __init init_dmars(void)
{
struct dmar_drhd_unit *drhd;
- struct dmar_rmrr_unit *rmrr;
bool copied_tables = false;
- struct device *dev;
struct intel_iommu *iommu;
- int i, ret;
+ int ret;
/*
* for each drhd
@@ -3466,32 +3352,6 @@ static int __init init_dmars(void)
goto free_iommu;
