Re: [virtio-comment] Re: [PATCH 0/2] Selective queue enabling

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

On Mon, Jun 26, 2023 at 10:19:48AM +0200, Eugenio Perez Martin wrote:
> On Mon, Jun 26, 2023 at 4:53âAM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Mon, Jun 26, 2023 at 5:32âAM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Sun, Jun 25, 2023 at 01:31:36PM +0800, Jason Wang wrote:
> > > > On Sun, Jun 25, 2023 at 2:41âAM Eugenio Perez Martin
> > > > <eperezma@redhat.com> wrote:
> > > > >
> > > > > On Sun, Jun 18, 2023 at 1:08âAM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > >
> > > > > > On Sat, Jun 17, 2023 at 02:53:46PM +0200, Eugenio Perez Martin wrote:
> > > > > > > On Fri, Jun 16, 2023 at 4:40âPM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > > >
> > > > > > > > On Thu, Jun 15, 2023 at 10:35:09AM +0200, Eugenio Perez Martin wrote:
> > > > > > > > > On Tue, Jun 13, 2023 at 2:29âPM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > > > > >
> > > > > > > > > > On Tue, Jun 13, 2023 at 12:12:43PM +0200, Eugenio Perez Martin wrote:
> > > > > > > > > > > On Tue, Jun 13, 2023 at 9:53âAM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > On Tue, Jun 13, 2023 at 03:46:35AM -0400, Michael S. Tsirkin wrote:
> > > > > > > > > > > > > On Mon, Jun 12, 2023 at 09:56:57AM +0200, Eugenio Perez Martin wrote:
> > > > > > > > > > > > > > > > Another drawback of that is the long time a device can spend to
> > > > > > > > > > > > > > > > reconfigure its memory maps.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > The map is already used for cvq though, I don't see why it would take
> > > > > > > > > > > > > > > much more with this.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > In the case of vDPA HW, CVQ maps may not need to reach the device's or
> > > > > > > > > > > > > > platform IOMMU, so the modification of that mapping is way cheaper.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Oh, I get it. It's setup time, but you are right.
> > > > > > > > > > > > >
> > > > > > > > > > > > > > > > taking that into account, does it make
> > > > > > > > > > > > > > > > sense to send a new version with the bit 1 on enable?
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Thanks!
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Current hardware won't support the extra bit though.
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > Thinking more about this, I think the following works:
> > > > > > > > > > > >
> > > > > > > > > > > > - enable all queues before DRIVER_OK
> > > > > > > > > > > > - reset all data queues before DRIVER_OK
> > > > > > > > > > > > - DRIVER_OK
> > > > > > > > > > > > - CVQ commands
> > > > > > > > > > > > - re-enable data queues
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > No?
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > If it is valid to write to queue_reset before DRIVER_OK, then sure. I
> > > > > > > > > > > asked for that in the past at
> > > > > > > > > > > https://lists.oasis-open.org/archives/virtio-dev/202305/msg00329.html
> > > > > > > > > > > .
> > > > > > > > > >
> > > > > > > > > > Well then Jason said:
> > > > > > > > > >  we should make svq work without RING_RESET
> > > > > > > > > > but now apparently we went full circle and propose
> > > > > > > > > > a feature bit after all?
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > My understanding is that RING_RESET covers more than just delaying the
> > > > > > > > > enabling of net data vqs, like to change the address or properties of
> > > > > > > > > vqs after DRIVER_OK. While the late enabling may be easy to implement
> > > > > > > > > in current hw, vq property changes may be more challenging. There are
> > > > > > > > > current HW that only support late enabling but not reconfigure the
> > > > > > > > > queues. Ideally, just the subset of late enabling is needed.
> > > > > > > > >
> > > > > > > > > Does it make sense to change the proposal to something in the line of:
> > > > > > > > > RING_F_LATE_ENABLING: This feature flag is a subset of _F_RING_RESET.
> > > > > > > > > If this feature is negotiated, all vqs start as if the vqs has been
> > > > > > > > > reset before DRIVER_OK. This feature does not allow resetting a queue
> > > > > > > > > once it is live.
> > > > > > > > >
> > > > > > > > > Thanks!
> > > > > > > >
> > > > > > > > Maybe. No idea - I am not a hardware designer. Can some hardware
> > > > > > > > vendors chime in and confirm?
> > > > > > > > It seems like a very very narrow "make my hack work" use-case.
> > > > > > > > And I'm not even sure how well it works.
> > > > > > > > For example what if cvq command enables rss? If the queues in
> > > > > > > > question are not enabled packets will be dropped.
> > > > > > > > So - fix some setups, break others?
> > > > > > > >
> > > > > > >
> > > > > > > I don't think it breaks, they are already dropped before setting
> > > > > > > DRIVER_OK anyway. If we restore the configuration before DRIVER_OK in
> > > > > > > the future, the device is supposed to drop packets meanwhile anyway,
> > > > > > > isn't it?
> > > > > >
> > > > > >
> > > > > > I just thought of an idea. Devices generally don't poke at rings
> > > > > > by themselves. How about at init time we cause kicks on data queues
> > > > > > to vmexit? Then on 1st kick we map hardware so following ones will
> > > > > > go straight to hardware. This will make sure cvq is processed
> > > > > > first. Hmm?
> > > >
> > > > This is not the case at least for vhost-net. It doesn't need to be
> > > > kicked in order to work. And it's an example that we need clearly
> > > > define the behaviour instead of depending on any existing
> > > > implementation details.
> > >
> > > This kind of guarantee however sounds generally useful to
> > > control which queues are accessed when.
> > >
> > > This is only needed after restore, which is ATM a vdpa thing,
> > > so can just be a vdpa flag, but I'm ok with it as a
> > > virtio flag too.
> >
> > Great. Having a virtio flag may help to reduce the extra and
> > duplicated effort spent in vdpa.
> >
> 
> So the flag should be something in the line of:
> VIRTIO_F_ACCESS_AFTER_KICK: The device will only access the ring after
> the first kick. Before that kick, it will not process any request.
> 
> ?
> 
> Thanks!

