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

[Jason Wang <jasowang@redhat.com>]

On Mon, Sep 25, 2023 at 6:41âPM Parav Pandit <parav@nvidia.com> wrote:
>
>
>
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Friday, September 22, 2023 8:38 AM
>
> > > Device context has no overlap.
> >
> > I can give you one example, e.g debugging.
> >
> Almost every feature needs debugging. :)
> So I am omitting it for time being.

Well, I don't think so. We have a lot of handy tools for that (ethtool -d?).

>
>
> > >
> > > Dirty page tracking has no overlap. What do you want to profile and monitor?
> > In case if you want to profile, it can be used without migration command
> > anyway?
> >
> > It works like a dirty bit of PTE. We all know it has a broader use case than
> > logging. For example, tracking working set and do optimization on
> > IOMMU/IOTLB or even device IOTLB.
> >
> > 1) Try to prove your facility can only work for one specific cases
> > 2) Try to prove your facility can work for more than one cases
> >
> > Which one is easier and more beneficial to virtio?
> >
> >
> > > If you describe, may be we I can split "device migration" chapter to
> > > two pieces, Device management and device migration.
> > >
> > > Device migration will use these basic facility.
> > > Would that help you?
> >
> > Definitely, but it needs to be done by not making it under the subsection of
> > admin commands, that's it.
> >
> > Let me repeat once again here for the possible steps to collaboration:
> >
> > 1) define virtqueue state, inflight descriptors in the section of basic facility but
> > not under the admin commands
> It will be part of the device context such a way that so that one can only read the vq state only instead of full device context.
> This will work.

I'm not sure what it looks like, but I think they are well decoupled
in this series. E.g driver can choose to just read e.g last_avail_idx
and report to ethtool or watchdog.

>
> > 2) define the dirty page tracking, device context/states in the section of basic
> > facility but not under the admin commands
> Great.
> Device context is already defined in the basic facility outside of the admin commands in [1].
> Current text is around device migration and spec evolves it can adopt more generic text without device migration.
>
> [1] https://lists.oasis-open.org/archives/virtio-comment/202309/msg00064.html

As I replied in other thread, I see several problems:

1) layer violation, PCI specific state were mixed into the basic facilities
2) I don't see a good definition on "device context"
3) TLV works fine for queue but not registers

What needs to be done first is to describe what device context means
and what it contains. Not the actual data structure since it may vary.

>
> > 3) define transport specific interfaces or admin commands to access them
> >
> As you also envisioned, it is done using admin commands to access it.

That's fine, but it should allow other ways.

>
> > Does this work? It seems you refused such steps in the past.
> >
> I didnât.
> There must be some confusion in many emails we both exchanged, because already posted v0 has it split such as device context.
>
> For dirty page tracking I couldnât find a solid use case without device migration, so I asked which you already replied above.

First, you never explain why such coupling gives us any benefit.
Second, I've given you sufficient examples but you tend to ignore
them. Why not go through Qemu codes then you will see the answer.

>
> > Actually, I would like to leave 2) as it's very complicated which might not
> > converge easily.
> >
> I will split current "device migration" section to two.
> 1. device management
> 2. device migration
>
> Device management covers device mode, device context and dirty page tracking.

I don't see any connection between "management" and "device context".

> Device migration refers to device management section.
>
> We can omit the dirty page tracking commands for a moment and first close on the device mode and device context as first step.
> Since it is already part of v0 and it is needed, I will keep it in subsequent v1 but moved to device management section.

Could you please answer what's wrong with the first 4 patches in this series?

Thakns

>
> Michael,
> Are you ok with this approach to step forward?