Re: [PATCH 1/1] iommu/vt-d: Tweak the description of a DMA fault

[Lu Baolu <baolu.lu@linux.intel.com>]

On 5/13/21 12:56 AM, Raj, Ashok wrote:
> On Wed, May 12, 2021 at 02:50:12PM +0800, Lu Baolu wrote:
>> The Intel IOMMU driver reports the DMA fault reason in a decimal number
>> while the VT-d specification uses a hexadecimal one. It's inconvenient
>> that users need to covert them everytime before consulting the spec.
>> Let's use hexadecimal number for a DMA fault reason.
>>
>> The fault message uses 0xffffffff as PASID for DMA requests w/o PASID.
>> This is confusing. Tweak this by adding "w/o PASID" explicitly.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> 
> Maybe simpler to call it NO_PASID, and just PASID 0xxxxx instead?

Yeah, it's okay for me.

> 
> with the minor suggestions below
> 
> Reviewed-by: Ashok Raj <ashok.raj@intel.com>

Thanks!

> 
>> ---
>>   drivers/iommu/intel/dmar.c | 22 ++++++++++++++--------
>>   1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
>> index 1757ac1e1623..11e37d2c2af2 100644
>> --- a/drivers/iommu/intel/dmar.c
>> +++ b/drivers/iommu/intel/dmar.c
>> @@ -1911,15 +1911,21 @@ static int dmar_fault_do_one(struct intel_iommu *iommu, int type,
>>   	reason = dmar_get_fault_reason(fault_reason, &fault_type);
>>   
>>   	if (fault_type == INTR_REMAP)
>> -		pr_err("[INTR-REMAP] Request device [%02x:%02x.%d] fault index %llx [fault reason %02d] %s\n",
>> -			source_id >> 8, PCI_SLOT(source_id & 0xFF),
>> -			PCI_FUNC(source_id & 0xFF), addr >> 48,
>> -			fault_reason, reason);
>> -	else
>> -		pr_err("[%s] Request device [%02x:%02x.%d] PASID %x fault addr %llx [fault reason %02d] %s\n",
>> +		pr_err("[INTR-REMAP] Request device [%02x:%02x.%d] fault index %llx [fault reason %02xh] %s\n",
>> +		       source_id >> 8, PCI_SLOT(source_id & 0xFF),
>> +		       PCI_FUNC(source_id & 0xFF), addr >> 48,
>> +		       fault_reason, reason);
>> +	else if (pasid == INVALID_IOASID)
>> +		pr_err("[%s w/o PASID] Request device [%02x:%02x.%d] fault addr %llx [fault reason %02xh] %s\n",
>>   		       type ? "DMA Read" : "DMA Write",
>>   		       source_id >> 8, PCI_SLOT(source_id & 0xFF),
>> -		       PCI_FUNC(source_id & 0xFF), pasid, addr,
>> +		       PCI_FUNC(source_id & 0xFF), addr,
>> +		       fault_reason, reason);
>> +	else
>> +		pr_err("[%s w/ PASID %x] Request device [%02x:%02x.%d] fault addr %llx [fault reason %02xh] %s\n",
> 
> Can you always lead hex values with 0x?

Yes.

> 
>> +		       type ? "DMA Read" : "DMA Write", pasid,
>> +		       source_id >> 8, PCI_SLOT(source_id & 0xFF),
>> +		       PCI_FUNC(source_id & 0xFF), addr,
>>   		       fault_reason, reason);
>>   	return 0;
>>   }
>> @@ -1987,7 +1993,7 @@ irqreturn_t dmar_fault(int irq, void *dev_id)
>>   		if (!ratelimited)
>>   			/* Using pasid -1 if pasid is not present */
>>   			dmar_fault_do_one(iommu, type, fault_reason,
>> -					  pasid_present ? pasid : -1,
>> +					  pasid_present ? pasid : INVALID_IOASID,
>>   					  source_id, guest_addr);
>>   
>>   		fault_index++;
>> -- 
>> 2.25.1
>>
> 

Best regards,
baolu