| Message ID | 20200626164307.3327380-2-dinechin@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | RFC: Move SPICE to a load module | expand |
Hello Christophe, On 6/26/20 6:42 PM, Christophe de Dinechin wrote: > In order to facilitate the move of large chunks of functionality to > load modules, it is simpler to create a wrapper with the same name > that simply relays the implementation. For efficiency, this is > typically done using inline functions in the header for the > corresponding functionality. In that case, we rename the actual > implementation by appending _implementation to its name. This makes it > easier to select which function you want to put a breakpoint on. > > Signed-off-by: Christophe de Dinechin <dinechin@redhat.com> > --- > include/qemu/module.h | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/include/qemu/module.h b/include/qemu/module.h > index 011ae1ae76..1922a0293c 100644 > --- a/include/qemu/module.h > +++ b/include/qemu/module.h > @@ -39,6 +39,30 @@ static void __attribute__((constructor)) do_qemu_init_ ## function(void) \ > } > #endif > > +#ifdef CONFIG_MODULES > +/* Identify which functions are replaced by a callback stub */ > +#ifdef MODULE_STUBS > +#define MODIFACE(Ret,Name,Args) \ > + Ret (*Name)Args; \ > + extern Ret Name##_implementation Args > +#else /* !MODULE_STUBS */ > +#define MODIFACE(Ret,Name,Args) \ > + extern Ret (*Name)Args; \ > + extern Ret Name##_implementation Args > +#endif /* MODULE_STUBS */ > + > +#define MODIMPL(Ret,Name,Args) \ > + static void __attribute__((constructor)) Name##_register(void) \ > + { \ > + Name = Name##_implementation; \ > + } \ > + Ret Name##_implementation Args > +#else /* !CONFIG_MODULES */ > +/* When not using a module, such functions are called directly */ > +#define MODIFACE(Ret,Name,Args) Ret Name Args > +#define MODIMPL(Ret,Name,Args) Ret Name Args > +#endif /* CONFIG_MODULES */ > + > typedef enum { > MODULE_INIT_MIGRATION, > MODULE_INIT_BLOCK, > Just as background, I am interested in all modules-related work, because of my long term plan to have target-specific modules as well: https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg04628.html I am not 100% clear on what is the goal and expected usage of this preprocessor code, despite the commit message, maybe you could clarify a bit with more verbosity? Additionally if you happen to be interested, maybe you know already or could think about what this could mean for target-specific modules, which will require some improvements to the modules "subsystem"(?) as well. In my experimentation I didn't have to do this preprocessor work, instead I had to fine tune a bit the makefile support (rules.mak and makefiles) to be able to accomodate for (even large) modules in target/ as well. Thanks! CLaudio
On Fri, Jun 26, 2020 at 06:42:58PM +0200, Christophe de Dinechin wrote: > In order to facilitate the move of large chunks of functionality to > load modules, it is simpler to create a wrapper with the same name > that simply relays the implementation. For efficiency, this is > typically done using inline functions in the header for the > corresponding functionality. In that case, we rename the actual > implementation by appending _implementation to its name. This makes it > easier to select which function you want to put a breakpoint on. > > Signed-off-by: Christophe de Dinechin <dinechin@redhat.com> > --- > include/qemu/module.h | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/include/qemu/module.h b/include/qemu/module.h > index 011ae1ae76..1922a0293c 100644 > --- a/include/qemu/module.h > +++ b/include/qemu/module.h > @@ -39,6 +39,30 @@ static void __attribute__((constructor)) do_qemu_init_ ## function(void) \ > } > #endif > > +#ifdef CONFIG_MODULES > +/* Identify which functions are replaced by a callback stub */ > +#ifdef MODULE_STUBS > +#define MODIFACE(Ret,Name,Args) \ > + Ret (*Name)Args; \ > + extern Ret Name##_implementation Args > +#else /* !MODULE_STUBS */ > +#define MODIFACE(Ret,Name,Args) \ > + extern Ret (*Name)Args; \ > + extern Ret Name##_implementation Args > +#endif /* MODULE_STUBS */ > + > +#define MODIMPL(Ret,Name,Args) \ > + static void __attribute__((constructor)) Name##_register(void) \ > + { \ > + Name = Name##_implementation; \ > + } \ > + Ret Name##_implementation Args > +#else /* !CONFIG_MODULES */ > +/* When not using a module, such functions are called directly */ > +#define MODIFACE(Ret,Name,Args) Ret Name Args > +#define MODIMPL(Ret,Name,Args) Ret Name Args > +#endif /* CONFIG_MODULES */ > + > typedef enum { > MODULE_INIT_MIGRATION, > MODULE_INIT_BLOCK, Hmm that's quite a bit of overhead for each call across modules. Can't code patching be used somehow? > -- > 2.26.2
On 2020-06-30 at 11:38 CEST, Michael S. Tsirkin wrote... > On Fri, Jun 26, 2020 at 06:42:58PM +0200, Christophe de Dinechin wrote: >> In order to facilitate the move of large chunks of functionality to >> load modules, it is simpler to create a wrapper with the same name >> that simply relays the implementation. For efficiency, this is >> typically done using inline functions in the header for the >> corresponding functionality. In that case, we rename the actual >> implementation by appending _implementation to its name. This makes it >> easier to select which function you want to put a breakpoint on. >> >> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com> >> --- >> include/qemu/module.h | 24 ++++++++++++++++++++++++ >> 1 file changed, 24 insertions(+) >> >> diff --git a/include/qemu/module.h b/include/qemu/module.h >> index 011ae1ae76..1922a0293c 100644 >> --- a/include/qemu/module.h >> +++ b/include/qemu/module.h >> @@ -39,6 +39,30 @@ static void __attribute__((constructor)) do_qemu_init_ ## function(void) \ >> } >> #endif >> >> +#ifdef CONFIG_MODULES >> +/* Identify which functions are replaced by a callback stub */ >> +#ifdef MODULE_STUBS >> +#define MODIFACE(Ret,Name,Args) \ >> + Ret (*Name)Args; \ >> + extern Ret Name##_implementation Args >> +#else /* !MODULE_STUBS */ >> +#define MODIFACE(Ret,Name,Args) \ >> + extern Ret (*Name)Args; \ >> + extern Ret Name##_implementation Args >> +#endif /* MODULE_STUBS */ >> + >> +#define MODIMPL(Ret,Name,Args) \ >> + static void __attribute__((constructor)) Name##_register(void) \ >> + { \ >> + Name = Name##_implementation; \ >> + } \ >> + Ret Name##_implementation Args >> +#else /* !CONFIG_MODULES */ >> +/* When not using a module, such functions are called directly */ >> +#define MODIFACE(Ret,Name,Args) Ret Name Args >> +#define MODIMPL(Ret,Name,Args) Ret Name Args >> +#endif /* CONFIG_MODULES */ >> + >> typedef enum { >> MODULE_INIT_MIGRATION, >> MODULE_INIT_BLOCK, > > Hmm that's quite a bit of overhead for each call across modules. Do you mean runtime overhead, i.e. the difference between foo(x) and (*foo)(x)? > Can't code patching be used somehow? I've considered it, but this seems like a bit of a hammer for half a dozen calls that are mostly init, so not performance sensitive. In the old times, ld.so used to do that for you. All you had to do was to mark one symbol as weak. Apparently for security reasons, that feature was dropped several years ago. At least, that put call patching where it belonged, i.e. in the loader. > > >> -- >> 2.26.2 -- Cheers, Christophe de Dinechin (IRC c3d)
On 2020-06-29 at 12:13 CEST, Claudio Fontana wrote... > Hello Christophe, > > On 6/26/20 6:42 PM, Christophe de Dinechin wrote: >> In order to facilitate the move of large chunks of functionality to >> load modules, it is simpler to create a wrapper with the same name >> that simply relays the implementation. For efficiency, this is >> typically done using inline functions in the header for the >> corresponding functionality. In that case, we rename the actual >> implementation by appending _implementation to its name. This makes it >> easier to select which function you want to put a breakpoint on. >> >> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com> >> --- >> include/qemu/module.h | 24 ++++++++++++++++++++++++ >> 1 file changed, 24 insertions(+) >> >> diff --git a/include/qemu/module.h b/include/qemu/module.h >> index 011ae1ae76..1922a0293c 100644 >> --- a/include/qemu/module.h >> +++ b/include/qemu/module.h >> @@ -39,6 +39,30 @@ static void __attribute__((constructor)) do_qemu_init_ ## function(void) \ >> } >> #endif >> >> +#ifdef CONFIG_MODULES >> +/* Identify which functions are replaced by a callback stub */ >> +#ifdef MODULE_STUBS >> +#define MODIFACE(Ret,Name,Args) \ >> + Ret (*Name)Args; \ >> + extern Ret Name##_implementation Args >> +#else /* !MODULE_STUBS */ >> +#define MODIFACE(Ret,Name,Args) \ >> + extern Ret (*Name)Args; \ >> + extern Ret Name##_implementation Args >> +#endif /* MODULE_STUBS */ >> + >> +#define MODIMPL(Ret,Name,Args) \ >> + static void __attribute__((constructor)) Name##_register(void) \ >> + { \ >> + Name = Name##_implementation; \ >> + } \ >> + Ret Name##_implementation Args >> +#else /* !CONFIG_MODULES */ >> +/* When not using a module, such functions are called directly */ >> +#define MODIFACE(Ret,Name,Args) Ret Name Args >> +#define MODIMPL(Ret,Name,Args) Ret Name Args >> +#endif /* CONFIG_MODULES */ >> + >> typedef enum { >> MODULE_INIT_MIGRATION, >> MODULE_INIT_BLOCK, >> > > Just as background, I am interested in all modules-related work, because of my long term plan to have target-specific modules as well: > > https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg04628.html > > I am not 100% clear on what is the goal and expected usage of this > preprocessor code, despite the commit message, maybe you could clarify a > bit with more verbosity? Well, so far, the preference seems to be to go through a more verbose approach with an explicit table of functions. What the preprocessor code did was: - If you build without modules, nothing changes, you get a direct call - If you build with modules: + In the DSO, foo is replaced with foo_implementation + Elsewhere, foo is replaced with a function pointer also called foo. + The implementation adds constructor code that sets foo to point to foo_implementation > > Additionally if you happen to be interested, maybe you know already or > could think about what this could mean for target-specific modules, which > will require some improvements to the modules "subsystem"(?) as well. So far, I've only integrated Gerd's workaround for target-specific modules. Some additional mechanics is needed to name target-specific modules, e.g. put them in some target directory. > > In my experimentation I didn't have to do this preprocessor work, instead > I had to fine tune a bit the makefile support (rules.mak and makefiles) to > be able to accomodate for (even large) modules in target/ as well. It's probably because the modules you were dealing with already had the required indirection and module_init calls, i.e. they were only invoked using QOM already. The mechanism I was proposing is to quickly add the indirection for qemu functionality that does not have such indirect calls yet. The consensus so far seems to be that the syntax I proposed is not nice, and that it's better to make it more explicit through a table and indirect calls, even if that means changing the call sites. > > Thanks! > > CLaudio -- Cheers, Christophe de Dinechin (IRC c3d)
diff --git a/include/qemu/module.h b/include/qemu/module.h index 011ae1ae76..1922a0293c 100644 --- a/include/qemu/module.h +++ b/include/qemu/module.h @@ -39,6 +39,30 @@ static void __attribute__((constructor)) do_qemu_init_ ## function(void) \ } #endif +#ifdef CONFIG_MODULES +/* Identify which functions are replaced by a callback stub */ +#ifdef MODULE_STUBS +#define MODIFACE(Ret,Name,Args) \ + Ret (*Name)Args; \ + extern Ret Name##_implementation Args +#else /* !MODULE_STUBS */ +#define MODIFACE(Ret,Name,Args) \ + extern Ret (*Name)Args; \ + extern Ret Name##_implementation Args +#endif /* MODULE_STUBS */ + +#define MODIMPL(Ret,Name,Args) \ + static void __attribute__((constructor)) Name##_register(void) \ + { \ + Name = Name##_implementation; \ + } \ + Ret Name##_implementation Args +#else /* !CONFIG_MODULES */ +/* When not using a module, such functions are called directly */ +#define MODIFACE(Ret,Name,Args) Ret Name Args +#define MODIMPL(Ret,Name,Args) Ret Name Args +#endif /* CONFIG_MODULES */ + typedef enum { MODULE_INIT_MIGRATION, MODULE_INIT_BLOCK,
In order to facilitate the move of large chunks of functionality to load modules, it is simpler to create a wrapper with the same name that simply relays the implementation. For efficiency, this is typically done using inline functions in the header for the corresponding functionality. In that case, we rename the actual implementation by appending _implementation to its name. This makes it easier to select which function you want to put a breakpoint on. Signed-off-by: Christophe de Dinechin <dinechin@redhat.com> --- include/qemu/module.h | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)