On Wed, Dec 12, 2018 at 05:31:39PM +0800, jiangyiwen wrote:
> +static struct virtio_vsock_pkt *receive_mergeable(struct virtqueue *vq,
> +		struct virtio_vsock *vsock, unsigned int *total_len)
> +{
> +	struct virtio_vsock_pkt *pkt;
> +	u16 num_buf;
> +	void *buf;
> +	unsigned int len;
> +	size_t vsock_hlen = sizeof(struct virtio_vsock_pkt);
> +
> +	buf = virtqueue_get_buf(vq, &len);
> +	if (!buf)
> +		return NULL;
> +
> +	*total_len = len;
> +	vsock->rx_buf_nr--;
> +
> +	if (unlikely(len < vsock_hlen)) {
> +		put_page(virt_to_head_page(buf));
> +		return NULL;
> +	}
> +
> +	pkt = buf;
> +	num_buf = le16_to_cpu(pkt->mrg_rxbuf_hdr.num_buffers);
> +	if (!num_buf || num_buf > VIRTIO_VSOCK_MAX_VEC_NUM) {
> +		put_page(virt_to_head_page(buf));
> +		return NULL;
> +	}
> +
> +	/* Initialize pkt residual structure */
> +	memset(&pkt->work, 0, vsock_hlen - sizeof(struct virtio_vsock_hdr) -
> +			sizeof(struct virtio_vsock_mrg_rxbuf_hdr));

struct virtio_vsock_pkt is an internal driver state structure.  It must
be able to change without breaking the host<->guest device interface.
Exposing struct virtio_vsock_pkt across the device interface makes this
impossible.

One way to solve this is by placing a header length field at the
beginning of the mergeable buffer.  That way the device knows at which
offset the payload should be placed and the driver can continue to use
the allocation scheme you've chosen (where a single page contains the
virtio_vsock_pkt and payload).

Another issue is that virtio requires exclusive read OR write
descriptors.  You cannot have a descriptor that is both read and write.
So there's not a huge advantage to combining the driver state struct and
payload, except maybe the driver can save a memory allocation.