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

[Jason Wang <jasowang@redhat.com>]

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?

Thanks