Re: Re: [PATCH v2 07/11] transport-fabrics: introduce opcodes

[zhenwei pi <pizhenwei@bytedance.com>]

On 6/1/23 04:55, Stefan Hajnoczi wrote:
> On Thu, May 04, 2023 at 04:19:06PM +0800, zhenwei pi wrote:
> > Define opcode with this rule:
> > The Virtio-oF transport layer commands use 0x0000-0x0fff, and the
> > device layer commands use 0x1000-0xffff.
> > get/set status/feature/
> > config use consecutive number.
> >
> > Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> > ---
> >   transport-fabrics.tex | 134 ++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 134 insertions(+)
> >
> > diff --git a/transport-fabrics.tex b/transport-fabrics.tex
> > index 37f57c6..026ff5f 100644
> > --- a/transport-fabrics.tex
> > +++ b/transport-fabrics.tex
> > @@ -704,3 +704,137 @@ \subsubsection{Commands Definition}\label{sec:Virtio Transport Options / Virtio
> >   Note that Virtio Over Fabrics does not define an interrupt mechanism, generally
> >   the initiator receives a Completion, it SHOULD generate a host interrupt
> >   (if no interrupt suspending on device).
> > +
> > +\subsubsection{Opcodes Definition}\label{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Opcodes Definition}
> > +This section defines command opcodes for Virtio Over Fabrics:
> > +
> > +\begin{lstlisting}
> > +#define virtio_of_op_connect               0x0000
> > +#define virtio_of_op_discconnect           0x0001
>
> "disconnect"

OK.

> > +#define virtio_of_op_get_feature           0x0002
> > +#define virtio_of_op_set_feature           0x0003
> > +#define virtio_of_op_keepalive             0x0004
> > +#define virtio_of_op_vring                 0x0fff
> > +#define virtio_of_op_get_vendor_id         0x1000
> > +#define virtio_of_op_get_device_id         0x1001
> > +#define virtio_of_op_get_generation        0x1002
> > +#define virtio_of_op_get_status            0x1004
> > +#define virtio_of_op_set_status            0x1005
> > +#define virtio_of_op_get_device_feature    0x1006
> > +#define virtio_of_op_set_driver_feature    0x1007
> > +#define virtio_of_op_get_num_queues        0x1008
> > +#define virtio_of_op_get_queue_size        0x100a
> > +#define virtio_of_op_get_config            0x100c
> > +#define virtio_of_op_set_config            0x100d
> > +\end{lstlisting}
>
> Is Queue Reset missing?
> https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html#x1-280001

Originally, I designed reset as set_status(0). But an explicit reset 
command is better! Add this in next version.

> > +
> > +\paragraph{virtio_of_op_connect}\label{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Opcodes Definition / virtio_of_op_connect}
> > +
> > +virtio_of_op_connect is used to connect a target for both control queue and virtqueue.
> > +The initiator MUST issue a \nameref{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Commands Definition / Connect Command}
> > +and specify the ndesc field as 1, also contains 1 structure virtio_of_vring_desc
> > +filled by structure virtio_of_command_status.
>
> What are the semantics of this command? Is the idea that the initiatior
> will establish 1 TCP connection to the target for every virtqueue (plus
> one for the control queue) and send a virtio_of_op_connect command as
> the first command in the connection in order to indicate that the
> connection is associated with a specific queue?

Yes.

> Is there a state machine related to connection and queue lifecycles?

No state machine related to connection. The lifecycles of Virtio-oF 
queues: establish a connection(transport specific), issue connect 
command, issue control/vq command ... issue disconnect command(optional, 
gracefully shutdown), disconnection connection.

> > +
> > +\paragraph{virtio_of_op_discconnect}\label{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Opcodes Definition / virtio_of_op_discconnect}
> > +
> > +virtio_of_op_discconnect is used to disconnect a target for both control queue and virtqueue.
> > +The initiator MUST issue a \nameref{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Commands Definition / Common Command}.
>
> What happens if the initiatior drops the connection without sending a
> virtio_of_op_discconnect command?

A Virtio-of queue could shutdown gracefully by issuing 
virtio_of_op_discconnect command. The inflight commands will be 
completed by target.
Otherwise the inflight commands may be dropped by target.

> Are there resources associated with a connected queue?

A control queue disconnects, the initiator and target should shutdown 
all the virtqueues associated with this control queue.

A connected virtqueue associates a QID for both initiator and target 
side, once a Virtio-of queue disconnects, the QID becomes free, allow 
reconnect.

> > +
> > +\paragraph{virtio_of_op_get_feature}\label{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Opcodes Definition / virtio_of_op_get_feature}
> > +
> > +virtio_of_op_get_feature is used to get features of target for control queue only.
>
> Does this command fail when sent to a virtqueue instead of the control
> queue?
>
> By the way, what's the difference between a connection and queue?

I need describe the mapping between a connection and queue in '[PATCH v2 
01/11] transport-fabrics: introduce Virtio Over Fabrics overview', and 
use 'Virtio Over Fabrics queue' only.

