* [virtio-comment] [PATCH V2 0/2] Introduce VIRTIO_F_QUEUE_STATE @ 2021-03-22 3:47 Jason Wang 2021-03-22 3:47 ` [virtio-comment] [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility Jason Wang ` (2 more replies) 0 siblings, 3 replies; 21+ messages in thread From: Jason Wang @ 2021-03-22 3:47 UTC (permalink / raw) To: mst, virtio-comment Cc: eperezma, lulu, rob.miller, pasic, sgarzare, cohuck, jim.harford, Jason Wang Hi All: This is a new version to support VIRTIO_F_QUEUE_STATE. The feautre extends the basic facility to allow the driver to set and get device internal virtqueue state. This main motivation is to support live migration of virtio devices. Please review. Changes since V1: - Introduce a dedicated part in the basic facility to describe the virtqueue state. - Split the state into avail state and used state, use two le16 isntead of a single u64 in the PCI implementation. Thanks Jason Wang (2): virtio: introduce virtqueue state as basic facility virtio-pci: implement VIRTIO_F_QUEUE_STATE content.tex | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) -- 2.24.3 (Apple Git-128) This publicly archived list offers a means to provide input to the OASIS Virtual I/O Device (VIRTIO) TC. In order to verify user consent to the Feedback License terms and to minimize spam in the list archive, subscription is required before posting. Subscribe: virtio-comment-subscribe@lists.oasis-open.org Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org List help: virtio-comment-help@lists.oasis-open.org List archive: https://lists.oasis-open.org/archives/virtio-comment/ Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists Committee: https://www.oasis-open.org/committees/virtio/ Join OASIS: https://www.oasis-open.org/join/ ^ permalink raw reply [flat|nested] 21+ messages in thread
* [virtio-comment] [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility 2021-03-22 3:47 [virtio-comment] [PATCH V2 0/2] Introduce VIRTIO_F_QUEUE_STATE Jason Wang @ 2021-03-22 3:47 ` Jason Wang 2021-03-22 8:19 ` [virtio-comment] " Eugenio Perez Martin 2021-03-23 10:27 ` [virtio-comment] " Stefan Hajnoczi 2021-03-22 3:47 ` [virtio-comment] [PATCH V2 2/2] virtio-pci: implement VIRTIO_F_QUEUE_STATE Jason Wang 2021-03-23 10:40 ` [virtio-comment] [PATCH V2 0/2] Introduce VIRTIO_F_QUEUE_STATE Stefan Hajnoczi 2 siblings, 2 replies; 21+ messages in thread From: Jason Wang @ 2021-03-22 3:47 UTC (permalink / raw) To: mst, virtio-comment Cc: eperezma, lulu, rob.miller, pasic, sgarzare, cohuck, jim.harford, Jason Wang This patch adds new device facility to save and restore virtqueue state. The virtqueue state is split into two parts: - The available state: The state that is used for read the next available buffer. - The used state: The state that is used for make buffer used. This will simply the transport specific method implemention. E.g two le16 could be used instead of a single le32). For split virtqueue, we only need the available state since the used state is implemented in the virtqueue itself (the used index). For packed virtqueue, we need both the available state and the used state. Those states are required to implement live migration support for virtio device. Signed-off-by: Jason Wang <jasowang@redhat.com> --- content.tex | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/content.tex b/content.tex index 620c0e2..af39111 100644 --- a/content.tex +++ b/content.tex @@ -385,6 +385,83 @@ \section{Exporting Objects}\label{sec:Basic Facilities of a Virtio Device / Expo types. It is RECOMMENDED that devices generate version 4 UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}. +\section{Virtqueue State}\label{sec:Virtqueues / Virtqueue State} + +When VIRTIO_F_QUEUE_STATE is negotiated, the driver can set and +get the device internal virtqueue state through the following +fields. The way to access those fields is transport specific. + +\subsection{\field{Available State} Field} + +The availabe state field is two bytes virtqueue state that is used by +the device to read for the next available buffer. + +When VIRTIO_RING_F_PACKED is not negotiated, it contains: + +\begin{lstlisting} +le16 last_avail_idx; +\end{lstlisting} + +The \field{last_avail_idx} field is the location where the device read +for the next index from the virtqueue available ring. + +When VIRTIO_RING_F_PACKED is negotiated, it contains: + +\begin{lstlisting} +le16 { + last_avail_idx : 15; + last_avail_wrap_counter : 1; +}; +\end{lstlisting} + +The \field{last_avail_idx} field is the location where the device read +for the next descriptor from the virtqueue descriptor ring. + +The \field{last_avail_wrap_counter} field is the last driver ring wrap +counter that is observed by the device. + +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}. + +\subsection{\field{Used State} Field} + +The availabe state field is two bytes virtqueue state that is used by +the device to make buffer used. + +When VIRTIO_RING_F_PACKED is negotiated, the used state contains: + +\begin{lstlisting} +le16 { + used_idx : 15; + used_wrap_counter : 1; +}; +\end{lstlisting} + +The \field{used_idx} field is the location where the device write next +used descriptor to the descriptor ring. + +The \field{used_wrap_counter} field is the wrap counter that is used +by the device. + +The used state is unnecessary when VIRTIO_RING_F_PACKED is not +negotiated. + +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}. + +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State} + +If VIRTIO_F_QUEUE_STATE has been negotiated, a driver MUST set the +state of each virtqueue after setting the FEATURE_OK status bit but +before setting the DRIVER_OK status bit. + +If VIRTIO_F_QUEUE_STATE has been negotiated. a driver MUST reset the +device before getting the virtqueue state. + +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State} + +If VIRTIO_F_QUEUE_STATE has been negotiated, a device MUST NOT clear +the queue state upon reset and MUST reset the queue state when the +driver sets ACKNOWLEDGE status bit. + \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation} We start with an overview of device initialization, then expand on the @@ -6596,6 +6673,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits} transport specific. For more details about driver notifications over PCI see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}. +\item[VIRTIO_F_QUEUE_STATE(40)] This feature indicates that the driver + can set and get the virtqueue state. + See \ref{sec:Virtqueues / Virtqueue State}~\nameref{sec:Virtqueues / Virtqueue State}. + \end{description} \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits} -- 2.24.3 (Apple Git-128) This publicly archived list offers a means to provide input to the OASIS Virtual I/O Device (VIRTIO) TC. In order to verify user consent to the Feedback License terms and to minimize spam in the list archive, subscription is required before posting. Subscribe: virtio-comment-subscribe@lists.oasis-open.org Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org List help: virtio-comment-help@lists.oasis-open.org List archive: https://lists.oasis-open.org/archives/virtio-comment/ Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists Committee: https://www.oasis-open.org/committees/virtio/ Join OASIS: https://www.oasis-open.org/join/ ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [virtio-comment] Re: [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility 2021-03-22 3:47 ` [virtio-comment] [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility Jason Wang @ 2021-03-22 8:19 ` Eugenio Perez Martin 2021-03-23 2:54 ` Jason Wang 2021-03-23 10:27 ` [virtio-comment] " Stefan Hajnoczi 1 sibling, 1 reply; 21+ messages in thread From: Eugenio Perez Martin @ 2021-03-22 8:19 UTC (permalink / raw) To: Jason Wang Cc: Michael Tsirkin, virtio-comment, Cindy Lu, Rob Miller, Halil Pasic, Stefano Garzarella, Cornelia Huck, Jim Harford On Mon, Mar 22, 2021 at 4:47 AM Jason Wang <jasowang@redhat.com> wrote: > > This patch adds new device facility to save and restore virtqueue > state. The virtqueue state is split into two parts: > > - The available state: The state that is used for read the next > available buffer. > - The used state: The state that is used for make buffer used. > > This will simply the transport specific method implemention. E.g two > le16 could be used instead of a single le32). For split virtqueue, we > only need the available state since the used state is implemented in > the virtqueue itself (the used index). > For packed virtqueue, we need > both the available state and the used state. > > Those states are required to implement live migration support for > virtio device. > > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > content.tex | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 81 insertions(+) > > diff --git a/content.tex b/content.tex > index 620c0e2..af39111 100644 > --- a/content.tex > +++ b/content.tex > @@ -385,6 +385,83 @@ \section{Exporting Objects}\label{sec:Basic Facilities of a Virtio Device / Expo > types. It is RECOMMENDED that devices generate version 4 > UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}. > > +\section{Virtqueue State}\label{sec:Virtqueues / Virtqueue State} > + > +When VIRTIO_F_QUEUE_STATE is negotiated, the driver can set and > +get the device internal virtqueue state through the following > +fields. The way to access those fields is transport specific. > + > +\subsection{\field{Available State} Field} > + > +The availabe state field is two bytes virtqueue state that is used by > +the device to read for the next available buffer. > + > +When VIRTIO_RING_F_PACKED is not negotiated, it contains: > + > +\begin{lstlisting} > +le16 last_avail_idx; > +\end{lstlisting} > + > +The \field{last_avail_idx} field is the location where the device read > +for the next index from the virtqueue available ring. > + > +When VIRTIO_RING_F_PACKED is negotiated, it contains: > + > +\begin{lstlisting} > +le16 { > + last_avail_idx : 15; > + last_avail_wrap_counter : 1; > +}; > +\end{lstlisting} > + > +The \field{last_avail_idx} field is the location where the device read > +for the next descriptor from the virtqueue descriptor ring. > + > +The \field{last_avail_wrap_counter} field is the last driver ring wrap > +counter that is observed by the device. > + > +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}. > + > +\subsection{\field{Used State} Field} > + > +The availabe state field is two bytes virtqueue state that is used by > +the device to make buffer used. > + > +When VIRTIO_RING_F_PACKED is negotiated, the used state contains: > + > +\begin{lstlisting} > +le16 { > + used_idx : 15; > + used_wrap_counter : 1; > +}; > +\end{lstlisting} > + > +The \field{used_idx} field is the location where the device write next > +used descriptor to the descriptor ring. > + > +The \field{used_wrap_counter} field is the wrap counter that is used > +by the device. > + > +The used state is unnecessary when VIRTIO_RING_F_PACKED is not > +negotiated. Maybe we could be more specific with "unnecessary"? Either to say: 1) "For split virtqueue the device/driver must ignore this field at start/recover state and use used_idx in the used ring.", so use of this field is totally unspecified. 2) "For split virtqueue, the device/driver must prefer to get/set state from virtqueue's used ring in case of discrepancy with this value". I prefer the former so there is only one place to set/retrieve it, but something like this would avoid confusion in my opinion. I find it equally convenient to prefer the virtqueue memory or the transport-specific used state field, though. > + > +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}. > + > +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State} > + > +If VIRTIO_F_QUEUE_STATE has been negotiated, a driver MUST set the > +state of each virtqueue after setting the FEATURE_OK status bit but > +before setting the DRIVER_OK status bit. > + > +If VIRTIO_F_QUEUE_STATE has been negotiated. a driver MUST reset the > +device before getting the virtqueue state. > + > +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State} > + > +If VIRTIO_F_QUEUE_STATE has been negotiated, a device MUST NOT clear > +the queue state upon reset and MUST reset the queue state when the > +driver sets ACKNOWLEDGE status bit. > + > \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation} > > We start with an overview of device initialization, then expand on the > @@ -6596,6 +6673,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits} > transport specific. > For more details about driver notifications over PCI see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}. > > +\item[VIRTIO_F_QUEUE_STATE(40)] This feature indicates that the driver > + can set and get the virtqueue state. > + See \ref{sec:Virtqueues / Virtqueue State}~\nameref{sec:Virtqueues / Virtqueue State}. > + > \end{description} > > \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits} > -- > 2.24.3 (Apple Git-128) > This publicly archived list offers a means to provide input to the OASIS Virtual I/O Device (VIRTIO) TC. In order to verify user consent to the Feedback License terms and to minimize spam in the list archive, subscription is required before posting. Subscribe: virtio-comment-subscribe@lists.oasis-open.org Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org List help: virtio-comment-help@lists.oasis-open.org List archive: https://lists.oasis-open.org/archives/virtio-comment/ Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists Committee: https://www.oasis-open.org/committees/virtio/ Join OASIS: https://www.oasis-open.org/join/ ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [virtio-comment] Re: [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility 2021-03-22 8:19 ` [virtio-comment] " Eugenio Perez Martin @ 2021-03-23 2:54 ` Jason Wang 2021-03-23 9:27 ` Eugenio Perez Martin 0 siblings, 1 reply; 21+ messages in thread From: Jason Wang @ 2021-03-23 2:54 UTC (permalink / raw) To: virtio-comment 在 2021/3/22 下午4:19, Eugenio Perez Martin 写道: > On Mon, Mar 22, 2021 at 4:47 AM Jason Wang <jasowang@redhat.com> wrote: >> This patch adds new device facility to save and restore virtqueue >> state. The virtqueue state is split into two parts: >> >> - The available state: The state that is used for read the next >> available buffer. >> - The used state: The state that is used for make buffer used. >> >> This will simply the transport specific method implemention. E.g two >> le16 could be used instead of a single le32). For split virtqueue, we >> only need the available state since the used state is implemented in >> the virtqueue itself (the used index). >> For packed virtqueue, we need >> both the available state and the used state. >> >> Those states are required to implement live migration support for >> virtio device. >> >> Signed-off-by: Jason Wang <jasowang@redhat.com> >> --- >> content.tex | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 81 insertions(+) >> >> diff --git a/content.tex b/content.tex >> index 620c0e2..af39111 100644 >> --- a/content.tex >> +++ b/content.tex >> @@ -385,6 +385,83 @@ \section{Exporting Objects}\label{sec:Basic Facilities of a Virtio Device / Expo >> types. It is RECOMMENDED that devices generate version 4 >> UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}. >> >> +\section{Virtqueue State}\label{sec:Virtqueues / Virtqueue State} >> + >> +When VIRTIO_F_QUEUE_STATE is negotiated, the driver can set and >> +get the device internal virtqueue state through the following >> +fields. The way to access those fields is transport specific. >> + >> +\subsection{\field{Available State} Field} >> + >> +The availabe state field is two bytes virtqueue state that is used by >> +the device to read for the next available buffer. >> + >> +When VIRTIO_RING_F_PACKED is not negotiated, it contains: >> + >> +\begin{lstlisting} >> +le16 last_avail_idx; >> +\end{lstlisting} >> + >> +The \field{last_avail_idx} field is the location where the device read >> +for the next index from the virtqueue available ring. >> + >> +When VIRTIO_RING_F_PACKED is negotiated, it contains: >> + >> +\begin{lstlisting} >> +le16 { >> + last_avail_idx : 15; >> + last_avail_wrap_counter : 1; >> +}; >> +\end{lstlisting} >> + >> +The \field{last_avail_idx} field is the location where the device read >> +for the next descriptor from the virtqueue descriptor ring. >> + >> +The \field{last_avail_wrap_counter} field is the last driver ring wrap >> +counter that is observed by the device. >> + >> +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}. >> + >> +\subsection{\field{Used State} Field} >> + >> +The availabe state field is two bytes virtqueue state that is used by >> +the device to make buffer used. >> + >> +When VIRTIO_RING_F_PACKED is negotiated, the used state contains: >> + >> +\begin{lstlisting} >> +le16 { >> + used_idx : 15; >> + used_wrap_counter : 1; >> +}; >> +\end{lstlisting} >> + >> +The \field{used_idx} field is the location where the device write next >> +used descriptor to the descriptor ring. >> + >> +The \field{used_wrap_counter} field is the wrap counter that is used >> +by the device. >> + >> +The used state is unnecessary when VIRTIO_RING_F_PACKED is not >> +negotiated. > Maybe we could be more specific with "unnecessary"? Either to say: > 1) "For split virtqueue the device/driver must ignore this field at > start/recover state and use used_idx in the used ring.", so use of > this field is totally unspecified. RIght, this could be added as normative part. An interesting part is the timing of the read, the current semantics depends on reset command to stop the device (to avoid introduing new status). So in order to get the state, driver needs: 1) reset the device 2) read the avail and used states Technically, after step 1, driver doesn't know whether nor not it's a split virtqueue. For write, it should be fine since we mandate it can only be done after FEATURE_OK so driver know it's a split or not. So how about something like in the driver normative: To get the virtqueue state, the driver MUST remember the queue format that is used and read the virtqueue state accordingly after resetting the device. E.g if the split virtqueue is used before resetting the device, the driver MUST NOT acces used state after restting the device. To write the virtqueue state, the driver MUST NOT write to the used state if VIRTIO_F_RING_PACKED is not negotiated. Thanks > 2) "For split virtqueue, the device/driver must prefer to get/set > state from virtqueue's used ring in case of discrepancy with this > value". > > I prefer the former so there is only one place to set/retrieve it, but > something like this would avoid confusion in my opinion. I find it > equally convenient to prefer the virtqueue memory or the > transport-specific used state field, though. This publicly archived list offers a means to provide input to the OASIS Virtual I/O Device (VIRTIO) TC. In order to verify user consent to the Feedback License terms and to minimize spam in the list archive, subscription is required before posting. Subscribe: virtio-comment-subscribe@lists.oasis-open.org Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org List help: virtio-comment-help@lists.oasis-open.org List archive: https://lists.oasis-open.org/archives/virtio-comment/ Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists Committee: https://www.oasis-open.org/committees/virtio/ Join OASIS: https://www.oasis-open.org/join/ ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [virtio-comment] Re: [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility 2021-03-23 2:54 ` Jason Wang @ 2021-03-23 9:27 ` Eugenio Perez Martin 0 siblings, 0 replies; 21+ messages in thread From: Eugenio Perez Martin @ 2021-03-23 9:27 UTC (permalink / raw) To: Jason Wang; +Cc: virtio-comment On Tue, Mar 23, 2021 at 3:54 AM Jason Wang <jasowang@redhat.com> wrote: > > > 在 2021/3/22 下午4:19, Eugenio Perez Martin 写道: > > On Mon, Mar 22, 2021 at 4:47 AM Jason Wang <jasowang@redhat.com> wrote: > >> This patch adds new device facility to save and restore virtqueue > >> state. The virtqueue state is split into two parts: > >> > >> - The available state: The state that is used for read the next > >> available buffer. > >> - The used state: The state that is used for make buffer used. > >> > >> This will simply the transport specific method implemention. E.g two > >> le16 could be used instead of a single le32). For split virtqueue, we > >> only need the available state since the used state is implemented in > >> the virtqueue itself (the used index). > >> For packed virtqueue, we need > >> both the available state and the used state. > >> > >> Those states are required to implement live migration support for > >> virtio device. > >> > >> Signed-off-by: Jason Wang <jasowang@redhat.com> > >> --- > >> content.tex | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 81 insertions(+) > >> > >> diff --git a/content.tex b/content.tex > >> index 620c0e2..af39111 100644 > >> --- a/content.tex > >> +++ b/content.tex > >> @@ -385,6 +385,83 @@ \section{Exporting Objects}\label{sec:Basic Facilities of a Virtio Device / Expo > >> types. It is RECOMMENDED that devices generate version 4 > >> UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}. > >> > >> +\section{Virtqueue State}\label{sec:Virtqueues / Virtqueue State} > >> + > >> +When VIRTIO_F_QUEUE_STATE is negotiated, the driver can set and > >> +get the device internal virtqueue state through the following > >> +fields. The way to access those fields is transport specific. > >> + > >> +\subsection{\field{Available State} Field} > >> + > >> +The availabe state field is two bytes virtqueue state that is used by > >> +the device to read for the next available buffer. > >> + > >> +When VIRTIO_RING_F_PACKED is not negotiated, it contains: > >> + > >> +\begin{lstlisting} > >> +le16 last_avail_idx; > >> +\end{lstlisting} > >> + > >> +The \field{last_avail_idx} field is the location where the device read > >> +for the next index from the virtqueue available ring. > >> + > >> +When VIRTIO_RING_F_PACKED is negotiated, it contains: > >> + > >> +\begin{lstlisting} > >> +le16 { > >> + last_avail_idx : 15; > >> + last_avail_wrap_counter : 1; > >> +}; > >> +\end{lstlisting} > >> + > >> +The \field{last_avail_idx} field is the location where the device read > >> +for the next descriptor from the virtqueue descriptor ring. > >> + > >> +The \field{last_avail_wrap_counter} field is the last driver ring wrap > >> +counter that is observed by the device. > >> + > >> +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}. > >> + > >> +\subsection{\field{Used State} Field} > >> + > >> +The availabe state field is two bytes virtqueue state that is used by > >> +the device to make buffer used. > >> + > >> +When VIRTIO_RING_F_PACKED is negotiated, the used state contains: > >> + > >> +\begin{lstlisting} > >> +le16 { > >> + used_idx : 15; > >> + used_wrap_counter : 1; > >> +}; > >> +\end{lstlisting} > >> + > >> +The \field{used_idx} field is the location where the device write next > >> +used descriptor to the descriptor ring. > >> + > >> +The \field{used_wrap_counter} field is the wrap counter that is used > >> +by the device. > >> + > >> +The used state is unnecessary when VIRTIO_RING_F_PACKED is not > >> +negotiated. > > Maybe we could be more specific with "unnecessary"? Either to say: > > 1) "For split virtqueue the device/driver must ignore this field at > > start/recover state and use used_idx in the used ring.", so use of > > this field is totally unspecified. > > > RIght, this could be added as normative part. > > An interesting part is the timing of the read, the current semantics > depends on reset command to stop the device (to avoid introduing new > status). > > So in order to get the state, driver needs: > > 1) reset the device > 2) read the avail and used states > > Technically, after step 1, driver doesn't know whether nor not it's a > split virtqueue. > > For write, it should be fine since we mandate it can only be done after > FEATURE_OK so driver know it's a split or not. > > So how about something like in the driver normative: > > To get the virtqueue state, the driver MUST remember the queue format > that is used and read the virtqueue state accordingly after resetting > the device. E.g if the split virtqueue is used before resetting the > device, the driver MUST NOT acces used state after restting the device. > > To write the virtqueue state, the driver MUST NOT write to the used > state if VIRTIO_F_RING_PACKED is not negotiated. > Right, I'm not sure if it will have a use case in the future but I find it better to explicitly state that. Thanks! > Thanks > > > > 2) "For split virtqueue, the device/driver must prefer to get/set > > state from virtqueue's used ring in case of discrepancy with this > > value". > > > > I prefer the former so there is only one place to set/retrieve it, but > > something like this would avoid confusion in my opinion. I find it > > equally convenient to prefer the virtqueue memory or the > > transport-specific used state field, though. > > > This publicly archived list offers a means to provide input to the > OASIS Virtual I/O Device (VIRTIO) TC. > > In order to verify user consent to the Feedback License terms and > to minimize spam in the list archive, subscription is required > before posting. > > Subscribe: virtio-comment-subscribe@lists.oasis-open.org > Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org > List help: virtio-comment-help@lists.oasis-open.org > List archive: https://lists.oasis-open.org/archives/virtio-comment/ > Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf > List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists > Committee: https://www.oasis-open.org/committees/virtio/ > Join OASIS: https://www.oasis-open.org/join/ > This publicly archived list offers a means to provide input to the OASIS Virtual I/O Device (VIRTIO) TC. In order to verify user consent to the Feedback License terms and to minimize spam in the list archive, subscription is required before posting. Subscribe: virtio-comment-subscribe@lists.oasis-open.org Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org List help: virtio-comment-help@lists.oasis-open.org List archive: https://lists.oasis-open.org/archives/virtio-comment/ Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists Committee: https://www.oasis-open.org/committees/virtio/ Join OASIS: https://www.oasis-open.org/join/ ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [virtio-comment] [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility 2021-03-22 3:47 ` [virtio-comment] [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility Jason Wang 2021-03-22 8:19 ` [virtio-comment] " Eugenio Perez Martin @ 2021-03-23 10:27 ` Stefan Hajnoczi 2021-03-24 8:15 ` Jason Wang 1 sibling, 1 reply; 21+ messages in thread From: Stefan Hajnoczi @ 2021-03-23 10:27 UTC (permalink / raw) To: Jason Wang Cc: mst, virtio-comment, eperezma, lulu, rob.miller, pasic, sgarzare, cohuck, jim.harford [-- Attachment #1: Type: text/plain, Size: 5300 bytes --] On Mon, Mar 22, 2021 at 11:47:16AM +0800, Jason Wang wrote: > This patch adds new device facility to save and restore virtqueue > state. The virtqueue state is split into two parts: > > - The available state: The state that is used for read the next > available buffer. > - The used state: The state that is used for make buffer used. > > This will simply the transport specific method implemention. E.g two > le16 could be used instead of a single le32). For split virtqueue, we > only need the available state since the used state is implemented in > the virtqueue itself (the used index). For packed virtqueue, we need > both the available state and the used state. > > Those states are required to implement live migration support for > virtio device. > > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > content.tex | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 81 insertions(+) > > diff --git a/content.tex b/content.tex > index 620c0e2..af39111 100644 > --- a/content.tex > +++ b/content.tex > @@ -385,6 +385,83 @@ \section{Exporting Objects}\label{sec:Basic Facilities of a Virtio Device / Expo > types. It is RECOMMENDED that devices generate version 4 > UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}. > > +\section{Virtqueue State}\label{sec:Virtqueues / Virtqueue State} > + > +When VIRTIO_F_QUEUE_STATE is negotiated, the driver can set and > +get the device internal virtqueue state through the following > +fields. The way to access those fields is transport specific. > + > +\subsection{\field{Available State} Field} > + > +The availabe state field is two bytes virtqueue state that is used by s/availabe/available/ s/bytes virtqueue state/bytes of virtqueue state/ > +the device to read for the next available buffer. > + > +When VIRTIO_RING_F_PACKED is not negotiated, it contains: > + > +\begin{lstlisting} > +le16 last_avail_idx; > +\end{lstlisting} > + > +The \field{last_avail_idx} field is the location where the device read > +for the next index from the virtqueue available ring. Perhaps this field can be defined in more detail: The \field{last_avail_idx} field is the free-running available ring index where the device will read the next available head of a descriptor chain. See also \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Available Ring}. > + > +When VIRTIO_RING_F_PACKED is negotiated, it contains: > + > +\begin{lstlisting} > +le16 { > + last_avail_idx : 15; > + last_avail_wrap_counter : 1; > +}; > +\end{lstlisting} > + > +The \field{last_avail_idx} field is the location where the device read > +for the next descriptor from the virtqueue descriptor ring. Similar wording as above can be used. > + > +The \field{last_avail_wrap_counter} field is the last driver ring wrap > +counter that is observed by the device. s/is/was/? > + > +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}. > + > +\subsection{\field{Used State} Field} > + > +The availabe state field is two bytes virtqueue state that is used by s/availabe/used/ s/bytes virtqueue state/bytes of virtqueue state/ > +the device to make buffer used. s/to make buffer used/when marking a buffer used/ > + > +When VIRTIO_RING_F_PACKED is negotiated, the used state contains: > + > +\begin{lstlisting} > +le16 { > + used_idx : 15; > + used_wrap_counter : 1; > +}; > +\end{lstlisting} > + > +The \field{used_idx} field is the location where the device write next > +used descriptor to the descriptor ring. Similar as above. It should at least mention whether this is a free-running index or always less than queue size. > + > +The \field{used_wrap_counter} field is the wrap counter that is used > +by the device. > + > +The used state is unnecessary when VIRTIO_RING_F_PACKED is not > +negotiated. > + > +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}. > + > +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State} > + > +If VIRTIO_F_QUEUE_STATE has been negotiated, a driver MUST set the > +state of each virtqueue after setting the FEATURE_OK status bit but > +before setting the DRIVER_OK status bit. > + > +If VIRTIO_F_QUEUE_STATE has been negotiated. a driver MUST reset the s/negotiated./negotiated,/ > +device before getting the virtqueue state. Interesting approach, but it makes sense that the device must be stopped before we mess with the queue state :). I wonder whether requiring the device to keep state across reset will cause issues in the future or for testing/fuzzing. Another approach would have been to add a new status register bit for pausing the device. That reminds me of vhost. > + > +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State} > + > +If VIRTIO_F_QUEUE_STATE has been negotiated, a device MUST NOT clear > +the queue state upon reset and MUST reset the queue state when the > +driver sets ACKNOWLEDGE status bit. I only see normative sections mentioning getting the virtqueue state but not setting it. Please explain how setting works. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [virtio-comment] [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility 2021-03-23 10:27 ` [virtio-comment] " Stefan Hajnoczi @ 2021-03-24 8:15 ` Jason Wang 2021-03-24 9:35 ` Stefan Hajnoczi 2021-03-24 9:38 ` Stefan Hajnoczi 0 siblings, 2 replies; 21+ messages in thread From: Jason Wang @ 2021-03-24 8:15 UTC (permalink / raw) To: virtio-comment 在 2021/3/23 下午6:27, Stefan Hajnoczi 写道: > On Mon, Mar 22, 2021 at 11:47:16AM +0800, Jason Wang wrote: >> This patch adds new device facility to save and restore virtqueue >> state. The virtqueue state is split into two parts: >> >> - The available state: The state that is used for read the next >> available buffer. >> - The used state: The state that is used for make buffer used. >> >> This will simply the transport specific method implemention. E.g two >> le16 could be used instead of a single le32). For split virtqueue, we >> only need the available state since the used state is implemented in >> the virtqueue itself (the used index). For packed virtqueue, we need >> both the available state and the used state. >> >> Those states are required to implement live migration support for >> virtio device. >> >> Signed-off-by: Jason Wang <jasowang@redhat.com> >> --- >> content.tex | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 81 insertions(+) >> >> diff --git a/content.tex b/content.tex >> index 620c0e2..af39111 100644 >> --- a/content.tex >> +++ b/content.tex >> @@ -385,6 +385,83 @@ \section{Exporting Objects}\label{sec:Basic Facilities of a Virtio Device / Expo >> types. It is RECOMMENDED that devices generate version 4 >> UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}. >> >> +\section{Virtqueue State}\label{sec:Virtqueues / Virtqueue State} >> + >> +When VIRTIO_F_QUEUE_STATE is negotiated, the driver can set and >> +get the device internal virtqueue state through the following >> +fields. The way to access those fields is transport specific. >> + >> +\subsection{\field{Available State} Field} >> + >> +The availabe state field is two bytes virtqueue state that is used by > s/availabe/available/ > > s/bytes virtqueue state/bytes of virtqueue state/ Will fix all the above. > >> +the device to read for the next available buffer. >> + >> +When VIRTIO_RING_F_PACKED is not negotiated, it contains: >> + >> +\begin{lstlisting} >> +le16 last_avail_idx; >> +\end{lstlisting} >> + >> +The \field{last_avail_idx} field is the location where the device read >> +for the next index from the virtqueue available ring. > Perhaps this field can be defined in more detail: > > The \field{last_avail_idx} field is the free-running available ring > index where the device will read the next available head of a > descriptor chain. > > See also \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Available Ring}. Good point. > >> + >> +When VIRTIO_RING_F_PACKED is negotiated, it contains: >> + >> +\begin{lstlisting} >> +le16 { >> + last_avail_idx : 15; >> + last_avail_wrap_counter : 1; >> +}; >> +\end{lstlisting} >> + >> +The \field{last_avail_idx} field is the location where the device read >> +for the next descriptor from the virtqueue descriptor ring. > Similar wording as above can be used. Yes. > >> + >> +The \field{last_avail_wrap_counter} field is the last driver ring wrap >> +counter that is observed by the device. > s/is/was/? Ok. > >> + >> +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}. >> + >> +\subsection{\field{Used State} Field} >> + >> +The availabe state field is two bytes virtqueue state that is used by > s/availabe/used/ Exactly. > > s/bytes virtqueue state/bytes of virtqueue state/ Right. > >> +the device to make buffer used. > s/to make buffer used/when marking a buffer used/ Ok. > >> + >> +When VIRTIO_RING_F_PACKED is negotiated, the used state contains: >> + >> +\begin{lstlisting} >> +le16 { >> + used_idx : 15; >> + used_wrap_counter : 1; >> +}; >> +\end{lstlisting} >> + >> +The \field{used_idx} field is the location where the device write next >> +used descriptor to the descriptor ring. > Similar as above. It should at least mention whether this is a > free-running index or always less than queue size. Yes, to be consistent, it should be free-running index. > >> + >> +The \field{used_wrap_counter} field is the wrap counter that is used >> +by the device. >> + >> +The used state is unnecessary when VIRTIO_RING_F_PACKED is not >> +negotiated. >> + >> +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}. >> + >> +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State} >> + >> +If VIRTIO_F_QUEUE_STATE has been negotiated, a driver MUST set the >> +state of each virtqueue after setting the FEATURE_OK status bit but >> +before setting the DRIVER_OK status bit. >> + >> +If VIRTIO_F_QUEUE_STATE has been negotiated. a driver MUST reset the > s/negotiated./negotiated,/ > >> +device before getting the virtqueue state. > Interesting approach, but it makes sense that the device must be stopped > before we mess with the queue state :). > > I wonder whether requiring the device to keep state across reset will > cause issues in the future or for testing/fuzzing. > > Another approach would have been to add a new status register bit for > pausing the device. That reminds me of vhost. That's the way I did here: https://lists.oasis-open.org/archives/virtio-comment/202012/msg00027.html > >> + >> +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State} >> + >> +If VIRTIO_F_QUEUE_STATE has been negotiated, a device MUST NOT clear >> +the queue state upon reset and MUST reset the queue state when the >> +driver sets ACKNOWLEDGE status bit. > I only see normative sections mentioning getting the virtqueue state but > not setting it. Please explain how setting works. You mean the device normative part? I think the description of avail state and used state can explain itself. Or I miss something? Thanks This publicly archived list offers a means to provide input to the OASIS Virtual I/O Device (VIRTIO) TC. In order to verify user consent to the Feedback License terms and to minimize spam in the list archive, subscription is required before posting. Subscribe: virtio-comment-subscribe@lists.oasis-open.org Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org List help: virtio-comment-help@lists.oasis-open.org List archive: https://lists.oasis-open.org/archives/virtio-comment/ Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists Committee: https://www.oasis-open.org/committees/virtio/ Join OASIS: https://www.oasis-open.org/join/ ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [virtio-comment] [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility 2021-03-24 8:15 ` Jason Wang @ 2021-03-24 9:35 ` Stefan Hajnoczi 2021-03-25 2:38 ` Jason Wang 2021-03-24 9:38 ` Stefan Hajnoczi 1 sibling, 1 reply; 21+ messages in thread From: Stefan Hajnoczi @ 2021-03-24 9:35 UTC (permalink / raw) To: Jason Wang; +Cc: virtio-comment [-- Attachment #1: Type: text/plain, Size: 1008 bytes --] On Wed, Mar 24, 2021 at 04:15:55PM +0800, Jason Wang wrote: > 在 2021/3/23 下午6:27, Stefan Hajnoczi 写道: > > On Mon, Mar 22, 2021 at 11:47:16AM +0800, Jason Wang wrote: > > > + > > > +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State} > > > + > > > +If VIRTIO_F_QUEUE_STATE has been negotiated, a device MUST NOT clear > > > +the queue state upon reset and MUST reset the queue state when the > > > +driver sets ACKNOWLEDGE status bit. > > I only see normative sections mentioning getting the virtqueue state but > > not setting it. Please explain how setting works. > > > You mean the device normative part? I think the description of avail state > and used state can explain itself. Or I miss something? Oops, I missed the earlier normative section that says the driver must set the virtqueue state after FEATURES_OK but before DRIVER_OK. When re-reading I noticed that there is a typo: s/FEATURE_OK/FEATURES_OK/. Stefan [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [virtio-comment] [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility 2021-03-24 9:35 ` Stefan Hajnoczi @ 2021-03-25 2:38 ` Jason Wang 0 siblings, 0 replies; 21+ messages in thread From: Jason Wang @ 2021-03-25 2:38 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: virtio-comment 在 2021/3/24 下午5:35, Stefan Hajnoczi 写道: > On Wed, Mar 24, 2021 at 04:15:55PM +0800, Jason Wang wrote: >> 在 2021/3/23 下午6:27, Stefan Hajnoczi 写道: >>> On Mon, Mar 22, 2021 at 11:47:16AM +0800, Jason Wang wrote: >>>> + >>>> +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State} >>>> + >>>> +If VIRTIO_F_QUEUE_STATE has been negotiated, a device MUST NOT clear >>>> +the queue state upon reset and MUST reset the queue state when the >>>> +driver sets ACKNOWLEDGE status bit. >>> I only see normative sections mentioning getting the virtqueue state but >>> not setting it. Please explain how setting works. >> >> You mean the device normative part? I think the description of avail state >> and used state can explain itself. Or I miss something? > Oops, I missed the earlier normative section that says the driver must > set the virtqueue state after FEATURES_OK but before DRIVER_OK. > > When re-reading I noticed that there is a typo: s/FEATURE_OK/FEATURES_OK/. Will fix. Thanks > > Stefan This publicly archived list offers a means to provide input to the OASIS Virtual I/O Device (VIRTIO) TC. In order to verify user consent to the Feedback License terms and to minimize spam in the list archive, subscription is required before posting. Subscribe: virtio-comment-subscribe@lists.oasis-open.org Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org List help: virtio-comment-help@lists.oasis-open.org List archive: https://lists.oasis-open.org/archives/virtio-comment/ Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists Committee: https://www.oasis-open.org/committees/virtio/ Join OASIS: https://www.oasis-open.org/join/ ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [virtio-comment] [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility 2021-03-24 8:15 ` Jason Wang 2021-03-24 9:35 ` Stefan Hajnoczi @ 2021-03-24 9:38 ` Stefan Hajnoczi 2021-03-25 2:42 ` Jason Wang 1 sibling, 1 reply; 21+ messages in thread From: Stefan Hajnoczi @ 2021-03-24 9:38 UTC (permalink / raw) To: Jason Wang; +Cc: virtio-comment [-- Attachment #1: Type: text/plain, Size: 1724 bytes --] On Wed, Mar 24, 2021 at 04:15:55PM +0800, Jason Wang wrote: > 在 2021/3/23 下午6:27, Stefan Hajnoczi 写道: > > On Mon, Mar 22, 2021 at 11:47:16AM +0800, Jason Wang wrote: > > > +The \field{used_wrap_counter} field is the wrap counter that is used > > > +by the device. > > > + > > > +The used state is unnecessary when VIRTIO_RING_F_PACKED is not > > > +negotiated. > > > + > > > +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}. > > > + > > > +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State} > > > + > > > +If VIRTIO_F_QUEUE_STATE has been negotiated, a driver MUST set the > > > +state of each virtqueue after setting the FEATURE_OK status bit but > > > +before setting the DRIVER_OK status bit. > > > + > > > +If VIRTIO_F_QUEUE_STATE has been negotiated. a driver MUST reset the > > s/negotiated./negotiated,/ > > > > > +device before getting the virtqueue state. > > Interesting approach, but it makes sense that the device must be stopped > > before we mess with the queue state :). > > > > I wonder whether requiring the device to keep state across reset will > > cause issues in the future or for testing/fuzzing. > > > > Another approach would have been to add a new status register bit for > > pausing the device. That reminds me of vhost. > > > That's the way I did here: > https://lists.oasis-open.org/archives/virtio-comment/202012/msg00027.html That design seems less risky. I think modifying the semantics of device reset might end up being a problem. Defining an explicit paused state for saving/loading state seems cleaner. Why did you end up switching approaches? Stefan [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [virtio-comment] [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility 2021-03-24 9:38 ` Stefan Hajnoczi @ 2021-03-25 2:42 ` Jason Wang 2021-03-25 9:59 ` Stefan Hajnoczi 0 siblings, 1 reply; 21+ messages in thread From: Jason Wang @ 2021-03-25 2:42 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: virtio-comment [-- Attachment #1: Type: text/plain, Size: 2256 bytes --] 在 2021/3/24 下午5:38, Stefan Hajnoczi 写道: > On Wed, Mar 24, 2021 at 04:15:55PM +0800, Jason Wang wrote: >> 在 2021/3/23 下午6:27, Stefan Hajnoczi 写道: >>> On Mon, Mar 22, 2021 at 11:47:16AM +0800, Jason Wang wrote: >>>> +The \field{used_wrap_counter} field is the wrap counter that is used >>>> +by the device. >>>> + >>>> +The used state is unnecessary when VIRTIO_RING_F_PACKED is not >>>> +negotiated. >>>> + >>>> +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}. >>>> + >>>> +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State} >>>> + >>>> +If VIRTIO_F_QUEUE_STATE has been negotiated, a driver MUST set the >>>> +state of each virtqueue after setting the FEATURE_OK status bit but >>>> +before setting the DRIVER_OK status bit. >>>> + >>>> +If VIRTIO_F_QUEUE_STATE has been negotiated. a driver MUST reset the >>> s/negotiated./negotiated,/ >>> >>>> +device before getting the virtqueue state. >>> Interesting approach, but it makes sense that the device must be stopped >>> before we mess with the queue state :). >>> >>> I wonder whether requiring the device to keep state across reset will >>> cause issues in the future or for testing/fuzzing. >>> >>> Another approach would have been to add a new status register bit for >>> pausing the device. That reminds me of vhost. >> >> That's the way I did here: >> https://lists.oasis-open.org/archives/virtio-comment/202012/msg00027.html > That design seems less risky. I think modifying the semantics of device > reset might end up being a problem. Defining an explicit paused state > for saving/loading state seems cleaner. > > Why did you end up switching approaches? I try to seek some short-cut withut introducing new status bit but I end up realizing it might not work as expected. Another reason is that, by using new status bit, we need to ability to resume the device by clearing the bit which is somehow conflict with what spec requries: " 2.1.1 Driver Requirements: Device Status Field The driver MUST NOT clear adevice statusbit. " Maybe we can have an exception for the device stop/pause in this case. Thanks > > Stefan [-- Attachment #2: Type: text/html, Size: 5392 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [virtio-comment] [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility 2021-03-25 2:42 ` Jason Wang @ 2021-03-25 9:59 ` Stefan Hajnoczi 0 siblings, 0 replies; 21+ messages in thread From: Stefan Hajnoczi @ 2021-03-25 9:59 UTC (permalink / raw) To: Jason Wang; +Cc: virtio-comment [-- Attachment #1: Type: text/plain, Size: 2812 bytes --] On Thu, Mar 25, 2021 at 10:42:38AM +0800, Jason Wang wrote: > > 在 2021/3/24 下午5:38, Stefan Hajnoczi 写道: > > On Wed, Mar 24, 2021 at 04:15:55PM +0800, Jason Wang wrote: > > > 在 2021/3/23 下午6:27, Stefan Hajnoczi 写道: > > > > On Mon, Mar 22, 2021 at 11:47:16AM +0800, Jason Wang wrote: > > > > > +The \field{used_wrap_counter} field is the wrap counter that is used > > > > > +by the device. > > > > > + > > > > > +The used state is unnecessary when VIRTIO_RING_F_PACKED is not > > > > > +negotiated. > > > > > + > > > > > +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}. > > > > > + > > > > > +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State} > > > > > + > > > > > +If VIRTIO_F_QUEUE_STATE has been negotiated, a driver MUST set the > > > > > +state of each virtqueue after setting the FEATURE_OK status bit but > > > > > +before setting the DRIVER_OK status bit. > > > > > + > > > > > +If VIRTIO_F_QUEUE_STATE has been negotiated. a driver MUST reset the > > > > s/negotiated./negotiated,/ > > > > > > > > > +device before getting the virtqueue state. > > > > Interesting approach, but it makes sense that the device must be stopped > > > > before we mess with the queue state :). > > > > > > > > I wonder whether requiring the device to keep state across reset will > > > > cause issues in the future or for testing/fuzzing. > > > > > > > > Another approach would have been to add a new status register bit for > > > > pausing the device. That reminds me of vhost. > > > > > > That's the way I did here: > > > https://lists.oasis-open.org/archives/virtio-comment/202012/msg00027.html > > That design seems less risky. I think modifying the semantics of device > > reset might end up being a problem. Defining an explicit paused state > > for saving/loading state seems cleaner. > > > > Why did you end up switching approaches? > > > I try to seek some short-cut withut introducing new status bit but I end up > realizing it might not work as expected. > > Another reason is that, by using new status bit, we need to ability to > resume the device by clearing the bit which is somehow conflict with what > spec requries: > > " > > 2.1.1 Driver Requirements: Device Status Field > > The driver MUST NOT clear adevice statusbit. > > " > > Maybe we can have an exception for the device stop/pause in this case. Yes, I think that is simply because the VIRTIO device lifecycle never needed to go back to a previous lifecycle state. It was a unidirectional state machine. The device stop/pause state seems like a valid case of going back to a previous lifecycle state and I don't think it breaks anything in the device model. Stefan [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* [virtio-comment] [PATCH V2 2/2] virtio-pci: implement VIRTIO_F_QUEUE_STATE 2021-03-22 3:47 [virtio-comment] [PATCH V2 0/2] Introduce VIRTIO_F_QUEUE_STATE Jason Wang 2021-03-22 3:47 ` [virtio-comment] [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility Jason Wang @ 2021-03-22 3:47 ` Jason Wang 2021-03-23 10:02 ` Stefan Hajnoczi 2021-03-23 10:40 ` [virtio-comment] [PATCH V2 0/2] Introduce VIRTIO_F_QUEUE_STATE Stefan Hajnoczi 2 siblings, 1 reply; 21+ messages in thread From: Jason Wang @ 2021-03-22 3:47 UTC (permalink / raw) To: mst, virtio-comment Cc: eperezma, lulu, rob.miller, pasic, sgarzare, cohuck, jim.harford, Jason Wang This patch adds two new le16 fields to common configruation structure to support VIRTIO_F_QUEUE_STATE. Signed-off-by: Jason Wang <jasowang@redhat.com> --- content.tex | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/content.tex b/content.tex index af39111..adfc279 100644 --- a/content.tex +++ b/content.tex @@ -914,6 +914,8 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport le64 queue_driver; /* read-write */ le64 queue_device; /* read-write */ le16 queue_notify_data; /* read-only for driver */ + le16 queue_avail_state; /* read-write */ + le16 queue_used_state; /* read-write */ }; \end{lstlisting} @@ -993,6 +995,16 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport may benefit from providing another value, for example an internal virtqueue identifier, or an internal offset related to the virtqueue number. \end{note} + +\item[\field{queue_avail_state}] + This field exists only if VIRTIO_F_QUEUE_STATE has been + negotiated. The driver sets and gets the available state of + the virtqueue here (see \ref{sec:Virtqueues / Virtqueue State}). + +\item[\field{queue_used_state}] + This field exists only if VIRTIO_F_QUEUE_STATE has been + negotiated. The driver sets and gets the used state of the + virtqueue here (see \ref{sec:Virtqueues / Virtqueue State}). \end{description} \devicenormative{\paragraph}{Common configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Common configuration structure layout} -- 2.24.3 (Apple Git-128) This publicly archived list offers a means to provide input to the OASIS Virtual I/O Device (VIRTIO) TC. In order to verify user consent to the Feedback License terms and to minimize spam in the list archive, subscription is required before posting. Subscribe: virtio-comment-subscribe@lists.oasis-open.org Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org List help: virtio-comment-help@lists.oasis-open.org List archive: https://lists.oasis-open.org/archives/virtio-comment/ Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists Committee: https://www.oasis-open.org/committees/virtio/ Join OASIS: https://www.oasis-open.org/join/ ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [virtio-comment] [PATCH V2 2/2] virtio-pci: implement VIRTIO_F_QUEUE_STATE 2021-03-22 3:47 ` [virtio-comment] [PATCH V2 2/2] virtio-pci: implement VIRTIO_F_QUEUE_STATE Jason Wang @ 2021-03-23 10:02 ` Stefan Hajnoczi 0 siblings, 0 replies; 21+ messages in thread From: Stefan Hajnoczi @ 2021-03-23 10:02 UTC (permalink / raw) To: Jason Wang Cc: mst, virtio-comment, eperezma, lulu, rob.miller, pasic, sgarzare, cohuck, jim.harford [-- Attachment #1: Type: text/plain, Size: 179 bytes --] On Mon, Mar 22, 2021 at 11:47:17AM +0800, Jason Wang wrote: > This patch adds two new le16 fields to common configruation structure If you respin: s/configruation/configuration/ [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [virtio-comment] [PATCH V2 0/2] Introduce VIRTIO_F_QUEUE_STATE 2021-03-22 3:47 [virtio-comment] [PATCH V2 0/2] Introduce VIRTIO_F_QUEUE_STATE Jason Wang 2021-03-22 3:47 ` [virtio-comment] [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility Jason Wang 2021-03-22 3:47 ` [virtio-comment] [PATCH V2 2/2] virtio-pci: implement VIRTIO_F_QUEUE_STATE Jason Wang @ 2021-03-23 10:40 ` Stefan Hajnoczi 2021-03-24 7:05 ` Jason Wang 2 siblings, 1 reply; 21+ messages in thread From: Stefan Hajnoczi @ 2021-03-23 10:40 UTC (permalink / raw) To: Jason Wang Cc: mst, virtio-comment, eperezma, lulu, rob.miller, pasic, sgarzare, cohuck, jim.harford [-- Attachment #1: Type: text/plain, Size: 1243 bytes --] On Mon, Mar 22, 2021 at 11:47:15AM +0800, Jason Wang wrote: > This is a new version to support VIRTIO_F_QUEUE_STATE. The feautre > extends the basic facility to allow the driver to set and get device > internal virtqueue state. This main motivation is to support live > migration of virtio devices. Can you describe the use cases that this interface covers as well as the steps involved in migrating a device? Traditionally live migration was transparent to the VIRTIO driver because it was performed by the hypervisor. I know you're aware but I think it's worth mentioning that this only supports stateless devices. Even the simple virtio-blk device has state in QEMU's implementation. If an I/O request fails it can be held by the device and resumed after live migration instead of failing the request immediately. The list of held requests needs to be migrated with the device and is not part of the virtqueue state. I'm concerned that using device reset will not work once this interface is extended to support device-specific state (e.g. the virtio-blk failed request list). There could be situations where reset really needs to reset (e.g. freeing I/O resources) and the device therefore cannot hold on to state across reset. Stefan [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [virtio-comment] [PATCH V2 0/2] Introduce VIRTIO_F_QUEUE_STATE 2021-03-23 10:40 ` [virtio-comment] [PATCH V2 0/2] Introduce VIRTIO_F_QUEUE_STATE Stefan Hajnoczi @ 2021-03-24 7:05 ` Jason Wang 2021-03-24 10:05 ` Stefan Hajnoczi 0 siblings, 1 reply; 21+ messages in thread From: Jason Wang @ 2021-03-24 7:05 UTC (permalink / raw) To: Stefan Hajnoczi Cc: mst, virtio-comment, eperezma, lulu, rob.miller, pasic, sgarzare, cohuck, jim.harford 在 2021/3/23 下午6:40, Stefan Hajnoczi 写道: > On Mon, Mar 22, 2021 at 11:47:15AM +0800, Jason Wang wrote: >> This is a new version to support VIRTIO_F_QUEUE_STATE. The feautre >> extends the basic facility to allow the driver to set and get device >> internal virtqueue state. This main motivation is to support live >> migration of virtio devices. > Can you describe the use cases that this interface covers as well as the > steps involved in migrating a device? Yes. I can describe the steps for live migrating virtio-net device. For other devices, we probably need other state. > Traditionally live migration was > transparent to the VIRTIO driver because it was performed by the > hypervisor. Right, but it could be possible that we may want live migrate between hardware virtio-pci devices. So it's up to the hypversior to save and restore states silently without the notice of guest driver as what we did for vhost. > > I know you're aware but I think it's worth mentioning that this only > supports stateless devices. Yes, that's why it's a queue state not a device state. > Even the simple virtio-blk device has state > in QEMU's implementation. If an I/O request fails it can be held by the > device and resumed after live migration instead of failing the request > immediately. The list of held requests needs to be migrated with the > device and is not part of the virtqueue state. Yes, I think we need to extend virtio spec to support save and restore device state. But anyway the virtqueue state is the infrastructure which should be introdouced first. > > I'm concerned that using device reset will not work once this interface > is extended to support device-specific state (e.g. the virtio-blk failed > request list). There could be situations where reset really needs to > reset (e.g. freeing I/O resources) and the device therefore cannot hold > on to state across reset. Good point. So here're some ways: 1) reuse device reset that is done in this patch 2) intorduce a new device status like what has been done in [1] 3) using queue_enable (as what has been done in the virtio-mmio, pci forbids to stop a queue currently, we may need to extend that) 4) use device specific way to stop the datapath Reusing device reset looks like a shortcut that might not be easy for stateful device as you said. 2) looks more general. 3) have the issues that it doesn't forbid the config changed. And 4) is also proposed by you and Michael. My understanding is that there should be no fundamental differences between 2) and 4). So I tend to respin [1], do you have any other ideas? Thanks [1] https://lists.oasis-open.org/archives/virtio-comment/202012/msg00027.html > > Stefan This publicly archived list offers a means to provide input to the OASIS Virtual I/O Device (VIRTIO) TC. In order to verify user consent to the Feedback License terms and to minimize spam in the list archive, subscription is required before posting. Subscribe: virtio-comment-subscribe@lists.oasis-open.org Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org List help: virtio-comment-help@lists.oasis-open.org List archive: https://lists.oasis-open.org/archives/virtio-comment/ Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists Committee: https://www.oasis-open.org/committees/virtio/ Join OASIS: https://www.oasis-open.org/join/ ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [virtio-comment] [PATCH V2 0/2] Introduce VIRTIO_F_QUEUE_STATE 2021-03-24 7:05 ` Jason Wang @ 2021-03-24 10:05 ` Stefan Hajnoczi 2021-03-25 2:57 ` Jason Wang 0 siblings, 1 reply; 21+ messages in thread From: Stefan Hajnoczi @ 2021-03-24 10:05 UTC (permalink / raw) To: Jason Wang Cc: mst, virtio-comment, eperezma, lulu, rob.miller, pasic, sgarzare, cohuck, jim.harford [-- Attachment #1: Type: text/plain, Size: 3956 bytes --] On Wed, Mar 24, 2021 at 03:05:30PM +0800, Jason Wang wrote: > > 在 2021/3/23 下午6:40, Stefan Hajnoczi 写道: > > On Mon, Mar 22, 2021 at 11:47:15AM +0800, Jason Wang wrote: > > > This is a new version to support VIRTIO_F_QUEUE_STATE. The feautre > > > extends the basic facility to allow the driver to set and get device > > > internal virtqueue state. This main motivation is to support live > > > migration of virtio devices. > > Can you describe the use cases that this interface covers as well as the > > steps involved in migrating a device? > > > Yes. I can describe the steps for live migrating virtio-net device. For > other devices, we probably need other state. Thanks, describing the steps for virtio-net would be great. > > > > Traditionally live migration was > > transparent to the VIRTIO driver because it was performed by the > > hypervisor. > > > Right, but it could be possible that we may want live migrate between > hardware virtio-pci devices. So it's up to the hypversior to save and > restore states silently without the notice of guest driver as what we did > for vhost. This is where I'd like to understand the steps in detail. The set/get state functionality introduced in this spec change requires that the hypervisor has access to the device's hardware registers - the same registers that the guest is also using. I'd like to understand the lifecycle and how conflicts between the hypervisor and the guest are avoided (unless this is integrated into vDPA/VFIO/SR-IOV in a way that I haven't thought of?). > > > > > > I know you're aware but I think it's worth mentioning that this only > > supports stateless devices. > > > Yes, that's why it's a queue state not a device state. > > > > Even the simple virtio-blk device has state > > in QEMU's implementation. If an I/O request fails it can be held by the > > device and resumed after live migration instead of failing the request > > immediately. The list of held requests needs to be migrated with the > > device and is not part of the virtqueue state. > > > Yes, I think we need to extend virtio spec to support save and restore > device state. But anyway the virtqueue state is the infrastructure which > should be introdouced first. Introducing virtqueue state save/load first seems fine, but before committing to a spec cange we need an approximate plan for per-device state so that it's clear the design can be extended to cover that case in the future. > > > > I'm concerned that using device reset will not work once this interface > > is extended to support device-specific state (e.g. the virtio-blk failed > > request list). There could be situations where reset really needs to > > reset (e.g. freeing I/O resources) and the device therefore cannot hold > > on to state across reset. > > > Good point. So here're some ways: > > > 1) reuse device reset that is done in this patch > 2) intorduce a new device status like what has been done in [1] > 3) using queue_enable (as what has been done in the virtio-mmio, pci forbids > to stop a queue currently, we may need to extend that) > 4) use device specific way to stop the datapath > > Reusing device reset looks like a shortcut that might not be easy for > stateful device as you said. 2) looks more general. 3) have the issues that > it doesn't forbid the config changed. And 4) is also proposed by you and > Michael. > > My understanding is that there should be no fundamental differences between > 2) and 4). So I tend to respin [1], do you have any other ideas? 2 or 4 sound good. I prefer 2 since one standard interface will be less work and complexity than multiple device-specific ways of stopping the data path. 3 is more flexible but needs to be augmented with a way to pause the entire device. It could be added on top of 2 or 4, if necessary, in the future. Stefan [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [virtio-comment] [PATCH V2 0/2] Introduce VIRTIO_F_QUEUE_STATE 2021-03-24 10:05 ` Stefan Hajnoczi @ 2021-03-25 2:57 ` Jason Wang 2021-03-25 10:03 ` [virtio-comment] [PATCH V2 0/2] Introduce VIRTIO_F_QUEUE_STATE\\ Stefan Hajnoczi 0 siblings, 1 reply; 21+ messages in thread From: Jason Wang @ 2021-03-25 2:57 UTC (permalink / raw) To: Stefan Hajnoczi Cc: mst, virtio-comment, eperezma, lulu, rob.miller, pasic, sgarzare, cohuck, jim.harford 在 2021/3/24 下午6:05, Stefan Hajnoczi 写道: > On Wed, Mar 24, 2021 at 03:05:30PM +0800, Jason Wang wrote: >> 在 2021/3/23 下午6:40, Stefan Hajnoczi 写道: >>> On Mon, Mar 22, 2021 at 11:47:15AM +0800, Jason Wang wrote: >>>> This is a new version to support VIRTIO_F_QUEUE_STATE. The feautre >>>> extends the basic facility to allow the driver to set and get device >>>> internal virtqueue state. This main motivation is to support live >>>> migration of virtio devices. >>> Can you describe the use cases that this interface covers as well as the >>> steps involved in migrating a device? >> >> Yes. I can describe the steps for live migrating virtio-net device. For >> other devices, we probably need other state. > Thanks, describing the steps for virtio-net would be great. > >> >>> Traditionally live migration was >>> transparent to the VIRTIO driver because it was performed by the >>> hypervisor. >> >> Right, but it could be possible that we may want live migrate between >> hardware virtio-pci devices. So it's up to the hypversior to save and >> restore states silently without the notice of guest driver as what we did >> for vhost. > This is where I'd like to understand the steps in detail. The set/get > state functionality introduced in this spec change requires that the > hypervisor has access to the device's hardware registers - the same > registers that the guest is also using. I'd like to understand the > lifecycle and how conflicts between the hypervisor and the guest are > avoided (unless this is integrated into vDPA/VFIO/SR-IOV in a way that I > haven't thought of?). Let's assume virito device is used through vhost-vdpa. In this case, there's actually two virtio devices 1) The device A that is used by vdpa driver and is connected to vdpa bus (vhost-vDPA). Usually it could be a virtio-pci device. 2) The device B that is emulated by Qemu. It could be virito-pci or even virtio-mmio device. So what guest driver can see is device B, and it can only access the status bit of device B. From the view of Qemu, device A works more like a vhost backend. It means it can stop device A (either via reset or other way like dedicated status bit) without noticing guest (touching the device status bit for device A). When we need to live migrate the VM: 1) Qemu need to stop device B (e.g stop vhost-vDPA) 2) Qemu get virtqueue states from device B 3) The virtqueue state will be passed from source to destinition 4) Qemu recovered the virtqueue states to device C which is the virtio/vDPA device that is on the destination 5) Qemu resume the dev C (e.g start vhost-vDPA) > >> >>> I know you're aware but I think it's worth mentioning that this only >>> supports stateless devices. >> >> Yes, that's why it's a queue state not a device state. >> >> >>> Even the simple virtio-blk device has state >>> in QEMU's implementation. If an I/O request fails it can be held by the >>> device and resumed after live migration instead of failing the request >>> immediately. The list of held requests needs to be migrated with the >>> device and is not part of the virtqueue state. >> >> Yes, I think we need to extend virtio spec to support save and restore >> device state. But anyway the virtqueue state is the infrastructure which >> should be introdouced first. > Introducing virtqueue state save/load first seems fine, but before > committing to a spec cange we need an approximate plan for per-device > state so that it's clear the design can be extended to cover that case > in the future. Yes, so as discussed. We might at least requires a API to fetch the inflight descriptors. Haven't thought it deeply but some possible ways: 1) transport specific way 2) generic method like a control vq command 1) looks simpler but may end up with function duplication, 2) may need some extension on the current virtio-blk. Actually we had 3), fetchn the information from the management deviec (like PF). > >>> I'm concerned that using device reset will not work once this interface >>> is extended to support device-specific state (e.g. the virtio-blk failed >>> request list). There could be situations where reset really needs to >>> reset (e.g. freeing I/O resources) and the device therefore cannot hold >>> on to state across reset. >> >> Good point. So here're some ways: >> >> >> 1) reuse device reset that is done in this patch >> 2) intorduce a new device status like what has been done in [1] >> 3) using queue_enable (as what has been done in the virtio-mmio, pci forbids >> to stop a queue currently, we may need to extend that) >> 4) use device specific way to stop the datapath >> >> Reusing device reset looks like a shortcut that might not be easy for >> stateful device as you said. 2) looks more general. 3) have the issues that >> it doesn't forbid the config changed. And 4) is also proposed by you and >> Michael. >> >> My understanding is that there should be no fundamental differences between >> 2) and 4). So I tend to respin [1], do you have any other ideas? > 2 or 4 sound good. I prefer 2 since one standard interface will be less > work and complexity than multiple device-specific ways of stopping the > data path. Yes. > > 3 is more flexible but needs to be augmented with a way to pause the > entire device. It could be added on top of 2 or 4, if necessary, in the > future. > > Stefan Right, let me try to continue the approach of new status bit to see if it works. Thanks This publicly archived list offers a means to provide input to the OASIS Virtual I/O Device (VIRTIO) TC. In order to verify user consent to the Feedback License terms and to minimize spam in the list archive, subscription is required before posting. Subscribe: virtio-comment-subscribe@lists.oasis-open.org Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org List help: virtio-comment-help@lists.oasis-open.org List archive: https://lists.oasis-open.org/archives/virtio-comment/ Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists Committee: https://www.oasis-open.org/committees/virtio/ Join OASIS: https://www.oasis-open.org/join/ ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [virtio-comment] [PATCH V2 0/2] Introduce VIRTIO_F_QUEUE_STATE\\ 2021-03-25 2:57 ` Jason Wang @ 2021-03-25 10:03 ` Stefan Hajnoczi 0 siblings, 0 replies; 21+ messages in thread From: Stefan Hajnoczi @ 2021-03-25 10:03 UTC (permalink / raw) To: Jason Wang Cc: mst, virtio-comment, eperezma, lulu, rob.miller, pasic, sgarzare, cohuck, jim.harford [-- Attachment #1: Type: text/plain, Size: 201 bytes --] On Thu, Mar 25, 2021 at 10:57:26AM +0800, Jason Wang wrote: > Right, let me try to continue the approach of new status bit to see if it > works. Thanks for the explanations, this sounds good. Stefan [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH V2 0/2] Vitqueue State Synchronization @ 2021-07-06 4:33 Jason Wang 2021-07-06 4:33 ` [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility Jason Wang 0 siblings, 1 reply; 21+ messages in thread From: Jason Wang @ 2021-07-06 4:33 UTC (permalink / raw) To: mst, jasowang, virtio-comment, virtio-dev Cc: stefanha, mgurtovoy, cohuck, eperezma, oren, shahafs, parav, bodong, amikheev, pasic Hi All: This is an updated version to implement virtqueue state synchronization which is a must for the migration support. The first patch introduces virtqueue states as a new basic facility of the virtio device. This is used by the driver to save and restore virtqueue state. The states were split into available state and used state to ease the transport specific implementation. It is also allowed for the device to have its own device specific way to save and resotre extra virtqueue states like in flight request. The second patch introduce a new status bit STOP. This bit is used for the driver to stop the device. The major difference from reset is that STOP must preserve all the virtqueue state plus the device state. A driver can then: - Get the virtqueue state if STOP status bit is set - Set the virtqueue state after FEATURE_OK but before DRIVER_OK Device specific state synchronization could be built on top. Please review. Thanks Changes from V1: - introduce used_idx for synchronize used state for split virtqueue, this is needed since the used ring is read only by the driver. - stick to the uni-directional state machine, this means driver is forbidden to clear STOP, the only way to 'resume' the device is to reset and re-initialization the device. This simplify the implementation and make it works more like a vhost device. - mandate the virtqueue state setting during device initialization - clarify the steps that are required for saving the virtqueue state - allow the device to have its specifc way to save and restore extra virtqueue state - rename DEVICE_STOPPED to STOP - various other tweaks Jason Wang (2): virtio: introduce virtqueue state as basic facility virtio: introduce STOP status bit content.tex | 180 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 180 insertions(+) -- 2.25.1 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility 2021-07-06 4:33 [PATCH V2 0/2] Vitqueue State Synchronization Jason Wang @ 2021-07-06 4:33 ` Jason Wang 2021-07-06 9:32 ` Michael S. Tsirkin 2021-07-06 12:27 ` Cornelia Huck 0 siblings, 2 replies; 21+ messages in thread From: Jason Wang @ 2021-07-06 4:33 UTC (permalink / raw) To: mst, jasowang, virtio-comment, virtio-dev Cc: stefanha, mgurtovoy, cohuck, eperezma, oren, shahafs, parav, bodong, amikheev, pasic This patch adds new device facility to save and restore virtqueue state. The virtqueue state is split into two parts: - The available state: The state that is used for read the next available buffer. - The used state: The state that is used for making buffer used. Note that, there could be devices that is required to set and get the requests that are being processed by the device. I leave such API to be device specific. This facility could be used by both migration and device diagnostic. Signed-off-by: Jason Wang <jasowang@redhat.com> --- content.tex | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 117 insertions(+) diff --git a/content.tex b/content.tex index 620c0e2..8877b6f 100644 --- a/content.tex +++ b/content.tex @@ -385,6 +385,116 @@ \section{Exporting Objects}\label{sec:Basic Facilities of a Virtio Device / Expo types. It is RECOMMENDED that devices generate version 4 UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}. +\section{Virtqueue State}\label{sec:Virtqueues / Virtqueue State} + +When VIRTIO_F_RING_STATE is negotiated, the driver can set and +get the device internal virtqueue state through the following +fields. The way to access those fields is transport specific. + +\subsection{\field{Available State} Field} + +The \field{Available State} field is two bytes for the driver to get +or set the state that is used by the virtqueue to read for the next +available buffer. + +When VIRTIO_F_RING_PACKED is not negotiated, it contains: + +\begin{lstlisting} +le16 { + last_avail_idx : 16; +} avail_state; +\end{lstlisting} + +The \field{last_avail_idx} field indicates where the device would read +for the next index from the virtqueue available ring(modulo the queue + size). This starts at the value set by the driver, and increases. + +When VIRTIO_F_RING_PACKED is negotiated, it contains: + +\begin{lstlisting} +le16 { + last_avail_idx : 15; + last_avail_wrap_counter : 1; +} avail_state; +\end{lstlisting} + +The \field{last_avail_idx} field indicates where the device would read for +the next descriptor head from the descriptor ring. This starts at the +value set by the driver and wraps around when reaching the end of the +ring. + +The \field{last_avail_wrap_counter} field indicates the last Driver Ring +Wrap Counter that is observed by the device. This starts at the value +set by the driver, and is flipped when reaching the end of the ring. + +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}. + +\subsection{\field{Used State} Field} + +The \field{Used State} field is two bytes for the driver to set and +get the state used by the virtqueue to make buffer used. + +When VIRTIO_F_RING_PACKED is not negotiated, the used state contains: + +\begin{lstlisting} +le16 { + used_idx : 16; +} used_state; +\end{lstlisting} + +The \field{used_idx} where the device would write the next used +descriptor head to the used ring (modulo the queue size). This starts +at the value set by the driver, and increases. It is easy to see this +is the initial value of the \field{idx} in the used ring. + +See also \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring} + +When VIRTIO_F_RING_PACKED is negotiated, the used state contains: + +\begin{lstlisting} +le16 { + used_idx : 15; + used_wrap_counter : 1; +} used_state; +\end{lstlisting} + +The \field{used_idx} indicates where the device would write the next used +descriptor head to the descriptor ring. This starts at the value set +by the driver, and warps around when reaching the end of the ring. + +\field{used_wrap_counter} is the Device Ring Wrap Counter. This starts +at the value set by the driver, and is flipped when reaching the end +of the ring. + +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}. + +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State} + +If VIRTIO_F_RING_STATE has been negotiated: +\begin{itemize} +\item A driver MUST NOT set the virtqueue state before setting the + FEATURE_OK status bit. +\item A driver MUST NOT set the virtqueue state after setting the + DRIVER_OK status bit. +\end{itemize} + +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State} + +If VIRTIO_F_RING_STATE has not been negotiated, a device MUST ingore +the read and write to the virtqueue state. + +If VIRTIO_F_RING_STATE has been negotiated: +\begin{itemize} +\item A device SHOULD ignore the write to the virtqueue state if the +FEATURE_OK status bit is not set. +\item A device SHOULD ignore the write to the virtqueue state if the +DRIVER_OK status bit is set. +\end{itemize} + +If VIRTIO_F_RING_STATE has been negotiated, a device MAY has its +device-specific way for the driver to set and get extra virtqueue +states such as in flight requests. + \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation} We start with an overview of device initialization, then expand on the @@ -420,6 +530,9 @@ \section{Device Initialization}\label{sec:General Initialization And Device Oper device, optional per-bus setup, reading and possibly writing the device's virtio configuration space, and population of virtqueues. +\item\label{itm:General Initialization And Device Operation / Device + Initialization / Virtqueue State Setup} When VIRTIO_F_RING_STATE has been negotiated, perform virtqueue state setup, including the initialization of the per virtqueue available state, used state and the possible device specific virtqueue state. + \item\label{itm:General Initialization And Device Operation / Device Initialization / Set DRIVER-OK} Set the DRIVER_OK status bit. At this point the device is ``live''. \end{enumerate} @@ -6596,6 +6709,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits} transport specific. For more details about driver notifications over PCI see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}. + \item[VIRTIO_F_RING_STATE(40)] This feature indicates that the driver + can set and get the device internal virtqueue state. + See \ref{sec:Virtqueues / Virtqueue State}~\nameref{sec:Virtqueues / Virtqueue State}. + \end{description} \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits} -- 2.25.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility 2021-07-06 4:33 ` [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility Jason Wang @ 2021-07-06 9:32 ` Michael S. Tsirkin 2021-07-06 17:09 ` Eugenio Perez Martin 2021-07-06 12:27 ` Cornelia Huck 1 sibling, 1 reply; 21+ messages in thread From: Michael S. Tsirkin @ 2021-07-06 9:32 UTC (permalink / raw) To: Jason Wang Cc: virtio-comment, virtio-dev, stefanha, mgurtovoy, cohuck, eperezma, oren, shahafs, parav, bodong, amikheev, pasic On Tue, Jul 06, 2021 at 12:33:33PM +0800, Jason Wang wrote: > This patch adds new device facility to save and restore virtqueue > state. The virtqueue state is split into two parts: > > - The available state: The state that is used for read the next > available buffer. > - The used state: The state that is used for making buffer used. > > Note that, there could be devices that is required to set and get the > requests that are being processed by the device. I leave such API to > be device specific. > > This facility could be used by both migration and device diagnostic. > > Signed-off-by: Jason Wang <jasowang@redhat.com> Hi Jason! I feel that for use-cases such as SRIOV, the facility to save/restore vq should be part of a PF that is there needs to be a way for one virtio device to address the state of another one. Thoughts? > --- > content.tex | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 117 insertions(+) > > diff --git a/content.tex b/content.tex > index 620c0e2..8877b6f 100644 > --- a/content.tex > +++ b/content.tex > @@ -385,6 +385,116 @@ \section{Exporting Objects}\label{sec:Basic Facilities of a Virtio Device / Expo > types. It is RECOMMENDED that devices generate version 4 > UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}. > > +\section{Virtqueue State}\label{sec:Virtqueues / Virtqueue State} > + > +When VIRTIO_F_RING_STATE is negotiated, the driver can set and > +get the device internal virtqueue state through the following > +fields. The way to access those fields is transport specific. > + > +\subsection{\field{Available State} Field} > + > +The \field{Available State} field is two bytes for the driver to get > +or set the state that is used by the virtqueue to read for the next > +available buffer. > + > +When VIRTIO_F_RING_PACKED is not negotiated, it contains: > + > +\begin{lstlisting} > +le16 { > + last_avail_idx : 16; > +} avail_state; > +\end{lstlisting} > + > +The \field{last_avail_idx} field indicates where the device would read > +for the next index from the virtqueue available ring(modulo the queue > + size). This starts at the value set by the driver, and increases. > + > +When VIRTIO_F_RING_PACKED is negotiated, it contains: > + > +\begin{lstlisting} > +le16 { > + last_avail_idx : 15; > + last_avail_wrap_counter : 1; > +} avail_state; > +\end{lstlisting} > + > +The \field{last_avail_idx} field indicates where the device would read for > +the next descriptor head from the descriptor ring. This starts at the > +value set by the driver and wraps around when reaching the end of the > +ring. > + > +The \field{last_avail_wrap_counter} field indicates the last Driver Ring > +Wrap Counter that is observed by the device. This starts at the value > +set by the driver, and is flipped when reaching the end of the ring. > + > +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}. > + > +\subsection{\field{Used State} Field} > + > +The \field{Used State} field is two bytes for the driver to set and > +get the state used by the virtqueue to make buffer used. > + > +When VIRTIO_F_RING_PACKED is not negotiated, the used state contains: > + > +\begin{lstlisting} > +le16 { > + used_idx : 16; > +} used_state; > +\end{lstlisting} > + > +The \field{used_idx} where the device would write the next used > +descriptor head to the used ring (modulo the queue size). This starts > +at the value set by the driver, and increases. It is easy to see this > +is the initial value of the \field{idx} in the used ring. > + > +See also \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring} > + > +When VIRTIO_F_RING_PACKED is negotiated, the used state contains: > + > +\begin{lstlisting} > +le16 { > + used_idx : 15; > + used_wrap_counter : 1; > +} used_state; > +\end{lstlisting} > + > +The \field{used_idx} indicates where the device would write the next used > +descriptor head to the descriptor ring. This starts at the value set > +by the driver, and warps around when reaching the end of the ring. > + > +\field{used_wrap_counter} is the Device Ring Wrap Counter. This starts > +at the value set by the driver, and is flipped when reaching the end > +of the ring. > + > +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}. Above only fully describes the vq state if descriptors are used in order or at least all out of order descriptors are consumed at time of save. Adding later option to devices such as net will need extra spec work. > +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State} > + > +If VIRTIO_F_RING_STATE has been negotiated: > +\begin{itemize} > +\item A driver MUST NOT set the virtqueue state before setting the > + FEATURE_OK status bit. > +\item A driver MUST NOT set the virtqueue state after setting the > + DRIVER_OK status bit. > +\end{itemize} > + > +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State} > + > +If VIRTIO_F_RING_STATE has not been negotiated, a device MUST ingore > +the read and write to the virtqueue state. > + > +If VIRTIO_F_RING_STATE has been negotiated: > +\begin{itemize} > +\item A device SHOULD ignore the write to the virtqueue state if the > +FEATURE_OK status bit is not set. > +\item A device SHOULD ignore the write to the virtqueue state if the > +DRIVER_OK status bit is set. > +\end{itemize} > + > +If VIRTIO_F_RING_STATE has been negotiated, a device MAY has its may have? should also go into a normative section > +device-specific way for the driver to set and get extra virtqueue > +states such as in flight requests. > + > \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation} > > We start with an overview of device initialization, then expand on the > @@ -420,6 +530,9 @@ \section{Device Initialization}\label{sec:General Initialization And Device Oper > device, optional per-bus setup, reading and possibly writing the > device's virtio configuration space, and population of virtqueues. > > +\item\label{itm:General Initialization And Device Operation / Device > + Initialization / Virtqueue State Setup} When VIRTIO_F_RING_STATE has been negotiated, perform virtqueue state setup, including the initialization of the per virtqueue available state, used state and the possible device specific virtqueue state. > + > \item\label{itm:General Initialization And Device Operation / Device Initialization / Set DRIVER-OK} Set the DRIVER_OK status bit. At this point the device is > ``live''. > \end{enumerate} > @@ -6596,6 +6709,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits} > transport specific. > For more details about driver notifications over PCI see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}. > > + \item[VIRTIO_F_RING_STATE(40)] This feature indicates that the driver > + can set and get the device internal virtqueue state. > + See \ref{sec:Virtqueues / Virtqueue State}~\nameref{sec:Virtqueues / Virtqueue State}. > + > \end{description} > > \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits} > -- > 2.25.1 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility 2021-07-06 9:32 ` Michael S. Tsirkin @ 2021-07-06 17:09 ` Eugenio Perez Martin 2021-07-06 19:08 ` Michael S. Tsirkin 0 siblings, 1 reply; 21+ messages in thread From: Eugenio Perez Martin @ 2021-07-06 17:09 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Jason Wang, virtio-comment, Virtio-Dev, Stefan Hajnoczi, Max Gurtovoy, Cornelia Huck, Oren Duer, Shahaf Shuler, Parav Pandit, Bodong Wang, Alexander Mikheev, Halil Pasic On Tue, Jul 6, 2021 at 11:32 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Jul 06, 2021 at 12:33:33PM +0800, Jason Wang wrote: > > This patch adds new device facility to save and restore virtqueue > > state. The virtqueue state is split into two parts: > > > > - The available state: The state that is used for read the next > > available buffer. > > - The used state: The state that is used for making buffer used. > > > > Note that, there could be devices that is required to set and get the > > requests that are being processed by the device. I leave such API to > > be device specific. > > > > This facility could be used by both migration and device diagnostic. > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > Hi Jason! > I feel that for use-cases such as SRIOV, > the facility to save/restore vq should be part of a PF > that is there needs to be a way for one virtio device to > address the state of another one. > Hi! In my opinion we should go the other way around: To make features as orthogonal/independent as possible, and just make them work together if we have to. In this particular case, I think it should be easier to decide how to report status, its needs, etc for a VF, and then open the possibility for the PF to query or set them, reusing format, behavior, etc. as much as possible. I think that the most controversial point about doing it non-SR IOV way is the exposing of these features/fields to the guest using specific transport facilities, like PCI common config. However I think it should not be hard for the hypervisor to intercept them and even to expose them conditionally. Please correct me if this guessing was not right and you had other concerns. > Thoughts? > > > --- > > content.tex | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 117 insertions(+) > > > > diff --git a/content.tex b/content.tex > > index 620c0e2..8877b6f 100644 > > --- a/content.tex > > +++ b/content.tex > > @@ -385,6 +385,116 @@ \section{Exporting Objects}\label{sec:Basic Facilities of a Virtio Device / Expo > > types. It is RECOMMENDED that devices generate version 4 > > UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}. > > > > +\section{Virtqueue State}\label{sec:Virtqueues / Virtqueue State} > > + > > +When VIRTIO_F_RING_STATE is negotiated, the driver can set and > > +get the device internal virtqueue state through the following > > +fields. The way to access those fields is transport specific. > > + > > +\subsection{\field{Available State} Field} > > + > > +The \field{Available State} field is two bytes for the driver to get > > +or set the state that is used by the virtqueue to read for the next > > +available buffer. > > + > > +When VIRTIO_F_RING_PACKED is not negotiated, it contains: > > + > > +\begin{lstlisting} > > +le16 { > > + last_avail_idx : 16; > > +} avail_state; > > +\end{lstlisting} > > + > > +The \field{last_avail_idx} field indicates where the device would read > > +for the next index from the virtqueue available ring(modulo the queue > > + size). This starts at the value set by the driver, and increases. > > + > > +When VIRTIO_F_RING_PACKED is negotiated, it contains: > > + > > +\begin{lstlisting} > > +le16 { > > + last_avail_idx : 15; > > + last_avail_wrap_counter : 1; > > +} avail_state; > > +\end{lstlisting} > > + > > +The \field{last_avail_idx} field indicates where the device would read for > > +the next descriptor head from the descriptor ring. This starts at the > > +value set by the driver and wraps around when reaching the end of the > > +ring. > > + > > +The \field{last_avail_wrap_counter} field indicates the last Driver Ring > > +Wrap Counter that is observed by the device. This starts at the value > > +set by the driver, and is flipped when reaching the end of the ring. > > + > > +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}. > > + > > +\subsection{\field{Used State} Field} > > + > > +The \field{Used State} field is two bytes for the driver to set and > > +get the state used by the virtqueue to make buffer used. > > + > > +When VIRTIO_F_RING_PACKED is not negotiated, the used state contains: > > + > > +\begin{lstlisting} > > +le16 { > > + used_idx : 16; > > +} used_state; > > +\end{lstlisting} > > + > > +The \field{used_idx} where the device would write the next used > > +descriptor head to the used ring (modulo the queue size). This starts > > +at the value set by the driver, and increases. It is easy to see this > > +is the initial value of the \field{idx} in the used ring. > > + > > +See also \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring} > > + > > +When VIRTIO_F_RING_PACKED is negotiated, the used state contains: > > + > > +\begin{lstlisting} > > +le16 { > > + used_idx : 15; > > + used_wrap_counter : 1; > > +} used_state; > > +\end{lstlisting} > > + > > +The \field{used_idx} indicates where the device would write the next used > > +descriptor head to the descriptor ring. This starts at the value set > > +by the driver, and warps around when reaching the end of the ring. > > + > > +\field{used_wrap_counter} is the Device Ring Wrap Counter. This starts > > +at the value set by the driver, and is flipped when reaching the end > > +of the ring. > > + > > +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}. > > > Above only fully describes the vq state if descriptors > are used in order or at least all out of order descriptors are consumed > at time of save. > I think that the most straightforward solution would be to add something similar to VHOST_USER_GET_INFLIGHT_FD, but without the _FD part. Thanks! > Adding later option to devices such as net will need extra spec work. > > > > +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State} > > + > > +If VIRTIO_F_RING_STATE has been negotiated: > > +\begin{itemize} > > +\item A driver MUST NOT set the virtqueue state before setting the > > + FEATURE_OK status bit. > > +\item A driver MUST NOT set the virtqueue state after setting the > > + DRIVER_OK status bit. > > +\end{itemize} > > + > > +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State} > > + > > +If VIRTIO_F_RING_STATE has not been negotiated, a device MUST ingore > > +the read and write to the virtqueue state. > > + > > +If VIRTIO_F_RING_STATE has been negotiated: > > +\begin{itemize} > > +\item A device SHOULD ignore the write to the virtqueue state if the > > +FEATURE_OK status bit is not set. > > +\item A device SHOULD ignore the write to the virtqueue state if the > > +DRIVER_OK status bit is set. > > +\end{itemize} > > + > > +If VIRTIO_F_RING_STATE has been negotiated, a device MAY has its > > > may have? > should also go into a normative section > > > +device-specific way for the driver to set and get extra virtqueue > > +states such as in flight requests. > > + > > \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation} > > > > We start with an overview of device initialization, then expand on the > > @@ -420,6 +530,9 @@ \section{Device Initialization}\label{sec:General Initialization And Device Oper > > device, optional per-bus setup, reading and possibly writing the > > device's virtio configuration space, and population of virtqueues. > > > > +\item\label{itm:General Initialization And Device Operation / Device > > + Initialization / Virtqueue State Setup} When VIRTIO_F_RING_STATE has been negotiated, perform virtqueue state setup, including the initialization of the per virtqueue available state, used state and the possible device specific virtqueue state. > > + > > \item\label{itm:General Initialization And Device Operation / Device Initialization / Set DRIVER-OK} Set the DRIVER_OK status bit. At this point the device is > > ``live''. > > \end{enumerate} > > @@ -6596,6 +6709,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits} > > transport specific. > > For more details about driver notifications over PCI see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}. > > > > + \item[VIRTIO_F_RING_STATE(40)] This feature indicates that the driver > > + can set and get the device internal virtqueue state. > > + See \ref{sec:Virtqueues / Virtqueue State}~\nameref{sec:Virtqueues / Virtqueue State}. > > + > > \end{description} > > > > \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits} > > -- > > 2.25.1 > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility 2021-07-06 17:09 ` Eugenio Perez Martin @ 2021-07-06 19:08 ` Michael S. Tsirkin 2021-07-06 23:49 ` Max Gurtovoy 0 siblings, 1 reply; 21+ messages in thread From: Michael S. Tsirkin @ 2021-07-06 19:08 UTC (permalink / raw) To: Eugenio Perez Martin Cc: Jason Wang, virtio-comment, Virtio-Dev, Stefan Hajnoczi, Max Gurtovoy, Cornelia Huck, Oren Duer, Shahaf Shuler, Parav Pandit, Bodong Wang, Alexander Mikheev, Halil Pasic On Tue, Jul 06, 2021 at 07:09:10PM +0200, Eugenio Perez Martin wrote: > On Tue, Jul 6, 2021 at 11:32 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Tue, Jul 06, 2021 at 12:33:33PM +0800, Jason Wang wrote: > > > This patch adds new device facility to save and restore virtqueue > > > state. The virtqueue state is split into two parts: > > > > > > - The available state: The state that is used for read the next > > > available buffer. > > > - The used state: The state that is used for making buffer used. > > > > > > Note that, there could be devices that is required to set and get the > > > requests that are being processed by the device. I leave such API to > > > be device specific. > > > > > > This facility could be used by both migration and device diagnostic. > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > Hi Jason! > > I feel that for use-cases such as SRIOV, > > the facility to save/restore vq should be part of a PF > > that is there needs to be a way for one virtio device to > > address the state of another one. > > > > Hi! > > In my opinion we should go the other way around: To make features as > orthogonal/independent as possible, and just make them work together > if we have to. In this particular case, I think it should be easier to > decide how to report status, its needs, etc for a VF, and then open > the possibility for the PF to query or set them, reusing format, > behavior, etc. as much as possible. > > I think that the most controversial point about doing it non-SR IOV > way is the exposing of these features/fields to the guest using > specific transport facilities, like PCI common config. However I think > it should not be hard for the hypervisor to intercept them and even to > expose them conditionally. Please correct me if this guessing was not > right and you had other concerns. Possibly. I'd like to see some guidance on how this all will work in practice then. Maybe make it all part of a non-normative section for now. I think that the feature itself is not very useful outside of migration so we don't really gain much by adding it as is without all the other missing pieces. I would say let's see more of the whole picture before we commit. > > Thoughts? > > > > > --- > > > content.tex | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 117 insertions(+) > > > > > > diff --git a/content.tex b/content.tex > > > index 620c0e2..8877b6f 100644 > > > --- a/content.tex > > > +++ b/content.tex > > > @@ -385,6 +385,116 @@ \section{Exporting Objects}\label{sec:Basic Facilities of a Virtio Device / Expo > > > types. It is RECOMMENDED that devices generate version 4 > > > UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}. > > > > > > +\section{Virtqueue State}\label{sec:Virtqueues / Virtqueue State} > > > + > > > +When VIRTIO_F_RING_STATE is negotiated, the driver can set and > > > +get the device internal virtqueue state through the following > > > +fields. The way to access those fields is transport specific. > > > + > > > +\subsection{\field{Available State} Field} > > > + > > > +The \field{Available State} field is two bytes for the driver to get > > > +or set the state that is used by the virtqueue to read for the next > > > +available buffer. > > > + > > > +When VIRTIO_F_RING_PACKED is not negotiated, it contains: > > > + > > > +\begin{lstlisting} > > > +le16 { > > > + last_avail_idx : 16; > > > +} avail_state; > > > +\end{lstlisting} > > > + > > > +The \field{last_avail_idx} field indicates where the device would read > > > +for the next index from the virtqueue available ring(modulo the queue > > > + size). This starts at the value set by the driver, and increases. > > > + > > > +When VIRTIO_F_RING_PACKED is negotiated, it contains: > > > + > > > +\begin{lstlisting} > > > +le16 { > > > + last_avail_idx : 15; > > > + last_avail_wrap_counter : 1; > > > +} avail_state; > > > +\end{lstlisting} > > > + > > > +The \field{last_avail_idx} field indicates where the device would read for > > > +the next descriptor head from the descriptor ring. This starts at the > > > +value set by the driver and wraps around when reaching the end of the > > > +ring. > > > + > > > +The \field{last_avail_wrap_counter} field indicates the last Driver Ring > > > +Wrap Counter that is observed by the device. This starts at the value > > > +set by the driver, and is flipped when reaching the end of the ring. > > > + > > > +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}. > > > + > > > +\subsection{\field{Used State} Field} > > > + > > > +The \field{Used State} field is two bytes for the driver to set and > > > +get the state used by the virtqueue to make buffer used. > > > + > > > +When VIRTIO_F_RING_PACKED is not negotiated, the used state contains: > > > + > > > +\begin{lstlisting} > > > +le16 { > > > + used_idx : 16; > > > +} used_state; > > > +\end{lstlisting} > > > + > > > +The \field{used_idx} where the device would write the next used > > > +descriptor head to the used ring (modulo the queue size). This starts > > > +at the value set by the driver, and increases. It is easy to see this > > > +is the initial value of the \field{idx} in the used ring. > > > + > > > +See also \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring} > > > + > > > +When VIRTIO_F_RING_PACKED is negotiated, the used state contains: > > > + > > > +\begin{lstlisting} > > > +le16 { > > > + used_idx : 15; > > > + used_wrap_counter : 1; > > > +} used_state; > > > +\end{lstlisting} > > > + > > > +The \field{used_idx} indicates where the device would write the next used > > > +descriptor head to the descriptor ring. This starts at the value set > > > +by the driver, and warps around when reaching the end of the ring. > > > + > > > +\field{used_wrap_counter} is the Device Ring Wrap Counter. This starts > > > +at the value set by the driver, and is flipped when reaching the end > > > +of the ring. > > > + > > > +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}. > > > > > > Above only fully describes the vq state if descriptors > > are used in order or at least all out of order descriptors are consumed > > at time of save. > > > > I think that the most straightforward solution would be to add > something similar to VHOST_USER_GET_INFLIGHT_FD, but without the _FD > part. > > Thanks! > > > Adding later option to devices such as net will need extra spec work. > > > > > > > +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State} > > > + > > > +If VIRTIO_F_RING_STATE has been negotiated: > > > +\begin{itemize} > > > +\item A driver MUST NOT set the virtqueue state before setting the > > > + FEATURE_OK status bit. > > > +\item A driver MUST NOT set the virtqueue state after setting the > > > + DRIVER_OK status bit. > > > +\end{itemize} > > > + > > > +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State} > > > + > > > +If VIRTIO_F_RING_STATE has not been negotiated, a device MUST ingore > > > +the read and write to the virtqueue state. > > > + > > > +If VIRTIO_F_RING_STATE has been negotiated: > > > +\begin{itemize} > > > +\item A device SHOULD ignore the write to the virtqueue state if the > > > +FEATURE_OK status bit is not set. > > > +\item A device SHOULD ignore the write to the virtqueue state if the > > > +DRIVER_OK status bit is set. > > > +\end{itemize} > > > + > > > +If VIRTIO_F_RING_STATE has been negotiated, a device MAY has its > > > > > > may have? > > should also go into a normative section > > > > > +device-specific way for the driver to set and get extra virtqueue > > > +states such as in flight requests. > > > + > > > \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation} > > > > > > We start with an overview of device initialization, then expand on the > > > @@ -420,6 +530,9 @@ \section{Device Initialization}\label{sec:General Initialization And Device Oper > > > device, optional per-bus setup, reading and possibly writing the > > > device's virtio configuration space, and population of virtqueues. > > > > > > +\item\label{itm:General Initialization And Device Operation / Device > > > + Initialization / Virtqueue State Setup} When VIRTIO_F_RING_STATE has been negotiated, perform virtqueue state setup, including the initialization of the per virtqueue available state, used state and the possible device specific virtqueue state. > > > + > > > \item\label{itm:General Initialization And Device Operation / Device Initialization / Set DRIVER-OK} Set the DRIVER_OK status bit. At this point the device is > > > ``live''. > > > \end{enumerate} > > > @@ -6596,6 +6709,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits} > > > transport specific. > > > For more details about driver notifications over PCI see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}. > > > > > > + \item[VIRTIO_F_RING_STATE(40)] This feature indicates that the driver > > > + can set and get the device internal virtqueue state. > > > + See \ref{sec:Virtqueues / Virtqueue State}~\nameref{sec:Virtqueues / Virtqueue State}. > > > + > > > \end{description} > > > > > > \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits} > > > -- > > > 2.25.1 > > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility 2021-07-06 19:08 ` Michael S. Tsirkin @ 2021-07-06 23:49 ` Max Gurtovoy 2021-07-07 2:50 ` Jason Wang 0 siblings, 1 reply; 21+ messages in thread From: Max Gurtovoy @ 2021-07-06 23:49 UTC (permalink / raw) To: Michael S. Tsirkin, Eugenio Perez Martin Cc: Jason Wang, virtio-comment, Virtio-Dev, Stefan Hajnoczi, Cornelia Huck, Oren Duer, Shahaf Shuler, Parav Pandit, Bodong Wang, Alexander Mikheev, Halil Pasic On 7/6/2021 10:08 PM, Michael S. Tsirkin wrote: > On Tue, Jul 06, 2021 at 07:09:10PM +0200, Eugenio Perez Martin wrote: >> On Tue, Jul 6, 2021 at 11:32 AM Michael S. Tsirkin <mst@redhat.com> wrote: >>> On Tue, Jul 06, 2021 at 12:33:33PM +0800, Jason Wang wrote: >>>> This patch adds new device facility to save and restore virtqueue >>>> state. The virtqueue state is split into two parts: >>>> >>>> - The available state: The state that is used for read the next >>>> available buffer. >>>> - The used state: The state that is used for making buffer used. >>>> >>>> Note that, there could be devices that is required to set and get the >>>> requests that are being processed by the device. I leave such API to >>>> be device specific. >>>> >>>> This facility could be used by both migration and device diagnostic. >>>> >>>> Signed-off-by: Jason Wang <jasowang@redhat.com> >>> Hi Jason! >>> I feel that for use-cases such as SRIOV, >>> the facility to save/restore vq should be part of a PF >>> that is there needs to be a way for one virtio device to >>> address the state of another one. >>> >> Hi! >> >> In my opinion we should go the other way around: To make features as >> orthogonal/independent as possible, and just make them work together >> if we have to. In this particular case, I think it should be easier to >> decide how to report status, its needs, etc for a VF, and then open >> the possibility for the PF to query or set them, reusing format, >> behavior, etc. as much as possible. >> >> I think that the most controversial point about doing it non-SR IOV >> way is the exposing of these features/fields to the guest using >> specific transport facilities, like PCI common config. However I think >> it should not be hard for the hypervisor to intercept them and even to >> expose them conditionally. Please correct me if this guessing was not >> right and you had other concerns. > > Possibly. I'd like to see some guidance on how this all will work > in practice then. Maybe make it all part of a non-normative section > for now. > I think that the feature itself is not very useful outside of > migration so we don't really gain much by adding it as is > without all the other missing pieces. > I would say let's see more of the whole picture before we commit. I agree here. I also can't see the whole picture for SRIOV case. I'll try to combine the admin control queue suggested in previous patch set to my proposal of PF managing the VF migration. Feature negotiation is part of virtio device-driver communication and not part of the migration software that should manage the migration process. For me, seems like queue state is something that should be internal and not be exposed to guest drivers that see this as a new feature. > > > >>> Thoughts? >>> >>>> --- >>>> content.tex | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 117 insertions(+) >>>> >>>> diff --git a/content.tex b/content.tex >>>> index 620c0e2..8877b6f 100644 >>>> --- a/content.tex >>>> +++ b/content.tex >>>> @@ -385,6 +385,116 @@ \section{Exporting Objects}\label{sec:Basic Facilities of a Virtio Device / Expo >>>> types. It is RECOMMENDED that devices generate version 4 >>>> UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}. >>>> >>>> +\section{Virtqueue State}\label{sec:Virtqueues / Virtqueue State} >>>> + >>>> +When VIRTIO_F_RING_STATE is negotiated, the driver can set and >>>> +get the device internal virtqueue state through the following >>>> +fields. The way to access those fields is transport specific. >>>> + >>>> +\subsection{\field{Available State} Field} >>>> + >>>> +The \field{Available State} field is two bytes for the driver to get >>>> +or set the state that is used by the virtqueue to read for the next >>>> +available buffer. >>>> + >>>> +When VIRTIO_F_RING_PACKED is not negotiated, it contains: >>>> + >>>> +\begin{lstlisting} >>>> +le16 { >>>> + last_avail_idx : 16; >>>> +} avail_state; >>>> +\end{lstlisting} >>>> + >>>> +The \field{last_avail_idx} field indicates where the device would read >>>> +for the next index from the virtqueue available ring(modulo the queue >>>> + size). This starts at the value set by the driver, and increases. >>>> + >>>> +When VIRTIO_F_RING_PACKED is negotiated, it contains: >>>> + >>>> +\begin{lstlisting} >>>> +le16 { >>>> + last_avail_idx : 15; >>>> + last_avail_wrap_counter : 1; >>>> +} avail_state; >>>> +\end{lstlisting} >>>> + >>>> +The \field{last_avail_idx} field indicates where the device would read for >>>> +the next descriptor head from the descriptor ring. This starts at the >>>> +value set by the driver and wraps around when reaching the end of the >>>> +ring. >>>> + >>>> +The \field{last_avail_wrap_counter} field indicates the last Driver Ring >>>> +Wrap Counter that is observed by the device. This starts at the value >>>> +set by the driver, and is flipped when reaching the end of the ring. >>>> + >>>> +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}. >>>> + >>>> +\subsection{\field{Used State} Field} >>>> + >>>> +The \field{Used State} field is two bytes for the driver to set and >>>> +get the state used by the virtqueue to make buffer used. >>>> + >>>> +When VIRTIO_F_RING_PACKED is not negotiated, the used state contains: >>>> + >>>> +\begin{lstlisting} >>>> +le16 { >>>> + used_idx : 16; >>>> +} used_state; >>>> +\end{lstlisting} >>>> + >>>> +The \field{used_idx} where the device would write the next used >>>> +descriptor head to the used ring (modulo the queue size). This starts >>>> +at the value set by the driver, and increases. It is easy to see this >>>> +is the initial value of the \field{idx} in the used ring. >>>> + >>>> +See also \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring} >>>> + >>>> +When VIRTIO_F_RING_PACKED is negotiated, the used state contains: >>>> + >>>> +\begin{lstlisting} >>>> +le16 { >>>> + used_idx : 15; >>>> + used_wrap_counter : 1; >>>> +} used_state; >>>> +\end{lstlisting} >>>> + >>>> +The \field{used_idx} indicates where the device would write the next used >>>> +descriptor head to the descriptor ring. This starts at the value set >>>> +by the driver, and warps around when reaching the end of the ring. >>>> + >>>> +\field{used_wrap_counter} is the Device Ring Wrap Counter. This starts >>>> +at the value set by the driver, and is flipped when reaching the end >>>> +of the ring. >>>> + >>>> +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}. >>> >>> Above only fully describes the vq state if descriptors >>> are used in order or at least all out of order descriptors are consumed >>> at time of save. >>> >> I think that the most straightforward solution would be to add >> something similar to VHOST_USER_GET_INFLIGHT_FD, but without the _FD >> part. >> >> Thanks! >> >>> Adding later option to devices such as net will need extra spec work. >>> >>> >>>> +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State} >>>> + >>>> +If VIRTIO_F_RING_STATE has been negotiated: >>>> +\begin{itemize} >>>> +\item A driver MUST NOT set the virtqueue state before setting the >>>> + FEATURE_OK status bit. >>>> +\item A driver MUST NOT set the virtqueue state after setting the >>>> + DRIVER_OK status bit. >>>> +\end{itemize} >>>> + >>>> +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State} >>>> + >>>> +If VIRTIO_F_RING_STATE has not been negotiated, a device MUST ingore >>>> +the read and write to the virtqueue state. >>>> + >>>> +If VIRTIO_F_RING_STATE has been negotiated: >>>> +\begin{itemize} >>>> +\item A device SHOULD ignore the write to the virtqueue state if the >>>> +FEATURE_OK status bit is not set. >>>> +\item A device SHOULD ignore the write to the virtqueue state if the >>>> +DRIVER_OK status bit is set. >>>> +\end{itemize} >>>> + >>>> +If VIRTIO_F_RING_STATE has been negotiated, a device MAY has its >>> >>> may have? >>> should also go into a normative section >>> >>>> +device-specific way for the driver to set and get extra virtqueue >>>> +states such as in flight requests. >>>> + >>>> \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation} >>>> >>>> We start with an overview of device initialization, then expand on the >>>> @@ -420,6 +530,9 @@ \section{Device Initialization}\label{sec:General Initialization And Device Oper >>>> device, optional per-bus setup, reading and possibly writing the >>>> device's virtio configuration space, and population of virtqueues. >>>> >>>> +\item\label{itm:General Initialization And Device Operation / Device >>>> + Initialization / Virtqueue State Setup} When VIRTIO_F_RING_STATE has been negotiated, perform virtqueue state setup, including the initialization of the per virtqueue available state, used state and the possible device specific virtqueue state. >>>> + >>>> \item\label{itm:General Initialization And Device Operation / Device Initialization / Set DRIVER-OK} Set the DRIVER_OK status bit. At this point the device is >>>> ``live''. >>>> \end{enumerate} >>>> @@ -6596,6 +6709,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits} >>>> transport specific. >>>> For more details about driver notifications over PCI see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}. >>>> >>>> + \item[VIRTIO_F_RING_STATE(40)] This feature indicates that the driver >>>> + can set and get the device internal virtqueue state. >>>> + See \ref{sec:Virtqueues / Virtqueue State}~\nameref{sec:Virtqueues / Virtqueue State}. >>>> + >>>> \end{description} >>>> >>>> \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits} >>>> -- >>>> 2.25.1 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility 2021-07-06 23:49 ` Max Gurtovoy @ 2021-07-07 2:50 ` Jason Wang 2021-07-07 12:03 ` Max Gurtovoy 0 siblings, 1 reply; 21+ messages in thread From: Jason Wang @ 2021-07-07 2:50 UTC (permalink / raw) To: Max Gurtovoy, Michael S. Tsirkin, Eugenio Perez Martin Cc: virtio-comment, Virtio-Dev, Stefan Hajnoczi, Cornelia Huck, Oren Duer, Shahaf Shuler, Parav Pandit, Bodong Wang, Alexander Mikheev, Halil Pasic 在 2021/7/7 上午7:49, Max Gurtovoy 写道: > > On 7/6/2021 10:08 PM, Michael S. Tsirkin wrote: >> On Tue, Jul 06, 2021 at 07:09:10PM +0200, Eugenio Perez Martin wrote: >>> On Tue, Jul 6, 2021 at 11:32 AM Michael S. Tsirkin <mst@redhat.com> >>> wrote: >>>> On Tue, Jul 06, 2021 at 12:33:33PM +0800, Jason Wang wrote: >>>>> This patch adds new device facility to save and restore virtqueue >>>>> state. The virtqueue state is split into two parts: >>>>> >>>>> - The available state: The state that is used for read the next >>>>> available buffer. >>>>> - The used state: The state that is used for making buffer used. >>>>> >>>>> Note that, there could be devices that is required to set and get the >>>>> requests that are being processed by the device. I leave such API to >>>>> be device specific. >>>>> >>>>> This facility could be used by both migration and device diagnostic. >>>>> >>>>> Signed-off-by: Jason Wang <jasowang@redhat.com> >>>> Hi Jason! >>>> I feel that for use-cases such as SRIOV, >>>> the facility to save/restore vq should be part of a PF >>>> that is there needs to be a way for one virtio device to >>>> address the state of another one. >>>> >>> Hi! >>> >>> In my opinion we should go the other way around: To make features as >>> orthogonal/independent as possible, and just make them work together >>> if we have to. In this particular case, I think it should be easier to >>> decide how to report status, its needs, etc for a VF, and then open >>> the possibility for the PF to query or set them, reusing format, >>> behavior, etc. as much as possible. >>> >>> I think that the most controversial point about doing it non-SR IOV >>> way is the exposing of these features/fields to the guest using >>> specific transport facilities, like PCI common config. However I think >>> it should not be hard for the hypervisor to intercept them and even to >>> expose them conditionally. Please correct me if this guessing was not >>> right and you had other concerns. >> >> Possibly. I'd like to see some guidance on how this all will work >> in practice then. Maybe make it all part of a non-normative section >> for now. >> I think that the feature itself is not very useful outside of >> migration so we don't really gain much by adding it as is >> without all the other missing pieces. >> I would say let's see more of the whole picture before we commit. > > I agree here. I also can't see the whole picture for SRIOV case. Again, it's not related to SR-IOV at all. It tries to introduce basic facility in the virtio level which can work for all types of virtio device. Transport such as PCI need to implement its own way to access those state. It's not hard to implement them simply via capability. It works like other basic facility like device status, features etc. For SR-IOV, it doesn't prevent you from implementing that via the admin virtqueue. > > I'll try to combine the admin control queue suggested in previous > patch set to my proposal of PF managing the VF migration. Note that, the admin virtqueue should be transport indepedent when trying to introduce them. > > Feature negotiation is part of virtio device-driver communication and > not part of the migration software that should manage the migration > process. > > For me, seems like queue state is something that should be internal > and not be exposed to guest drivers that see this as a new feature. This is not true, we have the case of nested virtualization. As mentioned in another thread, it's the hypervisor that need to choose between hiding or shadowing the internal virtqueue state. Thanks > >> >> >> >>>> Thoughts? >>>> >>>>> --- >>>>> content.tex | 117 >>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 117 insertions(+) >>>>> >>>>> diff --git a/content.tex b/content.tex >>>>> index 620c0e2..8877b6f 100644 >>>>> --- a/content.tex >>>>> +++ b/content.tex >>>>> @@ -385,6 +385,116 @@ \section{Exporting Objects}\label{sec:Basic >>>>> Facilities of a Virtio Device / Expo >>>>> types. It is RECOMMENDED that devices generate version 4 >>>>> UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}. >>>>> >>>>> +\section{Virtqueue State}\label{sec:Virtqueues / Virtqueue State} >>>>> + >>>>> +When VIRTIO_F_RING_STATE is negotiated, the driver can set and >>>>> +get the device internal virtqueue state through the following >>>>> +fields. The way to access those fields is transport specific. >>>>> + >>>>> +\subsection{\field{Available State} Field} >>>>> + >>>>> +The \field{Available State} field is two bytes for the driver to get >>>>> +or set the state that is used by the virtqueue to read for the next >>>>> +available buffer. >>>>> + >>>>> +When VIRTIO_F_RING_PACKED is not negotiated, it contains: >>>>> + >>>>> +\begin{lstlisting} >>>>> +le16 { >>>>> + last_avail_idx : 16; >>>>> +} avail_state; >>>>> +\end{lstlisting} >>>>> + >>>>> +The \field{last_avail_idx} field indicates where the device would >>>>> read >>>>> +for the next index from the virtqueue available ring(modulo the >>>>> queue >>>>> + size). This starts at the value set by the driver, and increases. >>>>> + >>>>> +When VIRTIO_F_RING_PACKED is negotiated, it contains: >>>>> + >>>>> +\begin{lstlisting} >>>>> +le16 { >>>>> + last_avail_idx : 15; >>>>> + last_avail_wrap_counter : 1; >>>>> +} avail_state; >>>>> +\end{lstlisting} >>>>> + >>>>> +The \field{last_avail_idx} field indicates where the device would >>>>> read for >>>>> +the next descriptor head from the descriptor ring. This starts at >>>>> the >>>>> +value set by the driver and wraps around when reaching the end of >>>>> the >>>>> +ring. >>>>> + >>>>> +The \field{last_avail_wrap_counter} field indicates the last >>>>> Driver Ring >>>>> +Wrap Counter that is observed by the device. This starts at the >>>>> value >>>>> +set by the driver, and is flipped when reaching the end of the ring. >>>>> + >>>>> +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap >>>>> Counters}. >>>>> + >>>>> +\subsection{\field{Used State} Field} >>>>> + >>>>> +The \field{Used State} field is two bytes for the driver to set and >>>>> +get the state used by the virtqueue to make buffer used. >>>>> + >>>>> +When VIRTIO_F_RING_PACKED is not negotiated, the used state >>>>> contains: >>>>> + >>>>> +\begin{lstlisting} >>>>> +le16 { >>>>> + used_idx : 16; >>>>> +} used_state; >>>>> +\end{lstlisting} >>>>> + >>>>> +The \field{used_idx} where the device would write the next used >>>>> +descriptor head to the used ring (modulo the queue size). This >>>>> starts >>>>> +at the value set by the driver, and increases. It is easy to see >>>>> this >>>>> +is the initial value of the \field{idx} in the used ring. >>>>> + >>>>> +See also \ref{sec:Basic Facilities of a Virtio Device / >>>>> Virtqueues / The Virtqueue Used Ring} >>>>> + >>>>> +When VIRTIO_F_RING_PACKED is negotiated, the used state contains: >>>>> + >>>>> +\begin{lstlisting} >>>>> +le16 { >>>>> + used_idx : 15; >>>>> + used_wrap_counter : 1; >>>>> +} used_state; >>>>> +\end{lstlisting} >>>>> + >>>>> +The \field{used_idx} indicates where the device would write the >>>>> next used >>>>> +descriptor head to the descriptor ring. This starts at the value set >>>>> +by the driver, and warps around when reaching the end of the ring. >>>>> + >>>>> +\field{used_wrap_counter} is the Device Ring Wrap Counter. This >>>>> starts >>>>> +at the value set by the driver, and is flipped when reaching the end >>>>> +of the ring. >>>>> + >>>>> +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap >>>>> Counters}. >>>> >>>> Above only fully describes the vq state if descriptors >>>> are used in order or at least all out of order descriptors are >>>> consumed >>>> at time of save. >>>> >>> I think that the most straightforward solution would be to add >>> something similar to VHOST_USER_GET_INFLIGHT_FD, but without the _FD >>> part. >>> >>> Thanks! >>> >>>> Adding later option to devices such as net will need extra spec work. >>>> >>>> >>>>> +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities >>>>> of a Virtio Device / Virtqueue State} >>>>> + >>>>> +If VIRTIO_F_RING_STATE has been negotiated: >>>>> +\begin{itemize} >>>>> +\item A driver MUST NOT set the virtqueue state before setting the >>>>> + FEATURE_OK status bit. >>>>> +\item A driver MUST NOT set the virtqueue state after setting the >>>>> + DRIVER_OK status bit. >>>>> +\end{itemize} >>>>> + >>>>> +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities >>>>> of a Virtio Device / Virtqueue State} >>>>> + >>>>> +If VIRTIO_F_RING_STATE has not been negotiated, a device MUST ingore >>>>> +the read and write to the virtqueue state. >>>>> + >>>>> +If VIRTIO_F_RING_STATE has been negotiated: >>>>> +\begin{itemize} >>>>> +\item A device SHOULD ignore the write to the virtqueue state if the >>>>> +FEATURE_OK status bit is not set. >>>>> +\item A device SHOULD ignore the write to the virtqueue state if the >>>>> +DRIVER_OK status bit is set. >>>>> +\end{itemize} >>>>> + >>>>> +If VIRTIO_F_RING_STATE has been negotiated, a device MAY has its >>>> >>>> may have? >>>> should also go into a normative section >>>> >>>>> +device-specific way for the driver to set and get extra virtqueue >>>>> +states such as in flight requests. >>>>> + >>>>> \chapter{General Initialization And Device >>>>> Operation}\label{sec:General Initialization And Device Operation} >>>>> >>>>> We start with an overview of device initialization, then expand >>>>> on the >>>>> @@ -420,6 +530,9 @@ \section{Device >>>>> Initialization}\label{sec:General Initialization And Device Oper >>>>> device, optional per-bus setup, reading and possibly writing the >>>>> device's virtio configuration space, and population of >>>>> virtqueues. >>>>> >>>>> +\item\label{itm:General Initialization And Device Operation / Device >>>>> + Initialization / Virtqueue State Setup} When >>>>> VIRTIO_F_RING_STATE has been negotiated, perform virtqueue state >>>>> setup, including the initialization of the per virtqueue available >>>>> state, used state and the possible device specific virtqueue state. >>>>> + >>>>> \item\label{itm:General Initialization And Device Operation / >>>>> Device Initialization / Set DRIVER-OK} Set the DRIVER_OK status >>>>> bit. At this point the device is >>>>> ``live''. >>>>> \end{enumerate} >>>>> @@ -6596,6 +6709,10 @@ \chapter{Reserved Feature >>>>> Bits}\label{sec:Reserved Feature Bits} >>>>> transport specific. >>>>> For more details about driver notifications over PCI see >>>>> \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / >>>>> PCI-specific Initialization And Device Operation / Available >>>>> Buffer Notifications}. >>>>> >>>>> + \item[VIRTIO_F_RING_STATE(40)] This feature indicates that the >>>>> driver >>>>> + can set and get the device internal virtqueue state. >>>>> + See \ref{sec:Virtqueues / Virtqueue >>>>> State}~\nameref{sec:Virtqueues / Virtqueue State}. >>>>> + >>>>> \end{description} >>>>> >>>>> \drivernormative{\section}{Reserved Feature Bits}{Reserved >>>>> Feature Bits} >>>>> -- >>>>> 2.25.1 > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility 2021-07-07 2:50 ` Jason Wang @ 2021-07-07 12:03 ` Max Gurtovoy 2021-07-07 12:11 ` [virtio-comment] " Jason Wang 0 siblings, 1 reply; 21+ messages in thread From: Max Gurtovoy @ 2021-07-07 12:03 UTC (permalink / raw) To: Jason Wang, Michael S. Tsirkin, Eugenio Perez Martin Cc: virtio-comment, Virtio-Dev, Stefan Hajnoczi, Cornelia Huck, Oren Duer, Shahaf Shuler, Parav Pandit, Bodong Wang, Alexander Mikheev, Halil Pasic On 7/7/2021 5:50 AM, Jason Wang wrote: > > 在 2021/7/7 上午7:49, Max Gurtovoy 写道: >> >> On 7/6/2021 10:08 PM, Michael S. Tsirkin wrote: >>> On Tue, Jul 06, 2021 at 07:09:10PM +0200, Eugenio Perez Martin wrote: >>>> On Tue, Jul 6, 2021 at 11:32 AM Michael S. Tsirkin <mst@redhat.com> >>>> wrote: >>>>> On Tue, Jul 06, 2021 at 12:33:33PM +0800, Jason Wang wrote: >>>>>> This patch adds new device facility to save and restore virtqueue >>>>>> state. The virtqueue state is split into two parts: >>>>>> >>>>>> - The available state: The state that is used for read the next >>>>>> available buffer. >>>>>> - The used state: The state that is used for making buffer used. >>>>>> >>>>>> Note that, there could be devices that is required to set and get >>>>>> the >>>>>> requests that are being processed by the device. I leave such API to >>>>>> be device specific. >>>>>> >>>>>> This facility could be used by both migration and device diagnostic. >>>>>> >>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com> >>>>> Hi Jason! >>>>> I feel that for use-cases such as SRIOV, >>>>> the facility to save/restore vq should be part of a PF >>>>> that is there needs to be a way for one virtio device to >>>>> address the state of another one. >>>>> >>>> Hi! >>>> >>>> In my opinion we should go the other way around: To make features as >>>> orthogonal/independent as possible, and just make them work together >>>> if we have to. In this particular case, I think it should be easier to >>>> decide how to report status, its needs, etc for a VF, and then open >>>> the possibility for the PF to query or set them, reusing format, >>>> behavior, etc. as much as possible. >>>> >>>> I think that the most controversial point about doing it non-SR IOV >>>> way is the exposing of these features/fields to the guest using >>>> specific transport facilities, like PCI common config. However I think >>>> it should not be hard for the hypervisor to intercept them and even to >>>> expose them conditionally. Please correct me if this guessing was not >>>> right and you had other concerns. >>> >>> Possibly. I'd like to see some guidance on how this all will work >>> in practice then. Maybe make it all part of a non-normative section >>> for now. >>> I think that the feature itself is not very useful outside of >>> migration so we don't really gain much by adding it as is >>> without all the other missing pieces. >>> I would say let's see more of the whole picture before we commit. >> >> I agree here. I also can't see the whole picture for SRIOV case. > > > Again, it's not related to SR-IOV at all. It tries to introduce basic > facility in the virtio level which can work for all types of virtio > device. > > Transport such as PCI need to implement its own way to access those > state. It's not hard to implement them simply via capability. > > It works like other basic facility like device status, features etc. > > For SR-IOV, it doesn't prevent you from implementing that via the > admin virtqueue. > > >> >> I'll try to combine the admin control queue suggested in previous >> patch set to my proposal of PF managing the VF migration. > > > Note that, the admin virtqueue should be transport indepedent when > trying to introduce them. > > >> >> Feature negotiation is part of virtio device-driver communication and >> not part of the migration software that should manage the migration >> process. >> >> For me, seems like queue state is something that should be internal >> and not be exposed to guest drivers that see this as a new feature. > > > This is not true, we have the case of nested virtualization. As > mentioned in another thread, it's the hypervisor that need to choose > between hiding or shadowing the internal virtqueue state. > > Thanks In the nested environment, do you mean the Level 1 is Real PF with X VFs and in Level 2 the X VF seen as PFs in the guests and expose another Y VFs ? If so, the guest PF will manage the migration for it's Y VFs. > > >> >>> >>> >>> >>>>> Thoughts? >>>>> >>>>>> --- >>>>>> content.tex | 117 >>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>> 1 file changed, 117 insertions(+) >>>>>> >>>>>> diff --git a/content.tex b/content.tex >>>>>> index 620c0e2..8877b6f 100644 >>>>>> --- a/content.tex >>>>>> +++ b/content.tex >>>>>> @@ -385,6 +385,116 @@ \section{Exporting Objects}\label{sec:Basic >>>>>> Facilities of a Virtio Device / Expo >>>>>> types. It is RECOMMENDED that devices generate version 4 >>>>>> UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}. >>>>>> >>>>>> +\section{Virtqueue State}\label{sec:Virtqueues / Virtqueue State} >>>>>> + >>>>>> +When VIRTIO_F_RING_STATE is negotiated, the driver can set and >>>>>> +get the device internal virtqueue state through the following >>>>>> +fields. The way to access those fields is transport specific. >>>>>> + >>>>>> +\subsection{\field{Available State} Field} >>>>>> + >>>>>> +The \field{Available State} field is two bytes for the driver to >>>>>> get >>>>>> +or set the state that is used by the virtqueue to read for the next >>>>>> +available buffer. >>>>>> + >>>>>> +When VIRTIO_F_RING_PACKED is not negotiated, it contains: >>>>>> + >>>>>> +\begin{lstlisting} >>>>>> +le16 { >>>>>> + last_avail_idx : 16; >>>>>> +} avail_state; >>>>>> +\end{lstlisting} >>>>>> + >>>>>> +The \field{last_avail_idx} field indicates where the device >>>>>> would read >>>>>> +for the next index from the virtqueue available ring(modulo the >>>>>> queue >>>>>> + size). This starts at the value set by the driver, and increases. >>>>>> + >>>>>> +When VIRTIO_F_RING_PACKED is negotiated, it contains: >>>>>> + >>>>>> +\begin{lstlisting} >>>>>> +le16 { >>>>>> + last_avail_idx : 15; >>>>>> + last_avail_wrap_counter : 1; >>>>>> +} avail_state; >>>>>> +\end{lstlisting} >>>>>> + >>>>>> +The \field{last_avail_idx} field indicates where the device >>>>>> would read for >>>>>> +the next descriptor head from the descriptor ring. This starts >>>>>> at the >>>>>> +value set by the driver and wraps around when reaching the end >>>>>> of the >>>>>> +ring. >>>>>> + >>>>>> +The \field{last_avail_wrap_counter} field indicates the last >>>>>> Driver Ring >>>>>> +Wrap Counter that is observed by the device. This starts at the >>>>>> value >>>>>> +set by the driver, and is flipped when reaching the end of the >>>>>> ring. >>>>>> + >>>>>> +See also \ref{sec:Packed Virtqueues / Driver and Device Ring >>>>>> Wrap Counters}. >>>>>> + >>>>>> +\subsection{\field{Used State} Field} >>>>>> + >>>>>> +The \field{Used State} field is two bytes for the driver to set and >>>>>> +get the state used by the virtqueue to make buffer used. >>>>>> + >>>>>> +When VIRTIO_F_RING_PACKED is not negotiated, the used state >>>>>> contains: >>>>>> + >>>>>> +\begin{lstlisting} >>>>>> +le16 { >>>>>> + used_idx : 16; >>>>>> +} used_state; >>>>>> +\end{lstlisting} >>>>>> + >>>>>> +The \field{used_idx} where the device would write the next used >>>>>> +descriptor head to the used ring (modulo the queue size). This >>>>>> starts >>>>>> +at the value set by the driver, and increases. It is easy to see >>>>>> this >>>>>> +is the initial value of the \field{idx} in the used ring. >>>>>> + >>>>>> +See also \ref{sec:Basic Facilities of a Virtio Device / >>>>>> Virtqueues / The Virtqueue Used Ring} >>>>>> + >>>>>> +When VIRTIO_F_RING_PACKED is negotiated, the used state contains: >>>>>> + >>>>>> +\begin{lstlisting} >>>>>> +le16 { >>>>>> + used_idx : 15; >>>>>> + used_wrap_counter : 1; >>>>>> +} used_state; >>>>>> +\end{lstlisting} >>>>>> + >>>>>> +The \field{used_idx} indicates where the device would write the >>>>>> next used >>>>>> +descriptor head to the descriptor ring. This starts at the value >>>>>> set >>>>>> +by the driver, and warps around when reaching the end of the ring. >>>>>> + >>>>>> +\field{used_wrap_counter} is the Device Ring Wrap Counter. This >>>>>> starts >>>>>> +at the value set by the driver, and is flipped when reaching the >>>>>> end >>>>>> +of the ring. >>>>>> + >>>>>> +See also \ref{sec:Packed Virtqueues / Driver and Device Ring >>>>>> Wrap Counters}. >>>>> >>>>> Above only fully describes the vq state if descriptors >>>>> are used in order or at least all out of order descriptors are >>>>> consumed >>>>> at time of save. >>>>> >>>> I think that the most straightforward solution would be to add >>>> something similar to VHOST_USER_GET_INFLIGHT_FD, but without the _FD >>>> part. >>>> >>>> Thanks! >>>> >>>>> Adding later option to devices such as net will need extra spec work. >>>>> >>>>> >>>>>> +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities >>>>>> of a Virtio Device / Virtqueue State} >>>>>> + >>>>>> +If VIRTIO_F_RING_STATE has been negotiated: >>>>>> +\begin{itemize} >>>>>> +\item A driver MUST NOT set the virtqueue state before setting the >>>>>> + FEATURE_OK status bit. >>>>>> +\item A driver MUST NOT set the virtqueue state after setting the >>>>>> + DRIVER_OK status bit. >>>>>> +\end{itemize} >>>>>> + >>>>>> +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities >>>>>> of a Virtio Device / Virtqueue State} >>>>>> + >>>>>> +If VIRTIO_F_RING_STATE has not been negotiated, a device MUST >>>>>> ingore >>>>>> +the read and write to the virtqueue state. >>>>>> + >>>>>> +If VIRTIO_F_RING_STATE has been negotiated: >>>>>> +\begin{itemize} >>>>>> +\item A device SHOULD ignore the write to the virtqueue state if >>>>>> the >>>>>> +FEATURE_OK status bit is not set. >>>>>> +\item A device SHOULD ignore the write to the virtqueue state if >>>>>> the >>>>>> +DRIVER_OK status bit is set. >>>>>> +\end{itemize} >>>>>> + >>>>>> +If VIRTIO_F_RING_STATE has been negotiated, a device MAY has its >>>>> >>>>> may have? >>>>> should also go into a normative section >>>>> >>>>>> +device-specific way for the driver to set and get extra virtqueue >>>>>> +states such as in flight requests. >>>>>> + >>>>>> \chapter{General Initialization And Device >>>>>> Operation}\label{sec:General Initialization And Device Operation} >>>>>> >>>>>> We start with an overview of device initialization, then expand >>>>>> on the >>>>>> @@ -420,6 +530,9 @@ \section{Device >>>>>> Initialization}\label{sec:General Initialization And Device Oper >>>>>> device, optional per-bus setup, reading and possibly writing >>>>>> the >>>>>> device's virtio configuration space, and population of >>>>>> virtqueues. >>>>>> >>>>>> +\item\label{itm:General Initialization And Device Operation / >>>>>> Device >>>>>> + Initialization / Virtqueue State Setup} When >>>>>> VIRTIO_F_RING_STATE has been negotiated, perform virtqueue state >>>>>> setup, including the initialization of the per virtqueue >>>>>> available state, used state and the possible device specific >>>>>> virtqueue state. >>>>>> + >>>>>> \item\label{itm:General Initialization And Device Operation / >>>>>> Device Initialization / Set DRIVER-OK} Set the DRIVER_OK status >>>>>> bit. At this point the device is >>>>>> ``live''. >>>>>> \end{enumerate} >>>>>> @@ -6596,6 +6709,10 @@ \chapter{Reserved Feature >>>>>> Bits}\label{sec:Reserved Feature Bits} >>>>>> transport specific. >>>>>> For more details about driver notifications over PCI see >>>>>> \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / >>>>>> PCI-specific Initialization And Device Operation / Available >>>>>> Buffer Notifications}. >>>>>> >>>>>> + \item[VIRTIO_F_RING_STATE(40)] This feature indicates that the >>>>>> driver >>>>>> + can set and get the device internal virtqueue state. >>>>>> + See \ref{sec:Virtqueues / Virtqueue >>>>>> State}~\nameref{sec:Virtqueues / Virtqueue State}. >>>>>> + >>>>>> \end{description} >>>>>> >>>>>> \drivernormative{\section}{Reserved Feature Bits}{Reserved >>>>>> Feature Bits} >>>>>> -- >>>>>> 2.25.1 >> > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [virtio-comment] Re: [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility 2021-07-07 12:03 ` Max Gurtovoy @ 2021-07-07 12:11 ` Jason Wang 0 siblings, 0 replies; 21+ messages in thread From: Jason Wang @ 2021-07-07 12:11 UTC (permalink / raw) To: virtio-comment 在 2021/7/7 下午8:03, Max Gurtovoy 写道: > > On 7/7/2021 5:50 AM, Jason Wang wrote: >> >> 在 2021/7/7 上午7:49, Max Gurtovoy 写道: >>> >>> On 7/6/2021 10:08 PM, Michael S. Tsirkin wrote: >>>> On Tue, Jul 06, 2021 at 07:09:10PM +0200, Eugenio Perez Martin wrote: >>>>> On Tue, Jul 6, 2021 at 11:32 AM Michael S. Tsirkin >>>>> <mst@redhat.com> wrote: >>>>>> On Tue, Jul 06, 2021 at 12:33:33PM +0800, Jason Wang wrote: >>>>>>> This patch adds new device facility to save and restore virtqueue >>>>>>> state. The virtqueue state is split into two parts: >>>>>>> >>>>>>> - The available state: The state that is used for read the next >>>>>>> available buffer. >>>>>>> - The used state: The state that is used for making buffer used. >>>>>>> >>>>>>> Note that, there could be devices that is required to set and >>>>>>> get the >>>>>>> requests that are being processed by the device. I leave such >>>>>>> API to >>>>>>> be device specific. >>>>>>> >>>>>>> This facility could be used by both migration and device >>>>>>> diagnostic. >>>>>>> >>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com> >>>>>> Hi Jason! >>>>>> I feel that for use-cases such as SRIOV, >>>>>> the facility to save/restore vq should be part of a PF >>>>>> that is there needs to be a way for one virtio device to >>>>>> address the state of another one. >>>>>> >>>>> Hi! >>>>> >>>>> In my opinion we should go the other way around: To make features as >>>>> orthogonal/independent as possible, and just make them work together >>>>> if we have to. In this particular case, I think it should be >>>>> easier to >>>>> decide how to report status, its needs, etc for a VF, and then open >>>>> the possibility for the PF to query or set them, reusing format, >>>>> behavior, etc. as much as possible. >>>>> >>>>> I think that the most controversial point about doing it non-SR IOV >>>>> way is the exposing of these features/fields to the guest using >>>>> specific transport facilities, like PCI common config. However I >>>>> think >>>>> it should not be hard for the hypervisor to intercept them and >>>>> even to >>>>> expose them conditionally. Please correct me if this guessing was not >>>>> right and you had other concerns. >>>> >>>> Possibly. I'd like to see some guidance on how this all will work >>>> in practice then. Maybe make it all part of a non-normative section >>>> for now. >>>> I think that the feature itself is not very useful outside of >>>> migration so we don't really gain much by adding it as is >>>> without all the other missing pieces. >>>> I would say let's see more of the whole picture before we commit. >>> >>> I agree here. I also can't see the whole picture for SRIOV case. >> >> >> Again, it's not related to SR-IOV at all. It tries to introduce basic >> facility in the virtio level which can work for all types of virtio >> device. >> >> Transport such as PCI need to implement its own way to access those >> state. It's not hard to implement them simply via capability. >> >> It works like other basic facility like device status, features etc. >> >> For SR-IOV, it doesn't prevent you from implementing that via the >> admin virtqueue. >> >> >>> >>> I'll try to combine the admin control queue suggested in previous >>> patch set to my proposal of PF managing the VF migration. >> >> >> Note that, the admin virtqueue should be transport indepedent when >> trying to introduce them. >> >> >>> >>> Feature negotiation is part of virtio device-driver communication >>> and not part of the migration software that should manage the >>> migration process. >>> >>> For me, seems like queue state is something that should be internal >>> and not be exposed to guest drivers that see this as a new feature. >> >> >> This is not true, we have the case of nested virtualization. As >> mentioned in another thread, it's the hypervisor that need to choose >> between hiding or shadowing the internal virtqueue state. >> >> Thanks > > In the nested environment, do you mean the Level 1 is Real PF with X > VFs and in Level 2 the X VF seen as PFs in the guests and expose > another Y VFs ? I meant PF is managed in L0. And the VF is assigned to L2 guest. In this case, we can expose the virtqueue state feature to L1 guest for migration L2 guest. > > If so, the guest PF will manage the migration for it's Y VFs. Does this mean you want to pass PF to L1 guest? Thanks This publicly archived list offers a means to provide input to the OASIS Virtual I/O Device (VIRTIO) TC. In order to verify user consent to the Feedback License terms and to minimize spam in the list archive, subscription is required before posting. Subscribe: virtio-comment-subscribe@lists.oasis-open.org Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org List help: virtio-comment-help@lists.oasis-open.org List archive: https://lists.oasis-open.org/archives/virtio-comment/ Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists Committee: https://www.oasis-open.org/committees/virtio/ Join OASIS: https://www.oasis-open.org/join/ ^ permalink raw reply [flat|nested] 21+ messages in thread
* [virtio-comment] Re: [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility 2021-07-06 4:33 ` [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility Jason Wang 2021-07-06 9:32 ` Michael S. Tsirkin @ 2021-07-06 12:27 ` Cornelia Huck 1 sibling, 0 replies; 21+ messages in thread From: Cornelia Huck @ 2021-07-06 12:27 UTC (permalink / raw) To: Jason Wang, mst, jasowang, virtio-comment, virtio-dev Cc: stefanha, mgurtovoy, eperezma, oren, shahafs, parav, bodong, amikheev, pasic On Tue, Jul 06 2021, Jason Wang <jasowang@redhat.com> wrote: > This patch adds new device facility to save and restore virtqueue > state. The virtqueue state is split into two parts: > > - The available state: The state that is used for read the next > available buffer. > - The used state: The state that is used for making buffer used. > > Note that, there could be devices that is required to set and get the > requests that are being processed by the device. I leave such API to > be device specific. > > This facility could be used by both migration and device diagnostic. > > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > content.tex | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 117 insertions(+) > +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State} > + > +If VIRTIO_F_RING_STATE has not been negotiated, a device MUST ingore > +the read and write to the virtqueue state. > + > +If VIRTIO_F_RING_STATE has been negotiated: > +\begin{itemize} > +\item A device SHOULD ignore the write to the virtqueue state if the > +FEATURE_OK status bit is not set. > +\item A device SHOULD ignore the write to the virtqueue state if the > +DRIVER_OK status bit is set. > +\end{itemize} > + > +If VIRTIO_F_RING_STATE has been negotiated, a device MAY has its > +device-specific way for the driver to set and get extra virtqueue > +states such as in flight requests. Maybe better "If VIRTIO_F_RING_STATE has been negotiated, a device MAY provide a device-specific mechanism to set and get extra virtqueue states such as in flight reqeuests." If a device type supports this facility, does it imply that it is always present when VIRTIO_RING_STATE has been negotiated? I guess it could define further device-specific features to make it more configurable. This publicly archived list offers a means to provide input to the OASIS Virtual I/O Device (VIRTIO) TC. In order to verify user consent to the Feedback License terms and to minimize spam in the list archive, subscription is required before posting. Subscribe: virtio-comment-subscribe@lists.oasis-open.org Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org List help: virtio-comment-help@lists.oasis-open.org List archive: https://lists.oasis-open.org/archives/virtio-comment/ Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists Committee: https://www.oasis-open.org/committees/virtio/ Join OASIS: https://www.oasis-open.org/join/ ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2021-07-07 12:11 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-22 3:47 [virtio-comment] [PATCH V2 0/2] Introduce VIRTIO_F_QUEUE_STATE Jason Wang 2021-03-22 3:47 ` [virtio-comment] [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility Jason Wang 2021-03-22 8:19 ` [virtio-comment] " Eugenio Perez Martin 2021-03-23 2:54 ` Jason Wang 2021-03-23 9:27 ` Eugenio Perez Martin 2021-03-23 10:27 ` [virtio-comment] " Stefan Hajnoczi 2021-03-24 8:15 ` Jason Wang 2021-03-24 9:35 ` Stefan Hajnoczi 2021-03-25 2:38 ` Jason Wang 2021-03-24 9:38 ` Stefan Hajnoczi 2021-03-25 2:42 ` Jason Wang 2021-03-25 9:59 ` Stefan Hajnoczi 2021-03-22 3:47 ` [virtio-comment] [PATCH V2 2/2] virtio-pci: implement VIRTIO_F_QUEUE_STATE Jason Wang 2021-03-23 10:02 ` Stefan Hajnoczi 2021-03-23 10:40 ` [virtio-comment] [PATCH V2 0/2] Introduce VIRTIO_F_QUEUE_STATE Stefan Hajnoczi 2021-03-24 7:05 ` Jason Wang 2021-03-24 10:05 ` Stefan Hajnoczi 2021-03-25 2:57 ` Jason Wang 2021-03-25 10:03 ` [virtio-comment] [PATCH V2 0/2] Introduce VIRTIO_F_QUEUE_STATE\\ Stefan Hajnoczi 2021-07-06 4:33 [PATCH V2 0/2] Vitqueue State Synchronization Jason Wang 2021-07-06 4:33 ` [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility Jason Wang 2021-07-06 9:32 ` Michael S. Tsirkin 2021-07-06 17:09 ` Eugenio Perez Martin 2021-07-06 19:08 ` Michael S. Tsirkin 2021-07-06 23:49 ` Max Gurtovoy 2021-07-07 2:50 ` Jason Wang 2021-07-07 12:03 ` Max Gurtovoy 2021-07-07 12:11 ` [virtio-comment] " Jason Wang 2021-07-06 12:27 ` Cornelia Huck
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.