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

Jason Wang
2023-09-21T03:14:49Z

On Wed, Sep 20, 2023 at 8:40 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Sep 20, 2023 at 08:16:13PM +0800, Zhu, Lingshan wrote:
> >
> >
> > On 9/20/2023 8:05 PM, Michael S. Tsirkin wrote:
> > > On Wed, Sep 20, 2023 at 07:22:32PM +0800, Zhu, Lingshan wrote:
> > > >
> > > > On 9/20/2023 6:36 PM, Michael S. Tsirkin wrote:
> > > > > On Wed, Sep 20, 2023 at 02:06:13PM +0800, Zhu, Lingshan wrote:
> > > > > > On 9/19/2023 2:49 AM, Michael S. Tsirkin wrote:
> > > > > > > On Mon, Sep 18, 2023 at 06:41:55PM +0000, Parav Pandit wrote:
> > > > > > > > > Please refer to the code for setting FEATURES_OK.
> > > > > > > > It wont work when one needs to suspend the device.
> > > > > > > > There is no point of doing such work over registers as fundamental framework is over the AQ.
> > > > > > > Well not really. It's over admin commands. When these were built the
> > > > > > > intent always was that it's possible to use admin commands through
> > > > > > > another interface, other than admin queue. Is there a problem
> > > > > > > implementing admin commands over a memory BAR? For example, I can see
> > > > > > > an "admin command" capability pointing at a BAR where
> > > > > > > commands are supplied, and using a new group type referring to
> > > > > > > device itself.
> > > > > > I am not sure, if a bar cap would be implemented as a proxy for the admin vq
> > > > > > based live migration.
> > > > > Not a proxy for a vq in that there's no vq then.
> > > > I think if the driver sends admin commands through a VF's bar, then
> > > > VF forwards the admin commands to the PF, it acts like a proxy,
> > > > or an agent. Anyway it takes admin commands.
> > > Why send them to the PF? They are controlling the VF anyway.
> > I think its still too heavy compared to this series proposal
>
> it will be on you to prove all the complexity is unnecessary though.
>
> > >
> > > > So the problems we have discussed still exist.
> > > > > > then the problems of admin vq LM that we have
> > > > > > discussed
> > > > > > still exist.
> > > > > I freely admit the finer points of this extended flamewar have been lost
> > > > > on me, and I wager I'm not the only one. I thought you wanted to migrate
> > > > > the device just by accessing the device itself (e.g. the VF) without
> > > > > accessing other devices (e.g. the PF), while Parav wants it in a
> > > > > separate device so the whole of the device itself can passed through to
> > > > > guest. Isn't this, fundamentally, the issue?
> > > > we are implementing basic facilities for live migration.
> > > >
> > > > We have pointed out lots of issues, there are many discussions with
> > > > Jason and Parav about the problems in migration by admin vq, for example:
> > > > security, QOS and nested.
> > > /me shrugs
> > > Thanks for the summary I guess. Same applies to almost any proposal.
> > > What would help make progress is an explanation why this has grown into
> > > a megathread. Do you understand Parav's thoughts well enough to
> > > summarize them?
> > as far as I see, I don't see admin vq as must for live migration.
> > and it does not serve nested for sure.
> > >
> > > > > > the bar is only a proxy, doesn't fix anything. and even larger
> > > > > > side channel attacking surface: vf-->pf-->vf
> > > > > In this model there's no pf. BAR belongs to vf itself
> > > > > and you submit commands for the VF through its BAR.
> > > > > Just separate from the pci config space.
> > > > If using the bar to process admin commands,
> > > > is this solution too heavy compared to my proposal in this series?
> > > somewhat - because it's more comprehensive - you can actually
> > > migrate a device using it.
> > > this series just begins to define how to poke at some
> > > of the vq state - it's a subset of the necessary functionality.
> > >
> > > And it will give you a bunch of side benefits, such as
> > > support for legacy compat commands that were merged.
> > next version will include in-flight descriptors and dirty page tracking.
>
> what we don't need is another version of this megathread.
> which it sounds like you intend to restart?
> nor do I cherish maintaining two independent mechanisms for doing
> the same thing in the spec.

I'm not sure how to define "the same thing" but we had different
transports for accessing basic facilities like virtqueue, device
status, etc. I don't get why live migration is different. Especially
considering migration is actually a combination of several independent
functions.

And we have already had different ways to transport legacy devices as
well. It's really hard to say one mechanism can work for all use
cases.

> all of the above is already in parav's patchset so you guys should find
> a way to work together rather than compete?

There're things that are missed in Parav's series for sure, that is
the migrating for owner and nest.

From my point of view there's no competition. The main issue I see so
far is that you want to couple migration with admin commands but I
don't see much advantages to doing this.

And before the collaboration, I need to first figure out if Parav's
proposal can work. Or what's wrong with this series. If it is just a
missing of any function, it could be added on top for sure, if it
belongs to the basic facility part, it can be reused (that's the
motivation for this series).

Thanks