From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36751) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bQQxw-0004nr-Hp for qemu-devel@nongnu.org; Thu, 21 Jul 2016 23:17:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bQQxr-0002rA-Dx for qemu-devel@nongnu.org; Thu, 21 Jul 2016 23:17:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46468) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bQQxr-0002r6-6A for qemu-devel@nongnu.org; Thu, 21 Jul 2016 23:17:27 -0400 Date: Fri, 22 Jul 2016 11:17:20 +0800 From: Peter Xu Message-ID: <20160722031720.GA22866@pxdev.xzpeter.org> References: <1468475796-7397-1-git-send-email-peterx@redhat.com> <1468475796-7397-14-git-send-email-peterx@redhat.com> <20160721204436-mutt-send-email-mst@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20160721204436-mutt-send-email-mst@kernel.org> Subject: Re: [Qemu-devel] [PATCH v12 13/27] intel_iommu: Add support for PCI MSI remap List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: qemu-devel@nongnu.org, imammedo@redhat.com, rth@twiddle.net, ehabkost@redhat.com, jasowang@redhat.com, marcel@redhat.com, pbonzini@redhat.com, jan.kiszka@web.de, rkrcmar@redhat.com, alex.williamson@redhat.com, wexu@redhat.com, davidkiarie4@gmail.com On Thu, Jul 21, 2016 at 08:45:30PM +0300, Michael S. Tsirkin wrote: > On Thu, Jul 14, 2016 at 01:56:22PM +0800, Peter Xu wrote: > > This patch enables interrupt remapping for PCI devices. > > > > To play the trick, one memory region "iommu_ir" is added as child region > > of the original iommu memory region, covering range 0xfeeXXXXX (which is > > the address range for APIC). All the writes to this range will be taken > > as MSI, and translation is carried out only when IR is enabled. > > > > Idea suggested by Paolo Bonzini. > > > > Signed-off-by: Peter Xu > > --- > > hw/i386/intel_iommu.c | 241 +++++++++++++++++++++++++++++++++++++++++ > > hw/i386/intel_iommu_internal.h | 2 + > > include/hw/i386/intel_iommu.h | 66 +++++++++++ > > 3 files changed, 309 insertions(+) > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > index 6a6cb3b..3d1b15d 100644 > > --- a/hw/i386/intel_iommu.c > > +++ b/hw/i386/intel_iommu.c > > @@ -1982,6 +1982,242 @@ static Property vtd_properties[] = { > > DEFINE_PROP_END_OF_LIST(), > > }; > > > > +/* Read IRTE entry with specific index */ > > +static int vtd_irte_get(IntelIOMMUState *iommu, uint16_t index, > > + VTD_IRTE *entry) > > +{ > > + dma_addr_t addr = 0x00; > > + > > + addr = iommu->intr_root + index * sizeof(*entry); > > + if (dma_memory_read(&address_space_memory, addr, entry, > > + sizeof(*entry))) { > > + VTD_DPRINTF(GENERAL, "error: fail to access IR root at 0x%"PRIx64 > > + " + %"PRIu16, iommu->intr_root, index); > > + return -VTD_FR_IR_ROOT_INVAL; > > + } > > + > > + if (!entry->present) { > > + VTD_DPRINTF(GENERAL, "error: present flag not set in IRTE" > > + " entry index %u value 0x%"PRIx64 " 0x%"PRIx64, > > + index, le64_to_cpu(entry->data[1]), > > + le64_to_cpu(entry->data[0])); > > + return -VTD_FR_IR_ENTRY_P; > > + } > > + > > + if (entry->__reserved_0 || entry->__reserved_1 || \ > > + entry->__reserved_2) { > > + VTD_DPRINTF(GENERAL, "error: IRTE entry index %"PRIu16 > > + " reserved fields non-zero: 0x%"PRIx64 " 0x%"PRIx64, > > + index, le64_to_cpu(entry->data[1]), > > + le64_to_cpu(entry->data[0])); > > + return -VTD_FR_IR_IRTE_RSVD; > > + } > > + > > + /* > > + * TODO: Check Source-ID corresponds to SVT (Source Validation > > + * Type) bits > > + */ > > + > > + return 0; > > +} > > + > > +/* Fetch IRQ information of specific IR index */ > > +static int vtd_remap_irq_get(IntelIOMMUState *iommu, uint16_t index, VTDIrq *irq) > > +{ > > + VTD_IRTE irte; > > + int ret = 0; > > + > > + bzero(&irte, sizeof(irte)); > > + > > + ret = vtd_irte_get(iommu, index, &irte); > > + if (ret) { > > + return ret; > > + } > > + > > + irq->trigger_mode = irte.trigger_mode; > > + irq->vector = irte.vector; > > + irq->delivery_mode = irte.delivery_mode; > > + /* Not support EIM yet: please refer to vt-d 9.10 DST bits */ > > +#define VTD_IR_APIC_DEST_MASK (0xff00ULL) > > +#define VTD_IR_APIC_DEST_SHIFT (8) > > + irq->dest = (le32_to_cpu(irte.dest_id) & VTD_IR_APIC_DEST_MASK) >> \ > > + VTD_IR_APIC_DEST_SHIFT; > > + irq->dest_mode = irte.dest_mode; > > + irq->redir_hint = irte.redir_hint; > > + > > + VTD_DPRINTF(IR, "remapping interrupt index %d: trig:%u,vec:%u," > > + "deliver:%u,dest:%u,dest_mode:%u", index, > > + irq->trigger_mode, irq->vector, irq->delivery_mode, > > + irq->dest, irq->dest_mode); > > + > > + return 0; > > +} > > + > > +/* Generate one MSI message from VTDIrq info */ > > +static void vtd_generate_msi_message(VTDIrq *irq, MSIMessage *msg_out) > > +{ > > + VTD_MSIMessage msg = {}; > > + > > + /* Generate address bits */ > > + msg.dest_mode = irq->dest_mode; > > + msg.redir_hint = irq->redir_hint; > > + msg.dest = irq->dest; > > + msg.__addr_head = cpu_to_le32(0xfee); > > + /* Keep this from original MSI address bits */ > > + msg.__not_used = irq->msi_addr_last_bits; > > + > > + /* Generate data bits */ > > + msg.vector = irq->vector; > > + msg.delivery_mode = irq->delivery_mode; > > + msg.level = 1; > > + msg.trigger_mode = irq->trigger_mode; > > + > > + msg_out->address = msg.msi_addr; > > + msg_out->data = msg.msi_data; > > +} > > + > > +/* Interrupt remapping for MSI/MSI-X entry */ > > +static int vtd_interrupt_remap_msi(IntelIOMMUState *iommu, > > + MSIMessage *origin, > > + MSIMessage *translated) > > +{ > > + int ret = 0; > > + VTD_IR_MSIAddress addr; > > + uint16_t index; > > + VTDIrq irq = {0}; > > + > > + assert(origin && translated); > > + > > + if (!iommu || !iommu->intr_enabled) { > > + goto do_not_translate; > > + } > > + > > + if (origin->address & VTD_MSI_ADDR_HI_MASK) { > > + VTD_DPRINTF(GENERAL, "error: MSI addr high 32 bits nonzero" > > + " during interrupt remapping: 0x%"PRIx32, > > + (uint32_t)((origin->address & VTD_MSI_ADDR_HI_MASK) >> \ > > + VTD_MSI_ADDR_HI_SHIFT)); > > + return -VTD_FR_IR_REQ_RSVD; > > + } > > + > > + addr.data = origin->address & VTD_MSI_ADDR_LO_MASK; > > Below you treat data as LE, but here you use VTD_MSI_ADDR_LO_MASK > which is native endian. This looks wrong to me. You are right. I see that this is merged in master. Will fix this in seperate patch. Thanks, -- peterx