Re: [Qemu-devel] [PATCH v4 2/2] qdisk - hw/block/xen_disk: grant copy implementation

[Paulina Szubarczyk <paulinaszubarczyk@gmail.com>]

On 08/02/2016 04:06 PM, 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 <paulinaszubarczyk@gmail.com>
> ---
> 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(-)

>       /* qemu block driver */
>       DriveInfo           *dinfo;
>       BlockBackend        *blk;
> @@ -489,6 +492,95 @@ static int ioreq_map(struct ioreq *ioreq)
>       return 0;
>   }

>   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);

I would add a condition to invoke the grant copy only if the ret 
argument with which the callback from BlockBackend 
'qemu_aio_complete(void *opaque, int ret)' is called is equal to 0
to not unnecessary copy invalid data.

> +            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:

Paulina