From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 References: <20220812171841.12183-1-mst@redhat.com> <20220812171841.12183-3-mst@redhat.com> <20220818044918-mutt-send-email-mst@kernel.org> In-Reply-To: <20220818044918-mutt-send-email-mst@kernel.org> From: Jason Wang Date: Fri, 19 Aug 2022 08:26:50 +0800 Message-ID: Subject: Re: [PATCH RFC v7 2/8] Introduce group administration commands Content-Type: text/plain; charset="UTF-8" To: "Michael S. Tsirkin" Cc: virtio-comment@lists.oasis-open.org, Virtio-Dev , Cornelia Huck , Stefano Garzarella , Stefan Hajnoczi , nrupal.jani@intel.com, "Uminski, Piotr" , hang.yuan@intel.com, virtio@lists.oasis-open.org, Zhu Lingshan , Oren Duer , Parav Pandit , Shahaf Shuler , Ariel Adam , eperezma , Max Gurtovoy List-ID: On Thu, Aug 18, 2022 at 4:51 PM Michael S. Tsirkin wrote: > > On Thu, Aug 18, 2022 at 04:46:45PM +0800, Jason Wang wrote: > > On Sat, Aug 13, 2022 at 1:19 AM Michael S. Tsirkin wrote: > > > > > > From: Max Gurtovoy > > > > > > These commands are used for essential administrative and management > > > operations. > > > > > > Following patches will introduce an interface for submitting these > > > commands to the device. > > > > > > Signed-off-by: Max Gurtovoy > > > Signed-off-by: Michael S. Tsirkin > > > --- > > > admin.tex | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++--- > > > 1 file changed, 94 insertions(+), 5 deletions(-) > > > > > > diff --git a/admin.tex b/admin.tex > > > index 6e9434a..4840dd4 100644 > > > --- a/admin.tex > > > +++ b/admin.tex > > > @@ -9,8 +9,10 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g > > > \item[Parent device] > > > or parent, the device controlling the group. > > > \item[Member device] > > > - a device within a group. Parent device itself may, or may > > > - not be a member of the group. > > > + a device within a group. Parent device itself is not > > > + a member of the group. In the future it is envisoned that > > > + new group types may be introduced where the parent > > > + device is a member of the group. > > > > This part should go with patch 1? > > > > > \item[Member identifier] > > > each member has this indentifier, unique within the group > > > and used to address it through the parent device. > > > @@ -20,9 +22,10 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g > > > and what kind of control does the parent have. > > > \item[Group identifier] > > > each group has this identifier, unique within the parent device. > > > - this specifies the group type and possibly selects the > > > - group if multiple groups of the same type can be controlled by the same > > > - parent device. > > > + This specifies the group type. In the future it is > > > + envisoned that new group types will be introduced where the group > > > + identifier also selects the group if multiple groups of the same > > > + type can be controlled by the same parent device. > > > \end{description} > > > > > > A single group type is currently specified: > > > @@ -46,4 +49,90 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g > > > PCI transport (see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus}). > > > \end{description} > > > > > > +\subsection{Group administration commands}\label{sec:Basic Facilities of a Virtio Device / Group administration commands} > > > + > > > +Group administration commands can be issued through a parent > > > +device to control member devices of a group. This mechanism can > > > +be used, for example, to configure a member device before it is > > > +initialized by its driver. > > > + > > > +All the group administration commands are of the following form: > > > + > > > +\begin{lstlisting} > > > +struct virtio_admin_cmd { > > > + /* Device-readable part */ > > > + le16 opcode; > > > + /* > > > + * 1 - SR-IOV > > > + * 2 - 65535 reserved > > > + */ > > > + le16 group_id; > > > > I wonder about the implication of this field. Does it mean: > > > > 1) a single admin virtqueue that manage two group of VFs > > > > or > > > > 2) a single admin virtqueue that manages both VFs and SFs (then > > 'group_type' should be better?) > > > > If it's 1, the group id seems not sufficient. If it's 2, I wonder if > > it scales (using a single admin virtqueue to manage both VF and SF). > > > > Thanks > > 2 at the moment. Not sure I understand why wouldn't it scale. Scaling as the number of member devices increases (e.g 1K or 10K). Or it's too early to care about those? Do we gain better latency if the implementation limits the number of member devices per admin virtqueue? > I don't > see a fundamental difference between 1 or 2 VQs. We are not going to be > adding 1000s of them, Yes, but not sure if it will cause delay for e.g live migration downtime in the future. Thanks > > > > + /* unused, reserved for future extensions */ > > > + u8 reserved1[12]; > > > + le64 group_member_id; > > > + u8 command_specific_data[]; > > > + > > > + /* Device-writable part */ > > > + u8 status; > > > + u8 command_specific_error; > > > + /* unused, reserved for future extensions */ > > > + u8 reserved2[6]; > > > + u8 command_specific_result[]; > > > +}; > > > +\end{lstlisting} > > > + > > > +For all commands, \field{opcode}, \field{group_id} and if > > > +necessary \field{group_member_id} and \field{command_specific_data} are > > > +set by the driver, and the parent device sets \field{status} and if > > > +needed \field{command_specific_error} and > > > +\field{command_specific_result}. > > > + > > > +As a rule, any unused device-readable fields are set to zero by the driver > > > +and ignored by the device. Any unused device-writeable fields are set to zero > > > +by the device and ignored by the driver. > > > + > > > +\field{opcode} specifies the command. The valid > > > +values for \field{opcode} can be found in the following table: > > > + > > > +\begin{tabular}{|l|l|} > > > +\hline > > > +opcode & Command Description \\ > > > +\hline \hline > > > +0000h - 7FFFh & Group administration commands \\ > > > +\hline > > > +8000h - FFFFh & Reserved \\ > > > +\hline > > > +\end{tabular} > > > + > > > +The \field{group_id} specifies the device group. > > > +The \field{group_member_id} specifies the member device within the group. > > > +See section \ref{sec:Introduction / Terminology / Device group} > > > +for the definition of the group identifier and group member > > > +identifier. > > > + > > > +The following table describes possible \field{status} values: > > > + > > > +\begin{tabular}{|l|l|l|} > > > +\hline > > > +Status & Name & Description \\ > > > +\hline \hline > > > +00h & VIRTIO_ADMIN_STATUS_OK & successful completion \\ > > > +\hline > > > +01h & VIRTIO_ADMIN_STATUS_CS_ERR & command specific error \\ > > > +\hline > > > +02h & VIRTIO_ADMIN_STATUS_OPCODE_UNSUPPORTED & unsupported or invalid \field{opcode} \\ > > > +\hline > > > +03h & VIRTIO_ADMIN_STATUS_INVALID_FIELD & invalid field within command specific data \\ > > > +\hline > > > +04h & VIRTIO_ADMIN_STATUS_INVALID_GROUP & invalid group identifier was set \\ > > > +\hline > > > +05h & VIRTIO_ADMIN_STATUS_INVALID_MEM & invalid group member identifier was set \\ > > > +\hline > > > +\end{tabular} > > > + > > > +The \field{command_specific_error} should be inspected by the driver only if \field{status} is set to > > > +VIRTIO_ADMIN_STATUS_CS_ERR by the device. In this case, the > > > +contents of \field{command_specific_error} > > > +holds the command specific error. If \field{status} is not set to VIRTIO_ADMIN_STATUS_CS_ERR, the > > > +\field{command_specific_error} value is undefined and should be ignored by the driver. > > > > > > -- > > > MST > > > >