On Mon, 17 Apr 2023 at 15:08, Eugenio Perez Martin <epere...@redhat.com> wrote:
>
> On Mon, Apr 17, 2023 at 7:14 PM Stefan Hajnoczi <stefa...@redhat.com> wrote:
> >
> > On Thu, Apr 13, 2023 at 12:14:24PM +0200, Eugenio Perez Martin wrote:
> > > On Wed, Apr 12, 2023 at 11:06 PM Stefan Hajnoczi <stefa...@redhat.com>
> > > wrote:
> > > >
> > > > On Tue, Apr 11, 2023 at 05:05:13PM +0200, Hanna Czenczek wrote:
> > > > > So-called "internal" virtio-fs migration refers to transporting the
> > > > > back-end's (virtiofsd's) state through qemu's migration stream. To do
> > > > > this, we need to be able to transfer virtiofsd's internal state to and
> > > > > from virtiofsd.
> > > > >
> > > > > Because virtiofsd's internal state will not be too large, we believe
> > > > > it
> > > > > is best to transfer it as a single binary blob after the streaming
> > > > > phase. Because this method should be useful to other vhost-user
> > > > > implementations, too, it is introduced as a general-purpose addition
> > > > > to
> > > > > the protocol, not limited to vhost-user-fs.
> > > > >
> > > > > These are the additions to the protocol:
> > > > > - New vhost-user protocol feature
> > > > > VHOST_USER_PROTOCOL_F_MIGRATORY_STATE:
> > > > > This feature signals support for transferring state, and is added so
> > > > > that migration can fail early when the back-end has no support.
> > > > >
> > > > > - SET_DEVICE_STATE_FD function: Front-end and back-end negotiate a
> > > > > pipe
> > > > > over which to transfer the state. The front-end sends an FD to the
> > > > > back-end into/from which it can write/read its state, and the
> > > > > back-end
> > > > > can decide to either use it, or reply with a different FD for the
> > > > > front-end to override the front-end's choice.
> > > > > The front-end creates a simple pipe to transfer the state, but maybe
> > > > > the back-end already has an FD into/from which it has to write/read
> > > > > its state, in which case it will want to override the simple pipe.
> > > > > Conversely, maybe in the future we find a way to have the front-end
> > > > > get an immediate FD for the migration stream (in some cases), in
> > > > > which
> > > > > case we will want to send this to the back-end instead of creating a
> > > > > pipe.
> > > > > Hence the negotiation: If one side has a better idea than a plain
> > > > > pipe, we will want to use that.
> > > > >
> > > > > - CHECK_DEVICE_STATE: After the state has been transferred through the
> > > > > pipe (the end indicated by EOF), the front-end invokes this function
> > > > > to verify success. There is no in-band way (through the pipe) to
> > > > > indicate failure, so we need to check explicitly.
> > > > >
> > > > > Once the transfer pipe has been established via SET_DEVICE_STATE_FD
> > > > > (which includes establishing the direction of transfer and migration
> > > > > phase), the sending side writes its data into the pipe, and the
> > > > > reading
> > > > > side reads it until it sees an EOF. Then, the front-end will check
> > > > > for
> > > > > success via CHECK_DEVICE_STATE, which on the destination side includes
> > > > > checking for integrity (i.e. errors during deserialization).
> > > > >
> > > > > Suggested-by: Stefan Hajnoczi <stefa...@redhat.com>
> > > > > Signed-off-by: Hanna Czenczek <hre...@redhat.com>
> > > > > ---
> > > > > include/hw/virtio/vhost-backend.h | 24 +++++
> > > > > include/hw/virtio/vhost.h | 79 ++++++++++++++++
> > > > > hw/virtio/vhost-user.c | 147
> > > > > ++++++++++++++++++++++++++++++
> > > > > hw/virtio/vhost.c | 37 ++++++++
> > > > > 4 files changed, 287 insertions(+)
> > > > >
> > > > > diff --git a/include/hw/virtio/vhost-backend.h
> > > > > b/include/hw/virtio/vhost-backend.h
> > > > > index ec3fbae58d..5935b32fe3 100644
> > > > > --- a/include/hw/virtio/vhost-backend.h
> > > > > +++ b/include/hw/virtio/vhost-backend.h
> > > > > @@ -26,6 +26,18 @@ typedef enum VhostSetConfigType {
> > > > > VHOST_SET_CONFIG_TYPE_MIGRATION = 1,
> > > > > } VhostSetConfigType;
> > > > >
> > > > > +typedef enum VhostDeviceStateDirection {
> > > > > + /* Transfer state from back-end (device) to front-end */
> > > > > + VHOST_TRANSFER_STATE_DIRECTION_SAVE = 0,
> > > > > + /* Transfer state from front-end to back-end (device) */
> > > > > + VHOST_TRANSFER_STATE_DIRECTION_LOAD = 1,
> > > > > +} VhostDeviceStateDirection;
> > > > > +
> > > > > +typedef enum VhostDeviceStatePhase {
> > > > > + /* The device (and all its vrings) is stopped */
> > > > > + VHOST_TRANSFER_STATE_PHASE_STOPPED = 0,
> > > > > +} VhostDeviceStatePhase;
> > > >
> > > > vDPA has:
> > > >
> > > > /* Suspend a device so it does not process virtqueue requests anymore
> > > > *
> > > > * After the return of ioctl the device must preserve all the
> > > > necessary state
> > > > * (the virtqueue vring base plus the possible device specific
> > > > states) that is
> > > > * required for restoring in the future. The device must not change
> > > > its
> > > > * configuration after that point.
> > > > */
> > > > #define VHOST_VDPA_SUSPEND _IO(VHOST_VIRTIO, 0x7D)
> > > >
> > > > /* Resume a device so it can resume processing virtqueue requests
> > > > *
> > > > * After the return of this ioctl the device will have restored all
> > > > the
> > > > * necessary states and it is fully operational to continue
> > > > processing the
> > > > * virtqueue descriptors.
> > > > */
> > > > #define VHOST_VDPA_RESUME _IO(VHOST_VIRTIO, 0x7E)
> > > >
> > > > I wonder if it makes sense to import these into vhost-user so that the
> > > > difference between kernel vhost and vhost-user is minimized. It's okay
> > > > if one of them is ahead of the other, but it would be nice to avoid
> > > > overlapping/duplicated functionality.
> > > >
> > >
> > > That's what I had in mind in the first versions. I proposed VHOST_STOP
> > > instead of VHOST_VDPA_STOP for this very reason. Later it did change
> > > to SUSPEND.
> > >
> > > Generally it is better if we make the interface less parametrized and
> > > we trust in the messages and its semantics in my opinion. In other
> > > words, instead of
> > > vhost_set_device_state_fd_op(VHOST_TRANSFER_STATE_PHASE_STOPPED), send
> > > individually the equivalent of VHOST_VDPA_SUSPEND vhost-user command.
> > >
> > > Another way to apply this is with the "direction" parameter. Maybe it
> > > is better to split it into "set_state_fd" and "get_state_fd"?
> > >
> > > In that case, reusing the ioctls as vhost-user messages would be ok.
> > > But that puts this proposal further from the VFIO code, which uses
> > > "migration_set_state(state)", and maybe it is better when the number
> > > of states is high.
> >
> > Hi Eugenio,
> > Another question about vDPA suspend/resume:
> >
> > /* Host notifiers must be enabled at this point. */
> > void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev, bool
> > vrings)
> > {
> > int i;
> >
> > /* should only be called after backend is connected */
> > assert(hdev->vhost_ops);
> > event_notifier_test_and_clear(
> > &hdev->vqs[VHOST_QUEUE_NUM_CONFIG_INR].masked_config_notifier);
> > event_notifier_test_and_clear(&vdev->config_notifier);
> >
> > trace_vhost_dev_stop(hdev, vdev->name, vrings);
> >
> > if (hdev->vhost_ops->vhost_dev_start) {
> > hdev->vhost_ops->vhost_dev_start(hdev, false);
> > ^^^ SUSPEND ^^^
> > }
> > if (vrings) {
> > vhost_dev_set_vring_enable(hdev, false);
> > }
> > for (i = 0; i < hdev->nvqs; ++i) {
> > vhost_virtqueue_stop(hdev,
> > vdev,
> > hdev->vqs + i,
> > hdev->vq_index + i);
> > ^^^ fetch virtqueue state from kernel ^^^
> > }
> > if (hdev->vhost_ops->vhost_reset_status) {
> > hdev->vhost_ops->vhost_reset_status(hdev);
> > ^^^ reset device^^^
> >
> > I noticed the QEMU vDPA code resets the device in vhost_dev_stop() ->
> > vhost_reset_status(). The device's migration code runs after
> > vhost_dev_stop() and the state will have been lost.
> >
>
> vhost_virtqueue_stop saves the vq state (indexes, vring base) in the
> qemu VirtIONet device model. This is for all vhost backends.
>
> Regarding the state like mac or mq configuration, SVQ runs for all the
> VM run in the CVQ. So it can track all of that status in the device
> model too.
>
> When a migration effectively occurs, all the frontend state is
> migrated as a regular emulated device. To route all of the state in a
> normalized way for qemu is what leaves open the possibility to do
> cross-backends migrations, etc.
>
> Does that answer your question?
I think you're confirming that changes would be necessary in order for
vDPA to support the save/load operation that Hanna is introducing.
> > It looks like vDPA changes are necessary in order to support stateful
> > devices even though QEMU already uses SUSPEND. Is my understanding
> > correct?
> >
>
> Changes are required elsewhere, as the code to restore the state
> properly in the destination has not been merged.
I'm not sure what you mean by elsewhere?
I'm asking about vDPA ioctls. Right now the sequence is SUSPEND and
then VHOST_VDPA_SET_STATUS 0.
In order to save device state from the vDPA device in the future, it
will be necessary to defer the VHOST_VDPA_SET_STATUS 0 call so that
the device state can be saved before the device is reset.
Does that sound right?
Stefan