That's not bad. I am not sure adding this to spec is worth
the effort but at least it's kind of reasonable.
Are there uses such as hardening where something like this could be useful?


> > Thanks
> >
> > >
> > > > > >
> > > > >
> > > > > That would be possible, but nothing prevents the hw to start
> > > > > processing requests even without that first kick, isn't it?
> > > >
> > > > Yes.
> > > >
> > > > Thanks
> > > >
> > > > >
> > > > > I think that was discussed also in the past, as previous qemu patches
> > > > > gave a spurious kick to start queues just in case. I think the
> > > > > conclusion was that at least the vDPA device will start processing
> > > > > requests at DRIVER_OK, no need to wait for a kick.
> > > > >
> > > > > https://lore.kernel.org/all/CACGkMEt8cMM1UxVzxb0eHeaWSpR0ApvGzaF901vrM4m-uGMiPA@mail.gmail.com/
> > > > >
> > > > > Thanks!
> > > > >
> > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > > --
> > > > > > > > > > > > > MST
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > This publicly archived list offers a means to provide input to the
> > > > > > > > > > > > OASIS Virtual I/O Device (VIRTIO) TC.
> > > > > > > > > > > >
> > > > > > > > > > > > In order to verify user consent to the Feedback License terms and
> > > > > > > > > > > > to minimize spam in the list archive, subscription is required
> > > > > > > > > > > > before posting.
> > > > > > > > > > > >
> > > > > > > > > > > > Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> > > > > > > > > > > > Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> > > > > > > > > > > > List help: virtio-comment-help@lists.oasis-open.org
> > > > > > > > > > > > List archive: https://lists.oasis-open.org/archives/virtio-comment/
> > > > > > > > > > > > Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> > > > > > > > > > > > List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> > > > > > > > > > > > Committee: https://www.oasis-open.org/committees/virtio/
> > > > > > > > > > > > Join OASIS: https://www.oasis-open.org/join/
> > > > > > > > > > > >
> > > > > > > > > >
> > > > > > > >
> > > > > >
> > > > >
> > >
> >