From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wei Wang Subject: Re: [PATCH] amd iommu: Add workaround for erratum 732 & 733 Date: Tue, 22 May 2012 17:30:38 +0200 Message-ID: <4FBBB11E.5070709@amd.com> References: <4FBB9B06.1030200@amd.com> <4FBBBA870200007800085296@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------080408010302010002060503" Return-path: In-Reply-To: <4FBBBA870200007800085296@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Keir Fraser , xen-devel List-Id: xen-devel@lists.xenproject.org --------------080408010302010002060503 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Hi Jan, Thanks for review it. New version has been attached. It should have fixed issues you mentioned. We don't have a particular number for loop count, so I cut it to 1000, it should be enough. Please have a look. Thanks, Wei On 05/22/2012 04:10 PM, Jan Beulich wrote: >>>> On 22.05.12 at 15:56, Wei Wang wrote: >> Hi Jan >> This patch implements the suggested workaround for erratum 732& 733. It >> is mostly derived from the Linux patch recently posted. Please review it. >> Thanks, >> Wei > > Looks good in principle, but came out with newline damage. Can > you please resend as attachment, ideally at once fixing a few > (mostly cosmetic) things: > >> # HG changeset patch >> # User Wei Wang >> # Date 1337690747 -7200 >> # Node ID 06aebc361de7f308b262b008153ae4549c4480c2 >> # Parent 592d15bd4d5ec58486d32ee9998319e7c95fcd66 >> amd iommu: Add workaround for erratum 733& 733 > > 732& 733 > >> Signed-off-by: Wei Wang >> >> diff -r 592d15bd4d5e -r 06aebc361de7 >> xen/drivers/passthrough/amd/iommu_init.c >> --- a/xen/drivers/passthrough/amd/iommu_init.c Fri May 18 16:19:21 >> 2012 +0100 >> +++ b/xen/drivers/passthrough/amd/iommu_init.c Tue May 22 14:45:47 >> 2012 +0200 >> @@ -29,6 +29,7 @@ >> #include >> #include >> #include >> +#include >> >> static int __initdata nr_amd_iommus; >> >> @@ -566,6 +567,7 @@ static void parse_event_log_entry(struct >> u16 domain_id, device_id, bdf, cword; >> u32 code; >> u64 *addr; >> + int count = 0; >> char * event_str[] = {"ILLEGAL_DEV_TABLE_ENTRY", >> "IO_PAGE_FAULT", >> "DEV_TABLE_HW_ERROR", >> @@ -575,9 +577,27 @@ static void parse_event_log_entry(struct >> "IOTLB_INV_TIMEOUT", >> "INVALID_DEV_REQUEST"}; >> >> +retry: >> code = get_field_from_reg_u32(entry[1], IOMMU_EVENT_CODE_MASK, >> IOMMU_EVENT_CODE_SHIFT); >> >> + /* Workaround for erratum 732. > > /* > * Workaround for erratum 732. > >> + * it can happen that the tail pointer is updated before the actual >> entry >> + * is written. Suggested by RevGuide, we initialize the event log >> buffer to >> + * all zeros and write event log entries to zero after they are >> processed. >> + */ >> + >> + if ( code == 0 ) >> + { >> + if ( unlikely(++count == IOMMU_LOG_ENTRY_TIMEOUT) ) >> + { >> + AMD_IOMMU_DEBUG("AMD-Vi: No event written to event log\n"); > > Perhaps remove the second "event". > >> + return; >> + } >> + udelay(1); >> + goto retry; >> + } >> + >> if ( (code> IOMMU_EVENT_INVALID_DEV_REQUEST) || >> (code< IOMMU_EVENT_ILLEGAL_DEV_TABLE_ENTRY) ) >> { >> @@ -615,6 +635,8 @@ static void parse_event_log_entry(struct >> AMD_IOMMU_DEBUG("event 0x%08x 0x%08x 0x%08x 0x%08x\n", entry[0], >> entry[1], entry[2], entry[3]); >> } >> + >> + memset(entry, 0, IOMMU_EVENT_LOG_ENTRY_SIZE); >> } >> >> static void iommu_check_event_log(struct amd_iommu *iommu) >> @@ -646,10 +668,32 @@ void parse_ppr_log_entry(struct amd_iomm >> { >> >> u16 device_id; >> - u8 bus, devfn; >> + u8 bus, devfn, code; >> struct pci_dev *pdev; >> struct domain *d; >> + int count = 0; >> >> +retry: >> + code = get_field_from_reg_u32(entry[1], IOMMU_PPR_LOG_CODE_MASK, >> + IOMMU_PPR_LOG_CODE_SHIFT); >> + >> + /* Workaround for erratum 733. > > See above. > >> + * it can happen that the tail pointer is updated before the actual >> entry >> + * is written. Suggested by RevGuide, we initialize the ppr log >> buffer to >> + * all zeros and write ppr log entries to zero after they are >> processed. >> + */ >> + >> + if ( code == 0 ) >> + { >> + if ( unlikely(++count == IOMMU_LOG_ENTRY_TIMEOUT) ) >> + { >> + AMD_IOMMU_DEBUG("AMD-Vi: No ppr written to ppr log\n"); > > Perhaps remove the second "ppr". > >> + return; >> + } >> + udelay(1); >> + goto retry; >> + } >> + >> /* here device_id is physical value */ >> device_id = iommu_get_devid_from_cmd(entry[0]); >> bus = PCI_BUS(device_id); >> @@ -665,6 +709,8 @@ void parse_ppr_log_entry(struct amd_iomm >> d = pdev->domain; >> >> guest_iommu_add_ppr_log(d, entry); >> + >> + memset(entry, 0, IOMMU_PPR_LOG_ENTRY_SIZE); >> } >> >> static void iommu_check_ppr_log(struct amd_iommu *iommu) > > I'd personally also prefer the loops to be written as such (i.e. > without goto-s). > >> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h Fri May 18 >> 16:19:21 2012 +0100 >> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h Tue May 22 >> 14:45:47 2012 +0200 >> @@ -301,6 +301,10 @@ >> #define IOMMU_PPR_LOG_TAIL_OFFSET 0x2038 >> #define IOMMU_PPR_LOG_DEVICE_ID_MASK 0x0000FFFF >> #define IOMMU_PPR_LOG_DEVICE_ID_SHIFT 0 >> +#define IOMMU_PPR_LOG_CODE_MASK 0xF0000000 >> +#define IOMMU_PPR_LOG_CODE_SHIFT 28 >> + >> +#define IOMMU_LOG_ENTRY_TIMEOUT 100000 > > That's rather long a timeout (100ms) for a busy loop - is that > really necessary? > > Jan > >> >> /* Control Register */ >> #define IOMMU_CONTROL_MMIO_OFFSET 0x18 > > > --------------080408010302010002060503 Content-Type: text/x-patch; name="iommu_errata.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="iommu_errata.patch" Content-Description: iommu_errata.patch # HG changeset patch # Parent 592d15bd4d5ec58486d32ee9998319e7c95fcd66 # User Wei Wang amd iommu: Add workaround for erratum 732 & 733 Signed-off-by: Wei Wang diff -r 592d15bd4d5e xen/drivers/passthrough/amd/iommu_init.c --- a/xen/drivers/passthrough/amd/iommu_init.c Fri May 18 16:19:21 2012 +0100 +++ b/xen/drivers/passthrough/amd/iommu_init.c Tue May 22 17:04:57 2012 +0200 @@ -29,6 +29,7 @@ #include #include #include +#include static int __initdata nr_amd_iommus; @@ -566,6 +567,7 @@ static void parse_event_log_entry(struct u16 domain_id, device_id, bdf, cword; u32 code; u64 *addr; + int count = 0; char * event_str[] = {"ILLEGAL_DEV_TABLE_ENTRY", "IO_PAGE_FAULT", "DEV_TABLE_HW_ERROR", @@ -578,6 +580,25 @@ static void parse_event_log_entry(struct code = get_field_from_reg_u32(entry[1], IOMMU_EVENT_CODE_MASK, IOMMU_EVENT_CODE_SHIFT); + /* + * Workaround for erratum 732. + * it can happen that the tail pointer is updated before the actual entry + * is written. Suggested by RevGuide, we initialize the event log buffer to + * all zeros and write event log entries to zero after they are processed. + */ + + while ( code == 0 ) + { + if ( unlikely(++count == IOMMU_LOG_ENTRY_TIMEOUT) ) + { + AMD_IOMMU_DEBUG("AMD-Vi: No event written to log\n"); + return; + } + udelay(1); + code = get_field_from_reg_u32(entry[1], IOMMU_EVENT_CODE_MASK, + IOMMU_EVENT_CODE_SHIFT); + } + if ( (code > IOMMU_EVENT_INVALID_DEV_REQUEST) || (code < IOMMU_EVENT_ILLEGAL_DEV_TABLE_ENTRY) ) { @@ -615,6 +636,8 @@ static void parse_event_log_entry(struct AMD_IOMMU_DEBUG("event 0x%08x 0x%08x 0x%08x 0x%08x\n", entry[0], entry[1], entry[2], entry[3]); } + + memset(entry, 0, IOMMU_EVENT_LOG_ENTRY_SIZE); } static void iommu_check_event_log(struct amd_iommu *iommu) @@ -646,10 +669,33 @@ void parse_ppr_log_entry(struct amd_iomm { u16 device_id; - u8 bus, devfn; + u8 bus, devfn, code; struct pci_dev *pdev; struct domain *d; + int count = 0; + code = get_field_from_reg_u32(entry[1], IOMMU_PPR_LOG_CODE_MASK, + IOMMU_PPR_LOG_CODE_SHIFT); + + /* + * Workaround for erratum 733. + * it can happen that the tail pointer is updated before the actual entry + * is written. Suggested by RevGuide, we initialize the ppr log buffer to + * all zeros and write ppr log entries to zero after they are processed. + */ + + while ( code == 0 ) + { + if ( unlikely(++count == IOMMU_LOG_ENTRY_TIMEOUT) ) + { + AMD_IOMMU_DEBUG("AMD-Vi: No ppr written to log\n"); + return; + } + udelay(1); + code = get_field_from_reg_u32(entry[1], IOMMU_PPR_LOG_CODE_MASK, + IOMMU_PPR_LOG_CODE_SHIFT); + } + /* here device_id is physical value */ device_id = iommu_get_devid_from_cmd(entry[0]); bus = PCI_BUS(device_id); @@ -665,6 +711,8 @@ void parse_ppr_log_entry(struct amd_iomm d = pdev->domain; guest_iommu_add_ppr_log(d, entry); + + memset(entry, 0, IOMMU_PPR_LOG_ENTRY_SIZE); } static void iommu_check_ppr_log(struct amd_iommu *iommu) diff -r 592d15bd4d5e xen/include/asm-x86/hvm/svm/amd-iommu-defs.h --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h Fri May 18 16:19:21 2012 +0100 +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h Tue May 22 17:04:57 2012 +0200 @@ -301,6 +301,10 @@ #define IOMMU_PPR_LOG_TAIL_OFFSET 0x2038 #define IOMMU_PPR_LOG_DEVICE_ID_MASK 0x0000FFFF #define IOMMU_PPR_LOG_DEVICE_ID_SHIFT 0 +#define IOMMU_PPR_LOG_CODE_MASK 0xF0000000 +#define IOMMU_PPR_LOG_CODE_SHIFT 28 + +#define IOMMU_LOG_ENTRY_TIMEOUT 1000 /* Control Register */ #define IOMMU_CONTROL_MMIO_OFFSET 0x18 --------------080408010302010002060503 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --------------080408010302010002060503--