Re: [RFC PATCH v5 21/26] vhost: Add vhost_svq_valid_guest_features to shadow vq
On Wed, Nov 3, 2021 at 3:44 PM Eugenio Perez Martin wrote:
>
> On Wed, Nov 3, 2021 at 4:18 AM Jason Wang wrote:
> >
> > On Tue, Nov 2, 2021 at 4:10 PM Eugenio Perez Martin
> > wrote:
> > >
> > > On Tue, Nov 2, 2021 at 6:26 AM Jason Wang wrote:
> > > >
> > > > On Sat, Oct 30, 2021 at 2:44 AM Eugenio Pérez
> > > > wrote:
> > > > >
> > > > > This allows it to test if the guest has aknowledge an invalid
> > > > > transport
> > > > > feature for SVQ. This will include packed vq layout or event_idx,
> > > > > where VirtIO device needs help from SVQ.
> > > > >
> > > > > There is not needed at this moment, but since SVQ will not
> > > > > re-negotiate
> > > > > features again with the guest, a failure in acknowledge them is fatal
> > > > > for SVQ.
> > > > >
> > > >
> > > > It's not clear to me why we need this. Maybe you can give me an
> > > > example. E.g isn't it sufficient to filter out the device with
> > > > event_idx?
> > > >
> > >
> > > If the guest did negotiate _F_EVENT_IDX, it expects to be notified
> > > only when device marks as used a specific number of descriptors.
> > >
> > > If we use VirtQueue notification, the VirtQueue code handles it
> > > transparently. But if we want to be able to change the guest VQ's
> > > call_fd, we cannot use VirtQueue's, so this needs to be handled by SVQ
> > > code. And that is still not implemented.
> > >
> > > Of course in the event_idx case we could just ignore it and notify in
> > > all used descriptors, but it seems not polite to me :). I will develop
> > > event_idx on top of this, either exposing the needed pieces in
> > > VirtQueue (I prefer this) or rolling our own in SVQ.
> >
> > Yes, but what I meant is, we can fail the SVQ enabling if the device
> > supports event_idx. Then we're sure guests won't negotiate event_idx.
> >
>
> We can go that way for sure, but then we leave out the scenario where
> the device supports event_idx but the guest has not acked it. This is
> a valid scenario for SVQ to work in.
If SVQ supports event idx in the future, we can remove it from the
blacklist. But I think it should be simpler to let SVQ use the same
features as guests. So in this case SVQ won't use the event index.
Thanks
>
> Thanks!
>
> > Thanks
> >
> > >
> > > Same reasoning can be applied to unknown transport features.
> > >
> > > Thanks!
> > >
> > > > Thanks
> > > >
> > > > > Signed-off-by: Eugenio Pérez
> > > > > ---
> > > > > hw/virtio/vhost-shadow-virtqueue.h | 1 +
> > > > > hw/virtio/vhost-shadow-virtqueue.c | 6 ++
> > > > > 2 files changed, 7 insertions(+)
> > > > >
> > > > > diff --git a/hw/virtio/vhost-shadow-virtqueue.h
> > > > > b/hw/virtio/vhost-shadow-virtqueue.h
> > > > > index 946b2c6295..ac55588009 100644
> > > > > --- a/hw/virtio/vhost-shadow-virtqueue.h
> > > > > +++ b/hw/virtio/vhost-shadow-virtqueue.h
> > > > > @@ -16,6 +16,7 @@
> > > > > typedef struct VhostShadowVirtqueue VhostShadowVirtqueue;
> > > > >
> > > > > bool vhost_svq_valid_device_features(uint64_t *features);
> > > > > +bool vhost_svq_valid_guest_features(uint64_t *features);
> > > > >
> > > > > void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int
> > > > > svq_kick_fd);
> > > > > void vhost_svq_set_guest_call_notifier(VhostShadowVirtqueue *svq,
> > > > > int call_fd);
> > > > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c
> > > > > b/hw/virtio/vhost-shadow-virtqueue.c
> > > > > index 6e0508a231..cb9ffcb015 100644
> > > > > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > > > > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > > > > @@ -62,6 +62,12 @@ bool vhost_svq_valid_device_features(uint64_t
> > > > > *dev_features)
> > > > > return true;
> > > > > }
> > > > >
> > > > > +/* If the guest is using some of these, SVQ cannot communicate */
> > > > > +bool vhost_svq_valid_guest_features(uint64_t *guest_features)
> > > > > +{
> > > > > +return true;
> > > > > +}
> > > > > +
> > > > > /* Forward guest notifications */
> > > > > static void vhost_handle_guest_kick(EventNotifier *n)
> > > > > {
> > > > > --
> > > > > 2.27.0
> > > > >
> > > >
> > >
> >
>
Re: [RFC PATCH v5 21/26] vhost: Add vhost_svq_valid_guest_features to shadow vq
On Wed, Nov 3, 2021 at 4:18 AM Jason Wang wrote:
>
> On Tue, Nov 2, 2021 at 4:10 PM Eugenio Perez Martin
> wrote:
> >
> > On Tue, Nov 2, 2021 at 6:26 AM Jason Wang wrote:
> > >
> > > On Sat, Oct 30, 2021 at 2:44 AM Eugenio Pérez wrote:
> > > >
> > > > This allows it to test if the guest has aknowledge an invalid transport
> > > > feature for SVQ. This will include packed vq layout or event_idx,
> > > > where VirtIO device needs help from SVQ.
> > > >
> > > > There is not needed at this moment, but since SVQ will not re-negotiate
> > > > features again with the guest, a failure in acknowledge them is fatal
> > > > for SVQ.
> > > >
> > >
> > > It's not clear to me why we need this. Maybe you can give me an
> > > example. E.g isn't it sufficient to filter out the device with
> > > event_idx?
> > >
> >
> > If the guest did negotiate _F_EVENT_IDX, it expects to be notified
> > only when device marks as used a specific number of descriptors.
> >
> > If we use VirtQueue notification, the VirtQueue code handles it
> > transparently. But if we want to be able to change the guest VQ's
> > call_fd, we cannot use VirtQueue's, so this needs to be handled by SVQ
> > code. And that is still not implemented.
> >
> > Of course in the event_idx case we could just ignore it and notify in
> > all used descriptors, but it seems not polite to me :). I will develop
> > event_idx on top of this, either exposing the needed pieces in
> > VirtQueue (I prefer this) or rolling our own in SVQ.
>
> Yes, but what I meant is, we can fail the SVQ enabling if the device
> supports event_idx. Then we're sure guests won't negotiate event_idx.
>
We can go that way for sure, but then we leave out the scenario where
the device supports event_idx but the guest has not acked it. This is
a valid scenario for SVQ to work in.
Thanks!
> Thanks
>
> >
> > Same reasoning can be applied to unknown transport features.
> >
> > Thanks!
> >
> > > Thanks
> > >
> > > > Signed-off-by: Eugenio Pérez
> > > > ---
> > > > hw/virtio/vhost-shadow-virtqueue.h | 1 +
> > > > hw/virtio/vhost-shadow-virtqueue.c | 6 ++
> > > > 2 files changed, 7 insertions(+)
> > > >
> > > > diff --git a/hw/virtio/vhost-shadow-virtqueue.h
> > > > b/hw/virtio/vhost-shadow-virtqueue.h
> > > > index 946b2c6295..ac55588009 100644
> > > > --- a/hw/virtio/vhost-shadow-virtqueue.h
> > > > +++ b/hw/virtio/vhost-shadow-virtqueue.h
> > > > @@ -16,6 +16,7 @@
> > > > typedef struct VhostShadowVirtqueue VhostShadowVirtqueue;
> > > >
> > > > bool vhost_svq_valid_device_features(uint64_t *features);
> > > > +bool vhost_svq_valid_guest_features(uint64_t *features);
> > > >
> > > > void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int
> > > > svq_kick_fd);
> > > > void vhost_svq_set_guest_call_notifier(VhostShadowVirtqueue *svq, int
> > > > call_fd);
> > > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c
> > > > b/hw/virtio/vhost-shadow-virtqueue.c
> > > > index 6e0508a231..cb9ffcb015 100644
> > > > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > > > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > > > @@ -62,6 +62,12 @@ bool vhost_svq_valid_device_features(uint64_t
> > > > *dev_features)
> > > > return true;
> > > > }
> > > >
> > > > +/* If the guest is using some of these, SVQ cannot communicate */
> > > > +bool vhost_svq_valid_guest_features(uint64_t *guest_features)
> > > > +{
> > > > +return true;
> > > > +}
> > > > +
> > > > /* Forward guest notifications */
> > > > static void vhost_handle_guest_kick(EventNotifier *n)
> > > > {
> > > > --
> > > > 2.27.0
> > > >
> > >
> >
>
Re: [RFC PATCH v5 21/26] vhost: Add vhost_svq_valid_guest_features to shadow vq
On Tue, Nov 2, 2021 at 4:10 PM Eugenio Perez Martin wrote:
>
> On Tue, Nov 2, 2021 at 6:26 AM Jason Wang wrote:
> >
> > On Sat, Oct 30, 2021 at 2:44 AM Eugenio Pérez wrote:
> > >
> > > This allows it to test if the guest has aknowledge an invalid transport
> > > feature for SVQ. This will include packed vq layout or event_idx,
> > > where VirtIO device needs help from SVQ.
> > >
> > > There is not needed at this moment, but since SVQ will not re-negotiate
> > > features again with the guest, a failure in acknowledge them is fatal
> > > for SVQ.
> > >
> >
> > It's not clear to me why we need this. Maybe you can give me an
> > example. E.g isn't it sufficient to filter out the device with
> > event_idx?
> >
>
> If the guest did negotiate _F_EVENT_IDX, it expects to be notified
> only when device marks as used a specific number of descriptors.
>
> If we use VirtQueue notification, the VirtQueue code handles it
> transparently. But if we want to be able to change the guest VQ's
> call_fd, we cannot use VirtQueue's, so this needs to be handled by SVQ
> code. And that is still not implemented.
>
> Of course in the event_idx case we could just ignore it and notify in
> all used descriptors, but it seems not polite to me :). I will develop
> event_idx on top of this, either exposing the needed pieces in
> VirtQueue (I prefer this) or rolling our own in SVQ.
Yes, but what I meant is, we can fail the SVQ enabling if the device
supports event_idx. Then we're sure guests won't negotiate event_idx.
Thanks
>
> Same reasoning can be applied to unknown transport features.
>
> Thanks!
>
> > Thanks
> >
> > > Signed-off-by: Eugenio Pérez
> > > ---
> > > hw/virtio/vhost-shadow-virtqueue.h | 1 +
> > > hw/virtio/vhost-shadow-virtqueue.c | 6 ++
> > > 2 files changed, 7 insertions(+)
> > >
> > > diff --git a/hw/virtio/vhost-shadow-virtqueue.h
> > > b/hw/virtio/vhost-shadow-virtqueue.h
> > > index 946b2c6295..ac55588009 100644
> > > --- a/hw/virtio/vhost-shadow-virtqueue.h
> > > +++ b/hw/virtio/vhost-shadow-virtqueue.h
> > > @@ -16,6 +16,7 @@
> > > typedef struct VhostShadowVirtqueue VhostShadowVirtqueue;
> > >
> > > bool vhost_svq_valid_device_features(uint64_t *features);
> > > +bool vhost_svq_valid_guest_features(uint64_t *features);
> > >
> > > void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int
> > > svq_kick_fd);
> > > void vhost_svq_set_guest_call_notifier(VhostShadowVirtqueue *svq, int
> > > call_fd);
> > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c
> > > b/hw/virtio/vhost-shadow-virtqueue.c
> > > index 6e0508a231..cb9ffcb015 100644
> > > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > > @@ -62,6 +62,12 @@ bool vhost_svq_valid_device_features(uint64_t
> > > *dev_features)
> > > return true;
> > > }
> > >
> > > +/* If the guest is using some of these, SVQ cannot communicate */
> > > +bool vhost_svq_valid_guest_features(uint64_t *guest_features)
> > > +{
> > > +return true;
> > > +}
> > > +
> > > /* Forward guest notifications */
> > > static void vhost_handle_guest_kick(EventNotifier *n)
> > > {
> > > --
> > > 2.27.0
> > >
> >
>
Re: [RFC PATCH v5 21/26] vhost: Add vhost_svq_valid_guest_features to shadow vq
On Tue, Nov 2, 2021 at 6:26 AM Jason Wang wrote:
>
> On Sat, Oct 30, 2021 at 2:44 AM Eugenio Pérez wrote:
> >
> > This allows it to test if the guest has aknowledge an invalid transport
> > feature for SVQ. This will include packed vq layout or event_idx,
> > where VirtIO device needs help from SVQ.
> >
> > There is not needed at this moment, but since SVQ will not re-negotiate
> > features again with the guest, a failure in acknowledge them is fatal
> > for SVQ.
> >
>
> It's not clear to me why we need this. Maybe you can give me an
> example. E.g isn't it sufficient to filter out the device with
> event_idx?
>
If the guest did negotiate _F_EVENT_IDX, it expects to be notified
only when device marks as used a specific number of descriptors.
If we use VirtQueue notification, the VirtQueue code handles it
transparently. But if we want to be able to change the guest VQ's
call_fd, we cannot use VirtQueue's, so this needs to be handled by SVQ
code. And that is still not implemented.
Of course in the event_idx case we could just ignore it and notify in
all used descriptors, but it seems not polite to me :). I will develop
event_idx on top of this, either exposing the needed pieces in
VirtQueue (I prefer this) or rolling our own in SVQ.
Same reasoning can be applied to unknown transport features.
Thanks!
> Thanks
>
> > Signed-off-by: Eugenio Pérez
> > ---
> > hw/virtio/vhost-shadow-virtqueue.h | 1 +
> > hw/virtio/vhost-shadow-virtqueue.c | 6 ++
> > 2 files changed, 7 insertions(+)
> >
> > diff --git a/hw/virtio/vhost-shadow-virtqueue.h
> > b/hw/virtio/vhost-shadow-virtqueue.h
> > index 946b2c6295..ac55588009 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.h
> > +++ b/hw/virtio/vhost-shadow-virtqueue.h
> > @@ -16,6 +16,7 @@
> > typedef struct VhostShadowVirtqueue VhostShadowVirtqueue;
> >
> > bool vhost_svq_valid_device_features(uint64_t *features);
> > +bool vhost_svq_valid_guest_features(uint64_t *features);
> >
> > void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int svq_kick_fd);
> > void vhost_svq_set_guest_call_notifier(VhostShadowVirtqueue *svq, int
> > call_fd);
> > diff --git a/hw/virtio/vhost-shadow-virtqueue.c
> > b/hw/virtio/vhost-shadow-virtqueue.c
> > index 6e0508a231..cb9ffcb015 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > @@ -62,6 +62,12 @@ bool vhost_svq_valid_device_features(uint64_t
> > *dev_features)
> > return true;
> > }
> >
> > +/* If the guest is using some of these, SVQ cannot communicate */
> > +bool vhost_svq_valid_guest_features(uint64_t *guest_features)
> > +{
> > +return true;
> > +}
> > +
> > /* Forward guest notifications */
> > static void vhost_handle_guest_kick(EventNotifier *n)
> > {
> > --
> > 2.27.0
> >
>
Re: [RFC PATCH v5 21/26] vhost: Add vhost_svq_valid_guest_features to shadow vq
On Sat, Oct 30, 2021 at 2:44 AM Eugenio Pérez wrote:
>
> This allows it to test if the guest has aknowledge an invalid transport
> feature for SVQ. This will include packed vq layout or event_idx,
> where VirtIO device needs help from SVQ.
>
> There is not needed at this moment, but since SVQ will not re-negotiate
> features again with the guest, a failure in acknowledge them is fatal
> for SVQ.
>
It's not clear to me why we need this. Maybe you can give me an
example. E.g isn't it sufficient to filter out the device with
event_idx?
Thanks
> Signed-off-by: Eugenio Pérez
> ---
> hw/virtio/vhost-shadow-virtqueue.h | 1 +
> hw/virtio/vhost-shadow-virtqueue.c | 6 ++
> 2 files changed, 7 insertions(+)
>
> diff --git a/hw/virtio/vhost-shadow-virtqueue.h
> b/hw/virtio/vhost-shadow-virtqueue.h
> index 946b2c6295..ac55588009 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.h
> +++ b/hw/virtio/vhost-shadow-virtqueue.h
> @@ -16,6 +16,7 @@
> typedef struct VhostShadowVirtqueue VhostShadowVirtqueue;
>
> bool vhost_svq_valid_device_features(uint64_t *features);
> +bool vhost_svq_valid_guest_features(uint64_t *features);
>
> void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int svq_kick_fd);
> void vhost_svq_set_guest_call_notifier(VhostShadowVirtqueue *svq, int
> call_fd);
> diff --git a/hw/virtio/vhost-shadow-virtqueue.c
> b/hw/virtio/vhost-shadow-virtqueue.c
> index 6e0508a231..cb9ffcb015 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.c
> +++ b/hw/virtio/vhost-shadow-virtqueue.c
> @@ -62,6 +62,12 @@ bool vhost_svq_valid_device_features(uint64_t
> *dev_features)
> return true;
> }
>
> +/* If the guest is using some of these, SVQ cannot communicate */
> +bool vhost_svq_valid_guest_features(uint64_t *guest_features)
> +{
> +return true;
> +}
> +
> /* Forward guest notifications */
> static void vhost_handle_guest_kick(EventNotifier *n)
> {
> --
> 2.27.0
>
[RFC PATCH v5 21/26] vhost: Add vhost_svq_valid_guest_features to shadow vq
This allows it to test if the guest has aknowledge an invalid transport
feature for SVQ. This will include packed vq layout or event_idx,
where VirtIO device needs help from SVQ.
There is not needed at this moment, but since SVQ will not re-negotiate
features again with the guest, a failure in acknowledge them is fatal
for SVQ.
Signed-off-by: Eugenio Pérez
---
hw/virtio/vhost-shadow-virtqueue.h | 1 +
hw/virtio/vhost-shadow-virtqueue.c | 6 ++
2 files changed, 7 insertions(+)
diff --git a/hw/virtio/vhost-shadow-virtqueue.h
b/hw/virtio/vhost-shadow-virtqueue.h
index 946b2c6295..ac55588009 100644
--- a/hw/virtio/vhost-shadow-virtqueue.h
+++ b/hw/virtio/vhost-shadow-virtqueue.h
@@ -16,6 +16,7 @@
typedef struct VhostShadowVirtqueue VhostShadowVirtqueue;
bool vhost_svq_valid_device_features(uint64_t *features);
+bool vhost_svq_valid_guest_features(uint64_t *features);
void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int svq_kick_fd);
void vhost_svq_set_guest_call_notifier(VhostShadowVirtqueue *svq, int call_fd);
diff --git a/hw/virtio/vhost-shadow-virtqueue.c
b/hw/virtio/vhost-shadow-virtqueue.c
index 6e0508a231..cb9ffcb015 100644
--- a/hw/virtio/vhost-shadow-virtqueue.c
+++ b/hw/virtio/vhost-shadow-virtqueue.c
@@ -62,6 +62,12 @@ bool vhost_svq_valid_device_features(uint64_t *dev_features)
return true;
}
+/* If the guest is using some of these, SVQ cannot communicate */
+bool vhost_svq_valid_guest_features(uint64_t *guest_features)
+{
+return true;
+}
+
/* Forward guest notifications */
static void vhost_handle_guest_kick(EventNotifier *n)
{
--
2.27.0
