Re: [PATCH] virtio-pci: introduce VIRITO_F_QUEUE_STATE

[Eugenio Perez Martin <eperezma@redhat.com>]

On Tue, Mar 16, 2021 at 7:08 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> å 2021/3/15 äå8:24, Eugenio Perez Martin åé:
> > On Mon, Mar 15, 2021 at 3:59 AM Jason Wang <jasowang@redhat.com> wrote:
> >> This patch adds the ability to save and restore virtqueue state via a
> >> new field in the common configuration infrastructure.
> >>
> >> To simply the implementation, no new device status is introduced. For
> >> device, the requirements is not to forget the queue state after
> >> virtio reset and clear the virtqueue state upon ACKNOWLEDGE. For
> >> driver, it must set the virtqueue state before setting DRIVER_OK.
> >>
> >> To save a virtqueue state, the driver then need:
> >>
> >> 1) reset device
> >> 2) read virtqueue statue
> >>
> >> To restore a virtqueue state, the driver need:
> >>
> >> 1) reset device
> >> 2) perform necessary setups (e.g features negotiation)
> >> 3) write virtqueue state
> >> 4) set DRIVER_OK
> >>
> >> The main user should be live migration.
> >>
> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >> ---
> >>   content.tex                 | 38 +++++++++++++++++++++++++++++++++++++
> >>   virtqueue-state-packed-le.c |  7 +++++++
> >>   virtqueue-state-split-le.c  |  4 ++++
> >>   3 files changed, 49 insertions(+)
> >>   create mode 100644 virtqueue-state-packed-le.c
> >>   create mode 100644 virtqueue-state-split-le.c
> >>
> >> diff --git a/content.tex b/content.tex
> >> index 620c0e2..d7bff25 100644
> >> --- a/content.tex
> >> +++ b/content.tex
> >> @@ -837,6 +837,7 @@ \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 */
> > In my opinion it would be more clear to add an explicit le16 reserved[3] here.
>
>
> Good point, but then we need to document those revserved dependency with
> this feature?
>

Maybe I should have proposed naming that field "padding", but with
"reserved" I tried to signal that it can have future uses.

> Another solution is not using an "opaque" u64 here. Instead we could use:
>
> le16 last_avail_idx;
> le16 {
>      last_avail_idx : 15;
>      last_avail_wrap_counter : 1
> };
> le16 {
>      used_idx : 15;
>      used_wrap_counter : 1;
> }
>
> Is this better?
>

I find this even better, actually, yes.

Thanks!

> Thanks
>
>
> >
> >> +        le64 queue_state;               /* read-write */
> >>   };
> >>   \end{lstlisting}
> >>
> >> @@ -916,6 +917,29 @@ \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_state}]
> >> +        This field exists only if VIRTIO_F_QUEUE_STATE has been
> >> +        negotiated. The driver will use this field to get or set the
> >> +        virtqueue state by reading or writing the 64bit from the
> >> +        field.
> >> +        When VIRTIO_F_RING_PACKED has not been negotiated, the driver
> >> +        can set and get the following states:
> >> +        \lstinputlisting{virtqueue-state-split-le.c}
> >> +        The field \field{last_avail_idx} is the location where the
> >> +        device read for next index from the available ring.
> >> +        When VIRTIO_F_RING_PACKED has been negotiated, the driver can
> >> +        set and get the following states:
> >> +        \lstinputlisting{virtqueue-state-packed-le.c}
> >> +        The field \field{last_avail_idx} is the next location where
> >> +        device read for the next descriptor from the descriptor
> >> +        ring. The field \field{last_avail_wrap_counter} is the last
> >> +        driver ring wrap counter that is observed by the device. The
> >> +        field \field{used_idx} is the next location where device write
> >> +        used descriptor do descriptor ring. The field
> >> +        \field{used_wrap_counter} is the wrap counter that is used by
> >> +        the device. See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}.
> >> +
> >>   \end{description}
> >>
> >>   \devicenormative{\paragraph}{Common configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Common configuration structure layout}
> >> @@ -964,6 +988,10 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
> >>   present either a value of 0 or a power of 2 in
> >>   \field{queue_size}.
> >>
> >> +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
> >> +ACKNOWLEDGE has been set through \field{device status} bit.
> >> +
> >>   \drivernormative{\paragraph}{Common configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Common configuration structure layout}
> >>
> >>   The driver MUST NOT write to \field{device_feature}, \field{num_queues}, \field{config_generation}, \field{queue_notify_off} or \field{queue_notify_data}.
> >> @@ -981,6 +1009,13 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
> >>
> >>   The driver MUST NOT write a 0 to \field{queue_enable}.
> >>
> >> +If VIRTIO_F_QUEUE_STATE has been negotiated, a driver SHOULD set the
> >> +state of each virtqueue through \field{queue_state} before setting the
> >> +DRIVER_OK \field{device status} bit and SHOULD NOT write to
> >> +\field{queue_state} after setting the DRIVER_OK \field{device status}
> >> +bit. If a driver want to get the virtqueue state, it MUST first reset
> >> +the device then read state from \field{queue_state}.
> >> +
> >>   \subsubsection{Notification structure layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Notification capability}
> >>
> >>   The notification location is found using the VIRTIO_PCI_CAP_NOTIFY_CFG
> >> @@ -6596,6 +6631,9 @@ \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.
> >> +
> >>   \end{description}
> >>
> >>   \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
> >> diff --git a/virtqueue-state-packed-le.c b/virtqueue-state-packed-le.c
> >> new file mode 100644
> >> index 0000000..f21f9c2
> >> --- /dev/null
> >> +++ b/virtqueue-state-packed-le.c
> >> @@ -0,0 +1,7 @@
> >> +le64 {
> >> +       last_avail_idx : 15;
> >> +       last_avail_wrap_counter : 1;
> >> +       used_idx : 15;
> >> +       used_wrap_counter : 1;
> >> +       reserved : 32;
> >> +};
> >> diff --git a/virtqueue-state-split-le.c b/virtqueue-state-split-le.c
> >> new file mode 100644
> >> index 0000000..daeb4a3
> >> --- /dev/null
> >> +++ b/virtqueue-state-split-le.c
> >> @@ -0,0 +1,4 @@
> >> +le64 {
> >> +       last_avail_idx : 16;
> >> +       reserved: 48;
> >> +};
> >> --
> >> 2.24.3 (Apple Git-128)
> >>
>