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

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

[suravee.suthikulpanit]

From: Suravee Suthikulpanit <suravee.suthikulpanit@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.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 drivers/iommu/amd_iommu.c |  161 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 134 insertions(+), 27 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 98f555d..477cfbb 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -592,7 +592,6 @@ static void amd_iommu_stats_init(void)
 	amd_iommu_stats_add(&invalidate_iotlb_all);
 	amd_iommu_stats_add(&pri_requests);
 }
-
 #endif
 
 /****************************************************************************
@@ -601,6 +600,99 @@ static void amd_iommu_stats_init(void)
  *
  ****************************************************************************/
 
+struct _event_log_flags {
+	u32	gn:1, /* 16 */
+		nx:1, /* 17 */
+		us:1, /* 18 */
+		 i:1, /* 19 */
+		pr:1, /* 20 */
+		rw:1, /* 21 */
+		pe:1, /* 22 */
+		rz:1, /* 23 */
+		tr:1, /* 24 */
+		type:3, /* [27:25] */
+		_reserved_:20; /* Reserved */
+};
+
+static const char * const _invalid_transaction_desc[] = {
+	/* 000 */"Read request or non-posted write in the interrupt "
+		 "addres range",
+	/* 001 */"Pretranslated transaction received from an I/O device "
+		 "that has I=0 or V=0 in DTE",
+	/* 010 */"Port I/O space transaction received from an I/O device "
+		 "that has IoCtl=00b in DTE",
+	/* 011 */"Posted write to invalid address range",
+	/* 100 */"Invalid read request or non-posted write",
+	/* 101 */"Posted write to the interrupt/EOI range from an I/O "
+		 "device that has IntCtl=00b in DTE",
+	/* 110 */"Posted write to a reserved interrupt address range",
+	/* 111 */"Invalid transaction to the system management address range",
+};
+
+static const char * const _invalid_translation_desc[] = {
+	/* 000 */"Translation request received from an I/O device that has "
+		 "I=0, or has V=0, or has V=1 and TV=0 in DTE",
+	/* 001 */"Translation request in invalid address range",
+	/* 010 */"Invalid translation request",
+	/* 011 */"Reserved",
+	/* 100 */"Reserved",
+	/* 101 */"Reserved",
+	/* 110 */"Reserved",
+	/* 111 */"Reserved",
+};
+
+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);
+
+	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;
+		}
+	} 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_err("AMD-Vi:    (Note: Please refer to AMD IOMMU specification for details.)\n");
+}
+
 static void dump_dte_entry(u16 devid)
 {
 	int i;
@@ -619,31 +711,10 @@ static void dump_command(unsigned long phys_addr)
 		pr_err("AMD-Vi: CMD[%d]: %08x\n", i, cmd->data[i]);
 }
 
