RE: [PATCH] iommu/vt-d: Fix potential soft lockup due to reclaim

[Kumar, Sanjay K]

Sorry, just catching up with emails.
I noticed another thing. 
In qi_submit_sync() input parameters, when the count is 0, the expectation should be that desc should be NULL right?
In that case, the below code will cause a problem.

type = desc->qw0 & GENMASK_ULL(3, 0);

The above line requires caller (when calling with count = 0) to pass a fake non-NULL desc pointer. Should we fix this as well? Alternatively, we can fix it whenever we create the use case of caller calling with count=0.

Thanks,
Sanjay

-----Original Message-----
From: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx> 
Sent: Thursday, July 25, 2024 7:42 PM
To: Tian, Kevin <kevin.tian@xxxxxxxxx>
Cc: iommu@xxxxxxxxxxxxxxx; LKML <linux-kernel@xxxxxxxxxxxxxxx>; Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>; Liu, Yi L <yi.l.liu@xxxxxxxxx>; Zhang, Tina <tina.zhang@xxxxxxxxx>; Kumar, Sanjay K <sanjay.k.kumar@xxxxxxxxx>; jacob.jun.pan@xxxxxxxxxxxxxxx
Subject: Re: [PATCH] iommu/vt-d: Fix potential soft lockup due to reclaim


On Fri, 26 Jul 2024 00:18:13 +0000, "Tian, Kevin" <kevin.tian@xxxxxxxxx>
wrote:

> > From: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
> > Sent: Friday, July 26, 2024 5:11 AM
> > > > > > @@ -1463,8 +1462,14 @@ int qi_submit_sync(struct intel_iommu
> > > > *iommu,
> > > > > > struct qi_desc *desc,
> > > > > >  		raw_spin_lock(&qi->q_lock);
> > > > > >  	}
> > > > > >
> > > > > > -	for (i = 0; i < count; i++)
> > > > > > -		qi->desc_status[(index + i) % QI_LENGTH] =
> > > > > > QI_DONE;
> > > > > > +	/*
> > > > > > +	 * The reclaim code can free descriptors from multiple
> > > > > > submissions
> > > > > > +	 * starting from the tail of the queue. When count ==
> > > > > > 0, the
> > > > > > +	 * status of the standalone wait descriptor at the
> > > > > > tail of the queue
> > > > > > +	 * must be set to QI_TO_BE_FREED to allow the reclaim
> > > > > > code to proceed.
> > > > > > +	 */
> > > > > > +	for (i = 0; i <= count; i++)
> > > > > > +		qi->desc_status[(index + i) % QI_LENGTH] =
> > > > > > QI_TO_BE_FREED;
> > > > >
> > > > > We don't really need a new flag. Just set them to QI_FREE and 
> > > > > then reclaim QI_FREE slots until hitting qi->head in 
> > > > > reclaim_free_desc().
> > > > We do need to have a separate state for descriptors pending to 
> > > > be freed. Otherwise, reclaim code will advance pass the intended range.
> > > >  
> > >
> > > The commit msg said that QI_DONE is currently used for conflicting 
> > > purpose.
> > >
> > > Using QI_FREE keeps it only for signaling that a wait desc is 
> > > completed.
> > >
> > > The key is that reclaim() should not change a desc's state before 
> > > it's consumed by the owner. Instead we always let the owner to 
> > > change the state and reclaim() only does scan and adjust the 
> > > tracking fields then such race condition disappears.
> > >
> > > In this example T2's slots are changed to QI_FREE by T2 after it 
> > > completes all the checks. Only at this point those slots can be 
> > > reclaimed.
> > 
> > The problem is that without the TO_BE_FREED state, the reclaim code 
> > would have no way of knowing which ones are to be reclaimed and 
> > which ones are currently free. Therefore, it cannot track free_cnt.
> > 
> > The current reclaim code is not aware of owners nor how many to reclaim.
> > 
> > If I make the following changes and run, free_cnt will keep going up 
> > and system cannot boot. Perhaps you meant another way?
> > 
> > --- a/drivers/iommu/intel/dmar.c
> > +++ b/drivers/iommu/intel/dmar.c
> > @@ -1204,8 +1204,7 @@ static void free_iommu(struct intel_iommu
> > *iommu)
> >   */
> >  static inline void reclaim_free_desc(struct q_inval *qi)  {
> > -       while (qi->desc_status[qi->free_tail] == QI_TO_BE_FREED) {
> > -               qi->desc_status[qi->free_tail] = QI_FREE;
> > +       while (qi->desc_status[qi->free_tail] == QI_FREE) {
> >                 qi->free_tail = (qi->free_tail + 1) % QI_LENGTH;
> >                 qi->free_cnt++;
> 
> Here miss a check to prevent reclaiming unused slots:
> 
> 		if (qi->free_tail == qi->free_head)
> 			break;
> 
> In the example flow reclaim_free_desc() in T1 will only reclaim slots 
> used by T1 as slots of T2 are either QI_IN_USE or QI_DONE. T2 slots 
> will be reclaimed when T2 calls reclaim_free_desc() after setting them 
> to QI_FREE, and reclaim will stop at qi->free_head.
> 
> If for some reason T2 completes earlier than T1. reclaim_free_desc() 
> in T2 does nothing as the first slot qi->free_tail belongs to T1 still 
> IN_USE. T2's slots will then wait until reclaim is triggered by T1 later.
> 
This makes sense. Unlike the original code, we now only allow freeing the submitter's own descriptors.

> >         }
> > @@ -1466,10 +1465,10 @@ int qi_submit_sync(struct intel_iommu 
> > *iommu, struct qi_desc *desc,
> >          * The reclaim code can free descriptors from multiple 
> > submissions
> >          * starting from the tail of the queue. When count == 0, the
> >          * status of the standalone wait descriptor at the tail of 
> > the queue
> > -        * must be set to QI_TO_BE_FREED to allow the reclaim code to
> > proceed.
> > +        * must be set to QI_FREE to allow the reclaim code to proceed.
> >          */
> >         for (i = 0; i <= count; i++)
> > -               qi->desc_status[(index + i) % QI_LENGTH] =
> > QI_TO_BE_FREED;
> > +               qi->desc_status[(index + i) % QI_LENGTH] = QI_FREE;
> > 
> >         reclaim_free_desc(qi);
> >         raw_spin_unlock_irqrestore(&qi->q_lock, flags); diff --git 
> > a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h index 
> > 1ab39f9145f2..eaf015b4353b 100644
> > --- a/drivers/iommu/intel/iommu.h
> > +++ b/drivers/iommu/intel/iommu.h
> > @@ -382,8 +382,7 @@ enum {
> >         QI_FREE,
> >         QI_IN_USE,
> >         QI_DONE,
> > -       QI_ABORT,
> > -       QI_TO_BE_FREED
> > +       QI_ABORT
> >  };
> > 
> > Thanks,
> > 
> > Jacob
> 


Thanks,

Jacob