* [RFC v2] iommu/vt-d: Allow iommu_domain_alloc to allocate IOMMU_DOMAIN_DMA
@ 2018-12-05 17:19 James Sewart
2019-01-02 14:22 ` James Sewart
2019-01-07 20:04 ` Jacob Pan
0 siblings, 2 replies; 4+ messages in thread
From: James Sewart @ 2018-12-05 17:19 UTC (permalink / raw)
To: iommu; +Cc: linux-kernel, Jacob Pan, Dmitry Safonov
Hey,
There exists an issue in the logic used to determine domain association
with devices. Currently the driver uses find_or_alloc_domain to either
reuse an existing domain or allocate a new one if one isn’t found. Domains
should be shared between all members of an IOMMU group as this is the
minimum granularity that we can guarantee address space isolation.
The intel IOMMU driver exposes pci_device_group in intel_iommu_ops as the
function to call to determine the group of a device, this is implemented
in the generic IOMMU code and checks: dma aliases, upstream pcie switch
ACS, pci aliases, and pci function aliases. The find_or_alloc_domain code
currently only uses dma aliases to determine if a domain is shared. This
causes a disconnect between IOMMU groups and domains. We have observed
devices under a pcie switch each having their own domain but assigned the
same group.
One solution would be to fix the logic in find_or_alloc_domain to add
checks for the other conditions that a device may share a domain. However,
this duplicates code which the generic IOMMU code implements. Instead this
issue can be fixed by allowing the allocation of default_domain on the
IOMMU group. This is not currently supported as the intel driver does not
allow allocation of domain type IOMMU_DOMAIN_DMA.
Allowing allocation of DMA domains has the effect that the default_domain
is non NULL and is attached to a device when initialising. This delegates
the handling of domains to the generic IOMMU code. Once this is
implemented it is possible to remove the lazy allocation of domains
entirely.
This patch implements DMA and identity domains to be allocated for
external management. As it isn’t known which device will be attached to a
domain, the dma domain is not initialised at alloc time. Instead it is
allocated when attached. As we may lose RMRR mappings when attaching a
device to a new domain, we also ensure these are mapped at attach time.
This will likely conflict with the work done for auxiliary domains by
Baolu but the code to accommodate won’t change much.
I had also started on a patch to remove find_or_alloc_domain and various
functions that call it but had issues with edge cases such as
iommu_prepare_isa that is doing domain operations at IOMMU init time.
Cheers,
James.
---
drivers/iommu/intel-iommu.c | 159 +++++++++++++++++++++++++-----------
1 file changed, 110 insertions(+), 49 deletions(-)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 41a4b8808802..6437cb2e9b22 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -351,6 +351,14 @@ static int hw_pass_through = 1;
/* si_domain contains mulitple devices */
#define DOMAIN_FLAG_STATIC_IDENTITY (1 << 1)
+/* Domain managed externally, don't cleanup if it isn't attached
+ * to any devices. */
+#define DOMAIN_FLAG_NO_CLEANUP (1 << 2)
+
+/* Set after domain initialisation. Used when allocating dma domains to
+ * defer domain initialisation until it is attached to a device */
+#define DOMAIN_FLAG_INITIALISED (1 << 4)
+
#define for_each_domain_iommu(idx, domain) \
for (idx = 0; idx < g_num_of_iommus; idx++) \
if (domain->iommu_refcnt[idx])
@@ -624,6 +632,16 @@ static inline int domain_type_is_vm_or_si(struct dmar_domain *domain)
DOMAIN_FLAG_STATIC_IDENTITY);
}
+static inline int domain_managed_externally(struct dmar_domain *domain)
+{
+ return domain->flags & DOMAIN_FLAG_NO_CLEANUP;
+}
+
+static inline int domain_is_initialised(struct dmar_domain *domain)
+{
+ return domain->flags & DOMAIN_FLAG_INITIALISED;
+}
+
static inline int domain_pfn_supported(struct dmar_domain *domain,
unsigned long pfn)
{
@@ -1717,7 +1735,7 @@ static void disable_dmar_iommu(struct intel_iommu *iommu)
__dmar_remove_one_dev_info(info);
- if (!domain_type_is_vm_or_si(domain)) {
+ if (!domain_managed_externally(domain)) {
/*
* The domain_exit() function can't be called under
* device_domain_lock, as it takes this lock itself.
@@ -1951,6 +1969,7 @@ static int domain_init(struct dmar_domain *domain, struct intel_iommu *iommu,
domain->pgd = (struct dma_pte *)alloc_pgtable_page(domain->nid);
if (!domain->pgd)
return -ENOMEM;
+ domain->flags |= DOMAIN_FLAG_INITIALISED;
__iommu_flush_cache(iommu, domain->pgd, PAGE_SIZE);
return 0;
}
@@ -3234,11 +3253,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
@@ -4529,7 +4522,7 @@ static int device_notifier(struct notifier_block *nb,
return 0;
dmar_remove_one_dev_info(domain, dev);
- if (!domain_type_is_vm_or_si(domain) && list_empty(&domain->devices))
+ if (!domain_managed_externally(domain) && list_empty(&domain->devices))
domain_exit(domain);
return 0;
@@ -4930,6 +4923,7 @@ static int md_domain_init(struct dmar_domain *domain, int guest_width)
domain->pgd = (struct dma_pte *)alloc_pgtable_page(domain->nid);
if (!domain->pgd)
return -ENOMEM;
+ domain->flags |= DOMAIN_FLAG_INITIALISED;
domain_flush_cache(domain, domain->pgd, PAGE_SIZE);
return 0;
}
@@ -4938,28 +4932,65 @@ static struct iommu_domain *intel_iommu_domain_alloc(unsigned type)
{
struct dmar_domain *dmar_domain;
struct iommu_domain *domain;
+ int flags = DOMAIN_FLAG_NO_CLEANUP;
+ int nid;
- if (type != IOMMU_DOMAIN_UNMANAGED)
- return NULL;
-
- dmar_domain = alloc_domain(DOMAIN_FLAG_VIRTUAL_MACHINE);
- if (!dmar_domain) {
- pr_err("Can't allocate dmar_domain\n");
- return NULL;
- }
- if (md_domain_init(dmar_domain, DEFAULT_DOMAIN_ADDRESS_WIDTH)) {
- pr_err("Domain initialization failed\n");
- domain_exit(dmar_domain);
+ switch (type) {
+ case IOMMU_DOMAIN_UNMANAGED:
+ flags |= DOMAIN_FLAG_VIRTUAL_MACHINE | DOMAIN_FLAG_INITIALISED;
+ dmar_domain = alloc_domain(flags);
+ if (!dmar_domain) {
+ pr_err("Can't allocate dmar_domain\n");
+ return NULL;
+ }
+ if (md_domain_init(dmar_domain, DEFAULT_DOMAIN_ADDRESS_WIDTH)) {
+ pr_err("Domain initialization failed\n");
+ domain_exit(dmar_domain);
+ return NULL;
+ }
+ domain_update_iommu_cap(dmar_domain);
+ domain->geometry.aperture_start = 0;
+ domain->geometry.aperture_end = __DOMAIN_MAX_ADDR(dmar_domain->gaw);
+ domain->geometry.force_aperture = true;
+ break;
+ case IOMMU_DOMAIN_DMA:
+ dmar_domain = alloc_domain(flags);
+ if (!dmar_domain) {
+ pr_err("Can't allocate dmar_domain\n");
+ return NULL;
+ }
+ /* init domain in device attach when we know IOMMU capabilities */
+ break;
+ case IOMMU_DOMAIN_IDENTITY:
+ flags |= DOMAIN_FLAG_STATIC_IDENTITY | DOMAIN_FLAG_INITIALISED;
+ dmar_domain = alloc_domain(flags);
+ if (!dmar_domain) {
+ pr_err("Can't allocate dmar_domain\n");
+ return NULL;
+ }
+ if (md_domain_init(dmar_domain, DEFAULT_DOMAIN_ADDRESS_WIDTH)) {
+ pr_err("Domain initialization failed\n");
+ domain_exit(dmar_domain);
+ return NULL;
+ }
+ domain_update_iommu_cap(dmar_domain);
+ for_each_online_node(nid) {
+ unsigned long start_pfn, end_pfn;
+ int i, ret;
+
+ for_each_mem_pfn_range(i, nid, &start_pfn, &end_pfn, NULL) {
+ ret = iommu_domain_identity_map(dmar_domain,
+ PFN_PHYS(start_pfn), PFN_PHYS(end_pfn));
+ if (ret)
+ return NULL;
+ }
+ }
+ break;
+ default:
return NULL;
}
- domain_update_iommu_cap(dmar_domain);
-
- domain = &dmar_domain->domain;
- domain->geometry.aperture_start = 0;
- domain->geometry.aperture_end = __DOMAIN_MAX_ADDR(dmar_domain->gaw);
- domain->geometry.force_aperture = true;
- return domain;
+ return &dmar_domain->domain;
}
static void intel_iommu_domain_free(struct iommu_domain *domain)
@@ -4974,6 +5005,9 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,
struct intel_iommu *iommu;
int addr_width;
u8 bus, devfn;
+ struct dmar_rmrr_unit *rmrr;
+ struct device *i_dev;
+ int i, ret;
if (device_is_rmrr_locked(dev)) {
dev_warn(dev, "Device is ineligible for IOMMU domain attach due to platform RMRR requirement. Contact your platform vendor.\n");
@@ -4990,8 +5024,8 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,
dmar_remove_one_dev_info(old_domain, dev);
rcu_read_unlock();
- if (!domain_type_is_vm_or_si(old_domain) &&
- list_empty(&old_domain->devices))
+ if (list_empty(&old_domain->devices) &&
+ !domain_managed_externally(old_domain))
domain_exit(old_domain);
}
}
@@ -5000,6 +5034,12 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,
if (!iommu)
return -ENODEV;
+ /* Initialise domain with IOMMU capabilities if it isn't already initialised */
+ if (!domain_is_initialised(dmar_domain)) {
+ if (domain_init(dmar_domain, iommu, DEFAULT_DOMAIN_ADDRESS_WIDTH))
+ return -ENOMEM;
+ }
+
/* check if this iommu agaw is sufficient for max mapped address */
addr_width = agaw_to_width(iommu->agaw);
if (addr_width > cap_mgaw(iommu->cap))
@@ -5028,6 +5068,27 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,
dmar_domain->agaw--;
}
+ if (domain_type_is_vm_or_si(dmar_domain))
+ goto out;
+
+ /* Ensure DMA domain has devices RMRR */
+ rcu_read_lock();
+ for_each_rmrr_units(rmrr) {
+ for_each_active_dev_scope(rmrr->devices, rmrr->devices_cnt,
+ i, i_dev) {
+ if (i_dev != dev)
+ continue;
+
+ ret = domain_prepare_identity_map(dev, dmar_domain,
+ rmrr->base_address,
+ rmrr->end_address);
+ if (ret)
+ dev_err(dev, "Mapping reserved region failed\n");
+ }
+ }
+ rcu_read_unlock();
+
+out:
return domain_add_dev_info(dmar_domain, dev);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC v2] iommu/vt-d: Allow iommu_domain_alloc to allocate IOMMU_DOMAIN_DMA
2018-12-05 17:19 [RFC v2] iommu/vt-d: Allow iommu_domain_alloc to allocate IOMMU_DOMAIN_DMA James Sewart
@ 2019-01-02 14:22 ` James Sewart
2019-01-07 20:04 ` Jacob Pan
1 sibling, 0 replies; 4+ messages in thread
From: James Sewart @ 2019-01-02 14:22 UTC (permalink / raw)
To: iommu; +Cc: linux-kernel, Jacob Pan, Dmitry Safonov
Bump
> On 5 Dec 2018, at 17:19, James Sewart <jamessewart@arista.com> wrote:
>
> Hey,
>
> There exists an issue in the logic used to determine domain association
> with devices. Currently the driver uses find_or_alloc_domain to either
> reuse an existing domain or allocate a new one if one isn’t found. Domains
> should be shared between all members of an IOMMU group as this is the
> minimum granularity that we can guarantee address space isolation.
>
> The intel IOMMU driver exposes pci_device_group in intel_iommu_ops as the
> function to call to determine the group of a device, this is implemented
> in the generic IOMMU code and checks: dma aliases, upstream pcie switch
> ACS, pci aliases, and pci function aliases. The find_or_alloc_domain code
> currently only uses dma aliases to determine if a domain is shared. This
> causes a disconnect between IOMMU groups and domains. We have observed
> devices under a pcie switch each having their own domain but assigned the
> same group.
>
> One solution would be to fix the logic in find_or_alloc_domain to add
> checks for the other conditions that a device may share a domain. However,
> this duplicates code which the generic IOMMU code implements. Instead this
> issue can be fixed by allowing the allocation of default_domain on the
> IOMMU group. This is not currently supported as the intel driver does not
> allow allocation of domain type IOMMU_DOMAIN_DMA.
>
> Allowing allocation of DMA domains has the effect that the default_domain
> is non NULL and is attached to a device when initialising. This delegates
> the handling of domains to the generic IOMMU code. Once this is
> implemented it is possible to remove the lazy allocation of domains
> entirely.
>
> This patch implements DMA and identity domains to be allocated for
> external management. As it isn’t known which device will be attached to a
> domain, the dma domain is not initialised at alloc time. Instead it is
> allocated when attached. As we may lose RMRR mappings when attaching a
> device to a new domain, we also ensure these are mapped at attach time.
>
> This will likely conflict with the work done for auxiliary domains by
> Baolu but the code to accommodate won’t change much.
>
> I had also started on a patch to remove find_or_alloc_domain and various
> functions that call it but had issues with edge cases such as
> iommu_prepare_isa that is doing domain operations at IOMMU init time.
>
> Cheers,
> James.
>
>
> ---
> drivers/iommu/intel-iommu.c | 159 +++++++++++++++++++++++++-----------
> 1 file changed, 110 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 41a4b8808802..6437cb2e9b22 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -351,6 +351,14 @@ static int hw_pass_through = 1;
> /* si_domain contains mulitple devices */
> #define DOMAIN_FLAG_STATIC_IDENTITY (1 << 1)
>
> +/* Domain managed externally, don't cleanup if it isn't attached
> + * to any devices. */
> +#define DOMAIN_FLAG_NO_CLEANUP (1 << 2)
> +
> +/* Set after domain initialisation. Used when allocating dma domains to
> + * defer domain initialisation until it is attached to a device */
> +#define DOMAIN_FLAG_INITIALISED (1 << 4)
> +
> #define for_each_domain_iommu(idx, domain) \
> for (idx = 0; idx < g_num_of_iommus; idx++) \
> if (domain->iommu_refcnt[idx])
> @@ -624,6 +632,16 @@ static inline int domain_type_is_vm_or_si(struct dmar_domain *domain)
> DOMAIN_FLAG_STATIC_IDENTITY);
> }
>
> +static inline int domain_managed_externally(struct dmar_domain *domain)
> +{
> + return domain->flags & DOMAIN_FLAG_NO_CLEANUP;
> +}
> +
> +static inline int domain_is_initialised(struct dmar_domain *domain)
> +{
> + return domain->flags & DOMAIN_FLAG_INITIALISED;
> +}
> +
> static inline int domain_pfn_supported(struct dmar_domain *domain,
> unsigned long pfn)
> {
> @@ -1717,7 +1735,7 @@ static void disable_dmar_iommu(struct intel_iommu *iommu)
>
> __dmar_remove_one_dev_info(info);
>
> - if (!domain_type_is_vm_or_si(domain)) {
> + if (!domain_managed_externally(domain)) {
> /*
> * The domain_exit() function can't be called under
> * device_domain_lock, as it takes this lock itself.
> @@ -1951,6 +1969,7 @@ static int domain_init(struct dmar_domain *domain, struct intel_iommu *iommu,
> domain->pgd = (struct dma_pte *)alloc_pgtable_page(domain->nid);
> if (!domain->pgd)
> return -ENOMEM;
> + domain->flags |= DOMAIN_FLAG_INITIALISED;
> __iommu_flush_cache(iommu, domain->pgd, PAGE_SIZE);
> return 0;
> }
> @@ -3234,11 +3253,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
> @@ -4529,7 +4522,7 @@ static int device_notifier(struct notifier_block *nb,
> return 0;
>
> dmar_remove_one_dev_info(domain, dev);
> - if (!domain_type_is_vm_or_si(domain) && list_empty(&domain->devices))
> + if (!domain_managed_externally(domain) && list_empty(&domain->devices))
> domain_exit(domain);
>
> return 0;
> @@ -4930,6 +4923,7 @@ static int md_domain_init(struct dmar_domain *domain, int guest_width)
> domain->pgd = (struct dma_pte *)alloc_pgtable_page(domain->nid);
> if (!domain->pgd)
> return -ENOMEM;
> + domain->flags |= DOMAIN_FLAG_INITIALISED;
> domain_flush_cache(domain, domain->pgd, PAGE_SIZE);
> return 0;
> }
> @@ -4938,28 +4932,65 @@ static struct iommu_domain *intel_iommu_domain_alloc(unsigned type)
> {
> struct dmar_domain *dmar_domain;
> struct iommu_domain *domain;
> + int flags = DOMAIN_FLAG_NO_CLEANUP;
> + int nid;
>
> - if (type != IOMMU_DOMAIN_UNMANAGED)
> - return NULL;
> -
> - dmar_domain = alloc_domain(DOMAIN_FLAG_VIRTUAL_MACHINE);
> - if (!dmar_domain) {
> - pr_err("Can't allocate dmar_domain\n");
> - return NULL;
> - }
> - if (md_domain_init(dmar_domain, DEFAULT_DOMAIN_ADDRESS_WIDTH)) {
> - pr_err("Domain initialization failed\n");
> - domain_exit(dmar_domain);
> + switch (type) {
> + case IOMMU_DOMAIN_UNMANAGED:
> + flags |= DOMAIN_FLAG_VIRTUAL_MACHINE | DOMAIN_FLAG_INITIALISED;
> + dmar_domain = alloc_domain(flags);
> + if (!dmar_domain) {
> + pr_err("Can't allocate dmar_domain\n");
> + return NULL;
> + }
> + if (md_domain_init(dmar_domain, DEFAULT_DOMAIN_ADDRESS_WIDTH)) {
> + pr_err("Domain initialization failed\n");
> + domain_exit(dmar_domain);
> + return NULL;
> + }
> + domain_update_iommu_cap(dmar_domain);
> + domain->geometry.aperture_start = 0;
> + domain->geometry.aperture_end = __DOMAIN_MAX_ADDR(dmar_domain->gaw);
> + domain->geometry.force_aperture = true;
> + break;
> + case IOMMU_DOMAIN_DMA:
> + dmar_domain = alloc_domain(flags);
> + if (!dmar_domain) {
> + pr_err("Can't allocate dmar_domain\n");
> + return NULL;
> + }
> + /* init domain in device attach when we know IOMMU capabilities */
> + break;
> + case IOMMU_DOMAIN_IDENTITY:
> + flags |= DOMAIN_FLAG_STATIC_IDENTITY | DOMAIN_FLAG_INITIALISED;
> + dmar_domain = alloc_domain(flags);
> + if (!dmar_domain) {
> + pr_err("Can't allocate dmar_domain\n");
> + return NULL;
> + }
> + if (md_domain_init(dmar_domain, DEFAULT_DOMAIN_ADDRESS_WIDTH)) {
> + pr_err("Domain initialization failed\n");
> + domain_exit(dmar_domain);
> + return NULL;
> + }
> + domain_update_iommu_cap(dmar_domain);
> + for_each_online_node(nid) {
> + unsigned long start_pfn, end_pfn;
> + int i, ret;
> +
> + for_each_mem_pfn_range(i, nid, &start_pfn, &end_pfn, NULL) {
> + ret = iommu_domain_identity_map(dmar_domain,
> + PFN_PHYS(start_pfn), PFN_PHYS(end_pfn));
> + if (ret)
> + return NULL;
> + }
> + }
> + break;
> + default:
> return NULL;
> }
> - domain_update_iommu_cap(dmar_domain);
> -
> - domain = &dmar_domain->domain;
> - domain->geometry.aperture_start = 0;
> - domain->geometry.aperture_end = __DOMAIN_MAX_ADDR(dmar_domain->gaw);
> - domain->geometry.force_aperture = true;
>
> - return domain;
> + return &dmar_domain->domain;
> }
>
> static void intel_iommu_domain_free(struct iommu_domain *domain)
> @@ -4974,6 +5005,9 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,
> struct intel_iommu *iommu;
> int addr_width;
> u8 bus, devfn;
> + struct dmar_rmrr_unit *rmrr;
> + struct device *i_dev;
> + int i, ret;
>
> if (device_is_rmrr_locked(dev)) {
> dev_warn(dev, "Device is ineligible for IOMMU domain attach due to platform RMRR requirement. Contact your platform vendor.\n");
> @@ -4990,8 +5024,8 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,
> dmar_remove_one_dev_info(old_domain, dev);
> rcu_read_unlock();
>
> - if (!domain_type_is_vm_or_si(old_domain) &&
> - list_empty(&old_domain->devices))
> + if (list_empty(&old_domain->devices) &&
> + !domain_managed_externally(old_domain))
> domain_exit(old_domain);
> }
> }
> @@ -5000,6 +5034,12 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,
> if (!iommu)
> return -ENODEV;
>
> + /* Initialise domain with IOMMU capabilities if it isn't already initialised */
> + if (!domain_is_initialised(dmar_domain)) {
> + if (domain_init(dmar_domain, iommu, DEFAULT_DOMAIN_ADDRESS_WIDTH))
> + return -ENOMEM;
> + }
> +
> /* check if this iommu agaw is sufficient for max mapped address */
> addr_width = agaw_to_width(iommu->agaw);
> if (addr_width > cap_mgaw(iommu->cap))
> @@ -5028,6 +5068,27 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,
> dmar_domain->agaw--;
> }
>
> + if (domain_type_is_vm_or_si(dmar_domain))
> + goto out;
> +
> + /* Ensure DMA domain has devices RMRR */
> + rcu_read_lock();
> + for_each_rmrr_units(rmrr) {
> + for_each_active_dev_scope(rmrr->devices, rmrr->devices_cnt,
> + i, i_dev) {
> + if (i_dev != dev)
> + continue;
> +
> + ret = domain_prepare_identity_map(dev, dmar_domain,
> + rmrr->base_address,
> + rmrr->end_address);
> + if (ret)
> + dev_err(dev, "Mapping reserved region failed\n");
> + }
> + }
> + rcu_read_unlock();
> +
> +out:
> return domain_add_dev_info(dmar_domain, dev);
> }
>
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC v2] iommu/vt-d: Allow iommu_domain_alloc to allocate IOMMU_DOMAIN_DMA
2018-12-05 17:19 [RFC v2] iommu/vt-d: Allow iommu_domain_alloc to allocate IOMMU_DOMAIN_DMA James Sewart
2019-01-02 14:22 ` James Sewart
@ 2019-01-07 20:04 ` Jacob Pan
2019-01-15 14:26 ` James Sewart
1 sibling, 1 reply; 4+ messages in thread
From: Jacob Pan @ 2019-01-07 20:04 UTC (permalink / raw)
To: James Sewart; +Cc: iommu, linux-kernel, Dmitry Safonov, jacob.jun.pan
On Wed, 5 Dec 2018 17:19:35 +0000
James Sewart <jamessewart@arista.com> wrote:
> Hey,
>
> There exists an issue in the logic used to determine domain
> association with devices. Currently the driver uses
> find_or_alloc_domain to either reuse an existing domain or allocate a
> new one if one isn’t found. Domains should be shared between all
> members of an IOMMU group as this is the minimum granularity that we
> can guarantee address space isolation.
>
> The intel IOMMU driver exposes pci_device_group in intel_iommu_ops as
> the function to call to determine the group of a device, this is
> implemented in the generic IOMMU code and checks: dma aliases,
> upstream pcie switch ACS, pci aliases, and pci function aliases. The
> find_or_alloc_domain code currently only uses dma aliases to
> determine if a domain is shared. This causes a disconnect between
> IOMMU groups and domains. We have observed devices under a pcie
> switch each having their own domain but assigned the same group.
>
> One solution would be to fix the logic in find_or_alloc_domain to add
> checks for the other conditions that a device may share a domain.
> However, this duplicates code which the generic IOMMU code
> implements. Instead this issue can be fixed by allowing the
> allocation of default_domain on the IOMMU group. This is not
> currently supported as the intel driver does not allow allocation of
> domain type IOMMU_DOMAIN_DMA.
>
> Allowing allocation of DMA domains has the effect that the
> default_domain is non NULL and is attached to a device when
> initialising. This delegates the handling of domains to the generic
> IOMMU code. Once this is implemented it is possible to remove the
> lazy allocation of domains entirely.
>
This can also consolidate the domain storage, i.e. move domain from
device_domain_info to iommu_group.
> This patch implements DMA and identity domains to be allocated for
> external management. As it isn’t known which device will be attached
> to a domain, the dma domain is not initialised at alloc time. Instead
> it is allocated when attached. As we may lose RMRR mappings when
> attaching a device to a new domain, we also ensure these are mapped
> at attach time.
>
> This will likely conflict with the work done for auxiliary domains by
> Baolu but the code to accommodate won’t change much.
>
> I had also started on a patch to remove find_or_alloc_domain and
> various functions that call it but had issues with edge cases such as
> iommu_prepare_isa that is doing domain operations at IOMMU init time.
>
> Cheers,
> James.
>
>
> ---
> drivers/iommu/intel-iommu.c | 159
> +++++++++++++++++++++++++----------- 1 file changed, 110
> insertions(+), 49 deletions(-)
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 41a4b8808802..6437cb2e9b22 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -351,6 +351,14 @@ static int hw_pass_through = 1;
> /* si_domain contains mulitple devices */
> #define DOMAIN_FLAG_STATIC_IDENTITY (1 << 1)
>
> +/* Domain managed externally, don't cleanup if it isn't attached
> + * to any devices. */
> +#define DOMAIN_FLAG_NO_CLEANUP (1 << 2)
> +
the name NO_CLEANUP is a little counter intuitive to me, should it be
called UNINITIALISED?
> +/* Set after domain initialisation. Used when allocating dma domains
> to
> + * defer domain initialisation until it is attached to a device */
> +#define DOMAIN_FLAG_INITIALISED (1 << 4)
> +
> #define for_each_domain_iommu(idx, domain) \
> for (idx = 0; idx < g_num_of_iommus; idx++) \
> if (domain->iommu_refcnt[idx])
> @@ -624,6 +632,16 @@ static inline int domain_type_is_vm_or_si(struct
> dmar_domain *domain) DOMAIN_FLAG_STATIC_IDENTITY);
> }
>
> +static inline int domain_managed_externally(struct dmar_domain
> *domain) +{
> + return domain->flags & DOMAIN_FLAG_NO_CLEANUP;
> +}
> +
> +static inline int domain_is_initialised(struct dmar_domain *domain)
> +{
> + return domain->flags & DOMAIN_FLAG_INITIALISED;
> +}
> +
> static inline int domain_pfn_supported(struct dmar_domain *domain,
> unsigned long pfn)
> {
> @@ -1717,7 +1735,7 @@ static void disable_dmar_iommu(struct
> intel_iommu *iommu)
> __dmar_remove_one_dev_info(info);
>
> - if (!domain_type_is_vm_or_si(domain)) {
> + if (!domain_managed_externally(domain)) {
> /*
> * The domain_exit() function can't be
> called under
> * device_domain_lock, as it takes this lock
> itself. @@ -1951,6 +1969,7 @@ static int domain_init(struct
> dmar_domain *domain, struct intel_iommu *iommu, domain->pgd = (struct
> dma_pte *)alloc_pgtable_page(domain->nid); if (!domain->pgd)
> return -ENOMEM;
> + domain->flags |= DOMAIN_FLAG_INITIALISED;
> __iommu_flush_cache(iommu, domain->pgd, PAGE_SIZE);
> return 0;
> }
> @@ -3234,11 +3253,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
> @@ -4529,7 +4522,7 @@ static int device_notifier(struct
> notifier_block *nb, return 0;
>
> dmar_remove_one_dev_info(domain, dev);
> - if (!domain_type_is_vm_or_si(domain) &&
> list_empty(&domain->devices))
> + if (!domain_managed_externally(domain) &&
> list_empty(&domain->devices)) domain_exit(domain);
>
> return 0;
> @@ -4930,6 +4923,7 @@ static int md_domain_init(struct dmar_domain
> *domain, int guest_width) domain->pgd = (struct dma_pte
> *)alloc_pgtable_page(domain->nid); if (!domain->pgd)
> return -ENOMEM;
> + domain->flags |= DOMAIN_FLAG_INITIALISED;
> domain_flush_cache(domain, domain->pgd, PAGE_SIZE);
> return 0;
> }
> @@ -4938,28 +4932,65 @@ static struct iommu_domain
> *intel_iommu_domain_alloc(unsigned type) {
> struct dmar_domain *dmar_domain;
> struct iommu_domain *domain;
> + int flags = DOMAIN_FLAG_NO_CLEANUP;
> + int nid;
>
> - if (type != IOMMU_DOMAIN_UNMANAGED)
> - return NULL;
> -
> - dmar_domain = alloc_domain(DOMAIN_FLAG_VIRTUAL_MACHINE);
> - if (!dmar_domain) {
> - pr_err("Can't allocate dmar_domain\n");
> - return NULL;
> - }
> - if (md_domain_init(dmar_domain,
> DEFAULT_DOMAIN_ADDRESS_WIDTH)) {
> - pr_err("Domain initialization failed\n");
> - domain_exit(dmar_domain);
> + switch (type) {
> + case IOMMU_DOMAIN_UNMANAGED:
> + flags |= DOMAIN_FLAG_VIRTUAL_MACHINE |
> DOMAIN_FLAG_INITIALISED;
> + dmar_domain = alloc_domain(flags);
> + if (!dmar_domain) {
> + pr_err("Can't allocate dmar_domain\n");
> + return NULL;
> + }
> + if (md_domain_init(dmar_domain,
> DEFAULT_DOMAIN_ADDRESS_WIDTH)) {
> + pr_err("Domain initialization failed\n");
> + domain_exit(dmar_domain);
> + return NULL;
> + }
> + domain_update_iommu_cap(dmar_domain);
> + domain->geometry.aperture_start = 0;
> + domain->geometry.aperture_end =
> __DOMAIN_MAX_ADDR(dmar_domain->gaw);
> + domain->geometry.force_aperture = true;
> + break;
> + case IOMMU_DOMAIN_DMA:
> + dmar_domain = alloc_domain(flags);
> + if (!dmar_domain) {
> + pr_err("Can't allocate dmar_domain\n");
> + return NULL;
> + }
> + /* init domain in device attach when we know IOMMU
> capabilities */
> + break;
> + case IOMMU_DOMAIN_IDENTITY:
> + flags |= DOMAIN_FLAG_STATIC_IDENTITY |
> DOMAIN_FLAG_INITIALISED;
> + dmar_domain = alloc_domain(flags);
> + if (!dmar_domain) {
> + pr_err("Can't allocate dmar_domain\n");
> + return NULL;
> + }
> + if (md_domain_init(dmar_domain,
> DEFAULT_DOMAIN_ADDRESS_WIDTH)) {
> + pr_err("Domain initialization failed\n");
> + domain_exit(dmar_domain);
> + return NULL;
> + }
> + domain_update_iommu_cap(dmar_domain);
> + for_each_online_node(nid) {
> + unsigned long start_pfn, end_pfn;
> + int i, ret;
> +
> + for_each_mem_pfn_range(i, nid, &start_pfn,
> &end_pfn, NULL) {
> + ret =
> iommu_domain_identity_map(dmar_domain,
> + PFN_PHYS(start_pfn),
> PFN_PHYS(end_pfn));
> + if (ret)
> + return NULL;
> + }
> + }
> + break;
> + default:
> return NULL;
> }
> - domain_update_iommu_cap(dmar_domain);
> -
> - domain = &dmar_domain->domain;
> - domain->geometry.aperture_start = 0;
> - domain->geometry.aperture_end =
> __DOMAIN_MAX_ADDR(dmar_domain->gaw);
> - domain->geometry.force_aperture = true;
>
> - return domain;
> + return &dmar_domain->domain;
> }
>
> static void intel_iommu_domain_free(struct iommu_domain *domain)
> @@ -4974,6 +5005,9 @@ static int intel_iommu_attach_device(struct
> iommu_domain *domain, struct intel_iommu *iommu;
> int addr_width;
> u8 bus, devfn;
> + struct dmar_rmrr_unit *rmrr;
> + struct device *i_dev;
> + int i, ret;
>
> if (device_is_rmrr_locked(dev)) {
> dev_warn(dev, "Device is ineligible for IOMMU domain
> attach due to platform RMRR requirement. Contact your platform
> vendor.\n"); @@ -4990,8 +5024,8 @@ static int
> intel_iommu_attach_device(struct iommu_domain *domain,
> dmar_remove_one_dev_info(old_domain, dev); rcu_read_unlock();
> - if (!domain_type_is_vm_or_si(old_domain) &&
> - list_empty(&old_domain->devices))
> + if (list_empty(&old_domain->devices) &&
> + !domain_managed_externally(old_domain))
> domain_exit(old_domain);
If the old_domain is allocated by lazy DMA map call, the default domain
is not used in any case, right? why can't you use the default domain
for map call? I guess that is the next step?
> }
> }
> @@ -5000,6 +5034,12 @@ static int intel_iommu_attach_device(struct
> iommu_domain *domain, if (!iommu)
> return -ENODEV;
>
> + /* Initialise domain with IOMMU capabilities if it isn't
> already initialised */
> + if (!domain_is_initialised(dmar_domain)) {
> + if (domain_init(dmar_domain, iommu,
> DEFAULT_DOMAIN_ADDRESS_WIDTH))
> + return -ENOMEM;
> + }
> +
> /* check if this iommu agaw is sufficient for max mapped
> address */ addr_width = agaw_to_width(iommu->agaw);
> if (addr_width > cap_mgaw(iommu->cap))
> @@ -5028,6 +5068,27 @@ static int intel_iommu_attach_device(struct
> iommu_domain *domain, dmar_domain->agaw--;
> }
>
> + if (domain_type_is_vm_or_si(dmar_domain))
> + goto out;
> +
> + /* Ensure DMA domain has devices RMRR */
> + rcu_read_lock();
> + for_each_rmrr_units(rmrr) {
> + for_each_active_dev_scope(rmrr->devices,
> rmrr->devices_cnt,
> + i, i_dev) {
> + if (i_dev != dev)
why do we need to go through all devices, can we deduce the information
from device_domain_info? If RMRR is already mapped during init_dmar.
> + continue;
> +
> + ret = domain_prepare_identity_map(dev,
> dmar_domain,
> +
> rmrr->base_address,
> +
> rmrr->end_address);
> + if (ret)
> + dev_err(dev, "Mapping reserved
> region failed\n");
> + }
> + }
> + rcu_read_unlock();
> +
> +out:
Isn't it duplicate with the RMRR setup in init_dmars? I am guessing,
for device with RMRR, we can reuse the domain setup up during init
time, just inherit the mappings.
> return domain_add_dev_info(dmar_domain, dev);
> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC v2] iommu/vt-d: Allow iommu_domain_alloc to allocate IOMMU_DOMAIN_DMA
2019-01-07 20:04 ` Jacob Pan
@ 2019-01-15 14:26 ` James Sewart
0 siblings, 0 replies; 4+ messages in thread
From: James Sewart @ 2019-01-15 14:26 UTC (permalink / raw)
To: Jacob Pan; +Cc: iommu, linux-kernel, Dmitry Safonov
Hey Jacob,
> On 7 Jan 2019, at 20:04, Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:
>
> On Wed, 5 Dec 2018 17:19:35 +0000
> James Sewart <jamessewart@arista.com> wrote:
>
>> Hey,
>>
>> There exists an issue in the logic used to determine domain
>> association with devices. Currently the driver uses
>> find_or_alloc_domain to either reuse an existing domain or allocate a
>> new one if one isn’t found. Domains should be shared between all
>> members of an IOMMU group as this is the minimum granularity that we
>> can guarantee address space isolation.
>>
>> The intel IOMMU driver exposes pci_device_group in intel_iommu_ops as
>> the function to call to determine the group of a device, this is
>> implemented in the generic IOMMU code and checks: dma aliases,
>> upstream pcie switch ACS, pci aliases, and pci function aliases. The
>> find_or_alloc_domain code currently only uses dma aliases to
>> determine if a domain is shared. This causes a disconnect between
>> IOMMU groups and domains. We have observed devices under a pcie
>> switch each having their own domain but assigned the same group.
>>
>> One solution would be to fix the logic in find_or_alloc_domain to add
>> checks for the other conditions that a device may share a domain.
>> However, this duplicates code which the generic IOMMU code
>> implements. Instead this issue can be fixed by allowing the
>> allocation of default_domain on the IOMMU group. This is not
>> currently supported as the intel driver does not allow allocation of
>> domain type IOMMU_DOMAIN_DMA.
>>
>> Allowing allocation of DMA domains has the effect that the
>> default_domain is non NULL and is attached to a device when
>> initialising. This delegates the handling of domains to the generic
>> IOMMU code. Once this is implemented it is possible to remove the
>> lazy allocation of domains entirely.
>>
> This can also consolidate the domain storage, i.e. move domain from
> device_domain_info to iommu_group.
The plan was to have a patchset that first added the functionality below,
making use of the group domain storage but keeping the lazy allocation.
Then subsequent patches that remove the lazy allocation. To also remove
the device_domain_info it looks like some of the information there might
need moving into the domain?
>> This patch implements DMA and identity domains to be allocated for
>> external management. As it isn’t known which device will be attached
>> to a domain, the dma domain is not initialised at alloc time. Instead
>> it is allocated when attached. As we may lose RMRR mappings when
>> attaching a device to a new domain, we also ensure these are mapped
>> at attach time.
>>
>> This will likely conflict with the work done for auxiliary domains by
>> Baolu but the code to accommodate won’t change much.
>>
>> I had also started on a patch to remove find_or_alloc_domain and
>> various functions that call it but had issues with edge cases such as
>> iommu_prepare_isa that is doing domain operations at IOMMU init time.
>>
>> Cheers,
>> James.
>>
>>
>> ---
>> drivers/iommu/intel-iommu.c | 159
>> +++++++++++++++++++++++++----------- 1 file changed, 110
>> insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>> index 41a4b8808802..6437cb2e9b22 100644
>> --- a/drivers/iommu/intel-iommu.c
>> +++ b/drivers/iommu/intel-iommu.c
>> @@ -351,6 +351,14 @@ static int hw_pass_through = 1;
>> /* si_domain contains mulitple devices */
>> #define DOMAIN_FLAG_STATIC_IDENTITY (1 << 1)
>>
>> +/* Domain managed externally, don't cleanup if it isn't attached
>> + * to any devices. */
>> +#define DOMAIN_FLAG_NO_CLEANUP (1 << 2)
>> +
> the name NO_CLEANUP is a little counter intuitive to me, should it be
> called UNINITIALISED?
I don’t think uninitialised is accurate, the domain may be initialised. It
is used to distinguish between domains allocated by the external API,
which we shouldn’t automatically cleanup, and domains allocated internally,
which should. I agree a better name could be found.
>> +/* Set after domain initialisation. Used when allocating dma domains
>> to
>> + * defer domain initialisation until it is attached to a device */
>> +#define DOMAIN_FLAG_INITIALISED (1 << 4)
>> +
>> #define for_each_domain_iommu(idx, domain) \
>> for (idx = 0; idx < g_num_of_iommus; idx++) \
>> if (domain->iommu_refcnt[idx])
>> @@ -624,6 +632,16 @@ static inline int domain_type_is_vm_or_si(struct
>> dmar_domain *domain) DOMAIN_FLAG_STATIC_IDENTITY);
>> }
>>
>> +static inline int domain_managed_externally(struct dmar_domain
>> *domain) +{
>> + return domain->flags & DOMAIN_FLAG_NO_CLEANUP;
>> +}
>> +
>> +static inline int domain_is_initialised(struct dmar_domain *domain)
>> +{
>> + return domain->flags & DOMAIN_FLAG_INITIALISED;
>> +}
>> +
>> static inline int domain_pfn_supported(struct dmar_domain *domain,
>> unsigned long pfn)
>> {
>> @@ -1717,7 +1735,7 @@ static void disable_dmar_iommu(struct
>> intel_iommu *iommu)
>> __dmar_remove_one_dev_info(info);
>>
>> - if (!domain_type_is_vm_or_si(domain)) {
>> + if (!domain_managed_externally(domain)) {
>> /*
>> * The domain_exit() function can't be
>> called under
>> * device_domain_lock, as it takes this lock
>> itself. @@ -1951,6 +1969,7 @@ static int domain_init(struct
>> dmar_domain *domain, struct intel_iommu *iommu, domain->pgd = (struct
>> dma_pte *)alloc_pgtable_page(domain->nid); if (!domain->pgd)
>> return -ENOMEM;
>> + domain->flags |= DOMAIN_FLAG_INITIALISED;
>> __iommu_flush_cache(iommu, domain->pgd, PAGE_SIZE);
>> return 0;
>> }
>> @@ -3234,11 +3253,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
>> @@ -4529,7 +4522,7 @@ static int device_notifier(struct
>> notifier_block *nb, return 0;
>>
>> dmar_remove_one_dev_info(domain, dev);
>> - if (!domain_type_is_vm_or_si(domain) &&
>> list_empty(&domain->devices))
>> + if (!domain_managed_externally(domain) &&
>> list_empty(&domain->devices)) domain_exit(domain);
>>
>> return 0;
>> @@ -4930,6 +4923,7 @@ static int md_domain_init(struct dmar_domain
>> *domain, int guest_width) domain->pgd = (struct dma_pte
>> *)alloc_pgtable_page(domain->nid); if (!domain->pgd)
>> return -ENOMEM;
>> + domain->flags |= DOMAIN_FLAG_INITIALISED;
>> domain_flush_cache(domain, domain->pgd, PAGE_SIZE);
>> return 0;
>> }
>> @@ -4938,28 +4932,65 @@ static struct iommu_domain
>> *intel_iommu_domain_alloc(unsigned type) {
>> struct dmar_domain *dmar_domain;
>> struct iommu_domain *domain;
>> + int flags = DOMAIN_FLAG_NO_CLEANUP;
>> + int nid;
>>
>> - if (type != IOMMU_DOMAIN_UNMANAGED)
>> - return NULL;
>> -
>> - dmar_domain = alloc_domain(DOMAIN_FLAG_VIRTUAL_MACHINE);
>> - if (!dmar_domain) {
>> - pr_err("Can't allocate dmar_domain\n");
>> - return NULL;
>> - }
>> - if (md_domain_init(dmar_domain,
>> DEFAULT_DOMAIN_ADDRESS_WIDTH)) {
>> - pr_err("Domain initialization failed\n");
>> - domain_exit(dmar_domain);
>> + switch (type) {
>> + case IOMMU_DOMAIN_UNMANAGED:
>> + flags |= DOMAIN_FLAG_VIRTUAL_MACHINE |
>> DOMAIN_FLAG_INITIALISED;
>> + dmar_domain = alloc_domain(flags);
>> + if (!dmar_domain) {
>> + pr_err("Can't allocate dmar_domain\n");
>> + return NULL;
>> + }
>> + if (md_domain_init(dmar_domain,
>> DEFAULT_DOMAIN_ADDRESS_WIDTH)) {
>> + pr_err("Domain initialization failed\n");
>> + domain_exit(dmar_domain);
>> + return NULL;
>> + }
>> + domain_update_iommu_cap(dmar_domain);
>> + domain->geometry.aperture_start = 0;
>> + domain->geometry.aperture_end =
>> __DOMAIN_MAX_ADDR(dmar_domain->gaw);
>> + domain->geometry.force_aperture = true;
>> + break;
>> + case IOMMU_DOMAIN_DMA:
>> + dmar_domain = alloc_domain(flags);
>> + if (!dmar_domain) {
>> + pr_err("Can't allocate dmar_domain\n");
>> + return NULL;
>> + }
>> + /* init domain in device attach when we know IOMMU
>> capabilities */
>> + break;
>> + case IOMMU_DOMAIN_IDENTITY:
>> + flags |= DOMAIN_FLAG_STATIC_IDENTITY |
>> DOMAIN_FLAG_INITIALISED;
>> + dmar_domain = alloc_domain(flags);
>> + if (!dmar_domain) {
>> + pr_err("Can't allocate dmar_domain\n");
>> + return NULL;
>> + }
>> + if (md_domain_init(dmar_domain,
>> DEFAULT_DOMAIN_ADDRESS_WIDTH)) {
>> + pr_err("Domain initialization failed\n");
>> + domain_exit(dmar_domain);
>> + return NULL;
>> + }
>> + domain_update_iommu_cap(dmar_domain);
>> + for_each_online_node(nid) {
>> + unsigned long start_pfn, end_pfn;
>> + int i, ret;
>> +
>> + for_each_mem_pfn_range(i, nid, &start_pfn,
>> &end_pfn, NULL) {
>> + ret =
>> iommu_domain_identity_map(dmar_domain,
>> + PFN_PHYS(start_pfn),
>> PFN_PHYS(end_pfn));
>> + if (ret)
>> + return NULL;
>> + }
>> + }
>> + break;
>> + default:
>> return NULL;
>> }
>> - domain_update_iommu_cap(dmar_domain);
>> -
>> - domain = &dmar_domain->domain;
>> - domain->geometry.aperture_start = 0;
>> - domain->geometry.aperture_end =
>> __DOMAIN_MAX_ADDR(dmar_domain->gaw);
>> - domain->geometry.force_aperture = true;
>>
>> - return domain;
>> + return &dmar_domain->domain;
>> }
>>
>> static void intel_iommu_domain_free(struct iommu_domain *domain)
>> @@ -4974,6 +5005,9 @@ static int intel_iommu_attach_device(struct
>> iommu_domain *domain, struct intel_iommu *iommu;
>> int addr_width;
>> u8 bus, devfn;
>> + struct dmar_rmrr_unit *rmrr;
>> + struct device *i_dev;
>> + int i, ret;
>>
>> if (device_is_rmrr_locked(dev)) {
>> dev_warn(dev, "Device is ineligible for IOMMU domain
>> attach due to platform RMRR requirement. Contact your platform
>> vendor.\n"); @@ -4990,8 +5024,8 @@ static int
>> intel_iommu_attach_device(struct iommu_domain *domain,
>> dmar_remove_one_dev_info(old_domain, dev); rcu_read_unlock();
>> - if (!domain_type_is_vm_or_si(old_domain) &&
>> - list_empty(&old_domain->devices))
>> + if (list_empty(&old_domain->devices) &&
>> + !domain_managed_externally(old_domain))
>> domain_exit(old_domain);
> If the old_domain is allocated by lazy DMA map call, the default domain
> is not used in any case, right? why can't you use the default domain
> for map call? I guess that is the next step?
With just this patch the default domain will be attached to all devices
during generic IOMMU code initialisation. A device may or may not have had
a domain lazy allocated at this point depending on whether a map call has
been made yet, i.e. for RMRR.
After the default domain is attached, subsequent map calls will use the
default domain.
>> }
>> }
>> @@ -5000,6 +5034,12 @@ static int intel_iommu_attach_device(struct
>> iommu_domain *domain, if (!iommu)
>> return -ENODEV;
>>
>> + /* Initialise domain with IOMMU capabilities if it isn't
>> already initialised */
>> + if (!domain_is_initialised(dmar_domain)) {
>> + if (domain_init(dmar_domain, iommu,
>> DEFAULT_DOMAIN_ADDRESS_WIDTH))
>> + return -ENOMEM;
>> + }
>> +
>> /* check if this iommu agaw is sufficient for max mapped
>> address */ addr_width = agaw_to_width(iommu->agaw);
>> if (addr_width > cap_mgaw(iommu->cap))
>> @@ -5028,6 +5068,27 @@ static int intel_iommu_attach_device(struct
>> iommu_domain *domain, dmar_domain->agaw--;
>> }
>>
>> + if (domain_type_is_vm_or_si(dmar_domain))
>> + goto out;
>> +
>> + /* Ensure DMA domain has devices RMRR */
>> + rcu_read_lock();
>> + for_each_rmrr_units(rmrr) {
>> + for_each_active_dev_scope(rmrr->devices,
>> rmrr->devices_cnt,
>> + i, i_dev) {
>> + if (i_dev != dev)
> why do we need to go through all devices, can we deduce the information
> from device_domain_info? If RMRR is already mapped during init_dmar.
I am unsure the best way to implement this. Currently RMRR structures are
setup so that we find applicable devices inside the RMRR struct, rather
than finding the RMRR via a device struct. I don’t think you can get the
RMRR info for a device via the device_domain_info struct.
I realised after posting this that the desired behaviour is already
implemented by intel_iommu_get_resv_regions. Currently this function is
used for exposing RMRR information via sysfs, but it is also used for
group domains to make identity mappings. We would automatically get the
desired behaviour by supporting default_domain.
The RMRR mappings done in init_dmar are lost when attaching a new domain
and I think they can/should be removed. I’m unsure if it is desirable to
inherit mappings of an old domain when attaching a new one, given the
generic IOMMU code will try to remap the RMRR regions anyway.
>> + continue;
>> +
>> + ret = domain_prepare_identity_map(dev,
>> dmar_domain,
>> +
>> rmrr->base_address,
>> +
>> rmrr->end_address);
>> + if (ret)
>> + dev_err(dev, "Mapping reserved
>> region failed\n");
>> + }
>> + }
>> + rcu_read_unlock();
>> +
>> +out:
> Isn't it duplicate with the RMRR setup in init_dmars? I am guessing,
> for device with RMRR, we can reuse the domain setup up during init
> time, just inherit the mappings.
As I said above I don’t think this is the best way to implement RMRR’s. It
will be tricky to correctly inherit mappings and it will be simpler to
remove RMRR setup in init_dmars and instead delegate the mapping to the
generic IOMMU steup.
>
>> return domain_add_dev_info(dmar_domain, dev);
>> }
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-01-15 14:26 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-05 17:19 [RFC v2] iommu/vt-d: Allow iommu_domain_alloc to allocate IOMMU_DOMAIN_DMA James Sewart
2019-01-02 14:22 ` James Sewart
2019-01-07 20:04 ` Jacob Pan
2019-01-15 14:26 ` James Sewart
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).