-static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
+static void iommu_print_event(int type, int devid, int domid,
+			      int flags, u64 address)
 {
-	int type, devid, domid, flags;
-	volatile u32 *event = __evt;
-	int count = 0;
-	u64 address;
-
-retry:
-	type    = (event[1] >> EVENT_TYPE_SHIFT)  & EVENT_TYPE_MASK;
-	devid   = (event[0] >> EVENT_DEVID_SHIFT) & EVENT_DEVID_MASK;
-	domid   = (event[1] >> EVENT_DOMID_SHIFT) & EVENT_DOMID_MASK;
-	flags   = (event[1] >> EVENT_FLAGS_SHIFT) & EVENT_FLAGS_MASK;
-	address = (u64)(((u64)event[3]) << 32) | event[2];
-
-	if (type == 0) {
-		/* Did we hit the erratum? */
-		if (++count == LOOP_TIMEOUT) {
-			pr_err("AMD-Vi: No event written to event log\n");
-			return;
-		}
-		udelay(1);
-		goto retry;
-	}
-
-	printk(KERN_ERR "AMD-Vi: Event logged [");
+	pr_err("AMD-Vi: Event logged [");
 
 	switch (type) {
 	case EVENT_TYPE_ILL_DEV:
@@ -651,6 +722,7 @@ retry:
 		       "address=0x%016llx flags=0x%04x]\n",
 		       PCI_BUS(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 		       address, flags);
+		dump_flags(flags, type);
 		dump_dte_entry(devid);
 		break;
 	case EVENT_TYPE_IO_FAULT:
@@ -658,18 +730,22 @@ retry:
 		       "domain=0x%04x address=0x%016llx flags=0x%04x]\n",
 		       PCI_BUS(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 		       domid, address, flags);
+		dump_flags(flags, type);
+		dump_dte_entry(devid);
 		break;
 	case EVENT_TYPE_DEV_TAB_ERR:
 		printk("DEV_TAB_HARDWARE_ERROR device=%02x:%02x.%x "
 		       "address=0x%016llx flags=0x%04x]\n",
 		       PCI_BUS(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 		       address, flags);
+		dump_flags(flags, type);
 		break;
 	case EVENT_TYPE_PAGE_TAB_ERR:
 		printk("PAGE_TAB_HARDWARE_ERROR device=%02x:%02x.%x "
 		       "domain=0x%04x address=0x%016llx flags=0x%04x]\n",
 		       PCI_BUS(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 		       domid, address, flags);
+		dump_flags(flags, type);
 		break;
 	case EVENT_TYPE_ILL_CMD:
 		printk("ILLEGAL_COMMAND_ERROR address=0x%016llx]\n", address);
@@ -678,6 +754,7 @@ retry:
 	case EVENT_TYPE_CMD_HARD_ERR:
 		printk("COMMAND_HARDWARE_ERROR address=0x%016llx "
 		       "flags=0x%04x]\n", address, flags);
+		dump_flags(flags, type);
 		break;
 	case EVENT_TYPE_IOTLB_INV_TO:
 		printk("IOTLB_INV_TIMEOUT device=%02x:%02x.%x "
@@ -690,11 +767,40 @@ retry:
 		       "address=0x%016llx flags=0x%04x]\n",
 		       PCI_BUS(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 		       address, flags);
+		dump_flags(flags, type);
+		dump_dte_entry(devid);
 		break;
 	default:
-		printk(KERN_ERR "UNKNOWN type=0x%02x]\n", type);
+		printk("UNKNOWN type=0x%02x]\n", type);
+	}
+}
+
+static void iommu_handle_event(struct amd_iommu *iommu, void *__evt)
+{
+	int type, devid, domid, flags;
+	u32 *event = __evt;
+	int count = 0;
+	u64 address;
+
+retry:
+	type    = (event[1] >> EVENT_TYPE_SHIFT)  & EVENT_TYPE_MASK;
+	devid   = (event[0] >> EVENT_DEVID_SHIFT) & EVENT_DEVID_MASK;
+	domid   = (event[1] >> EVENT_DOMID_SHIFT) & EVENT_DOMID_MASK;
+	flags   = (event[1] >> EVENT_FLAGS_SHIFT) & EVENT_FLAGS_MASK;
+	address = (u64)(((u64)event[3]) << 32) | event[2];
+
+	if (type == 0) {
+		/* Did we hit the erratum? */
+		if (++count == LOOP_TIMEOUT) {
+			pr_err("AMD-Vi: No event written to event log\n");
+			return;
+		}
+		udelay(1);
+		goto retry;
 	}
 
+	iommu_print_event(type, devid, domid, flags, address);
+
 	memset(__evt, 0, 4 * sizeof(u32));
 }
 
@@ -709,7 +815,7 @@ static void iommu_poll_events(struct amd_iommu *iommu)
 	tail = readl(iommu->mmio_base + MMIO_EVT_TAIL_OFFSET);
 
 	while (head != tail) {
-		iommu_print_event(iommu, iommu->evt_buf + head);
+		iommu_handle_event(iommu, iommu->evt_buf + head);
 		head = (head + EVENT_ENTRY_SIZE) % iommu->evt_buf_size;
 	}
 
@@ -3270,6 +3376,7 @@ static int __init alloc_passthrough_domain(void)
 
 	return 0;
 }
+
 static int amd_iommu_domain_init(struct iommu_domain *dom)
 {
 	struct protection_domain *domain;
-- 
1.7.10.4

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

[Suravee Suthikulanit]

Hi Joerg,

Do you have any comments or feedback about this patch set?

Thank you,

Suravee



On 3/27/2013 6:51 PM, 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.
>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit-5C7GfCeVMHo@public.gmane.org>
> ---
>   drivers/iommu/amd_iommu.c |  161 +++++++++++++++++++++++++++++++++++++--------
>   1 file changed, 134 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 98f555d..477cfbb 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -592,7 +592,6 @@ static void amd_iommu_stats_init(void)
>   	amd_iommu_stats_add(&invalidate_iotlb_all);
>   	amd_iommu_stats_add(&pri_requests);
>   }
> -
>   #endif
>   
>   /****************************************************************************
> @@ -601,6 +600,99 @@ static void amd_iommu_stats_init(void)
>    *
>    ****************************************************************************/
>   
> +struct _event_log_flags {
> +	u32	gn:1, /* 16 */
> +		nx:1, /* 17 */
> +		us:1, /* 18 */
> +		 i:1, /* 19 */
> +		pr:1, /* 20 */
> +		rw:1, /* 21 */
> +		pe:1, /* 22 */
> +		rz:1, /* 23 */
> +		tr:1, /* 24 */
> +		type:3, /* [27:25] */
> +		_reserved_:20; /* Reserved */
> +};
> +
> +static const char * const _invalid_transaction_desc[] = {
> +	/* 000 */"Read request or non-posted write in the interrupt "
> +		 "addres range",
> +	/* 001 */"Pretranslated transaction received from an I/O device "
> +		 "that has I=0 or V=0 in DTE",
> +	/* 010 */"Port I/O space transaction received from an I/O device "
> +		 "that has IoCtl=00b in DTE",
> +	/* 011 */"Posted write to invalid address range",
> +	/* 100 */"Invalid read request or non-posted write",
> +	/* 101 */"Posted write to the interrupt/EOI range from an I/O "
> +		 "device that has IntCtl=00b in DTE",
> +	/* 110 */"Posted write to a reserved interrupt address range",
> +	/* 111 */"Invalid transaction to the system management address range",
> +};
> +
> +static const char * const _invalid_translation_desc[] = {
> +	/* 000 */"Translation request received from an I/O device that has "
> +		 "I=0, or has V=0, or has V=1 and TV=0 in DTE",
> +	/* 001 */"Translation request in invalid address range",
> +	/* 010 */"Invalid translation request",
> +	/* 011 */"Reserved",
> +	/* 100 */"Reserved",
> +	/* 101 */"Reserved",
> +	/* 110 */"Reserved",
> +	/* 111 */"Reserved",
> +};
> +
> +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);
> +
> +	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;
> +		}
> +	} 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_err("AMD-Vi:    (Note: Please refer to AMD IOMMU specification for details.)\n");
> +}
> +
>   static void dump_dte_entry(u16 devid)
>   {
>   	int i;
> @@ -619,31 +711,10 @@ static void dump_command(unsigned long phys_addr)
>   		pr_err("AMD-Vi: CMD[%d]: %08x\n", i, cmd->data[i]);
>   }
>   
> -static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
> +static void iommu_print_event(int type, int devid, int domid,
> +			      int flags, u64 address)
>   {
> -	int type, devid, domid, flags;
> -	volatile u32 *event = __evt;
> -	int count = 0;
> -	u64 address;
> -
> -retry:
> -	type    = (event[1] >> EVENT_TYPE_SHIFT)  & EVENT_TYPE_MASK;
> -	devid   = (event[0] >> EVENT_DEVID_SHIFT) & EVENT_DEVID_MASK;
> -	domid   = (event[1] >> EVENT_DOMID_SHIFT) & EVENT_DOMID_MASK;
> -	flags   = (event[1] >> EVENT_FLAGS_SHIFT) & EVENT_FLAGS_MASK;
> -	address = (u64)(((u64)event[3]) << 32) | event[2];
> -
> -	if (type == 0) {
> -		/* Did we hit the erratum? */
> -		if (++count == LOOP_TIMEOUT) {
> -			pr_err("AMD-Vi: No event written to event log\n");
> -			return;
> -		}
> -		udelay(1);
> -		goto retry;
> -	}
> -
> -	printk(KERN_ERR "AMD-Vi: Event logged [");
> +	pr_err("AMD-Vi: Event logged [");
>   
>   	switch (type) {
>   	case EVENT_TYPE_ILL_DEV:
> @@ -651,6 +722,7 @@ retry:
>   		       "address=0x%016llx flags=0x%04x]\n",
>   		       PCI_BUS(devid), PCI_SLOT(devid), PCI_FUNC(devid),
>   		       address, flags);
> +		dump_flags(flags, type);
>   		dump_dte_entry(devid);
>   		break;
>   	case EVENT_TYPE_IO_FAULT:
> @@ -658,18 +730,22 @@ retry:
>   		       "domain=0x%04x address=0x%016llx flags=0x%04x]\n",
>   		       PCI_BUS(devid), PCI_SLOT(devid), PCI_FUNC(devid),
>   		       domid, address, flags);
> +		dump_flags(flags, type);
> +		dump_dte_entry(devid);
>   		break;
>   	case EVENT_TYPE_DEV_TAB_ERR:
>   		printk("DEV_TAB_HARDWARE_ERROR device=%02x:%02x.%x "
>   		       "address=0x%016llx flags=0x%04x]\n",
>   		       PCI_BUS(devid), PCI_SLOT(devid), PCI_FUNC(devid),
>   		       address, flags);
> +		dump_flags(flags, type);
>   		break;
>   	case EVENT_TYPE_PAGE_TAB_ERR:
>   		printk("PAGE_TAB_HARDWARE_ERROR device=%02x:%02x.%x "
>   		       "domain=0x%04x address=0x%016llx flags=0x%04x]\n",
>   		       PCI_BUS(devid), PCI_SLOT(devid), PCI_FUNC(devid),
>   		       domid, address, flags);
> +		dump_flags(flags, type);
>   		break;
>   	case EVENT_TYPE_ILL_CMD:
>   		printk("ILLEGAL_COMMAND_ERROR address=0x%016llx]\n", address);
> @@ -678,6 +754,7 @@ retry:
>   	case EVENT_TYPE_CMD_HARD_ERR:
>   		printk("COMMAND_HARDWARE_ERROR address=0x%016llx "
>   		       "flags=0x%04x]\n", address, flags);
> +		dump_flags(flags, type);
>   		break;
>   	case EVENT_TYPE_IOTLB_INV_TO:
>   		printk("IOTLB_INV_TIMEOUT device=%02x:%02x.%x "
> @@ -690,11 +767,40 @@ retry:
>   		       "address=0x%016llx flags=0x%04x]\n",
>   		       PCI_BUS(devid), PCI_SLOT(devid), PCI_FUNC(devid),
>   		       address, flags);
> +		dump_flags(flags, type);
> +		dump_dte_entry(devid);
>   		break;
>   	default:
> -		printk(KERN_ERR "UNKNOWN type=0x%02x]\n", type);
> +		printk("UNKNOWN type=0x%02x]\n", type);
> +	}
> +}
> +
> +static void iommu_handle_event(struct amd_iommu *iommu, void *__evt)
> +{
> +	int type, devid, domid, flags;
> +	u32 *event = __evt;
> +	int count = 0;
> +	u64 address;
> +
> +retry:
> +	type    = (event[1] >> EVENT_TYPE_SHIFT)  & EVENT_TYPE_MASK;
> +	devid   = (event[0] >> EVENT_DEVID_SHIFT) & EVENT_DEVID_MASK;
> +	domid   = (event[1] >> EVENT_DOMID_SHIFT) & EVENT_DOMID_MASK;
> +	flags   = (event[1] >> EVENT_FLAGS_SHIFT) & EVENT_FLAGS_MASK;
> +	address = (u64)(((u64)event[3]) << 32) | event[2];
> +
> +	if (type == 0) {
> +		/* Did we hit the erratum? */
> +		if (++count == LOOP_TIMEOUT) {
> +			pr_err("AMD-Vi: No event written to event log\n");
> +			return;
> +		}
> +		udelay(1);
> +		goto retry;
>   	}
>   
> +	iommu_print_event(type, devid, domid, flags, address);
> +
>   	memset(__evt, 0, 4 * sizeof(u32));
>   }
>   
> @@ -709,7 +815,7 @@ static void iommu_poll_events(struct amd_iommu *iommu)
>   	tail = readl(iommu->mmio_base + MMIO_EVT_TAIL_OFFSET);
>   
>   	while (head != tail) {
> -		iommu_print_event(iommu, iommu->evt_buf + head);
> +		iommu_handle_event(iommu, iommu->evt_buf + head);
>   		head = (head + EVENT_ENTRY_SIZE) % iommu->evt_buf_size;
>   	}
>   
> @@ -3270,6 +3376,7 @@ static int __init alloc_passthrough_domain(void)
>   
>   	return 0;
>   }
> +
>   static int amd_iommu_domain_init(struct iommu_domain *dom)
>   {
>   	struct protection_domain *domain;

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

[Joerg Roedel]

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.

> +
> +	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

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-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
>
>
>

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

[Borislav Petkov]

On Tue, Apr 02, 2013 at 04:33:36PM +0200, 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.

While you guys are at it, can someone fix this too pls (ASUS board with
a PD on it).

[    0.220342] [Firmware Bug]: AMD-Vi: IOAPIC[9] not in IVRS table
[    0.220398] [Firmware Bug]: AMD-Vi: IOAPIC[10] not in IVRS table
[    0.220451] [Firmware Bug]: AMD-Vi: No southbridge IOAPIC found in IVRS table
[    0.220506] AMD-Vi: Disabling interrupt remapping due to BIOS Bug(s)

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

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

[Joerg Roedel]

On Tue, Apr 02, 2013 at 04:40:37PM +0200, Borislav Petkov wrote:
> While you guys are at it, can someone fix this too pls (ASUS board with
> a PD on it).
> 
> [    0.220342] [Firmware Bug]: AMD-Vi: IOAPIC[9] not in IVRS table
> [    0.220398] [Firmware Bug]: AMD-Vi: IOAPIC[10] not in IVRS table
> [    0.220451] [Firmware Bug]: AMD-Vi: No southbridge IOAPIC found in IVRS table
> [    0.220506] AMD-Vi: Disabling interrupt remapping due to BIOS Bug(s)

That is actually a BIOS problem. I wonder whether it would help to turn this
into a WARN_ON to get the board vendors to release working BIOSes.
Opinions?


	Joerg

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

[Borislav Petkov]

On Tue, Apr 02, 2013 at 05:03:04PM +0200, Joerg Roedel wrote:
> On Tue, Apr 02, 2013 at 04:40:37PM +0200, Borislav Petkov wrote:
> > While you guys are at it, can someone fix this too pls (ASUS board with
> > a PD on it).
> > 
> > [    0.220342] [Firmware Bug]: AMD-Vi: IOAPIC[9] not in IVRS table
> > [    0.220398] [Firmware Bug]: AMD-Vi: IOAPIC[10] not in IVRS table
> > [    0.220451] [Firmware Bug]: AMD-Vi: No southbridge IOAPIC found in IVRS table
> > [    0.220506] AMD-Vi: Disabling interrupt remapping due to BIOS Bug(s)
> 
> That is actually a BIOS problem. I wonder whether it would help to turn this
> into a WARN_ON to get the board vendors to release working BIOSes.
> Opinions?

Good luck trying to get ASUS to fix anything in their BIOS :(.

Can't we detect the SB IOAPIC some other way in this case?

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

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

[Suravee Suthikulpanit]

On 4/2/2013 10:29 AM, Borislav Petkov wrote:
> On Tue, Apr 02, 2013 at 05:03:04PM +0200, Joerg Roedel wrote:
>> On Tue, Apr 02, 2013 at 04:40:37PM +0200, Borislav Petkov wrote:
>>> While you guys are at it, can someone fix this too pls (ASUS board with
>>> a PD on it).
>>>
>>> [    0.220342] [Firmware Bug]: AMD-Vi: IOAPIC[9] not in IVRS table
>>> [    0.220398] [Firmware Bug]: AMD-Vi: IOAPIC[10] not in IVRS table
>>> [    0.220451] [Firmware Bug]: AMD-Vi: No southbridge IOAPIC found in IVRS table
>>> [    0.220506] AMD-Vi: Disabling interrupt remapping due to BIOS Bug(s)
>> That is actually a BIOS problem. I wonder whether it would help to turn this
>> into a WARN_ON to get the board vendors to release working BIOSes.
>> Opinions?
> Good luck trying to get ASUS to fix anything in their BIOS :(.
I have tried to contact Asus in the past to have them fix the issue, but 
I got no luck.  Once it is out in the field, it's very difficult to get 
them to make changes.  I am also addressing this issue with the BIOS 
team for the future hardware.

Turning this into WARN_ON() at this point might break a lot of systems 
currently out in the field.  However, users can always switching to use 
"intremap=off" but this might not be obvious.

Suravee

> Can't we detect the SB IOAPIC some other way in this case?
>

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

[Joerg Roedel]

On Tue, Apr 02, 2013 at 10:41:25AM -0500, Suthikulpanit, Suravee wrote:
> Turning this into WARN_ON() at this point might break a lot of
> systems currently out in the field.  However, users can always
> switching to use "intremap=off" but this might not be obvious.

A WARN_ON doesn't break systems, it just creates more noise. Probably
enough noise to get board vendors to fix their stuff up. But there is
more to consider before making more noise, of course :)


	Joerg

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

[Joerg Roedel]

On Tue, Apr 02, 2013 at 05:29:56PM +0200, Borislav Petkov wrote:
> On Tue, Apr 02, 2013 at 05:03:04PM +0200, Joerg Roedel wrote:

> Good luck trying to get ASUS to fix anything in their BIOS :(.

Hmm...

> Can't we detect the SB IOAPIC some other way in this case?

I can certainly write a patch that works around your particular BIOS
bug. The problem is that such a fix will most certainly break other
systems.

Unfortunatly there is no reliable way to fixup the IO-APIC-ID->DEVID
mapping at runtime when the BIOS messed it up. The only thing I can do
is to check for potential problems and disable the intremap feature
then, so that the system will at least boot.


	Joerg

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

[Borislav Petkov]

On Tue, Apr 02, 2013 at 06:04:00PM +0200, Joerg Roedel wrote:
> I can certainly write a patch that works around your particular BIOS
> bug. The problem is that such a fix will most certainly break other
> systems.
>
> Unfortunatly there is no reliable way to fixup the IO-APIC-ID->DEVID
> mapping at runtime when the BIOS messed it up. The only thing I can do
> is to check for potential problems and disable the intremap feature
> then, so that the system will at least boot.

Yeah, that could work:

* do not issue message but try to fixup the mapping
* if it works, fine
* if it doesn't, then give up and disable intremap.

And yes, I'm very sceptical about having a WARN_ON and it starts
screaming on machines all over the place. Good luck explaining to
people that you actually wanted to prod BIOS vendors to fix their
monkey-on-crack code but they weren't listening in the first place.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

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

[Joerg Roedel]

On Tue, Apr 02, 2013 at 06:17:57PM +0200, Borislav Petkov wrote:
> On Tue, Apr 02, 2013 at 06:04:00PM +0200, Joerg Roedel wrote:
> > I can certainly write a patch that works around your particular BIOS
> > bug. The problem is that such a fix will most certainly break other
> > systems.
> >
> > Unfortunatly there is no reliable way to fixup the IO-APIC-ID->DEVID
> > mapping at runtime when the BIOS messed it up. The only thing I can do
> > is to check for potential problems and disable the intremap feature
> > then, so that the system will at least boot.
> 
> Yeah, that could work:
> 
> * do not issue message but try to fixup the mapping
> * if it works, fine
> * if it doesn't, then give up and disable intremap.

I can't find out in the driver whether the fix works or not. It will be
noticed later when the x86 code tries to setup the timers and finds out
that they don't work, which causes a kernel panic.

Okay, in theory I could implement a feedback loop between timer-setup
and intremap code and try fixups until it works. But that seems not to
be worth it to work around a buggy BIOS.

What I actually thought about was providing an IVRS-override on the
kernel command line. So that you can specify the IOAPIC_ID->DEVID
mapping there and make it work this way. What do you think?

> And yes, I'm very sceptical about having a WARN_ON and it starts
> screaming on machines all over the place. Good luck explaining to
> people that you actually wanted to prod BIOS vendors to fix their
> monkey-on-crack code but they weren't listening in the first place.

Yeah, that's my fear too. So we leave it better as it is...


	Joerg

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

[Borislav Petkov]

On Tue, Apr 02, 2013 at 06:33:18PM +0200, Joerg Roedel wrote:
> I can't find out in the driver whether the fix works or not. It will
> be noticed later when the x86 code tries to setup the timers and finds
> out that they don't work, which causes a kernel panic.
>
> Okay, in theory I could implement a feedback loop between timer-setup
> and intremap code and try fixups until it works. But that seems not to
> be worth it to work around a buggy BIOS.

Yeah, same here. It's not like we really need intremap to work - we're
only trying to fix the annoying error message currently. :-)

> What I actually thought about was providing an IVRS-override on the
> kernel command line. So that you can specify the IOAPIC_ID->DEVID
> mapping there and make it work this way. What do you think?

I guess that is workable. I can imagine people wanting this if they want
to do the intremap thing on such b0rked BIOSen. So how do I specify this
IOAPIC_ID->DEVID mapping on the cmdline exactly?

> > And yes, I'm very sceptical about having a WARN_ON and it starts
> > screaming on machines all over the place. Good luck explaining to
> > people that you actually wanted to prod BIOS vendors to fix their
> > monkey-on-crack code but they weren't listening in the first place.
> 
> Yeah, that's my fear too. So we leave it better as it is...

Hohumm.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

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

[Joerg Roedel]

On Tue, Apr 02, 2013 at 09:32:40PM +0200, Borislav Petkov wrote:
> On Tue, Apr 02, 2013 at 06:33:18PM +0200, Joerg Roedel wrote:

> > Okay, in theory I could implement a feedback loop between timer-setup
> > and intremap code and try fixups until it works. But that seems not to
> > be worth it to work around a buggy BIOS.
> 
> Yeah, same here. It's not like we really need intremap to work - we're
> only trying to fix the annoying error message currently. :-)

Right :)

> > What I actually thought about was providing an IVRS-override on the
> > kernel command line. So that you can specify the IOAPIC_ID->DEVID
> > mapping there and make it work this way. What do you think?
> 
> I guess that is workable. I can imagine people wanting this if they want
> to do the intremap thing on such b0rked BIOSen. So how do I specify this
> IOAPIC_ID->DEVID mapping on the cmdline exactly?


Don't know yet, probably something like ivrs_ioapic[<apicid>]=0:14.2 and
ivrs_hpet[<hpet-id>]=0:14.2. But not entierly sure yet, at least parsing
shouldn't require lex and yacc ;)

> > Yeah, that's my fear too. So we leave it better as it is...
> 
> Hohumm.

Thus shall it be!


	Joerg