On Mon, Dec 5, 2022 at 9:07 PM Eugenio Perez Martin <epere...@redhat.com> wrote: > > On Mon, Dec 5, 2022 at 5:27 AM Jason Wang <jasow...@redhat.com> wrote: > > > > On Thu, Dec 1, 2022 at 5:29 PM Eugenio Perez Martin <epere...@redhat.com> > > wrote: > > > > > > On Thu, Dec 1, 2022 at 9:39 AM Jason Wang <jasow...@redhat.com> wrote: > > > > > > > > On Wed, Nov 30, 2022 at 3:07 PM Eugenio Perez Martin > > > > <epere...@redhat.com> wrote: > > > > > > > > > > On Wed, Nov 30, 2022 at 8:02 AM Jason Wang <jasow...@redhat.com> > > > > > wrote: > > > > > > > > > > > > On Fri, Nov 25, 2022 at 1:33 AM Eugenio Pérez <epere...@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 <epere...@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! > > > > > >