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

From: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
To: Wei Liu <wei.liu2@citrix.com>
Cc: sstabellini@kernel.org, ian.jackson@eu.citrix.com,
	david.vrabel@citrix.com, anthony.perard@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 12:28:48 +0200	[thread overview]
Message-ID: <5788BAE0.9080607@gmail.com> (raw)
In-Reply-To: <20160714103752.GQ31770@citrix.com>

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.

> 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.

Paulina

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