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

[Jason Wang <jasowang@redhat.com>]

å 2023/5/11 21:28, Parav Pandit åé:
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Thursday, May 11, 2023 3:07 AM
> > > > 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.h
> > > tml
> > >
> > > 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.
> It is just bunch of admin commands in the new SIOV or virtual virtio device chapter.
> It is not a transport chapter.


Provisioning could be a part of the transport. For example SR-IOV allows 
static and dynamic provisioning. If a virtio device needs to be 
implemented in VF, it needs to be provisioned first.


> > > 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.
> On top of what?
> cfgq/cmdq is just another queue like any other VQ.
> it is orthogonal to accessing 1.x registers using group owner's queue.


I think there hasn't been any proposal that call any virtqueue like 
cfgq/cmdq. So using that may cause a lot of misunderstanding. I think 
the context here is to provide or extend transport facilities via a 
virtqueue not something like the control virtqueue.


> > > 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)
> IMS configuration usually supposed to go directly to the device.


Admin virtqueue doesn't preventing anything from letting IMS go directly 
to the device.


>   Long discussion with Thomas on IMS topic.
> I will avoid diverting to unrelated discussion for now.
>
> > > 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.
> Yes, the main motivation as for backward compatibility for the currently defined features.
>
> > > With that I don't see legacy 3 commands anyway conflict with [1].
> > It doesn't, but it's not elegant since:
> I donât see how making 3 commands to 9 commands makes it elegant by doing bisecting of registers offset in hypervisor.


Or it needs to be done by the hardware and cross register write/read 
needs to be handled there as well.


> Its same thing using more opcodes.


Do we really care about the number of opcodes? I think we've reserved 
sufficient spaces.


> > 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
> The 4 patches are not transporting all the PCI things over transport VQ. it is not a transport.


It really depends on the different viewpoint. To me, adminq is not 
specific to PCI, so this proposal looks more like a partial transport 
for legacy.

So we have

1) all commands via PCI
2) all commands via adminq
3) part of the commands via PCI and the rest via adminq

This proposal goes to for 3 with transport specific commands while I 
think we need to try 1 and 2 then it is more general and it helps to 
avoid duplication with future features.

Thanks



> It is mediation done for SIOV like how it's done for 2 patches.
>
> > 1) introduce legacy transport commands
> >
> > or
> >
> > 2) tweak your proposal to become a PCI over admin virtqueue
> This is not the objective to transport all PCI virtio fields over AQ.
> Transport VQ commands proposal commands and commands in this proposal are just fine the way there without quoting it as "transport".