Re: [virtio-comment] Re: [virtio-dev] Re: [PATCH v3 0/3] transport-pci: Introduce legacy registers access using AQ

[Jason Wang <jasowang@redhat.com>]

On Sun, Jun 11, 2023 at 8:27âAM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Jun 09, 2023 at 05:11:53PM +0000, Parav Pandit wrote:
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Friday, June 9, 2023 3:22 AM
> > >
> > > On Fri, Jun 09, 2023 at 02:27:01PM +0800, Jason Wang wrote:
> > > > > I would like to keep the stateful interactions of 1.x device outside of 0.9.5.
> > > >
> > > > I don't think this is a real problem, but let's see the drawbacks of
> > > > this proposal:
> > > >
> > > > 1) non-trivial changes of full new transport alike ABI
> > > > 2) can't work for nesting
> > > > 3) require vendor to implement legacy behaviour which can't be
> > > > documented precisely
> > > > 4) require admin virtqueue to work
> > > >
> > > > We won't have the above issues if we use modern + legacy header/mac.
> > > >
> > > > Thanks
> > >
> > >
> > > All this is true. But it's by design. It makes the legacy thing as isolated as
> > > possible. Because let's be frank we won't be able to drop legacy support in like
> > > 10 years. But hardware vendors will do that quickly if they can't sell hardware.
> >
> > I don't think above 4 points are true for below reasoning.
> >
> > Hypervisor flow without involving guest; first sanity round to figure out things can work:
> > 1. reset the device
> > 2. set ACK and DRIVER bit
> > 3. read features and make sure _MAC, _HDR are supported.
> > 4. if not abort.
> > 5. reset the device 2nd time so that guest can have right view of things.
> >
> > On guest access of legacy area:
> > 6. reset the device
> > 7. set ACK and DRIVER bit on guest request
> > 8. read features and make sure _MAC, _HDR are offered
> > 8.1 Hope for the best that on two completely unrelated device reset on #1 and #6, the device offers exact same feature bits.
> > And mention this in the spec 1.x for rest of the future.
>
> > We shouldn't be adding such a limitation to the spec.
> > We have seen this with mlx5 device where 5 years ago one thought this cannot happen.
> > And now with modern use case now features changes across two resets for mlx5 device.
>
> Interesting this actually violates a spec recommendation:
>
>         If a device has successfully negotiated a set of features
>         at least once (by accepting the FEATURES_OK \field{device
>         status} bit during device initialization), then it SHOULD
>         NOT fail re-negotiation of the same set of features after
>         a device or system reset.  Failure to do so would interfere
>         with resuming from suspend and error recovery.
>
>
> > Baking such limitation in current spec for past 1.x is sub-optimal.
> >
> > ok, so to avoid baking such reset flow nasty things in spec, lets avoid flow of #1 to #5 in hypervisor.
> > So, provision the device to support these new feature bits via AQ.
> > So AQ is required for feature provisioning anyway.
> >
> > So, your point #4 is required in both methods and so it scores same as this proposal.
> > Hence feature bit is not of an advantage.
> >
> > With _MAC now we need writable mac on 1.x config space.
> > for 1.x writable mac has two requirements.
> > 1. It must be atomic (not for 0.9.5, but must for 1.x like CVQ)
> > 2. should be synchronous to know success/failure to know when it is effective
> >
> > Both are present on the CVQ, so yet add another duplicate 1.x scheme that fulfill above requirements.
> >
> > ok. So may be let's do AQ command for just mac setting.
> >
> > This scores down now for two reasons.
> > a. Duplicate of existing 1.x feature
> > b. requires AQ.
> >
> > So, from RW mac we moved from 1.x cfg space to AQ. This mediation for 1.x is not good.
> > And now it's not trivial either as it is not just simple *p_mac = X.
> > Hence, #1 of non-trivial starts to looks less appealing.
> >
> > Your point #3 about vendor to implement legacy behavior.
> > If vendor needs to support legacy, vendor anyway needs to implement _HDR anyway in data path.
> > and above MAC change, and feature provisioning.
> >
> > For legacy there is no extra/special documentation.
> > All the behavior listed in Legacy interfaces section for configuration register present applies here.
> >
> > So, I don't see mac support is trivial by any means compared to proposed scheme for 1.x.
> > Hence, comparing trivialness of two solutions seems same for your point #1 once you do mac plumbing.
> >
> > Regarding #2 on nesting. I won't claim I understand this as your level of knowledge.
> > If you are sauing only the VF is in VM as virtio PCI device to supporting nested guest, that doesn't have AQ, hence it doesn't work due above issues.
>
> Nothing is tied to admin queue here btw - it's using admin commands.
> And by the way, there's a simple way to make this work with nesting
> down the road if we want to: add support for sending admin commands
> using MMIO. Jason, if you want to solve nesting I think that's the best
> way. Will address other use-cases too, not just legacy.

Probably, but then just for the legacy support it would be much easier
to allow legacy MMIO BAR in this case other than inventing new
capability for carrying admin commands.

And I found more issues:

1) The owner device itself is not a member of the group. We need to
relax this otherwise MMIO still requires a PF for example.
2) The admin command is variable length, which is kind of hard to be
implemented via a MMIO interface, we need tweak the spec to define a
maximum length or invent a way to probe the maximum length

Thanks

>
>
> > Then yes, but than it is back to square one, where you need sequence #1 to #8 to be done + non-forward-looking spec changes on reset flow.
> >
> > In that alternative, one can say, hey skip steps #1 to #5, and on step #8 doesn't have required feature bit, mark the device FAILED.
> > But this is common case and its late in the init flow to discover it.
> >
> > And now MAC cannot be set atomically either in nesting with just feature bit without ugly and non-trivial spec updates.
> > And when one needs nesting with legacy, probably PV is better.
> >
> > As Michael said,
> > Overall isolating legacy to AQ for config, intx and by reusing 1.x's notification is good trade off where 1.x device level interface is kept as detached from the legacy as possible.
> >
> > This is why the notification address query also was desired via AQ as proposed in v3, but it is small trade off if you think it should be discovered via PCI cap like v5.
>