Hey Jacob,
> On 9 Nov 2018, at 19:09, Jacob Pan <jacob.jun....@linux.intel.com> wrote:
>
> On Thu, 8 Nov 2018 11:30:04 +0000
> James Sewart <jamessew...@arista.com <mailto:jamessew...@arista.com>> wrote:
>
>> Hey,
>>
>>> On 8 Nov 2018, at 01:42, Lu Baolu <baolu...@linux.intel.com> wrote:
>>>
>>> Hi,
>>>
>>> On 11/8/18 1:55 AM, James Sewart wrote:
>>>> Hey,
>>>>> On 7 Nov 2018, at 02:10, Lu Baolu <baolu...@linux.intel.com>
>>>>> 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.
>>>
>>> That's only for domains of IOMMU_DOMAIN_UNMANAGED, right?
>>
>> attach_device has logic for cleaning up domains that are allocated by
>> the intel driver.
>>
>> old_domain = find_domain(dev);
>> if (old_domain) {
>> rcu_read_lock();
>> 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))
>> domain_exit(old_domain);
>> }
>>
>> This is checking the type of the old domain only, freeing it if is
>> not attached to any devices. Looking at this now, maybe the solution
>> would be to distinguish between internally allocated dma domains and
>> dma domains allocated via the external api, so we avoid freeing a
>> domain a driver has reference to.
>>
> I think this can also be solved by adopting default domain and moving
> internal domain to the generic code at group level. i.e. when device is
> added to a group, the group gets a default domain (also set as current
> domain) for DMA APIs. When device is attached to an unmanaged domain
> (VFIO container and IOMMU API), it will switch the current domain to
> the new domain. Old domain will be inactive but not exited.
>>>>>
>>>>> 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.
>>>
>>> Just out of curiosity, why do you want to share a single domain
>>> across multiple groups? By default, the groups and DMA domains are
>>> normally 1-1 mapped, right?
>>
>> Currently we see each device in a group with their own domain.
>> find_or_alloc_domain looks at dma aliases to determine who shares
>> domains whereas pci_device_group in the generic iommu code determines
>> groups using a few other checks. We have observed internally that
>> devices under a pcie switch will be put in the same group but they do
>> not share a domain within that group.
>>
>> Getting every device within a group to share a domain would get us
>> 90% of the way to what we want. But we have some configurations where
>> there exist devices put in other groups that we want to share
>> translations with.
>>
> This is indeed a huge disconnect between IOMMU API and VT-d handling
> of the DMA API. IOMMU API operates at group granu (per group domain)
> that is based on ACS whereas DMA API in VT-d has per device domain and
> sharing is not based on ACS. So I agree you could have multiple domains
> under the same group for DMA API use.
>
> I am working on a patch to align the two, e.g. use per group
> default domain for DMA APIs. My idea is as follows:
> - support DOMAIN_DMA type in intel_iommu_domain_alloc()
> - add device to group will attach to the default DMA domain
> - have to inherit any RMRR or identity mapping holes set up
> prior to IOMMU group setup.
> - Or perhaps implement apply_resv_region() in VT-d driver and
> move RMRR setup here.
> - DMA map API, will use per group default domain instead of per device
> private domain
>
> The change is rather pervasive so i am trying to set a balance for
> short term functionality and complete clean up. Any ideas welcome.
>
Sounds good, thanks. I am eager to test the patch.
>>>
>>>>> 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.
>>>
>>> Currently DMA API calls get_valid_domain_for_dev() to retrieve a DMA
>>> domain. If the domain has already been allocated, return directly.
>>> Otherwise, allocate and initialize a new one for the device. Let's
>>> call domains allocated by get_valid_domain_for_dev() as "A".
>>>
>>> If we open the door and allow another component to manage the DMA
>>> domains through domain iommu_domain_alloc/free(). Let's call domains
>>> allocated through iommu_domain_alloc() as "B".
>>>
>>> So how can we sync between A and B?
>>
>> I’m not sure we need to sync them. Domain A would be replaced
>> entirely for a certain device with domain B by attach_device. As we
>> saw above domain A would be freed if there are no more devices
>> attached to it. Domain B would be managed by the user of the iommu
>> api. If a device is detached from domain B, the find_or_alloc_domain
>> logic will take over once more and allocate a fresh domain.
>>
> Agreed, it is more like switching domains when you switching APIs. But
> the reserved region better be preserved.
>> Cheers,
>> James.
>>
>>>
>>> Need to go through the code to find out more.
>>>
>>> Best regards,
>>> Lu Baolu
>>>> 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
>>>>>> <baolu...@linux.intel.com> 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
>>>>>>>> iommu@lists.linux-foundation.org
>>>>>>>> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>>>>>>>>
>>
>
> [Jacob Pan]
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
