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

[Jason Wang <jasowang@redhat.com>]

On Mon, Jun 26, 2023 at 5:40âPM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> 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?

I think so, the driver can simply ignore any device notifications
before the first kick. But it doesn't solve all the problems, we still
need to take care about the device without this feature.

Thanks

>
>
> > > 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/
> > > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > > >
> > > >
> > >
>