Re: [virtio-dev] Re: [PATCH 0/5] virtio: introduce SUSPEND bit and vq state

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Sep 21, 2023 at 09:17:53AM +0000, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Thursday, September 21, 2023 1:41 PM
> 
> > > I mean to say,
> > > Virtio spec has not achieved mediation less, device migration.
> > > Virtio spec has not achieved device migration using mediation.
> > 
> > But yes it has - it was implemented with shadow vq.
> >
> Shadow vq + several other trap points on config space, cvq and more.

exactly.

> we cannot suspend the whole device and resume from where it was left off.
> Those extensions are happening now.
> 
> > > And two proposals are trying to do it.
> > >
> > > >
> > > > > >
> > > > > > > > 1. For mediation something that works within existing
> > > > > > > > mediation framework - e.g. reusing as he does feature bits -
> > > > > > > > will require less support than a completely separate facility.
> > > > > > > > I think Zhu Lingshan also believes that since there will be
> > > > > > > > less code -> less security issues.
> > > > > > > >
> > > > > > > With approach of [1], there is less code in the core device
> > > > > > > migration flow
> > > > > > because none of those fields etc are parsed/read/written by the
> > > > > > driver software.
> > > > > >
> > > > > > What is or is not executed in a specific flow is a separate question.
> > > > > > But the point is vdpa and any mediation have to talk virtio
> > > > > > things such as feature bits. So reusing e.g. feature bits needs
> > > > > > less code than operating the admin command machinery to check
> > > > > > what is supported. Yes, you can operate this machinery during
> > > > > > setup and not during migration itself. It's still less code to maintain.
> > > > > >
> > > > > I wouldn't go down the path of code comparison.
> > > > > But if you want to: we can take a concrete example of what is done
> > > > > by similar
> > > > device who uses admin command approach.
> > > > > The admin command-based approach migration driver is likely 10x
> > > > > smaller
> > > > than the actual driver driving the feature bits and rest of the config.
> > > >
> > > > yes but mediation driver already has to do feature bits. so if doing
> > > > mediation then the cost of adding this specific extension is low.
> > > >
> > > I thought first you were counting the cost of the code and not the spec in your
> > point "feature bits needs less code than operating".
> > 
> > yes - with vdpa it's mostly just
> > 	vdev->status |= SUSPEND
> > 	vdev->status &= ~SUSPEND
> > all over the place.
> > 
> + inflight descriptors.

for sure, this is just stopping it.

> > > > > If one needs more precise numbers of number of lines of code, I can
> > derive it.
> > > > > As features and functionality grows, every line of code gets added
> > > > > there in
> > > > mediation too.
> > > > > I agree such mediation has value and use case, as we know it is
> > > > > not the only
> > > > approach fitting all use cases.
> > > >
> > > > Do you see how this extension is easier for mediation than driving
> > > > admin queue though?
> > > >
> > > If we count the total cost of code than building the mediation framework +
> > extensions, than it is not.
> > > But as I said, I wouldn't compare two solutions as they are addressing a slightly
> > different requirement.
> > 
> > yes they are. the point of comparison was explaining why people who use
> > mediation anyway might not want to also use aq. can i assume that's clear?
> >
> I am not fully sure.
> I frankly don't find it right for member virtio device itself to be mediated.
> Vdpa stack make total sense when the underlying device is not virtio and hence emulation.
> But when there is native virtio member device, further mediation is overkill for certain scenarios.
> But I understand that it helps to utilize a vdpa stack and thereby overcome some limitations, while it introduces other limitations...

yes. whether it makes sense depends on the use-case.


> > > What to compare is what can be reused between two solutions.
> > >
> > > > > >
> > > > > > > > 2. With or without mediation, the mapping of commands to VFs
> > > > > > > > is simpler, allowing more control - for example, let's say
> > > > > > > > you want to reset a VF - you do not need to flush the queue
> > > > > > > > of existing commands, which might potentially take a long
> > > > > > > > time because some other VFs are very busy - you just reset
> > > > > > > > the VF which any unmap flow will
> > > > > > already do.
> > > > > > > >
> > > > > > > If I understand you right, to reset a VF, no need to flush the
> > > > > > > queues without
> > > > > > mediation too.
> > > > > > > Just do VF FLR or do device reset, both will be fine.
> > > > > >
> > > > > > Not to reset the VF - that's a narrow definition. To put it back
> > > > > > in its original state unrelated to any VMs.  Nope, FLR is not
> > > > > > enough - there could be commands in queue addressing the VF. If
> > > > > > they take effect after FLR VF state changed and it can't be
> > > > > > cleanly assigned to a new
> > > > VM.
> > > > > >
> > > > > If you mean the SR-PCIM command, than it is virtio spec to define them.
> > > > > We ratified this recently in the PCI-SIG of what gets cleared on
> > > > > VF FLR and
> > > > what stays that SR-PCIM interface to do.
> > > >
> > > > Interesting. Which ECN exactly do you refer to?
> > > >
> > > B405.
> > >
> > > > > So if other commands are in pipe, only after they are done, such
> > > > > VF will be
> > > > assigned to new VM.
> > > >
> > > > Exactly. this is exactly what I meant when I said "flush the queue"
> > > > - you have to wait until these commands are done, then do reset.
> > > >
> > > Not exactly. VF reset is fully controlled by the guest.
> > > Hence, it does not collide with admin side of commands,
> > 
> > No, host does VF reset in a number of situations, including guest restart, guest
> > shutdown, etc.
> > 
> That is fine, it can do.
> 
> > > Same admin command for dirty page tracking and device context do not mess
> > with FLR.
> > > This is the critical because VF FLR cannot clear the page addresses already
> > reported and not yet read by the driver.
> > > It is covered in admin proposal.
> > 
> > They might not mess with it in that you can still do FLR but they will mess with
> > what hosts commonly try to achieve with FLR which is getting a clean VF that
> > has nothing to do with a given guest state.
> > For example
> > - queue stop command on PF
> > - flr on VF
> > - stop command is seen by PF
> >
> This just fine, because FLR is not resetting what PF has done.
> PF is not touching FLR side of things either.
>  
> > will get you a VF that is not running and can not be given to another guest. You
> > have to
> > - queue stop command on PF
> > - stop command is seen by PF
> > - flr on VF
> > in this order.
> > 
> Since you are discussing the admin patches of [1], better to discuss there.
> But even if we follow the sequence you described, it is also fine.

well then flr itself is now insufficient to reset the VF completely to
its original state.  I can't say whether you see why one has to make
sure device is not stopped before assigning it to guest or not.
a stopped device can't be used by guest the way it expects.
And to make sure, one has to wait for some admin command to complete,
one way or another.

-- 
MST