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

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

On Wed, May 10, 2023 at 09:08:44PM +0000, Parav Pandit 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.
> > 
> > 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.
> > 
> > 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.
> > 
> > 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).
> 
> 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.
> 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.
> Or proposed command in [1] should be tagged as siov_r1, then things will be cleaner.
> 
> With that I don't see legacy 3 commands anyway conflict with [1].
> 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.

I think some kind of "enable" command for VFs might have value. No?

-- 
MST