Re: [Qemu-devel] [PATCH RFC v4 15/20] intel_iommu: provide its own replay() callback

[Peter Xu <peterx@redhat.com>]

On Mon, Jan 23, 2017 at 11:12:27AM +0800, Jason Wang wrote:
> 
> 
> On 2017年01月23日 10:54, Peter Xu wrote:
> >>>>>+            trace_vtd_page_walk_skip_read(iova, iova_next);
> >>>>>+            skipped_local++;
> >>>>>+            goto next;
> >>>>>+        }
> >>>>>+
> >>>>>+        if (vtd_slpte_nonzero_rsvd(slpte, level)) {
> >>>>>+            trace_vtd_page_walk_skip_reserve(iova, iova_next);
> >>>>>+            skipped_local++;
> >>>>>+            goto next;
> >>>>>+        }
> >>>>>+
> >>>>>+        /* Permissions are stacked with parents' */
> >>>>>+        read_cur = read && (slpte & VTD_SL_R);
> >>>>>+        write_cur = write && (slpte & VTD_SL_W);
> >>>>>+
> >>>>>+        /*
> >>>>>+         * As long as we have either read/write permission, this is
> >>>>>+         * a valid entry. The rule works for both page or page tables.
> >>>>>+         */
> >>>>>+        entry_valid = read_cur | write_cur;
> >>>>>+
> >>>>>+        if (vtd_is_last_slpte(slpte, level)) {
> >>>>>+            entry.target_as = &address_space_memory;
> >>>>>+            entry.iova = iova & subpage_mask;
> >>>>>+            /*
> >>>>>+             * This might be meaningless addr if (!read_cur &&
> >>>>>+             * !write_cur), but after all this field will be
> >>>>>+             * meaningless in that case, so let's share the code to
> >>>>>+             * generate the IOTLBs no matter it's an MAP or UNMAP
> >>>>>+             */
> >>>>>+            entry.translated_addr = vtd_get_slpte_addr(slpte);
> >>>>>+            entry.addr_mask = ~subpage_mask;
> >>>>>+            entry.perm = IOMMU_ACCESS_FLAG(read_cur, write_cur);
> >>>>>+            if (!entry_valid && !notify_unmap) {
> >>>>>+                trace_vtd_page_walk_skip_perm(iova, iova_next);
> >>>>>+                skipped_local++;
> >>>>>+                goto next;
> >>>>>+            }
> >>>>Under which case should we care about unmap here (better with a comment I
> >>>>think)?
> >>>When PSIs are for invalidation, rather than newly mapped entries. In
> >>>that case, notify_unmap will be true, and here we need to notify
> >>>IOMMU_NONE to do the cache flush or unmap.
> >>>
> >>>(this page walk is not only for replaying, it is for cache flushing as
> >>>  well)
> >>>
> >>>Do you have suggestion on the comments?
> >>I think then we'd better move this to patch 18 which will use notify_unmap.
> >Do you mean to add some more comment on patch 18?
> >
> 
> I mean move the unmap_nofity and its comment to patch 18 (real user). But if
> you want to keep it in the patch, I'm also fine.

(Since we discussed in IRC for this :)

So I'll keep it for this time. Thanks,

-- peterx