Hey, > On 7 Nov 2018, at 02:10, Lu Baolu <[email protected]> wrote: > > Hi, > > On 11/6/18 6:40 PM, James Sewart wrote: >> Hey Lu, >> Would you be able to go into more detail about the issues with >> allowing IOMMU_DOMAIN_DMA to be allocated via domain_alloc? > > This door is closed because intel iommu driver does everything for > IOMMU_DOMAIN_DMA: allocating a domain and setup the context entries > for the domain.
As far as I can tell, attach_device in the intel driver will handle cleaning up any old domain context mapping and ensure the new domain is mapped with calls to dmar_remove_one_dev_info and domain_add_dev_info. > > Why do we want to open this door? Probably we want the generic iommu > layer to handle these things (it's called default domain). I’d like to allocate a domain and attach it to multiple devices in a group/multiple groups so that they share address translation, but still allow drivers for devices in those groups to use the dma_map_ops api. > So we can't just open the door but not cleanup the things right? A user of domain_alloc and attach_device is responsible for detaching a domain if it is no longer needed and calling domain_free. Cheers, James. > > I haven't spent time on details. So I cc'ed Jacob for corrections. > > Best regards, > Lu Baolu > >> Cheers, >> James. >> On Fri, Nov 2, 2018 at 2:43 AM Lu Baolu <[email protected]> wrote: >>> >>> Hi, >>> >>> On 10/30/18 10:18 PM, James Sewart via iommu wrote: >>>> Hey, >>>> >>>> I’ve been investigating the relationship between iommu groups and domains >>>> on our systems and have a few question. Why does the intel iommu code not >>>> allow allocating IOMMU_DOMAIN_DMA? Returning NULL when given this domain >>>> type has the side effect that the default_domain for an iommu group is not >>>> set, which, when using for e.g. dma_map_ops.map_page, means a domain is >>>> allocated per device. >>> >>> Intel vt-d driver doesn't implement the default domain and allocates >>> domain only on demanded. There are lots of things to do before we allow >>> iommu API to allocate domains other than IOMMU_DOMAIN_UNMANAGED. >>> >>> Best regards, >>> Lu Baolu >>> >>>> >>>> This seems to be the opposite behaviour to the AMD iommu code which >>>> supports allocating an IOMMU_DOMAIN_DMA and will only look to the iommu >>>> group if a domain is not attached to the device rather than allocating a >>>> new one. On AMD every device in an iommu group will share the same domain. >>>> >>>> Appended is what I think a patch to implement domain_alloc for >>>> IOMMU_DOMAIN_DMA and also IOMMU_DOMAIN_IDENTITY would look like. Testing >>>> shows each device in a group will share a domain by default, it also >>>> allows allocating a new dma domain that can be successfully attached to a >>>> group with iommu_attach_group. >>>> >>>> Looking for comment on why the behaviour is how it is currently and if >>>> there are any issues with the solution I’ve been testing. >>>> >>>> Cheers, >>>> James. >>>> >>>> >>>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c >>>> index bff2abd6..3a58389f 100644 >>>> --- a/drivers/iommu/intel-iommu.c >>>> +++ b/drivers/iommu/intel-iommu.c >>>> @@ -5170,10 +5170,15 @@ static struct iommu_domain >>>> *intel_iommu_domain_alloc(unsigned type) >>>> struct dmar_domain *dmar_domain; >>>> struct iommu_domain *domain; >>>> >>>> - if (type != IOMMU_DOMAIN_UNMANAGED) >>>> + if (type == IOMMU_DOMAIN_UNMANAGED) >>>> + dmar_domain = alloc_domain(DOMAIN_FLAG_VIRTUAL_MACHINE); >>>> + else if(type == IOMMU_DOMAIN_DMA) >>>> + dmar_domain = alloc_domain(0); >>>> + else if(type == IOMMU_DOMAIN_IDENTITY) >>>> + dmar_domain = alloc_domain(DOMAIN_FLAG_STATIC_IDENTITY); >>>> + else >>>> return NULL; >>>> >>>> - dmar_domain = alloc_domain(DOMAIN_FLAG_VIRTUAL_MACHINE); >>>> if (!dmar_domain) { >>>> pr_err("Can't allocate dmar_domain\n"); >>>> return NULL; >>>> @@ -5186,9 +5191,12 @@ static struct iommu_domain >>>> *intel_iommu_domain_alloc(unsigned type) >>>> 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; >>>> + >>>> + if (type == IOMMU_DOMAIN_UNMANAGED) { >>>> + domain->geometry.aperture_start = 0; >>>> + domain->geometry.aperture_end = >>>> __DOMAIN_MAX_ADDR(dmar_domain->gaw); >>>> + domain->geometry.force_aperture = true; >>>> + } >>>> >>>> return domain; >>>> } >>>> _______________________________________________ >>>> iommu mailing list >>>> [email protected] >>>> https://lists.linuxfoundation.org/mailman/listinfo/iommu >>>> _______________________________________________ iommu mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/iommu
