Re: [PATCH 2/2] iommu/vt-d: Use device rbtree in iopf reporting path

[Baolu Lu]

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@xxxxxxxxx>
> > > > Signed-off-by: Huang Jiaqing <jiaqing.huang@xxxxxxxxx>
> > > > Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> > > > ---
> > > >   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