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

[Jason Wang <jasowang@redhat.com>]

On Tue, May 16, 2023 at 11:45âAM Parav Pandit <parav@nvidia.com> wrote:
>
>
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Monday, May 15, 2023 11:28 PM
> >
> > Hi Parav:
> >
> > å 2023/5/15 23:49, Parav Pandit åé:
> > >> From: Jason Wang <jasowang@redhat.com>
> > >> Sent: Monday, May 15, 2023 3:11 AM
> > >>> 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.
> > >>
> > > Provisioning a device by admin != transport guest driver <-> device
> > communication.
> > >
> > > Two very different things.
> >
> >
> > When developing a new transport, we need to consider at least how to probe
> > the device. Before probe the device, it needs to be provisioned.
> Yes, and yes to both. I agree.
>
> > If we want a new transport, it could implement the provisioning especially if it
> > want the design to be bus independent. For the proposal like transport
> > virtqueue, it's also fine to use bus specific facility to provision and probe the
> > device.
> >
> Provisioning is for defining the transport specific attributes.
> Each transport has few command things and few unique things.
> So provisioning commands can define transport specific attributes and few generic things.
> This is fine.
>
> >
> > That is what I want to say.
> >
> >
> > >
> > >>>>> 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.
> > > The context is to compose a PCI device for a guest which for the currently
> > defined features.
> > > Once a new attribute is done using cfgq/cmdq there is no problem of
> > transporting it via its group member.
> > >
> > > And therefore, saying any new attribute also to ride on same tranportq/aq via
> > is not appropriate.
> >
> >
> > The point is to try to find if there's a better way, more below.
> >
> >
> > >
> > >> Admin virtqueue doesn't preventing anything from letting IMS go
> > >> directly to the device.
> > > But there is no aq/cfgq/cmdq defined for the guest, so with current proposal it
> > is prevented.
> >
> >
> > It's a choice of the implementation but not spec. Spec doesn't say adminq can't
> > be used for guest.
> >
> For guest and by guest are two different things.
> I am proposing that a cfgq/cmdq should be used directly by the guest driver as base line like any other VQ.

I'm fine with this, but if I understand correctly, but I don't see any
proposal like this that is posted to the list?

> And if there is a need for mediation, it should be possible.
>
> > I think we've discussed many times that legacy is tricky for hardware.
> And we want to proceed for the described use case and requirements.
>
> > And your proposal proves this further by introducing modern alike
> > notification areas.
> Not really. A modern device is offering the notification area like legacy to the legacy interface.
> So this is just fine.

I don't say it's not fine, it's pretty fine and transport virtqueue
have similar commands. And this also answer the question that you
think tunnel PCI over adminq is not scalable. We can keep this command
in that case as well, or invent new capabilities.

>
> > We probably need to deal with others like endianess.
> >
> No point in discussing this again. One size does not fit all.
> It solves large part of the use cases so it is valuable and will be supported.

This is doable, we need to leave some space for future development. Or
is it just complicated to have an endian command?

>
> > For any case, having a simple and deterministic device design is always
> > better assuming we've agreed that mediation is a must for legacy
> > drivers. Using dedicated commands can make sure the implementation won't
> > need to go with corner cases of legacy.
> >
> Corner case handling just moved from the device to hypervisor.

That's not safe, the device should be ready for malicious inputs.

>
> >
> > >
> > >>> Its same thing using more opcodes.
> > >>
> > >> Do we really care about the number of opcodes? I think we've reserved
> > >> sufficient spaces.
> > >>
> > > I donât see opcode as problem, we have plenty.
> > > And hence, when there is a device that _does not implement config space for
> > SIOV/SF, it should be introduced.
> > > It is the introduction and bisection at the hypervisor level unnecessary.
> > >
> > > And I donât have object to it either, the main reason is: I donât see it being
> > useful for 1.x access going forward.
> >
> >
> > As you said for future SIOV/SF? For example, it can have better
> > scalability in IMS since it doesn't require any real capabilities or
> > registers.
> >
> For SIOV there is no backward compatibility requirement.

It has several ways to support current virtio-pci drivers in the
guest, and such compatibility is almost a must for the public cloud.
We can't enforce the user to reinstall/recompile their
kernels/application in order to work for SIOV.

> Hence, it can start with fresh infra for real capabilities, registers and IMS.
>
> >
> > >
> > >>>> 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.
> > > You wrote/asked for "PCI transport over admin q".
> > > That means PCI capabilities, PCI config registers and all things defined in the
> > PCI transport section to go over the AQ.
> > > This is clearly not the objective of these 2 patches.
> >
> >
> > It's not but it can achieve the same function as this series with much
> > broader use cases.
> >
> >
> > > Nor it is your proposal of transport VQ.
> >
> >
> > Same as the above.
> >
> > I think we know your proposal might but it's worthwhile to figure out
> > whether or not there could be other better ways to achieve the same
> > thing. That's why the discussion happens on the cover letter.
> >
>
> > Both transport virtqueue and "PCI over adminq" can support legacy guest
> > with a lot of other use cases.
> >
> PCI over admin queue just doesnât work at performance scale to transport doorbells over a queue interface.
> So, things just stop there.

I think we are discussing what it can be, so see the above reply for
the notification areas, we can stick
virtio_admin_cmd_lq_notify_query_result command for PCI over adminq,
or invent new capabilities to make it fully self contained.

>
> >
> > >
> > > You only want to exchange currently defined config registers, interrupts and
> > notification configuration using AQ.
> > >
> > > Transport VQ is NOT transporting actual data, it is not transporting
> > notifications etc.
> > > It is just a config channel.
> > > Hence, they are just commands not a "full transport".
> >
> >
> > For any case, a virtqueue could not be a full transport. It needs bus
> > facilities to do bootstrap/DMA/notifications at least. The idea is to
> > save for transport specific resources and use a more scalable way to
> > config devices.
> >
> Hence it needs only needs a cfgq, not the full transport. :)
>

Well, not a native speaker but I'm fine if you want to call it configq.

> >
> > >
> > >> To me, adminq is not
> > >> specific to PCI, so this proposal looks more like a partial transport
> > >> for legacy.
> > > It is not a partial transport for legacy.
> > > A future transport may be able to do for SIOV/MMIO by adding those group
> > member identifier.
> > >
> > > Hence, I agreed that commands in the tranportvq proposal is fine, as
> > commands, not as new transport.
> > >
> > >> 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.
> > >>
> > > All legacy interface via AQ.
> >
> >
> > That's pretty fine. So we have already had modern transport via a
> > virtqueue, why not unify the design to avoid duplication? Otherwise we
> > may end up with
> >
> > 1) legacy PCI alike commands via admin virtqueue with modern alike
> > notification areas
> Modern commands on SIOV device for some backward compat is needed.
> A brand new SIOV device doesnât need it.

So I meant, if we invent legacy commands for transport/admin/config
virtqueue, we don't need those PCI alike commands.

Thanks

>
> > 2) modern device commands via admin virtqueue