Re: [RFC v2] iommu/vt-d: Allow iommu_domain_alloc to allocate IOMMU_DOMAIN_DMA
Hey Jacob,
> On 7 Jan 2019, at 20:04, Jacob Pan wrote:
>
> On Wed, 5 Dec 2018 17:19:35 +
> James Sewart 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
Re: [RFC v2] iommu/vt-d: Allow iommu_domain_alloc to allocate IOMMU_DOMAIN_DMA
On Wed, 5 Dec 2018 17:19:35 +
James Sewart 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;
> }
>
Re: [RFC v2] iommu/vt-d: Allow iommu_domain_alloc to allocate IOMMU_DOMAIN_DMA
Bump
> On 5 Dec 2018, at 17:19, James Sewart 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)
> {
>
