Re: [PATCH v1 12/13] hw/virtio/vhost-user: don't suppress F_CONFIG when supported

[Michael S. Tsirkin]

On Tue, Mar 22, 2022 at 03:54:32PM +0000, Alex Bennée wrote:
> 
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Mon, Mar 21, 2022 at 03:30:36PM +0000, Alex Bennée wrote:
> >> Previously we would silently suppress VHOST_USER_PROTOCOL_F_CONFIG
> >> during the protocol negotiation if the QEMU stub hadn't implemented
> >> the vhost_dev_config_notifier. However this isn't the only way we can
> >> handle config messages, the existing vdc->get/set_config can do this
> >> as well.
> >
> > Could you give an example where the problem is encountered please?
> 
> Well I only came across it when I realised by rpmb implementation wasn't
> sending config updates to the vhost-user daemon because it only
> implemented set/get_config methods.
> 
> I think vhost-user-scsi suffers from this but I'm not able to test it as
> it's not clear how to do it. The vhost-user-scsi daemon want to attach
> to real SCSI devices rather than a file for the block device. I guess I
> need to somehow add "fake" scsi nodes to my host system which SCSI
> commands can be passed to?
> 
> When was the last time vhost-user-scsi was tested with anything?

It's for passthrough mostly, yes. I think you can use tcm_loop
on top of loopback on top of a file. Didn't try myself.

> >> Lightly re-factor the code to check for both potential methods and
> >> instead of silently squashing the feature error out. It is unlikely
> >> that a vhost-user backend expecting to handle CONFIG messages will
> >> behave correctly if they never get sent.
> >
> > Hmm but are you sure? Most devices work mostly fine without CONFIG
> > messages, there's a chance a backend set this flag just in case
> > without much thought ...
> 
> But that would be a bug right? Certainly a mismatch if something really
> does want to see config messages. Again RPMB needs this because it's the
> vhost-user daemon that knows the size of the device.


Could you pls clarify how do you create a config that's broken with RPMB?

