linux-kernel - Re: [PATCH v4 04/12] iommu/vt-d: Add 256-bit invalidation descriptor support

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 for Android: free password hash cracker in your pocket

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

Message-ID: <a6e43ddd-0889-4a4a-b908-993db28c6d87@linux.intel.com>
Date:   Thu, 8 Nov 2018 10:16:51 +0800
From:   Lu Baolu <baolu.lu@...ux.intel.com>
To:     "Liu, Yi L" <yi.l.liu@...el.com>, Joerg Roedel <joro@...tes.org>,
        David Woodhouse <dwmw2@...radead.org>
Cc:     baolu.lu@...ux.intel.com, "Raj, Ashok" <ashok.raj@...el.com>,
        "Kumar, Sanjay K" <sanjay.k.kumar@...el.com>,
        "Pan, Jacob jun" <jacob.jun.pan@...el.com>,
        "Tian, Kevin" <kevin.tian@...el.com>,
        "Sun, Yi Y" <yi.y.sun@...el.com>,
        "peterx@...hat.com" <peterx@...hat.com>,
        Jean-Philippe Brucker <jean-philippe.brucker@....com>,
        "iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Jacob Pan <jacob.jun.pan@...ux.intel.com>
Subject: Re: [PATCH v4 04/12] iommu/vt-d: Add 256-bit invalidation descriptor
 support

Hi Yi,

On 11/7/18 2:07 PM, Liu, Yi L wrote:
> Hi Baolu,
> 
>> From: Lu Baolu [mailto:baolu.lu@...ux.intel.com]
>> Sent: Monday, November 5, 2018 1:32 PM
> 
> [...]
> 
>> ---
>>   drivers/iommu/dmar.c                | 83 +++++++++++++++++++----------
>>   drivers/iommu/intel-svm.c           | 76 ++++++++++++++++----------
>>   drivers/iommu/intel_irq_remapping.c |  6 ++-
>>   include/linux/intel-iommu.h         |  9 +++-
>>   4 files changed, 115 insertions(+), 59 deletions(-)
>>
>> diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
>> index d9c748b6f9e4..ec10427b98ac 100644
>> --- a/drivers/iommu/dmar.c
>> +++ b/drivers/iommu/dmar.c
>> @@ -1160,6 +1160,7 @@ static int qi_check_fault(struct intel_iommu *iommu, int
>> index)
>>   	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)
>>   		return -EAGAIN;
>> @@ -1173,13 +1174,15 @@ static int qi_check_fault(struct intel_iommu *iommu,
>> int index)
>>   	 */
>>   	if (fault & DMA_FSTS_IQE) {
>>   		head = readl(iommu->reg + DMAR_IQH_REG);
>> -		if ((head >> DMAR_IQ_SHIFT) == index) {
>> +		if ((head >> shift) == index) {
>> +			struct qi_desc *desc = qi->desc + head;
>> +
>>   			pr_err("VT-d detected invalid descriptor: "
>>   				"low=%llx, high=%llx\n",
>> -				(unsigned long long)qi->desc[index].low,
>> -				(unsigned long long)qi->desc[index].high);
>> -			memcpy(&qi->desc[index], &qi->desc[wait_index],
>> -					sizeof(struct qi_desc));
>> +				(unsigned long long)desc->qw0,
>> +				(unsigned long long)desc->qw1);
> 
> Still missing qw2 and qw3. May make the print differ based on if smts is configed.

qw2 and qw3 are reserved from software point of view. We don't need to
print it for information.

> 
>> +			memcpy(desc, qi->desc + (wait_index << shift),
> 
> Would "memcpy(desc, (unsigned long long) (qi->desc +  (wait_index << shift)," be
> more safe?

Can that be compiled? memcpy() requires a "const void *" for the second
parameter. By the way, why it's safer with this casting?

> 
>> +			       1 << shift);
>>   			writel(DMA_FSTS_IQE, iommu->reg + DMAR_FSTS_REG);
>>   			return -EINVAL;
>>   		}
>> @@ -1191,10 +1194,10 @@ static int qi_check_fault(struct intel_iommu *iommu,
>> int index)
>>   	 */
>>   	if (fault & DMA_FSTS_ITE) {
>>   		head = readl(iommu->reg + DMAR_IQH_REG);
>> -		head = ((head >> DMAR_IQ_SHIFT) - 1 + QI_LENGTH) % QI_LENGTH;
>> +		head = ((head >> shift) - 1 + QI_LENGTH) % QI_LENGTH;
>>   		head |= 1;
>>   		tail = readl(iommu->reg + DMAR_IQT_REG);
>> -		tail = ((tail >> DMAR_IQ_SHIFT) - 1 + QI_LENGTH) % QI_LENGTH;
>> +		tail = ((tail >> shift) - 1 + QI_LENGTH) % QI_LENGTH;
>>
>>   		writel(DMA_FSTS_ITE, iommu->reg + DMAR_FSTS_REG);
>>
>> @@ -1222,15 +1225,14 @@ int qi_submit_sync(struct qi_desc *desc, struct
>> intel_iommu *iommu)
>>   {
>>   	int rc;
>>   	struct q_inval *qi = iommu->qi;
>> -	struct qi_desc *hw, wait_desc;
>> +	int offset, shift, length;
>> +	struct qi_desc wait_desc;
>>   	int wait_index, index;
>>   	unsigned long flags;
>>
>>   	if (!qi)
>>   		return 0;
>>
>> -	hw = qi->desc;
>> -
>>   restart:
>>   	rc = 0;
>>
>> @@ -1243,16 +1245,21 @@ int qi_submit_sync(struct qi_desc *desc, struct
>> intel_iommu *iommu)
>>
>>   	index = qi->free_head;
>>   	wait_index = (index + 1) % QI_LENGTH;
>> +	shift = qi_shift(iommu);
>> +	length = 1 << shift;
>>
>>   	qi->desc_status[index] = qi->desc_status[wait_index] = QI_IN_USE;
>>
>> -	hw[index] = *desc;
>> -
>> -	wait_desc.low = QI_IWD_STATUS_DATA(QI_DONE) |
>> +	offset = index << shift;
>> +	memcpy(qi->desc + offset, desc, length);
>> +	wait_desc.qw0 = QI_IWD_STATUS_DATA(QI_DONE) |
>>   			QI_IWD_STATUS_WRITE | QI_IWD_TYPE;
>> -	wait_desc.high = virt_to_phys(&qi->desc_status[wait_index]);
>> +	wait_desc.qw1 = virt_to_phys(&qi->desc_status[wait_index]);
>> +	wait_desc.qw2 = 0;
>> +	wait_desc.qw3 = 0;
>>
>> -	hw[wait_index] = wait_desc;
>> +	offset = wait_index << shift;
>> +	memcpy(qi->desc + offset, &wait_desc, length);
> 
> same question with above one.
> 

Ditto.

Best regards,
Lu Baolu