Re: [PATCH 1/3] iommu/amd: Add logic to decode AMD IOMMU event flag

[Suravee Suthikulanit]

On 4/2/2013 9:33 AM, Joerg Roedel wrote:
> Hi Suravee,
>
> On Wed, Mar 27, 2013 at 06:51:23PM -0500, suravee.suthikulpanit@xxxxxxx wrote:
> > From: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> >
> > Add logic to decode AMD IOMMU event flag based on information from AMD IOMMU specification.
> > This should simplify debugging IOMMU errors.  Also, dump DTE information in additional cases.
> The patch in general makes sense to have, but I have a couple of
> comments.
>
> > +static void dump_flags(int flags, int ev_type)
> > +{
> > +	struct _event_log_flags *p = (struct _event_log_flags *) &flags;
> > +	u32 err_type = p->type;
> > +
> > +	pr_err("AMD-Vi: Flags details:\n");
> > +	pr_err("AMD-Vi:    Guest / Nested  : %u\n", p->gn);
> > +	pr_err("AMD-Vi:    No Execute      : %u\n", p->nx);
> > +	pr_err("AMD-Vi:    User-Supervisor : %u\n", p->us);
> > +	pr_err("AMD-Vi:    Interrupt       : %u\n", p->i);
> > +	pr_err("AMD-Vi:    Present         : %u\n", p->pr);
> > +	pr_err("AMD-Vi:    Read / Write    : %u\n", p->rw);
> > +	pr_err("AMD-Vi:    Permission      : %u\n", p->pe);
> > +	pr_err("AMD-Vi:    Reserv bit not zero / Illegal level encoding : %u\n",
> > +		p->rz);
> > +	pr_err("AMD-Vi:    Translation / Transaction : %u\n",
> > +		p->tr);
> > +	pr_err("AMD-Vi:    Type of error   : (0x%x) ", err_type);
> Printing the flags multi-line is much too noisy for IOMMU events. Just
> print a char-shortcut for each flag set on the same line.
I will make the changes and send in for new patch for review.
> > +
> > +	if ((ev_type == EVENT_TYPE_DEV_TAB_ERR)  ||
> > +	    (ev_type == EVENT_TYPE_PAGE_TAB_ERR) ||
> > +	    (ev_type == EVENT_TYPE_CMD_HARD_ERR)) {
> > +		/* Only supports up to 2 bits */
> > +		err_type &= 0x3;
> > +		switch (err_type) {
> > +		case 0:
> > +			pr_err("Reserved\n");
> > +			break;
> > +		case 1:
> > +			pr_err("Master Abort\n");
> > +			break;
> > +		case 2:
> > +			pr_err("Target Abort\n");
> > +			break;
> > +		case 3:
> > +			pr_err("Data Error\n");
> > +			break;
> > +		}
> Why do you create string-arrays for other type-encodings but not for
> this one?
I can do the same way if that's preferred.

Thanks,

Suravee
> > +	} else if (ev_type == EVENT_TYPE_INV_DEV_REQ) {
> > +		if (p->tr == 0) {
> > +			if (err_type < ARRAY_SIZE(_invalid_translation_desc))
> > +				printk("%s\n",
> > +					_invalid_translation_desc[err_type]);
> > +		} else {
> > +			if (err_type < ARRAY_SIZE(_invalid_transaction_desc))
> > +				printk("%s\n",
> > +					_invalid_transaction_desc[err_type]);
> pr_cont instead of printk.
>
>
> 	Joerg


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/