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

[David Kiarie]

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.


>


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


> Thanks,
>
> -- peterx
>