[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 10/18] xen: add header and build dataplane/xen-q
From: |
Paul Durrant |
Subject: |
Re: [Qemu-block] [PATCH 10/18] xen: add header and build dataplane/xen-qdisk.c |
Date: |
Wed, 5 Dec 2018 17:31:10 +0000 |
> -----Original Message-----
> From: Anthony PERARD [mailto:address@hidden
> Sent: 03 December 2018 18:09
> To: Paul Durrant <address@hidden>
> Cc: address@hidden; address@hidden; xen-
> address@hidden; Stefan Hajnoczi <address@hidden>; Kevin
> Wolf <address@hidden>; Max Reitz <address@hidden>; Stefano Stabellini
> <address@hidden>
> Subject: Re: [PATCH 10/18] xen: add header and build dataplane/xen-qdisk.c
>
> 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 <address@hidden>
> > ---
> > 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 <address@hidden>
> > */
> >
> > +#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.
>
Ok. I'll clean these up.
> > +#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);
>
Done.
> > +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.)
>
Indeed. I added an error pointer to the function so it can bail cleanly.
> > 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.
>
Sure.
Paul
> > +
> > +typedef struct XenBlkDev XenQdiskDataPlane;
> > +
> > +XenQdiskDataPlane *xen_qdisk_dataplane_create(XenDevice *xendev,
> > + BlockConf *conf,
> > + IOThread *iothread);
>
> Thanks,
>
> --
> Anthony PERARD