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

From: Anthony PERARD <anthony.perard@citrix.com>
To: Wei Liu <wei.liu2@citrix.com>
Cc: sstabellini@kernel.org, ian.jackson@eu.citrix.com,
	Paulina Szubarczyk <paulinaszubarczyk@gmail.com>,
	david.vrabel@citrix.com, xen-devel@lists.xenproject.org,
	roger.pau@citrix.com
Subject: Re: [PATCH v3 2/2] qdisk - hw/block/xen_disk: grant copy implementation
Date: Fri, 15 Jul 2016 18:11:56 +0100	[thread overview]
Message-ID: <20160715171156.GO1729@perard.uk.xensource.com> (raw)
In-Reply-To: <20160715111545.GA21149@citrix.com>

On Fri, Jul 15, 2016 at 12:15:45PM +0100, Wei Liu wrote:
> On Fri, Jul 15, 2016 at 12:28:48PM +0200, Paulina Szubarczyk wrote:
> > 
> > 
> > On 07/14/2016 12:37 PM, Wei Liu wrote:
> > >On Wed, Jun 22, 2016 at 10:38:53AM +0200, Paulina Szubarczyk wrote:
> > >>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.
> > >
> > Do I understand correctly that I should add a new variable
> > "xen_stable_libs"? I could not find it in the qemu tree used anywhere else.
> > 
> 
> Hmm... there is already one in upstream QEMU -- which means you're
> perhaps using qemu-xen tree.
> 
> I think all new development should happen on upstream qemu, not in our
> qemu-xen tree.
> 
> > >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.
> > >
> > I will add that.
> > 
> > >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.
> > 
> > There is a variable CONFIG_XEN_CTRL_INTERFACE_VERSION holding current
> > version of the Xen control library this qemu is configured with. It is set
> > from the configure file. The feature could be guarded with ifdef by a new
> > variable CONFIG_XEN_LIBS_INTERFACE_VERSION or they could be unified to
> > CONFIG_XEN_TOOLS_INTERFACE_VERSION to not fill the same value twice.
> > 
> 
> Another way is to provide a stub for this function to always return 0.
> 
> Please wait for Stefano and Anthony to see which method they prefer.

I think using CONFIG_XEN_CTRL_INTERFACE_VERSION is fine. With maybe a
stub of xengnttab_grant_copy() in xen_common.h.

-- 
Anthony PERARD

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