| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20240703110812-mutt-send-email-mst@kernel.org>
Date: Wed, 3 Jul 2024 11:18:58 -0400
From: "Michael S. Tsirkin" <mst@...hat.com>
To: Mikhail Krasheninnikov <krashmisha@...il.com>
Cc: linux-kernel@...r.kernel.org, Ulf Hansson <ulf.hansson@...aro.org>,
linux-mmc@...r.kernel.org,
Matwey Kornilov <matwey.kornilov@...il.com>
Subject: Re: [PATCH] mmc/virtio: Add virtio MMC driver for QEMU emulation
On Mon, Jul 01, 2024 at 12:06:42PM +0000, Mikhail Krasheninnikov wrote:
> Introduce a new virtio MMC driver to enable virtio SD/MMC card
> emulation with QEMU. This driver allows emulating MMC cards in
> virtual environments, enhancing functionality and testing
> capabilities within QEMU.
>
> Link to the QEMU patch: https://lists.nongnu.org/archive/html/qemu-block/2024-06/msg00664.html
>
> No changes to existing dependencies or documentation.
>
> Signed-off-by: Mikhail Krasheninnikov <krashmisha@...il.com>
> CC: Ulf Hansson <ulf.hansson@...aro.org>
> CC: linux-mmc@...r.kernel.org
> CC: Matwey Kornilov <matwey.kornilov@...il.com>
you should likely add a MAINTAINERS entry so people
know to copy virtio ML and core maintainers when they are
making changes.
> ---
> drivers/mmc/core/mmc_ops.c | 2 +
> drivers/mmc/host/Kconfig | 14 ++
> drivers/mmc/host/Makefile | 2 +
> drivers/mmc/host/virtio-mmc.c | 266 ++++++++++++++++++++++++++++++++
> drivers/mmc/host/virtio-mmc.h | 42 +++++
> include/uapi/linux/virtio_ids.h | 1 +
> 6 files changed, 327 insertions(+)
> create mode 100644 drivers/mmc/host/virtio-mmc.c
> create mode 100644 drivers/mmc/host/virtio-mmc.h
>
> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> index 3b3adbddf664..4e663140048a 100644
> --- a/drivers/mmc/core/mmc_ops.c
> +++ b/drivers/mmc/core/mmc_ops.c
> @@ -5,6 +5,7 @@
> * Copyright 2006-2007 Pierre Ossman
> */
>
> +#include "linux/printk.h"
> #include <linux/slab.h>
> #include <linux/export.h>
> #include <linux/types.h>
> @@ -459,6 +460,7 @@ int mmc_switch_status(struct mmc_card *card, bool crc_err_fatal)
>
> static int mmc_busy_cb(void *cb_data, bool *busy)
> {
> + pr_info("mmc_busy_cb\n");
> struct mmc_busy_data *data = cb_data;
> struct mmc_host *host = data->card->host;
> u32 status = 0;
Debugging leftovers? Does not inspire confidence...
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 554e67103c1a..eb0b0e80250d 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -1069,3 +1069,17 @@ config MMC_LITEX
> module will be called litex_mmc.
>
> If unsure, say N.
> +
> +config MMC_VIRTIO
> + tristate "VirtIO MMC Host Controller support"
> + depends on VIRTIO
> + help
> + This enables support for the Virtio MMC driver, which allows the
> + kernel to interact with MMC devices over Virtio. Virtio is a
> + virtualization standard for network and disk device drivers,
> + providing a common API for virtualized environments.
> +
> + Enable this option if you are running the kernel in a virtualized
> + environment and need MMC support via Virtio.
> +
> + If unsure, say N.
> \ No newline at end of file
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index a693fa3d3f1c..d53493d0a692 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -108,3 +108,5 @@ endif
>
> obj-$(CONFIG_MMC_SDHCI_XENON) += sdhci-xenon-driver.o
> sdhci-xenon-driver-y += sdhci-xenon.o sdhci-xenon-phy.o
> +
> +obj-$(CONFIG_MMC_VIRTIO) += virtio-mmc.o
> \ No newline at end of file
> diff --git a/drivers/mmc/host/virtio-mmc.c b/drivers/mmc/host/virtio-mmc.c
> new file mode 100644
> index 000000000000..b192bd0b9ffd
> --- /dev/null
> +++ b/drivers/mmc/host/virtio-mmc.c
> @@ -0,0 +1,266 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * VirtIO SD/MMC driver
> + *
> + * Author: Mikhail Krasheninnikov <krashmisha@...il.com>
> + */
> +
> +#include "virtio-mmc.h"
> +#include "asm-generic/int-ll64.h"
> +#include "linux/completion.h"
> +#include "linux/kern_levels.h"
> +#include "linux/mmc/core.h"
> +#include "linux/mmc/host.h"
> +#include "linux/printk.h"
> +#include "linux/scatterlist.h"
> +#include "linux/types.h"
> +#include "linux/virtio_config.h"
> +#include <linux/virtio.h>
> +#include <linux/cdev.h>
> +#include <linux/device.h>
> +#include <linux/fs.h>
why this weird order and way to do includes?
and it looks like you are including way too much stuff.
> +
> +struct mmc_req {
> + u32 opcode;
> + u32 arg;
Using CPU endian format for driver to device communication
is not a good idea.
> +};
> +
> +struct virtio_mmc_request {
> + u8 flags;
> +
> +#define VIRTIO_MMC_REQUEST_DATA BIT(1)
> +#define VIRTIO_MMC_REQUEST_WRITE BIT(2)
> +#define VIRTIO_MMC_REQUEST_STOP BIT(3)
> +#define VIRTIO_MMC_REQUEST_SBC BIT(4)
> +
> + struct mmc_req request;
> +
> + u8 buf[4096];
> + size_t buf_len;
size_t is definitely not a good idea in a driver to device
communication.
> +
> + struct mmc_req stop_req;
> + struct mmc_req sbc_req;
> +};
> +
> +struct virtio_mmc_response {
> + u32 cmd_resp[4];
> + int cmd_resp_len;
int is also not a good idea in a driver to device communication.
> + u8 buf[4096];
> +};
looks like there's a bunch of stuff here that belongs in
uapi/linux/virtio-mmc.h (or whatever you decide to name this).
> +
> +struct virtio_mmc_host {
> + struct virtio_device *vdev;
> + struct mmc_host *mmc;
> + struct virtqueue *vq;
> + struct mmc_request *current_request;
> +
> + struct virtio_mmc_request virtio_request;
> + struct virtio_mmc_response virtio_response;
> +
> + struct completion request_handled;
> +};
> +
> +static void virtio_mmc_vq_callback(struct virtqueue *vq)
> +{
> + unsigned int len;
> + struct mmc_host *mmc;
> + struct virtio_mmc_host *host;
> + struct virtio_mmc_request *virtio_request;
> + struct virtio_mmc_response *virtio_response;
> + struct mmc_request *mrq;
> +
> + mmc = vq->vdev->priv;
> + host = mmc_priv(mmc);
> + mrq = host->current_request;
> + virtio_request = &host->virtio_request;
> +
> + virtio_response = virtqueue_get_buf(vq, &len);
> +
> + memcpy(mrq->cmd->resp, virtio_response->cmd_resp,
> + virtio_response->cmd_resp_len);
blindly trusting device that cmd_resp_len will not overflow
the buffer is not a good idea.
> +
> + if (virtio_request->flags & VIRTIO_MMC_REQUEST_DATA) {
> + if (!(virtio_request->flags & VIRTIO_MMC_REQUEST_WRITE)) {
> + sg_copy_from_buffer(mrq->data->sg, mrq->data->sg_len,
> + virtio_response->buf,
> + virtio_request->buf_len);
> + }
> + mrq->data->bytes_xfered = virtio_request->buf_len;
same here - validate it's reasonable
> + }
> +
> + host->current_request = NULL;
> + mmc_request_done(mmc, mrq);
> + complete(&host->request_handled);
> +}
> +
> +static void virtio_mmc_send_request_to_qemu(struct virtio_mmc_host *data)
> +{
> + struct scatterlist sg_out_linux, sg_in_linux;
> +
> + sg_init_one(&sg_out_linux, &data->virtio_request,
> + sizeof(struct virtio_mmc_request));
> + sg_init_one(&sg_in_linux, &data->virtio_response,
> + sizeof(struct virtio_mmc_response));
> +
> + struct scatterlist *request[] = { &sg_out_linux, &sg_in_linux };
> +
> + if (virtqueue_add_sgs(data->vq, request, 1, 1, &data->virtio_response,
> + GFP_KERNEL) < 0) {
> + dev_crit(&data->vdev->dev, "virtio_mmc: Failed to add sg\n");
> + return;
> + }
> +
> + virtqueue_kick(data->vq);
> + wait_for_completion(&data->request_handled);
> +}
> +
> +static inline size_t __calculate_len(struct mmc_data *data)
> +{
> + size_t len = 0;
> +
> + for (int i = 0; i < data->sg_len; i++)
> + len += data->sg[i].length;
> + return len;
> +}
> +
> +/* MMC layer callbacks */
> +
> +static void virtio_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
> +{
> + struct virtio_mmc_host *host;
> + struct virtio_mmc_request *virtio_req;
> + struct mmc_data *mrq_data;
> +
> + host = mmc_priv(mmc);
> + host->current_request = mrq; // Saving the request for the callback
no locking anywhere ...
does something else guarantee there's always just one request
outstanding?
> +
> + virtio_req = &host->virtio_request;
> + memset(virtio_req, 0, sizeof(struct virtio_mmc_request));
> +
> + virtio_req->request.opcode = mrq->cmd->opcode;
> + virtio_req->request.arg = mrq->cmd->arg;
> +
> + mrq_data = mrq->data;
> + if (mrq_data) {
> + virtio_req->flags |= VIRTIO_MMC_REQUEST_DATA;
> +
> + virtio_req->buf_len = __calculate_len(mrq->data);
> +
> + virtio_req->flags |= ((mrq_data->flags & MMC_DATA_WRITE) ?
> + VIRTIO_MMC_REQUEST_WRITE :
> + 0);
> + if (virtio_req->flags & VIRTIO_MMC_REQUEST_WRITE) {
> + sg_copy_to_buffer(mrq_data->sg, mrq_data->sg_len,
> + virtio_req->buf, virtio_req->buf_len);
> + }
> + }
> +
> + if (mrq->stop) {
> + virtio_req->flags |= VIRTIO_MMC_REQUEST_STOP;
> +
> + virtio_req->stop_req.opcode = mrq->stop->opcode;
> + virtio_req->stop_req.arg = mrq->stop->arg;
> + }
> +
> + if (mrq->sbc) {
> + virtio_req->flags |= VIRTIO_MMC_REQUEST_SBC;
> +
> + virtio_req->sbc_req.opcode = mrq->sbc->opcode;
> + virtio_req->sbc_req.arg = mrq->sbc->arg;
> + }
> +
> + virtio_mmc_send_request_to_qemu(host);
> +}
> +
> +static void virtio_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> +{
> +}
> +
> +static int virtio_mmc_get_ro(struct mmc_host *mmc)
> +{
> + return 0;
> +}
> +
> +static int virtio_mmc_get_cd(struct mmc_host *mmc)
> +{
> + return 1;
> +}
> +
> +static const struct mmc_host_ops virtio_mmc_host_ops = {
> + .request = virtio_mmc_request,
> + .set_ios = virtio_mmc_set_ios,
> + .get_ro = virtio_mmc_get_ro,
> + .get_cd = virtio_mmc_get_cd,
> +};
> +
> +static inline void __fill_host_attr(struct mmc_host *host)
> +{
> + host->ops = &virtio_mmc_host_ops;
> + host->f_min = 300000;
> + host->f_max = 500000;
> + host->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
> + host->caps = MMC_CAP_SD_HIGHSPEED;
> + host->caps2 = MMC_CAP2_NO_SDIO | MMC_CAP2_NO_MMC | MMC_CAP2_HS400;
> +}
> +
> +static int create_host(struct virtio_device *vdev)
> +{
> + int err;
> + struct mmc_host *mmc;
> + struct virtio_mmc_host *host;
> +
> + mmc = mmc_alloc_host(sizeof(struct virtio_mmc_host), &vdev->dev);
> + if (!mmc) {
> + pr_err("virtio_mmc: Failed to allocate host\n");
> + return -ENOMEM;
> + }
> +
> + __fill_host_attr(mmc);
> +
> + vdev->priv = mmc;
> +
> + host = mmc_priv(mmc);
> +
> + init_completion(&host->request_handled);
> +
> + host->vq =
> + virtio_find_single_vq(vdev, virtio_mmc_vq_callback, "vq_name");
> + if (!host->vq) {
> + pr_err("virtio_mmc: Failed to find virtqueue\n");
> + mmc_free_host(mmc);
> + return -ENODEV;
> + }
> +
> + err = mmc_add_host(mmc);
can you get requests immediately at this point?
if yes you need to call virtio_device_ready before
you add buffers.
> + if (err) {
> + pr_err("virtio_mmc: Failed to add host\n");
> + mmc_free_host(mmc);
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static int virtio_mmc_probe(struct virtio_device *vdev)
> +{
> + int err;
> +
> + err = create_host(vdev);
> + if (err)
> + pr_err("virtio_mmc: Failed to make host\n");
> +
> + return 0;
> +}
> +
> +static void remove_host(struct mmc_host *host)
> +{
does not look like you are doing anything to quiesce the
device.
> + mmc_remove_host(host);
> + mmc_free_host(host);
> +}
> +
> +static void virtio_mmc_remove(struct virtio_device *vdev)
> +{
> + struct mmc_host *host = vdev->priv;
> +
> + remove_host(host);
> +}
> diff --git a/drivers/mmc/host/virtio-mmc.h b/drivers/mmc/host/virtio-mmc.h
> new file mode 100644
> index 000000000000..086f33307f84
> --- /dev/null
> +++ b/drivers/mmc/host/virtio-mmc.h
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * VirtIO SD/MMC driver
> + *
> + * Author: Mikhail Krasheninnikov <krashmisha@...il.com>
> + */
> +
> +#ifndef _VIRTIO_MMC_H
> +#define _VIRTIO_MMC_H
> +
> +#include <linux/virtio.h>
> +#include <linux/virtio_ids.h>
> +
> +#define VIRTIO_MMC_DEV_ID 42
> +
> +static int virtio_mmc_probe(struct virtio_device *vdev);
> +
> +static void virtio_mmc_remove(struct virtio_device *vdev);
> +
> +static const struct virtio_device_id id_table[] = {
> + { VIRTIO_MMC_DEV_ID, VIRTIO_DEV_ANY_ID },
> + { 0 },
> +};
> +
> +static struct virtio_driver virtio_mmc_driver = {
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .owner = THIS_MODULE,
> + },
> + .id_table = id_table,
> + .probe = virtio_mmc_probe,
> + .remove = virtio_mmc_remove,
> +};
> +
> +module_virtio_driver(virtio_mmc_driver);
> +MODULE_DEVICE_TABLE(virtio, id_table);
> +
> +MODULE_AUTHOR("Mikhail Krasheninnikov");
> +MODULE_DESCRIPTION("VirtIO SD/MMC driver");
> +MODULE_LICENSE("GPL");
> +
> +#endif
> diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
> index 7aa2eb766205..9c8a09b0f004 100644
> --- a/include/uapi/linux/virtio_ids.h
> +++ b/include/uapi/linux/virtio_ids.h
> @@ -68,6 +68,7 @@
> #define VIRTIO_ID_AUDIO_POLICY 39 /* virtio audio policy */
> #define VIRTIO_ID_BT 40 /* virtio bluetooth */
> #define VIRTIO_ID_GPIO 41 /* virtio gpio */
> +#define VIRTIO_ID_MMC 42 /* virtio mmc */
>
> /*
> * Virtio Transitional IDs
> --
> 2.34.1
>
Powered by blists - more mailing lists