Re: [kvm-unit-tests PATCH 3/7] s390x: virtio: CCW transport implementation

[Thomas Huth <thuth@redhat.com>]

On 27/08/2021 12.17, Pierre Morel wrote:
> This is the implementation of the virtio-ccw transport level.
> 
> We only support VIRTIO revision 0.

That means only legacy virtio? Wouldn't it be better to shoot for modern 
virtio instead?

> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   lib/s390x/virtio-ccw.c | 374 +++++++++++++++++++++++++++++++++++++++++
>   lib/s390x/virtio-ccw.h | 111 ++++++++++++
>   lib/virtio-config.h    |  30 ++++
>   s390x/Makefile         |   2 +
>   4 files changed, 517 insertions(+)
>   create mode 100644 lib/s390x/virtio-ccw.c
>   create mode 100644 lib/s390x/virtio-ccw.h
>   create mode 100644 lib/virtio-config.h
> 
> diff --git a/lib/s390x/virtio-ccw.c b/lib/s390x/virtio-ccw.c
> new file mode 100644
> index 00000000..cf447de6
> --- /dev/null
> +++ b/lib/s390x/virtio-ccw.c
> @@ -0,0 +1,374 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Virtio CCW Library
> + *
> + * Copyright (c) 2021 IBM Corp
> + *
> + * Authors:
> + *  Pierre Morel <pmorel@linux.ibm.com>
> + *
> + */
> +
> +#include <libcflat.h>
> +#include <alloc_page.h>
> +#include <asm/page.h>
> +#include <string.h>
> +#include <interrupt.h>
> +#include <asm/arch_def.h>
> +#include <asm/facility.h>
> +#include <asm/uv.h>
> +
> +#include <css.h>
> +#include <virtio.h>
> +#include <virtio-config.h>
> +#include <virtio-ccw.h>
> +#include <malloc_io.h>
> +
> +static struct linked_list vcdev_list = {
> +	.prev = &vcdev_list,
> +	.next = &vcdev_list
> +};
> +
> +static inline uint32_t swap16(uint32_t b)
> +{
> +		return (((b & 0xff00U) <<  8) |
> +		((b & 0x00ff) >>  8));
> +}
> +
> +static inline uint32_t swap32(uint32_t b)
> +{
> +	return (((b & 0x000000ffU) << 24) |
> +		((b & 0x0000ff00U) <<  8) |
> +		((b & 0x00ff0000U) >>  8) |
> +		((b & 0xff000000U) >> 24));
> +}
> +
> +static inline uint64_t swap64(uint64_t x)
> +{
> +	return (((x & 0x00000000000000ffULL) << 56) |
> +		((x & 0x000000000000ff00ULL) << 40) |
> +		((x & 0x0000000000ff0000ULL) << 24) |
> +		((x & 0x00000000ff000000ULL) <<  8) |
> +		((x & 0x000000ff00000000ULL) >>  8) |
> +		((x & 0x0000ff0000000000ULL) >> 24) |
> +		((x & 0x00ff000000000000ULL) >> 40) |
> +		((x & 0xff00000000000000ULL) >> 56));
> +}

We already have macros for swapping in lib/asm-generic/io.h ... could you 
use those instead?

