Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Yongji Xie <xieyongji@bytedance.com>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: "Michael S. Tsirkin" <mst@redhat.com>,
	"Jason Wang" <jasowang@redhat.com>,
	"Stefan Hajnoczi" <stefanha@redhat.com>,
	"Stefano Garzarella" <sgarzare@redhat.com>,
	"Parav Pandit" <parav@nvidia.com>,
	"Christoph Hellwig" <hch@infradead.org>,
	"Christian Brauner" <christian.brauner@canonical.com>,
	"Randy Dunlap" <rdunlap@infradead.org>,
	"Matthew Wilcox" <willy@infradead.org>,
	"Al Viro" <viro@zeniv.linux.org.uk>,
	"Jens Axboe" <axboe@kernel.dk>,
	bcrl@kvack.org, "Jonathan Corbet" <corbet@lwn.net>,
	"Mika Penttilä" <mika.penttila@nextfour.com>,
	"Dan Carpenter" <dan.carpenter@oracle.com>,
	joro@8bytes.org, "He Zhe" <zhe.he@windriver.com>,
	"Liu Xiaodong" <xiaodong.liu@intel.com>,
	"Joe Perches" <joe@perches.com>,
	songmuchun@bytedance.com,
	virtualization <virtualization@lists.linux-foundation.org>,
	netdev@vger.kernel.org, kvm <kvm@vger.kernel.org>,
	linux-fsdevel@vger.kernel.org, iommu@lists.linux-foundation.org,
	linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v10 16/17] vduse: Introduce VDUSE - vDPA Device in Userspace
Date: Thu, 29 Jul 2021 17:57:21 +0800	[thread overview]
Message-ID: <CACycT3vDspiXSh=UoK9JXaMpv1+9C61DLy_-bWJV5XRxKs2xRw@mail.gmail.com> (raw)
In-Reply-To: <YQJuG7zrzdWm+ieZ@kroah.com>

