Re: [Qemu-devel] [PATCH][RFC] IOMMU: Add Support to VFIO devices with vIOMMU present

[Aviv B.D.]

On Tue, Mar 15, 2016 at 10:52 AM, Peter Xu <address@hidden> wrote:

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?

vtd_get_did_dev returns -1 if the device is not mapped to any domain (generally, the CE is not present).

probably a better interface is to return whether the device has a domain or not and returns the domain_id via the pointer argument.

 

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

With this code I do a unmap operation if the address was cached in the IOTLB, if not I'm assuming that the current invalidation invalidate an (previously) non present address and I should do a map operation (during the map operation I'm calling s->iommu_ops.translate that caches the address).  

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

You are correct, currently this code is pretty much at POC level, and i support only the page invalidation because this is what linux is using. The final version should support also those invalidation ops.

[...]

> >  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);

I will add this to the next version of the patch, thanks! 

Thanks.
Peter