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

[Maxime Coquelin]

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 <address@hidden>
> > ---
> >   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