On Thu, Jul 29, 2021 at 5:00 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Jul 29, 2021 at 03:35:02PM +0800, Xie Yongji wrote:
> > +/*
> > + * The basic configuration of a VDUSE device, which is used by
> > + * VDUSE_CREATE_DEV ioctl to create a VDUSE device.
> > + */
> > +struct vduse_dev_config {
>
> Please document this structure using kernel doc so we know what all the
> fields are.
>

Sure.

> > +#define VDUSE_NAME_MAX       256
> > +     char name[VDUSE_NAME_MAX]; /* vduse device name, needs to be NUL terminated */
> > +     __u32 vendor_id; /* virtio vendor id */
> > +     __u32 device_id; /* virtio device id */
> > +     __u64 features; /* virtio features */
> > +     __u32 vq_num; /* the number of virtqueues */
> > +     __u32 vq_align; /* the allocation alignment of virtqueue's metadata */
> > +     __u32 reserved[13]; /* for future use */
>
> This HAS to be tested to be all 0, otherwise you can never use it in the
> future.  I did not see the code doing that at all.
>

Make sense. Will do it in the next version.

> > +     __u32 config_size; /* the size of the configuration space */
> > +     __u8 config[0]; /* the buffer of the configuration space */
>
> config[]; please instead?  I thought we were getting rid of all of the
> 0-length arrays in the kernel tree.
>

OK.

> > +};
> > +
> > +/* Create a VDUSE device which is represented by a char device (/dev/vduse/$NAME) */
> > +#define VDUSE_CREATE_DEV     _IOW(VDUSE_BASE, 0x02, struct vduse_dev_config)
> > +
> > +/*
> > + * Destroy a VDUSE device. Make sure there are no more references
> > + * to the char device (/dev/vduse/$NAME).
> > + */
> > +#define VDUSE_DESTROY_DEV    _IOW(VDUSE_BASE, 0x03, char[VDUSE_NAME_MAX])
> > +
> > +/* The ioctls for VDUSE device (/dev/vduse/$NAME) */
> > +
> > +/*
> > + * The information of one IOVA region, which is retrieved from
> > + * VDUSE_IOTLB_GET_FD ioctl.
> > + */
> > +struct vduse_iotlb_entry {
> > +     __u64 offset; /* the mmap offset on returned file descriptor */
> > +     __u64 start; /* start of the IOVA range: [start, last] */
> > +     __u64 last; /* last of the IOVA range: [start, last] */
> > +#define VDUSE_ACCESS_RO 0x1
> > +#define VDUSE_ACCESS_WO 0x2
> > +#define VDUSE_ACCESS_RW 0x3
> > +     __u8 perm; /* access permission of this region */
> > +};
> > +
> > +/*
> > + * Find the first IOVA region that overlaps with the range [start, last]
> > + * and return the corresponding file descriptor. Return -EINVAL means the
> > + * IOVA region doesn't exist. Caller should set start and last fields.
> > + */
> > +#define VDUSE_IOTLB_GET_FD   _IOWR(VDUSE_BASE, 0x10, struct vduse_iotlb_entry)
> > +
> > +/*
> > + * Get the negotiated virtio features. It's a subset of the features in
> > + * struct vduse_dev_config which can be accepted by virtio driver. It's
> > + * only valid after FEATURES_OK status bit is set.
> > + */
> > +#define VDUSE_DEV_GET_FEATURES       _IOR(VDUSE_BASE, 0x11, __u64)
> > +
> > +/*
> > + * The information that is used by VDUSE_DEV_SET_CONFIG ioctl to update
> > + * device configuration space.
> > + */
> > +struct vduse_config_data {
> > +     __u32 offset; /* offset from the beginning of configuration space */
> > +     __u32 length; /* the length to write to configuration space */
> > +     __u8 buffer[0]; /* buffer used to write from */
>
> again, buffer[];?
>

OK.

> > +};
> > +
> > +/* Set device configuration space */
> > +#define VDUSE_DEV_SET_CONFIG _IOW(VDUSE_BASE, 0x12, struct vduse_config_data)
> > +
> > +/*
> > + * Inject a config interrupt. It's usually used to notify virtio driver
> > + * that device configuration space has changed.
> > + */
> > +#define VDUSE_DEV_INJECT_CONFIG_IRQ  _IO(VDUSE_BASE, 0x13)
> > +
> > +/*
> > + * The basic configuration of a virtqueue, which is used by
> > + * VDUSE_VQ_SETUP ioctl to setup a virtqueue.
> > + */
> > +struct vduse_vq_config {
> > +     __u32 index; /* virtqueue index */
> > +     __u16 max_size; /* the max size of virtqueue */
> > +};
> > +
> > +/*
> > + * Setup the specified virtqueue. Make sure all virtqueues have been
> > + * configured before the device is attached to vDPA bus.
> > + */
> > +#define VDUSE_VQ_SETUP               _IOW(VDUSE_BASE, 0x14, struct vduse_vq_config)
> > +
> > +struct vduse_vq_state_split {
> > +     __u16 avail_index; /* available index */
> > +};
> > +
> > +struct vduse_vq_state_packed {
> > +     __u16 last_avail_counter:1; /* last driver ring wrap counter observed by device */
> > +     __u16 last_avail_idx:15; /* device available index */
>
> Bit fields in a user structure?  Are you sure this is going to work
> well?  Why not just make this a __u16 and then mask off what you want so
> that you do not run into endian issues?
>

Good point! I will use __u16 for each field instead.

> > +     __u16 last_used_counter:1; /* device ring wrap counter */
> > +     __u16 last_used_idx:15; /* used index */
> > +};
> > +
> > +/*
> > + * The information of a virtqueue, which is retrieved from
> > + * VDUSE_VQ_GET_INFO ioctl.
> > + */
> > +struct vduse_vq_info {
> > +     __u32 index; /* virtqueue index */
> > +     __u32 num; /* the size of virtqueue */
> > +     __u64 desc_addr; /* address of desc area */
> > +     __u64 driver_addr; /* address of driver area */
> > +     __u64 device_addr; /* address of device area */
> > +     union {
> > +             struct vduse_vq_state_split split; /* split virtqueue state */
> > +             struct vduse_vq_state_packed packed; /* packed virtqueue state */
> > +     };
> > +     __u8 ready; /* ready status of virtqueue */
> > +};
> > +
> > +/* Get the specified virtqueue's information. Caller should set index field. */
> > +#define VDUSE_VQ_GET_INFO    _IOWR(VDUSE_BASE, 0x15, struct vduse_vq_info)
> > +
> > +/*
> > + * The eventfd configuration for the specified virtqueue. It's used by
> > + * VDUSE_VQ_SETUP_KICKFD ioctl to setup kick eventfd.
> > + */
> > +struct vduse_vq_eventfd {
> > +     __u32 index; /* virtqueue index */
> > +#define VDUSE_EVENTFD_DEASSIGN -1
> > +     int fd; /* eventfd, -1 means de-assigning the eventfd */
>
> Don't we have a file descriptor type?  I could be wrong.
>

It looks like I did not find it...

> > +};
> > +
> > +/*
> > + * Setup kick eventfd for specified virtqueue. The kick eventfd is used
> > + * by VDUSE kernel module to notify userspace to consume the avail vring.
> > + */
> > +#define VDUSE_VQ_SETUP_KICKFD        _IOW(VDUSE_BASE, 0x16, struct vduse_vq_eventfd)
> > +
> > +/*
> > + * Inject an interrupt for specific virtqueue. It's used to notify virtio driver
> > + * to consume the used vring.
> > + */
> > +#define VDUSE_VQ_INJECT_IRQ  _IOW(VDUSE_BASE, 0x17, __u32)
> > +
> > +/* The control messages definition for read/write on /dev/vduse/$NAME */
> > +
> > +enum vduse_req_type {
> > +     /* Get the state for specified virtqueue from userspace */
> > +     VDUSE_GET_VQ_STATE,
> > +     /* Set the device status */
> > +     VDUSE_SET_STATUS,
> > +     /*
> > +      * Notify userspace to update the memory mapping for specified
> > +      * IOVA range via VDUSE_IOTLB_GET_FD ioctl
> > +      */
> > +     VDUSE_UPDATE_IOTLB,
> > +};
> > +
> > +struct vduse_vq_state {
> > +     __u32 index; /* virtqueue index */
> > +     union {
> > +             struct vduse_vq_state_split split; /* split virtqueue state */
> > +             struct vduse_vq_state_packed packed; /* packed virtqueue state */
> > +     };
> > +};
> > +
> > +struct vduse_dev_status {
> > +     __u8 status; /* device status */
> > +};
> > +
> > +struct vduse_iova_range {
> > +     __u64 start; /* start of the IOVA range: [start, end] */
> > +     __u64 last; /* last of the IOVA range: [start, end] */
> > +};
> > +
> > +struct vduse_dev_request {
> > +     __u32 type; /* request type */
> > +     __u32 request_id; /* request id */
> > +     __u32 reserved[2]; /* for future use */
>
> Again, this HAS to be checked to be 0 and aborted if not, otherwise you
> can never use it in the future.
>

I see. This has already been done in the current version.

> > +     union {
> > +             struct vduse_vq_state vq_state; /* virtqueue state, only use index */
> > +             struct vduse_dev_status s; /* device status */
> > +             struct vduse_iova_range iova; /* IOVA range for updating */
> > +             __u32 padding[16]; /* padding */
> > +     };
> > +};
> > +
> > +struct vduse_dev_response {
> > +     __u32 request_id; /* corresponding request id */
> > +#define VDUSE_REQ_RESULT_OK  0x00
> > +#define VDUSE_REQ_RESULT_FAILED      0x01
> > +     __u32 result; /* the result of request */
> > +     __u32 reserved[2]; /* for future use */
>
> Same here, you have to check this.
>

Sure.

> > +     union {
> > +             struct vduse_vq_state vq_state; /* virtqueue state */
> > +             __u32 padding[16]; /* padding */
>
> Check this padding too.
>

OK.

Thanks,
Yongji

  reply	other threads:[~2021-07-29  9:57 UTC|newest]

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-29  7:34 [PATCH v10 00/17] " Xie Yongji
2021-07-29  7:34 ` [PATCH v10 01/17] iova: Export alloc_iova_fast() and free_iova_fast() Xie Yongji
2021-08-03  7:41   ` Jason Wang
2021-08-03  7:41   ` Jason Wang
2021-08-03  8:54     ` Yongji Xie
2021-08-03 10:53       ` Robin Murphy
2021-08-04  5:02         ` Yongji Xie
2021-08-04 15:43           ` Robin Murphy
2021-08-05 12:34             ` Yongji Xie
2021-08-05 13:31               ` Jason Wang
2021-08-09  5:56                 ` Yongji Xie
2021-08-10  3:02                   ` Jason Wang
2021-08-10  7:43                     ` Yongji Xie
2021-07-29  7:34 ` [PATCH v10 02/17] file: Export receive_fd() to modules Xie Yongji
2021-08-03  7:45   ` Jason Wang
2021-08-03  9:01     ` Yongji Xie
2021-08-04  8:27       ` Jason Wang
2021-07-29  7:34 ` [PATCH v10 03/17] vdpa: Fix code indentation Xie Yongji
2021-08-03  7:50   ` Jason Wang
2021-08-03  9:13     ` Yongji Xie
2021-07-29  7:34 ` [PATCH v10 04/17] vdpa: Fail the vdpa_reset() if fail to set device status to zero Xie Yongji
2021-08-03  7:58   ` Jason Wang
2021-08-03  9:31     ` Yongji Xie
2021-08-04  8:30       ` Jason Wang
2021-07-29  7:34 ` [PATCH v10 05/17] vhost-vdpa: Fail the vhost_vdpa_set_status() on reset failure Xie Yongji
2021-08-03  8:10   ` Jason Wang
2021-08-03  9:50     ` Yongji Xie
2021-08-04  8:33       ` Jason Wang
2021-07-29  7:34 ` [PATCH v10 06/17] vhost-vdpa: Handle the failure of vdpa_reset() Xie Yongji
2021-07-29  7:34 ` [PATCH v10 07/17] virtio: Don't set FAILED status bit on device index allocation failure Xie Yongji
2021-08-03  8:02   ` Jason Wang
2021-08-03  9:17     ` Yongji Xie
2021-07-29  7:34 ` [PATCH v10 08/17] virtio_config: Add a return value to reset function Xie Yongji
2021-07-29  7:34 ` [PATCH v10 09/17] virtio-vdpa: Handle the failure of vdpa_reset() Xie Yongji
2021-07-29  7:34 ` [PATCH v10 10/17] virtio: Handle device reset failure in register_virtio_device() Xie Yongji
2021-08-03  8:09   ` Jason Wang
2021-08-03  9:38     ` Yongji Xie
2021-08-04  8:32       ` Jason Wang
2021-08-04  8:50         ` Yongji Xie
2021-08-04  8:54           ` Jason Wang
2021-08-04  9:07             ` Yongji Xie
2021-08-05  7:12               ` Jason Wang
2021-07-29  7:34 ` [PATCH v10 11/17] vhost-iotlb: Add an opaque pointer for vhost IOTLB Xie Yongji
2021-07-29  7:34 ` [PATCH v10 12/17] vdpa: Add an opaque pointer for vdpa_config_ops.dma_map() Xie Yongji
2021-07-29  7:34 ` [PATCH v10 13/17] vdpa: factor out vhost_vdpa_pa_map() and vhost_vdpa_pa_unmap() Xie Yongji
2021-07-29  7:35 ` [PATCH v10 14/17] vdpa: Support transferring virtual addressing during DMA mapping Xie Yongji
2021-07-29  7:35 ` [PATCH v10 15/17] vduse: Implement an MMU-based software IOTLB Xie Yongji
2021-07-29  7:35 ` [PATCH v10 16/17] vduse: Introduce VDUSE - vDPA Device in Userspace Xie Yongji
2021-07-29  9:00   ` Greg KH
2021-07-29  9:57     ` Yongji Xie [this message]
2021-08-03  7:30   ` Jason Wang
2021-08-03  8:39     ` Yongji Xie
2021-07-29  7:35 ` [PATCH v10 17/17] Documentation: Add documentation for VDUSE Xie Yongji
2021-08-03  7:35   ` Jason Wang
2021-08-03  8:52     ` Yongji Xie

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CACycT3vDspiXSh=UoK9JXaMpv1+9C61DLy_-bWJV5XRxKs2xRw@mail.gmail.com' \
    --to=xieyongji@bytedance.com \
    --cc=axboe@kernel.dk \
    --cc=bcrl@kvack.org \
    --cc=christian.brauner@canonical.com \
    --cc=corbet@lwn.net \
    --cc=dan.carpenter@oracle.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hch@infradead.org \
    --cc=iommu@lists.linux-foundation.org \
    --cc=jasowang@redhat.com \
    --cc=joe@perches.com \
    --cc=joro@8bytes.org \
    --cc=kvm@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mika.penttila@nextfour.com \
    --cc=mst@redhat.com \
    --cc=netdev@vger.kernel.org \
    --cc=parav@nvidia.com \
    --cc=rdunlap@infradead.org \
    --cc=sgarzare@redhat.com \
    --cc=songmuchun@bytedance.com \
    --cc=stefanha@redhat.com \
    --cc=viro@zeniv.linux.org.uk \
    --cc=virtualization@lists.linux-foundation.org \
    --cc=willy@infradead.org \
    --cc=xiaodong.liu@intel.com \
    --cc=zhe.he@windriver.com \
    --subject='Re: [PATCH v10 16/17] vduse: Introduce VDUSE - vDPA Device in Userspace' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox