* [PATCH] drm/virtio: Add window server support
From: Tomeu Vizoso @ 2017-12-14 12:43 UTC (permalink / raw)
To: dri-devel
Cc: Tomeu Vizoso, Michael S. Tsirkin, David Airlie, linux-kernel,
virtualization, Zach Reizner
This is to allow clients running within VMs to be able to communicate
with a compositor in the host. Clients will use the communication
protocol that the compositor supports, and virtio-gpu will assist with
making buffers available in both sides, and copying content as needed.
It is expected that a service in the guest will act as a proxy,
interacting with virtio-gpu to support unmodified clients. For some
features of the protocol, the hypervisor might have to intervene and
also parse the protocol data to properly bridge resources. The following
IOCTLs have been added to this effect:
*_WINSRV_CONNECT: Opens a connection to the compositor in the host,
returns a FD that represents this connection and on which the following
IOCTLs can be used. Callers are expected to poll this FD for new
messages from the compositor.
*_WINSRV_TX: Asks the hypervisor to forward a message to the compositor
*_WINSRV_RX: Returns all queued messages
Alongside protocol data that is opaque to the kernel, the client can
send file descriptors that reference GEM buffers allocated by
virtio-gpu. The guest proxy is expected to figure out when a client is
passing a FD that refers to shared memory in the guest and allocate a
virtio-gpu buffer of the same size with DRM_VIRTGPU_RESOURCE_CREATE.
When the client notifies the compositor that it can read from that buffer,
the proxy should copy the contents from the SHM region to the virtio-gpu
resource and call DRM_VIRTGPU_TRANSFER_TO_HOST.
This has been tested with Wayland clients that make use of wl_shm to
pass buffers to the compositor, but is expected to work similarly for X
clients that make use of MIT-SHM with FD passing.
Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Cc: Zach Reizner <zachr@google.com>
---
Hi,
this work is based on the virtio_wl driver in the ChromeOS kernel by
Zach Reizner, currently at:
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.4/drivers/virtio/virtio_wl.c
There's two features missing in this patch when compared with virtio_wl:
* Allow the guest access directly host memory, without having to resort
to TRANSFER_TO_HOST
* Pass FDs from host to guest (Wayland specifies that the compositor
shares keyboard data with the guest via a shared buffer)
I plan to work on this next, but I would like to get some comments on
the general approach so I can better choose which patch to follow.
Thanks,
Tomeu
---
drivers/gpu/drm/virtio/virtgpu_drv.h | 39 ++++-
drivers/gpu/drm/virtio/virtgpu_ioctl.c | 168 +++++++++++++++++++
drivers/gpu/drm/virtio/virtgpu_kms.c | 58 +++++--
drivers/gpu/drm/virtio/virtgpu_vq.c | 283 +++++++++++++++++++++++++++++++++
include/uapi/drm/virtgpu_drm.h | 29 ++++
include/uapi/linux/virtio_gpu.h | 39 +++++
6 files changed, 602 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index da2fb585fea4..268b386e1232 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -178,6 +178,8 @@ struct virtio_gpu_device {
struct virtio_gpu_queue ctrlq;
struct virtio_gpu_queue cursorq;
+ struct virtio_gpu_queue winsrv_rxq;
+ struct virtio_gpu_queue winsrv_txq;
struct kmem_cache *vbufs;
bool vqs_ready;
@@ -205,10 +207,32 @@ struct virtio_gpu_device {
struct virtio_gpu_fpriv {
uint32_t ctx_id;
+
+ struct list_head winsrv_conns; /* list of virtio_gpu_winsrv_conn */
+ spinlock_t winsrv_lock;
+};
+
+struct virtio_gpu_winsrv_rx_qentry {
+ struct virtio_gpu_winsrv_rx *cmd;
+ struct list_head next;
+};
+
+struct virtio_gpu_winsrv_conn {
+ struct virtio_gpu_device *vgdev;
+
+ spinlock_t lock;
+
+ int fd;
+ struct drm_file *drm_file;
+
+ struct list_head cmdq; /* queue of virtio_gpu_winsrv_rx_qentry */
+ wait_queue_head_t cmdwq;
+
+ struct list_head next;
};
/* virtio_ioctl.c */
-#define DRM_VIRTIO_NUM_IOCTLS 10
+#define DRM_VIRTIO_NUM_IOCTLS 11
extern struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS];
/* virtio_kms.c */
@@ -318,9 +342,22 @@ virtio_gpu_cmd_resource_create_3d(struct virtio_gpu_device *vgdev,
void virtio_gpu_ctrl_ack(struct virtqueue *vq);
void virtio_gpu_cursor_ack(struct virtqueue *vq);
void virtio_gpu_fence_ack(struct virtqueue *vq);
+void virtio_gpu_winsrv_tx_ack(struct virtqueue *vq);
+void virtio_gpu_winsrv_rx_read(struct virtqueue *vq);
void virtio_gpu_dequeue_ctrl_func(struct work_struct *work);
void virtio_gpu_dequeue_cursor_func(struct work_struct *work);
+void virtio_gpu_dequeue_winsrv_rx_func(struct work_struct *work);
+void virtio_gpu_dequeue_winsrv_tx_func(struct work_struct *work);
void virtio_gpu_dequeue_fence_func(struct work_struct *work);
+void virtio_gpu_fill_winsrv_rx(struct virtio_gpu_device *vgdev);
+void virtio_gpu_queue_winsrv_rx_in(struct virtio_gpu_device *vgdev,
+ struct virtio_gpu_winsrv_rx *cmd);
+int virtio_gpu_cmd_winsrv_connect(struct virtio_gpu_device *vgdev, int fd);
+void virtio_gpu_cmd_winsrv_disconnect(struct virtio_gpu_device *vgdev, int fd);
+int virtio_gpu_cmd_winsrv_tx(struct virtio_gpu_device *vgdev,
+ const char __user *buffer, u32 len,
+ int *fds, struct virtio_gpu_winsrv_conn *conn,
+ bool nonblock);
/* virtio_gpu_display.c */
int virtio_gpu_framebuffer_init(struct drm_device *dev,
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index 0528edb4a2bf..2571cdafd594 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -25,6 +25,9 @@
* OTHER DEALINGS IN THE SOFTWARE.
*/
+#include <linux/anon_inodes.h>
+#include <linux/syscalls.h>
+
#include <drm/drmP.h>
#include <drm/virtgpu_drm.h>
#include <drm/ttm/ttm_execbuf_util.h>
@@ -527,6 +530,168 @@ static int virtio_gpu_get_caps_ioctl(struct drm_device *dev,
return 0;
}
+static unsigned int winsrv_poll(struct file *filp,
+ struct poll_table_struct *wait)
+{
+ struct virtio_gpu_winsrv_conn *conn = filp->private_data;
+ unsigned int mask = 0;
+
+ spin_lock(&conn->lock);
+ poll_wait(filp, &conn->cmdwq, wait);
+ if (!list_empty(&conn->cmdq))
+ mask |= POLLIN | POLLRDNORM;
+ spin_unlock(&conn->lock);
+
+ return mask;
+}
+
+static int winsrv_ioctl_rx(struct virtio_gpu_device *vgdev,
+ struct virtio_gpu_winsrv_conn *conn,
+ struct drm_virtgpu_winsrv *cmd)
+{
+ struct virtio_gpu_winsrv_rx_qentry *qentry, *tmp;
+ struct virtio_gpu_winsrv_rx *virtio_cmd;
+ int available_len = cmd->len;
+ int read_count = 0;
+
+ list_for_each_entry_safe(qentry, tmp, &conn->cmdq, next) {
+ virtio_cmd = qentry->cmd;
+ if (virtio_cmd->len > available_len)
+ return 0;
+
+ if (copy_to_user((void *)cmd->data + read_count,
+ virtio_cmd->data,
+ virtio_cmd->len)) {
+ /* return error unless we have some data to return */
+ if (read_count == 0)
+ return -EFAULT;
+ }
+
+ /*
+ * here we could export resource IDs to FDs, but no protocol
+ * as of today requires it
+ */
+
+ available_len -= virtio_cmd->len;
+ read_count += virtio_cmd->len;
+
+ virtio_gpu_queue_winsrv_rx_in(vgdev, virtio_cmd);
+
+ list_del(&qentry->next);
+ kfree(qentry);
+ }
+
+ cmd->len = read_count;
+
+ return 0;
+}
+
+static long winsrv_ioctl(struct file *filp, unsigned int cmd,
+ unsigned long arg)
+{
+ struct virtio_gpu_winsrv_conn *conn = filp->private_data;
+ struct virtio_gpu_device *vgdev = conn->vgdev;
+ struct drm_virtgpu_winsrv winsrv_cmd;
+ int ret;
+
+ if (_IOC_SIZE(cmd) > sizeof(winsrv_cmd))
+ return -EINVAL;
+
+ if (copy_from_user(&winsrv_cmd, (void __user *)arg,
+ _IOC_SIZE(cmd)) != 0)
+ return -EFAULT;
+
+ switch (cmd) {
+ case DRM_IOCTL_VIRTGPU_WINSRV_RX:
+ ret = winsrv_ioctl_rx(vgdev, conn, &winsrv_cmd);
+ if (copy_to_user((void __user *)arg, &winsrv_cmd,
+ _IOC_SIZE(cmd)) != 0)
+ return -EFAULT;
+
+ break;
+
+ case DRM_IOCTL_VIRTGPU_WINSRV_TX:
+ ret = virtio_gpu_cmd_winsrv_tx(vgdev,
+ (const char __user *) winsrv_cmd.data,
+ winsrv_cmd.len,
+ winsrv_cmd.fds,
+ conn,
+ filp->f_flags & O_NONBLOCK);
+ break;
+ default:
+ ret = -EINVAL;
+ }
+
+ return ret;
+}
+
+static int winsrv_release(struct inode *inodep, struct file *filp)
+{
+ struct virtio_gpu_winsrv_conn *conn = filp->private_data;
+ struct virtio_gpu_device *vgdev = conn->vgdev;
+
+ virtio_gpu_cmd_winsrv_disconnect(vgdev, conn->fd);
+
+ list_del(&conn->next);
+ kfree(conn);
+
+ return 0;
+}
+
+static const struct file_operations winsrv_fops = {
+
+ .poll = winsrv_poll,
+ .unlocked_ioctl = winsrv_ioctl,
+ .release = winsrv_release,
+};
+
+static int virtio_gpu_winsrv_connect(struct drm_device *dev, void *data,
+ struct drm_file *file)
+{
+ struct virtio_gpu_device *vgdev = dev->dev_private;
+ struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
+ struct drm_virtgpu_winsrv_connect *args = data;
+ struct virtio_gpu_winsrv_conn *conn;
+ int ret;
+
+ conn = kzalloc(sizeof(*conn), GFP_KERNEL);
+ if (!conn)
+ return -ENOMEM;
+
+ conn->vgdev = vgdev;
+ conn->drm_file = file;
+ spin_lock_init(&conn->lock);
+ INIT_LIST_HEAD(&conn->cmdq);
+ init_waitqueue_head(&conn->cmdwq);
+
+ ret = anon_inode_getfd("[virtgpu_winsrv]", &winsrv_fops, conn,
+ O_CLOEXEC | O_RDWR);
+ if (ret < 0)
+ goto free_conn;
+
+ conn->fd = ret;
+
+ ret = virtio_gpu_cmd_winsrv_connect(vgdev, conn->fd);
+ if (ret < 0)
+ goto close_fd;
+
+ spin_lock(&vfpriv->winsrv_lock);
+ list_add_tail(&conn->next, &vfpriv->winsrv_conns);
+ spin_unlock(&vfpriv->winsrv_lock);
+
+ args->fd = conn->fd;
+
+ return 0;
+
+close_fd:
+ sys_close(conn->fd);
+
+free_conn:
+ kfree(conn);
+
+ return ret;
+}
+
struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS] = {
DRM_IOCTL_DEF_DRV(VIRTGPU_MAP, virtio_gpu_map_ioctl,
DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW),
@@ -558,4 +723,7 @@ struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS] = {
DRM_IOCTL_DEF_DRV(VIRTGPU_GET_CAPS, virtio_gpu_get_caps_ioctl,
DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW),
+
+ DRM_IOCTL_DEF_DRV(VIRTGPU_WINSRV_CONNECT, virtio_gpu_winsrv_connect,
+ DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW),
};
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 6400506a06b0..ad7872037982 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -128,13 +128,15 @@ static void virtio_gpu_get_capsets(struct virtio_gpu_device *vgdev,
int virtio_gpu_driver_load(struct drm_device *dev, unsigned long flags)
{
static vq_callback_t *callbacks[] = {
- virtio_gpu_ctrl_ack, virtio_gpu_cursor_ack
+ virtio_gpu_ctrl_ack, virtio_gpu_cursor_ack,
+ virtio_gpu_winsrv_rx_read, virtio_gpu_winsrv_tx_ack
};
- static const char * const names[] = { "control", "cursor" };
+ static const char * const names[] = { "control", "cursor",
+ "winsrv-rx", "winsrv-tx" };
struct virtio_gpu_device *vgdev;
/* this will expand later */
- struct virtqueue *vqs[2];
+ struct virtqueue *vqs[4];
u32 num_scanouts, num_capsets;
int ret;
@@ -158,6 +160,10 @@ int virtio_gpu_driver_load(struct drm_device *dev, unsigned long flags)
init_waitqueue_head(&vgdev->resp_wq);
virtio_gpu_init_vq(&vgdev->ctrlq, virtio_gpu_dequeue_ctrl_func);
virtio_gpu_init_vq(&vgdev->cursorq, virtio_gpu_dequeue_cursor_func);
+ virtio_gpu_init_vq(&vgdev->winsrv_rxq,
+ virtio_gpu_dequeue_winsrv_rx_func);
+ virtio_gpu_init_vq(&vgdev->winsrv_txq,
+ virtio_gpu_dequeue_winsrv_tx_func);
vgdev->fence_drv.context = dma_fence_context_alloc(1);
spin_lock_init(&vgdev->fence_drv.lock);
@@ -175,13 +181,15 @@ int virtio_gpu_driver_load(struct drm_device *dev, unsigned long flags)
DRM_INFO("virgl 3d acceleration not supported by guest\n");
#endif
- ret = virtio_find_vqs(vgdev->vdev, 2, vqs, callbacks, names, NULL);
+ ret = virtio_find_vqs(vgdev->vdev, 4, vqs, callbacks, names, NULL);
if (ret) {
DRM_ERROR("failed to find virt queues\n");
goto err_vqs;
}
vgdev->ctrlq.vq = vqs[0];
vgdev->cursorq.vq = vqs[1];
+ vgdev->winsrv_rxq.vq = vqs[2];
+ vgdev->winsrv_txq.vq = vqs[3];
ret = virtio_gpu_alloc_vbufs(vgdev);
if (ret) {
DRM_ERROR("failed to alloc vbufs\n");
@@ -215,6 +223,9 @@ int virtio_gpu_driver_load(struct drm_device *dev, unsigned long flags)
goto err_modeset;
virtio_device_ready(vgdev->vdev);
+
+ virtio_gpu_fill_winsrv_rx(vgdev);
+
vgdev->vqs_ready = true;
if (num_capsets)
@@ -256,6 +267,8 @@ void virtio_gpu_driver_unload(struct drm_device *dev)
vgdev->vqs_ready = false;
flush_work(&vgdev->ctrlq.dequeue_work);
flush_work(&vgdev->cursorq.dequeue_work);
+ flush_work(&vgdev->winsrv_rxq.dequeue_work);
+ flush_work(&vgdev->winsrv_txq.dequeue_work);
flush_work(&vgdev->config_changed_work);
vgdev->vdev->config->del_vqs(vgdev->vdev);
@@ -274,25 +287,43 @@ int virtio_gpu_driver_open(struct drm_device *dev, struct drm_file *file)
uint32_t id;
char dbgname[64], tmpname[TASK_COMM_LEN];
- /* can't create contexts without 3d renderer */
- if (!vgdev->has_virgl_3d)
- return 0;
-
- get_task_comm(tmpname, current);
- snprintf(dbgname, sizeof(dbgname), "%s", tmpname);
- dbgname[63] = 0;
/* allocate a virt GPU context for this opener */
vfpriv = kzalloc(sizeof(*vfpriv), GFP_KERNEL);
if (!vfpriv)
return -ENOMEM;
- virtio_gpu_context_create(vgdev, strlen(dbgname), dbgname, &id);
+ /* can't create contexts without 3d renderer */
+ if (vgdev->has_virgl_3d) {
+ get_task_comm(tmpname, current);
+ snprintf(dbgname, sizeof(dbgname), "%s", tmpname);
+ dbgname[63] = 0;
+
+ virtio_gpu_context_create(vgdev, strlen(dbgname), dbgname, &id);
+
+ vfpriv->ctx_id = id;
+ }
+
+ spin_lock_init(&vfpriv->winsrv_lock);
+ INIT_LIST_HEAD(&vfpriv->winsrv_conns);
- vfpriv->ctx_id = id;
file->driver_priv = vfpriv;
+
return 0;
}
+static void virtio_gpu_cleanup_conns(struct virtio_gpu_fpriv *vfpriv)
+{
+ struct virtio_gpu_winsrv_conn *conn, *tmp;
+ struct virtio_gpu_winsrv_rx_qentry *qentry, *tmp2;
+
+ list_for_each_entry_safe(conn, tmp, &vfpriv->winsrv_conns, next) {
+ list_for_each_entry_safe(qentry, tmp2, &conn->cmdq, next) {
+ kfree(qentry);
+ }
+ kfree(conn);
+ }
+}
+
void virtio_gpu_driver_postclose(struct drm_device *dev, struct drm_file *file)
{
struct virtio_gpu_device *vgdev = dev->dev_private;
@@ -303,6 +334,7 @@ void virtio_gpu_driver_postclose(struct drm_device *dev, struct drm_file *file)
vfpriv = file->driver_priv;
+ virtio_gpu_cleanup_conns(vfpriv);
virtio_gpu_context_destroy(vgdev, vfpriv->ctx_id);
kfree(vfpriv);
file->driver_priv = NULL;
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index 9eb96fb2c147..93f2f86c19dd 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -72,6 +72,67 @@ void virtio_gpu_cursor_ack(struct virtqueue *vq)
schedule_work(&vgdev->cursorq.dequeue_work);
}
+void virtio_gpu_winsrv_rx_read(struct virtqueue *vq)
+{
+ struct drm_device *dev = vq->vdev->priv;
+ struct virtio_gpu_device *vgdev = dev->dev_private;
+
+ schedule_work(&vgdev->winsrv_rxq.dequeue_work);
+}
+
+void virtio_gpu_winsrv_tx_ack(struct virtqueue *vq)
+{
+ struct drm_device *dev = vq->vdev->priv;
+ struct virtio_gpu_device *vgdev = dev->dev_private;
+
+ schedule_work(&vgdev->winsrv_txq.dequeue_work);
+}
+
+void virtio_gpu_queue_winsrv_rx_in(struct virtio_gpu_device *vgdev,
+ struct virtio_gpu_winsrv_rx *cmd)
+{
+ struct virtqueue *vq = vgdev->winsrv_rxq.vq;
+ struct scatterlist sg[1];
+ int ret;
+
+ sg_init_one(sg, cmd, sizeof(*cmd));
+
+ spin_lock(&vgdev->winsrv_rxq.qlock);
+retry:
+ ret = virtqueue_add_inbuf(vq, sg, 1, cmd, GFP_KERNEL);
+ if (ret == -ENOSPC) {
+ spin_unlock(&vgdev->winsrv_rxq.qlock);
+ wait_event(vgdev->winsrv_rxq.ack_queue, vq->num_free);
+ spin_lock(&vgdev->winsrv_rxq.qlock);
+ goto retry;
+ }
+ virtqueue_kick(vq);
+ spin_unlock(&vgdev->winsrv_rxq.qlock);
+}
+
+void virtio_gpu_fill_winsrv_rx(struct virtio_gpu_device *vgdev)
+{
+ struct virtqueue *vq = vgdev->winsrv_rxq.vq;
+ struct virtio_gpu_winsrv_rx *cmd;
+ int ret = 0;
+
+ while (vq->num_free > 0) {
+ cmd = kmalloc(sizeof(*cmd), GFP_KERNEL);
+ if (!cmd) {
+ ret = -ENOMEM;
+ goto clear_queue;
+ }
+
+ virtio_gpu_queue_winsrv_rx_in(vgdev, cmd);
+ }
+
+ return;
+
+clear_queue:
+ while ((cmd = virtqueue_detach_unused_buf(vq)))
+ kfree(cmd);
+}
+
int virtio_gpu_alloc_vbufs(struct virtio_gpu_device *vgdev)
{
vgdev->vbufs = kmem_cache_create("virtio-gpu-vbufs",
@@ -258,6 +319,96 @@ void virtio_gpu_dequeue_cursor_func(struct work_struct *work)
wake_up(&vgdev->cursorq.ack_queue);
}
+void virtio_gpu_dequeue_winsrv_tx_func(struct work_struct *work)
+{
+ struct virtio_gpu_device *vgdev =
+ container_of(work, struct virtio_gpu_device,
+ winsrv_txq.dequeue_work);
+ struct virtio_gpu_vbuffer *vbuf;
+ int len;
+
+ spin_lock(&vgdev->winsrv_txq.qlock);
+ do {
+ while ((vbuf = virtqueue_get_buf(vgdev->winsrv_txq.vq, &len)))
+ free_vbuf(vgdev, vbuf);
+ } while (!virtqueue_enable_cb(vgdev->winsrv_txq.vq));
+ spin_unlock(&vgdev->winsrv_txq.qlock);
+
+ wake_up(&vgdev->winsrv_txq.ack_queue);
+}
+
+static struct virtio_gpu_winsrv_conn *find_conn(struct virtio_gpu_device *vgdev,
+ int fd)
+{
+ struct virtio_gpu_winsrv_conn *conn;
+ struct drm_device *ddev = vgdev->ddev;
+ struct drm_file *file;
+ struct virtio_gpu_fpriv *vfpriv;
+
+ mutex_lock(&ddev->filelist_mutex);
+ list_for_each_entry(file, &ddev->filelist, lhead) {
+ vfpriv = file->driver_priv;
+ spin_lock(&vfpriv->winsrv_lock);
+ list_for_each_entry(conn, &vfpriv->winsrv_conns, next) {
+ if (conn->fd == fd) {
+ spin_lock(&conn->lock);
+ spin_unlock(&vfpriv->winsrv_lock);
+ mutex_unlock(&ddev->filelist_mutex);
+ return conn;
+ }
+ }
+ spin_unlock(&vfpriv->winsrv_lock);
+ }
+ mutex_unlock(&ddev->filelist_mutex);
+
+ return NULL;
+}
+
+static void handle_rx_cmd(struct virtio_gpu_device *vgdev,
+ struct virtio_gpu_winsrv_rx *cmd)
+{
+ struct virtio_gpu_winsrv_conn *conn;
+ struct virtio_gpu_winsrv_rx_qentry *qentry;
+
+ conn = find_conn(vgdev, cmd->client_fd);
+ if (!conn) {
+ DRM_DEBUG("recv for unknown client fd %u\n", cmd->client_fd);
+ return;
+ }
+
+ qentry = kzalloc(sizeof(*qentry), GFP_KERNEL);
+ if (!qentry) {
+ spin_unlock(&conn->lock);
+ DRM_DEBUG("failed to allocate qentry for winsrv connection\n");
+ return;
+ }
+
+ qentry->cmd = cmd;
+
+ list_add_tail(&qentry->next, &conn->cmdq);
+ wake_up_interruptible(&conn->cmdwq);
+ spin_unlock(&conn->lock);
+}
+
+void virtio_gpu_dequeue_winsrv_rx_func(struct work_struct *work)
+{
+ struct virtio_gpu_device *vgdev =
+ container_of(work, struct virtio_gpu_device,
+ winsrv_rxq.dequeue_work);
+ struct virtio_gpu_winsrv_rx *cmd;
+ unsigned int len;
+
+ spin_lock(&vgdev->winsrv_rxq.qlock);
+ while ((cmd = virtqueue_get_buf(vgdev->winsrv_rxq.vq, &len)) != NULL) {
+ spin_unlock(&vgdev->winsrv_rxq.qlock);
+ handle_rx_cmd(vgdev, cmd);
+ spin_lock(&vgdev->winsrv_rxq.qlock);
+ }
+ spin_unlock(&vgdev->winsrv_rxq.qlock);
+
+ virtqueue_kick(vgdev->winsrv_rxq.vq);
+}
+
static int virtio_gpu_queue_ctrl_buffer_locked(struct virtio_gpu_device *vgdev,
struct virtio_gpu_vbuffer *vbuf)
__releases(&vgdev->ctrlq.qlock)
@@ -380,6 +531,41 @@ static int virtio_gpu_queue_cursor(struct virtio_gpu_device *vgdev,
return ret;
}
+static int virtio_gpu_queue_winsrv_tx(struct virtio_gpu_device *vgdev,
+ struct virtio_gpu_vbuffer *vbuf)
+{
+ struct virtqueue *vq = vgdev->winsrv_txq.vq;
+ struct scatterlist *sgs[2], vcmd, vout;
+ int ret;
+
+ if (!vgdev->vqs_ready)
+ return -ENODEV;
+
+ sg_init_one(&vcmd, vbuf->buf, vbuf->size);
+ sgs[0] = &vcmd;
+
+ sg_init_one(&vout, vbuf->data_buf, vbuf->data_size);
+ sgs[1] = &vout;
+
+ spin_lock(&vgdev->winsrv_txq.qlock);
+retry:
+ ret = virtqueue_add_sgs(vq, sgs, 2, 0, vbuf, GFP_ATOMIC);
+ if (ret == -ENOSPC) {
+ spin_unlock(&vgdev->winsrv_txq.qlock);
+ wait_event(vgdev->winsrv_txq.ack_queue, vq->num_free);
+ spin_lock(&vgdev->winsrv_txq.qlock);
+ goto retry;
+ }
+
+ virtqueue_kick(vq);
+
+ spin_unlock(&vgdev->winsrv_txq.qlock);
+
+ if (!ret)
+ ret = vq->num_free;
+ return ret;
+}
+
/* just create gem objects for userspace and long lived objects,
just use dma_alloced pages for the queue objects? */
@@ -890,3 +1076,100 @@ void virtio_gpu_cursor_ping(struct virtio_gpu_device *vgdev,
memcpy(cur_p, &output->cursor, sizeof(output->cursor));
virtio_gpu_queue_cursor(vgdev, vbuf);
}
+
+int virtio_gpu_cmd_winsrv_connect(struct virtio_gpu_device *vgdev, int fd)
+{
+ struct virtio_gpu_winsrv_connect *cmd_p;
+ struct virtio_gpu_vbuffer *vbuf;
+
+ cmd_p = virtio_gpu_alloc_cmd(vgdev, &vbuf, sizeof(*cmd_p));
+ memset(cmd_p, 0, sizeof(*cmd_p));
+
+ cmd_p->hdr.type = cpu_to_le32(VIRTIO_GPU_CMD_WINSRV_CONNECT);
+ cmd_p->client_fd = cpu_to_le32(fd);
+ return virtio_gpu_queue_ctrl_buffer(vgdev, vbuf);
+}
+
+void virtio_gpu_cmd_winsrv_disconnect(struct virtio_gpu_device *vgdev, int fd)
+{
+ struct virtio_gpu_winsrv_disconnect *cmd_p;
+ struct virtio_gpu_vbuffer *vbuf;
+
+ cmd_p = virtio_gpu_alloc_cmd(vgdev, &vbuf, sizeof(*cmd_p));
+ memset(cmd_p, 0, sizeof(*cmd_p));
+
+ cmd_p->hdr.type = cpu_to_le32(VIRTIO_GPU_CMD_WINSRV_DISCONNECT);
+ cmd_p->client_fd = cpu_to_le32(fd);
+ virtio_gpu_queue_ctrl_buffer(vgdev, vbuf);
+}
+
+int virtio_gpu_cmd_winsrv_tx(struct virtio_gpu_device *vgdev,
+ const char __user *buffer, u32 len,
+ int *fds, struct virtio_gpu_winsrv_conn *conn,
+ bool nonblock)
+{
+ int client_fd = conn->fd;
+ struct drm_file *file = conn->drm_file;
+ struct virtio_gpu_winsrv_tx *cmd_p;
+ struct virtio_gpu_vbuffer *vbuf;
+ uint32_t gem_handle;
+ struct drm_gem_object *gobj = NULL;
+ struct virtio_gpu_object *qobj = NULL;
+ int ret, i, fd;
+
+ cmd_p = virtio_gpu_alloc_cmd(vgdev, &vbuf, sizeof(*cmd_p));
+ memset(cmd_p, 0, sizeof(*cmd_p));
+
+ cmd_p->hdr.type = cpu_to_le32(VIRTIO_GPU_CMD_WINSRV_TX);
+
+ for (i = 0; i < VIRTIO_GPU_WINSRV_MAX_ALLOCS; i++) {
+ cmd_p->resource_ids[i] = -1;
+
+ fd = fds[i];
+ if (fd < 0)
+ break;
+
+ ret = drm_gem_prime_fd_to_handle(vgdev->ddev, file, fd,
+ &gem_handle);
+ if (ret != 0)
+ goto err_free_vbuf;
+
+ gobj = drm_gem_object_lookup(file, gem_handle);
+ if (gobj == NULL) {
+ ret = -ENOENT;
+ goto err_free_vbuf;
+ }
+
+ qobj = gem_to_virtio_gpu_obj(gobj);
+ cmd_p->resource_ids[i] = qobj->hw_res_handle;
+ }
+
+ cmd_p->client_fd = client_fd;
+ cmd_p->len = cpu_to_le32(len);
+
+ /* gets freed when the ring has consumed it */
+ vbuf->data_buf = kmalloc(cmd_p->len, GFP_KERNEL);
+ if (!vbuf->data_buf) {
+ DRM_ERROR("failed to allocate winsrv tx buffer\n");
+ ret = -ENOMEM;
+ goto err_free_vbuf;
+ }
+
+ vbuf->data_size = cmd_p->len;
+
+ if (copy_from_user(vbuf->data_buf, buffer, cmd_p->len)) {
+ ret = -EFAULT;
+ goto err_free_databuf;
+ }
+
+ virtio_gpu_queue_winsrv_tx(vgdev, vbuf);
+
+ return 0;
+
+err_free_databuf:
+ kfree(vbuf->data_buf);
+err_free_vbuf:
+ free_vbuf(vgdev, vbuf);
+
+ return ret;
+}
diff --git a/include/uapi/drm/virtgpu_drm.h b/include/uapi/drm/virtgpu_drm.h
index 91a31ffed828..89b0a1a707a7 100644
--- a/include/uapi/drm/virtgpu_drm.h
+++ b/include/uapi/drm/virtgpu_drm.h
@@ -46,6 +46,11 @@ extern "C" {
#define DRM_VIRTGPU_TRANSFER_TO_HOST 0x07
#define DRM_VIRTGPU_WAIT 0x08
#define DRM_VIRTGPU_GET_CAPS 0x09
+#define DRM_VIRTGPU_WINSRV_CONNECT 0x0a
+#define DRM_VIRTGPU_WINSRV_TX 0x0b
+#define DRM_VIRTGPU_WINSRV_RX 0x0c
+
+#define VIRTGPU_WINSRV_MAX_ALLOCS 28
struct drm_virtgpu_map {
__u64 offset; /* use for mmap system call */
@@ -132,6 +137,18 @@ struct drm_virtgpu_get_caps {
__u32 pad;
};
+struct drm_virtgpu_winsrv {
+ __s32 fds[VIRTGPU_WINSRV_MAX_ALLOCS];
+ __u64 data;
+ __u32 len;
+ __u32 pad;
+};
+
+struct drm_virtgpu_winsrv_connect {
+ __u32 fd; /* returned by kernel */
+ __u32 pad;
+};
+
#define DRM_IOCTL_VIRTGPU_MAP \
DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_MAP, struct drm_virtgpu_map)
@@ -167,6 +184,18 @@ struct drm_virtgpu_get_caps {
DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_GET_CAPS, \
struct drm_virtgpu_get_caps)
+#define DRM_IOCTL_VIRTGPU_WINSRV_CONNECT \
+ DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_WINSRV_CONNECT, \
+ struct drm_virtgpu_winsrv_connect)
+
+#define DRM_IOCTL_VIRTGPU_WINSRV_TX \
+ DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_WINSRV_TX, \
+ struct drm_virtgpu_winsrv)
+
+#define DRM_IOCTL_VIRTGPU_WINSRV_RX \
+ DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_WINSRV_RX, \
+ struct drm_virtgpu_winsrv)
+
#if defined(__cplusplus)
}
#endif
diff --git a/include/uapi/linux/virtio_gpu.h b/include/uapi/linux/virtio_gpu.h
index 4b04ead26cd9..1be9cf203ab1 100644
--- a/include/uapi/linux/virtio_gpu.h
+++ b/include/uapi/linux/virtio_gpu.h
@@ -71,6 +71,12 @@ enum virtio_gpu_ctrl_type {
VIRTIO_GPU_CMD_UPDATE_CURSOR = 0x0300,
VIRTIO_GPU_CMD_MOVE_CURSOR,
+ /* window server commands */
+ VIRTIO_GPU_CMD_WINSRV_CONNECT = 0x0400,
+ VIRTIO_GPU_CMD_WINSRV_DISCONNECT,
+ VIRTIO_GPU_CMD_WINSRV_TX,
+ VIRTIO_GPU_CMD_WINSRV_RX,
+
/* success responses */
VIRTIO_GPU_RESP_OK_NODATA = 0x1100,
VIRTIO_GPU_RESP_OK_DISPLAY_INFO,
@@ -290,6 +296,39 @@ struct virtio_gpu_resp_capset {
__u8 capset_data[];
};
+/* VIRTIO_GPU_CMD_WINSRV_CONNECT */
+struct virtio_gpu_winsrv_connect {
+ struct virtio_gpu_ctrl_hdr hdr;
+ __le32 client_fd;
+};
+
+/* VIRTIO_GPU_CMD_WINSRV_DISCONNECT */
+struct virtio_gpu_winsrv_disconnect {
+ struct virtio_gpu_ctrl_hdr hdr;
+ __le32 client_fd;
+};
+
+#define VIRTIO_GPU_WINSRV_MAX_ALLOCS 28
+#define VIRTIO_GPU_WINSRV_TX_MAX_DATA 4096
+
+/* VIRTIO_GPU_CMD_WINSRV_TX */
+/* these commands are followed in the queue descriptor by protocol buffers */
+struct virtio_gpu_winsrv_tx {
+ struct virtio_gpu_ctrl_hdr hdr;
+ __le32 client_fd;
+ __le32 len;
+ __le32 resource_ids[VIRTIO_GPU_WINSRV_MAX_ALLOCS];
+};
+
+/* VIRTIO_GPU_CMD_WINSRV_RX */
+struct virtio_gpu_winsrv_rx {
+ struct virtio_gpu_ctrl_hdr hdr;
+ __le32 client_fd;
+ __u8 data[VIRTIO_GPU_WINSRV_TX_MAX_DATA];
+ __le32 len;
+ __le32 resource_ids[VIRTIO_GPU_WINSRV_MAX_ALLOCS];
+};
+
#define VIRTIO_GPU_EVENT_DISPLAY (1 << 0)
struct virtio_gpu_config {
--
2.14.3
^ permalink raw reply related
* Re: [PATCH v19 3/7] xbitmap: add more operations
From: Matthew Wilcox @ 2017-12-14 12:37 UTC (permalink / raw)
To: Wei Wang
Cc: yang.zhang.wz, kvm, mst, Tetsuo Handa, liliang.opensource,
qemu-devel, virtualization, linux-mm, aarcange, virtio-dev,
mawilcox, quan.xu, nilal, riel, cornelia.huck, mhocko,
linux-kernel, amit.shah, pbonzini, akpm, mgorman
In-Reply-To: <5A311C5E.7000304@intel.com>
On Wed, Dec 13, 2017 at 08:26:06PM +0800, Wei Wang wrote:
> On 12/12/2017 09:20 PM, Tetsuo Handa wrote:
> > Can you eliminate exception path and fold all xbitmap patches into one, and
> > post only one xbitmap patch without virtio-baloon changes? If exception path
> > is valuable, you can add exception path after minimum version is merged.
> > This series is too difficult for me to close corner cases.
>
> That exception path is claimed to save memory, and I don't have a strong
> reason to remove that part.
> Matthew, could we get your feedback on this?
Sure. This code is derived from the IDA code in lib/idr.c. Eventually,
I intend to reunite them. For IDA, it clearly makes sense; the first 62
entries result in allocating no memory at all, which is going to be 99%
of users. After that, we allocate 128 bytes which will serve the first
1024 users.
The xbitmap, as used by Wei's patches here is going to be used somewhat
differently from that. I understand why Tetsuo wants the exceptional
path removed; I'm not sure the gains will be as important. But if we're
going to rebuild the IDA on top of the xbitmap, we need to keep them.
I really want to pay more attention to this, but I need to focus on
getting the XArray finished.
^ permalink raw reply
* Re: [virtio-dev] Re: [PATCH v19 3/7] xbitmap: add more operations
From: Wei Wang @ 2017-12-14 11:47 UTC (permalink / raw)
To: Tetsuo Handa
Cc: yang.zhang.wz, kvm, mst, liliang.opensource, qemu-devel,
virtualization, linux-mm, aarcange, virtio-dev, mawilcox, willy,
quan.xu, nilal, riel, cornelia.huck, mhocko, linux-kernel,
amit.shah, pbonzini, akpm, mgorman
In-Reply-To: <5A31F445.6070504@intel.com>
On 12/14/2017 11:47 AM, Wei Wang wrote:
> On 12/13/2017 10:16 PM, Tetsuo Handa wrote:
>
>
>>
>>> if (set)
>>> ret = find_next_bit(&tmp,
>>> BITS_PER_LONG, ebit);
>>> else
>>> ret = find_next_zero_bit(&tmp,
>>> BITS_PER_LONG,
>>> ebit);
>>> if (ret < BITS_PER_LONG)
>>> return ret - 2 + ida_start;
>>> } else if (bitmap) {
>>> if (set)
>>> ret = find_next_bit(bitmap->bitmap,
>>> IDA_BITMAP_BITS, bit);
>>> else
>>> ret =
>>> find_next_zero_bit(bitmap->bitmap,
>>> IDA_BITMAP_BITS, bit);
>> "bit" may not be 0 for the first round and "bit" is always 0 afterwords.
>> But where is the guaranteed that "end" is a multiple of
>> IDA_BITMAP_BITS ?
>> Please explain why it is correct to use IDA_BITMAP_BITS unconditionally
>> for the last round.
>
> There missed something here, it will be:
>
> nbits = min(end - ida_start + 1, IDA_BITMAP_BITS - bit);
captured a bug here, should be:
nbits = min(end - ida_start + 1, (unsigned long)IDA_BITMAP_BITS);
> if (set)
> ret = find_next_bit(bitmap->bitmap, nbits, bit);
> else
> ret = find_next_zero_bit(bitmap->bitmap,
> nbits, bit);
> if (ret < nbits)
> return ret + ida_start;
>
>
Best,
Wei
^ permalink raw reply
* [PULL 1/1] virtio/s390: implement PM operations for virtio_ccw
From: Cornelia Huck @ 2017-12-14 9:59 UTC (permalink / raw)
To: mst; +Cc: linux-s390, kvm, Cornelia Huck, virtualization
In-Reply-To: <20171214095954.7615-1-cohuck@redhat.com>
From: Christian Borntraeger <borntraeger@de.ibm.com>
Suspend/Resume to/from disk currently fails. Let us wire
up the necessary callbacks. This is mostly just forwarding
the requests to the virtio drivers. The only thing that
has to be done in virtio_ccw itself is to re-set the
virtio revision.
Suggested-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
Message-Id: <20171207141102.70190-2-borntraeger@de.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
drivers/s390/virtio/virtio_ccw.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index b18fe2014cf2..330b3fa3430a 100644
--- a/drivers/s390/virtio/virtio_ccw.c
+++ b/drivers/s390/virtio/virtio_ccw.c
@@ -1300,6 +1300,9 @@ static int virtio_ccw_cio_notify(struct ccw_device *cdev, int event)
vcdev->device_lost = true;
rc = NOTIFY_DONE;
break;
+ case CIO_OPER:
+ rc = NOTIFY_OK;
+ break;
default:
rc = NOTIFY_DONE;
break;
@@ -1312,6 +1315,25 @@ static struct ccw_device_id virtio_ids[] = {
{},
};
+static int virtio_ccw_freeze(struct ccw_device *cdev)
+{
+ struct virtio_ccw_device *vcdev = dev_get_drvdata(&cdev->dev);
+
+ return virtio_device_freeze(&vcdev->vdev);
+}
+
+static int virtio_ccw_restore(struct ccw_device *cdev)
+{
+ struct virtio_ccw_device *vcdev = dev_get_drvdata(&cdev->dev);
+ int ret;
+
+ ret = virtio_ccw_set_transport_rev(vcdev);
+ if (ret)
+ return ret;
+
+ return virtio_device_restore(&vcdev->vdev);
+}
+
static struct ccw_driver virtio_ccw_driver = {
.driver = {
.owner = THIS_MODULE,
@@ -1324,6 +1346,9 @@ static struct ccw_driver virtio_ccw_driver = {
.set_online = virtio_ccw_online,
.notify = virtio_ccw_cio_notify,
.int_class = IRQIO_VIR,
+ .freeze = virtio_ccw_freeze,
+ .thaw = virtio_ccw_restore,
+ .restore = virtio_ccw_restore,
};
static int __init pure_hex(char **cp, unsigned int *val, int min_digit,
--
2.13.6
^ permalink raw reply related
* [PULL 0/1] s390/virtio update
From: Cornelia Huck @ 2017-12-14 9:59 UTC (permalink / raw)
To: mst; +Cc: linux-s390, kvm, Cornelia Huck, virtualization
The following changes since commit e073f74a5a39c6dc45f28a5006c21aa94490d9b8:
Merge branch 'this' into vhost (2017-12-07 18:39:24 +0200)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/kvms390/linux.git tags/virtio-s390-20171214
for you to fetch changes up to 619b4b0ba832144d4be899640a2047f9675df849:
virtio/s390: implement PM operations for virtio_ccw (2017-12-14 10:32:21 +0100)
----------------------------------------------------------------
Hibernation support for virtio-ccw.
----------------------------------------------------------------
Christian Borntraeger (1):
virtio/s390: implement PM operations for virtio_ccw
drivers/s390/virtio/virtio_ccw.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
--
2.13.6
^ permalink raw reply
* Re: [PATCH v19 3/7] xbitmap: add more operations
From: Wei Wang @ 2017-12-14 3:47 UTC (permalink / raw)
To: Tetsuo Handa
Cc: yang.zhang.wz, kvm, mst, liliang.opensource, qemu-devel,
virtualization, linux-mm, aarcange, virtio-dev, mawilcox, willy,
quan.xu, nilal, riel, cornelia.huck, mhocko, linux-kernel,
amit.shah, pbonzini, akpm, mgorman
In-Reply-To: <201712132316.EJJ57332.MFOSJHOFFVLtQO@I-love.SAKURA.ne.jp>
On 12/13/2017 10:16 PM, Tetsuo Handa wrote:
> Wei Wang wrote:
>> On 12/12/2017 09:20 PM, Tetsuo Handa wrote:
>>> Wei Wang wrote:
>>>> +void xb_clear_bit_range(struct xb *xb, unsigned long start, unsigned long end)
>>>> +{
>>>> + struct radix_tree_root *root = &xb->xbrt;
>>>> + struct radix_tree_node *node;
>>>> + void **slot;
>>>> + struct ida_bitmap *bitmap;
>>>> + unsigned int nbits;
>>>> +
>>>> + for (; start < end; start = (start | (IDA_BITMAP_BITS - 1)) + 1) {
>>>> + unsigned long index = start / IDA_BITMAP_BITS;
>>>> + unsigned long bit = start % IDA_BITMAP_BITS;
>>>> +
>>>> + bitmap = __radix_tree_lookup(root, index, &node, &slot);
>>>> + if (radix_tree_exception(bitmap)) {
>>>> + unsigned long ebit = bit + 2;
>>>> + unsigned long tmp = (unsigned long)bitmap;
>>>> +
>>>> + nbits = min(end - start + 1, BITS_PER_LONG - ebit);
>>>> +
>>>> + if (ebit >= BITS_PER_LONG)
>>> What happens if we hit this "continue;" when "index == ULONG_MAX / IDA_BITMAP_BITS" ?
>> Thanks. I also improved the test case for this. I plan to change the
>> implementation a little bit to avoid such overflow (has passed the test
>> case that I have, just post out for another set of eyes):
>>
>> {
>> ...
>> unsigned long idx = start / IDA_BITMAP_BITS;
>> unsigned long bit = start % IDA_BITMAP_BITS;
>> unsigned long idx_end = end / IDA_BITMAP_BITS;
>> unsigned long ret;
>>
>> for (idx = start / IDA_BITMAP_BITS; idx <= idx_end; idx++) {
>> unsigned long ida_start = idx * IDA_BITMAP_BITS;
>>
>> bitmap = __radix_tree_lookup(root, idx, &node, &slot);
>> if (radix_tree_exception(bitmap)) {
>> unsigned long tmp = (unsigned long)bitmap;
>> unsigned long ebit = bit + 2;
>>
>> if (ebit >= BITS_PER_LONG)
>> continue;
> Will you please please do eliminate exception path?
Please first see my explanations below, I'll try to help you understand
it thoroughly. If it is really too complex to understand it finally,
then I think we can start from the fundamental part by removing the
exceptional path if no objections from others.
> I can't interpret what "ebit >= BITS_PER_LONG" means.
> The reason you "continue;" is that all bits beyond are "0", isn't it?
> Then, it would make sense to "continue;" when finding next "1" because
> all bits beyond are "0". But how does it make sense to "continue;" when
> finding next "0" despite all bits beyond are "0"?
Not the case actually. Please see this example:
1) xb_set_bit(10); // bit 10 is set, so an exceptional entry (i.e.
[0:62]) is used
2) xb_clear_bit_range(66, 2048);
- One ida bitmap size is 1024 bits, so this clear will be performed
with 2 loops, first to clear [66, 1024), second to clear [1024, 2048)
- When the first loop clears [66, 1024), and finds that it is an
exception entry (because bit 10 is set, and the 62 bit entry is enough
to cover). Another point we have to remember is that an exceptional
entry implies that the rest of bits [63, 1024) are all 0s.
- The starting bit 66 already exceeds the the exceptional entry bit
range [0, 62], and with the fact that the rest of bits are all 0s, so it
is time to just "continue", which goes to the second range [1024, 2048)
I used the example of xb_clear_bit_range(), and xb_find_next_bit() is
the same fundamentally. Please let me know if anywhere still looks fuzzy.
>
>> if (set)
>> ret = find_next_bit(&tmp,
>> BITS_PER_LONG, ebit);
>> else
>> ret = find_next_zero_bit(&tmp,
>> BITS_PER_LONG,
>> ebit);
>> if (ret < BITS_PER_LONG)
>> return ret - 2 + ida_start;
>> } else if (bitmap) {
>> if (set)
>> ret = find_next_bit(bitmap->bitmap,
>> IDA_BITMAP_BITS, bit);
>> else
>> ret = find_next_zero_bit(bitmap->bitmap,
>> IDA_BITMAP_BITS, bit);
> "bit" may not be 0 for the first round and "bit" is always 0 afterwords.
> But where is the guaranteed that "end" is a multiple of IDA_BITMAP_BITS ?
> Please explain why it is correct to use IDA_BITMAP_BITS unconditionally
> for the last round.
There missed something here, it will be:
nbits = min(end - ida_start + 1, IDA_BITMAP_BITS - bit);
if (set)
ret = find_next_bit(bitmap->bitmap, nbits, bit);
else
ret = find_next_zero_bit(bitmap->bitmap,
nbits, bit);
if (ret < nbits)
return ret + ida_start;
>>>> +/**
>>>> + * xb_find_next_set_bit - find the next set bit in a range
>>>> + * @xb: the xbitmap to search
>>>> + * @start: the start of the range, inclusive
>>>> + * @end: the end of the range, exclusive
>>>> + *
>>>> + * Returns: the index of the found bit, or @end + 1 if no such bit is found.
>>>> + */
>>>> +unsigned long xb_find_next_set_bit(struct xb *xb, unsigned long start,
>>>> + unsigned long end)
>>>> +{
>>>> + return xb_find_next_bit(xb, start, end, 1);
>>>> +}
>>> Won't "exclusive" loose ability to handle ULONG_MAX ? Since this is a
>>> library module, missing ability to handle ULONG_MAX sounds like an omission.
>>> Shouldn't we pass (or return) whether "found or not" flag (e.g. strtoul() in
>>> C library function)?
>>>
>>> bool xb_find_next_set_bit(struct xb *xb, unsigned long start, unsigned long end, unsigned long *result);
>>> unsigned long xb_find_next_set_bit(struct xb *xb, unsigned long start, unsigned long end, bool *found);
>> Yes, ULONG_MAX needs to be tested by xb_test_bit(). Compared to checking
>> the return value, would it be the same to let the caller check for the
>> ULONG_MAX boundary?
>>
> Why the caller needs to care about whether it is ULONG_MAX or not?
I don't stick with this one, and will use the method that you suggested.
Thanks for the review.
>
> Also, one more thing you need to check. Have you checked how long does
> xb_find_next_set_bit(xb, 0, ULONG_MAX) on an empty xbitmap takes?
> If it causes soft lockup warning, should we add cond_resched() ?
> If yes, you have to document that this API might sleep. If no, you
> have to document that the caller of this API is responsible for
> not to pass such a large value range.
Yes, that will take too long time. Probably we can document some
comments as a reminder for the callers.
Best,
Wei
^ permalink raw reply
* Re: [PATCHv2] virtio_mmio: fix devm cleanup
From: Mark Rutland @ 2017-12-13 14:34 UTC (permalink / raw)
To: Cornelia Huck, Michael S . Tsirkin
Cc: weiping zhang, linux-kernel, virtualization
In-Reply-To: <20171212180223.2dfb3b40.cohuck@redhat.com>
On Tue, Dec 12, 2017 at 06:02:23PM +0100, Cornelia Huck wrote:
> On Tue, 12 Dec 2017 13:45:50 +0000
> Mark Rutland <mark.rutland@arm.com> wrote:
>
> > Recent rework of the virtio_mmio probe/remove paths balanced a
> > devm_ioremap() with an iounmap() rather than its devm variant. This ends
> > up corrupting the devm datastructures, and results in the following
> > boot-time splat on arm64 under QEMU 2.9.0:
> >
> > [ 3.450397] ------------[ cut here ]------------
> > [ 3.453822] Trying to vfree() nonexistent vm area (00000000c05b4844)
> > [ 3.460534] WARNING: CPU: 1 PID: 1 at mm/vmalloc.c:1525 __vunmap+0x1b8/0x220
> > [ 3.475898] Kernel panic - not syncing: panic_on_warn set ...
> > [ 3.475898]
> > [ 3.493933] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.15.0-rc3 #1
> > [ 3.513109] Hardware name: linux,dummy-virt (DT)
> > [ 3.525382] Call trace:
> > [ 3.531683] dump_backtrace+0x0/0x368
> > [ 3.543921] show_stack+0x20/0x30
> > [ 3.547767] dump_stack+0x108/0x164
> > [ 3.559584] panic+0x25c/0x51c
> > [ 3.569184] __warn+0x29c/0x31c
> > [ 3.576023] report_bug+0x1d4/0x290
> > [ 3.586069] bug_handler.part.2+0x40/0x100
> > [ 3.597820] bug_handler+0x4c/0x88
> > [ 3.608400] brk_handler+0x11c/0x218
> > [ 3.613430] do_debug_exception+0xe8/0x318
> > [ 3.627370] el1_dbg+0x18/0x78
> > [ 3.634037] __vunmap+0x1b8/0x220
> > [ 3.648747] vunmap+0x6c/0xc0
> > [ 3.653864] __iounmap+0x44/0x58
> > [ 3.659771] devm_ioremap_release+0x34/0x68
> > [ 3.672983] release_nodes+0x404/0x880
> > [ 3.683543] devres_release_all+0x6c/0xe8
> > [ 3.695692] driver_probe_device+0x250/0x828
> > [ 3.706187] __driver_attach+0x190/0x210
> > [ 3.717645] bus_for_each_dev+0x14c/0x1f0
> > [ 3.728633] driver_attach+0x48/0x78
> > [ 3.740249] bus_add_driver+0x26c/0x5b8
> > [ 3.752248] driver_register+0x16c/0x398
> > [ 3.757211] __platform_driver_register+0xd8/0x128
> > [ 3.770860] virtio_mmio_init+0x1c/0x24
> > [ 3.782671] do_one_initcall+0xe0/0x398
> > [ 3.791890] kernel_init_freeable+0x594/0x660
> > [ 3.798514] kernel_init+0x18/0x190
> > [ 3.810220] ret_from_fork+0x10/0x18
> >
> > To fix this, we can simply rip out the explicit cleanup that the devm
> > infrastructure will do for us when our probe function returns an error
> > code, or when our remove function returns.
> >
> > We only need to ensure that we call put_device() if a call to
> > register_virtio_device() fails in the probe path.
> >
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Fixes: 7eb781b1bbb7136f ("virtio_mmio: add cleanup for virtio_mmio_probe")
> > Fixes: 25f32223bce5c580 ("virtio_mmio: add cleanup for virtio_mmio_remove")
> > Cc: Cornelia Huck <cohuck@redhat.com>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: weiping zhang <zhangweiping@didichuxing.com>
> > Cc: virtualization@lists.linux-foundation.org
> > ---
> > drivers/virtio/virtio_mmio.c | 43 +++++++++----------------------------------
> > 1 file changed, 9 insertions(+), 34 deletions(-)
>
> In the hope that I have grokked the devm_* interface by now,
>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Thanks!
Michael, could you please queue this as a fix for v4.15?
This regressed arm64 VMs booting between v4.15-rc1 and v4-15-rc2,
impacting our automated regression testing, and I'd very much like to
get back to testing pure mainline rather than mainline + local fixes.
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCH v19 3/7] xbitmap: add more operations
From: Wei Wang @ 2017-12-13 12:26 UTC (permalink / raw)
To: Tetsuo Handa, virtio-dev, linux-kernel, qemu-devel,
virtualization, kvm, linux-mm, mst, mhocko, akpm, mawilcox
Cc: aarcange, yang.zhang.wz, riel, liliang.opensource, willy,
amit.shah, quan.xu, cornelia.huck, pbonzini, nilal, mgorman
In-Reply-To: <201712122220.IFH05261.LtJOFFSFHVMQOO@I-love.SAKURA.ne.jp>
On 12/12/2017 09:20 PM, Tetsuo Handa wrote:
> Wei Wang wrote:
>> +void xb_clear_bit_range(struct xb *xb, unsigned long start, unsigned long end)
>> +{
>> + struct radix_tree_root *root = &xb->xbrt;
>> + struct radix_tree_node *node;
>> + void **slot;
>> + struct ida_bitmap *bitmap;
>> + unsigned int nbits;
>> +
>> + for (; start < end; start = (start | (IDA_BITMAP_BITS - 1)) + 1) {
>> + unsigned long index = start / IDA_BITMAP_BITS;
>> + unsigned long bit = start % IDA_BITMAP_BITS;
>> +
>> + bitmap = __radix_tree_lookup(root, index, &node, &slot);
>> + if (radix_tree_exception(bitmap)) {
>> + unsigned long ebit = bit + 2;
>> + unsigned long tmp = (unsigned long)bitmap;
>> +
>> + nbits = min(end - start + 1, BITS_PER_LONG - ebit);
>> +
>> + if (ebit >= BITS_PER_LONG)
> What happens if we hit this "continue;" when "index == ULONG_MAX / IDA_BITMAP_BITS" ?
Thanks. I also improved the test case for this. I plan to change the
implementation a little bit to avoid such overflow (has passed the test
case that I have, just post out for another set of eyes):
{
...
unsigned long idx = start / IDA_BITMAP_BITS;
unsigned long bit = start % IDA_BITMAP_BITS;
unsigned long idx_end = end / IDA_BITMAP_BITS;
unsigned long ret;
for (idx = start / IDA_BITMAP_BITS; idx <= idx_end; idx++) {
unsigned long ida_start = idx * IDA_BITMAP_BITS;
bitmap = __radix_tree_lookup(root, idx, &node, &slot);
if (radix_tree_exception(bitmap)) {
unsigned long tmp = (unsigned long)bitmap;
unsigned long ebit = bit + 2;
if (ebit >= BITS_PER_LONG)
continue;
if (set)
ret = find_next_bit(&tmp,
BITS_PER_LONG, ebit);
else
ret = find_next_zero_bit(&tmp,
BITS_PER_LONG,
ebit);
if (ret < BITS_PER_LONG)
return ret - 2 + ida_start;
} else if (bitmap) {
if (set)
ret = find_next_bit(bitmap->bitmap,
IDA_BITMAP_BITS, bit);
else
ret = find_next_zero_bit(bitmap->bitmap,
IDA_BITMAP_BITS, bit);
if (ret < IDA_BITMAP_BITS)
return ret + ida_start;
} else if (!bitmap && !set) {
return bit + IDA_BITMAP_BITS * idx;
}
bit = 0;
}
return end;
}
>
> Can you eliminate exception path and fold all xbitmap patches into one, and
> post only one xbitmap patch without virtio-baloon changes? If exception path
> is valuable, you can add exception path after minimum version is merged.
> This series is too difficult for me to close corner cases.
That exception path is claimed to save memory, and I don't have a strong
reason to remove that part.
Matthew, could we get your feedback on this?
>
>> +/**
>> + * xb_find_next_set_bit - find the next set bit in a range
>> + * @xb: the xbitmap to search
>> + * @start: the start of the range, inclusive
>> + * @end: the end of the range, exclusive
>> + *
>> + * Returns: the index of the found bit, or @end + 1 if no such bit is found.
>> + */
>> +unsigned long xb_find_next_set_bit(struct xb *xb, unsigned long start,
>> + unsigned long end)
>> +{
>> + return xb_find_next_bit(xb, start, end, 1);
>> +}
> Won't "exclusive" loose ability to handle ULONG_MAX ? Since this is a
> library module, missing ability to handle ULONG_MAX sounds like an omission.
> Shouldn't we pass (or return) whether "found or not" flag (e.g. strtoul() in
> C library function)?
>
> bool xb_find_next_set_bit(struct xb *xb, unsigned long start, unsigned long end, unsigned long *result);
> unsigned long xb_find_next_set_bit(struct xb *xb, unsigned long start, unsigned long end, bool *found);
Yes, ULONG_MAX needs to be tested by xb_test_bit(). Compared to checking
the return value, would it be the same to let the caller check for the
ULONG_MAX boundary?
Best,
Wei
^ permalink raw reply
* [bpf-next V1-RFC PATCH 11/14] virtio_net: setup xdp_rxq_info
From: Jesper Dangaard Brouer @ 2017-12-13 11:20 UTC (permalink / raw)
To: Daniel Borkmann, Alexei Starovoitov
Cc: Michael S. Tsirkin, netdev, Jesper Dangaard Brouer,
virtualization, dsahern, gospo, bjorn.topel, michael.chan
In-Reply-To: <151316391502.14967.13292358380181773729.stgit@firesoul>
The virtio_net driver doesn't dynamically change the RX-ring queue
layout and backing pages, but instead reject XDP setup if all the
conditions for XDP is not meet. Thus, the xdp_rxq_info also remains
fairly static. This allow us to simply add the init+reg/unreg to
net_device open/close functions.
Driver hook points for xdp_rxq_info:
* init+reg: virtnet_open
* unreg : virtnet_close
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: virtualization@lists.linux-foundation.org
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
drivers/net/virtio_net.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 19a985ef9104..9792183befbf 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -31,6 +31,7 @@
#include <linux/average.h>
#include <linux/filter.h>
#include <net/route.h>
+#include <net/xdp.h>
static int napi_weight = NAPI_POLL_WEIGHT;
module_param(napi_weight, int, 0444);
@@ -115,6 +116,8 @@ struct receive_queue {
/* Name of this receive queue: input.$index */
char name[40];
+
+ struct xdp_rxq_info xdp_rxq;
};
struct virtnet_info {
@@ -556,6 +559,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
xdp.data = xdp.data_hard_start + xdp_headroom;
xdp_set_data_meta_invalid(&xdp);
xdp.data_end = xdp.data + len;
+ xdp.rxq = &rq->xdp_rxq;
orig_data = xdp.data;
act = bpf_prog_run_xdp(xdp_prog, &xdp);
@@ -1229,6 +1233,13 @@ static int virtnet_open(struct net_device *dev)
/* Make sure we have some buffers: if oom use wq. */
if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
schedule_delayed_work(&vi->refill, 0);
+
+ /* XDP RX queue info */
+ xdp_rxq_info_init(&vi->rq[i].xdp_rxq);
+ vi->rq[i].xdp_rxq.dev = dev;
+ vi->rq[i].xdp_rxq.queue_index = i;
+ xdp_rxq_info_reg(&vi->rq[i].xdp_rxq);
+
virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
virtnet_napi_tx_enable(vi, vi->sq[i].vq, &vi->sq[i].napi);
}
@@ -1557,6 +1568,7 @@ static int virtnet_close(struct net_device *dev)
cancel_delayed_work_sync(&vi->refill);
for (i = 0; i < vi->max_queue_pairs; i++) {
+ xdp_rxq_info_unreg(&vi->rq[i].xdp_rxq);
napi_disable(&vi->rq[i].napi);
virtnet_napi_tx_disable(&vi->sq[i].napi);
}
^ permalink raw reply related
* Re: [PATCH v2 2/3] virtio: use put_device instead of kfree
From: Cornelia Huck @ 2017-12-13 8:27 UTC (permalink / raw)
To: weiping zhang; +Cc: virtualization, mst
In-Reply-To: <11026fc88be1033d7ce5bc8ecb9b355d27d82075.1513083885.git.zhangweiping@didichuxing.com>
On Tue, 12 Dec 2017 21:24:33 +0800
weiping zhang <zhangweiping@didichuxing.com> wrote:
> As mentioned at drivers/base/core.c:
> /*
> * NOTE: _Never_ directly free @dev after calling this function, even
> * if it returned an error! Always use put_device() to give up the
> * reference initialized in this function instead.
> */
> so we don't free vp_vdev until vp_vdev.dev.release be called.
>
> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> ---
> drivers/misc/mic/vop/vop_main.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply
* Re: [PATCHv2] virtio_mmio: fix devm cleanup
From: Cornelia Huck @ 2017-12-12 17:02 UTC (permalink / raw)
To: Mark Rutland
Cc: virtualization, weiping zhang, linux-kernel, Michael S . Tsirkin
In-Reply-To: <20171212134550.18378-1-mark.rutland@arm.com>
On Tue, 12 Dec 2017 13:45:50 +0000
Mark Rutland <mark.rutland@arm.com> wrote:
> Recent rework of the virtio_mmio probe/remove paths balanced a
> devm_ioremap() with an iounmap() rather than its devm variant. This ends
> up corrupting the devm datastructures, and results in the following
> boot-time splat on arm64 under QEMU 2.9.0:
>
> [ 3.450397] ------------[ cut here ]------------
> [ 3.453822] Trying to vfree() nonexistent vm area (00000000c05b4844)
> [ 3.460534] WARNING: CPU: 1 PID: 1 at mm/vmalloc.c:1525 __vunmap+0x1b8/0x220
> [ 3.475898] Kernel panic - not syncing: panic_on_warn set ...
> [ 3.475898]
> [ 3.493933] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.15.0-rc3 #1
> [ 3.513109] Hardware name: linux,dummy-virt (DT)
> [ 3.525382] Call trace:
> [ 3.531683] dump_backtrace+0x0/0x368
> [ 3.543921] show_stack+0x20/0x30
> [ 3.547767] dump_stack+0x108/0x164
> [ 3.559584] panic+0x25c/0x51c
> [ 3.569184] __warn+0x29c/0x31c
> [ 3.576023] report_bug+0x1d4/0x290
> [ 3.586069] bug_handler.part.2+0x40/0x100
> [ 3.597820] bug_handler+0x4c/0x88
> [ 3.608400] brk_handler+0x11c/0x218
> [ 3.613430] do_debug_exception+0xe8/0x318
> [ 3.627370] el1_dbg+0x18/0x78
> [ 3.634037] __vunmap+0x1b8/0x220
> [ 3.648747] vunmap+0x6c/0xc0
> [ 3.653864] __iounmap+0x44/0x58
> [ 3.659771] devm_ioremap_release+0x34/0x68
> [ 3.672983] release_nodes+0x404/0x880
> [ 3.683543] devres_release_all+0x6c/0xe8
> [ 3.695692] driver_probe_device+0x250/0x828
> [ 3.706187] __driver_attach+0x190/0x210
> [ 3.717645] bus_for_each_dev+0x14c/0x1f0
> [ 3.728633] driver_attach+0x48/0x78
> [ 3.740249] bus_add_driver+0x26c/0x5b8
> [ 3.752248] driver_register+0x16c/0x398
> [ 3.757211] __platform_driver_register+0xd8/0x128
> [ 3.770860] virtio_mmio_init+0x1c/0x24
> [ 3.782671] do_one_initcall+0xe0/0x398
> [ 3.791890] kernel_init_freeable+0x594/0x660
> [ 3.798514] kernel_init+0x18/0x190
> [ 3.810220] ret_from_fork+0x10/0x18
>
> To fix this, we can simply rip out the explicit cleanup that the devm
> infrastructure will do for us when our probe function returns an error
> code, or when our remove function returns.
>
> We only need to ensure that we call put_device() if a call to
> register_virtio_device() fails in the probe path.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Fixes: 7eb781b1bbb7136f ("virtio_mmio: add cleanup for virtio_mmio_probe")
> Fixes: 25f32223bce5c580 ("virtio_mmio: add cleanup for virtio_mmio_remove")
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: weiping zhang <zhangweiping@didichuxing.com>
> Cc: virtualization@lists.linux-foundation.org
> ---
> drivers/virtio/virtio_mmio.c | 43 +++++++++----------------------------------
> 1 file changed, 9 insertions(+), 34 deletions(-)
In the hope that I have grokked the devm_* interface by now,
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply
* (unknown),
From: Solen win @ 2017-12-12 16:06 UTC (permalink / raw)
To: virtualization
[-- Attachment #1.1: Type: text/plain, Size: 23 bytes --]
Solenwin@freshdesk.com
[-- Attachment #1.2: Type: text/html, Size: 141 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCHv2] virtio_mmio: fix devm cleanup
From: weiping zhang @ 2017-12-12 15:04 UTC (permalink / raw)
To: Mark Rutland
Cc: Michael S . Tsirkin, Cornelia Huck, weiping zhang, linux-kernel,
virtualization
In-Reply-To: <20171212144506.c4z6aqieo5uo3pcn@lakrids.cambridge.arm.com>
2017-12-12 22:45 GMT+08:00 Mark Rutland <mark.rutland@arm.com>:
> On Tue, Dec 12, 2017 at 10:26:24PM +0800, weiping zhang wrote:
>> 2017-12-12 21:45 GMT+08:00 Mark Rutland <mark.rutland@arm.com>:
>> Hi Mark,
>
> Hi,
>
>> thanks your patch, I dig into these three devm_xxx funciton,
>> all of them represented by a struct devres as following,
>>
>> struct devres_node {
>> struct list_head entry;
>> dr_release_t release;
>> #ifdef CONFIG_DEBUG_DEVRES
>> const char *name;
>> size_t size;
>> #endif
>>
>> };
>>
>> struct devres {
>> struct devres_node node;
>> /* -- 3 pointers */
>> unsigned long long data[]; /* guarantee ull alignment */
>> };
>
>> 2) devm_kzalloc -> devm_kmalloc
>>
>> dr = alloc_dr(devm_kmalloc_release, size, gfp, dev_to_node(dev));
>> "devm_kmalloc_release" is noop, do nothing.
>
> Please note that the release function is there to perform cleanup prior
> to the devm infrastructure releasing the memory.
>
> The devm_kmalloc_release function is a no-op since nothing has to be
> done prior to memory being freed, but the memory itself is still freed.
>
> In alloc_dr(), the struct devres is allocated together with the memory,
> since alloc_dr() does:
>
> size_t tot_size = sizeof(struct devres) + size;
> struct devres *dr;
>
> dr = kmalloc_node_track_caller(tot_size, gfp, nid);
>
> return dr->data;
>
> ... where dr->data points at the memory after the struct devres.
>
> Later, in release_nodes() we do:
>
> list_for_each_entry_safe_reverse(dr, tmp, &todo, node.entry) {
> devres_log(dev, &dr->node, "REL");
> dr->node.release(dev, dr->data);
> kfree(dr);
> }
>
> ... which will invoke the no-op devm_kmalloc_release, then free the
> devres allocation, including the dr->data memory the user requested.
>
>> so for case 2) above, we need a devm_kfree() before call
>> register_virtio_device
>
> As above, I do not believe that is the case.
>
Oh I see, thanks your detail explanation. Thanks a lot.
> Thanks,
> Mark.
^ permalink raw reply
* Re: [PATCHv2] virtio_mmio: fix devm cleanup
From: Mark Rutland @ 2017-12-12 14:45 UTC (permalink / raw)
To: weiping zhang
Cc: Michael S . Tsirkin, Cornelia Huck, weiping zhang, linux-kernel,
virtualization
In-Reply-To: <CAA70yB7qN_50xFFmx5Vxs61teSjBjm2jCzZY=XqwNiLU_dWMgw@mail.gmail.com>
On Tue, Dec 12, 2017 at 10:26:24PM +0800, weiping zhang wrote:
> 2017-12-12 21:45 GMT+08:00 Mark Rutland <mark.rutland@arm.com>:
> Hi Mark,
Hi,
> thanks your patch, I dig into these three devm_xxx funciton,
> all of them represented by a struct devres as following,
>
> struct devres_node {
> struct list_head entry;
> dr_release_t release;
> #ifdef CONFIG_DEBUG_DEVRES
> const char *name;
> size_t size;
> #endif
>
> };
>
> struct devres {
> struct devres_node node;
> /* -- 3 pointers */
> unsigned long long data[]; /* guarantee ull alignment */
> };
> 2) devm_kzalloc -> devm_kmalloc
>
> dr = alloc_dr(devm_kmalloc_release, size, gfp, dev_to_node(dev));
> "devm_kmalloc_release" is noop, do nothing.
Please note that the release function is there to perform cleanup prior
to the devm infrastructure releasing the memory.
The devm_kmalloc_release function is a no-op since nothing has to be
done prior to memory being freed, but the memory itself is still freed.
In alloc_dr(), the struct devres is allocated together with the memory,
since alloc_dr() does:
size_t tot_size = sizeof(struct devres) + size;
struct devres *dr;
dr = kmalloc_node_track_caller(tot_size, gfp, nid);
return dr->data;
... where dr->data points at the memory after the struct devres.
Later, in release_nodes() we do:
list_for_each_entry_safe_reverse(dr, tmp, &todo, node.entry) {
devres_log(dev, &dr->node, "REL");
dr->node.release(dev, dr->data);
kfree(dr);
}
... which will invoke the no-op devm_kmalloc_release, then free the
devres allocation, including the dr->data memory the user requested.
> so for case 2) above, we need a devm_kfree() before call
> register_virtio_device
As above, I do not believe that is the case.
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCHv2] virtio_mmio: fix devm cleanup
From: weiping zhang @ 2017-12-12 14:26 UTC (permalink / raw)
To: Mark Rutland
Cc: Michael S . Tsirkin, Cornelia Huck, weiping zhang, linux-kernel,
virtualization
In-Reply-To: <20171212134550.18378-1-mark.rutland@arm.com>
2017-12-12 21:45 GMT+08:00 Mark Rutland <mark.rutland@arm.com>:
> Recent rework of the virtio_mmio probe/remove paths balanced a
> devm_ioremap() with an iounmap() rather than its devm variant. This ends
> up corrupting the devm datastructures, and results in the following
> boot-time splat on arm64 under QEMU 2.9.0:
>
> [ 3.450397] ------------[ cut here ]------------
> [ 3.453822] Trying to vfree() nonexistent vm area (00000000c05b4844)
> [ 3.460534] WARNING: CPU: 1 PID: 1 at mm/vmalloc.c:1525 __vunmap+0x1b8/0x220
> [ 3.475898] Kernel panic - not syncing: panic_on_warn set ...
> [ 3.475898]
> [ 3.493933] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.15.0-rc3 #1
> [ 3.513109] Hardware name: linux,dummy-virt (DT)
> [ 3.525382] Call trace:
> [ 3.531683] dump_backtrace+0x0/0x368
> [ 3.543921] show_stack+0x20/0x30
> [ 3.547767] dump_stack+0x108/0x164
> [ 3.559584] panic+0x25c/0x51c
> [ 3.569184] __warn+0x29c/0x31c
> [ 3.576023] report_bug+0x1d4/0x290
> [ 3.586069] bug_handler.part.2+0x40/0x100
> [ 3.597820] bug_handler+0x4c/0x88
> [ 3.608400] brk_handler+0x11c/0x218
> [ 3.613430] do_debug_exception+0xe8/0x318
> [ 3.627370] el1_dbg+0x18/0x78
> [ 3.634037] __vunmap+0x1b8/0x220
> [ 3.648747] vunmap+0x6c/0xc0
> [ 3.653864] __iounmap+0x44/0x58
> [ 3.659771] devm_ioremap_release+0x34/0x68
> [ 3.672983] release_nodes+0x404/0x880
> [ 3.683543] devres_release_all+0x6c/0xe8
> [ 3.695692] driver_probe_device+0x250/0x828
> [ 3.706187] __driver_attach+0x190/0x210
> [ 3.717645] bus_for_each_dev+0x14c/0x1f0
> [ 3.728633] driver_attach+0x48/0x78
> [ 3.740249] bus_add_driver+0x26c/0x5b8
> [ 3.752248] driver_register+0x16c/0x398
> [ 3.757211] __platform_driver_register+0xd8/0x128
> [ 3.770860] virtio_mmio_init+0x1c/0x24
> [ 3.782671] do_one_initcall+0xe0/0x398
> [ 3.791890] kernel_init_freeable+0x594/0x660
> [ 3.798514] kernel_init+0x18/0x190
> [ 3.810220] ret_from_fork+0x10/0x18
>
> To fix this, we can simply rip out the explicit cleanup that the devm
> infrastructure will do for us when our probe function returns an error
> code, or when our remove function returns.
>
> We only need to ensure that we call put_device() if a call to
> register_virtio_device() fails in the probe path.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Fixes: 7eb781b1bbb7136f ("virtio_mmio: add cleanup for virtio_mmio_probe")
> Fixes: 25f32223bce5c580 ("virtio_mmio: add cleanup for virtio_mmio_remove")
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: weiping zhang <zhangweiping@didichuxing.com>
> Cc: virtualization@lists.linux-foundation.org
> ---
> drivers/virtio/virtio_mmio.c | 43 +++++++++----------------------------------
> 1 file changed, 9 insertions(+), 34 deletions(-)
>
> Since v1 [1]:
> * Fix cleanup in virtio_mmio_remove
>
> Mark.
>
> [1] https://lkml.kernel.org/r/20171212125302.6846-1-mark.rutland@arm.com
>
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index a9192fe4f345..c92131edfaba 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -522,10 +522,8 @@ static int virtio_mmio_probe(struct platform_device *pdev)
> return -EBUSY;
>
> vm_dev = devm_kzalloc(&pdev->dev, sizeof(*vm_dev), GFP_KERNEL);
> - if (!vm_dev) {
> - rc = -ENOMEM;
> - goto free_mem;
> - }
> + if (!vm_dev)
> + return -ENOMEM;
>
> vm_dev->vdev.dev.parent = &pdev->dev;
> vm_dev->vdev.dev.release = virtio_mmio_release_dev;
> @@ -535,17 +533,14 @@ static int virtio_mmio_probe(struct platform_device *pdev)
> spin_lock_init(&vm_dev->lock);
>
> vm_dev->base = devm_ioremap(&pdev->dev, mem->start, resource_size(mem));
> - if (vm_dev->base == NULL) {
> - rc = -EFAULT;
> - goto free_vmdev;
> - }
> + if (vm_dev->base == NULL)
> + return -EFAULT;
>
> /* Check magic value */
> magic = readl(vm_dev->base + VIRTIO_MMIO_MAGIC_VALUE);
> if (magic != ('v' | 'i' << 8 | 'r' << 16 | 't' << 24)) {
> dev_warn(&pdev->dev, "Wrong magic value 0x%08lx!\n", magic);
> - rc = -ENODEV;
> - goto unmap;
> + return -ENODEV;
> }
>
> /* Check device version */
> @@ -553,8 +548,7 @@ static int virtio_mmio_probe(struct platform_device *pdev)
> if (vm_dev->version < 1 || vm_dev->version > 2) {
> dev_err(&pdev->dev, "Version %ld not supported!\n",
> vm_dev->version);
> - rc = -ENXIO;
> - goto unmap;
> + return -ENXIO;
> }
>
> vm_dev->vdev.id.device = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID);
> @@ -563,8 +557,7 @@ static int virtio_mmio_probe(struct platform_device *pdev)
> * virtio-mmio device with an ID 0 is a (dummy) placeholder
> * with no function. End probing now with no error reported.
> */
> - rc = -ENODEV;
> - goto unmap;
> + return -ENODEV;
> }
> vm_dev->vdev.id.vendor = readl(vm_dev->base + VIRTIO_MMIO_VENDOR_ID);
>
> @@ -590,33 +583,15 @@ static int virtio_mmio_probe(struct platform_device *pdev)
> platform_set_drvdata(pdev, vm_dev);
>
> rc = register_virtio_device(&vm_dev->vdev);
> - if (rc) {
> - iounmap(vm_dev->base);
> - devm_release_mem_region(&pdev->dev, mem->start,
> - resource_size(mem));
> + if (rc)
> put_device(&vm_dev->vdev.dev);
> - }
> - return rc;
> -unmap:
> - iounmap(vm_dev->base);
> -free_mem:
> - devm_release_mem_region(&pdev->dev, mem->start,
> - resource_size(mem));
> -free_vmdev:
> - devm_kfree(&pdev->dev, vm_dev);
> +
> return rc;
> }
>
> static int virtio_mmio_remove(struct platform_device *pdev)
> {
> struct virtio_mmio_device *vm_dev = platform_get_drvdata(pdev);
> - struct resource *mem;
> -
> - iounmap(vm_dev->base);
> - mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - if (mem)
> - devm_release_mem_region(&pdev->dev, mem->start,
> - resource_size(mem));
> unregister_virtio_device(&vm_dev->vdev);
>
> return 0;
> --
> 2.11.0
>
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Hi Mark,
thanks your patch, I dig into these three devm_xxx funciton,
all of them represented by a struct devres as following,
struct devres_node {
struct list_head entry;
dr_release_t release;
#ifdef CONFIG_DEBUG_DEVRES
const char *name;
size_t size;
#endif
};
struct devres {
struct devres_node node;
/* -- 3 pointers */
unsigned long long data[]; /* guarantee ull alignment */
};
1) devm_request_mem_region -> __devm_request_region
dr = devres_alloc(devm_region_release, sizeof(struct region_devres),
"devm_region_release" will call __release_region to release resource
2) devm_kzalloc -> devm_kmalloc
dr = alloc_dr(devm_kmalloc_release, size, gfp, dev_to_node(dev));
"devm_kmalloc_release" is noop, do nothing.
3) devm_ioremap -> ... -> __devres_alloc_node
ptr = devres_alloc(devm_ioremap_release, sizeof(*ptr), GFP_KERNEL);
devm_ioremap_release do iounmap
so for case 2) above, we need a devm_kfree() before call register_virtio_device
^ permalink raw reply
* [PATCHv2] virtio_mmio: fix devm cleanup
From: Mark Rutland @ 2017-12-12 13:45 UTC (permalink / raw)
To: linux-kernel
Cc: Mark Rutland, Cornelia Huck, weiping zhang, virtualization,
Michael S . Tsirkin
Recent rework of the virtio_mmio probe/remove paths balanced a
devm_ioremap() with an iounmap() rather than its devm variant. This ends
up corrupting the devm datastructures, and results in the following
boot-time splat on arm64 under QEMU 2.9.0:
[ 3.450397] ------------[ cut here ]------------
[ 3.453822] Trying to vfree() nonexistent vm area (00000000c05b4844)
[ 3.460534] WARNING: CPU: 1 PID: 1 at mm/vmalloc.c:1525 __vunmap+0x1b8/0x220
[ 3.475898] Kernel panic - not syncing: panic_on_warn set ...
[ 3.475898]
[ 3.493933] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.15.0-rc3 #1
[ 3.513109] Hardware name: linux,dummy-virt (DT)
[ 3.525382] Call trace:
[ 3.531683] dump_backtrace+0x0/0x368
[ 3.543921] show_stack+0x20/0x30
[ 3.547767] dump_stack+0x108/0x164
[ 3.559584] panic+0x25c/0x51c
[ 3.569184] __warn+0x29c/0x31c
[ 3.576023] report_bug+0x1d4/0x290
[ 3.586069] bug_handler.part.2+0x40/0x100
[ 3.597820] bug_handler+0x4c/0x88
[ 3.608400] brk_handler+0x11c/0x218
[ 3.613430] do_debug_exception+0xe8/0x318
[ 3.627370] el1_dbg+0x18/0x78
[ 3.634037] __vunmap+0x1b8/0x220
[ 3.648747] vunmap+0x6c/0xc0
[ 3.653864] __iounmap+0x44/0x58
[ 3.659771] devm_ioremap_release+0x34/0x68
[ 3.672983] release_nodes+0x404/0x880
[ 3.683543] devres_release_all+0x6c/0xe8
[ 3.695692] driver_probe_device+0x250/0x828
[ 3.706187] __driver_attach+0x190/0x210
[ 3.717645] bus_for_each_dev+0x14c/0x1f0
[ 3.728633] driver_attach+0x48/0x78
[ 3.740249] bus_add_driver+0x26c/0x5b8
[ 3.752248] driver_register+0x16c/0x398
[ 3.757211] __platform_driver_register+0xd8/0x128
[ 3.770860] virtio_mmio_init+0x1c/0x24
[ 3.782671] do_one_initcall+0xe0/0x398
[ 3.791890] kernel_init_freeable+0x594/0x660
[ 3.798514] kernel_init+0x18/0x190
[ 3.810220] ret_from_fork+0x10/0x18
To fix this, we can simply rip out the explicit cleanup that the devm
infrastructure will do for us when our probe function returns an error
code, or when our remove function returns.
We only need to ensure that we call put_device() if a call to
register_virtio_device() fails in the probe path.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Fixes: 7eb781b1bbb7136f ("virtio_mmio: add cleanup for virtio_mmio_probe")
Fixes: 25f32223bce5c580 ("virtio_mmio: add cleanup for virtio_mmio_remove")
Cc: Cornelia Huck <cohuck@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: weiping zhang <zhangweiping@didichuxing.com>
Cc: virtualization@lists.linux-foundation.org
---
drivers/virtio/virtio_mmio.c | 43 +++++++++----------------------------------
1 file changed, 9 insertions(+), 34 deletions(-)
Since v1 [1]:
* Fix cleanup in virtio_mmio_remove
Mark.
[1] https://lkml.kernel.org/r/20171212125302.6846-1-mark.rutland@arm.com
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index a9192fe4f345..c92131edfaba 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -522,10 +522,8 @@ static int virtio_mmio_probe(struct platform_device *pdev)
return -EBUSY;
vm_dev = devm_kzalloc(&pdev->dev, sizeof(*vm_dev), GFP_KERNEL);
- if (!vm_dev) {
- rc = -ENOMEM;
- goto free_mem;
- }
+ if (!vm_dev)
+ return -ENOMEM;
vm_dev->vdev.dev.parent = &pdev->dev;
vm_dev->vdev.dev.release = virtio_mmio_release_dev;
@@ -535,17 +533,14 @@ static int virtio_mmio_probe(struct platform_device *pdev)
spin_lock_init(&vm_dev->lock);
vm_dev->base = devm_ioremap(&pdev->dev, mem->start, resource_size(mem));
- if (vm_dev->base == NULL) {
- rc = -EFAULT;
- goto free_vmdev;
- }
+ if (vm_dev->base == NULL)
+ return -EFAULT;
/* Check magic value */
magic = readl(vm_dev->base + VIRTIO_MMIO_MAGIC_VALUE);
if (magic != ('v' | 'i' << 8 | 'r' << 16 | 't' << 24)) {
dev_warn(&pdev->dev, "Wrong magic value 0x%08lx!\n", magic);
- rc = -ENODEV;
- goto unmap;
+ return -ENODEV;
}
/* Check device version */
@@ -553,8 +548,7 @@ static int virtio_mmio_probe(struct platform_device *pdev)
if (vm_dev->version < 1 || vm_dev->version > 2) {
dev_err(&pdev->dev, "Version %ld not supported!\n",
vm_dev->version);
- rc = -ENXIO;
- goto unmap;
+ return -ENXIO;
}
vm_dev->vdev.id.device = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID);
@@ -563,8 +557,7 @@ static int virtio_mmio_probe(struct platform_device *pdev)
* virtio-mmio device with an ID 0 is a (dummy) placeholder
* with no function. End probing now with no error reported.
*/
- rc = -ENODEV;
- goto unmap;
+ return -ENODEV;
}
vm_dev->vdev.id.vendor = readl(vm_dev->base + VIRTIO_MMIO_VENDOR_ID);
@@ -590,33 +583,15 @@ static int virtio_mmio_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, vm_dev);
rc = register_virtio_device(&vm_dev->vdev);
- if (rc) {
- iounmap(vm_dev->base);
- devm_release_mem_region(&pdev->dev, mem->start,
- resource_size(mem));
+ if (rc)
put_device(&vm_dev->vdev.dev);
- }
- return rc;
-unmap:
- iounmap(vm_dev->base);
-free_mem:
- devm_release_mem_region(&pdev->dev, mem->start,
- resource_size(mem));
-free_vmdev:
- devm_kfree(&pdev->dev, vm_dev);
+
return rc;
}
static int virtio_mmio_remove(struct platform_device *pdev)
{
struct virtio_mmio_device *vm_dev = platform_get_drvdata(pdev);
- struct resource *mem;
-
- iounmap(vm_dev->base);
- mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (mem)
- devm_release_mem_region(&pdev->dev, mem->start,
- resource_size(mem));
unregister_virtio_device(&vm_dev->vdev);
return 0;
--
2.11.0
^ permalink raw reply related
* Re: [PATCH] virtio_mmio: fix devm cleanup
From: Mark Rutland @ 2017-12-12 13:34 UTC (permalink / raw)
To: Cornelia Huck
Cc: virtualization, weiping zhang, linux-kernel, Michael S . Tsirkin
In-Reply-To: <20171212143148.4a022816.cohuck@redhat.com>
On Tue, Dec 12, 2017 at 02:31:48PM +0100, Cornelia Huck wrote:
> On Tue, 12 Dec 2017 13:29:02 +0000
> Mark Rutland <mark.rutland@arm.com> wrote:
>
> > On Tue, Dec 12, 2017 at 02:09:52PM +0100, Cornelia Huck wrote:
>
> > > Shouldn't the cleanup in _remove() then be removed as well?
> >
> > Ah, I'd missed that.
> >
> > I believe that can be removed, yes.
> >
> > Should I spin a v2 with that folded in?
>
> I think it has the same devm_ioremap vs. iounmap discrepancy, so it
> should probably be folded in.
Ah, yes. I'll spin a v2 now.
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCH] virtio_mmio: fix devm cleanup
From: Cornelia Huck @ 2017-12-12 13:31 UTC (permalink / raw)
To: Mark Rutland
Cc: virtualization, weiping zhang, linux-kernel, Michael S . Tsirkin
In-Reply-To: <20171212132902.t5vxs2yujsth2v2h@lakrids.cambridge.arm.com>
On Tue, 12 Dec 2017 13:29:02 +0000
Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Dec 12, 2017 at 02:09:52PM +0100, Cornelia Huck wrote:
> > Shouldn't the cleanup in _remove() then be removed as well?
>
> Ah, I'd missed that.
>
> I believe that can be removed, yes.
>
> Should I spin a v2 with that folded in?
I think it has the same devm_ioremap vs. iounmap discrepancy, so it
should probably be folded in.
^ permalink raw reply
* Re: [PATCH] virtio_mmio: fix devm cleanup
From: Mark Rutland @ 2017-12-12 13:29 UTC (permalink / raw)
To: Cornelia Huck
Cc: virtualization, weiping zhang, linux-kernel, Michael S . Tsirkin
In-Reply-To: <20171212140952.64d7908a.cohuck@redhat.com>
On Tue, Dec 12, 2017 at 02:09:52PM +0100, Cornelia Huck wrote:
> On Tue, 12 Dec 2017 12:53:02 +0000
> Mark Rutland <mark.rutland@arm.com> wrote:
>
> > Recent rework of the virtio_mmio probe path balanced a devm_ioremap()
> > with an iounmap() rather than its devm variant. This ends up corrupting
> > the devm datastructures, and results in the following boot failure on
> > arm64 under QEMU 2.9.0:
> >
> > [ 3.450397] ------------[ cut here ]------------
> > [ 3.453822] Trying to vfree() nonexistent vm area (00000000c05b4844)
> > [ 3.460534] WARNING: CPU: 1 PID: 1 at mm/vmalloc.c:1525 __vunmap+0x1b8/0x220
> > [ 3.475898] Kernel panic - not syncing: panic_on_warn set ...
> > [ 3.475898]
> > [ 3.493933] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.15.0-rc3 #1
> > [ 3.513109] Hardware name: linux,dummy-virt (DT)
> > [ 3.525382] Call trace:
> > [ 3.531683] dump_backtrace+0x0/0x368
> > [ 3.543921] show_stack+0x20/0x30
> > [ 3.547767] dump_stack+0x108/0x164
> > [ 3.559584] panic+0x25c/0x51c
> > [ 3.569184] __warn+0x29c/0x31c
> > [ 3.576023] report_bug+0x1d4/0x290
> > [ 3.586069] bug_handler.part.2+0x40/0x100
> > [ 3.597820] bug_handler+0x4c/0x88
> > [ 3.608400] brk_handler+0x11c/0x218
> > [ 3.613430] do_debug_exception+0xe8/0x318
> > [ 3.627370] el1_dbg+0x18/0x78
> > [ 3.634037] __vunmap+0x1b8/0x220
> > [ 3.648747] vunmap+0x6c/0xc0
> > [ 3.653864] __iounmap+0x44/0x58
> > [ 3.659771] devm_ioremap_release+0x34/0x68
> > [ 3.672983] release_nodes+0x404/0x880
> > [ 3.683543] devres_release_all+0x6c/0xe8
> > [ 3.695692] driver_probe_device+0x250/0x828
> > [ 3.706187] __driver_attach+0x190/0x210
> > [ 3.717645] bus_for_each_dev+0x14c/0x1f0
> > [ 3.728633] driver_attach+0x48/0x78
> > [ 3.740249] bus_add_driver+0x26c/0x5b8
> > [ 3.752248] driver_register+0x16c/0x398
> > [ 3.757211] __platform_driver_register+0xd8/0x128
> > [ 3.770860] virtio_mmio_init+0x1c/0x24
> > [ 3.782671] do_one_initcall+0xe0/0x398
> > [ 3.791890] kernel_init_freeable+0x594/0x660
> > [ 3.798514] kernel_init+0x18/0x190
> > [ 3.810220] ret_from_fork+0x10/0x18
>
> Oops.
>
> > To fix this, we can simply rip out the explicit cleanup that the devm
> > infrastructure will do for us when our probe function returns an error
> > code. We only need to ensure that we call put_device() if a call to
> > register_virtio_device() fails.
>
> OK, that was the subtility I obviously missed. Reading through the
> code, this seems correct (although I find the infrastructure a bit
> unintuitive).
No worries.
> Shouldn't the cleanup in _remove() then be removed as well?
Ah, I'd missed that.
I believe that can be removed, yes.
Should I spin a v2 with that folded in?
Thanks,
Mark.
^ permalink raw reply
* [PATCH v2 3/3] virtio: put reference count of virtio_device.dev
From: weiping zhang @ 2017-12-12 13:25 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
In-Reply-To: <cover.1513083885.git.zhangweiping@didichuxing.com>
rproc_virtio_dev_release will be called iff virtio_device.dev's
refer count became to 0. Here we should put vdev.dev, and then
rproc->dev's cleanup will be done in rproc_virtio_dev_release.
Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
drivers/remoteproc/remoteproc_virtio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index 2946348..b0633fd 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -327,7 +327,7 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
ret = register_virtio_device(vdev);
if (ret) {
- put_device(&rproc->dev);
+ put_device(&vdev->dev);
dev_err(dev, "failed to register vdev: %d\n", ret);
goto out;
}
--
2.9.4
^ permalink raw reply related
* [PATCH v2 2/3] virtio: use put_device instead of kfree
From: weiping zhang @ 2017-12-12 13:24 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
In-Reply-To: <cover.1513083885.git.zhangweiping@didichuxing.com>
As mentioned at drivers/base/core.c:
/*
* NOTE: _Never_ directly free @dev after calling this function, even
* if it returned an error! Always use put_device() to give up the
* reference initialized in this function instead.
*/
so we don't free vp_vdev until vp_vdev.dev.release be called.
Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
---
drivers/misc/mic/vop/vop_main.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c
index a341938..456e969 100644
--- a/drivers/misc/mic/vop/vop_main.c
+++ b/drivers/misc/mic/vop/vop_main.c
@@ -452,10 +452,12 @@ static irqreturn_t vop_virtio_intr_handler(int irq, void *data)
static void vop_virtio_release_dev(struct device *_d)
{
- /*
- * No need for a release method similar to virtio PCI.
- * Provide an empty one to avoid getting a warning from core.
- */
+ struct virtio_device *vdev =
+ container_of(_d, struct virtio_device, dev);
+ struct _vop_vdev *vop_vdev =
+ container_of(vdev, struct _vop_vdev, vdev);
+
+ kfree(vop_vdev);
}
/*
@@ -501,7 +503,9 @@ static int _vop_add_device(struct mic_device_desc __iomem *d,
dev_err(_vop_dev(vdev),
"Failed to register vop device %u type %u\n",
offset, type);
- goto free_irq;
+ vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev);
+ put_device(&vdev->vdev.dev);
+ return ret;
}
writeq((u64)vdev, &vdev->dc->vdev);
dev_dbg(_vop_dev(vdev), "%s: registered vop device %u type %u vdev %p\n",
@@ -509,8 +513,6 @@ static int _vop_add_device(struct mic_device_desc __iomem *d,
return 0;
-free_irq:
- vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev);
kfree:
kfree(vdev);
return ret;
@@ -568,7 +570,7 @@ static int _vop_remove_device(struct mic_device_desc __iomem *d,
iowrite8(-1, &dc->h2c_vdev_db);
if (status & VIRTIO_CONFIG_S_DRIVER_OK)
wait_for_completion(&vdev->reset_done);
- kfree(vdev);
+ put_device(&vdev->vdev.dev);
iowrite8(1, &dc->guest_ack);
dev_dbg(&vpdev->dev, "%s %d guest_ack %d\n",
__func__, __LINE__, ioread8(&dc->guest_ack));
--
2.9.4
^ permalink raw reply related
* [PATCH v2 1/3] virtio_pci: use put_device instead of kfree
From: weiping zhang @ 2017-12-12 13:24 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
In-Reply-To: <cover.1513083885.git.zhangweiping@didichuxing.com>
As mentioned at drivers/base/core.c:
/*
* NOTE: _Never_ directly free @dev after calling this function, even
* if it returned an error! Always use put_device() to give up the
* reference initialized in this function instead.
*/
so we don't free vp_dev until vp_dev->vdev.dev.release be called.
Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
drivers/virtio/virtio_pci_common.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index 1c4797e..91d20f7 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -551,16 +551,17 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
pci_set_master(pci_dev);
rc = register_virtio_device(&vp_dev->vdev);
- if (rc)
- goto err_register;
+ if (rc) {
+ if (vp_dev->ioaddr)
+ virtio_pci_legacy_remove(vp_dev);
+ else
+ virtio_pci_modern_remove(vp_dev);
+ pci_disable_device(pci_dev);
+ put_device(&vp_dev->vdev.dev);
+ }
- return 0;
+ return rc;
-err_register:
- if (vp_dev->ioaddr)
- virtio_pci_legacy_remove(vp_dev);
- else
- virtio_pci_modern_remove(vp_dev);
err_probe:
pci_disable_device(pci_dev);
err_enable_device:
--
2.9.4
^ permalink raw reply related
* [PATCH v2 0/3] fix cleanup for fail to register_virtio_device
From: weiping zhang @ 2017-12-12 13:13 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
This series fix the cleanup for the caller of register_virtio_device,
the main work is use put_device instead of kfree.
V1->V2:
* virtio_pci: add comments for the reason use put_device
* virtio vop: also use put_device in in _vop_remove_device()
weiping zhang (3):
virtio_pci: use put_device instead of kfree
virtio: use put_device instead of kfree
virtio: put reference count of virtio_device.dev
drivers/misc/mic/vop/vop_main.c | 18 ++++++++++--------
drivers/remoteproc/remoteproc_virtio.c | 2 +-
drivers/virtio/virtio_pci_common.c | 17 +++++++++--------
3 files changed, 20 insertions(+), 17 deletions(-)
--
2.9.4
^ permalink raw reply
* Re: [PATCH 2/3] virtio: use put_device instead of kfree
From: weiping zhang @ 2017-12-12 13:13 UTC (permalink / raw)
To: Cornelia Huck; +Cc: virtualization, mst
In-Reply-To: <20171212111742.4249c49e.cohuck@redhat.com>
On Tue, Dec 12, 2017 at 11:17:42AM +0100, Cornelia Huck wrote:
> On Mon, 11 Dec 2017 23:55:26 +0800
> weiping zhang <zhangweiping@didichuxing.com> wrote:
>
> > don't free vp_vdev until vp_vdev.dev.release be called.
>
> Same comment as for the virtio_pci patch.
OK, I'll add it at V2.
>
> >
> > Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> > ---
> > drivers/misc/mic/vop/vop_main.c | 16 +++++++++-------
> > 1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c
> > index a341938..8c716a0 100644
> > --- a/drivers/misc/mic/vop/vop_main.c
> > +++ b/drivers/misc/mic/vop/vop_main.c
> > @@ -452,10 +452,12 @@ static irqreturn_t vop_virtio_intr_handler(int irq, void *data)
> >
> > static void vop_virtio_release_dev(struct device *_d)
> > {
> > - /*
> > - * No need for a release method similar to virtio PCI.
> > - * Provide an empty one to avoid getting a warning from core.
> > - */
> > + struct virtio_device *vdev =
> > + container_of(_d, struct virtio_device, dev);
> > + struct _vop_vdev *vop_vdev =
> > + container_of(vdev, struct _vop_vdev, vdev);
> > +
> > + kfree(vop_vdev);
> > }
> >
> > /*
> > @@ -501,7 +503,9 @@ static int _vop_add_device(struct mic_device_desc __iomem *d,
> > dev_err(_vop_dev(vdev),
> > "Failed to register vop device %u type %u\n",
> > offset, type);
> > - goto free_irq;
> > + vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev);
> > + put_device(&vdev->vdev.dev);
> > + return ret;
> > }
> > writeq((u64)vdev, &vdev->dc->vdev);
> > dev_dbg(_vop_dev(vdev), "%s: registered vop device %u type %u vdev %p\n",
> > @@ -509,8 +513,6 @@ static int _vop_add_device(struct mic_device_desc __iomem *d,
> >
> > return 0;
> >
> > -free_irq:
> > - vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev);
> > kfree:
> > kfree(vdev);
> > return ret;
>
> BTW, the kfree(vdev) in _vop_remove_device() is also wrong (needs to be
> a put_device() as well).
thanks for your comments, I'll fix it at v2.
^ permalink raw reply
* Re: [PATCH 1/3] virtio_pci: use put_device instead of kfree
From: weiping zhang @ 2017-12-12 13:11 UTC (permalink / raw)
To: Cornelia Huck; +Cc: virtualization, mst
In-Reply-To: <20171212110025.5d029fce.cohuck@redhat.com>
On Tue, Dec 12, 2017 at 11:00:25AM +0100, Cornelia Huck wrote:
> On Mon, 11 Dec 2017 23:55:16 +0800
> weiping zhang <zhangweiping@didichuxing.com> wrote:
>
> > don't free vp_dev until vp_dev->vdev.dev.release be called.
>
> Maybe add the same description as in your virtio_mmio patch so that it
> is clear why the kfree() is not ok?
OK, I'll add it at V2.
> >
> > Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> > ---
> > drivers/virtio/virtio_pci_common.c | 17 +++++++++--------
> > 1 file changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > index 1c4797e..91d20f7 100644
> > --- a/drivers/virtio/virtio_pci_common.c
> > +++ b/drivers/virtio/virtio_pci_common.c
> > @@ -551,16 +551,17 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
> > pci_set_master(pci_dev);
> >
> > rc = register_virtio_device(&vp_dev->vdev);
> > - if (rc)
> > - goto err_register;
> > + if (rc) {
> > + if (vp_dev->ioaddr)
> > + virtio_pci_legacy_remove(vp_dev);
> > + else
> > + virtio_pci_modern_remove(vp_dev);
> > + pci_disable_device(pci_dev);
> > + put_device(&vp_dev->vdev.dev);
> > + }
> >
> > - return 0;
> > + return rc;
> >
> > -err_register:
> > - if (vp_dev->ioaddr)
> > - virtio_pci_legacy_remove(vp_dev);
> > - else
> > - virtio_pci_modern_remove(vp_dev);
> > err_probe:
> > pci_disable_device(pci_dev);
> > err_enable_device:
>
> Otherwise, looks good.
>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Thanks a ton
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox