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

[Peter Xu <peterx@redhat.com>]

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?

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

Thanks,

-- peterx