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

[Paulina Szubarczyk <paulinaszubarczyk@gmail.com>]

On 07/15/2016 07:11 PM, Anthony PERARD wrote:
> 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.
>
I will add the stub but the structure xengnttab_grant_copy_segment need 
to be repeated in the xen_common.h again, it is also not defined in the 
earlier versions.

Paulina

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