Re: Re: Re: [PATCH v4 4/7] transport-fabrics: introduce command set

[zhenwei pi <pizhenwei@bytedance.com>]

On 6/28/23 10:56, Jason Wang wrote:
> On Wed, Jun 28, 2023 at 9:37âAM zhenwei pi <pizhenwei@bytedance.com> wrote:
> > On 6/27/23 17:04, Jason Wang wrote:
> > > On Mon, Jun 26, 2023 at 3:28âPM zhenwei pi <pizhenwei@bytedance.com> wrote:
> > > > Introduce command set for Virtio-oF which includes:
> > > > - command id: a tag to distinguish in-flight commands
> > > > - status: indicate the result of a Virtio-oF command
> > > > - opcodes: the Virtio-oF transport layer commands use 0x0000-0x0fff,
[snip]
> > > > +\begin{lstlisting}
> > > > +#define virtio_of_op_connect               0x0000
> > > > +#define virtio_of_op_disconnect            0x0001
> > > > +#define virtio_of_op_keepalive             0x0002
> > > > +#define virtio_of_op_get_feature           0x0004
> > > > +#define virtio_of_op_set_feature           0x0005
> > > > +#define virtio_of_op_get_keyed_num_descs   0x0100
> > > > +#define virtio_of_op_vq                    0x0fff
> > > > +#define virtio_of_op_get_vendor_id         0x1000
> > > > +#define virtio_of_op_get_device_id         0x1001
> > > > +#define virtio_of_op_reset_device          0x1003
> > > > +#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    0x1009
> > > > +#define virtio_of_op_get_vq_size           0x100a
> > > > +#define virtio_of_op_get_config            0x100c
> > > > +#define virtio_of_op_set_config            0x100d
> > >
> > > We had a proposal of transport virtqueue that allows using a
> > > virtqueue[1] for configuring a device. And Ling shan will rebase it on
> > > top of the admin commands.
> > >
> > > That is to say, admin commands could be used for transport which will
> > > be duplicated with the commands you invented here.
> > >
> > > I wonder if the virtio-OF can just reuse the admin commands (have a
> > > quick glance and I didn't find any blocker for this).
> > >
> > > Thanks
> > >
> > > [1] https://lists.oasis-open.org/archives/virtio-comment/202208/msg00003.html
> >
> >   From my understand, the virtio device related commands looks
> > similar(set/get status/features, reset device ...), but not same:
> > 1, the admin commands are executed upon the admin virtqueue, it uses
> > virtqueue based transfer.
>
> Note that the actual commands are independent of admin virtqueue. It
> is defined separately as admin commands and it would be supported by
> the MMIO in the future.
>
> > The virtio-of commands are executed upon a virtio-of queue(established
> > reliable connection), not a virtqueue. A virtio-of device could support
> > admin commands in theory.
> > I suppose a virtio-of device with admin vq supported like:
> > +---------------------+
> > |   +-------------+   |
> > |   |control queue|   |
> > |   +-------------+   |
> > +---------------------+
> > |   +-------------+   |
> > |   | virtqueue0  |   |
> > |   +-------------+   |
> > |   | virtqueue1  |   |
> > |   +-------------+   |
> > |   | virtqueue ..|   |
> > |   +-------------+   |
> > |   |  admin vq   |   |
> > |   +-------------+   |
> > +---------------------+
> >
> > The virtio-of commands ares used by both virtio-of control queue and
> > virtio-of virtqueues(include admin vq).
> > The admin commands are used by admin vq only.
> >
> > 2, the virtio-of commands are used over fabrics, they are defined in a
> > fixed size,
>
> Any issue you see if it is variable?

For stream transfer, a command of variable length is difficult to decode.
For keyed transfer, the receiver side must know the size of commands, 
then post recv large enough memory. And unaligned length hurts the 
performance.

> > the admin commands are described by virtqueue descriptors.
>
> See above, it's not necessarily carried via descriptors.
>
> > 3, the virtio-of commands need a command_id field to distinguish each
> > other for command depth, but the admin commands don't need command_id
> > field(supported by virtqueue descriptor heads).
>
> I'm not sure I get this, could you give me an example?

Please see subsubsection{Command ID} in this patch.

> > 4, connect/disconnect are fabrics specific, stream/keyed transfer
> > related features/commands are fabrics specific too.
> > interrupt related commands are not used by virtio-of.
>
> So we had:
>
> struct virtio_admin_cmd {
>          /* Device-readable part */
>          le16 opcode;
>          /*
>           * 1       - SR-IOV
>           * 2-65535 - reserved
>           */
>          le16 group_type;
> ...
> };
>
> The of specific command could have a dedicated group in this case.
>
> Thanks

The command set to virtio-of looks like:
[Common configuration structure layout] to Virtio PCI,
[MMIO Device Register Layout] to Virtio MMIO,
[Channel Commands for Virtio] to Virtio Channel I/O.

By the way, [Channel Commands for Virtio] is already defined, is it 
possible to reuse it?

> > As far as I can see, they are quite different, defining them separately
> > would be fine.
> >
> > --
> > zhenwei pi

--
zhenwei pi