Re: [Qemu-devel] [RFC v2 4/4] spec/vhost-user spec: Add IOMMU support

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

On 04/17/2017 05:50 AM, Peter Xu wrote:
> On Fri, Apr 14, 2017 at 07:40:56PM +0200, Maxime Coquelin wrote:
> 
> [...]
> 
>> +IOMMU support
>> +-------------
>> +
>> +When the VIRTIO_F_IOMMU_PLATFORM feature has been negotiated, the master has
>> +to send IOTLB entries update & invalidation by sending VHOST_USER_IOTLB_MSG
>> +requests to the slave with a struct vhost_iotlb_msg payload. For update events,
>> +the iotlb payload has to be filled with the update message type (2), the I/O
>> +virtual address, the size, the user virtual address, and the permissions
>> +flags. For invalidation events, the iotlb payload has to be filled with the
>> +update message type (3), the I/O virtual address and the size. On success, the
> 
> s/update/invalidate/ again?

Oh, sorry, I missed to fix it last time.

>> +slave is expected to reply with a zero payload, non-zero otherwise.
>> +
>> +When the VHOST_USER_PROTOCOL_F_SLAVE_REQ is supported by the slave, and the
>> +master initiated the slave to master communication channel using the
>> +VHOST_USER_SET_SLAVE_REQ_FD request, the slave can send IOTLB miss and access
>> +failure events by sending VHOST_USER_SLAVE_IOTLB_MSG requests to the master
>> +with a struct vhost_iotlb_msg payload. For miss events, the iotlb payload has
>> +to be filled with the miss message type (1), the I/O virtual address and the
>> +permissions flags. For access failure event, the iotlb payload has to be
>> +filled with the access failure message type (4), the I/O virtual address and
>> +the permissions flags. For synchronization purpose, the slave may rely on the
>> +reply-ack feature, so the master may send a reply when operation is completed
>> +if the reply-ack feature is negotiated and slaves requests a reply.
>> +
>>   Slave communication
>>   -------------------
>>   
>> @@ -512,6 +554,38 @@ Master message types
>>         has been negotiated, and protocol feature bit VHOST_USER_PROTOCOL_F_SLAVE_REQ
>>         bit is present in VHOST_USER_GET_PROTOCOL_FEATURES.
>>   
>> + * VHOST_USER_IOTLB_MSG
>> +
>> +      Id: 22
>> +      Equivalent ioctl: N/A (equivalent to VHOST_IOTLB_MSG message type)
>> +      Master payload: struct vhost_iotlb_msg
>> +      Slave payload: u64
>> +
>> +      Send IOTLB messages with struct vhost_iotlb_msg as payload.
>> +      Master sends such requests to update and invalidate entries in the device
>> +      IOTLB. The slave has to acknowledge the request with sending zero as u64
>> +      payload for success, non-zero otherwise.
>> +      This request should be send only when VIRTIO_F_IOMMU_PLATFORM feature
>> +      has been successfully negotiated.
>> +
>> +Slave message types
>> +-------------------
>> +
>> + * VHOST_USER_SLAVE_IOTLB_MSG
>> +
>> +      Id: 1
>> +      Equivalent ioctl: N/A (equivalent to VHOST_IOTLB_MSG message type)
>> +      Slave payload: struct vhost_iotlb_msg
>> +      Master payload: N/A
> 
> Master payload should be u64 due to REPLY_ACK?

Not sure it should be specified, as this is an optional reply in this
case. Or it should be "u64 if REPLY_ACK".

> A comment regarding to this whole patch - I see that there will be
> lots of things in common between vhost-kernel and vhost-user iotlb
> support at least in this patch. Would it be nice that we consider to
> leverage shared codes with vhost-kernel? I see the most difference is
> the channel (one using vhost fd, one using socket pair), but the
> protocol and logic should merely the same after all. Not sure whether
> we can just abstract the channel handling out of the logic (e.g., on
> how to read/write to the channel, and how to deal with reply_ack).

Right, I think we can go further in sharing code between backends.

Thanks for the review,
Maxime