Missing trait items

[harshanavkis]

This commit requires one to implement the new 'get_inflight_fd' and 'set_inflight_fd' functions for VhostUserHandler from the VhostUserSlaveReqHandlerMut trait. The following error occurs otherwise:

error[E0046]: not all trait items implemented, missing: `get_inflight_fd`, `set_inflight_fd`
   --> /home/hvub/.cargo/git/checkouts/vhost-user-backend-0d66fc5baaf39798/ddc7dd5/src/lib.rs:556:1
    |
556 | impl<S: VhostUserBackend> VhostUserSlaveReqHandlerMut for VhostUserHandler<S> {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `get_inflight_fd`, `set_inflight_fd` in implementation
    |
    = help: implement the missing item: `fn get_inflight_fd(&mut self, _: &VhostUserInflight) -> std::result::Result<(VhostUserInflight, i32), vhost::vhost_user::Error> { todo!() }`
    = help: implement the missing item: `fn set_inflight_fd(&mut self, _: &VhostUserInflight, _: File) -> std::result::Result<(), vhost::vhost_user::Error> { todo!() }`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0046`.
error: could not compile `vhost-user-backend`

To learn more, run the command again with --verbose.

Is anybody working on implementing this? If not, I could give it a shot.

[slp]

@harshanavkis Hi! We need to fix this one ASAP. Do you still want to give it a try? Otherwise I'll fix it myself.

[harshanavkis]

@slp I can give it a shot, will send out some patches.

[harshanavkis]

@slp a few questions. I have implemented the memory region using a tmpfile for now. This region would hold N queue regions for each of the N virtqueues as described in the vhost-user protocol. Each queue region is represented as follows:

typedef struct DescStateSplit {
    /* Indicate whether this descriptor is inflight or not.
     * Only available for head-descriptor. */
    uint8_t inflight;

    /* Padding */
    uint8_t padding[5];

    /* Maintain a list for the last batch of used descriptors.
     * Only available when batching is used for submitting */
    uint16_t next;

    /* Used to preserve the order of fetching available descriptors.
     * Only available for head-descriptor. */
    uint64_t counter;
} DescStateSplit;

typedef struct QueueRegionSplit {
    /* The feature flags of this region. Now it's initialized to 0. */
    uint64_t features;

    /* The version of this region. It's 1 currently.
     * Zero value indicates an uninitialized buffer */
    uint16_t version;

    /* The size of DescStateSplit array. It's equal to the virtqueue
     * size. Slave could get it from queue size field of VhostUserInflight. */
    uint16_t desc_num;

    /* The head of list that track the last batch of used descriptors. */
    uint16_t last_batch_head;

    /* Store the idx value of used ring */
    uint16_t used_idx;

    /* Used to track the state of each descriptor in descriptor table */
    DescStateSplit desc[];
} QueueRegionSplit;

Where each region is represented by "QueueRegionSplit", and the final buffer shared with the master would be a, for example, vector of N "QueueRegionSplit". I could not find a definition of a similar structure in rust-vmm after searching for a bit. Do I need to define it on my own? Or put it another way, how do I figure out the size of the memory region that the tempfile needs to be truncated by?

[harshanavkis]

@slp I have implemented a minimal patch to address this issue. However, I see few problems with my current implementation:

1. "mmap_size" is not set correctly for VhostUserInflight as QueueRegionSplit structure has a vector. How is such a case handled in the project or in general?
2. Do I need to copy the data structure into the tempfile?
3. Would it be better to use a memfd? I see that qemu, for example, uses memfd to allocate an fd for this structure.

[slp]

@sboeuf Could you please give us some feedback on this?

[sboeuf]

Hi @harshanavkis
So first I think it'd be good to define DescStateSplit and QueueRegionSplit as part of the rust-vmm/vhost crate, instead of having the current crate do it.
You're looking at split virtqueues, but the spec says that packed virtqueues must be handled slightly differently with some other structures (which should also be defined as part of rust-vmm/vhost).

mmap_size" is not set correctly for VhostUserInflight as QueueRegionSplit structure has a vector. How is such a case handled in the project or in general?

What do you mean it's not set correctly? The mmap size is provided by the VMM based on the amount of queues and the size of each queue. If the queues are 256 in size, you'll have a vector of 256 DescStateSplit.

Do I need to copy the data structure into the tempfile?
Would it be better to use a memfd? I see that qemu, for example, uses memfd to allocate an fd for this structure.

You should definitely use a memfd here, and then mmap() it into the backend address space. This way the backend can update the content of the inflight shared region.

Last thing, from a vhost-user-backend perspective, implementing get/set inflight I/O can be quite simple since we don't expect too much from these calls. But it'd be nice to provide some helpers in order to avoid the duplication of the logic to update the shared memory content across all vhost-user backend implementations relying on vhost_user_backend.

[harshanavkis]

@sboeuf I have opened a pull request to add the structures into the rust-vmm/vhost crate. I am working on the rest now.