Re: [virtio-comment] Re: [PATCH] virtio-pci: introduce VIRITO_F_QUEUE_STATE

[Cornelia Huck <cohuck@redhat.com>]

On Tue, 16 Mar 2021 10:53:37 +0800
Jason Wang <jasowang@redhat.com> wrote:

> å 2021/3/15 äå11:24, Cornelia Huck åé:
> > On Mon, 15 Mar 2021 10:58:46 +0800
> > 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
> >>

> >> @@ -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.  
> > What happens if a driver tries to read the queue status outside of this
> > window? Should it get zeroes? Unpredictable values?  
> 
> 
> I'm not sure having normative like this can help. Can we leave it to 
> device? I had a driver normative to clarify when should driver read or 
> write to the value.

Ok, a normative statement is probably not that useful, if a driver
operating outside of the spec gets to keep the pieces when it breaks.

> 
> 
> >  
> >> +
> >>   \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}.  
> > What should the driver do with a 'fresh' device? Does it need to start
> > out with a reset, read the (zero) state, and then write it back?  
> 
> 
> If 'fresh' means a normal probe procedure, in this case we don't need to 
> get the virtqueue state. What we need is to set a proper state. For 
> split virtqueue, the driver should write 0 (as last_avail_idx). For 
> packed virtqueue, the driver shoudl write:
> 
> {.last_avail_idx = 0, .last_avail_wrap_counter=1, .used_idx=0, 
> used_wrap_counter=1}.

Yes, that was what I had been thinking of.

So, I think the driver won't get a useful state if it reads the state
after reset for a previously unused device (the device statement only
says that it MUST NOT clear the state after reset)? Do we need to add a
sentence that a driver needs to do that initial setup of the state for
a device it has not used previously?

> 
> If 'fresh' means start device after migration, we need to set the 
> virtqueue state to what source gives us:
> 
> in src:
> 
> 1) reset device
> 2) read virtqueue states
> 3) pass virtqueue states to dst
> 
> in dst:
> 
> 1) receivce virtqueue states from src
> 2) reset device
> 3) perform necesssary setup ( feature neogitaion etc.)
> 4) set the virtqueue states we received from src.
> 
> Btw, it looks to me we need to clearify that "The driver MUST write to 
> queue_state after FEATURE_OK but before DRIVER_OK)

Yes, I agree.