On Fri, Jun 09, 2023 at 05:11:53PM +0000, Parav Pandit wrote:
> > From: Michael S. Tsirkin <[email protected]>
> > 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.
> 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.
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
