Re: [PATCH v2] virtio: Implement Virtio Backend for SD/MMC in QEMU

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi Mikhail,

(mostly style comments)

On 2/7/24 20:58, Mikhail Krasheninnikov wrote:
> From: Mi <krashmisha@gmail.com>
> 
> Add a Virtio backend for SD/MMC devices. Confirmed interoperability with
> Linux.
> 
> Signed-off-by: Mikhail Krasheninnikov <krashmisha@gmail.com>
> CC: Matwey Kornilov <matwey.kornilov@gmail.com>
> CC: qemu-block@nongnu.org
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> 
> After a feedback, moved virtio.c from virtio core directory to hw/block.
>  From what I see from the examples of virtio drivers, other files should
> be where they are now. Correct me if I'm wrong.

Alternative is hw/sd/.

> 
>   hw/block/Kconfig                            |   5 +
>   hw/block/meson.build                        |   1 +
>   hw/block/virtio-mmc.c                       | 165 ++++++++++++++++++++

virtio-sdhci.c could be a better name.

>   hw/virtio/meson.build                       |   1 +
>   hw/virtio/virtio-mmc-pci.c                  |  85 ++++++++++
>   hw/virtio/virtio.c                          |   3 +-
>   include/hw/virtio/virtio-mmc.h              |  20 +++
>   include/standard-headers/linux/virtio_ids.h |   1 +
>   8 files changed, 280 insertions(+), 1 deletion(-)
>   create mode 100644 hw/block/virtio-mmc.c
>   create mode 100644 hw/virtio/virtio-mmc-pci.c
>   create mode 100644 include/hw/virtio/virtio-mmc.h
> 
> diff --git a/hw/block/Kconfig b/hw/block/Kconfig
> index 9e8f28f982..a3059261fa 100644
> --- a/hw/block/Kconfig
> +++ b/hw/block/Kconfig
> @@ -44,3 +44,8 @@ config VHOST_USER_BLK
>   
>   config SWIM
>       bool
> +
> +config VIRTIO_MMC

(config VIRTIO_SDHCI?)

> +    bool
> +    default y

for SDBus API:

        select SD

> +    depends on VIRTIO

[...]

> diff --git a/hw/block/virtio-mmc.c b/hw/block/virtio-mmc.c
> new file mode 100644
> index 0000000000..50bd7113c5
> --- /dev/null
> +++ b/hw/block/virtio-mmc.c
> @@ -0,0 +1,165 @@
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +
> +#include "hw/virtio/virtio.h"
> +#include "hw/virtio/virtio-mmc.h"
> +#include "qemu/typedefs.h"
> +#include "sysemu/blockdev.h"
> +
> +typedef struct mmc_req {
> +    uint32_t opcode;
> +    uint32_t arg;
> +} mmc_req;
> +
> +typedef struct virtio_mmc_req {
> +    uint8_t 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)
> +
> +    mmc_req request;
> +
> +    uint8_t buf[4096];
> +    size_t buf_len;
> +
> +    mmc_req stop_req;
> +    mmc_req sbc_req;
> +} virtio_mmc_req;
> +
> +typedef struct virtio_mmc_resp {
> +    uint32_t response[4];
> +    int resp_len;
> +    uint8_t buf[4096];
> +} virtio_mmc_resp;
> +
> +static void send_command(SDBus *sdbus, mmc_req *mmc_request, uint8_t *response,
> +                         virtio_mmc_resp *virtio_resp)
> +{
> +    SDRequest sdreq;

QEMU style declares variables in function prologue.

> +    sdreq.cmd = (uint8_t)mmc_request->opcode;
> +    sdreq.arg = mmc_request->arg;
> +    int resp_len = sdbus_do_command(sdbus, &sdreq, response);
> +    virtio_resp->resp_len = resp_len;
> +
> +    for (int i = 0; i < resp_len / sizeof(uint32_t); i++) {
> +        virtio_resp->response[i] = ldl_be_p(&virtio_resp->response[i]);
> +    }
> +}
> +
> +static void send_command_without_response(SDBus *sdbus, mmc_req *mmc_request)
> +{
> +    SDRequest sdreq;
> +    sdreq.cmd = (uint8_t)mmc_request->opcode;
> +    sdreq.arg = mmc_request->arg;
> +    uint8_t response[4];

Ditto style (various occurences).

> +    sdbus_do_command(sdbus, &sdreq, response);
> +}

[...]
> diff --git a/include/hw/virtio/virtio-mmc.h b/include/hw/virtio/virtio-mmc.h
> new file mode 100644
> index 0000000000..a68f45d7cb
> --- /dev/null
> +++ b/include/hw/virtio/virtio-mmc.h
> @@ -0,0 +1,20 @@
> +#pragma once
> +
> +#include "hw/virtio/virtio.h"
> +#include "hw/sd/sd.h"
> +#include "qemu/typedefs.h"
> +
> +#define VIRTIO_ID_MMC 42
> +
> +#define TYPE_VIRTIO_MMC "virtio-mmc-device"
> +#define VIRTIO_MMC(obj) \
> +    OBJECT_CHECK(VirtIOMMC, (obj), TYPE_VIRTIO_MMC)
> +#define VIRTIO_MMC_GET_PARENT_CLASS(obj) \
> +    OBJECT_GET_PARENT_CLASS(VIRTIO_MMC(obj), TYPE_VIRTIO_MMC)
> +
> +typedef struct VirtIOMMC {
> +    VirtIODevice parent_obj;

Please add a newline here

> +    VirtQueue *vq;
> +    SDBus sdbus;
> +    BlockBackend *blk;
> +} VirtIOMMC;

Otherwise (skipping virtio), for SD/MMC LGTM so far.

Regards,

Phil.