linux-kernel - Re: [PATCH v3 2/2] iommu/vt-d: don's issue devTLB flush request when device is disconnected

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <53591775-b93c-4963-b57b-8027e39a6ecc@linux.intel.com>
Date: Tue, 19 Dec 2023 15:27:14 +0800
From: Ethan Zhao <haifeng.zhao@...ux.intel.com>
To: Baolu Lu <baolu.lu@...ux.intel.com>
Cc: linux-pci@...r.kernel.org, iommu@...ts.linux.dev,
 linux-kernel@...r.kernel.org, bhelgaas@...gle.com, dwmw2@...radead.org,
 will@...nel.org, robin.murphy@....com, lukas@...ner.de
Subject: Re: [PATCH v3 2/2] iommu/vt-d: don's issue devTLB flush request when
 device is disconnected

Baolu,

On 12/19/2023 3:16 PM, Ethan Zhao wrote:
>
> On 12/19/2023 2:57 PM, Baolu Lu wrote:
>> On 2023/12/19 14:49, Ethan Zhao wrote:
>>>>> diff --git a/drivers/iommu/intel/pasid.c 
>>>>> b/drivers/iommu/intel/pasid.c
>>>>> index 74e8e4c17e81..182eb5df244d 100644
>>>>> --- a/drivers/iommu/intel/pasid.c
>>>>> +++ b/drivers/iommu/intel/pasid.c
>>>>> @@ -476,6 +476,23 @@ devtlb_invalidation_with_pasid(struct 
>>>>> intel_iommu *iommu,
>>>>>   {
>>>>>       struct device_domain_info *info;
>>>>>       u16 sid, qdep, pfsid;
>>>>> +    struct pci_dev *pdev;
>>>>> +
>>>>> +    pdev = to_pci_dev(dev);
>>>>> +    if (!pdev)
>>>>> +        return;
>>>>> +
>>>>> +    /*
>>>>> +     * If endpoint device's link was brough down by user's pci 
>>>>> configuration
>>>>> +     * access to it's hotplug capable slot link control register, 
>>>>> as sequence
>>>>> +     * response for DLLSC, pciehp_ist() will set the device 
>>>>> error_state to
>>>>> +     * pci_channel_io_perm_failure. Checking device's state here 
>>>>> to avoid
>>>>> +     * issuing meaningless devTLB flush request to it, that might 
>>>>> cause lockup
>>>>> +     * warning or deadlock because too long time waiting in 
>>>>> interrupt context.
>>>>> +     */
>>>>> +
>>>>> +    if (pci_dev_is_disconnected(pdev))
>>>>> +        return;
>>>>>         info = dev_iommu_priv_get(dev);
>>>>>       if (!info || !info->ats_enabled)
>>>>
>>>> It's likely better to check the device status after verifying
>>>> ats_enabled. How about below change?
>>>>
>>>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>>>> index 74e8e4c17e81..fa19c6cdfd8b 100644
>>>> --- a/drivers/iommu/intel/pasid.c
>>>> +++ b/drivers/iommu/intel/pasid.c
>>>> @@ -481,6 +481,9 @@ devtlb_invalidation_with_pasid(struct 
>>>> intel_iommu *iommu,
>>>>         if (!info || !info->ats_enabled)
>>>>                 return;
>>>>
>>>> +       if (pci_dev_is_disconnected(to_pci_dev(dev)))
>>>
>>> I like this kind of simplicity, but rationalist always brings me 
>>> back to the no-error(ugly)
>>>
>>> style.  🙂
>>
>> The rational is that Intel IOMMU driver only supports PCI ATS. So if
>> device is marked as ATS supported, then it must be a PCI device.
>> Therefore, it's safe to convert it to pci_device with to_pci_dev().
>
> Fair engough !
>
>
May I use  your reviewed-by sign in next version after that ?


Thanks,

Ethan

> Thanks,
>
> Ethan
>
>>
>> If you move this up before ATS checking, then you need to check
>> dev_is_pci().
>>
>> Best regards,
>> baolu
>>
>