Re: [PATCH] amd iommu: Add workaround for erratum 732 & 733

[Wei Wang <wei.wang2@amd.com>]

On 05/23/2012 08:57 AM, Jan Beulich wrote:
>>>> On 22.05.12 at 17:30, Wei Wang<wei.wang2@amd.com>  wrote:
>> 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.
>
> I adjusted it further to address a few things I noticed while
> pulling it into my local tree. Please let me know if any of the
> adjustments I did are in error.
>
> I'll also send a half-way related (and dependent in terms of
> being able to cleanly apply) follow-on patch in a minute.

Both look great to me. Acked.

Thanks,
Wei

> Jan
>
> **************************************************
>
> amd iommu: Add workaround for erratum 732&  733
>
> Signed-off-by: Wei Wang<wei.wang2@amd.com>
>
> Add missing barriers. Fix early return from parse_ppr_log_entry().
> Slightly adjust comments. Strip trailing blanks.
>
> Signed-off-by: Jan Beulich<jbeulich@suse.com>
>
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -29,6 +29,7 @@
>   #include<asm/hvm/svm/amd-iommu-proto.h>
>   #include<asm-x86/fixmap.h>
>   #include<mach_apic.h>
> +#include<xen/delay.h>
>
>   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
> +     * got written. As suggested by RevGuide, we initialize the event log
> +     * buffer to all zeros and clear event log entries after processing them.
> +     */
> +    while ( code == 0 )
> +    {
> +        if ( unlikely(++count == IOMMU_LOG_ENTRY_TIMEOUT) )
> +        {
> +            AMD_IOMMU_DEBUG("AMD-Vi: No event written to log\n");
> +            return;
> +        }
> +        udelay(1);
> +        rmb();
> +        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,9 +669,31 @@ 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
> +     * got written. As suggested by RevGuide, we initialize the event log
> +     * buffer to all zeros and clear ppr log entries after processing them.
> +     */
> +    while ( code == 0 )
> +    {
> +        if ( unlikely(++count == IOMMU_LOG_ENTRY_TIMEOUT) )
> +        {
> +            AMD_IOMMU_DEBUG("AMD-Vi: No ppr written to log\n");
> +            return;
> +        }
> +        udelay(1);
> +        rmb();
> +        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]);
> @@ -659,12 +704,10 @@ void parse_ppr_log_entry(struct amd_iomm
>       pdev = pci_get_pdev(iommu->seg, bus, devfn);
>       spin_unlock(&pcidevs_lock);
>
> -    if ( pdev == NULL )
> -        return;
> -
> -    d = pdev->domain;
> +    if ( pdev )
> +        guest_iommu_add_ppr_log(pdev->domain, entry);
>
> -    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)
> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> @@ -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
>
>
>