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@xxxxxxxxx> > CC: Ulf Hansson <ulf.hansson@xxxxxxxxxx> > CC: linux-mmc@xxxxxxxxxxxxxxx > CC: Matwey Kornilov <matwey.kornilov@xxxxxxxxx> 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@xxxxxxxxx> > + */ > + > +#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@xxxxxxxxx> > + */ > + > +#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 >
