From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56714) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjQdS-0003TG-DX for qemu-devel@nongnu.org; Mon, 12 Sep 2016 08:46:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bjQdO-0000nw-8n for qemu-devel@nongnu.org; Mon, 12 Sep 2016 08:46:54 -0400 Received: from mail-yb0-f175.google.com ([209.85.213.175]:35387) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjQdO-0000nU-2f for qemu-devel@nongnu.org; Mon, 12 Sep 2016 08:46:50 -0400 Received: by mail-yb0-f175.google.com with SMTP id d69so13509085ybf.2 for ; Mon, 12 Sep 2016 05:46:49 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20160912121122.GJ3776@pxdev.xzpeter.org> References: <1473674889-2727-1-git-send-email-davidkiarie4@gmail.com> <1473674889-2727-5-git-send-email-davidkiarie4@gmail.com> <20160912113450.GH3776@pxdev.xzpeter.org> <20160912121122.GJ3776@pxdev.xzpeter.org> From: David Kiarie Date: Mon, 12 Sep 2016 15:45:48 +0300 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [v4 4/6] hw/iommu: AMD IOMMU interrupt remapping List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Xu Cc: QEMU Developers , Jan Kiszka , "Michael S. Tsirkin" , rkrcmar@redhat.com, Eduardo Habkost , Paolo Bonzini , Alex Williamson On Mon, Sep 12, 2016 at 3:11 PM, Peter Xu 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 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 >