From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suravee Suthikulanit Subject: Re: [PATCH 1/3] iommu/amd: Add logic to decode AMD IOMMU event flag Date: Tue, 2 Apr 2013 09:39:26 -0500 Message-ID: <515AED9E.80204@amd.com> References: <1364428283-2548-1-git-send-email-suravee.suthikulpanit@amd.com> <20130402143335.GB15687@8bytes.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20130402143335.GB15687-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Joerg Roedel Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: iommu@lists.linux-foundation.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 >> >> 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 > > >