On Wed, Nov 21, 2018 at 03:12:03PM +0000, Paul Durrant wrote:
> This patch adds the transformations necessary to get dataplane/xen-qdisk.c
> to build against the new XenBus/XenDevice framework. MAINTAINERS is also
> updated due to the introduction of dataplane/xen-qdisk.h.
>
> NOTE: Existing data structure names are retained for the moment. These will
> be modified by subsequent patches. A typedef for XenQdiskDataPlane
> has been added to the header (based on the old struct XenBlkDev name
> for the moment) so that the old names don't need to leak out of the
> dataplane code.
>
> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>
> ---
> diff --git a/hw/block/dataplane/xen-qdisk.c b/hw/block/dataplane/xen-qdisk.c
> index 8e4368e7af..b075aa975d 100644
> --- a/hw/block/dataplane/xen-qdisk.c
> +++ b/hw/block/dataplane/xen-qdisk.c
> @@ -5,65 +5,56 @@
> * Based on original code (c) Gerd Hoffmann <kra...@redhat.com>
> */
>
> +#include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> +#include "qapi/error.h"
> +#include "hw/hw.h"
> +#include "hw/xen/xen.h"
xen.h isn't needed, xen_common.h should be enough.
> +#include "hw/xen/xen_common.h"
> +#include "hw/block/block.h"
block.h isn't needed, block-backend.h should be enough.
> +#include "hw/block/xen_blkif.h"
> +#include "sysemu/blockdev.h"
blockdev.h doesn't seems to be used.
> +#include "sysemu/block-backend.h"
> +#include "sysemu/iothread.h"
> +#include "xen-qdisk.h"
> +
> @@ -227,20 +219,24 @@ static int ioreq_grant_copy(struct ioreq *ioreq)
> file_blk;
> segs[i].dest.virt = virt;
> }
> - segs[i].len = (ioreq->req.seg[i].last_sect
> - - ioreq->req.seg[i].first_sect + 1) * file_blk;
> + segs[i].len = (ioreq->req.seg[i].last_sect -
> + ioreq->req.seg[i].first_sect + 1) * file_blk;
> virt += segs[i].len;
> }
>
> - rc = xen_be_copy_grant_refs(xendev, to_domain, segs, count);
> + xen_device_copy_grant_refs(xendev, to_domain, segs, count, &local_err);
> +
> + if (local_err) {
> + const char *msg = error_get_pretty(local_err);
> +
> + error_report("failed to copy data: %s", msg);
> + error_free(local_err);
You can do the following instead:
error_prepend(local_err, "failed to copy data: ")
error_report_err(local_err);
> +void xen_qdisk_dataplane_start(struct XenBlkDev *blkdev,
> + const unsigned int ring_ref[],
> + unsigned int nr_ring_ref,
> + unsigned int event_channel,
> + unsigned int protocol)
> {
> - struct XenBlkDev *blkdev = container_of(xendev, struct XenBlkDev,
> xendev);
> + XenDevice *xendev = blkdev->xendev;
> + unsigned int ring_size;
> + unsigned int i;
>
> - qemu_bh_schedule(blkdev->bh);
> + blkdev->nr_ring_ref = nr_ring_ref;
> + blkdev->ring_ref = g_new(unsigned int, nr_ring_ref);
> +
> + for (i = 0; i < nr_ring_ref; i++) {
> + blkdev->ring_ref[i] = ring_ref[i];
> + }
> +
> + blkdev->protocol = protocol;
> +
> + ring_size = XC_PAGE_SIZE * blkdev->nr_ring_ref;
> + switch (blkdev->protocol) {
> + case BLKIF_PROTOCOL_NATIVE:
> + {
> + blkdev->max_requests = __CONST_RING_SIZE(blkif, ring_size);
> + break;
> + }
> + case BLKIF_PROTOCOL_X86_32:
> + {
> + blkdev->max_requests = __CONST_RING_SIZE(blkif_x86_32, ring_size);
> + break;
> + }
> + case BLKIF_PROTOCOL_X86_64:
> + {
> + blkdev->max_requests = __CONST_RING_SIZE(blkif_x86_64, ring_size);
> + break;
> + }
> + default:
> + assert(false);
> + break;
This should return rather than keep going.
And maybe set an Error that could be added to the parameter of the
function.
> + }
> +
> + xen_device_set_max_grant_refs(xendev, blkdev->nr_ring_ref,
> + &error_fatal);
Do we really want to exit() here if an error happen, rather than let the
caller know? (Same question for other uses of error_fatal.)
> diff --git a/hw/block/dataplane/xen-qdisk.h b/hw/block/dataplane/xen-qdisk.h
> new file mode 100644
> index 0000000000..16bcd500bf
> --- /dev/null
> +++ b/hw/block/dataplane/xen-qdisk.h
> @@ -0,0 +1,25 @@
> +/*
> + * Copyright (c) Citrix Systems Inc.
> + * All rights reserved.
> + */
> +
> +#ifndef HW_BLOCK_DATAPLANE_QDISK_H
> +#define HW_BLOCK_DATAPLANE_QDISK_H
> +
> +#include "hw/xen/xen-bus.h"
> +#include "sysemu/iothread.h"
I would add #include "hw/block/block.h" since it includes the definition
of BlockConf.
> +
> +typedef struct XenBlkDev XenQdiskDataPlane;
> +
> +XenQdiskDataPlane *xen_qdisk_dataplane_create(XenDevice *xendev,
> + BlockConf *conf,
> + IOThread *iothread);
Thanks,
--
Anthony PERARD
