Re: [Intel-gfx] [PATCH] drm/i915: Use per device iommu check

Lu Baolu
2021-11-12T01:03:12Z

On 11/11/21 11:18 PM, Tvrtko Ursulin wrote:
>
> On 10/11/2021 14:37, Robin Murphy wrote:
>> On 2021-11-10 14:11, Tvrtko Ursulin wrote:
>>>
>>> On 10/11/2021 12:35, Lu Baolu wrote:
>>>> On 2021/11/10 20:08, Tvrtko Ursulin wrote:
>>>>>
>>>>> On 10/11/2021 12:04, Lu Baolu wrote:
>>>>>> On 2021/11/10 17:30, Tvrtko Ursulin wrote:
>>>>>>>
>>>>>>> On 10/11/2021 07:12, Lu Baolu wrote:
>>>>>>>> Hi Tvrtko,
>>>>>>>>
>>>>>>>> On 2021/11/9 20:17, Tvrtko Ursulin wrote:
>>>>>>>>> From: Tvrtko Ursulin<tvrtko.ursulin@intel.com>
>>>>>>>>>
>>>>>>>>> On igfx + dgfx setups, it appears that intel_iommu=igfx_off
>>>>>>>>> option only
>>>>>>>>> disables the igfx iommu. Stop relying on global
>>>>>>>>> intel_iommu_gfx_mapped
>>>>>>>>> and probe presence of iommu domain per device to accurately
>>>>>>>>> reflect its
>>>>>>>>> status.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Tvrtko Ursulin<tvrtko.ursulin@intel.com>
>>>>>>>>> Cc: Lu Baolu<baolu.lu@linux.intel.com>
>>>>>>>>> ---
>>>>>>>>> Baolu, is my understanding here correct? Maybe I am confused by
>>>>>>>>> both
>>>>>>>>> intel_iommu_gfx_mapped and dmar_map_gfx being globals in the
>>>>>>>>> intel_iommu
>>>>>>>>> driver. But it certainly appears the setup can assign some
>>>>>>>>> iommu ops (and
>>>>>>>>> assign the discrete i915 to iommu group) when those two are set
>>>>>>>>> to off.
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>>>>>>> b/drivers/gpu/drm/i915/i915_drv.h
>>>>>>>> index e967cd08f23e..9fb38a54f1fe 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>>>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>>>>>> @@ -1763,26 +1763,27 @@ static inline bool run_as_guest(void)
>>>>>>>>   #define HAS_D12_PLANE_MINIMIZATION(dev_priv)
>>>>>>>> (IS_ROCKETLAKE(dev_priv) || \
>>>>>>>>                             IS_ALDERLAKE_S(dev_priv))
>>>>>>>>
>>>>>>>> -static inline bool intel_vtd_active(void)
>>>>>>>> +static inline bool intel_vtd_active(struct drm_i915_private *i915)
>>>>>>>>   {
>>>>>>>> -#ifdef CONFIG_INTEL_IOMMU
>>>>>>>> -    if (intel_iommu_gfx_mapped)
>>>>>>>> +    if (iommu_get_domain_for_dev(i915->drm.dev))
>>>>>>>>           return true;
>>>>>>>> -#endif
>>>>>>>>
>>>>>>>>       /* Running as a guest, we assume the host is enforcing
>>>>>>>> VT'd */
>>>>>>>>       return run_as_guest();
>>>>>>>>   }
>>>>>>>>
>>>>>>>> Have you verified this change? I am afraid that
>>>>>>>> iommu_get_domain_for_dev() always gets a valid iommu domain even
>>>>>>>> intel_iommu_gfx_mapped == 0.
>>>>>>>
>>>>>>> Yes it seems to work as is:
>>>>>>>
>>>>>>> default:
>>>>>>>
>>>>>>> # grep -i iommu /sys/kernel/debug/dri/*/i915_capabilities
>>>>>>> /sys/kernel/debug/dri/0/i915_capabilities:iommu: enabled
>>>>>>> /sys/kernel/debug/dri/1/i915_capabilities:iommu: enabled
>>>>>>>
>>>>>>> intel_iommu=igfx_off:
>>>>>>>
>>>>>>> # grep -i iommu /sys/kernel/debug/dri/*/i915_capabilities
>>>>>>> /sys/kernel/debug/dri/0/i915_capabilities:iommu: disabled
>>>>>>> /sys/kernel/debug/dri/1/i915_capabilities:iommu: enabled
>>>>>>>
>>>>>>> On my system dri device 0 is integrated graphics and 1 is discrete.
>>>>>>
>>>>>> The drm device 0 has a dedicated iommu. When the user request igfx
>>>>>> not
>>>>>> mapped, the VT-d implementation will turn it off to save power.
>>>>>> But for
>>>>>> shared iommu, you definitely will get it enabled.
>>>>>
>>>>> Sorry I am not following, what exactly do you mean? Is there a
>>>>> platform with integrated graphics without a dedicated iommu, in
>>>>> which case intel_iommu=igfx_off results in intel_iommu_gfx_mapped
>>>>> == 0 and iommu_get_domain_for_dev returning non-NULL?
>>>>
>>>> Your code always work for an igfx with a dedicated iommu. This might be
>>>> always true on today's platforms. But from driver's point of view, we
>>>> should not make such assumption.
>>>>
>>>> For example, if the iommu implementation decides not to turn off the
>>>> graphic iommu (perhaps due to some hw quirk or for graphic
>>>> virtualization), your code will be broken.
>>>
>>> If I got it right, this would go back to your earlier recommendation
>>> to have the check look like this:
>>>
>>> static bool intel_vtd_active(struct drm_i915_private *i915)
>>> {
>>>          struct iommu_domain *domain;
>>>
>>>          domain = iommu_get_domain_for_dev(i915->drm.dev);
>>>          if (domain && (domain->type & __IOMMU_DOMAIN_PAGING))
>>>                  return true;
>>>      ...
>>>
>>> This would be okay as a first step?
>>>
>>> Elsewhere in the thread Robin suggested looking at the dec->dma_ops
>>> and comparing against iommu_dma_ops. These two solution would be
>>> effectively the same?
>>
>> Effectively, yes. See iommu_setup_dma_ops() - the only way to end up
>> with iommu_dma_ops is if a managed translation domain is present; if
>> the IOMMU is present but the default domain type has been set to
>> passthrough (either globally or forced for the given device) it will
>> do nothing and leave you with dma-direct, while if the IOMMU has been
>> ignored entirely then it should never even be called. Thus it neatly
>> encapsulates what you're after here.
>
> One concern I have is whether the pass-through mode truly does nothing
> or addresses perhaps still go through the dmar hardware just with no
> translation?

Pass-through mode means the latter.

>
> If latter then most like for like change is actually exactly what the
> first version of my patch did. That is replace intel_iommu_gfx_mapped
> with a plain non-NULL check on iommu_get_domain_for_dev.

Depends on what you want here,

#1) the graphic device works in iommu pass-through mode
- device have an iommu
- but iommu does no translation
- the dma transactions go through iommu with the same destination
memory address specified by the device;

#2) the graphic device works without a system iommu
- the iommu is off
- there's no iommu on the path of DMA transaction.

My suggestion works for #1). Robin's suggestion (device_iommu_mapped())
could work for #2).

Best regards,
baolu