| Message ID | 20170117190009.GA16720@char.us.oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@citrix.com> >>>> 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
> > 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. Roger did build it (FreeBSD uses it), but I don't think he tried the test-cases. Wait, I think he may have as his patch: f8c66c2ad2efdb281e4ebf15bf329d73c4f02ce7 Author: Roger Pau Monne <roger.pau@citrix.com> Date: Tue May 3 12:55:09 2016 +0200 xen/xsplice: add ELFOSABI_FREEBSD as a supported OSABI for payloads implies that he did build an payload (and I remember us trying to figure out if the issues he had been hitting were due to LTO or something else).
On Tue, Jan 17, 2017 at 03:46:30PM -0500, Konrad Rzeszutek Wilk wrote: > > > 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. > > Roger did build it (FreeBSD uses it), but I don't think he tried > the test-cases. I've tried livepatch, but not LTO, neither on it's own. > Wait, I think he may have as his patch: f8c66c2ad2efdb281e4ebf15bf329d73c4f02ce7 > Author: Roger Pau Monne <roger.pau@citrix.com> > Date: Tue May 3 12:55:09 2016 +0200 > > xen/xsplice: add ELFOSABI_FREEBSD as a supported OSABI for payloads > > implies that he did build an payload (and I remember us trying to figure > out if the issues he had been hitting were due to LTO or something else). IIRC issues where related to not translating error codes from the return of the livepatch hypercalls, and the patch upload tool refusing to upload patches with ELFOSABI_FREEBSD instead of ELFOSABI_NONE (which I assume is what Linux uses). It's been sometime since I've tried livepatch with FreeBSD, so I could give it a spin if you think there's a chance things might have broken. Roger.
On Wed, Jan 18, 2017 at 10:13:34AM +0000, Roger Pau Monné wrote: > On Tue, Jan 17, 2017 at 03:46:30PM -0500, Konrad Rzeszutek Wilk wrote: > > > > 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. > > > > Roger did build it (FreeBSD uses it), but I don't think he tried > > the test-cases. > > I've tried livepatch, but not LTO, neither on it's own. Gotcha. > > > Wait, I think he may have as his patch: f8c66c2ad2efdb281e4ebf15bf329d73c4f02ce7 > > Author: Roger Pau Monne <roger.pau@citrix.com> > > Date: Tue May 3 12:55:09 2016 +0200 > > > > xen/xsplice: add ELFOSABI_FREEBSD as a supported OSABI for payloads > > > > implies that he did build an payload (and I remember us trying to figure > > out if the issues he had been hitting were due to LTO or something else). > > IIRC issues where related to not translating error codes from the return of the > livepatch hypercalls, and the patch upload tool refusing to upload patches with > ELFOSABI_FREEBSD instead of ELFOSABI_NONE (which I assume is what Linux uses). > > It's been sometime since I've tried livepatch with FreeBSD, so I could give it > a spin if you think there's a chance things might have broken. That is OK. I think I can also try myself to build Xen with LTO on Linux and see how the test-cases fare. Not sure what is involved but I will find out. And to answer Andrew's question - No, nobody tried LTO with livepatching then :-( > > Roger.
On Wed, Jan 18, 2017 at 11:10:40AM -0500, Konrad Rzeszutek Wilk wrote: > On Wed, Jan 18, 2017 at 10:13:34AM +0000, Roger Pau Monné wrote: > > On Tue, Jan 17, 2017 at 03:46:30PM -0500, Konrad Rzeszutek Wilk wrote: > > > > > 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. > > > > > > Roger did build it (FreeBSD uses it), but I don't think he tried > > > the test-cases. > > > > I've tried livepatch, but not LTO, neither on it's own. > > Gotcha. > > > > > Wait, I think he may have as his patch: f8c66c2ad2efdb281e4ebf15bf329d73c4f02ce7 > > > Author: Roger Pau Monne <roger.pau@citrix.com> > > > Date: Tue May 3 12:55:09 2016 +0200 > > > > > > xen/xsplice: add ELFOSABI_FREEBSD as a supported OSABI for payloads > > > > > > implies that he did build an payload (and I remember us trying to figure > > > out if the issues he had been hitting were due to LTO or something else). > > > > IIRC issues where related to not translating error codes from the return of the > > livepatch hypercalls, and the patch upload tool refusing to upload patches with > > ELFOSABI_FREEBSD instead of ELFOSABI_NONE (which I assume is what Linux uses). > > > > It's been sometime since I've tried livepatch with FreeBSD, so I could give it > > a spin if you think there's a chance things might have broken. > > That is OK. > > I think I can also try myself to build Xen with LTO on Linux and see how > the test-cases fare. > > Not sure what is involved but I will find out. > > And to answer Andrew's question - No, nobody tried LTO with livepatching then :-( TBH, I think nobody has tried LTO on it's own in a long time, much less with livepatch. Adding Wei who recently sent some LTO makefile fixes, maybe he tried it. Roger.
On Wed, Jan 18, 2017 at 04:21:06PM +0000, Roger Pau Monné wrote: > On Wed, Jan 18, 2017 at 11:10:40AM -0500, Konrad Rzeszutek Wilk wrote: > > On Wed, Jan 18, 2017 at 10:13:34AM +0000, Roger Pau Monné wrote: > > > On Tue, Jan 17, 2017 at 03:46:30PM -0500, Konrad Rzeszutek Wilk wrote: > > > > > > 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. > > > > > > > > Roger did build it (FreeBSD uses it), but I don't think he tried > > > > the test-cases. > > > > > > I've tried livepatch, but not LTO, neither on it's own. > > > > Gotcha. > > > > > > > Wait, I think he may have as his patch: f8c66c2ad2efdb281e4ebf15bf329d73c4f02ce7 > > > > Author: Roger Pau Monne <roger.pau@citrix.com> > > > > Date: Tue May 3 12:55:09 2016 +0200 > > > > > > > > xen/xsplice: add ELFOSABI_FREEBSD as a supported OSABI for payloads > > > > > > > > implies that he did build an payload (and I remember us trying to figure > > > > out if the issues he had been hitting were due to LTO or something else). > > > > > > IIRC issues where related to not translating error codes from the return of the > > > livepatch hypercalls, and the patch upload tool refusing to upload patches with > > > ELFOSABI_FREEBSD instead of ELFOSABI_NONE (which I assume is what Linux uses). > > > > > > It's been sometime since I've tried livepatch with FreeBSD, so I could give it > > > a spin if you think there's a chance things might have broken. > > > > That is OK. > > > > I think I can also try myself to build Xen with LTO on Linux and see how > > the test-cases fare. > > > > Not sure what is involved but I will find out. > > > > And to answer Andrew's question - No, nobody tried LTO with livepatching then :-( > > TBH, I think nobody has tried LTO on it's own in a long time, much less with > livepatch. Adding Wei who recently sent some LTO makefile fixes, maybe he tried > it. > LTO is now controlled by CONFIG_LTO in kconfig. Xen build system is broken when LTO is enabled. Furthermore GCC toolchain can't handle LTO yet. Konrad, feel free to fix LTO build. :-) Wei. > Roger.
On 18/01/17 16:30, Wei Liu wrote: > On Wed, Jan 18, 2017 at 04:21:06PM +0000, Roger Pau Monné wrote: >> On Wed, Jan 18, 2017 at 11:10:40AM -0500, Konrad Rzeszutek Wilk wrote: >>> On Wed, Jan 18, 2017 at 10:13:34AM +0000, Roger Pau Monné wrote: >>>> On Tue, Jan 17, 2017 at 03:46:30PM -0500, Konrad Rzeszutek Wilk wrote: >>>>>>> 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. >>>>> Roger did build it (FreeBSD uses it), but I don't think he tried >>>>> the test-cases. >>>> I've tried livepatch, but not LTO, neither on it's own. >>> Gotcha. >>>>> Wait, I think he may have as his patch: f8c66c2ad2efdb281e4ebf15bf329d73c4f02ce7 >>>>> Author: Roger Pau Monne <roger.pau@citrix.com> >>>>> Date: Tue May 3 12:55:09 2016 +0200 >>>>> >>>>> xen/xsplice: add ELFOSABI_FREEBSD as a supported OSABI for payloads >>>>> >>>>> implies that he did build an payload (and I remember us trying to figure >>>>> out if the issues he had been hitting were due to LTO or something else). >>>> IIRC issues where related to not translating error codes from the return of the >>>> livepatch hypercalls, and the patch upload tool refusing to upload patches with >>>> ELFOSABI_FREEBSD instead of ELFOSABI_NONE (which I assume is what Linux uses). >>>> >>>> It's been sometime since I've tried livepatch with FreeBSD, so I could give it >>>> a spin if you think there's a chance things might have broken. >>> That is OK. >>> >>> I think I can also try myself to build Xen with LTO on Linux and see how >>> the test-cases fare. >>> >>> Not sure what is involved but I will find out. >>> >>> And to answer Andrew's question - No, nobody tried LTO with livepatching then :-( >> TBH, I think nobody has tried LTO on it's own in a long time, much less with >> livepatch. Adding Wei who recently sent some LTO makefile fixes, maybe he tried >> it. >> > LTO is now controlled by CONFIG_LTO in kconfig. > > Xen build system is broken when LTO is enabled. Furthermore GCC > toolchain can't handle LTO yet. > > Konrad, feel free to fix LTO build. :-) Last time I tried LTO: * The GCC build did work, but the net binary was bigger rather than smaller, and it successfully boot * The Clang build worked, made a much smaller binary, but due to a clang bug, "optimised" code into using SSE, and blew up spectacularly when booting. From experiments with XTF, Clang 3.9 LTO is far more comprehensive, and is basically capable of optimising entire XTF tests into one single test function, and properly respects the "no FPU ever" options used during boot. ~Andrew
On 1/18/17 12:04 PM, Andrew Cooper wrote: > Last time I tried LTO: > > * The GCC build did work, but the net binary was bigger rather than > smaller, and it successfully boot > * The Clang build worked, made a much smaller binary, but due to a clang > bug, "optimised" code into using SSE, and blew up spectacularly when > booting. > > From experiments with XTF, Clang 3.9 LTO is far more comprehensive, and > is basically capable of optimising entire XTF tests into one single test > function, and properly respects the "no FPU ever" options used during boot. > > ~Andrew FWIW, some of the clang issues I reported against LTO and FPU bits are marked as fixed in 3.9 and 4.0. Not sure if things got backported.
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; }