On Mon, 23 Jan 2017 18:23:44 +0800
Jason Wang <jasow...@redhat.com> wrote:
> On 2017年01月23日 11:34, Peter Xu wrote:
> > On Mon, Jan 23, 2017 at 09:55:39AM +0800, Jason Wang wrote:
> >>
> >> On 2017年01月22日 17:04, Peter Xu wrote:
> >>> On Sun, Jan 22, 2017 at 04:08:04PM +0800, Jason Wang wrote:
> >>>
> >>> [...]
> >>>
> >>>>> +static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s,
> >>>>> + uint16_t domain_id, hwaddr
> >>>>> addr,
> >>>>> + uint8_t am)
> >>>>> +{
> >>>>> + IntelIOMMUNotifierNode *node;
> >>>>> + VTDContextEntry ce;
> >>>>> + int ret;
> >>>>> +
> >>>>> + QLIST_FOREACH(node, &(s->notifiers_list), next) {
> >>>>> + VTDAddressSpace *vtd_as = node->vtd_as;
> >>>>> + ret = vtd_dev_to_context_entry(s, pci_bus_num(vtd_as->bus),
> >>>>> + vtd_as->devfn, &ce);
> >>>>> + if (!ret && domain_id == VTD_CONTEXT_ENTRY_DID(ce.hi)) {
> >>>>> + vtd_page_walk(&ce, addr, addr + (1 << am) * VTD_PAGE_SIZE,
> >>>>> + vtd_page_invalidate_notify_hook,
> >>>>> + (void *)&vtd_as->iommu, true);
> >>>> Why not simply trigger the notifier here? (or is this vfio required?)
> >>> Because we may only want to notify part of the region - we are with
> >>> mask here, but not exact size.
> >>>
> >>> Consider this: guest (with caching mode) maps 12K memory (4K*3 pages),
> >>> the mask will be extended to 16K in the guest. In that case, we need
> >>> to explicitly go over the page entry to know that the 4th page should
> >>> not be notified.
> >> I see. Then it was required by vfio only, I think we can add a fast path
> >> for
> >> !CM in this case by triggering the notifier directly.
> > I noted this down (to be further investigated in my todo), but I don't
> > know whether this can work, due to the fact that I think it is still
> > legal that guest merge more than one PSIs into one. For example, I
> > don't know whether below is legal:
> >
> > - guest invalidate page (0, 4k)
> > - guest map new page (4k, 8k)
> > - guest send single PSI of (0, 8k)
> >
> > In that case, it contains both map/unmap, and looks like it didn't
> > disobay the spec as well?
>
> Not sure I get your meaning, you mean just send single PSI instead of two?
>
> >
> >> Another possible issue is, consider (with CM) a 16K contiguous iova with
> >> the
> >> last page has already been mapped. In this case, if we want to map first
> >> three pages, when handling IOTLB invalidation, am would be 16K, then the
> >> last page will be mapped twice. Can this lead some issue?
> > I don't know whether guest has special handling of this kind of
> > request.
>
> This seems quite usual I think? E.g iommu_flush_iotlb_psi() did:
>
> static void iommu_flush_iotlb_psi(struct intel_iommu *iommu,
> struct dmar_domain *domain,
> unsigned long pfn, unsigned int pages,
> int ih, int map)
> {
> unsigned int mask = ilog2(__roundup_pow_of_two(pages));
> uint64_t addr = (uint64_t)pfn << VTD_PAGE_SHIFT;
> u16 did = domain->iommu_did[iommu->seq_id];
> ...
>
>
> >
> > Besides, imho to completely solve this problem, we still need that
> > per-domain tree. Considering that currently the tree is inside vfio, I
> > see this not a big issue as well.
>
> Another issue I found is: with this series, VFIO_IOMMU_MAP_DMA seems
> become guest trigger-able. And since VFIO allocate its own structure to
> record dma mapping, this seems open a window for evil guest to exhaust
> host memory which is even worse.
You're thinking of pci-assign, vfio does page accounting such that a
user can only lock pages up to their locked memory limit. Exposing the
mapping ioctl within the guest is not a different problem from exposing
the ioctl to the host user from a vfio perspective. Thanks,
Alex
