Re: [PATCH] intel/vt-d: Send Page Request Drain only if supported

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

On 9/12/25 10:35, Baolu Lu wrote:
> On 9/9/25 16:58, Joel Granados wrote:
>> The intel_iommu_drain_pasid_prq function sends QI_OPT_WAIT_DRAIN to
>> hardware without verifying Page Request Drain Support (PDS). According
>> to VT-d specification section 6.5.2.8, PRQ drain functionality should
>> only be used when PDS (bit 42) is set in the extended capability
>> register. Add ecap_pds() check to conditionally use QI_OPT_WAIT_DRAIN
>> based on hardware capability.
>>
>> Signed-off-by: Joel Granados<joel.granados@kernel.org>
>> ---
>>   drivers/iommu/intel/prq.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/intel/prq.c b/drivers/iommu/intel/prq.c
>> index 
>> 52570e42a14c05b7492957909568805dc9c7b6ef..f89916de31a3439866f059f5400e45fb362a6a7d 100644
>> --- a/drivers/iommu/intel/prq.c
>> +++ b/drivers/iommu/intel/prq.c
>> @@ -119,7 +119,7 @@ void intel_iommu_drain_pasid_prq(struct device 
>> *dev, u32 pasid)
>>       }
>>   qi_retry:
>>       reinit_completion(&iommu->prq_complete);
>> -    qi_submit_sync(iommu, desc, 3, QI_OPT_WAIT_DRAIN);
>> +    qi_submit_sync(iommu, desc, 3, ecap_pds(iommu->ecap) ? 
>> QI_OPT_WAIT_DRAIN : 0);
> 
> I'm afraid that draining page requests and responses won't work as
> expected without the PDS capability. We should perhaps fail to enable
> IOPF if the PDS capability isn't supported.
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 9c3ab9d9f69a..ca6a6eaea62c 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -3983,6 +3983,10 @@ int intel_iommu_enable_iopf(struct device *dev)
>          if (!info->pri_enabled)
>                  return -ENODEV;
> 
> +       /* PDS is required to drain page requests and responses. */
> +       if (!ecap_pds(iommu->ecap))
> +               return -ENODEV;
> +
>          /* pri_enabled is protected by the group mutex. */
>          iommu_group_mutex_assert(dev);
>          if (info->iopf_refcount) {
> 
> At the same time, qi_submit_sync() should not set PD bit in the wait
> descriptor as the spec Section 6.5.2.9 "Invalidation Wait Descriptor"
> states:
> 
>   Page-request Drain (PD): Remapping hardware implementations reporting
>   Page-request draining as not supported (PDS = 0 in ECAP_REG) treats
>   this field as reserved.
> 
> therefore,
> 
> diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
> index ec975c73cfe6..e38af2274032 100644
> --- a/drivers/iommu/intel/dmar.c
> +++ b/drivers/iommu/intel/dmar.c
> @@ -1427,7 +1427,7 @@ int qi_submit_sync(struct intel_iommu *iommu, 
> struct qi_desc *desc,
> 
>          wait_desc.qw0 = QI_IWD_STATUS_DATA(QI_DONE) |
>                          QI_IWD_STATUS_WRITE | QI_IWD_TYPE;
> -       if (options & QI_OPT_WAIT_DRAIN)
> +       if ((options & QI_OPT_WAIT_DRAIN) && !WARN_ON_ONCE(! 
> ecap_pds(iommu->ecap)))
>                  wait_desc.qw0 |= QI_IWD_PRQ_DRAIN;
>          wait_desc.qw1 = virt_to_phys(&qi->desc_status[wait_index]);
>          wait_desc.qw2 = 0;
> 
> ?

I submitted a fix patch for this. Feel free to comment on it there.

https://lore.kernel.org/linux-iommu/20250915062946.120196-1-baolu.lu@linux.intel.com/

Thanks,
baolu