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

[Suravee Suthikulanit <suravee.suthikulpanit-5C7GfCeVMHo@public.gmane.org>]

On 4/2/2013 9:33 AM, Joerg Roedel wrote:
> Hi Suravee,
>
> On Wed, Mar 27, 2013 at 06:51:23PM -0500, suravee.suthikulpanit-5C7GfCeVMHo@public.gmane.org wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit-5C7GfCeVMHo@public.gmane.org>
>>
>> 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
>
>
>