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

[Lu Baolu]

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

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