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