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

[Peter Xu <peterx@redhat.com>]

On Mon, Sep 12, 2016 at 02:51:27PM +0300, David Kiarie wrote:
> On Mon, Sep 12, 2016 at 2:34 PM, Peter Xu <peterx@redhat.com> 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.

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

> 
> 
> >
> 
> 
> > > +            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