Re: [Xen-devel] [PATCH] xen/common: Drop function calls for Xen compile/version information

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 17/01/17 19:00, Konrad Rzeszutek Wilk wrote:
> On Tue, Jan 17, 2017 at 01:42:54PM -0500, Konrad Rzeszutek Wilk wrote:
>> On Tue, Jan 17, 2017 at 06:16:36PM +0000, Andrew Cooper wrote:
>>> On 17/01/17 18:05, Konrad Rzeszutek Wilk wrote:
>>>> On Mon, Jan 16, 2017 at 01:04:09PM +0000, Andrew Cooper wrote:
>>>>> The chageset/version/compile information is currently exported as a set of
>>>>> function calls into a separate translation unit, which is inefficient for 
>>>>> all
>>>>> callers.
>>>>>
>>>>> Replace the function calls with externs pointing appropriately into 
>>>>> .rodata,
>>>>> which allows all users to generate code referencing the data directly.
>>>>>
>>>>> No functional change, but causes smaller and more efficient compiled code.
>>>>>
>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>> Ah crud. That breaks the livepatch test-cases (they patch the 
>>>> xen_extra_version
>>>> function).
>>> Lucky I haven't pushed it then, (although the livepatch build seems to
>>> still work fine for me, despite this change.)
>> make tests should fail.
> (which is not by built by default as requested by Jan - but the
> OSSTest test cases I am working would do this).
>>>> Are there some other code that can be modified that is reported
>>>> by 'xl info' on which the test-cases can run (and reported easily?).
>>> Patch do_version() itself to return the same difference of information?
>> Ugh. That is going to make the building of test-cases quite complex.
>>
>> I guess it can just do it and .. return only one value :-)
> As in something like this (not compile tested):
>
>
> diff --git a/xen/arch/x86/test/xen_hello_world_func.c 
> b/xen/arch/x86/test/xen_hello_world_func.c
> index 2e4af9c..3572600 100644
> --- a/xen/arch/x86/test/xen_hello_world_func.c
> +++ b/xen/arch/x86/test/xen_hello_world_func.c
> @@ -10,7 +10,7 @@
>  static unsigned long *non_canonical_addr = (unsigned long 
> *)0xdead000000000000ULL;
>  
>  /* Our replacement function for xen_extra_version. */
> -const char *xen_hello_world(void)
> +const char *xen_version_hello_world(int cmd, XEN_GUEST_HANDLE_PARAM(void) 
> arg)
>  {
>      unsigned long tmp;
>      int rc;
> @@ -21,7 +21,19 @@ const char *xen_hello_world(void)
>      rc = __get_user(tmp, non_canonical_addr);
>      BUG_ON(rc != -EFAULT);
>  
> -    return "Hello World";
> +    if ( cmd == XENVER_extraversion )
> +    {
> +        xen_extraversion_t extraversion = "Hello World";
> +
> +        if ( copy_to_guest(arg, extraversion, ARRAY_SIZE(extraversion)) )
> +            return -EFAULT;
> +        return 0;
> +    }
> +    /*
> +     * Can't return -EPERM as certain subversions can't deal with negative
> +     * values.
> +     */
> +    return 0;
>  }
>
>
> That will make three of the test-patches work but not for the last
> one - the NOP one (which needs to patch the _whole_ function).
>
> Argh.
>
> Is this patch of yours that neccessary? Could at least some of the
> functions still exist?

Well.  This patch is manually doing what LTO would do automatically when
it has a cross-translation-unit view of things, and come to the
conclusion that in all cases, inlining cross-unit will make the calling
code shorter, and allow the functions to be elided entirely.

I take it from this that noone has tried livepatching an LTO build of Xen.

~Andrew

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