Re: [PATCH v2 3/4] vdpa: handle VIRTIO_NET_CTRL_ANNOUNCE in vhost_vdpa_net_handle_ctrl_avail

[Eugenio Perez Martin]

On Mon, Dec 5, 2022 at 5:27 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Thu, Dec 1, 2022 at 5:29 PM Eugenio Perez Martin <eperezma@redhat.com> 
> wrote:
> >
> > On Thu, Dec 1, 2022 at 9:39 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Wed, Nov 30, 2022 at 3:07 PM Eugenio Perez Martin
> > > <eperezma@redhat.com> wrote:
> > > >
> > > > On Wed, Nov 30, 2022 at 8:02 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > >
> > > > > On Fri, Nov 25, 2022 at 1:33 AM Eugenio Pérez <eperezma@redhat.com> 
> > > > > wrote:
> > > > > >
> > > > > > Since this capability is emulated by qemu shadowed CVQ cannot 
> > > > > > forward it
> > > > > > to the device. Process all that command within qemu.
> > > > > >
> > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > > ---
> > > > > >  net/vhost-vdpa.c | 15 ++++++++++++---
> > > > > >  1 file changed, 12 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > > > index 2b4b85d8f8..8172aa8449 100644
> > > > > > --- a/net/vhost-vdpa.c
> > > > > > +++ b/net/vhost-vdpa.c
> > > > > > @@ -489,9 +489,18 @@ static int 
> > > > > > vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
> > > > > >      out.iov_len = iov_to_buf(elem->out_sg, elem->out_num, 0,
> > > > > >                               s->cvq_cmd_out_buffer,
> > > > > >                               vhost_vdpa_net_cvq_cmd_len());
> > > > > > -    dev_written = vhost_vdpa_net_cvq_add(s, out.iov_len, 
> > > > > > sizeof(status));
> > > > > > -    if (unlikely(dev_written < 0)) {
> > > > > > -        goto out;
> > > > > > +    if (*(uint8_t *)s->cvq_cmd_out_buffer == 
> > > > > > VIRTIO_NET_CTRL_ANNOUNCE) {
> > > > > > +        /*
> > > > > > +         * Guest announce capability is emulated by qemu, so dont 
> > > > > > forward to
> > > > >
> > > > > s/dont/don't/
> > > > >
> > > >
> > > > I'll correct it, thanks!
> > > >
> > > > > > +         * the device.
> > > > > > +         */
> > > > > > +        dev_written = sizeof(status);
> > > > > > +        *s->status = VIRTIO_NET_OK;
> > > > >
> > > > > I wonder if we should avoid negotiating ANNOUNCE with vDPA parents if
> > > > > we do this?
> > > > >
> > > >
> > > > I can re-check, but the next patch should avoid it.
> > >
> > > Kind of, it makes sure guest can always see _F_ANNOUNCE. But does it
> > > prevent _F_ANNOUNCE from being negotiated?
> > >
> >
> > It should go like:
> > * vhost_net_ack_features calls vhost_ack_features with feature_bits =
> > vdpa_feature_bits and features = guest acked features.
> > vhost_ack_features stores in hdev->acked_features only the features
> > that met features & bit_mask, so it will not store _F_ANNOUNCE.
> > * vhost_vdpa_set_features is called from vhost_dev_set_features with
> > features = dev->acked_features. Both functions can add features by
> > themselves (VHOST_F_LOG_ALL, VIRTIO_F_IOMMU_PLATFORM), but no
> > _F_ANNOUNCE.
> >
> > Still untested.
>
> Ok.
>
> >
> > > > Even if
> > > > negotiated, the parent should never set the announce status bit, since
> > > > we never tell the device is a destination device.
> > >
> > > That's the point, do we have such a guarantee? Or I wonder if there's
> > > any parent that supports _F_ANNOUNCE if yes, how it is supposed to
> > > work?
> > >
> >
> > At the moment it is impossible to work since there is no support for
> > config interrupt from the device. Even with config interrupt,
> > something external from qemu should make the device enable the status
> > bit, since the current migration protocol makes no difference between
> > to be a migration destination and to start the device from scratch.
> > Unless it enables the bit maliciously or by mistake.
> >
> > Just for completion, the current method works with no need of vdpa
> > device config interrupt support thanks to being 100% emulated in qemu,
> > which has the support of injecting config interrupts.
>
> Ok, rethink this feature, I think I can find one use case for
> _F_ANNOUNCE, that is, the migration is totally done through the vDPA
> device (DPU) itself.
>

To make sure we are on the same page, this migration would save some
things like transfer the status through qemu, but it is not possible
at the moment. A few things need to be developed for that to make it
possible.

The default behavior is to emulate the announce feature / status bit
at the moment, so no ack to the device is needed. If we want that
passthrough, a new parameter or similar needs to be developed, so the
feature is negotiated with the device and not emulated in get_config.

Is that accurate?

Thanks!

> I think we can go forward and revisit this issue in the future.
>
> Thanks
>
> >
> > Thanks!
> >
>