Re: [PATCH v2 0/7] iommu/vt-d: Fix-up device-domain relationship by refactoring to use iommu group default domain.

[Lu Baolu <baolu.lu-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>]

Hi James,

On 3/22/19 6:05 PM, James Sewart wrote:
> Hey Lu,
> 
>> On 20 Mar 2019, at 01:26, Lu Baolu <baolu.lu@linux.intel.com> wrote:
>>
>> Hi James,
>>
>> On 3/19/19 9:35 PM, James Sewart wrote:
>>> Hey Lu,
>>>> On 15 Mar 2019, at 03:13, Lu Baolu <baolu.lu@linux.intel.com> wrote:
>>>>
>>>> Hi James,
>>>>
>>>> On 3/14/19 7:56 PM, James Sewart wrote:
>>>>> Patches 1 and 2 are the same as v1.
>>>>> v1-v2:
>>>>>    Refactored ISA direct mappings to be returned by iommu_get_resv_regions.
>>>>>    Integrated patch by Lu to defer turning on DMAR until iommu.c has mapped
>>>>> reserved regions.
>>>>>    Integrated patches by Lu to remove more unused code in cleanup.
>>>>> Lu: I didn't integrate your patch to set the default domain type as it
>>>>> isn't directly related to the aim of this patchset. Instead patch 4
>>>>
>>>> Without those patches, user experience will be affected and some devices
>>>> will not work on Intel platforms anymore.
>>>>
>>>> For a long time, Intel IOMMU driver has its own logic to determine
>>>> whether a device requires an identity domain. For example, when user
>>>> specifies "iommu=pt" in kernel parameter, all device will be attached
>>>> with the identity domain. Further more, some quirky devices require
>>>> an identity domain to be used before enabling DMA remapping, otherwise,
>>>> it will not work. This was done by adding quirk bits in Intel IOMMU
>>>> driver.
>>>>
>>>> So from my point of view, one way is porting all those quirks and kernel
>>>> parameters into IOMMU generic layer, or opening a door for vendor IOMMU
>>>> driver to determine the default domain type by their own. I prefer the
>>>> latter option since it will not impact any behaviors on other
>>>> architectures.
>>> I see your point. I’m not confident that using the proposed door to set a
>>> groups default domain has the desired behaviour. As discussed before the
>>> default domain type will be set based on the desired type for only the
>>> first device attached to a group. I think to change the default domain
>>> type you would need a slightly different door that wasn’t conditioned on
>>> device.
>>
>> I think this as another problem. Just a summarize for the ease of
>> discussion. We saw two problems:
>>
>> 1. When allocating a new group for a device, how should we determine the
>> type of the default domain? This is what my proposal patches trying to
>> address.
> 
> This will work as expected only if all devices within a group get the same
> result from is_identity_map. Otherwise wee see issue 2.
> 
>>
>> 2. If we need to put a device into an existing group which uses a
>> different type of domain from what the device desires to use, we might
>> break the functionality of the device. For this problem I'd second your
>> proposal below if I get your point correctly.
>>
>>> For situations where individual devices require an identity domain because
>>> of quirks then maybe calling is_identity_map per device in
>>> iommu_group_get_for_dev is a better solution than the one I proposed.
>>
>> Do you mean if we see a quirky device requires a different domain type
>> other than the default domain type of the group, we will assign a new
>> group to it? That looks good to me as far as I can see. I suppose this
>> should be done in vt-d's ops callback.
> 
> I have thought about this a bit and I think the cleanest approach is to
> put devices that require an identity domain into their own group, this can
> be done in the device_group callback, avoiding any situation where we have
> to deal with creating a new group based on domain type in
> iommu_group_get_for_dev. This way we shouldn’t ever be in a situation with
> multiple different domain types per group. This way your patches will work
> as expected. See below for a possible implementation.
> 
>>
>> Best regards,
>> Lu Baolu
> 
> Cheers,
> James.
> 
> Devices that require an identity map because of quirks or other reasons
> should be put in their own IOMMU group so as to not end up with multiple
> different domains per group.

Yeah! This looks good to me.

Best regards,
Lu Baolu

> 
> Signed-off-by: James Sewart <jamessewart@arista.com>
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 3cb8b36abf50..0f5a121d31a0 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -5421,6 +5421,22 @@ struct intel_iommu *intel_svm_device_to_iommu(struct device *dev)
>   }
>   #endif /* CONFIG_INTEL_IOMMU_SVM */
> 
> +static struct iommu_group *intel_identity_group;
> +
> +struct iommu_group *intel_iommu_pci_device_group(struct device *dev)
> +{
> +       if (iommu_no_mapping(dev)) {
> +               if (!intel_identity_group) {
> +                       intel_identity_group = iommu_group_alloc();
> +                       if (IS_ERR(intel_identity_group))
> +                               return NULL;
> +               }
> +               return intel_identity_group;
> +       }
> +
> +       return pci_device_group(dev);
> +}
> +
>   const struct iommu_ops intel_iommu_ops = {
>          .capable                = intel_iommu_capable,
>          .domain_alloc           = intel_iommu_domain_alloc,
> @@ -5435,7 +5451,7 @@ const struct iommu_ops intel_iommu_ops = {
>          .get_resv_regions       = intel_iommu_get_resv_regions,
>          .put_resv_regions       = intel_iommu_put_resv_regions,
>          .apply_resv_region      = intel_iommu_apply_resv_region,
> -       .device_group           = pci_device_group,
> +       .device_group           = intel_iommu_pci_device_group,
>          .pgsize_bitmap          = INTEL_IOMMU_PGSIZES,
>   };
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu