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

[Jason Wang]

On 2017年01月22日 16:51, Peter Xu wrote:
> On Sun, Jan 22, 2017 at 03:56:10PM +0800, Jason Wang wrote:
>
> [...]
>
> > > +/**
> > > + * vtd_page_walk_level - walk over specific level for IOVA range
> > > + *
> > > + * @addr: base GPA addr to start the walk
> > > + * @start: IOVA range start address
> > > + * @end: IOVA range end address (start <= addr < end)
> > > + * @hook_fn: hook func to be called when detected page
> > > + * @private: private data to be passed into hook func
> > > + * @read: whether parent level has read permission
> > > + * @write: whether parent level has write permission
> > > + * @skipped: accumulated skipped ranges
> > What's the usage for this parameter? Looks like it was never used in this
> > series.
> This was for debugging purpose before, and I kept it in case one day
> it can be used again, considering that will not affect much on the
> overall performance.

I think we usually do not keep debugging codes outside debug macros.

> > > + * @notify_unmap: whether we should notify invalid entries
> > > + */
> > > +static int vtd_page_walk_level(dma_addr_t addr, uint64_t start,
> > > +                               uint64_t end, vtd_page_walk_hook hook_fn,
> > > +                               void *private, uint32_t level,
> > > +                               bool read, bool write, uint64_t *skipped,
> > > +                               bool notify_unmap)
> > > +{
> > > +    bool read_cur, write_cur, entry_valid;
> > > +    uint32_t offset;
> > > +    uint64_t slpte;
> > > +    uint64_t subpage_size, subpage_mask;
> > > +    IOMMUTLBEntry entry;
> > > +    uint64_t iova = start;
> > > +    uint64_t iova_next;
> > > +    uint64_t skipped_local = 0;
> > > +    int ret = 0;
> > > +
> > > +    trace_vtd_page_walk_level(addr, level, start, end);
> > > +
> > > +    subpage_size = 1ULL << vtd_slpt_level_shift(level);
> > > +    subpage_mask = vtd_slpt_level_page_mask(level);
> > > +
> > > +    while (iova < end) {
> > > +        iova_next = (iova & subpage_mask) + subpage_size;
> > > +
> > > +        offset = vtd_iova_level_offset(iova, level);
> > > +        slpte = vtd_get_slpte(addr, offset);
> > > +
> > > +        /*
> > > +         * When one of the following case happens, we assume the whole
> > > +         * range is invalid:
> > > +         *
> > > +         * 1. read block failed
> > Don't get the meaning (and don't see any code relate to this comment).
> I took above vtd_get_slpte() a "read", so I was trying to mean that we
> failed to read the SLPTE due to some reason, we assume the range is
> invalid.

I see, so we'd better move the comment above of vtd_get_slpte().

> > > +         * 3. reserved area non-zero
> > > +         * 2. both read & write flag are not set
> > Should be 1,2,3? And the above comment is conflict with the code at least
> > when notify_unmap is true.
> Yes, okay I don't know why 3 jumped. :(
>
> And yes, I should mention that "both read & write flag not set" only
> suites for page tables here.
>
> > > +         */
> > > +
> > > +        if (slpte == (uint64_t)-1) {
> > If this is true, vtd_slpte_nonzero_rsvd(slpte) should be true too I think?
> Yes, but we are doing two checks here:
>
> - checking against -1 to make sure slpte read success
> - checking against nonzero reserved fields to make sure it follows spec
>
> Imho we should not skip the first check here, only if one day removing
> this may really matter (e.g., for performance reason? I cannot think
> of one yet).

Ok. (return -1 seems not good, but we can address this in the future).

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

> > > +            trace_vtd_page_walk_one(level, entry.iova, entry.translated_addr,
> > > +                                    entry.addr_mask, entry.perm);
> > > +            if (hook_fn) {
> > > +                ret = hook_fn(&entry, private);
> > For better performance, we could try to merge adjacent mappings here. I
> > think both vfio and vhost support this and it can save a lot of ioctls.
> Looks so, and this is in my todo list.
>
> Do you mind I do it later after this series merged? I would really
> appreciate if we can have this codes settled down first (considering
> that this series has been dangling for half a year, or more, startint
> from Aviv's series), and I am just afraid this will led to
> unconvergence of this series (and I believe there are other places
> that can be enhanced in the future as well).

Yes, let's do it on top.

> > > +                if (ret < 0) {
> > > +                    error_report("Detected error in page walk hook "
> > > +                                 "function, stop walk.");
> > > +                    return ret;
> > > +                }
> > > +            }
> > > +        } else {
> > > +            if (!entry_valid) {
> > > +                trace_vtd_page_walk_skip_perm(iova, iova_next);
> > > +                skipped_local++;
> > > +                goto next;
> > > +            }
> > > +            trace_vtd_page_walk_level(vtd_get_slpte_addr(slpte), level - 1,
> > > +                                      iova, MIN(iova_next, end));
> > This looks duplicated?
> I suppose not. The level is different.

Seem not? level - 1 was passed to vtd_page_walk_level() as level which did:

trace_vtd_page_walk_level(addr, level, start, end);


> > > +            ret = vtd_page_walk_level(vtd_get_slpte_addr(slpte), iova,
> > > +                                      MIN(iova_next, end), hook_fn, private,
> > > +                                      level - 1, read_cur, write_cur,
> > > +                                      &skipped_local, notify_unmap);
> > > +            if (ret < 0) {
> > > +                error_report("Detected page walk error on addr 0x%"PRIx64
> > > +                             " level %"PRIu32", stop walk.", addr, level - 1);
> > Guest triggered, so better use debug macro or tracepoint.
> Sorry. Will replace all the error_report() in the whole series.
>
> > > +                return ret;
> > > +            }
> > > +        }
> > > +
> > > +next:
> > > +        iova = iova_next;
> > > +    }
> > > +
> > > +    if (skipped) {
> > > +        *skipped += skipped_local;
> > > +    }
> > > +
> > > +    return 0;
> > > +}
> > > +
> > > +/**
> > > + * vtd_page_walk - walk specific IOVA range, and call the hook
> > > + *
> > > + * @ce: context entry to walk upon
> > > + * @start: IOVA address to start the walk
> > > + * @end: IOVA range end address (start <= addr < end)
> > > + * @hook_fn: the hook that to be called for each detected area
> > > + * @private: private data for the hook function
> > > + */
> > > +static int vtd_page_walk(VTDContextEntry *ce, uint64_t start, uint64_t end,
> > > +                         vtd_page_walk_hook hook_fn, void *private)
> > > +{
> > > +    dma_addr_t addr = vtd_get_slpt_base_from_context(ce);
> > > +    uint32_t level = vtd_get_level_from_context_entry(ce);
> > > +
> > > +    if (!vtd_iova_range_check(start, ce)) {
> > > +        error_report("IOVA start 0x%"PRIx64 " end 0x%"PRIx64" exceeds limits",
> > > +                     start, end);
> > Guest triggered, better use debug macro or tracepoint.
> Same.
>
> > > +        return -VTD_FR_ADDR_BEYOND_MGAW;
> > > +    }
> > > +
> > > +    if (!vtd_iova_range_check(end, ce)) {
> > > +        /* Fix end so that it reaches the maximum */
> > > +        end = vtd_iova_limit(ce);
> > > +    }
> > > +
> > > +    trace_vtd_page_walk_level(addr, level, start, end);
> > Duplicated with the tracepoint in vtd_page_walk_level() too?
> Nop? This should be the top level.
>
> > > +
> > > +    return vtd_page_walk_level(addr, start, end, hook_fn, private,
> > > +                               level, true, true, NULL, false);
> > > +}
> > > +
> > >   /* Map a device to its corresponding domain (context-entry) */
> > >   static int vtd_dev_to_context_entry(IntelIOMMUState *s, uint8_t bus_num,
> > >                                       uint8_t devfn, VTDContextEntry *ce)
> > > @@ -2395,6 +2569,37 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, 
> > > PCIBus *bus, int devfn)
> > >       return vtd_dev_as;
> > >   }
> > > +static int vtd_replay_hook(IOMMUTLBEntry *entry, void *private)
> > > +{
> > > +    memory_region_notify_one((IOMMUNotifier *)private, entry);
> > > +    return 0;
> > > +}
> > > +
> > > +static void vtd_iommu_replay(MemoryRegion *mr, IOMMUNotifier *n)
> > > +{
> > > +    VTDAddressSpace *vtd_as = container_of(mr, VTDAddressSpace, iommu);
> > > +    IntelIOMMUState *s = vtd_as->iommu_state;
> > > +    uint8_t bus_n = pci_bus_num(vtd_as->bus);
> > > +    VTDContextEntry ce;
> > > +
> > > +    if (vtd_dev_to_context_entry(s, bus_n, vtd_as->devfn, &ce) == 0) {
> > > +        /*
> > > +         * Scanned a valid context entry, walk over the pages and
> > > +         * notify when needed.
> > > +         */
> > > +        trace_vtd_replay_ce_valid(bus_n, PCI_SLOT(vtd_as->devfn),
> > > +                                  PCI_FUNC(vtd_as->devfn),
> > > +                                  VTD_CONTEXT_ENTRY_DID(ce.hi),
> > > +                                  ce.hi, ce.lo);
> > > +        vtd_page_walk(&ce, 0, ~0, vtd_replay_hook, (void *)n);
> > ~0ULL?
> Fixing up.
>
> Thanks,
>
> -- peterx