Re: [Qemu-devel] [PATCH 6/6] spec/vhost-user spec: Add IOMMU support

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 05/19/2017 08:48 AM, Jason Wang wrote:
> 
> 
> On 2017年05月17日 22:10, Maxime Coquelin wrote:
>>
>>
>> On 05/17/2017 04:53 AM, Jason Wang wrote:
>>>
>>>
>>> On 2017年05月16日 23:16, Michael S. Tsirkin wrote:
>>>> On Mon, May 15, 2017 at 01:45:28PM +0800, Jason Wang wrote:
>>>>>
>>>>> On 2017年05月13日 08:02, Michael S. Tsirkin wrote:
>>>>>> On Fri, May 12, 2017 at 04:21:58PM +0200, Maxime Coquelin wrote:
>>>>>>> On 05/11/2017 08:25 PM, Michael S. Tsirkin wrote:
>>>>>>>> On Thu, May 11, 2017 at 02:32:46PM +0200, Maxime Coquelin wrote:
>>>>>>>>> This patch specifies and implements the master/slave communication
>>>>>>>>> to support device IOTLB in slave.
>>>>>>>>>
>>>>>>>>> The vhost_iotlb_msg structure introduced for kernel backends is
>>>>>>>>> re-used, making the design close between the two backends.
>>>>>>>>>
>>>>>>>>> An exception is the use of the secondary channel to enable the
>>>>>>>>> slave to send IOTLB miss requests to the master.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>>>>>> ---
>>>>>>>>>     docs/specs/vhost-user.txt | 75 
>>>>>>>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>>     hw/virtio/vhost-user.c    | 31 ++++++++++++++++++++
>>>>>>>>>     2 files changed, 106 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
>>>>>>>>> index 5fa7016..4a1f0c3 100644
>>>>>>>>> --- a/docs/specs/vhost-user.txt
>>>>>>>>> +++ b/docs/specs/vhost-user.txt
>>>>>>>>> @@ -97,6 +97,23 @@ Depending on the request type, payload can be:
>>>>>>>>>        log offset: offset from start of supplied file descriptor
>>>>>>>>>            where logging starts (i.e. where guest address 0 
>>>>>>>>> would be logged)
>>>>>>>>> + * An IOTLB message
>>>>>>>>> + ---------------------------------------------------------
>>>>>>>>> +   | iova | size | user address | permissions flags | type |
>>>>>>>>> + ---------------------------------------------------------
>>>>>>>>> +
>>>>>>>>> +   IOVA: a 64-bit guest I/O virtual address
>>>>>>>> guest -> VM
>>>>>>> Ok.
>>>>>>>
>>>>>>>>> +   Size: a 64-bit size
>>>>>>>> How do you specify "all memory"? give special meaning to size 0?
>>>>>>> Good point, it does not support all memory currently.
>>>>>>> It is not vhost-user specific, but general to the vhost 
>>>>>>> implementation.
>>>>>> But iommu needs it to support passthrough.
>>>>> Probably not, we will just pass the mappings in vhost_memory_region to
>>>>> vhost. Its memory_size is also a __u64.
>>>>>
>>>>> Thanks
>>>> That's different since that's chunks of qemu virtual memory.
>>>>
>>>> IOMMU maps IOVA to GPA.
>>>>
>>>
>>> But we're in fact cache IOVA -> HVA mapping in the remote IOTLB. When 
>>> passthrough mode is enabled, IOVA == GPA, so passing mappings in 
>>> vhost_memory_region should be fine.
>>
>> Not sure this is a good idea, because when configured in passthrough,
>> QEMU will see the IOMMU as enabled, so the the VIRTIO_F_IOMMU_PLATFORM
>> feature will be negotiated if both guest and backend support it.
>> So how the backend will know whether it should directly pick the
>> translation directly into the vhost_memory_region, or translate it
>> through the device IOTLB?
> 
> This no need for backend to know about this, since IOVA equals to GPA, 
> vhost_memory_region stores IOVA -> HVA mapping. If we pass them, device 
> IOTLB should work as usual?

Ok, I think there were a misunderstanding. I understood you said there
were no need to use the device IOTLB in this case.

> 
>>
>> Maybe the solution would be for QEMU to wrap "all memory" IOTLB updates
>> & invalidations to vhost_memory_regions, since the backend won't anyway
>> be able to perform accesses outside these regions?
> 
> This is just what I mean, you can refer Peter's series. >
>>
>>> The only possible "issue" with "all memory" is if you can not use a 
>>> single TLB invalidation to invalidate all caches in remote TLB.
>>
>> If needed, maybe we could introduce a new VHOST_IOTLB_INVALIDATE message
>> type?
>> For older kernel backend that doesn't support it, -EINVAL will be
>> returned, so QEMU could handle it another way in this case.
> 
> We could, but not sure it was really needed.

I meant VHOST_IOTLB_INVALIDATE_ALL, and yes, I'm not sure this is
needed. But this is an option we have it turns out to be at some point.

Thanks,
Maxime