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
