vhost reply_ack negotiation (a.k.a differences in vhost-user behaviour with libvhost-user and vhost-user-backend.rs)

* vhost reply_ack negotiation (a.k.a differences in vhost-user behaviour with libvhost-user and vhost-user-backend.rs)
@ 2021-02-19 16:04 Alex Bennée
  2021-02-22 13:06 ` Dr. David Alan Gilbert
  2021-02-23 11:44 ` Michael S. Tsirkin
  0 siblings, 2 replies; 11+ messages in thread
From: Alex Bennée @ 2021-02-19 16:04 UTC (permalink / raw)
  To: qemu-devel, rust-vmm
  Cc: Marc-André Lureau, Dr. David Alan Gilbert, Sergio Lopez,
	Stefan Hajnoczi, Michael S. Tsirkin

Hi,

I finally got a chance to get down into the guts of vhost-user while
attempting to port my original C RPMB daemon to Rust using the
vhost-user-backend and related crates. I ended up with this hang during
negotiation:

  startup

  vhost_user_write req:1 flags:0x1
  vhost_user_read_start
  vhost_user_read req:1 flags:0x5
  vhost_user_backend_init: we got 170000000
  vhost_user_write req:15 flags:0x1
  vhost_user_read_start
  vhost_user_read req:15 flags:0x5
  vhost_user_set_protocol_features: 2008
  vhost_user_write req:16 flags:0x1
  vhost_user_write req:3 flags:0x1
  vhost_user_write req:1 flags:0x1
  vhost_user_read_start
  vhost_user_read req:1 flags:0x5
  vhost_user_write req:13 flags:0x1

  kernel initialises device

  virtio_rpmb virtio1: init done!
  vhost_user_write req:13 flags:0x1
  vhost_dev_set_features: 130000000
  vhost_user_set_features: 130000000
  vhost_user_write req:2 flags:0x1
  vhost_user_write req:5 flags:0x9
  vhost_user_read_start

The proximate cause is the vhost crate handling:

  MasterReq::SET_MEM_TABLE => {
      let res = self.set_mem_table(&hdr, size, &buf, rfds);
      self.send_ack_message(&hdr, res)?;
  }

which gates on the replay_ack_enabled flag:

    fn send_ack_message(
        &mut self,
        req: &VhostUserMsgHeader<MasterReq>,
        res: Result<()>,
    ) -> Result<()> {
        if dbg!(self.reply_ack_enabled) {
            let hdr = self.new_reply_header::<VhostUserU64>(req, 0)?;
            let val = match res {
                Ok(_) => 0,
                Err(_) => 1,
            };
            let msg = VhostUserU64::new(val);
            self.main_sock.send_message(&hdr, &msg, None)?;
        }
        Ok(())
    }

which is only set when we have all the appropriate acknowledged flags:

    fn update_reply_ack_flag(&mut self) {
        let vflag = VhostUserVirtioFeatures::PROTOCOL_FEATURES.bits();
        let pflag = VhostUserProtocolFeatures::REPLY_ACK;
        if (self.virtio_features & vflag) != 0
            && (self.acked_virtio_features & vflag) != 0
            && self.protocol_features.contains(pflag)
            && (self.acked_protocol_features & pflag.bits()) != 0
        {
            self.reply_ack_enabled = true;
        } else {
            self.reply_ack_enabled = false;
        }
    }

which from above you can see QEMU helpfully dropped those bits in the
reply. It does however work in the C/libvhost version:

  virtio_rpmb virtio1: init done!
  vhost_user_write req:13 flags:0x1
  vhost_dev_set_features: 130000000
  vhost_user_set_features: 130000000
  vhost_user_write req:2 flags:0x1
  vhost_user_write req:37 flags:0x9
  vhost_user_read_start
  vhost_user_read req:37 flags:0x5
  vhost_user_write req:8 flags:0x1
  vhost_user_write req:10 flags:0x1
  vhost_user_write req:9 flags:0x1
  vhost_user_write req:12 flags:0x1
  vhost_user_write req:13 flags:0x1

albeit with a slightly different message sequence
(VHOST_USER_ADD_MEM_REG instead of VHOST_USER_SET_MEM_TABLE). Reading
the C code you can see why:

    need_reply = vmsg.flags & VHOST_USER_NEED_REPLY_MASK;

    reply_requested = vu_process_message(dev, &vmsg);
    if (!reply_requested && need_reply) {
        vmsg_set_reply_u64(&vmsg, 0);
        reply_requested = 1;
    }

So regardless of what may have been negotiated it will always reply with
something if the master requested it do so. This points us at the
specification which reads:

  - Bit 3 is the need_reply flag - see :ref:`REPLY_ACK <reply_ack>` for
    details.

which says in VHOST_USER_PROTOCOL_F_REPLY_ACK that this bit should only
be honoured when the feature has been negotiated. Which brings us to a
series of questions:

 - Should QEMU have preserved VhostUserVirtioFeatures::PROTOCOL_FEATURES
   when doing the eventual VHOST_USER_SET_FEATURES reply?

 - Is vhost.rs being to strict or libvhost-user too lax in interpreting
   the negotiated features before processing the ``need_reply`` [Bit 3]
   field of the messages?

 - are VHOST_USER_SET_MEM_TABLE to VHOST_USER_SET_INFLIGHT_FD included
   in the "list of the ones that do" require replies or do they only
   reply when REPLY_ACK has been negotiated as the ambiguous "seealso::"
   box out seems to imply?

Currently I have some hacks in:

  https://github.com/stsquad/vhost/tree/my-hacks

which gets my daemon booting up to the point we actually need to do a
transaction. However I won't submit a PR until I've worked out exactly
where the problems are.

-- 
Alex Bennée

^ permalink raw reply	[flat|nested] 11+ messages in thread