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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.05.12 at 15:56, Wei Wang <wei.wang2@xxxxxxx> wrote:
> Hi Jan
> This patch implements the suggested workaround for erratum 732 & 733. It 
> is mostly derived from the Linux patch recently posted. Please review it.
> Thanks,
> Wei

Looks good in principle, but came out with newline damage. Can
you please resend as attachment, ideally at once fixing a few
(mostly cosmetic) things:

> # HG changeset patch
> # User Wei Wang <wei.wang2@xxxxxxx>
> # Date 1337690747 -7200
> # Node ID 06aebc361de7f308b262b008153ae4549c4480c2
> # Parent  592d15bd4d5ec58486d32ee9998319e7c95fcd66
> amd iommu: Add workaround for erratum 733 & 733

732 & 733

> Signed-off-by: Wei Wang <wei.wang2@xxxxxxx>
> 
> diff -r 592d15bd4d5e -r 06aebc361de7 
> xen/drivers/passthrough/amd/iommu_init.c
> --- a/xen/drivers/passthrough/amd/iommu_init.c    Fri May 18 16:19:21 
> 2012 +0100
> +++ b/xen/drivers/passthrough/amd/iommu_init.c    Tue May 22 14:45:47 
> 2012 +0200
> @@ -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",
> @@ -575,9 +577,27 @@ static void parse_event_log_entry(struct
>                             "IOTLB_INV_TIMEOUT",
>                             "INVALID_DEV_REQUEST"};
> 
> +retry:
>       code = get_field_from_reg_u32(entry[1], IOMMU_EVENT_CODE_MASK,
>                                               IOMMU_EVENT_CODE_SHIFT);
> 
> +    /* Workaround for erratum 732.

    /*
     * Workaround for erratum 732.

> +     * it can happen that the tail pointer is updated before the actual 
> entry
> +     * is written. Suggested by RevGuide, we initialize the event log 
> buffer to
> +     * all zeros and write event log entries to zero after they are 
> processed.
> +     */
> +
> +    if ( code == 0 )
> +    {
> +        if ( unlikely(++count == IOMMU_LOG_ENTRY_TIMEOUT) )
> +        {
> +            AMD_IOMMU_DEBUG("AMD-Vi: No event written to event log\n");

Perhaps remove the second "event".

> +            return;
> +        }
> +        udelay(1);
> +        goto retry;
> +    }
> +
>       if ( (code > IOMMU_EVENT_INVALID_DEV_REQUEST) ||
>           (code < IOMMU_EVENT_ILLEGAL_DEV_TABLE_ENTRY) )
>       {
> @@ -615,6 +635,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,10 +668,32 @@ 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;
> 
> +retry:
> +    code = get_field_from_reg_u32(entry[1], IOMMU_PPR_LOG_CODE_MASK,
> +                                            IOMMU_PPR_LOG_CODE_SHIFT);
> +
> +    /* Workaround for erratum 733.

See above.

> +     * it can happen that the tail pointer is updated before the actual 
> entry
> +     * is written. Suggested by RevGuide, we initialize the ppr log 
> buffer to
> +     * all zeros and write ppr log entries to zero after they are 
> processed.
> +     */
> +
> +    if ( code == 0 )
> +    {
> +        if ( unlikely(++count == IOMMU_LOG_ENTRY_TIMEOUT) )
> +        {
> +            AMD_IOMMU_DEBUG("AMD-Vi: No ppr written to ppr log\n");

Perhaps remove the second "ppr".

> +            return;
> +        }
> +        udelay(1);
> +        goto retry;
> +    }
> +
>       /* here device_id is physical value */
>       device_id = iommu_get_devid_from_cmd(entry[0]);
>       bus = PCI_BUS(device_id);
> @@ -665,6 +709,8 @@ void parse_ppr_log_entry(struct amd_iomm
>       d = pdev->domain;
> 
>       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)

I'd personally also prefer the loops to be written as such (i.e.
without goto-s).

> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h    Fri May 18 
> 16:19:21 2012 +0100
> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h    Tue May 22 
> 14:45:47 2012 +0200
> @@ -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                         100000

That's rather long a timeout (100ms) for a busy loop - is that
really necessary?

Jan

> 
>   /* Control Register */
>   #define IOMMU_CONTROL_MMIO_OFFSET            0x18



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel