Hi Suravee,
On Wed, Mar 27, 2013 at 06:51:23PM -0500, suravee.suthikulpa...@amd.com wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpa...@amd.com>
>
> 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.
> +
> + 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?
> + } 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
