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

[Lu Baolu <baolu.lu@linux.intel.com>]

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