Re: [PATCH v5] introduce vfio-user protocol specification

[John Levon]

On Wed, Oct 28, 2020 at 04:41:31PM +0000, Thanos Makatos wrote:

> FYI here's v5 of the vfio-user protocol, my --cc in git send-email got messed 
> up somehow

Hi Thanos, this looks great, I just had some minor questions below.

> Command Concurrency
> -------------------
> A client may pipeline multiple commands without waiting for previous command
> replies.  The server will process commands in the order they are received.
> A consequence of this is if a client issues a command with the *No_reply* bit,
> then subseqently issues a command without *No_reply*, the older command will
> have been processed before the reply to the younger command is sent by the
> server.  The client must be aware of the device's capability to process 
> concurrent
> commands if pipelining is used.  For example, pipelining allows multiple 
> client
> threads to concurently access device memory; the client must ensure these 
> acceses
> obey device semantics.
> 
> An example is a frame buffer device, where the device may allow concurrent 
> access
> to different areas of video memory, but may have indeterminate behavior if 
> concurrent
> acceses are performed to command or status registers.

Is it valid for an unrelated server->client message to appear in between a
client->server request/reply, or not? And vice versa? Either way, seems useful
for the spec to say.

> |                | +-----+------------+ |
> |                | | Bit | Definition | |
> |                | +=====+============+ |
> |                | | 0-3 | Type       | |
> |                | +-----+------------+ |
> |                | | 4   | No_reply   | |
> |                | +-----+------------+ |
> |                | | 5   | Error      | |
> |                | +-----+------------+ |
> +----------------+--------+-------------+
> | Error          | 12     | 4           |
> +----------------+--------+-------------+
> 
> * *Message ID* identifies the message, and is echoed in the command's reply 
> message.

Is it valid to re-use an ID? When/when not?

>   * *Error* in a reply message indicates the command being acknowledged had
>     an error. In this case, the *Error* field will be valid.
> 
> * *Error* in a reply message is a UNIX errno value. It is reserved in a 
> command message.

I'm not quite following why we need a bit flag and an error field. Do you
anticipate a failure, but with errno==0?

> VFIO_USER_VERSION
> -----------------
> 
> +--------------+------------------------+
> | Message size | 16 + version length    |

Terminating NUL included?

> +--------------+--------+---------------------------------------------------+
> | Name         | Type   | Description                                       |
> +==============+========+===================================================+
> | version      | object | ``{"major": <number>, "minor": <number>}``        |
> |              |        |                                                   |
> |              |        | Version supported by the sender, e.g. "0.1".      |

It seems quite unlikely but this should specify it's strings not floating point
values maybe?

Definitely applies to max_fds too.

> Common capabilities:
> 
> +---------------+------------------------------------------------------------+
> | Name          | Description                                                |
> +===============+============================================================+
> | ``max_fds``   | Maximum number of file descriptors that can be received by |
> |               | the sender. Optional.                                      |

Could specify the meaning when absent?

By array I presume you mean associative array i.e. an Object. Does the whole
thing look like this:

 {
   "major": ..
   "minor": ..
   "capabilities": {
      "max_fds": ..,
      "migration
   }
 }

or something else?

> Versioning and Feature Support
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Upon accepting a connection, the server must send a VFIO_USER_VERSION message
> proposing a protocol version and a set of capabilities. The client compares
> these with the versions and capabilities it supports and sends a
> VFIO_USER_VERSION reply according to the following rules.

I'm curious if there was a specific reason it's this way around, when it seems
more natural for the client to propose first, and the server to reply?

> VFIO_USER_DMA_MAP and VFIO_USER_DMA_UNMAP
> -----------------------------------------

Huge nit, but why are these DMA_*MAP when vfio uses *MAP_DMA ?

> VFIO bitmap format
> * *size* the size for the bitmap, in bytes.

Should this clarify it does *not* include the bitmap header in its size, unlike
other size fields?

> VFIO_USER_DMA_MAP
> """""""""""""""""
> If a DMA region being added can be directly mapped by the server, an array of
> file descriptors must be sent as part of the message meta-data. Each region
> entry must have a corresponding file descriptor.

"Each mappable region entry" ?

> descriptors must be passed as SCM_RIGHTS type ancillary data. Otherwise, if a
> DMA region cannot be directly mapped by the server, it can be accessed by the
> server using VFIO_USER_DMA_READ and VFIO_USER_DMA_WRITE messages, explained in
> `Read and Write Operations`_. A command to map over an existing region must be
> failed by the server with ``EEXIST`` set in error field in the reply.
> 
> VFIO_USER_DMA_UNMAP
> """""""""""""""""""
> Upon receiving a VFIO_USER_DMA_UNMAP command, if the file descriptor is mapped
> then the server must release all references to that DMA region before 
> replying,
> which includes potentially in flight DMA transactions. Removing a portion of a
> DMA region is possible. If the VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP bit is set
> in the request, the server must append to the header the ``struct 
> vfio_bitmap``
> received in the command, followed by the bitmap. Thus, the message size the
> client should is expect is the size of the header plus the size of
> ``struct vfio_bitmap`` plus ``vfio_bitmap.size`` bytes. Each bit in the bitmap
> represents one page of size ``vfio_bitmap.pgsize``.

I'm finding this makes the sizing a bit confusing between map and unmap, could
we may be separate them out, and always define a vfio_bitmap slot for unmap?

Also, shouldn't the client expect sizeof (header) + (nr_table_entries_in_request
* (each vfio_bitmap's size))  in the server's response?

Does the reply header size field reflect this?

> VFIO_USER_DMA_WRITE
> -------------------
> 
> This command message is sent from the server to the client to write to server
> memory.

"write to client memory"?

> VFIO_USER_DIRY_PAGES

Nit, "DIRTY"

thanks
john