Re: [PATCH v3 2/2] qdisk - hw/block/xen_disk: grant copy implementation

[Wei Liu <wei.liu2@citrix.com>]

On Wed, Jun 22, 2016 at 10:38:53AM +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.
> The body of the function 'ioreq_runio_qemu_aio' is moved to
> 'ioreq_runio_qemu_aio_blk' and in the 'ioreq_runio_qemu_aio' depending
> on the support for grant copy according checks, initialization, grant
> operation are made, then the 'ioreq_runio_qemu_aio_blk' function is
> called.
> 
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
> ---
> Changes since v2:
> - to use the xengnttab_* function directly added -lxengnttab to configure
>   and include <xengnttab.h> in include/hw/xen/xen_common.h
> - in ioreq_copy removed an out path, changed a log level, made explicit 
>   assignement to 'xengnttab_copy_grant_segment'
> * 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                   |   2 +-
>  hw/block/xen_disk.c         | 171 ++++++++++++++++++++++++++++++++++++++++----
>  include/hw/xen/xen_common.h |   2 +
>  3 files changed, 162 insertions(+), 13 deletions(-)
> 
> diff --git a/configure b/configure
> index e41876a..355d3fa 100755
> --- a/configure
> +++ b/configure
> @@ -1843,7 +1843,7 @@ fi
>  # xen probe
>  
>  if test "$xen" != "no" ; then
> -  xen_libs="-lxenstore -lxenctrl -lxenguest"
> +  xen_libs="-lxenstore -lxenctrl -lxenguest -lxengnttab"
>  

First thing, -lxengnttab should be in xen_stable_libs.

The probing needs to be more sophisticated.

You need to probe the new function your added as well. Just a few lines
below xen_stable_libs there is a section for hand-coded probing source
code, which you would need to modify.

Assuming your gnttab change will be merged into 4.8 (the release under
development at the moment), you need to have a separate program for it.

After you've done proper probing, you will know which version of Xen
this qemu is compiling against.  And then, there should be some fallback
mechanism to compile and run this qemu with older version of xen. This
is not too hard because you can guard your code with feature flag or
ifdef (please consult Stefan and Anthony which method to use).

Feel free to ask questions. I will try my best to explain.

>  
> +    blkdev->feature_grant_copy =
> +                (xengnttab_grant_copy(blkdev->xendev.gnttabdev, 0, NULL) == 0);

This is a bit problematic. As this patch stands, it won't compile on
older version of Xen because there is no such function there.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel