Re: [Qemu-devel] [v4 4/6] hw/iommu: AMD IOMMU interrupt remapping

[David Kiarie]

On Mon, Sep 12, 2016 at 3:11 PM, Peter Xu <address@hidden> wrote:

> On Mon, Sep 12, 2016 at 02:51:27PM +0300, David Kiarie wrote:
> > On Mon, Sep 12, 2016 at 2:34 PM, Peter Xu <address@hidden> wrote:
> >
> > > On Mon, Sep 12, 2016 at 01:08:07PM +0300, David Kiarie wrote:
> > >
> > > [...]
> > >
> > > >  /* configure MMIO registers at startup/reset */
> > > >  static void amdvi_set_quad(AMDVIState *s, hwaddr addr, uint64_t val,
> > > >                             uint64_t romask, uint64_t w1cmask)
> > > > @@ -641,6 +667,11 @@ static void amdvi_inval_inttable(AMDVIState *s,
> > > CMDInvalIntrTable *inval)
> > > >          amdvi_log_illegalcom_error(s, inval->type, s->cmdbuf +
> > > s->cmdbuf_head);
> > > >          return;
> > > >      }
> > > > +
> > > > +    if (s->ir_cache) {
> > >
> > > Here, we notify IEC only if ir_cache == true, while...
> > >
> > > [...]
> > >
> > > > +static int amdvi_int_remap(X86IOMMUState *iommu, MSIMessage *src,
> > > > +                           MSIMessage *dst, uint16_t sid)
> > > > +{
> > > > +    trace_amdvi_ir_request(src->data, src->address, sid);
> > > > +
> > > > +    AMDVIState *s = AMD_IOMMU_DEVICE(iommu);
> > > > +    int ret = 0;
> > > > +    uint64_t dte[4];
> > > > +    uint32_t bitpos;
> > > > +    IRTE irte;
> > > > +
> > > > +    amdvi_get_dte(s, sid, dte);
> > > > +
> > > > +    /* interrupt remapping disabled */
> > > > +    if (!(dte[2] & AMDVI_IR_VALID)) {
> > > > +        memcpy(dst, src, sizeof(*src));
> > > > +        return ret;
> > > > +    }
> > > > +
> > > > +    ret = amdvi_irte_get(s, src, &irte, dte, sid);
> > > > +    if (ret < 0) {
> > > > +        goto no_remap;
> > > > +    }
> > > > +    switch (src->data & AMDVI_IR_TYPE_MASK) {
> > > > +    case AMDVI_MT_FIXED:
> > > > +    case AMDVI_MT_ARBIT:
> > > > +        ret = amdvi_remap_ir_intctl(dte[2], irte, src, dst);
> > > > +        if (ret < 0) {
> > > > +            goto no_remap;
> > > > +        } else {
> > > > +            s->ir_cache = true;
> > >
> > > Here we set it only if the interrupts are triggered.
> > >
> > > Shouldn't we notify IEC in all cases? Since the caches are setup
> > > during configuration, not the first time the interrupt is triggered,
> > > no?
> >
> >
> > I did have a problem with this. I don't know whether Intel IOMMU behaves
> > the same way but AMD IOMMU invalidates interrupt cache for each and every
> > device at boot(every possible device). Having the cache invalidation
> > trigger this many times bugs the guest at boot. I was of the opinion that
> > caches will not contain anything until translations actually happen.
>
> When we say cache here, we are mostly talking about GSI routes in
> kernel, right? Since we still don't have other kind of interrupt
> caches AFAIK. If so, GSI routes should already been setup even if the
> interrupts are not triggered for a single time. So we need to
> invalidate them even ir_cache == false.
>

You're right but I'm not sure how to implement that while avoiding
triggering the notifier numerous pointless times during boot.


> I think the problem is why cache invalidations during boot will bug
> the system. Any clue?
>

The issue is not too many invalidations. I don't have a very clear idea of
how notifiers work but I would assume it spawns a thread or they somehow
use a multithreaded approach which would mean triggering the notifier too
many times within a very short period many trigger a bunch of issues.


> >
> >
> > >
> >
> >
> > > > +            trace_amdvi_ir_remap(dst->data, dst->address, sid);
> > > > +            return ret;
> > > > +        }
> > > > +    /* not handling SMI currently */
> > > > +    case AMDVI_MT_SMI:
> > > > +        error_report("SMI interrupts not currently handled");
> > > > +        goto no_remap;
> > > > +    case AMDVI_MT_NMI:
> > > > +        bitpos = AMDVI_DTE_NMIPASS_LSHIFT;
> > > > +        break;
> > > > +    case AMDVI_MT_INIT:
> > > > +        bitpos = AMDVI_DTE_INTPASS_LSHIFT;
> > > > +        break;
> > > > +    case AMDVI_MT_EXTINT:
> > > > +        bitpos = AMDVI_DTE_EINTPASS_LSHIFT;
> > > > +        break;
> > > > +    case AMDVI_MT_LINT1:
> > > > +        bitpos = AMDVI_DTE_LINT1PASS_LSHIFT;
> > > > +        break;
> > > > +    case AMDVI_MT_LINT0:
> > > > +        bitpos = AMDVI_DTE_LINT0PASS_LSHIFT;
> > > > +    default:
> > > > +        goto no_remap;
> > > > +    }
> > > > +
> > > > +    ret = amdvi_ir_handle_non_vectored(src, dst, bitpos, dte[2]);
> > > > +    if (ret < 0){
> > > > +        goto no_remap;
> > > > +    }
> > > > +    s->ir_cache = true;
> > > > +    trace_amdvi_ir_remap(dst->data, dst->address, sid);
> > > > +    return ret;
> > > > +no_remap:
> > > > +    memcpy(dst, src, sizeof(*src));
> > >
> > > Shall we drop it and report when the remapping failed in some way?
> > >
> > > I'm totally okay that we just drop it here, and we can do the
> > > reporting in the future. But using the old is not a good one, since if
> > > someone injects fault interrupts, it will be delivered just like
> > > without IOMMU. So we have no protection at all.
> > >
> >
> > Wont this get dropped based on the return value ? I think the 'memcpy' is
> > not necessary but  my understanding is kvm will drop the translation
> based
> > on the return value, no ?
>
> Yeah you are right. Then I'll suggest something like:
>
>     no_remap:
>         memcpy(...);
>     remap_fail:
>         trace_...();
>         return ret;
>
> And we goto remap_fail when error happens. That'll be cleaner to me,
> and after all we don't need to memcpy() if something failed.
>
> Thanks,
>
> -- peterx
>