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

[Ethan Zhao]

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 ?
But with irq_off, as side effect, telling all the CPUs not to handle interrrupt
that moment.

Thanks,
Ethan

> Best regards,
> baolu