| Message ID | 20191002112004.25793-6-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add hypervisor sysfs-like support | expand |
On 02.10.2019 13:20, Juergen Gross wrote: > Add the /buildinfo/config entry to the hypervisor filesystem. This > entry contains the .config file used to build the hypervisor. I think this is the 2nd step ahead of the 1st: Much of the stuff exposed as XENVER_* sub-ops should manifest itself here ahead of exposing xen/.config. > @@ -79,3 +80,11 @@ subdir-$(CONFIG_UBSAN) += ubsan > > subdir-$(CONFIG_NEEDS_LIBELF) += libelf > subdir-$(CONFIG_HAS_DEVICE_TREE) += libfdt > + > +config_data.c: ../.config > + ( echo "char xen_config_data[] ="; \ > + ../tools/bin2c <$<; \ > + echo ";" ) > $@ This is the typical kind of construct that may break (a subsequent build attempt) when interrupted in the middle. This pretty clearly is a move-if-changed candidate, at the same time also avoiding a (cheap, but anyway) pointless re-build in case .config was touched without actually changing. Furthermore is there a reason to expose this as plain text, when Linux exposes a gzip-ed version in /proc? The file isn't very large now, but this was also the case for Linux many years ago. > --- a/xen/common/hypfs.c > +++ b/xen/common/hypfs.c > @@ -25,6 +25,10 @@ static struct hypfs_entry hypfs_root_entry = { > .dir = &hypfs_root, > }; > > +static struct hypfs_dir hypfs_buildinfo = { > + .list = LIST_HEAD_INIT(hypfs_buildinfo.list), > +}; > + > static int hypfs_add_entry(struct hypfs_dir *parent, struct hypfs_entry *new) > { > int ret = -ENOENT; > @@ -316,3 +320,16 @@ long do_hypfs_op(unsigned int cmd, > > return ret; > } > + > +static int __init hypfs_init(void) > +{ > + int ret; > + > + ret = hypfs_new_dir(&hypfs_root, "buildinfo", &hypfs_buildinfo); > + BUG_ON(ret); > + ret = hypfs_new_entry_string(&hypfs_buildinfo, "config", xen_config_data); > + BUG_ON(ret); > + > + return 0; > +} > +__initcall(hypfs_init); Hmm, do you really want to centralize population of the file system here, rather than having the individual components take care of it? > --- a/xen/tools/Makefile > +++ b/xen/tools/Makefile > @@ -1,13 +1,18 @@ > > include $(XEN_ROOT)/Config.mk > > +PROGS = symbols bin2c > + > .PHONY: default > default: > - $(MAKE) symbols > + $(MAKE) $(PROGS) Could I ask you to take the opportunity and do away with the unnecessary (as it seems to me) make recursion? $(PROGS) could easily become a dependency of "default" afaict. Jan
On 12.11.19 15:22, Jan Beulich wrote: > On 02.10.2019 13:20, Juergen Gross wrote: >> Add the /buildinfo/config entry to the hypervisor filesystem. This >> entry contains the .config file used to build the hypervisor. > > I think this is the 2nd step ahead of the 1st: Much of the stuff > exposed as XENVER_* sub-ops should manifest itself here ahead of > exposing xen/.config. Yes and no. This is meant as a replacement for my previous patch series adding .config read support. It is no problem to add other data as well, but the need for being able to read .config contents was already agreed on. > >> @@ -79,3 +80,11 @@ subdir-$(CONFIG_UBSAN) += ubsan >> >> subdir-$(CONFIG_NEEDS_LIBELF) += libelf >> subdir-$(CONFIG_HAS_DEVICE_TREE) += libfdt >> + >> +config_data.c: ../.config >> + ( echo "char xen_config_data[] ="; \ >> + ../tools/bin2c <$<; \ >> + echo ";" ) > $@ > > This is the typical kind of construct that may break (a subsequent > build attempt) when interrupted in the middle. This pretty clearly > is a move-if-changed candidate, at the same time also avoiding a > (cheap, but anyway) pointless re-build in case .config was touched > without actually changing. Okay. > > Furthermore is there a reason to expose this as plain text, when > Linux exposes a gzip-ed version in /proc? The file isn't very > large now, but this was also the case for Linux many years ago. gzip data may contain bytes with 0x00. Supporting that would require a different interface at all levels. > >> --- a/xen/common/hypfs.c >> +++ b/xen/common/hypfs.c >> @@ -25,6 +25,10 @@ static struct hypfs_entry hypfs_root_entry = { >> .dir = &hypfs_root, >> }; >> >> +static struct hypfs_dir hypfs_buildinfo = { >> + .list = LIST_HEAD_INIT(hypfs_buildinfo.list), >> +}; >> + >> static int hypfs_add_entry(struct hypfs_dir *parent, struct hypfs_entry *new) >> { >> int ret = -ENOENT; >> @@ -316,3 +320,16 @@ long do_hypfs_op(unsigned int cmd, >> >> return ret; >> } >> + >> +static int __init hypfs_init(void) >> +{ >> + int ret; >> + >> + ret = hypfs_new_dir(&hypfs_root, "buildinfo", &hypfs_buildinfo); >> + BUG_ON(ret); >> + ret = hypfs_new_entry_string(&hypfs_buildinfo, "config", xen_config_data); >> + BUG_ON(ret); >> + >> + return 0; >> +} >> +__initcall(hypfs_init); > > Hmm, do you really want to centralize population of the file system > here, rather than having the individual components take care of it? I can add a new source, e.g. common/buildinfo.c if you like that better. > >> --- a/xen/tools/Makefile >> +++ b/xen/tools/Makefile >> @@ -1,13 +1,18 @@ >> >> include $(XEN_ROOT)/Config.mk >> >> +PROGS = symbols bin2c >> + >> .PHONY: default >> default: >> - $(MAKE) symbols >> + $(MAKE) $(PROGS) > > Could I ask you to take the opportunity and do away with the > unnecessary (as it seems to me) make recursion? $(PROGS) could > easily become a dependency of "default" afaict. Fine with me. Juergen
On 12.11.2019 17:45, Jürgen Groß wrote: > On 12.11.19 15:22, Jan Beulich wrote: >> On 02.10.2019 13:20, Juergen Gross wrote: >>> @@ -79,3 +80,11 @@ subdir-$(CONFIG_UBSAN) += ubsan >>> >>> subdir-$(CONFIG_NEEDS_LIBELF) += libelf >>> subdir-$(CONFIG_HAS_DEVICE_TREE) += libfdt >>> + >>> +config_data.c: ../.config >>> + ( echo "char xen_config_data[] ="; \ >>> + ../tools/bin2c <$<; \ >>> + echo ";" ) > $@ >> >> Furthermore is there a reason to expose this as plain text, when >> Linux exposes a gzip-ed version in /proc? The file isn't very >> large now, but this was also the case for Linux many years ago. > > gzip data may contain bytes with 0x00. Supporting that would require a > different interface at all levels. Then perhaps better do so now, when the code is still in flux, than after the fact, especially if "at all levels" is meant to also include the public interface? >>> --- a/xen/common/hypfs.c >>> +++ b/xen/common/hypfs.c >>> @@ -25,6 +25,10 @@ static struct hypfs_entry hypfs_root_entry = { >>> .dir = &hypfs_root, >>> }; >>> >>> +static struct hypfs_dir hypfs_buildinfo = { >>> + .list = LIST_HEAD_INIT(hypfs_buildinfo.list), >>> +}; >>> + >>> static int hypfs_add_entry(struct hypfs_dir *parent, struct hypfs_entry *new) >>> { >>> int ret = -ENOENT; >>> @@ -316,3 +320,16 @@ long do_hypfs_op(unsigned int cmd, >>> >>> return ret; >>> } >>> + >>> +static int __init hypfs_init(void) >>> +{ >>> + int ret; >>> + >>> + ret = hypfs_new_dir(&hypfs_root, "buildinfo", &hypfs_buildinfo); >>> + BUG_ON(ret); >>> + ret = hypfs_new_entry_string(&hypfs_buildinfo, "config", xen_config_data); >>> + BUG_ON(ret); >>> + >>> + return 0; >>> +} >>> +__initcall(hypfs_init); >> >> Hmm, do you really want to centralize population of the file system >> here, rather than having the individual components take care of it? > > I can add a new source, e.g. common/buildinfo.c if you like that better. I was rather thinking of moving this into common/kernel.c, next to the version hypercall handling, and together with exposing the suggested values here ahead of exposing .config. Jan
On 13.11.19 15:12, Jan Beulich wrote: > On 12.11.2019 17:45, Jürgen Groß wrote: >> On 12.11.19 15:22, Jan Beulich wrote: >>> On 02.10.2019 13:20, Juergen Gross wrote: >>>> @@ -79,3 +80,11 @@ subdir-$(CONFIG_UBSAN) += ubsan >>>> >>>> subdir-$(CONFIG_NEEDS_LIBELF) += libelf >>>> subdir-$(CONFIG_HAS_DEVICE_TREE) += libfdt >>>> + >>>> +config_data.c: ../.config >>>> + ( echo "char xen_config_data[] ="; \ >>>> + ../tools/bin2c <$<; \ >>>> + echo ";" ) > $@ >>> >>> Furthermore is there a reason to expose this as plain text, when >>> Linux exposes a gzip-ed version in /proc? The file isn't very >>> large now, but this was also the case for Linux many years ago. >> >> gzip data may contain bytes with 0x00. Supporting that would require a >> different interface at all levels. > > Then perhaps better do so now, when the code is still in flux, than > after the fact, especially if "at all levels" is meant to also > include the public interface? I'll have a look into that. > >>>> --- a/xen/common/hypfs.c >>>> +++ b/xen/common/hypfs.c >>>> @@ -25,6 +25,10 @@ static struct hypfs_entry hypfs_root_entry = { >>>> .dir = &hypfs_root, >>>> }; >>>> >>>> +static struct hypfs_dir hypfs_buildinfo = { >>>> + .list = LIST_HEAD_INIT(hypfs_buildinfo.list), >>>> +}; >>>> + >>>> static int hypfs_add_entry(struct hypfs_dir *parent, struct hypfs_entry *new) >>>> { >>>> int ret = -ENOENT; >>>> @@ -316,3 +320,16 @@ long do_hypfs_op(unsigned int cmd, >>>> >>>> return ret; >>>> } >>>> + >>>> +static int __init hypfs_init(void) >>>> +{ >>>> + int ret; >>>> + >>>> + ret = hypfs_new_dir(&hypfs_root, "buildinfo", &hypfs_buildinfo); >>>> + BUG_ON(ret); >>>> + ret = hypfs_new_entry_string(&hypfs_buildinfo, "config", xen_config_data); >>>> + BUG_ON(ret); >>>> + >>>> + return 0; >>>> +} >>>> +__initcall(hypfs_init); >>> >>> Hmm, do you really want to centralize population of the file system >>> here, rather than having the individual components take care of it? >> >> I can add a new source, e.g. common/buildinfo.c if you like that better. > > I was rather thinking of moving this into common/kernel.c, next to the > version hypercall handling, and together with exposing the suggested > values here ahead of exposing .config. Okay. Juergen
diff --git a/.gitignore b/.gitignore index 954c1da2cb..16dfbb8302 100644 --- a/.gitignore +++ b/.gitignore @@ -295,6 +295,7 @@ xen/arch/*/efi/boot.c xen/arch/*/efi/compat.c xen/arch/*/efi/efi.h xen/arch/*/efi/runtime.c +xen/common/config_data.c xen/include/headers*.chk xen/include/asm xen/include/asm-*/asm-offsets.h @@ -312,6 +313,7 @@ xen/test/livepatch/xen_bye_world.livepatch xen/test/livepatch/xen_hello_world.livepatch xen/test/livepatch/xen_nop.livepatch xen/test/livepatch/xen_replace_world.livepatch +xen/tools/bin2c xen/tools/kconfig/.tmp_gtkcheck xen/tools/kconfig/.tmp_qtcheck xen/tools/symbols diff --git a/docs/misc/hypfs-paths.pandoc b/docs/misc/hypfs-paths.pandoc index 67de8d2cf8..81353546ef 100644 --- a/docs/misc/hypfs-paths.pandoc +++ b/docs/misc/hypfs-paths.pandoc @@ -93,3 +93,12 @@ A populated Xen hypervisor file system might look like the following example: #### / The root of the hypervisor file system. + +#### /buildinfo/ + +A directory containing static information generated while building the +hypervisor. + +#### /buildinfo/config = STRING + +The contents of the `xen/.config` file at the time of the hypervisor build. diff --git a/xen/common/Makefile b/xen/common/Makefile index a3f66aa0c0..de7e0fa645 100644 --- a/xen/common/Makefile +++ b/xen/common/Makefile @@ -1,6 +1,7 @@ obj-$(CONFIG_ARGO) += argo.o obj-y += bitmap.o obj-y += bsearch.o +obj-y += config_data.o obj-$(CONFIG_CORE_PARKING) += core_parking.o obj-y += cpu.o obj-y += cpupool.o @@ -79,3 +80,11 @@ subdir-$(CONFIG_UBSAN) += ubsan subdir-$(CONFIG_NEEDS_LIBELF) += libelf subdir-$(CONFIG_HAS_DEVICE_TREE) += libfdt + +config_data.c: ../.config + ( echo "char xen_config_data[] ="; \ + ../tools/bin2c <$<; \ + echo ";" ) > $@ + +clean:: + rm config_data.c 2>/dev/null || true diff --git a/xen/common/hypfs.c b/xen/common/hypfs.c index c8c5c504cf..ab17d0de49 100644 --- a/xen/common/hypfs.c +++ b/xen/common/hypfs.c @@ -25,6 +25,10 @@ static struct hypfs_entry hypfs_root_entry = { .dir = &hypfs_root, }; +static struct hypfs_dir hypfs_buildinfo = { + .list = LIST_HEAD_INIT(hypfs_buildinfo.list), +}; + static int hypfs_add_entry(struct hypfs_dir *parent, struct hypfs_entry *new) { int ret = -ENOENT; @@ -316,3 +320,16 @@ long do_hypfs_op(unsigned int cmd, return ret; } + +static int __init hypfs_init(void) +{ + int ret; + + ret = hypfs_new_dir(&hypfs_root, "buildinfo", &hypfs_buildinfo); + BUG_ON(ret); + ret = hypfs_new_entry_string(&hypfs_buildinfo, "config", xen_config_data); + BUG_ON(ret); + + return 0; +} +__initcall(hypfs_init); diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h index 548b64da9f..5ff2280b0f 100644 --- a/xen/include/xen/kernel.h +++ b/xen/include/xen/kernel.h @@ -100,5 +100,7 @@ extern enum system_state { bool_t is_active_kernel_text(unsigned long addr); +extern char xen_config_data[]; + #endif /* _LINUX_KERNEL_H */ diff --git a/xen/tools/Makefile b/xen/tools/Makefile index e940939d61..cd2bbbf647 100644 --- a/xen/tools/Makefile +++ b/xen/tools/Makefile @@ -1,13 +1,18 @@ include $(XEN_ROOT)/Config.mk +PROGS = symbols bin2c + .PHONY: default default: - $(MAKE) symbols + $(MAKE) $(PROGS) .PHONY: clean clean: - rm -f *.o symbols + rm -f *.o $(PROGS) symbols: symbols.c $(HOSTCC) $(HOSTCFLAGS) -o $@ $< + +bin2c: bin2c.c + $(HOSTCC) $(HOSTCFLAGS) -o $@ $< diff --git a/xen/tools/bin2c.c b/xen/tools/bin2c.c new file mode 100644 index 0000000000..c332399b70 --- /dev/null +++ b/xen/tools/bin2c.c @@ -0,0 +1,28 @@ +/* + * Unloved program to convert a binary on stdin to a C include on stdout + * + * Jan 1999 Matt Mackall <mpm@selenic.com> + * + * This software may be used and distributed according to the terms + * of the GNU General Public License, incorporated herein by reference. + */ + +#include <stdio.h> + +int main(int argc, char *argv[]) +{ + int ch, total = 0; + + do { + printf("\t\""); + while ((ch = getchar()) != EOF) { + total++; + printf("\\x%02x", ch); + if (total % 16 == 0) + break; + } + printf("\"\n"); + } while (ch != EOF); + + return 0; +}
Add the /buildinfo/config entry to the hypervisor filesystem. This entry contains the .config file used to build the hypervisor. Signed-off-by: Juergen Gross <jgross@suse.com> --- .gitignore | 2 ++ docs/misc/hypfs-paths.pandoc | 9 +++++++++ xen/common/Makefile | 9 +++++++++ xen/common/hypfs.c | 17 +++++++++++++++++ xen/include/xen/kernel.h | 2 ++ xen/tools/Makefile | 9 +++++++-- xen/tools/bin2c.c | 28 ++++++++++++++++++++++++++++ 7 files changed, 74 insertions(+), 2 deletions(-) create mode 100644 xen/tools/bin2c.c