[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: RE: [EXT] [PATCH v9 04/13] admin-cmds-capabilities: Add capabilities admin commands
> From: Satananda Burla <sburla@marvell.com>
> Sent: Thursday, February 15, 2024 2:55 AM
>
> > -----Original Message-----
> > From: Parav Pandit <parav@nvidia.com>
> > Sent: Monday, February 12, 2024 8:23 PM
> > To: Satananda Burla <sburla@marvell.com>; virtio-comment@lists.oasis-
> > open.org; mst@redhat.com; cohuck@redhat.com
> > Cc: hengqi@linux.alibaba.com; Shahaf Shuler <shahafs@nvidia.com>; si-
> > wei.liu@oracle.com; peter.hilber@opensynergy.com;
> jasowang@redhat.com;
> > xuanzhuo@linux.alibaba.com
> > Subject: RE: [EXT] [PATCH v9 04/13] admin-cmds-capabilities: Add
> > capabilities admin commands
> >
> >
> >
> > > From: Satananda Burla <sburla@marvell.com>
> > > Sent: Tuesday, February 13, 2024 12:21 AM
> > >
> > > Hi Parav
> > >
> > > > -----Original Message-----
> > > > From: Parav Pandit <parav@nvidia.com>
> > > > Sent: Sunday, February 11, 2024 7:42 PM
> > > > To: Satananda Burla <sburla@marvell.com>; virtio-
> > comment@lists.oasis-
> > > > open.org; mst@redhat.com; cohuck@redhat.com
> > > > Cc: hengqi@linux.alibaba.com; Shahaf Shuler <shahafs@nvidia.com>;
> > si-
> > > > wei.liu@oracle.com; peter.hilber@opensynergy.com;
> > > jasowang@redhat.com;
> > > > xuanzhuo@linux.alibaba.com
> > > > Subject: RE: [EXT] [PATCH v9 04/13] admin-cmds-capabilities: Add
> > > > capabilities admin commands
> > > >
> > > > Hi Satananda,
> > > >
> > > > > From: Satananda Burla <sburla@marvell.com>
> > > > > Sent: Monday, February 12, 2024 12:51 AM
> > > > >
> > > > > Hi Parav
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Parav Pandit <parav@nvidia.com>
> > > > > > Sent: Tuesday, February 6, 2024 11:23 PM
> > > > > > To: virtio-comment@lists.oasis-open.org; mst@redhat.com;
> > > > > > cohuck@redhat.com
> > > > > > Cc: hengqi@linux.alibaba.com; Satananda Burla
> > > > > > <sburla@marvell.com>; shahafs@nvidia.com;
> > > > > > si-wei.liu@oracle.com; peter.hilber@opensynergy.com;
> > > > > > jasowang@redhat.com; xuanzhuo@linux.alibaba.com; Parav Pandit
> > > > > > <parav@nvidia.com>
> > > > > > Subject: [EXT] [PATCH v9 04/13] admin-cmds-capabilities: Add
> > > > > > capabilities admin commands
> > > > > >
> > > > > > External Email
> > > > > >
> > > > > > --------------------------------------------------------------
> > > > > > --
> > --
> > > > > > --
> > > > --
> > > > > > Add three capabilities related commands.
> > > > > > First to read the device and driver capabilities.
> > > > > > Second to write the driver capabilities.
> > > > > > Third for driver to discover which capabilities can be accessed.
> > > > > >
> > > > > > Write capabilities by the owner driver is not currently
> > supported.
> > > > > > It will be supported in future by the owner driver to write
> > > > > > for the member device.
> > > > > >
> > > > > > Fixes: https://urldefense.proofpoint.com/v2/url?u=https-
> > > > > > 3A__github.com_oasis-2Dtcs_virtio-
> > > > > >
> > > > >
> > >
> 2Dspec_issues_179&d=DwIDAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=NHDPsfcAY
> > > > > lN2z-
> > > > > > NXHHG4WB09qqS0voo_nf6_kGS625A&m=KI-
> > > > > >
> > > > >
> > >
> QceHuScbIZDc_Kxteq_nU775BHpY5uMRJiwQuxhrgFOpnAx0SON8L2UXPJqty&
> > > > > s=jnFn-
> > > > > > DDDK_3FuIG5OPXgWud35KVh8KaeUDAro8N3jqU&e=
> > > > > > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > > > > > ---
> > > > > > admin-cmds-capabilities.tex | 161
> > > > > ++++++++++++++++++++++++++++++++++++
> > > > > > admin.tex | 9 +-
> > > > > > capabilities.tex | 3 +
> > > > > > conformance.tex | 2 +
> > > > > > 4 files changed, 174 insertions(+), 1 deletion(-) create
> > > > > > mode
> > > > 100644
> > > > > > admin-cmds-capabilities.tex
> > > > > >
> > > > > > diff --git a/admin-cmds-capabilities.tex b/admin-cmds-
> > > > capabilities.tex
> > > > > > new file mode 100644 index 0000000..47394c7
> > > > > > --- /dev/null
> > > > > > +++ b/admin-cmds-capabilities.tex
> > > > > > @@ -0,0 +1,161 @@
> > > > > > +\subsubsection{Device and Driver
> > > > > > +Capabilities}\label{sec:Basic
> > > > > > Facilities of a Virtio Device / Device groups / Group
> > > > > > administration commands / Device and Driver Capabilities}
> > > > > > +
> > > > > > +The device and driver capabilities commands are currently
> > defined
> > > > for
> > > > > > self group type and for
> > > > > > +SR-IOV group type.
> > > > > > +
> > > > > > +\begin{enumerate}
> > > > > > +\item Device Capabilities Support Query Command \item
> > > > > > +Capabilities Read Command \item Capabilities Write Command
> > > > > > +\end{enumerate}
> > > > > > +
> > > > > > +\paragraph{Device Capabilities Support Query
> > > > Command}\label{par:Basic
> > > > > > Facilities of a Virtio Device / Device groups / Group
> > > > > > administration commands / Device and Driver Capabilities /
> > Device
> > > > > > Capabilities Support Query Command}
> > > > > > +
> > > > > > +For the command VIRTIO_ADMIN_CMD_CAP_SUPPORT_QUERY,
> > > > > \field{opcode} is
> > > > > > set to 0x7.
> > > > > > +
> > > > > > +This command queries the supported device capabilities
> > > > > > +identifiers
> > > > > > bitmap in
> > > > > > +\field{supported_caps}. Each bit in \field{supported_caps}
> > > > > > +corresponds
> > > > > > to the
> > > > > > +capability identifier. Thus, supported_caps[0] refers to the
> > > > > > +first
> > > > > > +64-
> > > > > > bit value
> > > > > > +in this array corresponding to identifiers 0 to 63,
> > > > supported_caps[1]
> > > > > > +is the second 64-bit value corresponding to identifiers 64 to
> > > > > > +127,
> > > > etc.
> > > > > > +For example, with \field{num_caps = 2}, the array of size 2
> > > > including
> > > > > > the values
> > > > > > +0x3 in supported_caps[0], and 0x1 in supported_caps[1]
> > indicates
> > > > that
> > > > > > only
> > > > > > +identifiers 0, 1 and 64 are supported.
> > > > > > +
> > > > > > +The length of the array \field{supported_caps} depends on the
> > > > > > +supported identifiers - it is large enough to include bits
> > > > > > +set for all supported identifiers, that is the length is set
> > > > > > +by calculating by starting with
> > > > > > the
> > > > > > +largest supported opcode adding one, dividing by 64 and
> > rounding
> > > > up.
> > > > > > +
> > > > > > +The array \field{supported_caps} is also allowed to be larger
> > and
> > > > to
> > > > > > +additionally include an arbitrary number of all-zero entries.
> > > > > > +
> > > > > > +For each identifier, the driver can read the device
> > capabilities
> > > > and
> > > > > > write
> > > > > > +the driver capabilities. The driver can also read the written
> > > > > > capabilities.
> > > > > > +
> > > > > > +This command has no command specific data.
> > > > > > +
> > > > > > +\begin{lstlisting}
> > > > > > +struct virtio_admin_cmd_query_supported_cap_result {
> > > > > > + le16 num_caps;
> > > > > > + u8 reserved[6];
> > > > > > + le64 supported_caps[]; }; \end{lstlisting}
> > > > > Since this is not fixed size, what happens if the allocated
> > > > > write
> > > > buffer for the
> > > > > command cannot fit the result ?
> > > > The device returns only the capabilities worth of supplied buffer.
> > > >
> > > > > Does the device return error with specific error code?
> > > > No. it truncates to the supplied length.
> > > > This command relies on the existing admin cmd spec.
> > > > Please see the snippet from the spec.
> > > >
> > > > "Drivers are allowed to submit buffers that are shorter than what
> > the
> > > > device expects (that is, shorter than the length of status through
> > > > command_specific_result). This allows the device to maintain a
> > single
> > > > format structure even if some structure fields are unused by the
> > > > driver.
> > > > The device compares the length of each part (device-readable and
> > > > device-
> > > > writeable) of the buffer as submitted by driver to what it expects
> > > > and then silently truncates the structures to either the length
> > > > submitted by the driver, or the
> > length
> > > > described in this specification, whichever is shorter."
> > > But how would driver know if it has read in all the capabilities or
> > not ?
> >
> > Lets take example:
> > The driver interested in capability identifier=A, the driver's view of
> > capability struct = X bytes.
> > The device view of the capability = Y bytes.
> >
> > Example_1:
> > Typically, old driver, new device.
> > X < Y.
> > Result: Device copies X bytes. So driver will see only capabilities
> > field worth of X bytes.
> >
> > Example_2:
> > Typically, new driver, old device.
> > X > Y.
> > Result: Device copies Y bytes because device only understands Y bytes .
> > So driver will see only capabilities field worth of only Y bytes from
> > the X bytes it allocated.
> >
> So this means that the driver needs to allocate for just enough device
> writable buffer that can cover for the largest capability id that the driver
> cares about.
Uhmm, no.
Each capability is read individually. A capability consists of multiple fields.
Please refer to flow filter two capabilities added at [1].
[1] https://lists.oasis-open.org/archives/virtio-comment/202402/msg00027.html
So the driver which is interested in flow filter cap, will read these two, and not with the largest id.
The buffer should be just worth of the individual capability id for example of [2],
Driver will reach each capability individually for id FF_RESOURCE_CAP and id, FF_MATCH_CAP.
Capabilities struct is not suggested to expand in size. It is recommended in [2].
But in case if we want to relax this also in future, it should be fine as admin commands can have the notion of backward compat structures.
[2] https://lists.oasis-open.org/archives/virtio-comment/202402/msg00020.html
> The phrase
> "It is large enough to include bits set for all supported identifiers"
> was what threw me off. "for all supported identifiers" in the above means
> identifiers supported by driver and not device.
>
Above cited text is only applicable to LIST_QUERY and LIST_USE set of commands.
> Regards
> Satananda
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]