[PATCH] vhost-user: Check vhost features for CONFIGURE_MEM_SLOTS

[PATCH] vhost-user: Check vhost features for CONFIGURE_MEM_SLOTS

[Dylan Reid]

The `CONFIGURE_MEM_SLOTS` feature is specified by vhost, not by virtio.
Check the vhost flags for it being set.

I noticed this while testing a new vhost implementation that doesn't yet
support configuring memory slots and retested with dpdk's block example
as well.

Signed-off-by: Dylan Reid <dgreid@dylanreid.com>
---
 hw/virtio/vhost-user.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 2fdd5daf74..faa0e133f2 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1860,7 +1860,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque)
         }
 
         /* get max memory regions if backend supports configurable RAM slots */
-        if (!virtio_has_feature(dev->protocol_features,
+        if (!virtio_has_feature(features,
                                 VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS)) {
             u->user->memory_slots = VHOST_MEMORY_BASELINE_NREGIONS;
         } else {
-- 
2.25.1

Re: [PATCH] vhost-user: Check vhost features for CONFIGURE_MEM_SLOTS

[Raphael Norwitz]

I don't think this is right.

On Tue, Dec 22, 2020 at 03:41:26PM -0800, Dylan Reid wrote:
> 
> The `CONFIGURE_MEM_SLOTS` feature is specified by vhost, not by virtio.
> Check the vhost flags for it being set.
>

Agreed - VHOST_USER_PROTCOL_F_CONFIGURE_MEM_SLOTS is negotiated at the 
vhost-user, not virtio layer. The dev->protocol_features flags are taken
from the VHOST_USER_GET_PROTOCOL_FEATURES message, which retrieves the 
supported vhost-user features from the backend. See libvhost-user for a
simple reference implementation.

The VHOST_USER_GET_FEATURES message retrieves the virtio protocol
features, so checking features instead of dev->protocol_features would
incorrectly check the supported virtio features instead of the vhost-user
ones.

Am I missing something here?

> I noticed this while testing a new vhost implementation that doesn't yet
> support configuring memory slots and retested with dpdk's block example
> as well.

Could you clarify what you tested with DPDK? I haven't had problems 
testing other backends which do not support configuring memory slots. 

> 
> Signed-off-by: Dylan Reid <dgreid@dylanreid.com>
> ---
>  hw/virtio/vhost-user.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 2fdd5daf74..faa0e133f2 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1860,7 +1860,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque)
>          }
>  
>          /* get max memory regions if backend supports configurable RAM slots */
> -        if (!virtio_has_feature(dev->protocol_features,
> +        if (!virtio_has_feature(features,
>                                  VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS)) {
>              u->user->memory_slots = VHOST_MEMORY_BASELINE_NREGIONS;
>          } else {
> -- 
> 2.25.1
> 
> 
~    

Re: [PATCH] vhost-user: Check vhost features for CONFIGURE_MEM_SLOTS

[Dylan Reid]

>Agreed - VHOST_USER_PROTCOL_F_CONFIGURE_MEM_SLOTS is negotiated at the
>vhost-user, not virtio layer. The dev->protocol_features flags are taken
>from the VHOST_USER_GET_PROTOCOL_FEATURES message, which retrieves the
>supported vhost-user features from the backend. See libvhost-user for a
>simple reference implementation.
>
>The VHOST_USER_GET_FEATURES message retrieves the virtio protocol
>features, so checking features instead of dev->protocol_features would
>incorrectly check the supported virtio features instead of the vhost-user
>ones.
>
>Am I missing something here?

No, you're not missing anything, I am(so is the code I was working with).
I hade the message number and feature bit backwards.

Thanks for taking the time to read this, your explaination got me back on the right track, I'll go fix the library I was using.