linux-kernel - RE: [PATCH v2 2/7] iommu/vt-d: Multiple descriptors per qi_submit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <AADFC41AFE54684AB9EE6CBC0274A5D19D82066C@SHSMSX104.ccr.corp.intel.com>
Date:   Wed, 15 Apr 2020 08:51:10 +0000
From:   "Tian, Kevin" <kevin.tian@...el.com>
To:     Lu Baolu <baolu.lu@...ux.intel.com>, Joerg Roedel <joro@...tes.org>
CC:     "Raj, Ashok" <ashok.raj@...el.com>,
        "jacob.jun.pan@...ux.intel.com" <jacob.jun.pan@...ux.intel.com>,
        "Liu, Yi L" <yi.l.liu@...el.com>,
        "iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2 2/7] iommu/vt-d: Multiple descriptors per
 qi_submit_sync()

> From: Lu Baolu <baolu.lu@...ux.intel.com>
> Sent: Wednesday, April 15, 2020 4:30 PM
> 
> On 2020/4/15 16:18, Tian, Kevin wrote:
> >> From: Lu Baolu<baolu.lu@...ux.intel.com>
> >> 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@...ux.intel.com>
> >> Signed-off-by: Lu Baolu<baolu.lu@...ux.intel.com>
> >> ---
> >>   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.
> 

Thanks. you are right.