Message ID | 1538734535-7483-1-git-send-email-thuth@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | hw/core/generic-loader: Compile only once, not for each target | expand |
On 5 October 2018 at 11:15, Thomas Huth <thuth@redhat.com> wrote: > The generic-loader is currently compiled target specific due to one > single "#ifdef TARGET_WORDS_BIGENDIAN" in the file. We've already > got a function called target_words_bigendian() for this instead, so > we can put the generic-loader into common-obj to save some compilation > time. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > exec.c | 1 - > hw/core/Makefile.objs | 2 +- > hw/core/generic-loader.c | 6 +----- > hw/virtio/virtio.c | 1 - > include/qom/cpu.h | 2 ++ > qom/cpu.c | 1 - > 6 files changed, 4 insertions(+), 9 deletions(-) > > diff --git a/exec.c b/exec.c > index d0821e6..3ae0e54 100644 > --- a/exec.c > +++ b/exec.c > @@ -3910,7 +3910,6 @@ int qemu_target_page_bits_min(void) > * A helper function for the _utterly broken_ virtio device model to find out if > * it's running on a big endian machine. Don't do this at home kids! > */ > -bool target_words_bigendian(void); > bool target_words_bigendian(void) > { > #if defined(TARGET_WORDS_BIGENDIAN) > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -1085,6 +1085,8 @@ void cpu_exec_initfn(CPUState *cpu); > void cpu_exec_realizefn(CPUState *cpu, Error **errp); > void cpu_exec_unrealizefn(CPUState *cpu); > > +bool target_words_bigendian(void); > + Can we have a doc comment for a new global function in a header file, please? In particular this one needs a big health warning that using it is almost always the wrong thing (compare the comment in the .c file). thanks -- PMM
Hi Thomas, On 10/5/18 12:15 PM, Thomas Huth wrote: > The generic-loader is currently compiled target specific due to one > single "#ifdef TARGET_WORDS_BIGENDIAN" in the file. We've already > got a function called target_words_bigendian() for this instead, so > we can put the generic-loader into common-obj to save some compilation > time. Please do this in 2 patches: - clean the target_words_bigendian() mess, if possible documenting the new function added in "qom/cpu.h" - then move generic-loader to common Is the comment added in 8e4a424b305 still valid? /* * A helper function for the _utterly broken_ virtio device model to find out if * it's running on a big endian machine. Don't do this at home kids! */ bool target_words_bigendian(void); Thanks, Phil. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > exec.c | 1 - > hw/core/Makefile.objs | 2 +- > hw/core/generic-loader.c | 6 +----- > hw/virtio/virtio.c | 1 - > include/qom/cpu.h | 2 ++ > qom/cpu.c | 1 - > 6 files changed, 4 insertions(+), 9 deletions(-) > > diff --git a/exec.c b/exec.c > index d0821e6..3ae0e54 100644 > --- a/exec.c > +++ b/exec.c > @@ -3910,7 +3910,6 @@ int qemu_target_page_bits_min(void) > * A helper function for the _utterly broken_ virtio device model to find out if > * it's running on a big endian machine. Don't do this at home kids! > */ > -bool target_words_bigendian(void); > bool target_words_bigendian(void) > { > #if defined(TARGET_WORDS_BIGENDIAN) > diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs > index eb88ca9..b736ce2 100644 > --- a/hw/core/Makefile.objs > +++ b/hw/core/Makefile.objs > @@ -20,6 +20,6 @@ common-obj-$(CONFIG_SOFTMMU) += register.o > common-obj-$(CONFIG_SOFTMMU) += or-irq.o > common-obj-$(CONFIG_SOFTMMU) += split-irq.o > common-obj-$(CONFIG_PLATFORM_BUS) += platform-bus.o > +common-obj-$(CONFIG_SOFTMMU) += generic-loader.o > > -obj-$(CONFIG_SOFTMMU) += generic-loader.o > obj-$(CONFIG_SOFTMMU) += null-machine.o > diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c > index be29ae1..fbae05f 100644 > --- a/hw/core/generic-loader.c > +++ b/hw/core/generic-loader.c > @@ -130,11 +130,7 @@ static void generic_loader_realize(DeviceState *dev, Error **errp) > s->cpu = first_cpu; > } > > -#ifdef TARGET_WORDS_BIGENDIAN > - big_endian = 1; > -#else > - big_endian = 0; > -#endif > + big_endian = target_words_bigendian(); > > if (s->file) { > AddressSpace *as = s->cpu ? s->cpu->as : NULL; > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 94f5c8e..4e61944 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -1169,7 +1169,6 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val) > return 0; > } > > -bool target_words_bigendian(void); > static enum virtio_device_endian virtio_default_endian(void) > { > if (target_words_bigendian()) { > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index dc130cd..0e2ce80 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -1085,6 +1085,8 @@ void cpu_exec_initfn(CPUState *cpu); > void cpu_exec_realizefn(CPUState *cpu, Error **errp); > void cpu_exec_unrealizefn(CPUState *cpu); > > +bool target_words_bigendian(void); > + > #ifdef NEED_CPU_H > > #ifdef CONFIG_SOFTMMU > diff --git a/qom/cpu.c b/qom/cpu.c > index 92599f3..f774654 100644 > --- a/qom/cpu.c > +++ b/qom/cpu.c > @@ -194,7 +194,6 @@ static bool cpu_common_debug_check_watchpoint(CPUState *cpu, CPUWatchpoint *wp) > return true; > } > > -bool target_words_bigendian(void); > static bool cpu_common_virtio_is_big_endian(CPUState *cpu) > { > return target_words_bigendian(); >
On 10/05/18 12:49, Philippe Mathieu-Daudé wrote: > Hi Thomas, > > On 10/5/18 12:15 PM, Thomas Huth wrote: >> The generic-loader is currently compiled target specific due to one >> single "#ifdef TARGET_WORDS_BIGENDIAN" in the file. We've already >> got a function called target_words_bigendian() for this instead, so >> we can put the generic-loader into common-obj to save some compilation >> time. > > Please do this in 2 patches: > - clean the target_words_bigendian() mess, if possible documenting the > new function added in "qom/cpu.h" > - then move generic-loader to common > > Is the comment added in 8e4a424b305 still valid? > > /* > * A helper function for the _utterly broken_ virtio device model to > find out if > * it's running on a big endian machine. Don't do this at home kids! > */ > bool target_words_bigendian(void); Yeah, that comment irks me too. Not that it isn't valid, necessarily. "hw/virtio/virtio.c" still features an embedded declaration of this function. Just above virtio_default_endian(), which calls it. And the latter function is called in several places (mostly from commit 616a655219a92). So I can't really tell what's "utterly broken" here -- the device model (i.e., the virtio implementation in QEMU), or the legacy *spec* that required QEMU to jump through such hoops? (Either way, the embedded declaration of the function should go.) BTW, I don't think angry comments are useful in *code*. They look & feel great for about a week. In a year, they just look unprofessional. (NB, this is not about "political correctness"; it's just that, when someone reads such a comment, they receive the anger *before* they understand, or recall, the underlying problem.) (Don't mean to derail Thomas's patch with some bikeshedding here...) Thanks, Laszlo >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> exec.c | 1 - >> hw/core/Makefile.objs | 2 +- >> hw/core/generic-loader.c | 6 +----- >> hw/virtio/virtio.c | 1 - >> include/qom/cpu.h | 2 ++ >> qom/cpu.c | 1 - >> 6 files changed, 4 insertions(+), 9 deletions(-) >> >> diff --git a/exec.c b/exec.c >> index d0821e6..3ae0e54 100644 >> --- a/exec.c >> +++ b/exec.c >> @@ -3910,7 +3910,6 @@ int qemu_target_page_bits_min(void) >> * A helper function for the _utterly broken_ virtio device model to find out if >> * it's running on a big endian machine. Don't do this at home kids! >> */ >> -bool target_words_bigendian(void); >> bool target_words_bigendian(void) >> { >> #if defined(TARGET_WORDS_BIGENDIAN) >> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs >> index eb88ca9..b736ce2 100644 >> --- a/hw/core/Makefile.objs >> +++ b/hw/core/Makefile.objs >> @@ -20,6 +20,6 @@ common-obj-$(CONFIG_SOFTMMU) += register.o >> common-obj-$(CONFIG_SOFTMMU) += or-irq.o >> common-obj-$(CONFIG_SOFTMMU) += split-irq.o >> common-obj-$(CONFIG_PLATFORM_BUS) += platform-bus.o >> +common-obj-$(CONFIG_SOFTMMU) += generic-loader.o >> >> -obj-$(CONFIG_SOFTMMU) += generic-loader.o >> obj-$(CONFIG_SOFTMMU) += null-machine.o >> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c >> index be29ae1..fbae05f 100644 >> --- a/hw/core/generic-loader.c >> +++ b/hw/core/generic-loader.c >> @@ -130,11 +130,7 @@ static void generic_loader_realize(DeviceState *dev, Error **errp) >> s->cpu = first_cpu; >> } >> >> -#ifdef TARGET_WORDS_BIGENDIAN >> - big_endian = 1; >> -#else >> - big_endian = 0; >> -#endif >> + big_endian = target_words_bigendian(); >> >> if (s->file) { >> AddressSpace *as = s->cpu ? s->cpu->as : NULL; >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >> index 94f5c8e..4e61944 100644 >> --- a/hw/virtio/virtio.c >> +++ b/hw/virtio/virtio.c >> @@ -1169,7 +1169,6 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val) >> return 0; >> } >> >> -bool target_words_bigendian(void); >> static enum virtio_device_endian virtio_default_endian(void) >> { >> if (target_words_bigendian()) { >> diff --git a/include/qom/cpu.h b/include/qom/cpu.h >> index dc130cd..0e2ce80 100644 >> --- a/include/qom/cpu.h >> +++ b/include/qom/cpu.h >> @@ -1085,6 +1085,8 @@ void cpu_exec_initfn(CPUState *cpu); >> void cpu_exec_realizefn(CPUState *cpu, Error **errp); >> void cpu_exec_unrealizefn(CPUState *cpu); >> >> +bool target_words_bigendian(void); >> + >> #ifdef NEED_CPU_H >> >> #ifdef CONFIG_SOFTMMU >> diff --git a/qom/cpu.c b/qom/cpu.c >> index 92599f3..f774654 100644 >> --- a/qom/cpu.c >> +++ b/qom/cpu.c >> @@ -194,7 +194,6 @@ static bool cpu_common_debug_check_watchpoint(CPUState *cpu, CPUWatchpoint *wp) >> return true; >> } >> >> -bool target_words_bigendian(void); >> static bool cpu_common_virtio_is_big_endian(CPUState *cpu) >> { >> return target_words_bigendian(); >> >
On 10/05/18 14:17, Laszlo Ersek wrote: > Not that it isn't valid, necessarily. "hw/virtio/virtio.c" still > features an embedded declaration of this function. Just above > virtio_default_endian(), which calls it. [...] > > (Either way, the embedded declaration of the function should go.) (which is what the patch already does, of course!) Laszlo
On 5 October 2018 at 13:17, Laszlo Ersek <lersek@redhat.com> wrote: > On 10/05/18 12:49, Philippe Mathieu-Daudé wrote: >> Is the comment added in 8e4a424b305 still valid? >> >> /* >> * A helper function for the _utterly broken_ virtio device model to >> find out if >> * it's running on a big endian machine. Don't do this at home kids! >> */ >> bool target_words_bigendian(void); > > Yeah, that comment irks me too. > > Not that it isn't valid, necessarily. "hw/virtio/virtio.c" still > features an embedded declaration of this function. Just above > virtio_default_endian(), which calls it. And the latter function is > called in several places (mostly from commit 616a655219a92). > > So I can't really tell what's "utterly broken" here -- the device model > (i.e., the virtio implementation in QEMU), or the legacy *spec* that > required QEMU to jump through such hoops? AIUI, the brokenness here is that the virtio specification requires that a device knows the endianness of the CPU, which is an awkward layering violation [*]. The point of the comment is really to say "virtio needs this, but that is a special weird case, think twice or three times before using this function anywhere else" (which I suspect is also why the declaration is not in a header file, to make it less easily accessible). [*] Actually it's not even the endianness of the CPU, because bi-endian CPUs like Arm can be in big-endian mode even though TARGET_WORDS_BIGENDIAN is false... I entirely agree that we should clean up the comment to be clearer about what it's trying to say. > BTW, I don't think angry comments are useful in *code*. They look & feel > great for about a week. In a year, they just look unprofessional. (NB, > this is not about "political correctness"; it's just that, when someone > reads such a comment, they receive the anger *before* they understand, > or recall, the underlying problem. Agreed 100%. thanks -- PMM
diff --git a/exec.c b/exec.c index d0821e6..3ae0e54 100644 --- a/exec.c +++ b/exec.c @@ -3910,7 +3910,6 @@ int qemu_target_page_bits_min(void) * A helper function for the _utterly broken_ virtio device model to find out if * it's running on a big endian machine. Don't do this at home kids! */ -bool target_words_bigendian(void); bool target_words_bigendian(void) { #if defined(TARGET_WORDS_BIGENDIAN) diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs index eb88ca9..b736ce2 100644 --- a/hw/core/Makefile.objs +++ b/hw/core/Makefile.objs @@ -20,6 +20,6 @@ common-obj-$(CONFIG_SOFTMMU) += register.o common-obj-$(CONFIG_SOFTMMU) += or-irq.o common-obj-$(CONFIG_SOFTMMU) += split-irq.o common-obj-$(CONFIG_PLATFORM_BUS) += platform-bus.o +common-obj-$(CONFIG_SOFTMMU) += generic-loader.o -obj-$(CONFIG_SOFTMMU) += generic-loader.o obj-$(CONFIG_SOFTMMU) += null-machine.o diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c index be29ae1..fbae05f 100644 --- a/hw/core/generic-loader.c +++ b/hw/core/generic-loader.c @@ -130,11 +130,7 @@ static void generic_loader_realize(DeviceState *dev, Error **errp) s->cpu = first_cpu; } -#ifdef TARGET_WORDS_BIGENDIAN - big_endian = 1; -#else - big_endian = 0; -#endif + big_endian = target_words_bigendian(); if (s->file) { AddressSpace *as = s->cpu ? s->cpu->as : NULL; diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 94f5c8e..4e61944 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -1169,7 +1169,6 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val) return 0; } -bool target_words_bigendian(void); static enum virtio_device_endian virtio_default_endian(void) { if (target_words_bigendian()) { diff --git a/include/qom/cpu.h b/include/qom/cpu.h index dc130cd..0e2ce80 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -1085,6 +1085,8 @@ void cpu_exec_initfn(CPUState *cpu); void cpu_exec_realizefn(CPUState *cpu, Error **errp); void cpu_exec_unrealizefn(CPUState *cpu); +bool target_words_bigendian(void); + #ifdef NEED_CPU_H #ifdef CONFIG_SOFTMMU diff --git a/qom/cpu.c b/qom/cpu.c index 92599f3..f774654 100644 --- a/qom/cpu.c +++ b/qom/cpu.c @@ -194,7 +194,6 @@ static bool cpu_common_debug_check_watchpoint(CPUState *cpu, CPUWatchpoint *wp) return true; } -bool target_words_bigendian(void); static bool cpu_common_virtio_is_big_endian(CPUState *cpu) { return target_words_bigendian();
The generic-loader is currently compiled target specific due to one single "#ifdef TARGET_WORDS_BIGENDIAN" in the file. We've already got a function called target_words_bigendian() for this instead, so we can put the generic-loader into common-obj to save some compilation time. Signed-off-by: Thomas Huth <thuth@redhat.com> --- exec.c | 1 - hw/core/Makefile.objs | 2 +- hw/core/generic-loader.c | 6 +----- hw/virtio/virtio.c | 1 - include/qom/cpu.h | 2 ++ qom/cpu.c | 1 - 6 files changed, 4 insertions(+), 9 deletions(-)