From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] AMD/iommu: SR56x0 Erratum 64 - Reset Command and Event head & tail pointers Date: Tue, 21 May 2013 17:36:19 +0100 Message-ID: <519BA283.2070207@citrix.com> References: <519BAB7E02000078000D7D5B@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <519BAB7E02000078000D7D5B@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 (Xen.org)" , Jacob Shin , SuraveeSuthikulpanit , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 21/05/13 16:14, Jan Beulich wrote: >>>> On 21.05.13 at 16:52, Andrew Cooper wrote: >> Reference at time of patch: >> http://support.amd.com/us/ChipsetMotherboard_TechDocs/46303.pdf >> >> Erratum 64 states that the head and tail pointers for the Command buffer and >> Event log are only reset on a cold boot, not a warm boot. >> >> While the erratum is limited to systems using SR56xx chipsets (such as >> Family >> 10h CPUs), resetting the pointers is a sensible action in all cases. >> >> Signed-off-by: Andrew Cooper > You sent this twice in close succession - is there any difference > between the two instances? No - I tried sending it once, got an error from the SMTP server, tried again 20 mins later and both got forwarded at that point. > >> The code appears to lack an MMIO 64bit write function which would be the >> correct solution here. However, for these four registers, bit 19 is the >> highest non-reserved bit, meaning that the writel() will do the correct >> thing. >> >> I suspect that a writeq() function would make a huge difference to the >> legibility and brevity of this code. > Oh, we should of course have a writeq() - I think I stumbled across > the lack thereof too, and probably more than once. At the moment, writel() is using a voiltile int * cast. Given that the b,w,l and q suffixes have specific widths implied, would it be better to use explicit uintX_t casts? > >> --- a/xen/drivers/passthrough/amd/iommu_init.c >> +++ b/xen/drivers/passthrough/amd/iommu_init.c >> @@ -154,6 +154,15 @@ static void register_iommu_cmd_buffer_in >> IOMMU_CMD_BUFFER_LENGTH_MASK, >> IOMMU_CMD_BUFFER_LENGTH_SHIFT, &entry); >> writel(entry, iommu->mmio_base+IOMMU_CMD_BUFFER_BASE_HIGH_OFFSET); >> + >> + /* AMD SR56x0 Erratum 64. CMD buffer head and tail pointers are not reset >> + * on warm boot. A newer BIOS may or may not do this for us, as per the >> + * workaround advise. >> + * >> + * However, it is a safe and sensible action to perform unconditionally. >> + */ >> + writel(0, iommu->mmio_base + IOMMU_CMD_BUFFER_HEAD_OFFSET); >> + writel(0, iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET); >> } >> >> static void register_iommu_event_log_in_mmio_space(struct amd_iommu *iommu) >> @@ -182,6 +191,15 @@ static void register_iommu_event_log_in_ >> IOMMU_EVENT_LOG_LENGTH_MASK, >> IOMMU_EVENT_LOG_LENGTH_SHIFT, &entry); >> writel(entry, iommu->mmio_base+IOMMU_EVENT_LOG_BASE_HIGH_OFFSET); >> + >> + /* AMD SR56x0 Erratum 64. Event log head and tail pointers are not reset >> + * on warm boot. A newer BIOS may or may not do this for us, as per the >> + * workaround advise. >> + * >> + * However, it is a safe and sensible action to perform unconditionally. >> + */ >> + writel(0, iommu->mmio_base + IOMMU_EVENT_LOG_HEAD_OFFSET); >> + writel(0, iommu->mmio_base + IOMMU_EVENT_LOG_TAIL_OFFSET); >> } >> >> static void register_iommu_ppr_log_in_mmio_space(struct amd_iommu *iommu) > If "it is a safe and sensible action to perform unconditionally", why > don't you then also do the same for the PPR log? > > Jan > Because those were the only entries referenced by the erratum, and I am still learning the AMD IOMMU architecture. I shall extend this to include the PPR log. ~Andrew