Discussion:
[PATCH v3 0/8] xen_disk: legacy code removal and cleanup
(too old to reply)
Paul Durrant
2018-05-04 19:25:59 UTC
Permalink
The grant copy operation was added to libxengnttab in Xen 4.8.0 (released
nearly 18 months ago) but the xen_disk PV backend QEMU is still carrying
a significant amount of code purely to remain compatible with older
versions of Xen.

As can be inferred from the diff stats below, removing this support for
older versions of Xen from QEMU reduces the size of the xen_disk source by
around 320 lines (~25%).

This versionseries maintains compatibility with older Xen, and OS
not supporting the grant copy operation, by adding an emulation of it
into the xen_backend code. Thus xen_disk can be simplified without
regressing support for any environment. This series also performs
general cleanup of the code by introducing and consistently using
helper functions for calling into libxenttab.

Paul Durrant (8):
xen_backend: add grant table helpers
xen_disk: remove open-coded use of libxengnttab
xen: remove other open-coded use of libxengnttab
xen_backend: add an emulation of grant copy
xen_disk: remove use of grant map/unmap
xen_backend: make the xen_feature_grant_copy flag private
xen_disk: use a single entry iovec
xen_disk: be consistent with use of xendev and blkdev->xendev

hw/9pfs/xen-9p-backend.c | 32 ++-
hw/block/xen_disk.c | 614 +++++++------------------------------------
hw/char/xen_console.c | 9 +-
hw/net/xen_nic.c | 34 ++-
hw/usb/xen-usb.c | 37 ++-
hw/xen/xen_backend.c | 178 ++++++++++++-
include/hw/xen/xen_backend.h | 34 ++-
7 files changed, 351 insertions(+), 587 deletions(-)
---
Cc: Anthony Perard <***@citrix.com>
Cc: Gerd Hoffmann <***@redhat.com>
Cc: Greg Kurz <***@kaod.org>
Cc: Jason Wang <***@redhat.com>
Cc: Kevin Wolf <***@redhat.com>
Cc: Max Reitz <***@redhat.com>
Cc: Paolo Bonzini <***@redhat.com>
Cc: Stefano Stabellini <***@kernel.org>
--
2.1.4
Paul Durrant
2018-05-04 19:26:00 UTC
Permalink
This patch adds grant table helper functions to the xen_backend code to
localize error reporting and use of xen_domid.

The patch also defers the call to xengnttab_open() until just before the
initialise method in XenDevOps is invoked. This method is responsible for
mapping the shared ring. No prior method requires access to the grant table.

Signed-off-by: Paul Durrant <***@citrix.com>
---
Cc: Stefano Stabellini <***@kernel.org>
Cc: Anthony Perard <***@citrix.com>

v2:
- New in v2
---
hw/xen/xen_backend.c | 123 ++++++++++++++++++++++++++++++++++++++-----
include/hw/xen/xen_backend.h | 33 ++++++++++++
2 files changed, 144 insertions(+), 12 deletions(-)

diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
index 7445b50..50412d6 100644
--- a/hw/xen/xen_backend.c
+++ b/hw/xen/xen_backend.c
@@ -106,6 +106,103 @@ int xen_be_set_state(struct XenDevice *xendev, enum xenbus_state state)
return 0;
}

+void xen_be_set_max_grant_refs(struct XenDevice *xendev,
+ unsigned int nr_refs)
+{
+ assert(xendev->ops->flags & DEVOPS_FLAG_NEED_GNTDEV);
+
+ if (xengnttab_set_max_grants(xendev->gnttabdev, nr_refs)) {
+ xen_pv_printf(xendev, 0, "xengnttab_set_max_grants failed: %s\n",
+ strerror(errno));
+ }
+}
+
+void *xen_be_map_grant_refs(struct XenDevice *xendev, uint32_t *refs,
+ unsigned int nr_refs, int prot)
+{
+ void *ptr;
+
+ assert(xendev->ops->flags & DEVOPS_FLAG_NEED_GNTDEV);
+
+ ptr = xengnttab_map_domain_grant_refs(xendev->gnttabdev, nr_refs,
+ xen_domid, refs, prot);
+ if (!ptr) {
+ xen_pv_printf(xendev, 0,
+ "xengnttab_map_domain_grant_refs failed: %s\n",
+ strerror(errno));
+ }
+
+ return ptr;
+}
+
+void xen_be_unmap_grant_refs(struct XenDevice *xendev, void *ptr,
+ unsigned int nr_refs)
+{
+ assert(xendev->ops->flags & DEVOPS_FLAG_NEED_GNTDEV);
+
+ if (xengnttab_unmap(xendev->gnttabdev, ptr, nr_refs)) {
+ xen_pv_printf(xendev, 0, "xengnttab_unmap failed: %s\n",
+ strerror(errno));
+ }
+}
+
+int xen_be_copy_grant_refs(struct XenDevice *xendev,
+ bool to_domain,
+ XenGrantCopySegment segs[],
+ unsigned int nr_segs)
+{
+ xengnttab_grant_copy_segment_t *xengnttab_segs;
+ unsigned int i;
+ int rc;
+
+ assert(xendev->ops->flags & DEVOPS_FLAG_NEED_GNTDEV);
+
+ xengnttab_segs = g_new0(xengnttab_grant_copy_segment_t, nr_segs);
+
+ for (i = 0; i < nr_segs; i++) {
+ XenGrantCopySegment *seg = &segs[i];
+ xengnttab_grant_copy_segment_t *xengnttab_seg = &xengnttab_segs[i];
+
+ if (to_domain) {
+ xengnttab_seg->flags = GNTCOPY_dest_gref;
+ xengnttab_seg->dest.foreign.domid = xen_domid;
+ xengnttab_seg->dest.foreign.ref = seg->dest.foreign.ref;
+ xengnttab_seg->dest.foreign.offset = seg->dest.foreign.offset;
+ xengnttab_seg->source.virt = seg->source.virt;
+ } else {
+ xengnttab_seg->flags = GNTCOPY_source_gref;
+ xengnttab_seg->source.foreign.domid = xen_domid;
+ xengnttab_seg->source.foreign.ref = seg->source.foreign.ref;
+ xengnttab_seg->source.foreign.offset =
+ seg->source.foreign.offset;
+ xengnttab_seg->dest.virt = seg->dest.virt;
+ }
+
+ xengnttab_seg->len = seg->len;
+ }
+
+ rc = xengnttab_grant_copy(xendev->gnttabdev, nr_segs, xengnttab_segs);
+
+ if (rc) {
+ xen_pv_printf(xendev, 0, "xengnttab_copy failed: %s\n",
+ strerror(errno));
+ }
+
+ for (i = 0; i < nr_segs; i++) {
+ xengnttab_grant_copy_segment_t *xengnttab_seg =
+ &xengnttab_segs[i];
+
+ if (xengnttab_seg->status != GNTST_okay) {
+ xen_pv_printf(xendev, 0, "segment[%u] status: %d\n", i,
+ xengnttab_seg->status);
+ rc = -1;
+ }
+ }
+
+ g_free(xengnttab_segs);
+ return rc;
+}
+
/*
* get xen backend device, allocate a new one if it doesn't exist.
*/
@@ -149,18 +246,6 @@ static struct XenDevice *xen_be_get_xendev(const char *type, int dom, int dev,
}
qemu_set_cloexec(xenevtchn_fd(xendev->evtchndev));

- if (ops->flags & DEVOPS_FLAG_NEED_GNTDEV) {
- xendev->gnttabdev = xengnttab_open(NULL, 0);
- if (xendev->gnttabdev == NULL) {
- xen_pv_printf(NULL, 0, "can't open gnttab device\n");
- xenevtchn_close(xendev->evtchndev);
- qdev_unplug(DEVICE(xendev), NULL);
- return NULL;
- }
- } else {
- xendev->gnttabdev = NULL;
- }
-
xen_pv_insert_xendev(xendev);

if (xendev->ops->alloc) {
@@ -322,6 +407,16 @@ static int xen_be_try_initialise(struct XenDevice *xendev)
}
}

+ if (xendev->ops->flags & DEVOPS_FLAG_NEED_GNTDEV) {
+ xendev->gnttabdev = xengnttab_open(NULL, 0);
+ if (xendev->gnttabdev == NULL) {
+ xen_pv_printf(NULL, 0, "can't open gnttab device\n");
+ return -1;
+ }
+ } else {
+ xendev->gnttabdev = NULL;
+ }
+
if (xendev->ops->initialise) {
rc = xendev->ops->initialise(xendev);
}
@@ -369,6 +464,10 @@ static void xen_be_disconnect(struct XenDevice *xendev, enum xenbus_state state)
xendev->ops->disconnect) {
xendev->ops->disconnect(xendev);
}
+ if (xendev->gnttabdev) {
+ xengnttab_close(xendev->gnttabdev);
+ xendev->gnttabdev = NULL;
+ }
if (xendev->be_state != state) {
xen_be_set_state(xendev, state);
}
diff --git a/include/hw/xen/xen_backend.h b/include/hw/xen/xen_backend.h
index 3a27692..29bf1c3 100644
--- a/include/hw/xen/xen_backend.h
+++ b/include/hw/xen/xen_backend.h
@@ -42,6 +42,39 @@ void xen_be_register_common(void);
int xen_be_register(const char *type, struct XenDevOps *ops);
int xen_be_set_state(struct XenDevice *xendev, enum xenbus_state state);
int xen_be_bind_evtchn(struct XenDevice *xendev);
+void xen_be_set_max_grant_refs(struct XenDevice *xendev,
+ unsigned int nr_refs);
+void *xen_be_map_grant_refs(struct XenDevice *xendev, uint32_t *refs,
+ unsigned int nr_refs, int prot);
+void xen_be_unmap_grant_refs(struct XenDevice *xendev, void *ptr,
+ unsigned int nr_refs);
+
+typedef struct XenGrantCopySegment {
+ union {
+ void *virt;
+ struct {
+ uint32_t ref;
+ off_t offset;
+ } foreign;
+ } source, dest;
+ size_t len;
+} XenGrantCopySegment;
+
+int xen_be_copy_grant_refs(struct XenDevice *xendev,
+ bool to_domain, XenGrantCopySegment segs[],
+ unsigned int nr_segs);
+
+static inline void *xen_be_map_grant_ref(struct XenDevice *xendev,
+ uint32_t ref, int prot)
+{
+ return xen_be_map_grant_refs(xendev, &ref, 1, prot);
+}
+
+static inline void xen_be_unmap_grant_ref(struct XenDevice *xendev,
+ void *ptr)
+{
+ return xen_be_unmap_grant_refs(xendev, ptr, 1);
+}