> > +The initiator MUST issue a \nameref{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Commands Definition / Feature Command}.
>
> When? As the first message or immediately after virtio_of_op_connect?

At any time theory. As the first message or immediately after 
virtio_of_op_connect is recommended.

> > +
> > +\begin{tabular}{ |l|l|l| }
> > +\hline
> > +Feature Select & Value & Description \\
> > +\hline
> > +virtio_of_feature_max_segment & 0x0 & Get the maximum segments within a Vring Command supported by target \\
>
> Does Vring Command mean virtio_of_op_vring? What is the difference
> between "segments" and "descriptors"?
>
> Does the max_segment value have a type or does the initiator have to
> support up to u64?
>
> How does max_segment affect the maximum number of virtqueue buffer
> elements?
>
> What happens when a feature that is not supported by the target is
> queried by the initiator?
>
> I was expecting a feature bit negotiation mechanism, but it seems the
> "feature" is a parameter value, not just a single but like VIRTIO
> Feature Bits. Please rename this to "parameter", "setting", or similar
> to avoid confusion with Feature Bits.

VIRTIO Feature Bits style is fine. Change into this style in next 
version. Then I'd introduce a new opcode 
virtio_of_op_get_max_segment(required command, no feature bits testing).

> > +\hline
> > +\end{tabular}
> > +
> > +\paragraph{virtio_of_op_set_feature}\label{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Opcodes Definition / virtio_of_op_set_feature}
> > +
> > +virtio_of_op_set_feature is used to set features of initiator for control queue only.
>
> "set features of initiator" sounds like the target uses it to set up the
> initiator, but I think this command is sent from the initiator to the
> target. Maybe:
>
>    "virtio_of_op_set_feature sets feature values in the target and is
>    sent on the control queue."

OK.

> > +The initiator MUST issue a \nameref{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Commands Definition / Feature Command}.
>
> There are currently no features defined that can be set using
> virtio_of_op_set_feature?

'[PATCH v2 11/11] transport-fabrics: support inline data for keyed 
transmission' would be the first bit.

> > +\paragraph{virtio_of_op_keepalive}\label{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Opcodes Definition / virtio_of_op_keepalive}
> > +
> > +virtio_of_op_keepalive is used to keep alive with the target for control queue only.
> > +The initiator MUST issue a \nameref{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Commands Definition / Common Command}.
>
> What is the purpose of this command? It is only sent on the control
> queue so its purpose cannot be to actually keep connections alive since
> virtqueue connections would not stay alive.
>
> Maybe this is really a health check (ping/pong) to detect when the
> control queue becomes unavailable?

Originally I thought that keepalive for control is enough: once the 
control becomes unavailable, shutdown the control queue and all the 
related virtqueues. the virtio_of_op_vring commands detect the virtqueue 
connection implicitly.

This command for both control queue and virtqueue is fine.

> > +\paragraph{}\label{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Opcodes Definition / virtio_of_op_vring}
> > +
> > +virtio_of_op_vring is used to transmit buffer for both control queue and virtqueue.
>
> "buffers"
>
> My understanding is that the control queue is not a virtqueue. How can a
> vring operation make sense on something that is not a virtqueue?

Oh, my fault. this is for virtqueue only. and virtio_of_op_vring should 
be renamed to virtio_of_op_vq.

> I think the term used by the VIRTIO spec (2.6 Virtqueues) is "supply an
> available buffer to the device" rather than "transmit buffer".

OK.

> > +The initiator MUST issues \nameref{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Commands Definition / Vring Command}
>
> "issue"
>
> > +and specify the ndesc field as the number of buffer segments,
>
> buffer segments == data segments == segment descriptor (struct virtio_of_vring_desc)?
>
> Please pick one term and use it consistently.

OK.

> > +also contains ndesc structure virtio_of_vring_desc.
> > +Each structure virtio_of_vring_desc is filled by each buffer segment one by one.
> > +
> > +\paragraph{virtio_of_op_get_vendor_id}\label{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Opcodes Definition / virtio_of_op_get_vendor_id}
> > +
> > +virtio_of_op_get_vendor_id is used to get vendor id for control queue only.
>
> The spec uses slightly more specific terms that avoid confusion with
> other types of device/vendor IDs: either "get the Virtio Vendor ID" or
> "get the Virtio Subsystem Vendor ID".

OK.

> > +The initiator MUST issue a \nameref{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Commands Definition / Common Command},
> > +and reads from value field of Completion as le32.
> > +
> > +\paragraph{virtio_of_op_get_device_id}\label{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Opcodes Definition / virtio_of_op_get_device_id}
> > +
> > +virtio_of_op_get_device_id is used to get device id for control queue only.
>
> "get the Virtio Device ID" or "get the Virtio Subsystem Device ID"

OK.

