[PATCH] AMD/iommu: SR56x0 Erratum 64 - Reset Command and Event head & tail pointers

[PATCH] AMD/iommu: SR56x0 Erratum 64 - Reset Command and Event head & tail pointers

[Andrew Cooper]

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)

[PATCH] AMD/iommu: SR56x0 Erratum 64 - Reset Command and Event head & tail pointers

[Andrew Cooper]

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)

Re: [PATCH] AMD/iommu: SR56x0 Erratum 64 - Reset Command and Event head & tail pointers

[Jan Beulich]

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

Re: [PATCH] AMD/iommu: SR56x0 Erratum 64 - Reset Command and Event head & tail pointers

[Andrew Cooper]

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

Re: [PATCH] AMD/iommu: SR56x0 Erratum 64 - Reset Command and Event head & tail pointers

[Jan Beulich]

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