Re: [PATCH v2 2/7] iommu/vt-d: Multiple descriptors per qi_submit_sync()

[Lu Baolu]

On 2020/4/15 16:18, Tian, Kevin wrote:
> > From: Lu Baolu<baolu.lu@xxxxxxxxxxxxxxx>
> > Sent: Wednesday, April 15, 2020 1:26 PM
> >
> > Extend qi_submit_sync() function to support multiple descriptors.
> >
> > Signed-off-by: Jacob Pan<jacob.jun.pan@xxxxxxxxxxxxxxx>
> > Signed-off-by: Lu Baolu<baolu.lu@xxxxxxxxxxxxxxx>
> > ---
> >   drivers/iommu/dmar.c        | 39 +++++++++++++++++++++++--------------
> >   include/linux/intel-iommu.h |  1 +
> >   2 files changed, 25 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
> > index bb42177e2369..61d049e91f84 100644
> > --- a/drivers/iommu/dmar.c
> > +++ b/drivers/iommu/dmar.c
> > @@ -1157,12 +1157,11 @@ static inline void reclaim_free_desc(struct
> > q_inval *qi)
> >   	}
> >   }
> >
> > -static int qi_check_fault(struct intel_iommu *iommu, int index)
> > +static int qi_check_fault(struct intel_iommu *iommu, int index, int
> > wait_index)
> >   {
> >   	u32 fault;
> >   	int head, tail;
> >   	struct q_inval *qi = iommu->qi;
> > -	int wait_index = (index + 1) % QI_LENGTH;
> >   	int shift = qi_shift(iommu);
> >
> >   	if (qi->desc_status[wait_index] == QI_ABORT)
> > @@ -1234,12 +1233,12 @@ static int qi_check_fault(struct intel_iommu
> > *iommu, int index)
> >   int qi_submit_sync(struct intel_iommu *iommu, struct qi_desc *desc,
> >   		   unsigned int count, unsigned long options)
> >   {
> > -	int rc;
> >   	struct q_inval *qi = iommu->qi;
> > -	int offset, shift, length;
> >   	struct qi_desc wait_desc;
> >   	int wait_index, index;
> >   	unsigned long flags;
> > +	int offset, shift;
> > +	int rc, i;
> >
> >   	if (!qi)
> >   		return 0;
> > @@ -1248,32 +1247,41 @@ int qi_submit_sync(struct intel_iommu *iommu,
> > struct qi_desc *desc,
> >   	rc = 0;
> >
> >   	raw_spin_lock_irqsave(&qi->q_lock, flags);
> > -	while (qi->free_cnt < 3) {
> > +	/*
> > +	 * Check if we have enough empty slots in the queue to submit,
> > +	 * the calculation is based on:
> > +	 * # of desc + 1 wait desc + 1 space between head and tail
> > +	 */
> > +	while (qi->free_cnt < count + 2) {
> >   		raw_spin_unlock_irqrestore(&qi->q_lock, flags);
> >   		cpu_relax();
> >   		raw_spin_lock_irqsave(&qi->q_lock, flags);
> >   	}
> >
> >   	index = qi->free_head;
> > -	wait_index = (index + 1) % QI_LENGTH;
> > +	wait_index = (index + count) % QI_LENGTH;
> >   	shift = qi_shift(iommu);
> > -	length = 1 << shift;
> >
> > -	qi->desc_status[index] = qi->desc_status[wait_index] = QI_IN_USE;
> > +	for (i = 0; i < count; i++) {
> > +		offset = ((index + i) % QI_LENGTH) << shift;
> > +		memcpy(qi->desc + offset, &desc[i], 1 << shift);
> > +		qi->desc_status[(index + i) % QI_LENGTH] = QI_IN_USE;
> > +	}
> what about doing one memcpy and leave the loop only for updating
> qi status?

One memcpy might cross the table boundary.

Best regards,
baolu