Re: [PATCH v5 7/8] virtio/s390: use DMA memory for ccw I/O and classic notifiers

Michael Mueller
2019-06-13T08:33:00Z

On 12.06.19 13:12, Halil Pasic wrote:
> Before virtio-ccw could get away with not using DMA API for the pieces of
> memory it does ccw I/O with. With protected virtualization this has to
> change, since the hypervisor needs to read and sometimes also write these
> pieces of memory.
>
> The hypervisor is supposed to poke the classic notifiers, if these are
> used, out of band with regards to ccw I/O. So these need to be allocated
> as DMA memory (which is shared memory for protected virtualization
> guests).
>
> Let us factor out everything from struct virtio_ccw_device that needs to
> be DMA memory in a satellite that is allocated as such.
>
> Note: The control blocks of I/O instructions do not need to be shared.
> These are marshalled by the ultravisor.
>
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> ---
> drivers/s390/virtio/virtio_ccw.c | 169 ++++++++++++++++---------------
> 1 file changed, 89 insertions(+), 80 deletions(-)
>
> diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
> index e96a8cc56ec2..800252955a2f 100644
> --- a/drivers/s390/virtio/virtio_ccw.c
> +++ b/drivers/s390/virtio/virtio_ccw.c
> @@ -46,9 +46,15 @@ struct vq_config_block {
> #define VIRTIO_CCW_CONFIG_SIZE 0x100
> /* same as PCI config space size, should be enough for all drivers */
>
> +struct vcdev_dma_area {
> + unsigned long indicators;
> + unsigned long indicators2;
> + struct vq_config_block config_block;
> + __u8 status;
> +};
> +
> struct virtio_ccw_device {
> struct virtio_device vdev;
> - __u8 *status;
> __u8 config[VIRTIO_CCW_CONFIG_SIZE];
> struct ccw_device *cdev;
> __u32 curr_io;
> @@ -58,24 +64,22 @@ struct virtio_ccw_device {
> spinlock_t lock;
> struct mutex io_lock; /* Serializes I/O requests */
> struct list_head virtqueues;
> - unsigned long indicators;
> - unsigned long indicators2;
> - struct vq_config_block *config_block;
> bool is_thinint;
> bool going_away;
> bool device_lost;
> unsigned int config_ready;
> void *airq_info;
> + struct vcdev_dma_area *dma_area;
> };
>
> static inline unsigned long *indicators(struct virtio_ccw_device *vcdev)
> {
> - return &vcdev->indicators;
> + return &vcdev->dma_area->indicators;
> }
>
> static inline unsigned long *indicators2(struct virtio_ccw_device *vcdev)
> {
> - return &vcdev->indicators2;
> + return &vcdev->dma_area->indicators2;
> }
>
> struct vq_info_block_legacy {
> @@ -336,8 +340,8 @@ static void virtio_ccw_drop_indicator(struct virtio_ccw_device *vcdev,
> struct airq_info *airq_info = vcdev->airq_info;
>
> if (vcdev->is_thinint) {
> - thinint_area = kzalloc(sizeof(*thinint_area),
> - GFP_DMA | GFP_KERNEL);
> + thinint_area = ccw_device_dma_zalloc(vcdev->cdev,
> + sizeof(*thinint_area));
> if (!thinint_area)
> return;
> thinint_area->summary_indicator =
> @@ -348,8 +352,8 @@ static void virtio_ccw_drop_indicator(struct virtio_ccw_device *vcdev,
> ccw->cda = (__u32)(unsigned long) thinint_area;
> } else {
> /* payload is the address of the indicators */
> - indicatorp = kmalloc(sizeof(indicators(vcdev)),
> - GFP_DMA | GFP_KERNEL);
> + indicatorp = ccw_device_dma_zalloc(vcdev->cdev,
> + sizeof(indicators(vcdev)));
> if (!indicatorp)
> return;
> *indicatorp = 0;
> @@ -369,8 +373,8 @@ static void virtio_ccw_drop_indicator(struct virtio_ccw_device *vcdev,
> "Failed to deregister indicators (%d)\n", ret);
> else if (vcdev->is_thinint)
> virtio_ccw_drop_indicators(vcdev);
> - kfree(indicatorp);
> - kfree(thinint_area);
> + ccw_device_dma_free(vcdev->cdev, indicatorp, sizeof(indicators(vcdev)));
> + ccw_device_dma_free(vcdev->cdev, thinint_area, sizeof(*thinint_area));
> }
>
> static inline long __do_kvm_notify(struct subchannel_id schid,
> @@ -417,15 +421,15 @@ static int virtio_ccw_read_vq_conf(struct virtio_ccw_device *vcdev,
> {
> int ret;
>
> - vcdev->config_block->index = index;
> + vcdev->dma_area->config_block.index = index;
> ccw->cmd_code = CCW_CMD_READ_VQ_CONF;
> ccw->flags = 0;
> ccw->count = sizeof(struct vq_config_block);
> - ccw->cda = (__u32)(unsigned long)(vcdev->config_block);
> + ccw->cda = (__u32)(unsigned long)(&vcdev->dma_area->config_block);
> ret = ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_READ_VQ_CONF);
> if (ret)
> return ret;
> - return vcdev->config_block->num ?: -ENOENT;
> + return vcdev->dma_area->config_block.num ?: -ENOENT;
> }
>
> static void virtio_ccw_del_vq(struct virtqueue *vq, struct ccw1 *ccw)
> @@ -470,7 +474,8 @@ static void virtio_ccw_del_vq(struct virtqueue *vq, struct ccw1 *ccw)
> ret, index);
>
> vring_del_virtqueue(vq);
> - kfree(info->info_block);
> + ccw_device_dma_free(vcdev->cdev, info->info_block,
> + sizeof(*info->info_block));
> kfree(info);
> }
>
> @@ -480,7 +485,7 @@ static void virtio_ccw_del_vqs(struct virtio_device *vdev)
> struct ccw1 *ccw;
> struct virtio_ccw_device *vcdev = to_vc_device(vdev);
>
> - ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
> + ccw = ccw_device_dma_zalloc(vcdev->cdev, sizeof(*ccw));
> if (!ccw)
> return;
>
> @@ -489,7 +494,7 @@ static void virtio_ccw_del_vqs(struct virtio_device *vdev)
> list_for_each_entry_safe(vq, n, &vdev->vqs, list)
> virtio_ccw_del_vq(vq, ccw);
>
> - kfree(ccw);
> + ccw_device_dma_free(vcdev->cdev, ccw, sizeof(*ccw));
> }
>
> static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev,
> @@ -512,8 +517,8 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev,
> err = -ENOMEM;
> goto out_err;
> }
> - info->info_block = kzalloc(sizeof(*info->info_block),
> - GFP_DMA | GFP_KERNEL);
> + info->info_block = ccw_device_dma_zalloc(vcdev->cdev,
> + sizeof(*info->info_block));
> if (!info->info_block) {
> dev_warn(&vcdev->cdev->dev, "no info block\n");
> err = -ENOMEM;
> @@ -577,7 +582,8 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev,
> if (vq)
> vring_del_virtqueue(vq);
> if (info) {
> - kfree(info->info_block);
> + ccw_device_dma_free(vcdev->cdev, info->info_block,
> + sizeof(*info->info_block));
> }
> kfree(info);
> return ERR_PTR(err);
> @@ -591,7 +597,8 @@ static int virtio_ccw_register_adapter_ind(struct virtio_ccw_device *vcdev,
> struct virtio_thinint_area *thinint_area = NULL;
> struct airq_info *info;
>
> - thinint_area = kzalloc(sizeof(*thinint_area), GFP_DMA | GFP_KERNEL);
> + thinint_area = ccw_device_dma_zalloc(vcdev->cdev,
> + sizeof(*thinint_area));
> if (!thinint_area) {
> ret = -ENOMEM;
> goto out;
> @@ -627,7 +634,7 @@ static int virtio_ccw_register_adapter_ind(struct virtio_ccw_device *vcdev,
> virtio_ccw_drop_indicators(vcdev);
> }
> out:
> - kfree(thinint_area);
> + ccw_device_dma_free(vcdev->cdev, thinint_area, sizeof(*thinint_area));
> return ret;
> }
>
> @@ -643,7 +650,7 @@ static int virtio_ccw_find_vqs(struct virtio_device *vdev, unsigned nvqs,
> int ret, i, queue_idx = 0;
> struct ccw1 *ccw;
>
> - ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
> + ccw = ccw_device_dma_zalloc(vcdev->cdev, sizeof(*ccw));
> if (!ccw)
> return -ENOMEM;
>
> @@ -667,7 +674,8 @@ static int virtio_ccw_find_vqs(struct virtio_device *vdev, unsigned nvqs,
> * We need a data area under 2G to communicate. Our payload is
> * the address of the indicators.
> */
> - indicatorp = kmalloc(sizeof(indicators(vcdev)), GFP_DMA | GFP_KERNEL);
> + indicatorp = ccw_device_dma_zalloc(vcdev->cdev,
> + sizeof(indicators(vcdev)));
> if (!indicatorp)
> goto out;
> *indicatorp = (unsigned long) indicators(vcdev);
> @@ -699,12 +707,16 @@ static int virtio_ccw_find_vqs(struct virtio_device *vdev, unsigned nvqs,
> if (ret)
> goto out;
>
> - kfree(indicatorp);
> - kfree(ccw);
> + if (indicatorp)
> + ccw_device_dma_free(vcdev->cdev, indicatorp,
> + sizeof(indicators(vcdev)));
> + ccw_device_dma_free(vcdev->cdev, ccw, sizeof(*ccw));
> return 0;
> out:
> - kfree(indicatorp);
> - kfree(ccw);
> + if (indicatorp)
> + ccw_device_dma_free(vcdev->cdev, indicatorp,
> + sizeof(indicators(vcdev)));
> + ccw_device_dma_free(vcdev->cdev, ccw, sizeof(*ccw));
> virtio_ccw_del_vqs(vdev);
> return ret;
> }
> @@ -714,12 +726,12 @@ static void virtio_ccw_reset(struct virtio_device *vdev)
> struct virtio_ccw_device *vcdev = to_vc_device(vdev);
> struct ccw1 *ccw;
>
> - ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
> + ccw = ccw_device_dma_zalloc(vcdev->cdev, sizeof(*ccw));
> if (!ccw)
> return;
>
> /* Zero status bits. */
> - *vcdev->status = 0;
> + vcdev->dma_area->status = 0;
>
> /* Send a reset ccw on device. */
> ccw->cmd_code = CCW_CMD_VDEV_RESET;
> @@ -727,7 +739,7 @@ static void virtio_ccw_reset(struct virtio_device *vdev)
> ccw->count = 0;
> ccw->cda = 0;
> ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_RESET);
> - kfree(ccw);
> + ccw_device_dma_free(vcdev->cdev, ccw, sizeof(*ccw));
> }
>
> static u64 virtio_ccw_get_features(struct virtio_device *vdev)
> @@ -738,11 +750,11 @@ static u64 virtio_ccw_get_features(struct virtio_device *vdev)
> u64 rc;
> struct ccw1 *ccw;
>
> - ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
> + ccw = ccw_device_dma_zalloc(vcdev->cdev, sizeof(*ccw));
> if (!ccw)
> return 0;
>
> - features = kzalloc(sizeof(*features), GFP_DMA | GFP_KERNEL);
> + features = ccw_device_dma_zalloc(vcdev->cdev, sizeof(*features));
> if (!features) {
> rc = 0;
> goto out_free;
> @@ -775,8 +787,8 @@ static u64 virtio_ccw_get_features(struct virtio_device *vdev)
> rc |= (u64)le32_to_cpu(features->features) << 32;
>
> out_free:
> - kfree(features);
> - kfree(ccw);
> + ccw_device_dma_free(vcdev->cdev, features, sizeof(*features));
> + ccw_device_dma_free(vcdev->cdev, ccw, sizeof(*ccw));
> return rc;
> }
>
> @@ -801,11 +813,11 @@ static int virtio_ccw_finalize_features(struct virtio_device *vdev)
> return -EINVAL;
> }
>
> - ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
> + ccw = ccw_device_dma_zalloc(vcdev->cdev, sizeof(*ccw));
> if (!ccw)
> return -ENOMEM;
>
> - features = kzalloc(sizeof(*features), GFP_DMA | GFP_KERNEL);
> + features = ccw_device_dma_zalloc(vcdev->cdev, sizeof(*features));
> if (!features) {
> ret = -ENOMEM;
> goto out_free;
> @@ -840,8 +852,8 @@ static int virtio_ccw_finalize_features(struct virtio_device *vdev)
> ret = ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_FEAT);
>
> out_free:
> - kfree(features);
> - kfree(ccw);
> + ccw_device_dma_free(vcdev->cdev, features, sizeof(*features));
> + ccw_device_dma_free(vcdev->cdev, ccw, sizeof(*ccw));
>
> return ret;
> }
> @@ -855,11 +867,12 @@ static void virtio_ccw_get_config(struct virtio_device *vdev,
> void *config_area;
> unsigned long flags;
>
> - ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
> + ccw = ccw_device_dma_zalloc(vcdev->cdev, sizeof(*ccw));
> if (!ccw)
> return;
>
> - config_area = kzalloc(VIRTIO_CCW_CONFIG_SIZE, GFP_DMA | GFP_KERNEL);
> + config_area = ccw_device_dma_zalloc(vcdev->cdev,
> + VIRTIO_CCW_CONFIG_SIZE);
> if (!config_area)
> goto out_free;
>
> @@ -881,8 +894,8 @@ static void virtio_ccw_get_config(struct virtio_device *vdev,
> memcpy(buf, config_area + offset, len);
>
> out_free:
> - kfree(config_area);
> - kfree(ccw);
> + ccw_device_dma_free(vcdev->cdev, config_area, VIRTIO_CCW_CONFIG_SIZE);
> + ccw_device_dma_free(vcdev->cdev, ccw, sizeof(*ccw));
> }
>
> static void virtio_ccw_set_config(struct virtio_device *vdev,
> @@ -894,11 +907,12 @@ static void virtio_ccw_set_config(struct virtio_device *vdev,
> void *config_area;
> unsigned long flags;
>
> - ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
> + ccw = ccw_device_dma_zalloc(vcdev->cdev, sizeof(*ccw));
> if (!ccw)
> return;
>
> - config_area = kzalloc(VIRTIO_CCW_CONFIG_SIZE, GFP_DMA | GFP_KERNEL);
> + config_area = ccw_device_dma_zalloc(vcdev->cdev,
> + VIRTIO_CCW_CONFIG_SIZE);
> if (!config_area)
> goto out_free;
>
> @@ -917,61 +931,61 @@ static void virtio_ccw_set_config(struct virtio_device *vdev,
> ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_CONFIG);
>
> out_free:
> - kfree(config_area);
> - kfree(ccw);
> + ccw_device_dma_free(vcdev->cdev, config_area, VIRTIO_CCW_CONFIG_SIZE);
> + ccw_device_dma_free(vcdev->cdev, ccw, sizeof(*ccw));
> }
>
> static u8 virtio_ccw_get_status(struct virtio_device *vdev)
> {
> struct virtio_ccw_device *vcdev = to_vc_device(vdev);
> - u8 old_status = *vcdev->status;
> + u8 old_status = vcdev->dma_area->status;
> struct ccw1 *ccw;
>
> if (vcdev->revision < 1)
> - return *vcdev->status;
> + return vcdev->dma_area->status;
>
> - ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
> + ccw = ccw_device_dma_zalloc(vcdev->cdev, sizeof(*ccw));
> if (!ccw)
> return old_status;
>
> ccw->cmd_code = CCW_CMD_READ_STATUS;
> ccw->flags = 0;
> - ccw->count = sizeof(*vcdev->status);
> - ccw->cda = (__u32)(unsigned long)vcdev->status;
> + ccw->count = sizeof(vcdev->dma_area->status);
> + ccw->cda = (__u32)(unsigned long)&vcdev->dma_area->status;
> ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_READ_STATUS);
> /*
> * If the channel program failed (should only happen if the device
> * was hotunplugged, and then we clean up via the machine check
> - * handler anyway), vcdev->status was not overwritten and we just
> + * handler anyway), vcdev->dma_area->status was not overwritten and we just
> * return the old status, which is fine.
> */
> - kfree(ccw);
> + ccw_device_dma_free(vcdev->cdev, ccw, sizeof(*ccw));
>
> - return *vcdev->status;
> + return vcdev->dma_area->status;
> }
>
> static void virtio_ccw_set_status(struct virtio_device *vdev, u8 status)
> {
> struct virtio_ccw_device *vcdev = to_vc_device(vdev);
> - u8 old_status = *vcdev->status;
> + u8 old_status = vcdev->dma_area->status;
> struct ccw1 *ccw;
> int ret;
>
> - ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
> + ccw = ccw_device_dma_zalloc(vcdev->cdev, sizeof(*ccw));
> if (!ccw)
> return;
>
> /* Write the status to the host. */
> - *vcdev->status = status;
> + vcdev->dma_area->status = status;
> ccw->cmd_code = CCW_CMD_WRITE_STATUS;
> ccw->flags = 0;
> ccw->count = sizeof(status);
> - ccw->cda = (__u32)(unsigned long)vcdev->status;
> + ccw->cda = (__u32)(unsigned long)&vcdev->dma_area->status;
> ret = ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_STATUS);
> /* Write failed? We assume status is unchanged. */
> if (ret)
> - *vcdev->status = old_status;
> - kfree(ccw);
> + vcdev->dma_area->status = old_status;
> + ccw_device_dma_free(vcdev->cdev, ccw, sizeof(*ccw));
> }
>
> static const char *virtio_ccw_bus_name(struct virtio_device *vdev)
> @@ -1004,8 +1018,8 @@ static void virtio_ccw_release_dev(struct device *_d)
> struct virtio_device *dev = dev_to_virtio(_d);
> struct virtio_ccw_device *vcdev = to_vc_device(dev);
>
> - kfree(vcdev->status);
> - kfree(vcdev->config_block);
> + ccw_device_dma_free(vcdev->cdev, vcdev->dma_area,
> + sizeof(*vcdev->dma_area));
> kfree(vcdev);
> }
>
> @@ -1213,12 +1227,12 @@ static int virtio_ccw_set_transport_rev(struct virtio_ccw_device *vcdev)
> struct ccw1 *ccw;
> int ret;
>
> - ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
> + ccw = ccw_device_dma_zalloc(vcdev->cdev, sizeof(*ccw));
> if (!ccw)
> return -ENOMEM;
> - rev = kzalloc(sizeof(*rev), GFP_DMA | GFP_KERNEL);
> + rev = ccw_device_dma_zalloc(vcdev->cdev, sizeof(*rev));
> if (!rev) {
> - kfree(ccw);
> + ccw_device_dma_free(vcdev->cdev, ccw, sizeof(*ccw));
> return -ENOMEM;
> }
>
> @@ -1248,8 +1262,8 @@ static int virtio_ccw_set_transport_rev(struct virtio_ccw_device *vcdev)
> }
> } while (ret == -EOPNOTSUPP);
>
> - kfree(ccw);
> - kfree(rev);
> + ccw_device_dma_free(vcdev->cdev, ccw, sizeof(*ccw));
> + ccw_device_dma_free(vcdev->cdev, rev, sizeof(*rev));
> return ret;
> }
>
> @@ -1266,14 +1280,10 @@ static int virtio_ccw_online(struct ccw_device *cdev)
> goto out_free;
> }
> vcdev->vdev.dev.parent = &cdev->dev;
> - vcdev->config_block = kzalloc(sizeof(*vcdev->config_block),
> - GFP_DMA | GFP_KERNEL);
> - if (!vcdev->config_block) {
> - ret = -ENOMEM;
> - goto out_free;
> - }
> - vcdev->status = kzalloc(sizeof(*vcdev->status), GFP_DMA | GFP_KERNEL);
> - if (!vcdev->status) {
> + vcdev->cdev = cdev;
> + vcdev->dma_area = ccw_device_dma_zalloc(vcdev->cdev,
> + sizeof(*vcdev->dma_area));
> + if (!vcdev->dma_area) {
> ret = -ENOMEM;
> goto out_free;
> }
> @@ -1282,7 +1292,6 @@ static int virtio_ccw_online(struct ccw_device *cdev)
>
> vcdev->vdev.dev.release = virtio_ccw_release_dev;
> vcdev->vdev.config = &virtio_ccw_config_ops;
> - vcdev->cdev = cdev;
> init_waitqueue_head(&vcdev->wait_q);
> INIT_LIST_HEAD(&vcdev->virtqueues);
> spin_lock_init(&vcdev->lock);
> @@ -1313,8 +1322,8 @@ static int virtio_ccw_online(struct ccw_device *cdev)
> return ret;
> out_free:
> if (vcdev) {
> - kfree(vcdev->status);
> - kfree(vcdev->config_block);
> + ccw_device_dma_free(vcdev->cdev, vcdev->dma_area,
> + sizeof(*vcdev->dma_area));
> }
> kfree(vcdev);
> return ret;
>

Reviewed-by: Michael Mueller <mimu@linux.ibm.com>

Michael