From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42279) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXAv4-0007HN-R0 for qemu-devel@nongnu.org; Tue, 09 Aug 2016 13:34:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bXAuy-0004az-LF for qemu-devel@nongnu.org; Tue, 09 Aug 2016 13:34:25 -0400 Received: from mail-wm0-x244.google.com ([2a00:1450:400c:c09::244]:36767) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXAuy-0004aP-9L for qemu-devel@nongnu.org; Tue, 09 Aug 2016 13:34:20 -0400 Received: by mail-wm0-x244.google.com with SMTP id i138so4558077wmf.3 for ; Tue, 09 Aug 2016 10:34:19 -0700 (PDT) References: <1470146790-6168-1-git-send-email-paulinaszubarczyk@gmail.com> <1470146790-6168-3-git-send-email-paulinaszubarczyk@gmail.com> <20160809165602.GD1835@perard.uk.xensource.com> From: Paulina Szubarczyk Message-ID: <57AA1416.6020307@gmail.com> Date: Tue, 9 Aug 2016 19:34:14 +0200 MIME-Version: 1.0 In-Reply-To: <20160809165602.GD1835@perard.uk.xensource.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 2/2] qdisk - hw/block/xen_disk: grant copy implementation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony PERARD Cc: xen-devel@lists.xenproject.org, roger.pau@citrix.com, wei.liu2@citrix.com, ian.jackson@eu.citrix.com, david.vrabel@citrix.com, sstabellini@kernel.org, qemu-devel@nongnu.org On 08/09/2016 06:56 PM, Anthony PERARD wrote: > On Tue, Aug 02, 2016 at 04:06:30PM +0200, Paulina Szubarczyk wrote: >> Copy data operated on during request from/to local buffers to/from >> the grant references. >> >> Before grant copy operation local buffers must be allocated what is >> done by calling ioreq_init_copy_buffers. For the 'read' operation, >> first, the qemu device invokes the read operation on local buffers >> and on the completion grant copy is called and buffers are freed. >> For the 'write' operation grant copy is performed before invoking >> write by qemu device. >> >> A new value 'feature_grant_copy' is added to recognize when the >> grant copy operation is supported by a guest. >> >> Signed-off-by: Paulina Szubarczyk >> --- >> Changes since v3: >> - qemu_memalign/qemu_free is used instead function allocating >> memory from xc. >> - removed the get_buffer function instead there is a direct call >> to qemu_memalign. >> - moved ioreq_copy for write operation to ioreq_runio_qemu_aio. >> - added struct xengnttab_grant_copy_segment_t and stub in >> xen_common.h for version of xen earlier then 480. >> - added checking for version 480 to configure. The test repeats >> all the operation that are required for version < 480 and >> checks if xengnttab_grant_copy() is implemented. >> >> * I did not change the way of testing if grant_copy operation is >> implemented. As far as I understand if the code from >> gnttab_unimp.c is used then the gnttab device is unavailable >> and the handler to gntdev would be invalid. But if the handler >> is valid then the ioctl should return operation unimplemented >> if the gntdev does not implement the operation. >> --- >> configure | 56 +++++++++++++++++ >> hw/block/xen_disk.c | 142 ++++++++++++++++++++++++++++++++++++++++++-- >> include/hw/xen/xen_common.h | 25 ++++++++ >> 3 files changed, 218 insertions(+), 5 deletions(-) >> >> diff --git a/configure b/configure >> index f57fcc6..b5bf7d4 100755 >> --- a/configure >> +++ b/configure >> @@ -1956,6 +1956,62 @@ EOF >> /* >> * If we have stable libs the we don't want the libxc compat >> * layers, regardless of what CFLAGS we may have been given. >> + * >> + * Also, check if xengnttab_grant_copy_segment_t is defined and >> + * grant copy operation is implemented. >> + */ >> +#undef XC_WANT_COMPAT_EVTCHN_API >> +#undef XC_WANT_COMPAT_GNTTAB_API >> +#undef XC_WANT_COMPAT_MAP_FOREIGN_API >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#if !defined(HVM_MAX_VCPUS) >> +# error HVM_MAX_VCPUS not defined >> +#endif >> +int main(void) { >> + xc_interface *xc = NULL; >> + xenforeignmemory_handle *xfmem; >> + xenevtchn_handle *xe; >> + xengnttab_handle *xg; >> + xen_domain_handle_t handle; >> + xengnttab_grant_copy_segment_t* seg = NULL; >> + >> + xs_daemon_open(); >> + >> + xc = xc_interface_open(0, 0, 0); >> + xc_hvm_set_mem_type(0, 0, HVMMEM_ram_ro, 0, 0); >> + xc_domain_add_to_physmap(0, 0, XENMAPSPACE_gmfn, 0, 0); >> + xc_hvm_inject_msi(xc, 0, 0xf0000000, 0x00000000); >> + xc_hvm_create_ioreq_server(xc, 0, HVM_IOREQSRV_BUFIOREQ_ATOMIC, NULL); >> + xc_domain_create(xc, 0, handle, 0, NULL, NULL); >> + >> + xfmem = xenforeignmemory_open(0, 0); >> + xenforeignmemory_map(xfmem, 0, 0, 0, 0, 0); >> + >> + xe = xenevtchn_open(0, 0); >> + xenevtchn_fd(xe); >> + >> + xg = xengnttab_open(0, 0); >> + xengnttab_map_grant_ref(xg, 0, 0, 0); >> + xengnttab_grant_copy(xg, 0, seg); >> + >> + return 0; >> +} >> +EOF >> + compile_prog "" "$xen_libs $xen_stable_libs" >> + then >> + xen_ctrl_version=480 >> + xen=yes >> + elif >> + cat > $TMPC <> +/* >> + * If we have stable libs the we don't want the libxc compat >> + * layers, regardless of what CFLAGS we may have been given. >> */ >> #undef XC_WANT_COMPAT_EVTCHN_API >> #undef XC_WANT_COMPAT_GNTTAB_API >> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c >> index 3b8ad33..2dd1464 100644 >> --- a/hw/block/xen_disk.c >> +++ b/hw/block/xen_disk.c >> @@ -119,6 +119,9 @@ struct XenBlkDev { >> unsigned int persistent_gnt_count; >> unsigned int max_grants; >> >> + /* Grant copy */ >> + gboolean feature_grant_copy; >> + >> /* qemu block driver */ >> DriveInfo *dinfo; >> BlockBackend *blk; >> @@ -489,6 +492,95 @@ static int ioreq_map(struct ioreq *ioreq) >> return 0; >> } >> >> +static void free_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); >> + if (!ioreq->pages) { > > qemu_memalign never returns NULL, you don't have to check. > >> + return -1; >> + } >> + >> + 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_copy(struct ioreq *ioreq) >> +{ >> + xengnttab_handle *gnt = ioreq->blkdev->xendev.gnttabdev; >> + xengnttab_grant_copy_segment_t segs[BLKIF_MAX_SEGMENTS_PER_REQUEST]; >> + int i, count = 0, r, rc; >> + int64_t file_blk = ioreq->blkdev->file_blk; >> + >> + if (ioreq->v.niov == 0) { >> + return 0; >> + } >> + >> + count = ioreq->v.niov; >> + >> + for (i = 0; i < count; i++) { >> + >> + if (ioreq->req.operation == BLKIF_OP_READ) { >> + segs[i].flags = GNTCOPY_dest_gref; >> + 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; >> + } >> + segs[i].len = (ioreq->req.seg[i].last_sect >> + - ioreq->req.seg[i].first_sect + 1) * file_blk; >> + >> + } >> + >> + rc = xengnttab_grant_copy(gnt, count, segs); >> + >> + if (rc) { >> + xen_be_printf(&ioreq->blkdev->xendev, 0, >> + "failed to copy data %d\n", rc); >> + ioreq->aio_errors++; >> + return -1; >> + } else { >> + r = 0; >> + } >> + >> + for (i = 0; i < count; i++) { >> + if (segs[i].status != GNTST_okay) { >> + xen_be_printf(&ioreq->blkdev->xendev, 3, >> + "failed to copy data %d for gref %d, domid %d\n", rc, >> + ioreq->refs[i], ioreq->domids[i]); >> + ioreq->aio_errors++; >> + r = -1; >> + } >> + } >> + >> + return r; >> +} >> + >> static int ioreq_runio_qemu_aio(struct ioreq *ioreq); >> >> static void qemu_aio_complete(void *opaque, int ret) >> @@ -511,8 +603,29 @@ static void qemu_aio_complete(void *opaque, int ret) >> return; >> } >> >> + if (ioreq->blkdev->feature_grant_copy) { >> + switch (ioreq->req.operation) { >> + case BLKIF_OP_READ: >> + /* in case of failure ioreq->aio_errors is increased */ >> + ioreq_copy(ioreq); >> + free_buffers(ioreq); >> + break; >> + case BLKIF_OP_WRITE: >> + case BLKIF_OP_FLUSH_DISKCACHE: >> + if (!ioreq->req.nr_segments) { >> + break; >> + } >> + free_buffers(ioreq); >> + break; >> + default: >> + break; >> + } >> + } >> + >> ioreq->status = ioreq->aio_errors ? BLKIF_RSP_ERROR : BLKIF_RSP_OKAY; >> - ioreq_unmap(ioreq); >> + if (!ioreq->blkdev->feature_grant_copy) { >> + ioreq_unmap(ioreq); >> + } >> ioreq_finish(ioreq); >> switch (ioreq->req.operation) { >> case BLKIF_OP_WRITE: >> @@ -538,8 +651,20 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq) >> { >> struct XenBlkDev *blkdev = ioreq->blkdev; >> >> - if (ioreq->req.nr_segments && ioreq_map(ioreq) == -1) { >> - goto err_no_map; >> + if (ioreq->blkdev->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)) { >> + if (ioreq_copy(ioreq)) { >> + free_buffers(ioreq); >> + goto err; >> + } >> + } >> + >> + } else { >> + if (ioreq->req.nr_segments && ioreq_map(ioreq)) { >> + goto err; >> + } >> } >> >> ioreq->aio_inflight++; >> @@ -582,6 +707,9 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq) >> } >> default: >> /* unknown operation (shouldn't happen -- parse catches this) */ >> + if (!ioreq->blkdev->feature_grant_copy) { >> + ioreq_unmap(ioreq); >> + } >> goto err; >> } >> >> @@ -590,8 +718,6 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq) >> return 0; >> >> err: >> - ioreq_unmap(ioreq); >> -err_no_map: >> ioreq_finish(ioreq); >> ioreq->status = BLKIF_RSP_ERROR; >> return -1; >> @@ -1032,6 +1158,12 @@ static int blk_connect(struct XenDevice *xendev) >> >> xen_be_bind_evtchn(&blkdev->xendev); >> >> + blkdev->feature_grant_copy = >> + (xengnttab_grant_copy(blkdev->xendev.gnttabdev, 0, NULL) == 0); >> + >> + xen_be_printf(&blkdev->xendev, 3, "grant copy operation %s\n", >> + blkdev->feature_grant_copy ? "enabled" : "disabled"); >> + >> xen_be_printf(&blkdev->xendev, 1, "ok: proto %s, ring-ref %d, " >> "remote port %d, local port %d\n", >> blkdev->xendev.protocol, blkdev->ring_ref, >> diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h >> index 640c31e..e80c61f 100644 >> --- a/include/hw/xen/xen_common.h >> +++ b/include/hw/xen/xen_common.h >> @@ -25,6 +25,31 @@ >> */ >> >> /* Xen 4.2 through 4.6 */ >> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 480 >> + >> +struct xengnttab_grant_copy_segment { >> + union xengnttab_copy_ptr { >> + void *virt; >> + struct { >> + uint32_t ref; >> + uint16_t offset; >> + uint16_t domid; >> + } foreign; >> + } source, dest; >> + uint16_t len; >> + uint16_t flags; >> + int16_t status; >> +}; > > I don't think it's a good idee to define a struct that is not going to > be used, and does not belong here. The typedef is OK. I added it since it is needed to know all the fields of that struct in ioreq_copy but if I could replace that function with stubs I will do that. > > In xen_disk.c, you could use "#if CONFIG_XEN_CTRL_INTERFACE_VERSION ..." > around free_buffers, ioreq_init_copy_buffers and ioreq_copy, and replace > them by stubs when Xen does not support grant copy. I think that would > be better. > > Also, could you try to compile again xen unstable, without your other patch? > Right now, it does not compile. That is true, I am sorry. I need to add the headers that are included in the #else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 471 */ #include #include #include #endif May I move that part to separate #if CONFIG_XEN_CTRL_INTERFACE_VERSION >= 471 in that header? > >> +typedef struct xengnttab_grant_copy_segment xengnttab_grant_copy_segment_t; >> + >> +static inline int xengnttab_grant_copy(xengnttab_handle *xgt, uint32_t count, >> + xengnttab_grant_copy_segment_t *segs) >> +{ >> + return -1; > > return -ENOSYS would be more appropriate. > > > Otherwise, the patch looks good. > > Thanks, > Thank you, Paulina