Re: [PATCH v2 0/2] transport-pci: Introduce legacy registers access using AQ

[Jason Wang <jasowang@redhat.com>]

On Thu, May 11, 2023 at 5:08âAM Parav Pandit <parav@nvidia.com> wrote:
>
> Hi Jason, Michel,
>
> > From: virtio-comment@lists.oasis-open.org <virtio-comment@lists.oasis-
> > open.org> On Behalf Of Parav Pandit
> > Sent: Wednesday, May 10, 2023 1:34 PM
> >
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Wednesday, May 10, 2023 12:16 PM
> > >
> > > On Wed, May 10, 2023 at 12:11:50PM -0400, Parav Pandit wrote:
> > > >
> > > >
> > > > On 5/10/2023 2:04 AM, Michael S. Tsirkin wrote:
> > > > > On Mon, May 08, 2023 at 10:23:39AM +0800, Jason Wang wrote:
> > > > > > > I thought so too originally. Unfortunately I now think that
> > > > > > > no, legacy is not going to be a byproduct of transport
> > > > > > > virtqueue for modern - it is different enough that it needs dedicated
> > commands.
> > > > > >
> > > > > > If you mean the transport virtqueue, I think some dedicated
> > > > > > commands for legacy are needed. Then it would be a transport
> > > > > > that supports transitional devices. It would be much better than
> > > > > > having commands for a partial transport like this patch did.
> > > > >
> > > > > OK I am beginning to get what you are saying.  So your criticism
> > > > > is
> > > > > this: what if device supports vq transport for modern, and we want
> > > > > to build a transitional device on top.  how will that look. yes?
> > > > > A reasonable thing to include at least in the commit log. Parav?
> > > > >
> > > > I am still trying to understand what is "vq transport for modern"?
> > > > Do you mean transporting currently defined config space access over vq?
> > > > If so, is this VQ belong to the guest or hypervisor?
> > >
> > > https://lore.kernel.org/all/20220826100034.200432-2-
> > > lingshan.zhu%40intel.com/t.mbox.gz
> >
> > The gz link is not accessible.
> > But I got the right link [1].
> >
> > [1] https://lore.kernel.org/all/20220826100034.200432-2-
> > lingshan.zhu@intel.com/
> >
> > 1. Above patch cover letter [1] is missing the basic objective/problem
> > statement.
> > i.e. why a transport virtqueue is needed?
> > But I probably get the idea of [1] as we did the AQ.
> >
> > 2. Commit log says about
> > a. querying resource of management device (aka group owner in AQ now) b.
> > creating and destroying the managed device  (aka group owner creating group
> > member devices) c. configure the managed device (aka group owner
> > configuring/composing group member devices such as VFs, SFs, SIOV).
> >
> > So, all above 2.a to 2.c belongs to the admin group owner and group
> > management commands like how it is defined in the AQ proposal.

Yes, the plan is to rebase on top of AQ. Lingshan may add more details here.

> >
> > So, 3 out of the 4 motivations are achieved by AQ proposal.
> > This AQ belongs to the hypervisor. I am clear on this part.
> >
> > 4th point in cover letter is: "config virtqueues of the managed device".
> >
> > This work belongs to the driver -> device direct communication using a queue
> > from driver to device.
> > So, I imagine this work can be done using a queue by the guest driver and
> > serviced by the device like how a guest driver configures the queue today
> > without any mediation.
> > For PCI, MMIO transport, surely this can be done by the PCI device directly
> > being is PF, VF or SIOV.
> > (Instead of config register, using a new queue interface). Looks fine to me.

Great.

> >
> > Can this new cfg queue mediated like CVQ that is done in a sw? May be yes.
> > Should it be always mediated when it is of VF, SIOV Device? Mostly no because
> > it is yet another VQ for PF, VF, SIOV.

Yes, the mediation is always conditional but not mandated when doing the design.

> >
> > I am yet to parse rest of the 4 patches, please give me some time to review it.
>
> I went over the past work of [1], [2].
>
> [1] https://lore.kernel.org/all/20220826100034.200432-2-lingshan.zhu@intel.com/
> [2] https://lists.oasis-open.org/archives/virtio-comment/202208/msg00141.html
>
> The "virtio q as transport" in [2] is bit misleading as its only role is to transport the _registers_ of the SIOV_R1 device through its parent PF.
> Rest of the work is the pure management work to manage the life cycle of SIOV devices (create/delete/configure/compose).

I think this can be tweaked for static provisioning setups like
SR-IOV. E.g group owner can choose to not advertise the life cycle
management commands. Then the hypervisor may know the provisioning is
transport specific.

>
> And the motivation is also clear is to provide composing a virtio device for the guest VM for the backward compatibility for 1.x part of the specification.
>
> It seems fine and indeed orthogonal to me that: it is for backward compatibility for already defined config fields for existing guest VM driver.
>
> It does not conflict with the cfgq/cmdq idea whose main purpose is for the new config fields, new use cases that doesn't require any mediation.
> Such cfgq works across PF, VF, SF/SIOV devices in uniform way without mediation.

My understanding is that the cfgq/cmdq could be done on top, since the
commands are part of transport unless I miss something.

> It also scales device register memory also very well in predictable way.
>
> The registers and feature bits access described in [2], can certainly be done using new non_legacy new AQ commands.
> Both legacy and non-legacy command have different semantics as Michael mentioned.
>
> The AQ semantics that Michael did as opposed to "virtqueue as transport" fits well for the use case described in [1].
>
> There are changes on going in MSI-X area and SIOV, so you might want to wait for it.

I think you meant IMS, actually, it's not hard to add new commands to
support a general IMS. We can start from a virtio specific one (as it
looks a common requirement not only for PCI/SIOV but also for
transport like MMIO)

> Or proposed command in [1] should be tagged as siov_r1, then things will be cleaner.

Maybe we can say, one of the implementations is siov_r1, since it can
be used to implement devices in other transport. One of the main
motivations for transport virtqueue is to reduce the transport
specific resources to ease the virtio device implementation.

>
> With that I don't see legacy 3 commands anyway conflict with [1].

It doesn't, but it's not elegant since:

1) Modern transport, admin virtqueue with well defined transport commands
2) Legacy transport, works moe like a PCI transport on top of the
admin virtqueue

Transport virtqueue tend to be transport independent, but 2) tie it
somehow to the PCI. That's why I suggest to

1) introduce legacy transport commands

or

2) tweak your proposal to become a PCI over admin virtqueue

> Some commands functionality is common between [1] and this proposal.
> But that's how the legacy is. It is confined to legacy emulation.
> So [1] can be done as follow_on to AQ and these series.
>
> A small note about [2].
> virtio_transportq_ctrl_dev_attribute should be detached from CREATE call and split to two commands.
> So that VF and SF/SIOV can both utilize it.
> SF/SIOV_R1 can use the creation and config part.
> VFs will use only the device features + config space.

Good point. This could be fixed.

Thanks

>