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

[Jason Wang]

On Mon, Dec 5, 2022 at 9:07 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
>
> 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.

Somehow, it means the DPU is in charge of doing all the migration.

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

Yes.

Thanks

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