> +/*
> + * flags: flags for CCW
> + * Returns !0 on failure
> + * Returns 0 on success
> + */
> +int ccw_send(struct virtio_ccw_device *vcdev, int code, void *data, int count,
> +	     unsigned char flags)
> +{
> +	struct ccw1 *ccw;
> +	int ret = -1;
> +
> +	ccw = alloc_io_mem(sizeof(*ccw), 0);
> +	if (!ccw)
> +		return ret;
> +
> +	/* Build the CCW chain with a single CCW */
> +	ccw->code = code;
> +	ccw->flags = flags;
> +	ccw->count = count;
> +	ccw->data_address = (unsigned long)data;
> +
> +	ret = start_ccw1_chain(vcdev->schid, ccw);
> +	if (!ret)
> +		ret = wait_and_check_io_completion(vcdev->schid);
> +
> +	free_io_mem(ccw, sizeof(*ccw));
> +	return ret;
> +}
> +
> +int virtio_ccw_set_revision(struct virtio_ccw_device *vcdev)
> +{
> +	struct virtio_rev_info *rev_info;
> +	int ret = -1;
> +
> +	rev_info = alloc_io_mem(sizeof(*rev_info), 0);
> +	if (!rev_info)
> +		return ret;
> +
> +	rev_info->revision = VIRTIO_CCW_REV_MAX;
> +	rev_info->revision = 0;

Either VIRTIO_CCW_REV_MAX or 0, but not both?

> +	do {
> +		ret = ccw_send(vcdev, CCW_CMD_SET_VIRTIO_REV, rev_info,
> +			       sizeof(*rev_info), 0);
> +	} while (ret && rev_info->revision--);
> +
> +	free_io_mem(rev_info, sizeof(*rev_info));
> +
> +	return ret ? -1 : rev_info->revision;
> +}
> +
> +int virtio_ccw_reset(struct virtio_ccw_device *vcdev)
> +{
> +	return ccw_send(vcdev, CCW_CMD_VDEV_RESET, 0, 0, 0);
> +}
> +
> +int virtio_ccw_read_status(struct virtio_ccw_device *vcdev)
> +{
> +	return ccw_send(vcdev, CCW_CMD_READ_STATUS, &vcdev->status,
> +			sizeof(vcdev->status), 0);
> +}
> +
> +int virtio_ccw_write_status(struct virtio_ccw_device *vcdev)
> +{
> +	return ccw_send(vcdev, CCW_CMD_WRITE_STATUS, &vcdev->status,
> +			sizeof(vcdev->status), 0);
> +}
> +
> +int virtio_ccw_read_features(struct virtio_ccw_device *vcdev, uint64_t *features)
> +{
> +	struct virtio_feature_desc *f_desc = &vcdev->f_desc;
> +
> +	f_desc->index = 0;
> +	if (ccw_send(vcdev, CCW_CMD_READ_FEAT, f_desc, sizeof(*f_desc), 0))
> +		return -1;
> +	*features = swap32(f_desc->features);
> +
> +	f_desc->index = 1;
> +	if (ccw_send(vcdev, CCW_CMD_READ_FEAT, f_desc, sizeof(*f_desc), 0))
> +		return -1;
> +	*features |= (uint64_t)swap32(f_desc->features) << 32;

Weren't the upper feature bits only available for modern virtio anyway?

> +	return 0;
> +}
> +
> +int virtio_ccw_write_features(struct virtio_ccw_device *vcdev, uint64_t features)
> +{
> +	struct virtio_feature_desc *f_desc = &vcdev->f_desc;
> +
> +	f_desc->index = 0;
> +	f_desc->features = swap32((uint32_t)features & 0xffffffff);
> +	if (ccw_send(vcdev, CCW_CMD_WRITE_FEAT, &f_desc, sizeof(*f_desc), 0))
> +		return -1;
> +
> +	f_desc->index = 1;
> +	f_desc->features = swap32((uint32_t)(features >> 32) & 0xffffffff);
> +	if (ccw_send(vcdev, CCW_CMD_WRITE_FEAT, &f_desc, sizeof(*f_desc), 0))
> +		return -1;
> +
> +	return 0;
> +}
> +
> +int virtio_ccw_read_config(struct virtio_ccw_device *vcdev)
> +{
> +	return ccw_send(vcdev, CCW_CMD_READ_CONF, &vcdev->config,
> +			sizeof(vcdev->config), 0);
> +}
> +
> +int virtio_ccw_write_config(struct virtio_ccw_device *vcdev)
> +{
> +	return ccw_send(vcdev, CCW_CMD_WRITE_CONF, &vcdev->config,
> +			sizeof(vcdev->config), 0);
> +}
> +
> +int virtio_ccw_setup_indicators(struct virtio_ccw_device *vcdev)
> +{
> +	vcdev->ind = alloc_io_mem(sizeof(PAGE_SIZE), 0);
> +	if (ccw_send(vcdev, CCW_CMD_SET_IND, &vcdev->ind,
> +		     sizeof(vcdev->ind), 0))
> +		return -1;
> +
> +	vcdev->conf_ind = alloc_io_mem(PAGE_SIZE, 0);
> +	if (ccw_send(vcdev, CCW_CMD_SET_CONF_IND, &vcdev->conf_ind,
> +		     sizeof(vcdev->conf_ind), 0))
> +		return -1;
> +
> +	return 0;
> +}
> +
> +static uint64_t virtio_ccw_notify_host(int schid, int queue, uint64_t cookie)
> +{
> +	register unsigned long nr asm("1") = 0x03;
> +	register unsigned long s asm("2") = schid;
> +	register unsigned long q asm("3") = queue;
> +	register long rc asm("2");

Using asm("2") for two variables looks somewhat weird ... but ok, as long as 
it works... (otherwise you could also use only one variable and mark it as 
input + output parameter below).

> +	register long c asm("4") = cookie;
> +
> +	asm volatile ("diag 2,4,0x500\n"
> +			: "=d" (rc)
> +			: "d" (nr), "d" (s), "d" (q), "d"(c)
> +			: "memory", "cc");
> +	return rc;
> +}
> +
> +static bool virtio_ccw_notify(struct virtqueue *vq)
> +{
> +	struct virtio_ccw_device *vcdev = to_vc_device(vq->vdev);
> +	struct virtio_ccw_vq_info *info = vq->priv;
> +
> +	info->cookie = virtio_ccw_notify_host(vcdev->schid, vq->index,
> +					      info->cookie);
> +	if (info->cookie < 0)
> +		return false;
> +	return true;
> +}
> +
> +/* allocates a vring_virtqueue but returns a pointer to the
> + * virtqueue inside of it or NULL on error.
> + */
> +static struct virtqueue *setup_vq(struct virtio_device *vdev, int index,
> +				  void (*callback)(struct virtqueue *vq),
> +				  const char *name)
> +{
> +	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
> +	struct virtio_ccw_vq_info *info;
> +	struct vring_virtqueue *vq;
> +	struct vring *vr;
> +	void *queue;
> +
> +	vq = alloc_io_mem(sizeof(*vq), 0);
> +	info = alloc_io_mem(sizeof(*info), 0);
> +	queue = alloc_io_mem(4 * PAGE_SIZE, 0);
> +	assert(vq && queue && info);
> +
> +	info->info_block = alloc_io_mem(sizeof(*info->info_block), 0);
> +	assert(info->info_block);
> +
> +	vcdev->vq_conf.index = index;
> +	if (ccw_send(vcdev, CCW_CMD_READ_VQ_CONF, &vcdev->vq_conf,
> +		     sizeof(vcdev->vq_conf), 0))
> +		return NULL;
> +
> +	vring_init_virtqueue(vq, index, vcdev->vq_conf.max_num, PAGE_SIZE, vdev,
> +			     queue, virtio_ccw_notify, callback, name);
> +
> +	vr = &vq->vring;
> +	info->info_block->s.desc = vr->desc;
> +	info->info_block->s.index = index;
> +	info->info_block->s.num = vr->num;
> +	info->info_block->s.avail = vr->avail;
> +	info->info_block->s.used = vr->used;
> +
> +	info->info_block->l.desc = vr->desc;
> +	info->info_block->l.index = index;
> +	info->info_block->l.num = vr->num;
> +	info->info_block->l.align = PAGE_SIZE;
> +
> +	if (ccw_send(vcdev, CCW_CMD_SET_VQ, info->info_block,
> +		     sizeof(info->info_block->l), 0))
> +		return NULL;
> +
> +	info->vq = &vq->vq;
> +	vq->vq.priv = info;
> +
> +	return &vq->vq;
> +}
> +
> +static int virtio_ccw_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
> +			       struct virtqueue *vqs[], vq_callback_t *callbacks[],
> +			       const char *names[])
> +{
> +	int i;
> +
> +	for (i = 0; i < nvqs; ++i) {
> +		vqs[i] = setup_vq(vdev, i,
> +				  callbacks ? callbacks[i] : NULL,
> +				  names ? names[i] : "");
> +		if (!vqs[i])
> +			return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +static void virtio_ccw_config_get(struct virtio_device *vdev,
> +				  unsigned int offset, void *buf,
> +				  unsigned int len)
> +{
> +	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
> +
> +	if (virtio_ccw_read_config(vcdev))
> +		return;
> +	memcpy(buf, vcdev->config, len);
> +}
> +
> +static void virtio_ccw_config_set(struct virtio_device *vdev,
> +				  unsigned int offset, const void *buf,
> +				  unsigned int len)
> +{
> +	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
> +
> +	memcpy(vcdev->config, buf, len);
> +	virtio_ccw_write_config(vcdev);
> +}
> +
> +static const struct virtio_config_ops virtio_ccw_ops = {
> +	.get = virtio_ccw_config_get,
> +	.set = virtio_ccw_config_set,
> +	.find_vqs = virtio_ccw_find_vqs,
> +};
> +
> +const struct virtio_config_ops *virtio_ccw_register(void)
> +{
> +	return &virtio_ccw_ops;
> +}
> +
> +static int sense(struct virtio_ccw_device *vcdev)
> +{
> +	struct senseid *senseid;
> +
> +	senseid = alloc_io_mem(sizeof(*senseid), 0);
> +	assert(senseid);
> +
> +	assert(!ccw_send(vcdev, CCW_CMD_SENSE_ID, senseid, sizeof(*senseid), 0));
> +
> +	assert(senseid->reserved == 0xff);
> +
> +	vcdev->cu_type = senseid->cu_type;
> +	vcdev->cu_model = senseid->cu_model;
> +	vcdev->dev_type = senseid->dev_type;
> +	vcdev->dev_model = senseid->dev_model;
> +
> +	return 0;
> +}
> +
> +static struct virtio_ccw_device *find_vcdev_by_devid(int devid)
> +{
> +	struct virtio_ccw_device *dev;
> +	struct linked_list *l;
> +
> +	for (l = vcdev_list.next; l != &vcdev_list; l = l->next) {
> +		dev = container_of(l, struct virtio_ccw_device, list);
> +		if (dev->cu_model == devid)
> +			return dev;
> +	}
> +	return NULL;
> +}
> +
> +struct virtio_device *virtio_bind(u32 devid)
> +{
> +	struct virtio_ccw_device *vcdev;
> +
> +	vcdev = find_vcdev_by_devid(devid);
> +
> +	return &vcdev->vdev;
> +}
> +
> +static int virtio_enumerate(int schid)
> +{
> +	struct virtio_ccw_device *vcdev;
> +
> +	vcdev = alloc_io_mem(sizeof(*vcdev), 0);
> +	assert(vcdev);
> +	vcdev->schid = schid;
> +
> +	list_add(&vcdev_list, &vcdev->list);
> +
> +	assert(css_enable(schid, IO_SCH_ISC) == 0);
> +	sense(vcdev);
> +
> +	return 0;
> +}
> +
> +/* Must get a param */

I don't understand that comment, could you elaborate?

> +bool virtio_ccw_init(void)
> +{
> +	return css_enumerate(virtio_enumerate) != 0;
> +}

  Thomas