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

[Jason Wang <jasowang@redhat.com>]

On 2017年01月25日 09:31, Alex Williamson wrote:
> On Wed, 25 Jan 2017 09:19:25 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> On 2017年01月24日 03:40, Alex Williamson wrote:
>>> On Mon, 23 Jan 2017 18:23:44 +0800
>>> Jason Wang<jasowang@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
>>>   
>> Yes, but what if an evil guest that maps all iovas to the same gpa?
> Doesn't matter, we'd account that gpa each time it's mapped, so
> effectively the locked memory limit is equal to the iova size the user
> can map.  Thanks,
>
> Alex

I see. Good to know this.

Thanks