From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60267) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1afkif-0001hN-Vt for qemu-devel@nongnu.org; Tue, 15 Mar 2016 04:52:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1afkia-0004bt-P8 for qemu-devel@nongnu.org; Tue, 15 Mar 2016 04:52:49 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41275) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1afkia-0004bm-Hj for qemu-devel@nongnu.org; Tue, 15 Mar 2016 04:52:44 -0400 Date: Tue, 15 Mar 2016 16:52:36 +0800 From: Peter Xu Message-ID: <20160315085236.GA23495@pxdev.xzpeter.org> References: <56E70871.3050305@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <56E70871.3050305@gmail.com> Subject: Re: [Qemu-devel] [PATCH][RFC] IOMMU: Add Support to VFIO devices with vIOMMU present List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: marcel@redhat.com, bd.aviv@gmail.com Cc: Jan Kiszka , Alex Williamson , qemu-devel@nongnu.org, "Michael S. Tsirkin" On Mon, Mar 14, 2016 at 08:52:33PM +0200, Marcel Apfelbaum wrote: > On 03/12/2016 06:13 PM, Aviv B.D. wrote: > Adding (possibly) interested developers to the thread. Thanks CC. Hi, Aviv, several questions inline. [...] > >@@ -1020,14 +1037,53 @@ static void vtd_iotlb_page_invalidate(IntelIOMMUState *s, uint16_t domain_id, > > hwaddr addr, uint8_t am) > > { > > VTDIOTLBPageInvInfo info; > >+ VFIOGuestIOMMU * giommu; > >+ bool flag = false; > > assert(am <= VTD_MAMV); > > info.domain_id = domain_id; > > info.addr = addr; > > info.mask = ~((1 << am) - 1); > >+ > >+ QLIST_FOREACH(giommu, &(s->giommu_list), iommu_next){ > >+ VTDAddressSpace *vtd_as = container_of(giommu->iommu, VTDAddressSpace, iommu); > >+ uint16_t vfio_source_id = vtd_make_source_id(pci_bus_num(vtd_as->bus), vtd_as->devfn); > >+ uint16_t vfio_domain_id = vtd_get_did_dev(s, pci_bus_num(vtd_as->bus), vtd_as->devfn); > >+ if (vfio_domain_id != (uint16_t)-1 && Could you (or anyone) help explain what does vfio_domain_id != -1 mean? > >+ domain_id == vfio_domain_id){ > >+ VTDIOTLBEntry *iotlb_entry = vtd_lookup_iotlb(s, vfio_source_id, addr); > >+ if (iotlb_entry != NULL){ Here, shall we notify VFIO even if the address is not cached in IOTLB? Anyway, we need to do the unmap() of the address, am I correct? > >+ IOMMUTLBEntry entry; > >+ VTD_DPRINTF(GENERAL, "Remove addr 0x%"PRIx64 " mask %d", addr, am); > >+ entry.iova = addr & VTD_PAGE_MASK_4K; > >+ entry.translated_addr = vtd_get_slpte_addr(iotlb_entry->slpte) & VTD_PAGE_MASK_4K; > >+ entry.addr_mask = ~VTD_PAGE_MASK_4K; > >+ entry.perm = IOMMU_NONE; > >+ memory_region_notify_iommu(giommu->iommu, entry); > >+ flag = true; > >+ > >+ } > >+ } > >+ > >+ } > >+ > > g_hash_table_foreach_remove(s->iotlb, vtd_hash_remove_by_page, &info); > >-} > >+ QLIST_FOREACH(giommu, &(s->giommu_list), iommu_next){ > >+ VTDAddressSpace *vtd_as = container_of(giommu->iommu, VTDAddressSpace, iommu); > >+ uint16_t vfio_domain_id = vtd_get_did_dev(s, pci_bus_num(vtd_as->bus), vtd_as->devfn); > >+ if (vfio_domain_id != (uint16_t)-1 && > >+ domain_id == vfio_domain_id && !flag){ > >+ /* do vfio map */ > >+ VTD_DPRINTF(GENERAL, "add addr 0x%"PRIx64 " mask %d", addr, am); > >+ /* call to vtd_iommu_translate */ > >+ IOMMUTLBEntry entry = s->iommu_ops.translate(giommu->iommu, addr, 0); > >+ entry.perm = IOMMU_RW; > >+ memory_region_notify_iommu(giommu->iommu, entry); > >+ //g_vfio_iommu->n.notify(&g_vfio_iommu->n, &entry); > >+ } > >+ } > >+} I see that we did handled all the page invalidations. Would it possible that guest kernel send domain/global invalidations? Should we handle them too? [...] > > static void vfio_listener_region_add(MemoryListener *listener, > > MemoryRegionSection *section) > > { > >@@ -344,6 +347,7 @@ static void vfio_listener_region_add(MemoryListener *listener, > > iova = TARGET_PAGE_ALIGN(section->offset_within_address_space); > > llend = int128_make64(section->offset_within_address_space); > > llend = int128_add(llend, section->size); > >+ llend = int128_add(llend, int128_exts64(-1)); Here, -1 should fix the assertion core dump. However, shall we also handle all the rest places that used "llend" (possibly with variable "end") too? For example, at the end of current function, when we map dma regions: ret = vfio_dma_map(container, iova, end - iova, vaddr, section->readonly); To: ret = vfio_dma_map(container, iova, end + 1 - iova, vaddr, section->readonly); Thanks. Peter