| Message ID | 20221019125210.226291-3-eperezma@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Guest announce feature emulation using Shadow VirtQueue | expand |
On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <eperezma@redhat.com> wrote: > > At this moment only _F_LOG is added there. > > However future patches add features that depend on the kind of device. > In particular, only net devices can add VIRTIO_F_GUEST_ANNOUNCE. So > let's allow vhost_vdpa creator to set custom emulated device features. > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > --- > include/hw/virtio/vhost-vdpa.h | 2 ++ > hw/virtio/vhost-vdpa.c | 8 ++++---- > net/vhost-vdpa.c | 4 ++++ > 3 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h > index 1111d85643..50083e1e3b 100644 > --- a/include/hw/virtio/vhost-vdpa.h > +++ b/include/hw/virtio/vhost-vdpa.h > @@ -31,6 +31,8 @@ typedef struct vhost_vdpa { > bool iotlb_batch_begin_sent; > MemoryListener listener; > struct vhost_vdpa_iova_range iova_range; > + /* VirtIO device features that can be emulated by qemu */ > + uint64_t added_features; Any reason we need a per vhost_vdpa storage for this? Or is there a chance that this field could be different among the devices? Thanks > uint64_t acked_features; > bool shadow_vqs_enabled; > /* IOVA mapping used by the Shadow Virtqueue */ > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > index 7468e44b87..ddb5e29288 100644 > --- a/hw/virtio/vhost-vdpa.c > +++ b/hw/virtio/vhost-vdpa.c > @@ -660,8 +660,8 @@ static int vhost_vdpa_set_features(struct vhost_dev *dev, > > v->acked_features = features; > > - /* We must not ack _F_LOG if SVQ is enabled */ > - features &= ~BIT_ULL(VHOST_F_LOG_ALL); > + /* Do not ack features emulated by qemu */ > + features &= ~v->added_features; > } > > trace_vhost_vdpa_set_features(dev, features); > @@ -1244,8 +1244,8 @@ static int vhost_vdpa_get_features(struct vhost_dev *dev, > int ret = vhost_vdpa_get_dev_features(dev, features); > > if (ret == 0 && v->shadow_vqs_enabled) { > - /* Add SVQ logging capabilities */ > - *features |= BIT_ULL(VHOST_F_LOG_ALL); > + /* Add emulated capabilities */ > + *features |= v->added_features; > } > > return ret; > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c > index eebf29f5c1..3803452800 100644 > --- a/net/vhost-vdpa.c > +++ b/net/vhost-vdpa.c > @@ -599,6 +599,10 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer, > s->vhost_vdpa.index = queue_pair_index; > s->vhost_vdpa.shadow_vqs_enabled = svq; > s->vhost_vdpa.iova_tree = iova_tree; > + if (svq) { > + /* Add SVQ logging capabilities */ > + s->vhost_vdpa.added_features |= BIT_ULL(VHOST_F_LOG_ALL); > + } > if (!is_datapath) { > s->cvq_cmd_out_buffer = qemu_memalign(qemu_real_host_page_size(), > vhost_vdpa_net_cvq_cmd_page_len()); > -- > 2.31.1 >
On Thu, Oct 20, 2022 at 6:23 AM Jason Wang <jasowang@redhat.com> wrote: > > On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <eperezma@redhat.com> wrote: > > > > At this moment only _F_LOG is added there. > > > > However future patches add features that depend on the kind of device. > > In particular, only net devices can add VIRTIO_F_GUEST_ANNOUNCE. So > > let's allow vhost_vdpa creator to set custom emulated device features. > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > --- > > include/hw/virtio/vhost-vdpa.h | 2 ++ > > hw/virtio/vhost-vdpa.c | 8 ++++---- > > net/vhost-vdpa.c | 4 ++++ > > 3 files changed, 10 insertions(+), 4 deletions(-) > > > > diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h > > index 1111d85643..50083e1e3b 100644 > > --- a/include/hw/virtio/vhost-vdpa.h > > +++ b/include/hw/virtio/vhost-vdpa.h > > @@ -31,6 +31,8 @@ typedef struct vhost_vdpa { > > bool iotlb_batch_begin_sent; > > MemoryListener listener; > > struct vhost_vdpa_iova_range iova_range; > > + /* VirtIO device features that can be emulated by qemu */ > > + uint64_t added_features; > > Any reason we need a per vhost_vdpa storage for this? Or is there a > chance that this field could be different among the devices? > Yes, one device could support SVQ and the other one could not support it because of different feature sets for example. Thanks! > Thanks > > > uint64_t acked_features; > > bool shadow_vqs_enabled; > > /* IOVA mapping used by the Shadow Virtqueue */ > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > > index 7468e44b87..ddb5e29288 100644 > > --- a/hw/virtio/vhost-vdpa.c > > +++ b/hw/virtio/vhost-vdpa.c > > @@ -660,8 +660,8 @@ static int vhost_vdpa_set_features(struct vhost_dev *dev, > > > > v->acked_features = features; > > > > - /* We must not ack _F_LOG if SVQ is enabled */ > > - features &= ~BIT_ULL(VHOST_F_LOG_ALL); > > + /* Do not ack features emulated by qemu */ > > + features &= ~v->added_features; > > } > > > > trace_vhost_vdpa_set_features(dev, features); > > @@ -1244,8 +1244,8 @@ static int vhost_vdpa_get_features(struct vhost_dev *dev, > > int ret = vhost_vdpa_get_dev_features(dev, features); > > > > if (ret == 0 && v->shadow_vqs_enabled) { > > - /* Add SVQ logging capabilities */ > > - *features |= BIT_ULL(VHOST_F_LOG_ALL); > > + /* Add emulated capabilities */ > > + *features |= v->added_features; > > } > > > > return ret; > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c > > index eebf29f5c1..3803452800 100644 > > --- a/net/vhost-vdpa.c > > +++ b/net/vhost-vdpa.c > > @@ -599,6 +599,10 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer, > > s->vhost_vdpa.index = queue_pair_index; > > s->vhost_vdpa.shadow_vqs_enabled = svq; > > s->vhost_vdpa.iova_tree = iova_tree; > > + if (svq) { > > + /* Add SVQ logging capabilities */ > > + s->vhost_vdpa.added_features |= BIT_ULL(VHOST_F_LOG_ALL); > > + } > > if (!is_datapath) { > > s->cvq_cmd_out_buffer = qemu_memalign(qemu_real_host_page_size(), > > vhost_vdpa_net_cvq_cmd_page_len()); > > -- > > 2.31.1 > > >
On Thu, Oct 20, 2022 at 2:34 PM Eugenio Perez Martin <eperezma@redhat.com> wrote: > > On Thu, Oct 20, 2022 at 6:23 AM Jason Wang <jasowang@redhat.com> wrote: > > > > On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <eperezma@redhat.com> wrote: > > > > > > At this moment only _F_LOG is added there. > > > > > > However future patches add features that depend on the kind of device. > > > In particular, only net devices can add VIRTIO_F_GUEST_ANNOUNCE. So > > > let's allow vhost_vdpa creator to set custom emulated device features. > > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > > --- > > > include/hw/virtio/vhost-vdpa.h | 2 ++ > > > hw/virtio/vhost-vdpa.c | 8 ++++---- > > > net/vhost-vdpa.c | 4 ++++ > > > 3 files changed, 10 insertions(+), 4 deletions(-) > > > > > > diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h > > > index 1111d85643..50083e1e3b 100644 > > > --- a/include/hw/virtio/vhost-vdpa.h > > > +++ b/include/hw/virtio/vhost-vdpa.h > > > @@ -31,6 +31,8 @@ typedef struct vhost_vdpa { > > > bool iotlb_batch_begin_sent; > > > MemoryListener listener; > > > struct vhost_vdpa_iova_range iova_range; > > > + /* VirtIO device features that can be emulated by qemu */ > > > + uint64_t added_features; > > > > Any reason we need a per vhost_vdpa storage for this? Or is there a > > chance that this field could be different among the devices? > > > > Yes, one device could support SVQ and the other one could not support > it because of different feature sets for example. Right, but for those devices that don't support SVQ, we don't even need mediation for feature like F_LOG and _F_STATUS? Thanks > > Thanks! > > > Thanks > > > > > uint64_t acked_features; > > > bool shadow_vqs_enabled; > > > /* IOVA mapping used by the Shadow Virtqueue */ > > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > > > index 7468e44b87..ddb5e29288 100644 > > > --- a/hw/virtio/vhost-vdpa.c > > > +++ b/hw/virtio/vhost-vdpa.c > > > @@ -660,8 +660,8 @@ static int vhost_vdpa_set_features(struct vhost_dev *dev, > > > > > > v->acked_features = features; > > > > > > - /* We must not ack _F_LOG if SVQ is enabled */ > > > - features &= ~BIT_ULL(VHOST_F_LOG_ALL); > > > + /* Do not ack features emulated by qemu */ > > > + features &= ~v->added_features; > > > } > > > > > > trace_vhost_vdpa_set_features(dev, features); > > > @@ -1244,8 +1244,8 @@ static int vhost_vdpa_get_features(struct vhost_dev *dev, > > > int ret = vhost_vdpa_get_dev_features(dev, features); > > > > > > if (ret == 0 && v->shadow_vqs_enabled) { > > > - /* Add SVQ logging capabilities */ > > > - *features |= BIT_ULL(VHOST_F_LOG_ALL); > > > + /* Add emulated capabilities */ > > > + *features |= v->added_features; > > > } > > > > > > return ret; > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c > > > index eebf29f5c1..3803452800 100644 > > > --- a/net/vhost-vdpa.c > > > +++ b/net/vhost-vdpa.c > > > @@ -599,6 +599,10 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer, > > > s->vhost_vdpa.index = queue_pair_index; > > > s->vhost_vdpa.shadow_vqs_enabled = svq; > > > s->vhost_vdpa.iova_tree = iova_tree; > > > + if (svq) { > > > + /* Add SVQ logging capabilities */ > > > + s->vhost_vdpa.added_features |= BIT_ULL(VHOST_F_LOG_ALL); > > > + } > > > if (!is_datapath) { > > > s->cvq_cmd_out_buffer = qemu_memalign(qemu_real_host_page_size(), > > > vhost_vdpa_net_cvq_cmd_page_len()); > > > -- > > > 2.31.1 > > > > > >
On Fri, Oct 21, 2022 at 4:57 AM Jason Wang <jasowang@redhat.com> wrote: > > On Thu, Oct 20, 2022 at 2:34 PM Eugenio Perez Martin > <eperezma@redhat.com> wrote: > > > > On Thu, Oct 20, 2022 at 6:23 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <eperezma@redhat.com> wrote: > > > > > > > > At this moment only _F_LOG is added there. > > > > > > > > However future patches add features that depend on the kind of device. > > > > In particular, only net devices can add VIRTIO_F_GUEST_ANNOUNCE. So > > > > let's allow vhost_vdpa creator to set custom emulated device features. > > > > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > > > --- > > > > include/hw/virtio/vhost-vdpa.h | 2 ++ > > > > hw/virtio/vhost-vdpa.c | 8 ++++---- > > > > net/vhost-vdpa.c | 4 ++++ > > > > 3 files changed, 10 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h > > > > index 1111d85643..50083e1e3b 100644 > > > > --- a/include/hw/virtio/vhost-vdpa.h > > > > +++ b/include/hw/virtio/vhost-vdpa.h > > > > @@ -31,6 +31,8 @@ typedef struct vhost_vdpa { > > > > bool iotlb_batch_begin_sent; > > > > MemoryListener listener; > > > > struct vhost_vdpa_iova_range iova_range; > > > > + /* VirtIO device features that can be emulated by qemu */ > > > > + uint64_t added_features; > > > > > > Any reason we need a per vhost_vdpa storage for this? Or is there a > > > chance that this field could be different among the devices? > > > > > > > Yes, one device could support SVQ and the other one could not support > > it because of different feature sets for example. > > Right, but for those devices that don't support SVQ, we don't even > need mediation for feature like F_LOG and _F_STATUS? > No, and we cannot offer it to the guest either. > Thanks > > > > > Thanks! > > > > > Thanks > > > > > > > uint64_t acked_features; > > > > bool shadow_vqs_enabled; > > > > /* IOVA mapping used by the Shadow Virtqueue */ > > > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > > > > index 7468e44b87..ddb5e29288 100644 > > > > --- a/hw/virtio/vhost-vdpa.c > > > > +++ b/hw/virtio/vhost-vdpa.c > > > > @@ -660,8 +660,8 @@ static int vhost_vdpa_set_features(struct vhost_dev *dev, > > > > > > > > v->acked_features = features; > > > > > > > > - /* We must not ack _F_LOG if SVQ is enabled */ > > > > - features &= ~BIT_ULL(VHOST_F_LOG_ALL); > > > > + /* Do not ack features emulated by qemu */ > > > > + features &= ~v->added_features; > > > > } > > > > > > > > trace_vhost_vdpa_set_features(dev, features); > > > > @@ -1244,8 +1244,8 @@ static int vhost_vdpa_get_features(struct vhost_dev *dev, > > > > int ret = vhost_vdpa_get_dev_features(dev, features); > > > > > > > > if (ret == 0 && v->shadow_vqs_enabled) { > > > > - /* Add SVQ logging capabilities */ > > > > - *features |= BIT_ULL(VHOST_F_LOG_ALL); > > > > + /* Add emulated capabilities */ > > > > + *features |= v->added_features; > > > > } > > > > > > > > return ret; > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c > > > > index eebf29f5c1..3803452800 100644 > > > > --- a/net/vhost-vdpa.c > > > > +++ b/net/vhost-vdpa.c > > > > @@ -599,6 +599,10 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer, > > > > s->vhost_vdpa.index = queue_pair_index; > > > > s->vhost_vdpa.shadow_vqs_enabled = svq; > > > > s->vhost_vdpa.iova_tree = iova_tree; > > > > + if (svq) { > > > > + /* Add SVQ logging capabilities */ > > > > + s->vhost_vdpa.added_features |= BIT_ULL(VHOST_F_LOG_ALL); > > > > + } > > > > if (!is_datapath) { > > > > s->cvq_cmd_out_buffer = qemu_memalign(qemu_real_host_page_size(), > > > > vhost_vdpa_net_cvq_cmd_page_len()); > > > > -- > > > > 2.31.1 > > > > > > > > > >
On Fri, Oct 21, 2022 at 4:56 PM Eugenio Perez Martin <eperezma@redhat.com> wrote: > > On Fri, Oct 21, 2022 at 4:57 AM Jason Wang <jasowang@redhat.com> wrote: > > > > On Thu, Oct 20, 2022 at 2:34 PM Eugenio Perez Martin > > <eperezma@redhat.com> wrote: > > > > > > On Thu, Oct 20, 2022 at 6:23 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <eperezma@redhat.com> wrote: > > > > > > > > > > At this moment only _F_LOG is added there. > > > > > > > > > > However future patches add features that depend on the kind of device. > > > > > In particular, only net devices can add VIRTIO_F_GUEST_ANNOUNCE. So > > > > > let's allow vhost_vdpa creator to set custom emulated device features. > > > > > > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > > > > --- > > > > > include/hw/virtio/vhost-vdpa.h | 2 ++ > > > > > hw/virtio/vhost-vdpa.c | 8 ++++---- > > > > > net/vhost-vdpa.c | 4 ++++ > > > > > 3 files changed, 10 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h > > > > > index 1111d85643..50083e1e3b 100644 > > > > > --- a/include/hw/virtio/vhost-vdpa.h > > > > > +++ b/include/hw/virtio/vhost-vdpa.h > > > > > @@ -31,6 +31,8 @@ typedef struct vhost_vdpa { > > > > > bool iotlb_batch_begin_sent; > > > > > MemoryListener listener; > > > > > struct vhost_vdpa_iova_range iova_range; > > > > > + /* VirtIO device features that can be emulated by qemu */ > > > > > + uint64_t added_features; > > > > > > > > Any reason we need a per vhost_vdpa storage for this? Or is there a > > > > chance that this field could be different among the devices? > > > > > > > > > > Yes, one device could support SVQ and the other one could not support > > > it because of different feature sets for example. > > > > Right, but for those devices that don't support SVQ, we don't even > > need mediation for feature like F_LOG and _F_STATUS? > > > > No, and we cannot offer it to the guest either. Just to make sure we are on the same page, what I meant is, consider in the future SVQ get the support of all features, so we can remove this field? This is because _F_STATUS can be mediated unconditionally anyhow. Thanks > > > Thanks > > > > > > > > Thanks! > > > > > > > Thanks > > > > > > > > > uint64_t acked_features; > > > > > bool shadow_vqs_enabled; > > > > > /* IOVA mapping used by the Shadow Virtqueue */ > > > > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > > > > > index 7468e44b87..ddb5e29288 100644 > > > > > --- a/hw/virtio/vhost-vdpa.c > > > > > +++ b/hw/virtio/vhost-vdpa.c > > > > > @@ -660,8 +660,8 @@ static int vhost_vdpa_set_features(struct vhost_dev *dev, > > > > > > > > > > v->acked_features = features; > > > > > > > > > > - /* We must not ack _F_LOG if SVQ is enabled */ > > > > > - features &= ~BIT_ULL(VHOST_F_LOG_ALL); > > > > > + /* Do not ack features emulated by qemu */ > > > > > + features &= ~v->added_features; > > > > > } > > > > > > > > > > trace_vhost_vdpa_set_features(dev, features); > > > > > @@ -1244,8 +1244,8 @@ static int vhost_vdpa_get_features(struct vhost_dev *dev, > > > > > int ret = vhost_vdpa_get_dev_features(dev, features); > > > > > > > > > > if (ret == 0 && v->shadow_vqs_enabled) { > > > > > - /* Add SVQ logging capabilities */ > > > > > - *features |= BIT_ULL(VHOST_F_LOG_ALL); > > > > > + /* Add emulated capabilities */ > > > > > + *features |= v->added_features; > > > > > } > > > > > > > > > > return ret; > > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c > > > > > index eebf29f5c1..3803452800 100644 > > > > > --- a/net/vhost-vdpa.c > > > > > +++ b/net/vhost-vdpa.c > > > > > @@ -599,6 +599,10 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer, > > > > > s->vhost_vdpa.index = queue_pair_index; > > > > > s->vhost_vdpa.shadow_vqs_enabled = svq; > > > > > s->vhost_vdpa.iova_tree = iova_tree; > > > > > + if (svq) { > > > > > + /* Add SVQ logging capabilities */ > > > > > + s->vhost_vdpa.added_features |= BIT_ULL(VHOST_F_LOG_ALL); > > > > > + } > > > > > if (!is_datapath) { > > > > > s->cvq_cmd_out_buffer = qemu_memalign(qemu_real_host_page_size(), > > > > > vhost_vdpa_net_cvq_cmd_page_len()); > > > > > -- > > > > > 2.31.1 > > > > > > > > > > > > > > >
On Mon, Oct 24, 2022 at 4:14 AM Jason Wang <jasowang@redhat.com> wrote: > > On Fri, Oct 21, 2022 at 4:56 PM Eugenio Perez Martin > <eperezma@redhat.com> wrote: > > > > On Fri, Oct 21, 2022 at 4:57 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > On Thu, Oct 20, 2022 at 2:34 PM Eugenio Perez Martin > > > <eperezma@redhat.com> wrote: > > > > > > > > On Thu, Oct 20, 2022 at 6:23 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <eperezma@redhat.com> wrote: > > > > > > > > > > > > At this moment only _F_LOG is added there. > > > > > > > > > > > > However future patches add features that depend on the kind of device. > > > > > > In particular, only net devices can add VIRTIO_F_GUEST_ANNOUNCE. So > > > > > > let's allow vhost_vdpa creator to set custom emulated device features. > > > > > > > > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > > > > > --- > > > > > > include/hw/virtio/vhost-vdpa.h | 2 ++ > > > > > > hw/virtio/vhost-vdpa.c | 8 ++++---- > > > > > > net/vhost-vdpa.c | 4 ++++ > > > > > > 3 files changed, 10 insertions(+), 4 deletions(-) > > > > > > > > > > > > diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h > > > > > > index 1111d85643..50083e1e3b 100644 > > > > > > --- a/include/hw/virtio/vhost-vdpa.h > > > > > > +++ b/include/hw/virtio/vhost-vdpa.h > > > > > > @@ -31,6 +31,8 @@ typedef struct vhost_vdpa { > > > > > > bool iotlb_batch_begin_sent; > > > > > > MemoryListener listener; > > > > > > struct vhost_vdpa_iova_range iova_range; > > > > > > + /* VirtIO device features that can be emulated by qemu */ > > > > > > + uint64_t added_features; > > > > > > > > > > Any reason we need a per vhost_vdpa storage for this? Or is there a > > > > > chance that this field could be different among the devices? > > > > > > > > > > > > > Yes, one device could support SVQ and the other one could not support > > > > it because of different feature sets for example. > > > > > > Right, but for those devices that don't support SVQ, we don't even > > > need mediation for feature like F_LOG and _F_STATUS? > > > > > > > No, and we cannot offer it to the guest either. > > Just to make sure we are on the same page, what I meant is, consider > in the future SVQ get the support of all features, so we can remove > this field? This is because _F_STATUS can be mediated unconditionally > anyhow. > For _F_STATUS that is right. But we cannot handle full _F_GUEST_ANNOUNCE since control SVQ (will) needs features from the device that cannot be emulated, like ASID. I think your point is "Since qemu cannot migrate these devices it will never set VIRTIO_NET_S_ANNOUNCE, so the guest will never send VIRTIO_NET_CTRL_ANNOUNCE messages". And I think that is totally right, but I still feel it is weird to expose it if we cannot handle it. Maybe a good first step is to move added_features to vhost_net, or maybe to convert it to "bool guest_announce_emulated" or something similar? This way hw/virtio/vhost-vdpa is totally unaware of this and changes are more self contained. Thanks! > Thanks > > > > > > Thanks > > > > > > > > > > > Thanks! > > > > > > > > > Thanks > > > > > > > > > > > uint64_t acked_features; > > > > > > bool shadow_vqs_enabled; > > > > > > /* IOVA mapping used by the Shadow Virtqueue */ > > > > > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > > > > > > index 7468e44b87..ddb5e29288 100644 > > > > > > --- a/hw/virtio/vhost-vdpa.c > > > > > > +++ b/hw/virtio/vhost-vdpa.c > > > > > > @@ -660,8 +660,8 @@ static int vhost_vdpa_set_features(struct vhost_dev *dev, > > > > > > > > > > > > v->acked_features = features; > > > > > > > > > > > > - /* We must not ack _F_LOG if SVQ is enabled */ > > > > > > - features &= ~BIT_ULL(VHOST_F_LOG_ALL); > > > > > > + /* Do not ack features emulated by qemu */ > > > > > > + features &= ~v->added_features; > > > > > > } > > > > > > > > > > > > trace_vhost_vdpa_set_features(dev, features); > > > > > > @@ -1244,8 +1244,8 @@ static int vhost_vdpa_get_features(struct vhost_dev *dev, > > > > > > int ret = vhost_vdpa_get_dev_features(dev, features); > > > > > > > > > > > > if (ret == 0 && v->shadow_vqs_enabled) { > > > > > > - /* Add SVQ logging capabilities */ > > > > > > - *features |= BIT_ULL(VHOST_F_LOG_ALL); > > > > > > + /* Add emulated capabilities */ > > > > > > + *features |= v->added_features; > > > > > > } > > > > > > > > > > > > return ret; > > > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c > > > > > > index eebf29f5c1..3803452800 100644 > > > > > > --- a/net/vhost-vdpa.c > > > > > > +++ b/net/vhost-vdpa.c > > > > > > @@ -599,6 +599,10 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer, > > > > > > s->vhost_vdpa.index = queue_pair_index; > > > > > > s->vhost_vdpa.shadow_vqs_enabled = svq; > > > > > > s->vhost_vdpa.iova_tree = iova_tree; > > > > > > + if (svq) { > > > > > > + /* Add SVQ logging capabilities */ > > > > > > + s->vhost_vdpa.added_features |= BIT_ULL(VHOST_F_LOG_ALL); > > > > > > + } > > > > > > if (!is_datapath) { > > > > > > s->cvq_cmd_out_buffer = qemu_memalign(qemu_real_host_page_size(), > > > > > > vhost_vdpa_net_cvq_cmd_page_len()); > > > > > > -- > > > > > > 2.31.1 > > > > > > > > > > > > > > > > > > > > >
On Mon, Oct 24, 2022 at 5:26 PM Eugenio Perez Martin <eperezma@redhat.com> wrote: > > On Mon, Oct 24, 2022 at 4:14 AM Jason Wang <jasowang@redhat.com> wrote: > > > > On Fri, Oct 21, 2022 at 4:56 PM Eugenio Perez Martin > > <eperezma@redhat.com> wrote: > > > > > > On Fri, Oct 21, 2022 at 4:57 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > On Thu, Oct 20, 2022 at 2:34 PM Eugenio Perez Martin > > > > <eperezma@redhat.com> wrote: > > > > > > > > > > On Thu, Oct 20, 2022 at 6:23 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > > > On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <eperezma@redhat.com> wrote: > > > > > > > > > > > > > > At this moment only _F_LOG is added there. > > > > > > > > > > > > > > However future patches add features that depend on the kind of device. > > > > > > > In particular, only net devices can add VIRTIO_F_GUEST_ANNOUNCE. So > > > > > > > let's allow vhost_vdpa creator to set custom emulated device features. > > > > > > > > > > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > > > > > > --- > > > > > > > include/hw/virtio/vhost-vdpa.h | 2 ++ > > > > > > > hw/virtio/vhost-vdpa.c | 8 ++++---- > > > > > > > net/vhost-vdpa.c | 4 ++++ > > > > > > > 3 files changed, 10 insertions(+), 4 deletions(-) > > > > > > > > > > > > > > diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h > > > > > > > index 1111d85643..50083e1e3b 100644 > > > > > > > --- a/include/hw/virtio/vhost-vdpa.h > > > > > > > +++ b/include/hw/virtio/vhost-vdpa.h > > > > > > > @@ -31,6 +31,8 @@ typedef struct vhost_vdpa { > > > > > > > bool iotlb_batch_begin_sent; > > > > > > > MemoryListener listener; > > > > > > > struct vhost_vdpa_iova_range iova_range; > > > > > > > + /* VirtIO device features that can be emulated by qemu */ > > > > > > > + uint64_t added_features; > > > > > > > > > > > > Any reason we need a per vhost_vdpa storage for this? Or is there a > > > > > > chance that this field could be different among the devices? > > > > > > > > > > > > > > > > Yes, one device could support SVQ and the other one could not support > > > > > it because of different feature sets for example. > > > > > > > > Right, but for those devices that don't support SVQ, we don't even > > > > need mediation for feature like F_LOG and _F_STATUS? > > > > > > > > > > No, and we cannot offer it to the guest either. > > > > Just to make sure we are on the same page, what I meant is, consider > > in the future SVQ get the support of all features, so we can remove > > this field? This is because _F_STATUS can be mediated unconditionally > > anyhow. > > > > For _F_STATUS that is right. But we cannot handle full > _F_GUEST_ANNOUNCE since control SVQ (will) needs features from the > device that cannot be emulated, like ASID. > > I think your point is "Since qemu cannot migrate these devices it will > never set VIRTIO_NET_S_ANNOUNCE, so the guest will never send > VIRTIO_NET_CTRL_ANNOUNCE messages". And I think that is totally right, > but I still feel it is weird to expose it if we cannot handle it. > > Maybe a good first step is to move added_features to vhost_net, or > maybe to convert it to "bool guest_announce_emulated" or something > similar? This way hw/virtio/vhost-vdpa is totally unaware of this and > changes are more self contained. This reminds me of something. For vhost, if Qemu can handle some feature bits, we don't need to validate if vhost has such support. E.g we don't have _F_SATAUS and _F_GUEST_ANNOUNCE in kernel_feature_bits. I wonder if we can do something similar for vhost-vDPA? Then we don't need to bother new fields. Thanks > > Thanks! > > > > > Thanks > > > > > > > > > Thanks > > > > > > > > > > > > > > Thanks! > > > > > > > > > > > Thanks > > > > > > > > > > > > > uint64_t acked_features; > > > > > > > bool shadow_vqs_enabled; > > > > > > > /* IOVA mapping used by the Shadow Virtqueue */ > > > > > > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > > > > > > > index 7468e44b87..ddb5e29288 100644 > > > > > > > --- a/hw/virtio/vhost-vdpa.c > > > > > > > +++ b/hw/virtio/vhost-vdpa.c > > > > > > > @@ -660,8 +660,8 @@ static int vhost_vdpa_set_features(struct vhost_dev *dev, > > > > > > > > > > > > > > v->acked_features = features; > > > > > > > > > > > > > > - /* We must not ack _F_LOG if SVQ is enabled */ > > > > > > > - features &= ~BIT_ULL(VHOST_F_LOG_ALL); > > > > > > > + /* Do not ack features emulated by qemu */ > > > > > > > + features &= ~v->added_features; > > > > > > > } > > > > > > > > > > > > > > trace_vhost_vdpa_set_features(dev, features); > > > > > > > @@ -1244,8 +1244,8 @@ static int vhost_vdpa_get_features(struct vhost_dev *dev, > > > > > > > int ret = vhost_vdpa_get_dev_features(dev, features); > > > > > > > > > > > > > > if (ret == 0 && v->shadow_vqs_enabled) { > > > > > > > - /* Add SVQ logging capabilities */ > > > > > > > - *features |= BIT_ULL(VHOST_F_LOG_ALL); > > > > > > > + /* Add emulated capabilities */ > > > > > > > + *features |= v->added_features; > > > > > > > } > > > > > > > > > > > > > > return ret; > > > > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c > > > > > > > index eebf29f5c1..3803452800 100644 > > > > > > > --- a/net/vhost-vdpa.c > > > > > > > +++ b/net/vhost-vdpa.c > > > > > > > @@ -599,6 +599,10 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer, > > > > > > > s->vhost_vdpa.index = queue_pair_index; > > > > > > > s->vhost_vdpa.shadow_vqs_enabled = svq; > > > > > > > s->vhost_vdpa.iova_tree = iova_tree; > > > > > > > + if (svq) { > > > > > > > + /* Add SVQ logging capabilities */ > > > > > > > + s->vhost_vdpa.added_features |= BIT_ULL(VHOST_F_LOG_ALL); > > > > > > > + } > > > > > > > if (!is_datapath) { > > > > > > > s->cvq_cmd_out_buffer = qemu_memalign(qemu_real_host_page_size(), > > > > > > > vhost_vdpa_net_cvq_cmd_page_len()); > > > > > > > -- > > > > > > > 2.31.1 > > > > > > > > > > > > > > > > > > > > > > > > > > > >
On Tue, Oct 25, 2022 at 4:45 AM Jason Wang <jasowang@redhat.com> wrote: > > On Mon, Oct 24, 2022 at 5:26 PM Eugenio Perez Martin > <eperezma@redhat.com> wrote: > > > > On Mon, Oct 24, 2022 at 4:14 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > On Fri, Oct 21, 2022 at 4:56 PM Eugenio Perez Martin > > > <eperezma@redhat.com> wrote: > > > > > > > > On Fri, Oct 21, 2022 at 4:57 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > On Thu, Oct 20, 2022 at 2:34 PM Eugenio Perez Martin > > > > > <eperezma@redhat.com> wrote: > > > > > > > > > > > > On Thu, Oct 20, 2022 at 6:23 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > > > > > On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <eperezma@redhat.com> wrote: > > > > > > > > > > > > > > > > At this moment only _F_LOG is added there. > > > > > > > > > > > > > > > > However future patches add features that depend on the kind of device. > > > > > > > > In particular, only net devices can add VIRTIO_F_GUEST_ANNOUNCE. So > > > > > > > > let's allow vhost_vdpa creator to set custom emulated device features. > > > > > > > > > > > > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > > > > > > > --- > > > > > > > > include/hw/virtio/vhost-vdpa.h | 2 ++ > > > > > > > > hw/virtio/vhost-vdpa.c | 8 ++++---- > > > > > > > > net/vhost-vdpa.c | 4 ++++ > > > > > > > > 3 files changed, 10 insertions(+), 4 deletions(-) > > > > > > > > > > > > > > > > diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h > > > > > > > > index 1111d85643..50083e1e3b 100644 > > > > > > > > --- a/include/hw/virtio/vhost-vdpa.h > > > > > > > > +++ b/include/hw/virtio/vhost-vdpa.h > > > > > > > > @@ -31,6 +31,8 @@ typedef struct vhost_vdpa { > > > > > > > > bool iotlb_batch_begin_sent; > > > > > > > > MemoryListener listener; > > > > > > > > struct vhost_vdpa_iova_range iova_range; > > > > > > > > + /* VirtIO device features that can be emulated by qemu */ > > > > > > > > + uint64_t added_features; > > > > > > > > > > > > > > Any reason we need a per vhost_vdpa storage for this? Or is there a > > > > > > > chance that this field could be different among the devices? > > > > > > > > > > > > > > > > > > > Yes, one device could support SVQ and the other one could not support > > > > > > it because of different feature sets for example. > > > > > > > > > > Right, but for those devices that don't support SVQ, we don't even > > > > > need mediation for feature like F_LOG and _F_STATUS? > > > > > > > > > > > > > No, and we cannot offer it to the guest either. > > > > > > Just to make sure we are on the same page, what I meant is, consider > > > in the future SVQ get the support of all features, so we can remove > > > this field? This is because _F_STATUS can be mediated unconditionally > > > anyhow. > > > > > > > For _F_STATUS that is right. But we cannot handle full > > _F_GUEST_ANNOUNCE since control SVQ (will) needs features from the > > device that cannot be emulated, like ASID. > > > > I think your point is "Since qemu cannot migrate these devices it will > > never set VIRTIO_NET_S_ANNOUNCE, so the guest will never send > > VIRTIO_NET_CTRL_ANNOUNCE messages". And I think that is totally right, > > but I still feel it is weird to expose it if we cannot handle it. > > > > Maybe a good first step is to move added_features to vhost_net, or > > maybe to convert it to "bool guest_announce_emulated" or something > > similar? This way hw/virtio/vhost-vdpa is totally unaware of this and > > changes are more self contained. > > This reminds me of something. For vhost, if Qemu can handle some > feature bits, we don't need to validate if vhost has such support. > > E.g we don't have _F_SATAUS and _F_GUEST_ANNOUNCE in kernel_feature_bits. > > I wonder if we can do something similar for vhost-vDPA? Then we don't > need to bother new fields. > That is valid for _F_GUEST_ANNOUNCE, because all of it is emulated and we must forbid the ack of it for simplicity. But _F_STATUS has two parts, one bit that must be exposed to the guest from the physical NIC, since qemu cannot know when to change it (VIRTIO_NET_S_LINK_UP) and another one that must be 100% emulated (VIRTIO_NET_S_ANNOUNCE). VIRTIO_NET_F_STATUS is 100% emulated only if it is not offered by the device. I'm moving to vhost_net for the next series, and I'll try to make all of this more explicit. Thanks!
diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h index 1111d85643..50083e1e3b 100644 --- a/include/hw/virtio/vhost-vdpa.h +++ b/include/hw/virtio/vhost-vdpa.h @@ -31,6 +31,8 @@ typedef struct vhost_vdpa { bool iotlb_batch_begin_sent; MemoryListener listener; struct vhost_vdpa_iova_range iova_range; + /* VirtIO device features that can be emulated by qemu */ + uint64_t added_features; uint64_t acked_features; bool shadow_vqs_enabled; /* IOVA mapping used by the Shadow Virtqueue */ diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c index 7468e44b87..ddb5e29288 100644 --- a/hw/virtio/vhost-vdpa.c +++ b/hw/virtio/vhost-vdpa.c @@ -660,8 +660,8 @@ static int vhost_vdpa_set_features(struct vhost_dev *dev, v->acked_features = features; - /* We must not ack _F_LOG if SVQ is enabled */ - features &= ~BIT_ULL(VHOST_F_LOG_ALL); + /* Do not ack features emulated by qemu */ + features &= ~v->added_features; } trace_vhost_vdpa_set_features(dev, features); @@ -1244,8 +1244,8 @@ static int vhost_vdpa_get_features(struct vhost_dev *dev, int ret = vhost_vdpa_get_dev_features(dev, features); if (ret == 0 && v->shadow_vqs_enabled) { - /* Add SVQ logging capabilities */ - *features |= BIT_ULL(VHOST_F_LOG_ALL); + /* Add emulated capabilities */ + *features |= v->added_features; } return ret; diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c index eebf29f5c1..3803452800 100644 --- a/net/vhost-vdpa.c +++ b/net/vhost-vdpa.c @@ -599,6 +599,10 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer, s->vhost_vdpa.index = queue_pair_index; s->vhost_vdpa.shadow_vqs_enabled = svq; s->vhost_vdpa.iova_tree = iova_tree; + if (svq) { + /* Add SVQ logging capabilities */ + s->vhost_vdpa.added_features |= BIT_ULL(VHOST_F_LOG_ALL); + } if (!is_datapath) { s->cvq_cmd_out_buffer = qemu_memalign(qemu_real_host_page_size(), vhost_vdpa_net_cvq_cmd_page_len());
At this moment only _F_LOG is added there. However future patches add features that depend on the kind of device. In particular, only net devices can add VIRTIO_F_GUEST_ANNOUNCE. So let's allow vhost_vdpa creator to set custom emulated device features. Signed-off-by: Eugenio Pérez <eperezma@redhat.com> --- include/hw/virtio/vhost-vdpa.h | 2 ++ hw/virtio/vhost-vdpa.c | 8 ++++---- net/vhost-vdpa.c | 4 ++++ 3 files changed, 10 insertions(+), 4 deletions(-)