> >> Fixes: 1c3e5a2617 ("vhost-user: back SET/GET_CONFIG requests with a 
> >> protocol feature")
> >
> > I'm not sure whether something is broken or this is a cleanup patch.
> > Fixes tag means "if you have 1c3e5a2617 you should pick this patch", so
> > cleanups don't need a fixes: tag.
> 
> No I think it's broken, we just didn't notice because as you say most
> devices don't need to care.

I was mostly asking about the fixes tag.  If no existing user cares then
it's best to avoid the fixes tag, it's a hint for backporters which
versions need the patch.


> >
> >
> >> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >> 
> >> ---
> >>   - we can't check for get_config/set_config as the stack squashed vdev
> >>   - use vhost-user-state to transmit this
> >> ---
> >>  include/hw/virtio/vhost-user.h |  1 +
> >>  hw/scsi/vhost-user-scsi.c      |  1 +
> >>  hw/virtio/vhost-user.c         | 46 ++++++++++++++++++++++++----------
> >>  3 files changed, 35 insertions(+), 13 deletions(-)
> >> 
> >> diff --git a/include/hw/virtio/vhost-user.h 
> >> b/include/hw/virtio/vhost-user.h
> >> index e44a41bb70..6e0e8a71a3 100644
> >> --- a/include/hw/virtio/vhost-user.h
> >> +++ b/include/hw/virtio/vhost-user.h
> >> @@ -22,6 +22,7 @@ typedef struct VhostUserState {
> >>      CharBackend *chr;
> >>      VhostUserHostNotifier notifier[VIRTIO_QUEUE_MAX];
> >>      int memory_slots;
> >> +    bool supports_config;
> >>  } VhostUserState;
> >>  
> >>  bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error 
> >> **errp);
> >> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
> >> index 1b2f7eed98..9be21d07ee 100644
> >> --- a/hw/scsi/vhost-user-scsi.c
> >> +++ b/hw/scsi/vhost-user-scsi.c
> >> @@ -121,6 +121,7 @@ static void vhost_user_scsi_realize(DeviceState *dev, 
> >> Error **errp)
> >>      vsc->dev.backend_features = 0;
> >>      vqs = vsc->dev.vqs;
> >>  
> >> +    s->vhost_user.supports_config = true;
> >>      ret = vhost_dev_init(&vsc->dev, &s->vhost_user,
> >>                           VHOST_BACKEND_TYPE_USER, 0, errp);
> >>      if (ret < 0) {
> >> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> >> index b27b8c56e2..6ce082861b 100644
> >> --- a/hw/virtio/vhost-user.c
> >> +++ b/hw/virtio/vhost-user.c
> >> @@ -1949,14 +1949,15 @@ static int 
> >> vhost_user_postcopy_notifier(NotifierWithReturn *notifier,
> >>  static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque,
> >>                                     Error **errp)
> >>  {
> >> -    uint64_t features, protocol_features, ram_slots;
> >> +    uint64_t features, ram_slots;
> >>      struct vhost_user *u;
> >> +    VhostUserState *vus = (VhostUserState *) opaque;
> >>      int err;
> >>  
> >>      assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
> >>  
> >>      u = g_new0(struct vhost_user, 1);
> >> -    u->user = opaque;
> >> +    u->user = vus;
> >>      u->dev = dev;
> >>      dev->opaque = u;
> >>  
> >> @@ -1967,6 +1968,10 @@ static int vhost_user_backend_init(struct vhost_dev 
> >> *dev, void *opaque,
> >>      }
> >>  
> >>      if (virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) {
> >> +        bool supports_f_config = vus->supports_config ||
> >> +            (dev->config_ops && 
> >> dev->config_ops->vhost_dev_config_notifier);
> >> +        uint64_t protocol_features;
> >> +
> >>          dev->backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
> >>  
> >>          err = vhost_user_get_u64(dev, VHOST_USER_GET_PROTOCOL_FEATURES,
> >> @@ -1976,19 +1981,34 @@ static int vhost_user_backend_init(struct 
> >> vhost_dev *dev, void *opaque,
> >>              return -EPROTO;
> >>          }
> >>  
> >> -        dev->protocol_features =
> >> -            protocol_features & VHOST_USER_PROTOCOL_FEATURE_MASK;
> >> -
> >> -        if (!dev->config_ops || 
> >> !dev->config_ops->vhost_dev_config_notifier) {
> >> -            /* Don't acknowledge CONFIG feature if device doesn't support 
> >> it */
> >> -            dev->protocol_features &= ~(1ULL << 
> >> VHOST_USER_PROTOCOL_F_CONFIG);
> >> -        } else if (!(protocol_features &
> >> -                    (1ULL << VHOST_USER_PROTOCOL_F_CONFIG))) {
> >> -            error_setg(errp, "Device expects VHOST_USER_PROTOCOL_F_CONFIG 
> >> "
> >> -                       "but backend does not support it.");
> >> -            return -EINVAL;
> >> +        /*
> >> +         * We will use all the protocol features we support - although
> >> +         * we suppress F_CONFIG if we know QEMUs internal code can not 
> >> support
> >> +         * it.
> >> +         */
> >> +        protocol_features &= VHOST_USER_PROTOCOL_FEATURE_MASK;
> >> +
> >> +        if (supports_f_config) {
> >> +            if (!virtio_has_feature(protocol_features,
> >> +                                    VHOST_USER_PROTOCOL_F_CONFIG)) {
> >> +                error_setg(errp, "vhost-user device %s expecting "
> >> +                           "VHOST_USER_PROTOCOL_F_CONFIG but the 
> >> vhost-user backend does "
> >> +                           "not support it.", dev->vdev->name);
> >> +                return -EPROTO;
> >> +            }
> >> +        } else {
> >> +            if (virtio_has_feature(protocol_features,
> >> +                                   VHOST_USER_PROTOCOL_F_CONFIG)) {
> >> +                warn_reportf_err(*errp, "vhost-user backend supports "
> >> +                                 "VHOST_USER_PROTOCOL_F_CONFIG for "
> >> +                                 "device %s but QEMU does not.",
> >> +                                 dev->vdev->name);
> >> +                protocol_features &= ~(1ULL << 
> >> VHOST_USER_PROTOCOL_F_CONFIG);
> >> +            }
> >>          }
> >>  
> >> +        /* final set of protocol features */
> >> +        dev->protocol_features = protocol_features;
> >>          err = vhost_user_set_protocol_features(dev, 
> >> dev->protocol_features);
> >>          if (err < 0) {
> >>              error_setg_errno(errp, EPROTO, "vhost_backend_init failed");
> >> -- 
> >> 2.30.2
> 
> 
> -- 
> Alex Bennée