Received-SPF: pass (google.com: domain of linux-kernel+bounces-70821-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <2fa8af78-5277-4278-b0e2-8a983dbbab2c@linux.intel.com>
Date: Mon, 19 Feb 2024 15:22:33 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Cc: baolu.lu@linux.intel.com, Huang Jiaqing <jiaqing.huang@intel.com>,
 iommu@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] iommu/vt-d: Use device rbtree in iopf reporting path
Content-Language: en-US
To: Ethan Zhao <haifeng.zhao@linux.intel.com>, Joerg Roedel
 <joro@8bytes.org>, Will Deacon <will@kernel.org>,
 Robin Murphy <robin.murphy@arm.com>, Jason Gunthorpe <jgg@ziepe.ca>,
 Kevin Tian <kevin.tian@intel.com>
References: <20240215072249.4465-1-baolu.lu@linux.intel.com>
 <20240215072249.4465-3-baolu.lu@linux.intel.com>
 <0da83c5c-0e35-4087-aacf-6831060cab8e@linux.intel.com>
 <e59f6ba0-a289-4c3f-8746-b58f9474a35b@linux.intel.com>
 <be26926a-5843-46c6-a0b9-c51b5d8e29fc@linux.intel.com>
From: Baolu Lu <baolu.lu@linux.intel.com>
In-Reply-To: <be26926a-5843-46c6-a0b9-c51b5d8e29fc@linux.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

On 2024/2/19 15:06, Ethan Zhao wrote:
> On 2/19/2024 2:58 PM, Baolu Lu wrote:
>> On 2024/2/19 14:54, Ethan Zhao wrote:
>>> On 2/15/2024 3:22 PM, Lu Baolu wrote:
>>>> The existing IO page fault handler currently locates the PCI device by
>>>> calling pci_get_domain_bus_and_slot(). This function searches the list
>>>> of all PCI devices until the desired device is found. To improve lookup
>>>> efficiency, a helper function named device_rbtree_find() is introduced
>>>> to search for the device within the rbtree. Replace
>>>> pci_get_domain_bus_and_slot() in the IO page fault handling path.
>>>>
>>>> Co-developed-by: Huang Jiaqing <jiaqing.huang@intel.com>
>>>> Signed-off-by: Huang Jiaqing <jiaqing.huang@intel.com>
>>>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>>>> ---
>>>>   drivers/iommu/intel/iommu.h |  1 +
>>>>   drivers/iommu/intel/iommu.c | 29 +++++++++++++++++++++++++++++
>>>>   drivers/iommu/intel/svm.c   | 14 ++++++--------
>>>>   3 files changed, 36 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
>>>> index 54eeaa8e35a9..f13c228924f8 100644
>>>> --- a/drivers/iommu/intel/iommu.h
>>>> +++ b/drivers/iommu/intel/iommu.h
>>>> @@ -1081,6 +1081,7 @@ void free_pgtable_page(void *vaddr);
>>>>   void iommu_flush_write_buffer(struct intel_iommu *iommu);
>>>>   struct iommu_domain *intel_nested_domain_alloc(struct iommu_domain 
>>>> *parent,
>>>>                              const struct iommu_user_data *user_data);
>>>> +struct device *device_rbtree_find(struct intel_iommu *iommu, u16 rid);
>>>>   #ifdef CONFIG_INTEL_IOMMU_SVM
>>>>   void intel_svm_check(struct intel_iommu *iommu);
>>>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>>>> index 09009d96e553..d92c680bcc96 100644
>>>> --- a/drivers/iommu/intel/iommu.c
>>>> +++ b/drivers/iommu/intel/iommu.c
>>>> @@ -120,6 +120,35 @@ static int device_rid_cmp(struct rb_node *lhs, 
>>>> const struct rb_node *rhs)
>>>>       return device_rid_cmp_key(&key, rhs);
>>>>   }
>>>> +/*
>>>> + * Looks up an IOMMU-probed device using its source ID.
>>>> + *
>>>> + * If the device is found:
>>>> + *  - Increments its reference count.
>>>> + *  - Returns a pointer to the device.
>>>> + *  - The caller must call put_device() after using the pointer.
>>>> + *
>>>> + * If the device is not found, returns NULL.
>>>> + */
>>>> +struct device *device_rbtree_find(struct intel_iommu *iommu, u16 rid)
>>>> +{
>>>> +    struct device_domain_info *info;
>>>> +    struct device *dev = NULL;
>>>> +    struct rb_node *node;
>>>> +    unsigned long flags;
>>>> +
>>>> +    spin_lock_irqsave(&iommu->device_rbtree_lock, flags);
>>>
>>> Though per iommu device rbtree isn't a big tree, given already holds 
>>> spin_lock
>>> why still needs irq off ?
>>
>> I want it to be usable not only in the normal execution flow, but also
>> in the interrupt context, such as for the DMA remapping fault
>> (unrecoverable) reporting path.
> 
> Holding rbtree_lock only should work in interrrupt context, I missed 
> something ?

No. That will possibly cause dead lock.

Best regards,
baolu