Re: [Xen-devel] [PATCH] AMD/IOMMU: Common the #732/#733 errata handling in iommu_read_log()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.02.2020 19:55, Andrew Cooper wrote:
> There is no need to have both helpers implement the same workaround.  The size
> and layout of the the Event and PPR logs (and others for that matter) share a
> lot of commonality.
> 
> Use MASK_EXTR() to locate the code field, and use ACCESS_ONCE() rather than
> barrier() to prevent hoisting of the repeated read.
> 
> Avoid unnecessary zeroing by only clobbering the 'code' field - this alone is
> sufficient to spot the errata when the rings wrap.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with one remark / adjustment request:

> @@ -319,11 +319,36 @@ static int iommu_read_log(struct amd_iommu *iommu,
>  
>      while ( tail != log->head )
>      {
> -        /* read event log entry */
> -        entry = log->buffer + log->head;
> +        uint32_t *entry = log->buffer + log->head;
> +        unsigned int count = 0;
> +
> +        /* Event and PPR logs have their code field in the same position. */
> +        unsigned int code = MASK_EXTR(entry[1], IOMMU_EVENT_CODE_MASK);
> +
> +        /*
> +         * Workaround for errata #732, #733:
> +         *
> +         * It can happen that the tail pointer is updated before the actual
> +         * entry got written. As suggested by RevGuide, we initialize the
> +         * buffer to all zeros and clear entries after processing them.

I don't think "clear entries" is applicable anymore with ...

> +         */
> +        while ( unlikely(code == 0) )
> +        {
> +            if ( unlikely(++count == IOMMU_LOG_ENTRY_TIMEOUT) )
> +            {
> +                AMD_IOMMU_DEBUG("AMD-Vi: No entry written to %s Log\n",
> +                                log == &iommu->event_log ? "Event" : "PPR");
> +                return 0;
> +            }
> +            udelay(1);
> +            code = MASK_EXTR(ACCESS_ONCE(entry[1]), IOMMU_EVENT_CODE_MASK);
> +        }
>  
>          parse_func(iommu, entry);
>  
> +        /* Clear 'code' to be able to spot the erratum when the ring wraps. 
> */
> +        ACCESS_ONCE(entry[1]) = 0;

... this. Perhaps at least add "sufficiently"?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel