Andrew Cooper
2013-May-21 14:36 UTC
[PATCH] AMD/iommu: SR56x0 Erratum 64 - Reset Command and Event head & tail pointers
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 <andrew.cooper3@citrix.com> -- 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. diff -r 2369a9d759f0 -r e1a69214aff3 xen/drivers/passthrough/amd/iommu_init.c --- 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)
Andrew Cooper
2013-May-21 14:52 UTC
[PATCH] AMD/iommu: SR56x0 Erratum 64 - Reset Command and Event head & tail pointers
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 <andrew.cooper3@citrix.com> -- 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. diff -r 2369a9d759f0 -r e1a69214aff3 xen/drivers/passthrough/amd/iommu_init.c --- 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)
Jan Beulich
2013-May-21 15:14 UTC
Re: [PATCH] AMD/iommu: SR56x0 Erratum 64 - Reset Command and Event head & tail pointers
>>> On 21.05.13 at 16:52, Andrew Cooper <andrew.cooper3@citrix.com> 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 <andrew.cooper3@citrix.com>You sent this twice in close succession - is there any difference between the two instances?> 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.> --- 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
Andrew Cooper
2013-May-21 16:36 UTC
Re: [PATCH] AMD/iommu: SR56x0 Erratum 64 - Reset Command and Event head & tail pointers
On 21/05/13 16:14, Jan Beulich wrote:>>>> On 21.05.13 at 16:52, Andrew Cooper <andrew.cooper3@citrix.com> 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 <andrew.cooper3@citrix.com> > 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
Jan Beulich
2013-May-22 07:03 UTC
Re: [PATCH] AMD/iommu: SR56x0 Erratum 64 - Reset Command and Event head & tail pointers
>>> On 21.05.13 at 18:36, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 21/05/13 16:14, Jan Beulich wrote: >>>>> On 21.05.13 at 16:52, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >>> 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?Yes, and indeed - as you already suggest - they should also be converted to unsigned types; similarly for read{b,w,l,q}(). The only problem here is with you likely wanting this backported: I''m not sure adding writeq() for x86-32 makes a whole lot of sense, and hence I''d recommend re-submitting the patch here just with the PPR log addition, and then a second one doing the cleanup. Then we can backport the first one, but leave the second one for just -unstable. Jan