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

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

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.

>> +   User address: a 64-bit user address
>> +   Permissions flags: a 8-bit bit field:
>> +    - Bit 0: Read access
>> +    - Bit 1: Write access
> 
> Can both bits be set? Can none?

Both. I will change it by listing values directly:
  - 0 : No access
  - 1 : Read
  - 2 : Write
  - 3 : Read Write

>> +   Type: a 8-bit IOTLB message type:
>> +    - 1: IOTLB miss
>> +    - 2: IOTLB update
>> +    - 3: IOTLB invalidate
>> +    - 4: IOTLB access fail
>> +
>>   In QEMU the vhost-user message is implemented with the following struct:
>>   
>>   typedef struct VhostUserMsg {
>> @@ -109,6 +126,7 @@ typedef struct VhostUserMsg {
>>           struct vhost_vring_addr addr;
>>           VhostUserMemory memory;
>>           VhostUserLog log;
>> +        struct vhost_iotlb_msg iotlb;
>>       };
>>   } QEMU_PACKED VhostUserMsg;
>>   
>> @@ -253,6 +271,31 @@ Once the source has finished migration, rings will be stopped by
>>   the source. No further update must be done before rings are
>>   restarted.
>>   
>> +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.
> 
> Always? This seems a bit strange since iommu can be enabled/disabled
> dynamically.
Ok, what about:

When the VIRTIO_F_IOMMU_PLATFORM feature has been negotiated and iommu
is enbaled, the master sends IOTLB entries update & invalidation via
VHOST_USER_IOTLB_MSG requests to the slave with a struct vhost_iotlb_msg
payload.


> Closing channel seems like a wrong thing to do for this.

Sorry, I'm not sure to get your comment.

>> 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
>> +invalidation message type (3), the I/O virtual address and the size. On
>> +success, the 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
>>   -------------------
>>   
>> @@ -514,6 +557,38 @@ Master message types
>>         If VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated, slave must respond
>>         with zero for success, non-zero otherwise.
>>   
>> + * 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
>> +
>> +      Send IOTLB messages with struct vhost_iotlb_msg as payload.
>> +      Slave sends such requests to notify of an IOTLB miss, or an IOTLB
>> +      access failure. If VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated,
>> +      and slave set the VHOST_USER_NEED_REPLY flag, master must respond with
>> +      zero when operation is successfully completed, or non-zero otherwise.
>> +      This request should be send only when VIRTIO_F_IOMMU_PLATFORM feature
>> +      has been successfully negotiated.
>> +
> 
> Are there limitations on number of messages in flight?

I didn't think about this, I would say the maximum number of messages in
flight is dependent on the socket buffer size (which is kept to default
in this series).

You question highlights a bug in by DPDK prototype, as the MISS request
can be sent by multiple threads, and I didn't protected this with a lock
to prevent concurrent read on the socket when waiting for the REPLY_ACK.

Thanks,
Maxime