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

[Raj, Ashok]

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@xxxxxxxxxxxxxxx>

Maybe simpler to call it NO_PASID, and just PASID 0xxxxx instead?

with the minor suggestions below

Reviewed-by: Ashok Raj <ashok.raj@xxxxxxxxx>

> ---
>  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?

> +		       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
> 

-- 
Cheers,
Ashok

[Forgiveness is the attribute of the STRONG - Gandhi]