Re: [Minios-devel] [UNIKRAFT PATCH v2 03/12] plat/drivers: Init virtio block device

["Justin He (Arm Technology China)" <Justin.He@xxxxxxx>]

Hi Roxana

> -----Original Message-----
> From: Minios-devel <minios-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf
> Of Roxana Nicolescu
> Sent: Wednesday, October 30, 2019 6:46 PM
> To: minios-devel@xxxxxxxxxxxxx
> Cc: sharan.santhanam@xxxxxxxxx
> Subject: [Minios-devel] [UNIKRAFT PATCH v2 03/12] plat/drivers: Init virtio
> block device
>
> This patch introduces the initialization of virtio block device.
> First, the driver sets what features supports, and then the driver
> negotiates with the device for the common features.
>
> Signed-off-by: Roxana Nicolescu <nicolescu.roxana1996@xxxxxxxxx>
> ---
>  plat/drivers/include/virtio/virtio_blk.h | 52 ++++++++++++++
>  plat/drivers/virtio/virtio_blk.c         | 88 ++++++++++++++++++++++++
>  2 files changed, 140 insertions(+)
>  create mode 100644 plat/drivers/include/virtio/virtio_blk.h
>
> diff --git a/plat/drivers/include/virtio/virtio_blk.h
> b/plat/drivers/include/virtio/virtio_blk.h
> new file mode 100644
> index 00000000..48625258
> --- /dev/null
> +++ b/plat/drivers/include/virtio/virtio_blk.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: BSD-3-Clause */
> +/* This header is BSD licensed so anyone can use the definitions to
> implement
> + * compatible drivers/servers.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of IBM nor the names of its contributors
> + *    may be used to endorse or promote products derived from this
> software
> + *    without specific prior written permission.
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> CONTRIBUTORS
> + * ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
> THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
> PARTICULAR PURPOSE
> + * ARE DISCLAIMED.  IN NO EVENT SHALL IBM OR CONTRIBUTORS BE LIABLE
> + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> SUBSTITUTE GOODS
> + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> INTERRUPTION)
> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> CONTRACT, STRICT
> + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN
> ANY WAY
> + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> POSSIBILITY OF
> + * SUCH DAMAGE.
> + **/
> +/**
> + * Taken and modified from Linux kernel
> + * include/uapi/linux/virtio_blk.h
> + *
> + * commit-id: 8005803a2
> + */
> +#ifndef __PLAT_DRV_VIRTIO_BLK_H
> +#define __PLAT_DRV_VIRTIO_BLK_H
> +#include <virtio/virtio_ids.h>
> +#include <virtio/virtio_config.h>
> +#include <virtio/virtio_types.h>
> +
> +/* Feature bits */
> +#define VIRTIO_BLK_F_RO              5       /* Disk is read-only */
> +#define VIRTIO_BLK_F_BLK_SIZE        6       /* Block size of disk is
> available*/
> +
> +struct virtio_blk_config {
> +     /* The capacity (in 512-byte sectors). */
> +     __u64 capacity;
> +     /* block size of device (if VIRTIO_BLK_F_BLK_SIZE) */
> +     __u32 blk_size;
> +} __packed;
> +
> +#endif /* __PLAT_DRV_VIRTIO_BLK_H */
> diff --git a/plat/drivers/virtio/virtio_blk.c 
> b/plat/drivers/virtio/virtio_blk.c
> index 884f86d9..09724d96 100644
> --- a/plat/drivers/virtio/virtio_blk.c
> +++ b/plat/drivers/virtio/virtio_blk.c
> @@ -28,13 +28,22 @@
>  #include <virtio/virtio_bus.h>
>  #include <virtio/virtio_ids.h>
>  #include <uk/blkdev.h>
> +#include <virtio/virtio_blk.h>
>  #include <uk/blkdev_driver.h>
>
>  #define DRIVER_NAME          "virtio-blk"
> +#define DEFAULT_SECTOR_SIZE  512
>
>  #define to_virtioblkdev(bdev) \
>       __containerof(bdev, struct virtio_blk_device, blkdev)
>
> +/* Features are:
> + *   Access Mode
> + *   Sector_size;
> + **/
> +#define VIRTIO_BLK_DRV_FEATURES(features) \
> +     (VIRTIO_FEATURES_UPDATE(features, VIRTIO_BLK_F_RO | \
> +     VIRTIO_BLK_F_BLK_SIZE))
>
>  static struct uk_alloc *a;
>  static const char *drv_name = DRIVER_NAME;
> @@ -44,6 +53,74 @@ struct virtio_blk_device {
>       struct uk_blkdev blkdev;
>       /* The blkdevice identifier */
>       __u16 uid;
> +     /* Virtio Device */
> +     struct virtio_dev *vdev;
> +};
> +
> +static int virtio_blkdev_feature_negotiate(struct virtio_blk_device *vbdev)
> +{
> +     struct uk_blkdev_cap *cap;
> +     __u64 host_features = 0;
> +     int bytes_to_read;
> +     __sector sectors;
> +     __sector ssize;
> +     int rc = 0;
> +
> +     UK_ASSERT(vbdev);
> +     cap = &vbdev->blkdev.capabilities;
> +     host_features = virtio_feature_get(vbdev->vdev);
> +
> +     /* Get size of device */
> +     bytes_to_read = virtio_config_get(vbdev->vdev,
> +                     __offsetof(struct virtio_blk_config, capacity),
> +                     &sectors,
> +                     sizeof(sectors),
> +                     1);
> +     if (bytes_to_read != sizeof(sectors))  {
> +             uk_pr_err("Failed to get nb of sectors from device %d\n", rc);
> +             rc = -EAGAIN;
> +             goto exit;
> +     }
> +
> +     if (!virtio_has_features(host_features, VIRTIO_BLK_F_BLK_SIZE)) {
> +             ssize = DEFAULT_SECTOR_SIZE;
> +     } else {
> +             bytes_to_read = virtio_config_get(vbdev->vdev,
> +                             __offsetof(struct virtio_blk_config, blk_size),
> +                             &ssize,
> +                             sizeof(ssize),
> +                             1);
> +             if (bytes_to_read != sizeof(ssize))  {
> +                     uk_pr_err("Failed to get ssize from the device %d\n",
> +                                     rc);
> +                     rc = -EAGAIN;
> +                     goto exit;
> +             }
> +     }
> +
> +     cap->ssize = ssize;
> +     cap->sectors = sectors;
> +     cap->ioalign = sizeof(void *);
> +     cap->mode = (virtio_has_features(
> +                     host_features, VIRTIO_BLK_F_RO)) ? O_RDONLY :
> O_RDWR;
> +
> +     /**
> +      * Mask out features supported by both driver and device.
> +      */
> +     vbdev->vdev->features &= host_features;
> +     virtio_feature_set(vbdev->vdev, vbdev->vdev->features);
> +
> +exit:
> +     return rc;
> +}
> +
> +static inline void virtio_blkdev_feature_set(struct virtio_blk_device *vbdev)
> +{
> +     vbdev->vdev->features = 0;
> +

Can this initialization be removed since it is initialized in 
VIRTIO_BLK_DRV_FEATURES?

--
Cheers,
Justin (Jia He)


> +     /* Setting the feature the driver support */
> +     VIRTIO_BLK_DRV_FEATURES(vbdev->vdev->features);
> +}
>  };
>
>  static int virtio_blk_add_dev(struct virtio_dev *vdev)
> @@ -57,6 +134,8 @@ static int virtio_blk_add_dev(struct virtio_dev *vdev)
>       if (!vbdev)
>               return -ENOMEM;
>
> +     vbdev->vdev = vdev;
> +
>       rc = uk_blkdev_drv_register(&vbdev->blkdev, a, drv_name);
>       if (rc < 0) {
>               uk_pr_err("Failed to register virtio_blk device: %d\n", rc);
> @@ -64,10 +143,19 @@ static int virtio_blk_add_dev(struct virtio_dev *vdev)
>       }
>
>       vbdev->uid = rc;
> +     virtio_blkdev_feature_set(vbdev);
> +     rc = virtio_blkdev_feature_negotiate(vbdev);
> +     if (rc) {
> +             uk_pr_err("Failed to negotiate the device feature %d\n", rc);
> +             goto err_negotiate_feature;
> +     }
> +
>       uk_pr_info("Virtio-blk device registered with libukblkdev\n");
>
>  out:
>       return rc;
> +err_negotiate_feature:
> +     virtio_dev_status_update(vbdev->vdev,
> VIRTIO_CONFIG_STATUS_FAIL);
>  err_out:
>       uk_free(a, vbdev);
>       goto out;
> --
> 2.17.1
>
>
> _______________________________________________
> Minios-devel mailing list
> Minios-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/minios-devel
IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel