Re: [Qemu-devel] [PATCH RFC v4 18/20] intel_iommu: enable vfio devices

[Alex Williamson]

On Mon, 23 Jan 2017 18:23:44 +0800
Jason Wang <address@hidden> 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