/* actual backend drivers */
extern struct XenDevOps xen_console_ops; /* xen_console.c */
--
2.1.4
Paul Durrant
2018-05-16 14:00:18 UTC
Permalink
-----Original Message-----
Sent: 16 May 2018 14:50
Subject: Re: [PATCH v3 1/8] xen_backend: add grant table helpers
Post by Paul Durrant
This patch adds grant table helper functions to the xen_backend code to
localize error reporting and use of xen_domid.
The patch also defers the call to xengnttab_open() until just before the
initialise method in XenDevOps is invoked. This method is responsible for
mapping the shared ring. No prior method requires access to the grant
table.
Post by Paul Durrant
---
- New in v2
---
hw/xen/xen_backend.c | 123
++++++++++++++++++++++++++++++++++++++-----
Post by Paul Durrant
include/hw/xen/xen_backend.h | 33 ++++++++++++
2 files changed, 144 insertions(+), 12 deletions(-)
diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
index 7445b50..50412d6 100644
--- a/hw/xen/xen_backend.c
+++ b/hw/xen/xen_backend.c
@@ -106,6 +106,103 @@ int xen_be_set_state(struct XenDevice *xendev,
enum xenbus_state state)
Post by Paul Durrant
return 0;
}
+void xen_be_set_max_grant_refs(struct XenDevice *xendev,
+ unsigned int nr_refs)
Is it fine to ignore error from set_max_grants and continue ? xen_disk.c
seems to fail the initialisation if set_max_grants call fails. On the
other end, xen-usb.c just keep going.
I guess the upshot will be that a subsequent grant map would fail, so I think it should be sufficient to deal with the failure there. As you say it's use is inconsistent, and just plain missing in some cases.
Post by Paul Durrant
+{
+ assert(xendev->ops->flags & DEVOPS_FLAG_NEED_GNTDEV);
+
+ if (xengnttab_set_max_grants(xendev->gnttabdev, nr_refs)) {
+ xen_pv_printf(xendev, 0, "xengnttab_set_max_grants failed: %s\n",
+ strerror(errno));
+ }
+}
+
+int xen_be_copy_grant_refs(struct XenDevice *xendev,
+ bool to_domain,
+ XenGrantCopySegment segs[],
+ unsigned int nr_segs)
+{
+ xengnttab_grant_copy_segment_t *xengnttab_segs;
+ unsigned int i;
+ int rc;
+
+ assert(xendev->ops->flags & DEVOPS_FLAG_NEED_GNTDEV);
+
+ xengnttab_segs = g_new0(xengnttab_grant_copy_segment_t,
nr_segs);
Post by Paul Durrant
+
+ for (i = 0; i < nr_segs; i++) {
+ XenGrantCopySegment *seg = &segs[i];
+ xengnttab_grant_copy_segment_t *xengnttab_seg =
&xengnttab_segs[i];
Post by Paul Durrant
+
+ if (to_domain) {
+ xengnttab_seg->flags = GNTCOPY_dest_gref;
+ xengnttab_seg->dest.foreign.domid = xen_domid;
+ xengnttab_seg->dest.foreign.ref = seg->dest.foreign.ref;
+ xengnttab_seg->dest.foreign.offset = seg->dest.foreign.offset;
+ xengnttab_seg->source.virt = seg->source.virt;
+ } else {
+ xengnttab_seg->flags = GNTCOPY_source_gref;
+ xengnttab_seg->source.foreign.domid = xen_domid;
+ xengnttab_seg->source.foreign.ref = seg->source.foreign.ref;
+ xengnttab_seg->source.foreign.offset =
+ seg->source.foreign.offset;
+ xengnttab_seg->dest.virt = seg->dest.virt;
+ }
That's not going to work because xengnttab_grant_copy_segment_t doesn't
exist on Xen 4.7.
Ah, I'd missed the ifdef around that in xen_disk. I'll add it here.

Cheers,

Paul
Post by Paul Durrant
+
+ xengnttab_seg->len = seg->len;
+ }
+
+ rc = xengnttab_grant_copy(xendev->gnttabdev, nr_segs,
xengnttab_segs);
Thanks,
--
Anthony PERARD
Paul Durrant
2018-05-04 19:26:06 UTC
Permalink
Since xen_disk now always copies data to and from a guest there is no need
to maintain a vector entry corresponding to every page of a request.
This means there is less per-request state to maintain so the ioreq
structure can shrink significantly.

Signed-off-by: Paul Durrant <***@citrix.com>
---
Cc: Stefano Stabellini <***@kernel.org>
Cc: Anthony Perard <***@citrix.com>
Cc: Kevin Wolf <***@redhat.com>
Cc: Max Reitz <***@redhat.com>

v3:
- Un-break by fixing mis-placed qemu_iovec_add()

v2:
- Re-based
---
hw/block/xen_disk.c | 76 +++++++++++++++--------------------------------------
1 file changed, 21 insertions(+), 55 deletions(-)

diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 28be8b6..28651c5 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -46,13 +46,10 @@ struct ioreq {
/* parsed request */
off_t start;
QEMUIOVector v;
+ void *buf;
+ size_t size;
int presync;

- /* grant mapping */
- uint32_t refs[BLKIF_MAX_SEGMENTS_PER_REQUEST];
- void *page[BLKIF_MAX_SEGMENTS_PER_REQUEST];
- void *pages;
-
/* aio status */
int aio_inflight;
int aio_errors;
@@ -110,12 +107,10 @@ static void ioreq_reset(struct ioreq *ioreq)
memset(&ioreq->req, 0, sizeof(ioreq->req));
ioreq->status = 0;
ioreq->start = 0;
+ ioreq->buf = NULL;
+ ioreq->size = 0;
ioreq->presync = 0;

- memset(ioreq->refs, 0, sizeof(ioreq->refs));
- memset(ioreq->page, 0, sizeof(ioreq->page));
- ioreq->pages = NULL;
-
ioreq->aio_inflight = 0;
ioreq->aio_errors = 0;

@@ -138,7 +133,7 @@ static struct ioreq *ioreq_start(struct XenBlkDev *blkdev)
ioreq = g_malloc0(sizeof(*ioreq));
ioreq->blkdev = blkdev;
blkdev->requests_total++;
- qemu_iovec_init(&ioreq->v, BLKIF_MAX_SEGMENTS_PER_REQUEST);
+ qemu_iovec_init(&ioreq->v, 1);
} else {
/* get one from freelist */
ioreq = QLIST_FIRST(&blkdev->freelist);
@@ -183,7 +178,6 @@ static void ioreq_release(struct ioreq *ioreq, bool finish)
static int ioreq_parse(struct ioreq *ioreq)
{
struct XenBlkDev *blkdev = ioreq->blkdev;
- uintptr_t mem;
size_t len;
int i;

@@ -230,13 +224,10 @@ static int ioreq_parse(struct ioreq *ioreq)
goto err;
}

- ioreq->refs[i] = ioreq->req.seg[i].gref;
-
- mem = ioreq->req.seg[i].first_sect * blkdev->file_blk;
len = (ioreq->req.seg[i].last_sect - ioreq->req.seg[i].first_sect + 1) * blkdev->file_blk;
- qemu_iovec_add(&ioreq->v, (void*)mem, len);
+ ioreq->size += len;
}
- if (ioreq->start + ioreq->v.size > blkdev->file_size) {
+ if (ioreq->start + ioreq->size > blkdev->file_size) {
xen_pv_printf(&blkdev->xendev, 0, "error: access beyond end of file\n");
goto err;
}
@@ -247,35 +238,6 @@ err:
return -1;
}

-static void ioreq_free_copy_buffers(struct ioreq *ioreq)
-{
- int i;
-
- for (i = 0; i < ioreq->v.niov; i++) {
- ioreq->page[i] = NULL;
- }
-
- qemu_vfree(ioreq->pages);
-}
-
-static int ioreq_init_copy_buffers(struct ioreq *ioreq)
-{
- int i;
-
- if (ioreq->v.niov == 0) {
- return 0;
- }
-
- ioreq->pages = qemu_memalign(XC_PAGE_SIZE, ioreq->v.niov * XC_PAGE_SIZE);
-
- for (i = 0; i < ioreq->v.niov; i++) {
- ioreq->page[i] = ioreq->pages + i * XC_PAGE_SIZE;
- ioreq->v.iov[i].iov_base = ioreq->page[i];
- }
-
- return 0;
-}
-
static int ioreq_grant_copy(struct ioreq *ioreq)
{
struct XenBlkDev *blkdev = ioreq->blkdev;
@@ -284,25 +246,27 @@ static int ioreq_grant_copy(struct ioreq *ioreq)
int i, count, rc;
int64_t file_blk = ioreq->blkdev->file_blk;
bool to_domain = (ioreq->req.operation == BLKIF_OP_READ);
+ void *virt = ioreq->buf;

- if (ioreq->v.niov == 0) {
+ if (ioreq->req.nr_segments == 0) {
return 0;
}

- count = ioreq->v.niov;
+ count = ioreq->req.nr_segments;

for (i = 0; i < count; i++) {
if (to_domain) {
- segs[i].dest.foreign.ref = ioreq->refs[i];
+ segs[i].dest.foreign.ref = ioreq->req.seg[i].gref;
segs[i].dest.foreign.offset = ioreq->req.seg[i].first_sect * file_blk;
- segs[i].source.virt = ioreq->v.iov[i].iov_base;
+ segs[i].source.virt = virt;
} else {
- segs[i].source.foreign.ref = ioreq->refs[i];
+ segs[i].source.foreign.ref = ioreq->req.seg[i].gref;
segs[i].source.foreign.offset = ioreq->req.seg[i].first_sect * file_blk;
- segs[i].dest.virt = ioreq->v.iov[i].iov_base;
+ segs[i].dest.virt = virt;
}
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);
@@ -348,14 +312,14 @@ static void qemu_aio_complete(void *opaque, int ret)
if (ret == 0) {
ioreq_grant_copy(ioreq);
}
- ioreq_free_copy_buffers(ioreq);
+ qemu_vfree(ioreq->buf);
break;
case BLKIF_OP_WRITE:
case BLKIF_OP_FLUSH_DISKCACHE:
if (!ioreq->req.nr_segments) {
break;
}
- ioreq_free_copy_buffers(ioreq);
+ qemu_vfree(ioreq->buf);
break;
default:
break;
@@ -423,12 +387,12 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
{
struct XenBlkDev *blkdev = ioreq->blkdev;

- ioreq_init_copy_buffers(ioreq);
+ ioreq->buf = qemu_memalign(XC_PAGE_SIZE, ioreq->size);
if (ioreq->req.nr_segments &&
(ioreq->req.operation == BLKIF_OP_WRITE ||
ioreq->req.operation == BLKIF_OP_FLUSH_DISKCACHE) &&
ioreq_grant_copy(ioreq)) {
- ioreq_free_copy_buffers(ioreq);
+ qemu_vfree(ioreq->buf);
goto err;
}

@@ -440,6 +404,7 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)

switch (ioreq->req.operation) {
case BLKIF_OP_READ:
+ qemu_iovec_add(&ioreq->v, ioreq->buf, ioreq->size);
block_acct_start(blk_get_stats(blkdev->blk), &ioreq->acct,
ioreq->v.size, BLOCK_ACCT_READ);
ioreq->aio_inflight++;
@@ -452,6 +417,7 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
break;
}

+ qemu_iovec_add(&ioreq->v, ioreq->buf, ioreq->size);
block_acct_start(blk_get_stats(blkdev->blk), &ioreq->acct,
ioreq->v.size,
ioreq->req.operation == BLKIF_OP_WRITE ?
--
2.1.4
Paul Durrant
2018-05-04 19:26:03 UTC
Permalink
Not all Xen environments support the xengnttab_grant_copy() operation.
E.g. where the OS is FreeBSD or Xen is older than 4.8.0.

This patch introduces an emulation of that operation using
xengnttab_map_domain_grant_refs() and memcpy() for those environments.

Signed-off-by: Paul Durrant <***@citrix.com>
---
Cc: Stefano Stabellini <***@kernel.org>
Cc: Anthony Perard <***@citrix.com>

v2:
- New in v2
---
hw/xen/xen_backend.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 53 insertions(+)

diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
index 50412d6..3c3fc2c 100644
--- a/hw/xen/xen_backend.c
+++ b/hw/xen/xen_backend.c
@@ -146,6 +146,55 @@ void xen_be_unmap_grant_refs(struct XenDevice *xendev, void *ptr,
}
}

+static int compat_copy_grant_refs(struct XenDevice *xendev,
+ bool to_domain,
+ XenGrantCopySegment segs[],
+ unsigned int nr_segs)
+{
+ uint32_t *refs = g_new(uint32_t, nr_segs);
+ int prot = to_domain ? PROT_WRITE : PROT_READ;
+ void *pages;
+ unsigned int i;
+
+ for (i = 0; i < nr_segs; i++) {
+ XenGrantCopySegment *seg = &segs[i];
+
+ refs[i] = to_domain ?
+ seg->dest.foreign.ref : seg->source.foreign.ref;
+ }
+
+ pages = xengnttab_map_domain_grant_refs(xendev->gnttabdev, nr_segs,
+ xen_domid, refs, prot);
+ if (!pages) {
+ xen_pv_printf(xendev, 0,
+ "xengnttab_map_domain_grant_refs failed: %s\n",
+ strerror(errno));
+ g_free(refs);
+ return -1;
+ }
+
+ for (i = 0; i < nr_segs; i++) {
+ XenGrantCopySegment *seg = &segs[i];
+ void *page = pages + (i * XC_PAGE_SIZE);
+
+ if (to_domain) {
+ memcpy(page + seg->dest.foreign.offset, seg->source.virt,
+ seg->len);
+ } else {
+ memcpy(seg->dest.virt, page + seg->source.foreign.offset,
+ seg->len);
+ }
+ }
+
+ if (xengnttab_unmap(xendev->gnttabdev, pages, nr_segs)) {
+ xen_pv_printf(xendev, 0, "xengnttab_unmap failed: %s\n",
+ strerror(errno));
+ }
+
+ g_free(refs);
+ return 0;
+}
+
int xen_be_copy_grant_refs(struct XenDevice *xendev,
bool to_domain,
XenGrantCopySegment segs[],
@@ -157,6 +206,10 @@ int xen_be_copy_grant_refs(struct XenDevice *xendev,

assert(xendev->ops->flags & DEVOPS_FLAG_NEED_GNTDEV);

+ if (!xen_feature_grant_copy) {
+ return compat_copy_grant_refs(xendev, to_domain, segs, nr_segs);
+ }
+
xengnttab_segs = g_new0(xengnttab_grant_copy_segment_t, nr_segs);

for (i = 0; i < nr_segs; i++) {
--
2.1.4
Anthony PERARD
2018-05-16 14:30:32 UTC
Permalink
Post by Paul Durrant
Not all Xen environments support the xengnttab_grant_copy() operation.
E.g. where the OS is FreeBSD or Xen is older than 4.8.0.
This patch introduces an emulation of that operation using
xengnttab_map_domain_grant_refs() and memcpy() for those environments.
---
The patch looks ok, but since patch 1 is going to be change, this one is
going to need to be change as well.
--
Anthony PERARD
Paul Durrant
2018-05-16 14:34:18 UTC
Permalink
-----Original Message-----
Sent: 16 May 2018 15:31
Subject: Re: [PATCH v3 4/8] xen_backend: add an emulation of grant copy
Post by Paul Durrant
Not all Xen environments support the xengnttab_grant_copy() operation.
E.g. where the OS is FreeBSD or Xen is older than 4.8.0.
This patch introduces an emulation of that operation using
xengnttab_map_domain_grant_refs() and memcpy() for those
environments.
Post by Paul Durrant
---
The patch looks ok, but since patch 1 is going to be change, this one is
going to need to be change as well.
Sure. Thanks for the review.

Paul
--
Anthony PERARD
Paul Durrant
2018-05-04 19:26:05 UTC
Permalink
There is no longer any use of this flag outside of the xen_backend code.

Signed-off-by: Paul Durrant <***@citrix.com>
---
Cc: Stefano Stabellini <***@kernel.org>
Cc: Anthony Perard <***@citrix.com>

v2:
- New in v2
---
hw/xen/xen_backend.c | 2 +-
include/hw/xen/xen_backend.h | 1 -
2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
index 3c3fc2c..9a8e877 100644
--- a/hw/xen/xen_backend.c
+++ b/hw/xen/xen_backend.c
@@ -44,9 +44,9 @@ BusState *xen_sysbus;
/* public */
struct xs_handle *xenstore = NULL;
const char *xen_protocol;
-bool xen_feature_grant_copy;

/* private */
+static bool xen_feature_grant_copy;
static int debug;

int xenstore_write_be_str(struct XenDevice *xendev, const char *node, const char *val)
diff --git a/include/hw/xen/xen_backend.h b/include/hw/xen/xen_backend.h
index 29bf1c3..9c17fdd 100644
--- a/include/hw/xen/xen_backend.h
+++ b/include/hw/xen/xen_backend.h
@@ -16,7 +16,6 @@
/* variables */
extern struct xs_handle *xenstore;
extern const char *xen_protocol;
-extern bool xen_feature_grant_copy;
extern DeviceState *xen_sysdev;
extern BusState *xen_sysbus;
--
2.1.4
Paul Durrant
2018-05-04 19:26:01 UTC
Permalink
Now that helpers are present in xen_backend, this patch removes open-coded
calls to libxengnttab from the xen_disk code.

This patch also fixes one whitspace error in the assignment of the
XenDevOps initialise method.

Signed-off-by: Paul Durrant <***@citrix.com>
---
Cc: Stefano Stabellini <***@kernel.org>
Cc: Anthony Perard <***@citrix.com>
Cc: Kevin Wolf <***@redhat.com>
Cc: Max Reitz <***@redhat.com>

v2:
- New in v2
---
hw/block/xen_disk.c | 122 ++++++++++++++--------------------------------------
1 file changed, 32 insertions(+), 90 deletions(-)

diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index f74fcd4..66ed2b7 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -68,7 +68,6 @@ struct ioreq {
uint8_t mapped;

/* grant mapping */
- uint32_t domids[BLKIF_MAX_SEGMENTS_PER_REQUEST];
uint32_t refs[BLKIF_MAX_SEGMENTS_PER_REQUEST];
int prot;
void *page[BLKIF_MAX_SEGMENTS_PER_REQUEST];
@@ -142,7 +141,6 @@ static void ioreq_reset(struct ioreq *ioreq)
ioreq->presync = 0;
ioreq->mapped = 0;

- memset(ioreq->domids, 0, sizeof(ioreq->domids));
memset(ioreq->refs, 0, sizeof(ioreq->refs));
ioreq->prot = 0;
memset(ioreq->page, 0, sizeof(ioreq->page));
@@ -168,16 +166,12 @@ static gint int_cmp(gconstpointer a, gconstpointer b, gpointer user_data)
static void destroy_grant(gpointer pgnt)
{
PersistentGrant *grant = pgnt;
- xengnttab_handle *gnt = grant->blkdev->xendev.gnttabdev;
+ struct XenBlkDev *blkdev = grant->blkdev;
+ struct XenDevice *xendev = &blkdev->xendev;

- if (xengnttab_unmap(gnt, grant->page, 1) != 0) {
- xen_pv_printf(&grant->blkdev->xendev, 0,
- "xengnttab_unmap failed: %s\n",
- strerror(errno));
- }
+ xen_be_unmap_grant_ref(xendev, grant->page);
grant->blkdev->persistent_gnt_count--;
- xen_pv_printf(&grant->blkdev->xendev, 3,
- "unmapped grant %p\n", grant->page);
+ xen_pv_printf(xendev, 3, "unmapped grant %p\n", grant->page);
g_free(grant);
}

@@ -185,15 +179,10 @@ static void remove_persistent_region(gpointer data, gpointer dev)
{
PersistentRegion *region = data;
struct XenBlkDev *blkdev = dev;
- xengnttab_handle *gnt = blkdev->xendev.gnttabdev;
+ struct XenDevice *xendev = &blkdev->xendev;

- if (xengnttab_unmap(gnt, region->addr, region->num) != 0) {
- xen_pv_printf(&blkdev->xendev, 0,
- "xengnttab_unmap region %p failed: %s\n",
- region->addr, strerror(errno));
- }
- xen_pv_printf(&blkdev->xendev, 3,
- "unmapped grant region %p with %d pages\n",
+ xen_be_unmap_grant_refs(xendev, region->addr, region->num);
+ xen_pv_printf(xendev, 3, "unmapped grant region %p with %d pages\n",
region->addr, region->num);
g_free(region);
}
@@ -304,7 +293,6 @@ static int ioreq_parse(struct ioreq *ioreq)
goto err;
}

- ioreq->domids[i] = blkdev->xendev.dom;
ioreq->refs[i] = ioreq->req.seg[i].gref;

mem = ioreq->req.seg[i].first_sect * blkdev->file_blk;
@@ -324,7 +312,8 @@ err:

static void ioreq_unmap(struct ioreq *ioreq)
{
- xengnttab_handle *gnt = ioreq->blkdev->xendev.gnttabdev;
+ struct XenBlkDev *blkdev = ioreq->blkdev;
+ struct XenDevice *xendev = &blkdev->xendev;
int i;

if (ioreq->num_unmap == 0 || ioreq->mapped == 0) {
@@ -334,11 +323,7 @@ static void ioreq_unmap(struct ioreq *ioreq)
if (!ioreq->pages) {
return;
}
- if (xengnttab_unmap(gnt, ioreq->pages, ioreq->num_unmap) != 0) {
- xen_pv_printf(&ioreq->blkdev->xendev, 0,
- "xengnttab_unmap failed: %s\n",
- strerror(errno));
- }
+ xen_be_unmap_grant_refs(xendev, ioreq->pages, ioreq->num_unmap);
ioreq->blkdev->cnt_map -= ioreq->num_unmap;
ioreq->pages = NULL;
} else {
@@ -346,11 +331,7 @@ static void ioreq_unmap(struct ioreq *ioreq)
if (!ioreq->page[i]) {
continue;
}
- if (xengnttab_unmap(gnt, ioreq->page[i], 1) != 0) {
- xen_pv_printf(&ioreq->blkdev->xendev, 0,
- "xengnttab_unmap failed: %s\n",
- strerror(errno));
- }
+ xen_be_unmap_grant_ref(xendev, ioreq->page[i]);
ioreq->blkdev->cnt_map--;
ioreq->page[i] = NULL;
}
@@ -360,14 +341,14 @@ static void ioreq_unmap(struct ioreq *ioreq)

static int ioreq_map(struct ioreq *ioreq)
{
- xengnttab_handle *gnt = ioreq->blkdev->xendev.gnttabdev;
- uint32_t domids[BLKIF_MAX_SEGMENTS_PER_REQUEST];
+ struct XenBlkDev *blkdev = ioreq->blkdev;
+ struct XenDevice *xendev = &blkdev->xendev;
uint32_t refs[BLKIF_MAX_SEGMENTS_PER_REQUEST];
void *page[BLKIF_MAX_SEGMENTS_PER_REQUEST];
int i, j, new_maps = 0;
PersistentGrant *grant;
PersistentRegion *region;
- /* domids and refs variables will contain the information necessary
+ /* refs variable will contain the information necessary
* to map the grants that are needed to fulfill this request.
*
* After mapping the needed grants, the page array will contain the
@@ -392,7 +373,6 @@ static int ioreq_map(struct ioreq *ioreq)
/* Add the grant to the list of grants that
* should be mapped
*/
- domids[new_maps] = ioreq->domids[i];
refs[new_maps] = ioreq->refs[i];
page[i] = NULL;
new_maps++;
@@ -405,14 +385,13 @@ static int ioreq_map(struct ioreq *ioreq)
} else {
/* All grants in the request should be mapped */
memcpy(refs, ioreq->refs, sizeof(refs));
- memcpy(domids, ioreq->domids, sizeof(domids));
memset(page, 0, sizeof(page));
new_maps = ioreq->v.niov;
}

if (batch_maps && new_maps) {
- ioreq->pages = xengnttab_map_grant_refs
- (gnt, new_maps, domids, refs, ioreq->prot);
+ ioreq->pages = xen_be_map_grant_refs(xendev, refs, new_maps,
+ ioreq->prot);
if (ioreq->pages == NULL) {
xen_pv_printf(&ioreq->blkdev->xendev, 0,
"can't map %d grant refs (%s, %d maps)\n",
@@ -427,8 +406,8 @@ static int ioreq_map(struct ioreq *ioreq)
ioreq->blkdev->cnt_map += new_maps;
} else if (new_maps) {
for (i = 0; i < new_maps; i++) {
- ioreq->page[i] = xengnttab_map_grant_ref
- (gnt, domids[i], refs[i], ioreq->prot);
+ ioreq->page[i] = xen_be_map_grant_ref(xendev, refs[i],
+ ioreq->prot);
if (ioreq->page[i] == NULL) {
xen_pv_printf(&ioreq->blkdev->xendev, 0,
"can't map grant ref %d (%s, %d maps)\n",
@@ -529,10 +508,12 @@ static int ioreq_init_copy_buffers(struct ioreq *ioreq)

static int ioreq_grant_copy(struct ioreq *ioreq)
{
- xengnttab_handle *gnt = ioreq->blkdev->xendev.gnttabdev;
- xengnttab_grant_copy_segment_t segs[BLKIF_MAX_SEGMENTS_PER_REQUEST];
+ struct XenBlkDev *blkdev = ioreq->blkdev;
+ struct XenDevice *xendev = &blkdev->xendev;
+ XenGrantCopySegment segs[BLKIF_MAX_SEGMENTS_PER_REQUEST];
int i, count, rc;
int64_t file_blk = ioreq->blkdev->file_blk;
+ bool to_domain = (ioreq->req.operation == BLKIF_OP_READ);

if (ioreq->v.niov == 0) {
return 0;
@@ -541,16 +522,12 @@ static int ioreq_grant_copy(struct ioreq *ioreq)
count = ioreq->v.niov;

for (i = 0; i < count; i++) {
- if (ioreq->req.operation == BLKIF_OP_READ) {
- segs[i].flags = GNTCOPY_dest_gref;
+ if (to_domain) {
segs[i].dest.foreign.ref = ioreq->refs[i];
- segs[i].dest.foreign.domid = ioreq->domids[i];
segs[i].dest.foreign.offset = ioreq->req.seg[i].first_sect * file_blk;
segs[i].source.virt = ioreq->v.iov[i].iov_base;
} else {
- segs[i].flags = GNTCOPY_source_gref;
segs[i].source.foreign.ref = ioreq->refs[i];
- segs[i].source.foreign.domid = ioreq->domids[i];
segs[i].source.foreign.offset = ioreq->req.seg[i].first_sect * file_blk;
segs[i].dest.virt = ioreq->v.iov[i].iov_base;
}
@@ -558,7 +535,7 @@ static int ioreq_grant_copy(struct ioreq *ioreq)
- ioreq->req.seg[i].first_sect + 1) * file_blk;
}

- rc = xengnttab_grant_copy(gnt, count, segs);
+ rc = xen_be_copy_grant_refs(xendev, to_domain, segs, count);

if (rc) {
xen_pv_printf(&ioreq->blkdev->xendev, 0,
@@ -567,16 +544,6 @@ static int ioreq_grant_copy(struct ioreq *ioreq)
return -1;
}

- for (i = 0; i < count; i++) {
- if (segs[i].status != GNTST_okay) {
- xen_pv_printf(&ioreq->blkdev->xendev, 3,
- "failed to copy data %d for gref %d, domid %d\n",
- segs[i].status, ioreq->refs[i], ioreq->domids[i]);
- ioreq->aio_errors++;
- rc = -1;
- }
- }
-
return rc;
}
#else
@@ -1085,7 +1052,6 @@ static int blk_connect(struct XenDevice *xendev)
int order, ring_ref;
unsigned int ring_size, max_grants;
unsigned int i;
- uint32_t *domids;

trace_xen_disk_connect(xendev->name);

@@ -1247,31 +1213,11 @@ static int blk_connect(struct XenDevice *xendev)
/* Add on the number needed for the ring pages */
max_grants += blkdev->nr_ring_ref;

- blkdev->xendev.gnttabdev = xengnttab_open(NULL, 0);
- if (blkdev->xendev.gnttabdev == NULL) {
- xen_pv_printf(xendev, 0, "xengnttab_open failed: %s\n",
- strerror(errno));
- return -1;
- }
- if (xengnttab_set_max_grants(blkdev->xendev.gnttabdev, max_grants)) {
- xen_pv_printf(xendev, 0, "xengnttab_set_max_grants failed: %s\n",
- strerror(errno));
- return -1;
- }
-
- domids = g_new0(uint32_t, blkdev->nr_ring_ref);
- for (i = 0; i < blkdev->nr_ring_ref; i++) {
- domids[i] = blkdev->xendev.dom;
- }
-
- blkdev->sring = xengnttab_map_grant_refs(blkdev->xendev.gnttabdev,
- blkdev->nr_ring_ref,
- domids,
- blkdev->ring_ref,
- PROT_READ | PROT_WRITE);
-
- g_free(domids);
+ xen_be_set_max_grant_refs(xendev, max_grants);

+ blkdev->sring = xen_be_map_grant_refs(xendev, blkdev->ring_ref,
+ blkdev->nr_ring_ref,
+ PROT_READ | PROT_WRITE);
if (!blkdev->sring) {
return -1;
}
@@ -1344,8 +1290,8 @@ static void blk_disconnect(struct XenDevice *xendev)
aio_context_release(blkdev->ctx);

if (blkdev->sring) {
- xengnttab_unmap(blkdev->xendev.gnttabdev, blkdev->sring,
- blkdev->nr_ring_ref);
+ xen_be_unmap_grant_refs(xendev, blkdev->sring,
+ blkdev->nr_ring_ref);
blkdev->cnt_map--;
blkdev->sring = NULL;
}
@@ -1369,11 +1315,6 @@ static void blk_disconnect(struct XenDevice *xendev)
}
blkdev->feature_persistent = false;
}
-
- if (blkdev->xendev.gnttabdev) {
- xengnttab_close(blkdev->xendev.gnttabdev);
- blkdev->xendev.gnttabdev = NULL;
- }
}

static int blk_free(struct XenDevice *xendev)
@@ -1410,10 +1351,11 @@ static void blk_event(struct XenDevice *xendev)
}

struct XenDevOps xen_blkdev_ops = {
+ .flags = DEVOPS_FLAG_NEED_GNTDEV,
.size = sizeof(struct XenBlkDev),
.alloc = blk_alloc,
.init = blk_init,
- .initialise = blk_connect,
+ .initialise = blk_connect,
.disconnect = blk_disconnect,
.event = blk_event,
.free = blk_free,
--
2.1.4
Anthony PERARD
2018-05-16 14:02:59 UTC
Permalink
Post by Paul Durrant
Now that helpers are present in xen_backend, this patch removes open-coded
calls to libxengnttab from the xen_disk code.
This patch also fixes one whitspace error in the assignment of the
XenDevOps initialise method.
Acked-by: Anthony PERARD <***@citrix.com>
--
Anthony PERARD
Paul Durrant
2018-05-04 19:26:02 UTC
Permalink
Now that helpers are available in xen_backend, use them throughout all
Xen PV backends.

Signed-off-by: Paul Durrant <***@citrix.com>
---
Cc: Stefano Stabellini <***@kernel.org>
Cc: Anthony Perard <***@citrix.com>
Cc: Greg Kurz <***@kaod.org>
Cc: Paolo Bonzini <***@redhat.com>
Cc: Jason Wang <***@redhat.com>
Cc: Gerd Hoffmann <***@redhat.com>

v2:
- New in v2
---
hw/9pfs/xen-9p-backend.c | 32 +++++++++++++++-----------------
hw/char/xen_console.c | 9 ++++-----
hw/net/xen_nic.c | 34 +++++++++++++++-------------------
hw/usb/xen-usb.c | 37 +++++++++++++++++--------------------
4 files changed, 51 insertions(+), 61 deletions(-)

diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
index 95e50c4..6026780 100644
--- a/hw/9pfs/xen-9p-backend.c
+++ b/hw/9pfs/xen-9p-backend.c
@@ -331,14 +331,14 @@ static int xen_9pfs_free(struct XenDevice *xendev)

for (i = 0; i < xen_9pdev->num_rings; i++) {
if (xen_9pdev->rings[i].data != NULL) {
- xengnttab_unmap(xen_9pdev->xendev.gnttabdev,
- xen_9pdev->rings[i].data,
- (1 << xen_9pdev->rings[i].ring_order));
+ xen_be_unmap_grant_refs(&xen_9pdev->xendev,
+ xen_9pdev->rings[i].data,
+ (1 << xen_9pdev->rings[i].ring_order));
}
if (xen_9pdev->rings[i].intf != NULL) {
- xengnttab_unmap(xen_9pdev->xendev.gnttabdev,
- xen_9pdev->rings[i].intf,
- 1);
+ xen_be_unmap_grant_refs(&xen_9pdev->xendev,
+ xen_9pdev->rings[i].intf,
+ 1);
}
if (xen_9pdev->rings[i].bh != NULL) {
qemu_bh_delete(xen_9pdev->rings[i].bh);
@@ -390,11 +390,10 @@ static int xen_9pfs_connect(struct XenDevice *xendev)
}
g_free(str);

- xen_9pdev->rings[i].intf = xengnttab_map_grant_ref(
- xen_9pdev->xendev.gnttabdev,
- xen_9pdev->xendev.dom,
- xen_9pdev->rings[i].ref,
- PROT_READ | PROT_WRITE);
+ xen_9pdev->rings[i].intf =
+ xen_be_map_grant_ref(&xen_9pdev->xendev,
+ xen_9pdev->rings[i].ref,
+ PROT_READ | PROT_WRITE);
if (!xen_9pdev->rings[i].intf) {
goto out;
}
@@ -403,12 +402,11 @@ static int xen_9pfs_connect(struct XenDevice *xendev)
goto out;
}
xen_9pdev->rings[i].ring_order = ring_order;
- xen_9pdev->rings[i].data = xengnttab_map_domain_grant_refs(
- xen_9pdev->xendev.gnttabdev,
- (1 << ring_order),
- xen_9pdev->xendev.dom,
- xen_9pdev->rings[i].intf->ref,
- PROT_READ | PROT_WRITE);
+ xen_9pdev->rings[i].data =
+ xen_be_map_grant_refs(&xen_9pdev->xendev,
+ xen_9pdev->rings[i].intf->ref,
+ (1 << ring_order),
+ PROT_READ | PROT_WRITE);
if (!xen_9pdev->rings[i].data) {
goto out;
}
diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index bdfaa40..8b4b4bf 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -233,12 +233,11 @@ static int con_initialise(struct XenDevice *xendev)
if (!xendev->dev) {
xen_pfn_t mfn = con->ring_ref;
con->sring = xenforeignmemory_map(xen_fmem, con->xendev.dom,
- PROT_READ|PROT_WRITE,
+ PROT_READ | PROT_WRITE,
1, &mfn, NULL);
} else {
- con->sring = xengnttab_map_grant_ref(xendev->gnttabdev, con->xendev.dom,
- con->ring_ref,
- PROT_READ|PROT_WRITE);
+ con->sring = xen_be_map_grant_ref(xendev, con->ring_ref,
+ PROT_READ | PROT_WRITE);
}
if (!con->sring)
return -1;
@@ -267,7 +266,7 @@ static void con_disconnect(struct XenDevice *xendev)
if (!xendev->dev) {
xenforeignmemory_unmap(xen_fmem, con->sring, 1);
} else {
- xengnttab_unmap(xendev->gnttabdev, con->sring, 1);
+ xen_be_unmap_grant_ref(xendev, con->sring);
}
con->sring = NULL;
}
diff --git a/hw/net/xen_nic.c b/hw/net/xen_nic.c
index 20c43a6..73d6f1b 100644
--- a/hw/net/xen_nic.c
+++ b/hw/net/xen_nic.c
@@ -160,9 +160,8 @@ static void net_tx_packets(struct XenNetDev *netdev)
(txreq.flags & NETTXF_more_data) ? " more_data" : "",
(txreq.flags & NETTXF_extra_info) ? " extra_info" : "");

- page = xengnttab_map_grant_ref(netdev->xendev.gnttabdev,
- netdev->xendev.dom,
- txreq.gref, PROT_READ);
+ page = xen_be_map_grant_refs(&netdev->xendev,
+ &txreq.gref, 1, PROT_READ);
if (page == NULL) {
xen_pv_printf(&netdev->xendev, 0,
"error: tx gref dereference failed (%d)\n",
@@ -183,7 +182,7 @@ static void net_tx_packets(struct XenNetDev *netdev)
qemu_send_packet(qemu_get_queue(netdev->nic),
page + txreq.offset, txreq.size);
}
- xengnttab_unmap(netdev->xendev.gnttabdev, page, 1);
+ xen_be_unmap_grant_ref(&netdev->xendev, page);
net_tx_response(netdev, &txreq, NETIF_RSP_OKAY);
}
if (!netdev->tx_work) {
@@ -254,9 +253,8 @@ static ssize_t net_rx_packet(NetClientState *nc, const uint8_t *buf, size_t size
memcpy(&rxreq, RING_GET_REQUEST(&netdev->rx_ring, rc), sizeof(rxreq));
netdev->rx_ring.req_cons = ++rc;

- page = xengnttab_map_grant_ref(netdev->xendev.gnttabdev,
- netdev->xendev.dom,
- rxreq.gref, PROT_WRITE);
+ page = xen_be_map_grant_refs(&netdev->xendev, &rxreq.gref, 1,
+ PROT_WRITE);
if (page == NULL) {
xen_pv_printf(&netdev->xendev, 0,
"error: rx gref dereference failed (%d)\n",
@@ -265,7 +263,7 @@ static ssize_t net_rx_packet(NetClientState *nc, const uint8_t *buf, size_t size
return -1;
}
memcpy(page + NET_IP_ALIGN, buf, size);
- xengnttab_unmap(netdev->xendev.gnttabdev, page, 1);
+ xen_be_unmap_grant_ref(&netdev->xendev, page);
net_rx_response(netdev, &rxreq, NETIF_RSP_OKAY, NET_IP_ALIGN, size, 0);

return size;
@@ -338,19 +336,17 @@ static int net_connect(struct XenDevice *xendev)
return -1;
}

- netdev->txs = xengnttab_map_grant_ref(netdev->xendev.gnttabdev,
- netdev->xendev.dom,
- netdev->tx_ring_ref,
- PROT_READ | PROT_WRITE);
+ netdev->txs = xen_be_map_grant_ref(&netdev->xendev,
+ netdev->tx_ring_ref,
+ PROT_READ | PROT_WRITE);
if (!netdev->txs) {
return -1;
}
- netdev->rxs = xengnttab_map_grant_ref(netdev->xendev.gnttabdev,
- netdev->xendev.dom,
- netdev->rx_ring_ref,
- PROT_READ | PROT_WRITE);
+ netdev->rxs = xen_be_map_grant_ref(&netdev->xendev,
+ netdev->rx_ring_ref,
+ PROT_READ | PROT_WRITE);
if (!netdev->rxs) {
- xengnttab_unmap(netdev->xendev.gnttabdev, netdev->txs, 1);
+ xen_be_unmap_grant_ref(&netdev->xendev, netdev->txs);
netdev->txs = NULL;
return -1;
}
@@ -375,11 +371,11 @@ static void net_disconnect(struct XenDevice *xendev)
xen_pv_unbind_evtchn(&netdev->xendev);

if (netdev->txs) {
- xengnttab_unmap(netdev->xendev.gnttabdev, netdev->txs, 1);
+ xen_be_unmap_grant_ref(&netdev->xendev, netdev->txs);
netdev->txs = NULL;
}
if (netdev->rxs) {
- xengnttab_unmap(netdev->xendev.gnttabdev, netdev->rxs, 1);
+ xen_be_unmap_grant_ref(&netdev->xendev, netdev->rxs);
netdev->rxs = NULL;
}
}
diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c
index b3a90c0..5b2e21e 100644
--- a/hw/usb/xen-usb.c
+++ b/hw/usb/xen-usb.c
@@ -173,8 +173,9 @@ static int usbback_gnttab_map(struct usbback_req *usbback_req)
for (i = 0; i < usbback_req->nr_buffer_segs; i++) {
ref[i] = usbback_req->req.seg[i].gref;
}
- usbback_req->buffer = xengnttab_map_domain_grant_refs(xendev->gnttabdev,
- usbback_req->nr_buffer_segs, xendev->dom, ref, prot);
+ usbback_req->buffer =
+ xen_be_map_grant_refs(xendev, ref, usbback_req->nr_buffer_segs,
+ prot);

if (!usbback_req->buffer) {
return -ENOMEM;
@@ -206,8 +207,9 @@ static int usbback_gnttab_map(struct usbback_req *usbback_req)
for (i = 0; i < usbback_req->nr_extra_segs; i++) {
ref[i] = usbback_req->req.seg[i + usbback_req->req.nr_buffer_segs].gref;
}
- usbback_req->isoc_buffer = xengnttab_map_domain_grant_refs(
- xendev->gnttabdev, usbback_req->nr_extra_segs, xendev->dom, ref, prot);
+ usbback_req->isoc_buffer =
+ xen_be_map_grant_refs(xendev, ref, usbback_req->nr_extra_segs,
+ prot);

if (!usbback_req->isoc_buffer) {
return -ENOMEM;
@@ -291,14 +293,14 @@ static void usbback_do_response(struct usbback_req *usbback_req, int32_t status,
}

if (usbback_req->buffer) {
- xengnttab_unmap(xendev->gnttabdev, usbback_req->buffer,
- usbback_req->nr_buffer_segs);
+ xen_be_unmap_grant_refs(xendev, usbback_req->buffer,
+ usbback_req->nr_buffer_segs);
usbback_req->buffer = NULL;
}

if (usbback_req->isoc_buffer) {
- xengnttab_unmap(xendev->gnttabdev, usbback_req->isoc_buffer,
- usbback_req->nr_extra_segs);
+ xen_be_unmap_grant_refs(xendev, usbback_req->isoc_buffer,
+ usbback_req->nr_extra_segs);
usbback_req->isoc_buffer = NULL;
}

@@ -834,11 +836,11 @@ static void usbback_disconnect(struct XenDevice *xendev)
xen_pv_unbind_evtchn(xendev);

if (usbif->urb_sring) {
- xengnttab_unmap(xendev->gnttabdev, usbif->urb_sring, 1);
+ xen_be_unmap_grant_ref(xendev, usbif->urb_sring);
usbif->urb_sring = NULL;
}
if (usbif->conn_sring) {
- xengnttab_unmap(xendev->gnttabdev, usbif->conn_sring, 1);
+ xen_be_unmap_grant_ref(xendev, usbif->conn_sring);
usbif->conn_sring = NULL;
}

@@ -877,12 +879,10 @@ static int usbback_connect(struct XenDevice *xendev)
return -1;
}

- usbif->urb_sring = xengnttab_map_grant_ref(xendev->gnttabdev, xendev->dom,
- urb_ring_ref,
- PROT_READ | PROT_WRITE);
- usbif->conn_sring = xengnttab_map_grant_ref(xendev->gnttabdev, xendev->dom,
- conn_ring_ref,
- PROT_READ | PROT_WRITE);
+ usbif->urb_sring = xen_be_map_grant_ref(xendev, urb_ring_ref,
+ PROT_READ | PROT_WRITE);
+ usbif->conn_sring = xen_be_map_grant_ref(xendev, conn_ring_ref,
+ PROT_READ | PROT_WRITE);
if (!usbif->urb_sring || !usbif->conn_sring) {
xen_pv_printf(xendev, 0, "error mapping rings\n");
usbback_disconnect(xendev);
@@ -1024,10 +1024,7 @@ static void usbback_alloc(struct XenDevice *xendev)

/* max_grants: for each request and for the rings (request and connect). */
max_grants = USBIF_MAX_SEGMENTS_PER_REQUEST * USB_URB_RING_SIZE + 2;
- if (xengnttab_set_max_grants(xendev->gnttabdev, max_grants) < 0) {
- xen_pv_printf(xendev, 0, "xengnttab_set_max_grants failed: %s\n",
- strerror(errno));
- }
+ xen_be_set_max_grant_refs(xendev, max_grants);
}

static int usbback_free(struct XenDevice *xendev)
--
2.1.4
Paul Durrant
2018-05-16 14:22:59 UTC
Permalink
-----Original Message-----
Sent: 16 May 2018 15:14
Subject: Re: [PATCH v3 3/8] xen: remove other open-coded use of
libxengnttab
Post by Paul Durrant
Now that helpers are available in xen_backend, use them throughout all
Xen PV backends.
---
diff --git a/hw/net/xen_nic.c b/hw/net/xen_nic.c
index 20c43a6..73d6f1b 100644
--- a/hw/net/xen_nic.c
+++ b/hw/net/xen_nic.c
@@ -160,9 +160,8 @@ static void net_tx_packets(struct XenNetDev
*netdev)
Post by Paul Durrant
(txreq.flags & NETTXF_more_data) ? " more_data" : "",
(txreq.flags & NETTXF_extra_info) ? " extra_info" : "");
- page = xengnttab_map_grant_ref(netdev->xendev.gnttabdev,
- netdev->xendev.dom,
- txreq.gref, PROT_READ);
+ page = xen_be_map_grant_refs(&netdev->xendev,
+ &txreq.gref, 1, PROT_READ);
xen_be_map_grant_ref instead?
Yep.
Post by Paul Durrant
if (page == NULL) {
xen_pv_printf(&netdev->xendev, 0,
"error: tx gref dereference failed (%d)\n",
@@ -183,7 +182,7 @@ static void net_tx_packets(struct XenNetDev
*netdev)
Post by Paul Durrant
qemu_send_packet(qemu_get_queue(netdev->nic),
page + txreq.offset, txreq.size);
}
- xengnttab_unmap(netdev->xendev.gnttabdev, page, 1);
+ xen_be_unmap_grant_ref(&netdev->xendev, page);
net_tx_response(netdev, &txreq, NETIF_RSP_OKAY);
}
if (!netdev->tx_work) {
@@ -254,9 +253,8 @@ static ssize_t net_rx_packet(NetClientState *nc,
const uint8_t *buf, size_t size
Post by Paul Durrant
memcpy(&rxreq, RING_GET_REQUEST(&netdev->rx_ring, rc),
sizeof(rxreq));
Post by Paul Durrant
netdev->rx_ring.req_cons = ++rc;
- page = xengnttab_map_grant_ref(netdev->xendev.gnttabdev,
- netdev->xendev.dom,
- rxreq.gref, PROT_WRITE);
+ page = xen_be_map_grant_refs(&netdev->xendev, &rxreq.gref, 1,
+ PROT_WRITE);
xen_be_map_grant_ref instead?
And yep again.
Thanks :-)

Paul
--
Anthony PERARD
Paul Durrant
2018-05-04 19:26:07 UTC
Permalink
Certain functions in xen_disk are called with a pointer to xendev
(struct XenDevice *). They then use continer_of() to acces the surrounding
blkdev (struct XenBlkDev) but then in various places use &blkdev->xendev
when use of the original xendev pointer is shorter to express and clearly
equivalent.

This patch is a purely cosmetic patch which makes sure there is a xendev
pointer on stack for any function where the pointer is need on multiple
occasions modified those functions to use it consistently.

Signed-off-by: Paul Durrant <***@citrix.com>
---
Cc: Stefano Stabellini <***@kernel.org>
Cc: Anthony Perard <***@citrix.com>
Cc: Kevin Wolf <***@redhat.com>
Cc: Max Reitz <***@redhat.com>

v2:
- Re-based
---
hw/block/xen_disk.c | 90 +++++++++++++++++++++++++++--------------------------
1 file changed, 46 insertions(+), 44 deletions(-)

diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 28651c5..9fbc0cd 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -178,10 +178,11 @@ static void ioreq_release(struct ioreq *ioreq, bool finish)
static int ioreq_parse(struct ioreq *ioreq)
{
struct XenBlkDev *blkdev = ioreq->blkdev;
+ struct XenDevice *xendev = &blkdev->xendev;
size_t len;
int i;

- xen_pv_printf(&blkdev->xendev, 3,
+ xen_pv_printf(xendev, 3,
"op %d, nr %d, handle %d, id %" PRId64 ", sector %" PRId64 "\n",
ioreq->req.operation, ioreq->req.nr_segments,
ioreq->req.handle, ioreq->req.id, ioreq->req.sector_number);
@@ -199,28 +200,28 @@ static int ioreq_parse(struct ioreq *ioreq)
case BLKIF_OP_DISCARD:
return 0;
default:
- xen_pv_printf(&blkdev->xendev, 0, "error: unknown operation (%d)\n",
+ xen_pv_printf(xendev, 0, "error: unknown operation (%d)\n",
ioreq->req.operation);
goto err;
};

if (ioreq->req.operation != BLKIF_OP_READ && blkdev->mode[0] != 'w') {
- xen_pv_printf(&blkdev->xendev, 0, "error: write req for ro device\n");
+ xen_pv_printf(xendev, 0, "error: write req for ro device\n");
goto err;
}

ioreq->start = ioreq->req.sector_number * blkdev->file_blk;
for (i = 0; i < ioreq->req.nr_segments; i++) {
if (i == BLKIF_MAX_SEGMENTS_PER_REQUEST) {
- xen_pv_printf(&blkdev->xendev, 0, "error: nr_segments too big\n");
+ xen_pv_printf(xendev, 0, "error: nr_segments too big\n");
goto err;
}
if (ioreq->req.seg[i].first_sect > ioreq->req.seg[i].last_sect) {
- xen_pv_printf(&blkdev->xendev, 0, "error: first > last sector\n");
+ xen_pv_printf(xendev, 0, "error: first > last sector\n");
goto err;
}
if (ioreq->req.seg[i].last_sect * BLOCK_SIZE >= XC_PAGE_SIZE) {
- xen_pv_printf(&blkdev->xendev, 0, "error: page crossing\n");
+ xen_pv_printf(xendev, 0, "error: page crossing\n");
goto err;
}

@@ -228,7 +229,7 @@ static int ioreq_parse(struct ioreq *ioreq)
ioreq->size += len;
}
if (ioreq->start + ioreq->size > blkdev->file_size) {
- xen_pv_printf(&blkdev->xendev, 0, "error: access beyond end of file\n");
+ xen_pv_printf(xendev, 0, "error: access beyond end of file\n");
goto err;
}
return 0;
@@ -244,7 +245,7 @@ static int ioreq_grant_copy(struct ioreq *ioreq)
struct XenDevice *xendev = &blkdev->xendev;
XenGrantCopySegment segs[BLKIF_MAX_SEGMENTS_PER_REQUEST];
int i, count, rc;
- int64_t file_blk = ioreq->blkdev->file_blk;
+ int64_t file_blk = blkdev->file_blk;
bool to_domain = (ioreq->req.operation == BLKIF_OP_READ);
void *virt = ioreq->buf;

@@ -272,7 +273,7 @@ static int ioreq_grant_copy(struct ioreq *ioreq)
rc = xen_be_copy_grant_refs(xendev, to_domain, segs, count);

if (rc) {
- xen_pv_printf(&ioreq->blkdev->xendev, 0,
+ xen_pv_printf(xendev, 0,
"failed to copy data %d\n", rc);
ioreq->aio_errors++;
return -1;
@@ -287,11 +288,12 @@ static void qemu_aio_complete(void *opaque, int ret)
{
struct ioreq *ioreq = opaque;
struct XenBlkDev *blkdev = ioreq->blkdev;
+ struct XenDevice *xendev = &blkdev->xendev;

aio_context_acquire(blkdev->ctx);

if (ret != 0) {
- xen_pv_printf(&blkdev->xendev, 0, "%s I/O error\n",
+ xen_pv_printf(xendev, 0, "%s I/O error\n",
ioreq->req.operation == BLKIF_OP_READ ? "read" : "write");
ioreq->aio_errors++;
}
@@ -625,16 +627,17 @@ static void blk_alloc(struct XenDevice *xendev)

static void blk_parse_discard(struct XenBlkDev *blkdev)
{
+ struct XenDevice *xendev = &blkdev->xendev;
int enable;

blkdev->feature_discard = true;

- if (xenstore_read_be_int(&blkdev->xendev, "discard-enable", &enable) == 0) {
+ if (xenstore_read_be_int(xendev, "discard-enable", &enable) == 0) {
blkdev->feature_discard = !!enable;
}

if (blkdev->feature_discard) {
- xenstore_write_be_int(&blkdev->xendev, "feature-discard", 1);
+ xenstore_write_be_int(xendev, "feature-discard", 1);
}
}

@@ -649,7 +652,7 @@ static int blk_init(struct XenDevice *xendev)
/* read xenstore entries */
if (blkdev->params == NULL) {
char *h = NULL;
- blkdev->params = xenstore_read_be_str(&blkdev->xendev, "params");
+ blkdev->params = xenstore_read_be_str(xendev, "params");
if (blkdev->params != NULL) {
h = strchr(blkdev->params, ':');
}
@@ -669,18 +672,18 @@ static int blk_init(struct XenDevice *xendev)
blkdev->fileproto = "vpc";
}
if (blkdev->mode == NULL) {
- blkdev->mode = xenstore_read_be_str(&blkdev->xendev, "mode");
+ blkdev->mode = xenstore_read_be_str(xendev, "mode");
}
if (blkdev->type == NULL) {
- blkdev->type = xenstore_read_be_str(&blkdev->xendev, "type");
+ blkdev->type = xenstore_read_be_str(xendev, "type");
}
if (blkdev->dev == NULL) {
- blkdev->dev = xenstore_read_be_str(&blkdev->xendev, "dev");
+ blkdev->dev = xenstore_read_be_str(xendev, "dev");
}
if (blkdev->devtype == NULL) {
- blkdev->devtype = xenstore_read_be_str(&blkdev->xendev, "device-type");
+ blkdev->devtype = xenstore_read_be_str(xendev, "device-type");
}
- directiosafe = xenstore_read_be_str(&blkdev->xendev, "direct-io-safe");
+ directiosafe = xenstore_read_be_str(xendev, "direct-io-safe");
blkdev->directiosafe = (directiosafe && atoi(directiosafe));

/* do we have all we need? */
@@ -706,10 +709,10 @@ static int blk_init(struct XenDevice *xendev)
/* fill info
* blk_connect supplies sector-size and sectors
*/
- xenstore_write_be_int(&blkdev->xendev, "feature-flush-cache", 1);
- xenstore_write_be_int(&blkdev->xendev, "info", info);
+ xenstore_write_be_int(xendev, "feature-flush-cache", 1);
+ xenstore_write_be_int(xendev, "info", info);

- xenstore_write_be_int(&blkdev->xendev, "max-ring-page-order",
+ xenstore_write_be_int(xendev, "max-ring-page-order",
MAX_RING_PAGE_ORDER);

blk_parse_discard(blkdev);
@@ -761,7 +764,7 @@ static int blk_connect(struct XenDevice *xendev)
}

/* init qemu block driver */
- index = (blkdev->xendev.dev - 202 * 256) / 16;
+ index = (xendev->dev - 202 * 256) / 16;
blkdev->dinfo = drive_get(IF_XEN, 0, index);
if (!blkdev->dinfo) {
Error *local_err = NULL;
@@ -773,11 +776,11 @@ static int blk_connect(struct XenDevice *xendev)
}

/* setup via xenbus -> create new block driver instance */
- xen_pv_printf(&blkdev->xendev, 2, "create new bdrv (xenbus setup)\n");
+ xen_pv_printf(xendev, 2, "create new bdrv (xenbus setup)\n");
blkdev->blk = blk_new_open(blkdev->filename, NULL, options,
qflags, &local_err);
if (!blkdev->blk) {
- xen_pv_printf(&blkdev->xendev, 0, "error: %s\n",
+ xen_pv_printf(xendev, 0, "error: %s\n",
error_get_pretty(local_err));
error_free(local_err);
return -1;
@@ -785,11 +788,11 @@ static int blk_connect(struct XenDevice *xendev)
blk_set_enable_write_cache(blkdev->blk, !writethrough);
} else {
/* setup via qemu cmdline -> already setup for us */
- xen_pv_printf(&blkdev->xendev, 2,
+ xen_pv_printf(xendev, 2,
"get configured bdrv (cmdline setup)\n");
blkdev->blk = blk_by_legacy_dinfo(blkdev->dinfo);
if (blk_is_read_only(blkdev->blk) && !readonly) {
- xen_pv_printf(&blkdev->xendev, 0, "Unexpected read-only drive");
+ xen_pv_printf(xendev, 0, "Unexpected read-only drive");
blkdev->blk = NULL;
return -1;
}
@@ -802,7 +805,7 @@ static int blk_connect(struct XenDevice *xendev)
if (blkdev->file_size < 0) {
BlockDriverState *bs = blk_bs(blkdev->blk);
const char *drv_name = bs ? bdrv_get_format_name(bs) : NULL;
- xen_pv_printf(&blkdev->xendev, 1, "blk_getlength: %d (%s) | drv %s\n",
+ xen_pv_printf(xendev, 1, "blk_getlength: %d (%s) | drv %s\n",
(int)blkdev->file_size, strerror(-blkdev->file_size),
drv_name ?: "-");
blkdev->file_size = 0;
@@ -814,15 +817,15 @@ static int blk_connect(struct XenDevice *xendev)
blkdev->file_size, blkdev->file_size >> 20);

/* Fill in number of sector size and number of sectors */
- xenstore_write_be_int(&blkdev->xendev, "sector-size", blkdev->file_blk);
- xenstore_write_be_int64(&blkdev->xendev, "sectors",
+ xenstore_write_be_int(xendev, "sector-size", blkdev->file_blk);
+ xenstore_write_be_int64(xendev, "sectors",
blkdev->file_size / blkdev->file_blk);

- if (xenstore_read_fe_int(&blkdev->xendev, "ring-page-order",
+ if (xenstore_read_fe_int(xendev, "ring-page-order",
&order) == -1) {
blkdev->nr_ring_ref = 1;

- if (xenstore_read_fe_int(&blkdev->xendev, "ring-ref",
+ if (xenstore_read_fe_int(xendev, "ring-ref",
&ring_ref) == -1) {
return -1;
}
@@ -839,7 +842,7 @@ static int blk_connect(struct XenDevice *xendev)
return -1;
}

- if (xenstore_read_fe_int(&blkdev->xendev, key,
+ if (xenstore_read_fe_int(xendev, key,
&ring_ref) == -1) {
g_free(key);
return -1;
@@ -854,18 +857,18 @@ static int blk_connect(struct XenDevice *xendev)
return -1;
}

- if (xenstore_read_fe_int(&blkdev->xendev, "event-channel",
- &blkdev->xendev.remote_port) == -1) {
+ if (xenstore_read_fe_int(xendev, "event-channel",
+ &xendev->remote_port) == -1) {
return -1;
}

- if (!blkdev->xendev.protocol) {
+ if (!xendev->protocol) {
blkdev->protocol = BLKIF_PROTOCOL_NATIVE;
- } else if (strcmp(blkdev->xendev.protocol, XEN_IO_PROTO_ABI_NATIVE) == 0) {
+ } else if (strcmp(xendev->protocol, XEN_IO_PROTO_ABI_NATIVE) == 0) {
blkdev->protocol = BLKIF_PROTOCOL_NATIVE;
- } else if (strcmp(blkdev->xendev.protocol, XEN_IO_PROTO_ABI_X86_32) == 0) {
+ } else if (strcmp(xendev->protocol, XEN_IO_PROTO_ABI_X86_32) == 0) {
blkdev->protocol = BLKIF_PROTOCOL_X86_32;
- } else if (strcmp(blkdev->xendev.protocol, XEN_IO_PROTO_ABI_X86_64) == 0) {
+ } else if (strcmp(xendev->protocol, XEN_IO_PROTO_ABI_X86_64) == 0) {
blkdev->protocol = BLKIF_PROTOCOL_X86_64;
} else {
blkdev->protocol = BLKIF_PROTOCOL_NATIVE;
@@ -896,7 +899,6 @@ static int blk_connect(struct XenDevice *xendev)
max_grants = blkdev->nr_ring_ref;

xen_be_set_max_grant_refs(xendev, max_grants);
-
blkdev->sring = xen_be_map_grant_refs(xendev, blkdev->ring_ref,
blkdev->nr_ring_ref,
PROT_READ | PROT_WRITE);
@@ -929,12 +931,12 @@ static int blk_connect(struct XenDevice *xendev)

blk_set_aio_context(blkdev->blk, blkdev->ctx);

- xen_be_bind_evtchn(&blkdev->xendev);
+ xen_be_bind_evtchn(xendev);

- xen_pv_printf(&blkdev->xendev, 1, "ok: proto %s, nr-ring-ref %u, "
+ xen_pv_printf(xendev, 1, "ok: proto %s, nr-ring-ref %u, "
"remote port %d, local port %d\n",
- blkdev->xendev.protocol, blkdev->nr_ring_ref,
- blkdev->xendev.remote_port, blkdev->xendev.local_port);
+ xendev->protocol, blkdev->nr_ring_ref,
+ xendev->remote_port, xendev->local_port);
return 0;
}

@@ -952,7 +954,7 @@ static void blk_disconnect(struct XenDevice *xendev)
blk_unref(blkdev->blk);
blkdev->blk = NULL;
}
- xen_pv_unbind_evtchn(&blkdev->xendev);
+ xen_pv_unbind_evtchn(xendev);

aio_context_release(blkdev->ctx);
--
2.1.4
Paul Durrant
2018-05-04 19:26:04 UTC
Permalink
Now that the (native or emulated) xen_be_copy_grant_refs() helper is
always available, the xen_disk code can be significantly simplified by
removing direct use of grant map and unmap operations.

Signed-off-by: Paul Durrant <***@citrix.com>
---
Cc: Stefano Stabellini <***@kernel.org>
Cc: Anthony Perard <***@citrix.com>
Cc: Kevin Wolf <***@redhat.com>
Cc: Max Reitz <***@redhat.com>

v2:
- Squashed in separate patche removing persistent grant use
- Re-based
---
hw/block/xen_disk.c | 370 ++++------------------------------------------------
1 file changed, 25 insertions(+), 345 deletions(-)

diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 66ed2b7..28be8b6 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -36,27 +36,9 @@

/* ------------------------------------------------------------- */

-static int batch_maps = 0;
-
-/* ------------------------------------------------------------- */
-
#define BLOCK_SIZE 512
#define IOCB_COUNT (BLKIF_MAX_SEGMENTS_PER_REQUEST + 2)

-struct PersistentGrant {
- void *page;
- struct XenBlkDev *blkdev;
-};
-
-typedef struct PersistentGrant PersistentGrant;
-
-struct PersistentRegion {
- void *addr;
- int num;
-};
-
-typedef struct PersistentRegion PersistentRegion;
-
struct ioreq {
blkif_request_t req;
int16_t status;
@@ -65,14 +47,11 @@ struct ioreq {
off_t start;
QEMUIOVector v;
int presync;
- uint8_t mapped;

/* grant mapping */
uint32_t refs[BLKIF_MAX_SEGMENTS_PER_REQUEST];
- int prot;
void *page[BLKIF_MAX_SEGMENTS_PER_REQUEST];
void *pages;
- int num_unmap;

/* aio status */
int aio_inflight;
@@ -103,7 +82,6 @@ struct XenBlkDev {
int protocol;
blkif_back_rings_t rings;
int more_work;
- int cnt_map;

/* request lists */
QLIST_HEAD(inflight_head, ioreq) inflight;
@@ -114,13 +92,7 @@ struct XenBlkDev {
int requests_finished;
unsigned int max_requests;

- /* Persistent grants extension */
gboolean feature_discard;
- gboolean feature_persistent;
- GTree *persistent_gnts;
- GSList *persistent_regions;
- unsigned int persistent_gnt_count;
- unsigned int max_grants;

/* qemu block driver */
DriveInfo *dinfo;
@@ -139,10 +111,8 @@ static void ioreq_reset(struct ioreq *ioreq)
ioreq->status = 0;
ioreq->start = 0;
ioreq->presync = 0;
- ioreq->mapped = 0;

memset(ioreq->refs, 0, sizeof(ioreq->refs));
- ioreq->prot = 0;
memset(ioreq->page, 0, sizeof(ioreq->page));
ioreq->pages = NULL;

@@ -156,37 +126,6 @@ static void ioreq_reset(struct ioreq *ioreq)
qemu_iovec_reset(&ioreq->v);
}

-static gint int_cmp(gconstpointer a, gconstpointer b, gpointer user_data)
-{
- uint ua = GPOINTER_TO_UINT(a);
- uint ub = GPOINTER_TO_UINT(b);
- return (ua > ub) - (ua < ub);
-}
-
-static void destroy_grant(gpointer pgnt)
-{
- PersistentGrant *grant = pgnt;
- struct XenBlkDev *blkdev = grant->blkdev;
- struct XenDevice *xendev = &blkdev->xendev;
-
- xen_be_unmap_grant_ref(xendev, grant->page);
- grant->blkdev->persistent_gnt_count--;
- xen_pv_printf(xendev, 3, "unmapped grant %p\n", grant->page);
- g_free(grant);
-}
-
-static void remove_persistent_region(gpointer data, gpointer dev)
-{
- PersistentRegion *region = data;
- struct XenBlkDev *blkdev = dev;
- struct XenDevice *xendev = &blkdev->xendev;
-
- xen_be_unmap_grant_refs(xendev, region->addr, region->num);
- xen_pv_printf(xendev, 3, "unmapped grant region %p with %d pages\n",
- region->addr, region->num);
- g_free(region);
-}
-
static struct ioreq *ioreq_start(struct XenBlkDev *blkdev)
{
struct ioreq *ioreq = NULL;
@@ -254,7 +193,6 @@ static int ioreq_parse(struct ioreq *ioreq)
ioreq->req.handle, ioreq->req.id, ioreq->req.sector_number);
switch (ioreq->req.operation) {
case BLKIF_OP_READ:
- ioreq->prot = PROT_WRITE; /* to memory */
break;
case BLKIF_OP_FLUSH_DISKCACHE:
ioreq->presync = 1;
@@ -263,7 +201,6 @@ static int ioreq_parse(struct ioreq *ioreq)
}
/* fall through */
case BLKIF_OP_WRITE:
- ioreq->prot = PROT_READ; /* from memory */
break;
case BLKIF_OP_DISCARD:
return 0;
@@ -310,173 +247,6 @@ err:
return -1;
}

-static void ioreq_unmap(struct ioreq *ioreq)
-{
- struct XenBlkDev *blkdev = ioreq->blkdev;
- struct XenDevice *xendev = &blkdev->xendev;
- int i;
-
- if (ioreq->num_unmap == 0 || ioreq->mapped == 0) {
- return;
- }
- if (batch_maps) {
- if (!ioreq->pages) {
- return;
- }
- xen_be_unmap_grant_refs(xendev, ioreq->pages, ioreq->num_unmap);
- ioreq->blkdev->cnt_map -= ioreq->num_unmap;
- ioreq->pages = NULL;
- } else {
- for (i = 0; i < ioreq->num_unmap; i++) {
- if (!ioreq->page[i]) {
- continue;
- }
- xen_be_unmap_grant_ref(xendev, ioreq->page[i]);
- ioreq->blkdev->cnt_map--;
- ioreq->page[i] = NULL;
- }
- }
- ioreq->mapped = 0;
-}
-
-static int ioreq_map(struct ioreq *ioreq)
-{
- struct XenBlkDev *blkdev = ioreq->blkdev;
- struct XenDevice *xendev = &blkdev->xendev;
- uint32_t refs[BLKIF_MAX_SEGMENTS_PER_REQUEST];
- void *page[BLKIF_MAX_SEGMENTS_PER_REQUEST];
- int i, j, new_maps = 0;
- PersistentGrant *grant;
- PersistentRegion *region;
- /* refs variable will contain the information necessary
- * to map the grants that are needed to fulfill this request.
- *
- * After mapping the needed grants, the page array will contain the
- * memory address of each granted page in the order specified in ioreq
- * (disregarding if it's a persistent grant or not).
- */
-
- if (ioreq->v.niov == 0 || ioreq->mapped == 1) {
- return 0;
- }
- if (ioreq->blkdev->feature_persistent) {
- for (i = 0; i < ioreq->v.niov; i++) {
- grant = g_tree_lookup(ioreq->blkdev->persistent_gnts,
- GUINT_TO_POINTER(ioreq->refs[i]));
-
- if (grant != NULL) {
- page[i] = grant->page;
- xen_pv_printf(&ioreq->blkdev->xendev, 3,
- "using persistent-grant %" PRIu32 "\n",
- ioreq->refs[i]);
- } else {
- /* Add the grant to the list of grants that
- * should be mapped
- */
- refs[new_maps] = ioreq->refs[i];
- page[i] = NULL;
- new_maps++;
- }
- }
- /* Set the protection to RW, since grants may be reused later
- * with a different protection than the one needed for this request
- */
- ioreq->prot = PROT_WRITE | PROT_READ;
- } else {
- /* All grants in the request should be mapped */
- memcpy(refs, ioreq->refs, sizeof(refs));
- memset(page, 0, sizeof(page));
- new_maps = ioreq->v.niov;
- }
-
- if (batch_maps && new_maps) {
- ioreq->pages = xen_be_map_grant_refs(xendev, refs, new_maps,
- ioreq->prot);
- if (ioreq->pages == NULL) {
- xen_pv_printf(&ioreq->blkdev->xendev, 0,
- "can't map %d grant refs (%s, %d maps)\n",
- new_maps, strerror(errno), ioreq->blkdev->cnt_map);
- return -1;
- }
- for (i = 0, j = 0; i < ioreq->v.niov; i++) {
- if (page[i] == NULL) {
- page[i] = ioreq->pages + (j++) * XC_PAGE_SIZE;
- }
- }
- ioreq->blkdev->cnt_map += new_maps;
- } else if (new_maps) {
- for (i = 0; i < new_maps; i++) {
- ioreq->page[i] = xen_be_map_grant_ref(xendev, refs[i],
- ioreq->prot);
- if (ioreq->page[i] == NULL) {
- xen_pv_printf(&ioreq->blkdev->xendev, 0,
- "can't map grant ref %d (%s, %d maps)\n",
- refs[i], strerror(errno), ioreq->blkdev->cnt_map);
- ioreq->mapped = 1;
- ioreq_unmap(ioreq);
- return -1;
- }
- ioreq->blkdev->cnt_map++;
- }
- for (i = 0, j = 0; i < ioreq->v.niov; i++) {
- if (page[i] == NULL) {
- page[i] = ioreq->page[j++];
- }
- }
- }
- if (ioreq->blkdev->feature_persistent && new_maps != 0 &&
- (!batch_maps || (ioreq->blkdev->persistent_gnt_count + new_maps <=
- ioreq->blkdev->max_grants))) {
- /*
- * If we are using persistent grants and batch mappings only
- * add the new maps to the list of persistent grants if the whole
- * area can be persistently mapped.
- */
- if (batch_maps) {
- region = g_malloc0(sizeof(*region));
- region->addr = ioreq->pages;
- region->num = new_maps;
- ioreq->blkdev->persistent_regions = g_slist_append(
- ioreq->blkdev->persistent_regions,
- region);
- }
- while ((ioreq->blkdev->persistent_gnt_count < ioreq->blkdev->max_grants)
- && new_maps) {
- /* Go through the list of newly mapped grants and add as many
- * as possible to the list of persistently mapped grants.
- *
- * Since we start at the end of ioreq->page(s), we only need
- * to decrease new_maps to prevent this granted pages from
- * being unmapped in ioreq_unmap.
- */
- grant = g_malloc0(sizeof(*grant));
- new_maps--;
- if (batch_maps) {
- grant->page = ioreq->pages + (new_maps) * XC_PAGE_SIZE;
- } else {
- grant->page = ioreq->page[new_maps];
- }
- grant->blkdev = ioreq->blkdev;
- xen_pv_printf(&ioreq->blkdev->xendev, 3,
- "adding grant %" PRIu32 " page: %p\n",
- refs[new_maps], grant->page);
- g_tree_insert(ioreq->blkdev->persistent_gnts,
- GUINT_TO_POINTER(refs[new_maps]),
- grant);
- ioreq->blkdev->persistent_gnt_count++;
- }
- assert(!batch_maps || new_maps == 0);
- }
- for (i = 0; i < ioreq->v.niov; i++) {
- ioreq->v.iov[i].iov_base += (uintptr_t)page[i];
- }
- ioreq->mapped = 1;
- ioreq->num_unmap = new_maps;
- return 0;
-}
-
-#if CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40800
-
static void ioreq_free_copy_buffers(struct ioreq *ioreq)
{
int i;
@@ -546,22 +316,6 @@ static int ioreq_grant_copy(struct ioreq *ioreq)

return rc;
}
-#else
-static void ioreq_free_copy_buffers(struct ioreq *ioreq)
-{
- abort();
-}
-
-static int ioreq_init_copy_buffers(struct ioreq *ioreq)
-{
- abort();
-}
-
-static int ioreq_grant_copy(struct ioreq *ioreq)
-{
- abort();
-}
-#endif

static int ioreq_runio_qemu_aio(struct ioreq *ioreq);

@@ -588,32 +342,28 @@ static void qemu_aio_complete(void *opaque, int ret)
goto done;
}

- if (xen_feature_grant_copy) {
- switch (ioreq->req.operation) {
- case BLKIF_OP_READ:
- /* in case of failure ioreq->aio_errors is increased */
- if (ret == 0) {
- ioreq_grant_copy(ioreq);
- }
- ioreq_free_copy_buffers(ioreq);
- break;
- case BLKIF_OP_WRITE:
- case BLKIF_OP_FLUSH_DISKCACHE:
- if (!ioreq->req.nr_segments) {
- break;
- }
- ioreq_free_copy_buffers(ioreq);
- break;
- default:
+ switch (ioreq->req.operation) {
+ case BLKIF_OP_READ:
+ /* in case of failure ioreq->aio_errors is increased */
+ if (ret == 0) {
+ ioreq_grant_copy(ioreq);
+ }
+ ioreq_free_copy_buffers(ioreq);
+ break;
+ case BLKIF_OP_WRITE:
+ case BLKIF_OP_FLUSH_DISKCACHE:
+ if (!ioreq->req.nr_segments) {
break;
}
+ ioreq_free_copy_buffers(ioreq);
+ break;
+ default:
+ break;
}

ioreq->status = ioreq->aio_errors ? BLKIF_RSP_ERROR : BLKIF_RSP_OKAY;
- if (!xen_feature_grant_copy) {
- ioreq_unmap(ioreq);
- }
ioreq_finish(ioreq);
+
switch (ioreq->req.operation) {
case BLKIF_OP_WRITE:
case BLKIF_OP_FLUSH_DISKCACHE:
@@ -673,18 +423,13 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
{
struct XenBlkDev *blkdev = ioreq->blkdev;

- if (xen_feature_grant_copy) {
- ioreq_init_copy_buffers(ioreq);
- if (ioreq->req.nr_segments && (ioreq->req.operation == BLKIF_OP_WRITE ||
- ioreq->req.operation == BLKIF_OP_FLUSH_DISKCACHE) &&
- ioreq_grant_copy(ioreq)) {
- ioreq_free_copy_buffers(ioreq);
- goto err;
- }
- } else {
- if (ioreq->req.nr_segments && ioreq_map(ioreq)) {
- goto err;
- }
+ ioreq_init_copy_buffers(ioreq);
+ if (ioreq->req.nr_segments &&
+ (ioreq->req.operation == BLKIF_OP_WRITE ||
+ ioreq->req.operation == BLKIF_OP_FLUSH_DISKCACHE) &&
+ ioreq_grant_copy(ioreq)) {
+ ioreq_free_copy_buffers(ioreq);
+ goto err;
}

ioreq->aio_inflight++;
@@ -725,9 +470,6 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
}
default:
/* unknown operation (shouldn't happen -- parse catches this) */
- if (!xen_feature_grant_copy) {
- ioreq_unmap(ioreq);
- }
goto err;
}

@@ -913,10 +655,6 @@ static void blk_alloc(struct XenDevice *xendev)

blkdev->ctx = iothread_get_aio_context(blkdev->iothread);
blkdev->bh = aio_bh_new(blkdev->ctx, blk_bh, blkdev);
-
- if (xen_mode != XEN_EMULATE) {
- batch_maps = 1;
- }
}

static void blk_parse_discard(struct XenBlkDev *blkdev)
@@ -999,15 +737,10 @@ static int blk_init(struct XenDevice *xendev)

blkdev->file_blk = BLOCK_SIZE;

- xen_pv_printf(&blkdev->xendev, 3, "grant copy operation %s\n",
- xen_feature_grant_copy ? "enabled" : "disabled");
-
/* fill info
* blk_connect supplies sector-size and sectors
*/
xenstore_write_be_int(&blkdev->xendev, "feature-flush-cache", 1);
- xenstore_write_be_int(&blkdev->xendev, "feature-persistent",
- !xen_feature_grant_copy);
xenstore_write_be_int(&blkdev->xendev, "info", info);

xenstore_write_be_int(&blkdev->xendev, "max-ring-page-order",
@@ -1034,19 +767,10 @@ out_error:
return -1;
}

-/*
- * We need to account for the grant allocations requiring contiguous
- * chunks; the worst case number would be
- * max_req * max_seg + (max_req - 1) * (max_seg - 1) + 1,
- * but in order to keep things simple just use
- * 2 * max_req * max_seg.
- */
-#define MAX_GRANTS(max_req, max_seg) (2 * (max_req) * (max_seg))
-
static int blk_connect(struct XenDevice *xendev)
{
struct XenBlkDev *blkdev = container_of(xendev, struct XenBlkDev, xendev);
- int pers, index, qflags;
+ int index, qflags;
bool readonly = true;
bool writethrough = true;
int order, ring_ref;
@@ -1168,11 +892,6 @@ static int blk_connect(struct XenDevice *xendev)
&blkdev->xendev.remote_port) == -1) {
return -1;
}
- if (xenstore_read_fe_int(&blkdev->xendev, "feature-persistent", &pers)) {
- blkdev->feature_persistent = FALSE;
- } else {
- blkdev->feature_persistent = !!pers;
- }

if (!blkdev->xendev.protocol) {
blkdev->protocol = BLKIF_PROTOCOL_NATIVE;
@@ -1207,11 +926,8 @@ static int blk_connect(struct XenDevice *xendev)
return -1;
}

- /* Calculate the maximum number of grants needed by ioreqs */
- max_grants = MAX_GRANTS(blkdev->max_requests,
- BLKIF_MAX_SEGMENTS_PER_REQUEST);
/* Add on the number needed for the ring pages */
- max_grants += blkdev->nr_ring_ref;
+ max_grants = blkdev->nr_ring_ref;

xen_be_set_max_grant_refs(xendev, max_grants);

@@ -1222,8 +938,6 @@ static int blk_connect(struct XenDevice *xendev)
return -1;
}

- blkdev->cnt_map++;
-
switch (blkdev->protocol) {
case BLKIF_PROTOCOL_NATIVE:
{
@@ -1247,19 +961,6 @@ static int blk_connect(struct XenDevice *xendev)
}
}

- if (blkdev->feature_persistent) {
- /* Init persistent grants */
- blkdev->max_grants = blkdev->max_requests *
- BLKIF_MAX_SEGMENTS_PER_REQUEST;
- blkdev->persistent_gnts = g_tree_new_full((GCompareDataFunc)int_cmp,
- NULL, NULL,
- batch_maps ?
- (GDestroyNotify)g_free :
- (GDestroyNotify)destroy_grant);
- blkdev->persistent_regions = NULL;
- blkdev->persistent_gnt_count = 0;
- }
-
blk_set_aio_context(blkdev->blk, blkdev->ctx);

xen_be_bind_evtchn(&blkdev->xendev);
@@ -1292,29 +993,8 @@ static void blk_disconnect(struct XenDevice *xendev)
if (blkdev->sring) {
xen_be_unmap_grant_refs(xendev, blkdev->sring,
blkdev->nr_ring_ref);
- blkdev->cnt_map--;
blkdev->sring = NULL;
}
-
- /*
- * Unmap persistent grants before switching to the closed state
- * so the frontend can free them.
- *
- * In the !batch_maps case g_tree_destroy will take care of unmapping
- * the grant, but in the batch_maps case we need to iterate over every
- * region in persistent_regions and unmap it.
- */
- if (blkdev->feature_persistent) {
- g_tree_destroy(blkdev->persistent_gnts);
- assert(batch_maps || blkdev->persistent_gnt_count == 0);
- if (batch_maps) {
- blkdev->persistent_gnt_count = 0;
- g_slist_foreach(blkdev->persistent_regions,
- (GFunc)remove_persistent_region, blkdev);
- g_slist_free(blkdev->persistent_regions);
- }
- blkdev->feature_persistent = false;
- }
}

static int blk_free(struct XenDevice *xendev)
--
2.1.4
Loading...