> > +The initiator MUST issue a \nameref{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Commands Definition / Common Command},
> > +and reads from value field of Completion as le32.
> > +
> > +\paragraph{virtio_of_op_get_generation}\label{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Opcodes Definition / virtio_of_op_get_generation}
> > +
> > +virtio_of_op_get_generation is used to get config generation for control queue only.
> > +The initiator MUST issue a \nameref{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Commands Definition / Common Command},
> > +and reads from value field of Completion as le32.
>
> Maybe every virtio_of_op_get_config completion should include the
> generation counter value. That way fewer roundtrips are required because
> virtio_of_op_get_generation commands are not necessary.
>
> The advantage to virtio_of_op_get_generation is that it maps nicely to
> existing VIRTIO driver frameworks that expect to read the generation
> counter separately, so I guess it's okay to keep it even if it's
> inefficient over a fabric.
>
> We could do both, too.

Great! I'd define a new structure for 
virtio_of_op_get_generation(include generation field) and drop 
virtio_of_op_get_generation.

> > +
> > +\paragraph{virtio_of_op_get_status}\label{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Opcodes Definition / virtio_of_op_get_status}
> > +
> > +virtio_of_op_get_status is used to get device status for control queue only.
> > +The initiator MUST issue a \nameref{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Commands Definition / Common Command},
> > +and reads from value field of Completion as le32.
> > +
> > +\paragraph{virtio_of_op_set_status}\label{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Opcodes Definition / virtio_of_op_set_status}
> > +
> > +virtio_of_op_set_status is used to set device status for control queue only.
> > +The initiator MUST issue a \nameref{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Commands Definition / Common Command},
> > +and specify the value field of Common Command as le32.
> > +
> > +\paragraph{virtio_of_op_get_device_feature}\label{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Opcodes Definition / virtio_of_op_get_device_feature}
> > +
> > +virtio_of_op_get_device_feature is used to get device feature for control queue only.
> > +The initiator MUST issue a \nameref{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Commands Definition / Feature Command},
> > +and reads from value field of Completion as le64.
>
> What happens when feature_select is out of range? I guess
> Completion.value is set to 0.

Yes. But I think the feature_select is always in range, bit64-bit 
127(feature_select == 1) is not offered currently, so 
Completion.value.u64 is 0.

> Does virtio_of_op_get_device_feature return the feature bits offered by
> the device or does it update to reflect negotiated feature bits after
> virtio_of_op_set_driver_feature?

virtio_of_op_get_device_feature returns the same feature bits after 
virtio_of_op_set_driver_feature. Because 1) the device feature is 
capability of device, 2) a target may be shared by multi initiators.

For now, I don't see any dependence on getting driver feature. Do you 
have any concern about this?

> > +
> > +\paragraph{virtio_of_op_set_driver_feature}\label{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Opcodes Definition / virtio_of_op_set_driver_feature}
> > +
> > +virtio_of_op_set_driver_feature is used to set driver feature for control queue only.
> > +The initiator MUST issue a \nameref{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Commands Definition / Feature Command},
> > +and specify the value field of Common Command as le64.
> > +
> > +The initiator uses feature_select field to select which feature bits to set.
> > +Value 0x0 selects Feature Bits 0 to 63, 0x1 selects Feature Bits 64 to 128, etc.
> > +
> > +\paragraph{virtio_of_op_get_num_queues}\label{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Opcodes Definition / virtio_of_op_get_num_queues}
> > +
> > +virtio_of_op_get_num_queues is used to get the number of queues for control queue only.
> > +The initiator MUST issue a \nameref{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Commands Definition / Common Command},
> > +and reads from value field of Completion as le16.
> > +
> > +\paragraph{virtio_of_op_get_queue_size}\label{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Opcodes Definition / virtio_of_op_get_queue_size}
> > +
> > +virtio_of_op_get_queue_size is used to get the size of a specified queue for control queue only.
> > +The initiator MUST issue a \nameref{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Commands Definition / Queue Command} with specified queue_id,
> > +and reads from value field of Completion as le16.
>
> Is it possible to set the queue size? For example, the PCI Transport
> allows the driver to lower the queue size but not increase it (see
> 4.1.5.1.3 Virtqueue Configuration).

Agree. Because a target may be shared by multi initiators, it's not 
reasonable to set queue size of target, the queue size only affect this 
initiator itself.
For example, a target supports queue size 1024. initiatorX uses 128 
queue size, and initiatorY uses 1024. Do you have any suggestion about this?

> > +
> > +\paragraph{virtio_of_op_get_config}\label{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Opcodes Definition / virtio_of_op_get_config}
> > +
> > +virtio_of_op_get_config is used to get the config of a device for control queue only.
> > +The initiator MUST issue a \nameref{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Commands Definition / Config Command} with specified offset and bytes,
> > +and reads from value field of Completion.
> > +
> > +\paragraph{virtio_of_op_set_config}\label{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Opcodes Definition / virtio_of_op_set_config}
> > +
> > +virtio_of_op_set_config is used to set the config of a device for control queue only.
> > +The initiator MUST issue a \nameref{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Commands Definition / Config Command} with specified offset and bytes and value fields.
> > --
> > 2.25.1

--
zhenwei pi