Re: [Xen-devel] [PATCH 2/2 V2] iommu/amd: Workaround for erratum 787

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.06.13 at 15:55, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>>>> On 10.06.13 at 14:43, Tim Deegan <tim@xxxxxxx> wrote:
>> How about:
>>  write-to-clear status.pending
>>  process the log
>>  if (status.pending)
>>     reschedule the tasklet
>>  else
>>     unmask the interrupt.
> 
> Actually, I don't think that's right: Clearing the pending bit with
> the respective interrupt source disabled doesn't allow the
> pending bit to become set again upon arrival of a new log entry,
> and hence we might still be leaving entries unhandled for an
> indefinite period of time. So I now think we need to do
> 
>   write-to-clear status.pending
>   process the log
>   unmask the interrupt
>   process the log again
>   if (status.pending)
>      reschedule the tasklet
> 
> Of course we could skip the unmask and parse-again if the
> interrupt wasn't masked in the first place, which is possible since
> it gets masked only for any IOMMU that had its interrupt handler
> executed before the tasklet gets busy on it.

So with this done I now realized that all of these transformations
don't really belong in this erratum workaround patch. They either
should be a prereq patch (or folded into patch 1), or a follow-up
one, with the one here then nevertheless going back to the
original simple loop approach. Do you view this any different?

Here is what one of the two functions now looks like, just for
reference:

static void iommu_check_event_log(struct amd_iommu *iommu)
{
    u32 entry;
    unsigned long flags;

    for ( ; ; )
    {
        /* RW1C interrupt status bit */
        writel(IOMMU_STATUS_EVENT_LOG_INT_MASK,
               iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);

        iommu_read_log(iommu, &iommu->event_log,
                       sizeof(event_entry_t), parse_event_log_entry);

        spin_lock_irqsave(&iommu->lock, flags);

        /* Check event overflow. */
        entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
        if ( entry & IOMMU_STATUS_EVENT_OVERFLOW_MASK )
            iommu_reset_log(iommu, &iommu->event_log,
                            set_iommu_event_log_control);
        else
        {
            entry = readl(iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET);
            if ( !(entry & IOMMU_CONTROL_EVENT_LOG_INT_MASK) )
            {
                entry |= IOMMU_CONTROL_EVENT_LOG_INT_MASK;
                writel(entry, iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET);
                spin_unlock_irqrestore(&iommu->lock, flags);
                continue;
            }
        }

        break;
    }

    /*
     * Workaround for erratum787:
     * Re-check to make sure the bit has been cleared.
     */
    entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
    if ( entry & IOMMU_STATUS_EVENT_LOG_INT_MASK )
        tasklet_schedule(&amd_iommu_irq_tasklet);

    spin_unlock_irqrestore(&iommu->lock, flags);
}

You'll note that even here we're having a loop again, which you
presumably won't like much. The only alternative I see is to run
iommu_read_log() with iommu->lock held, which has the caveat of
the function itself taking a lock (albeit - without having done any
proving yet - I think that lock is taken completely pointlessly).

In any case - the erratum workaround is really just the comment
and three following lines. Everything else belongs elsewhere imo.

Jan


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