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

Tvrtko Ursulin
2021-11-12T14:10:53Z

On 12/11/2021 00:58, Lu Baolu wrote:
> 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;

Do you have any indications of the slowdown this adds?

> #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).

On the question of what do I want here. It seems that to preserve
like-for-like with the current and past i915 usage, ie.
intel_iommu_gfx_mapped, the first version of my patch should be used.

In other words if I configure the boot with iommu=pt, then
intel_iommu_gfx_mapped is true. So if I add the __IOMMU_DOMAIN_PAGING
check, the new intel_vtd_active would return false, where the old
version would return true.

So v1 of the patch feels like the safest route given I don't know which
workarounds are due remapping slowdown, and which may be present even in
the pass-through mode.

I would explain the situation in the comment inside intel_vtd_active for
future reference.

Regards,

Tvrtko