| Message ID | 20201001144152.1555659-1-philmd@redhat.com |
|---|---|
| Headers | show |
| Series | target/i386: Restrict 'feature-words' property to x86 machines | expand |
On 01/10/20 16:41, Philippe Mathieu-Daudé wrote: > The feature-words properties are not used in user-mode emulation, > restrict it to system-mode. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> This seems a bit pointless honestly. It's going to be a few KBs of code at most. I would end the work with the patches that have already been queued. Paolo > --- > target/i386/cpu.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index 2eec53ca22..9f72342506 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -4607,6 +4607,7 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, const char *name, > cpu->env.tsc_khz = cpu->env.user_tsc_khz = value / 1000; > } > > +#if !defined(CONFIG_USER_ONLY) > /* Generic getter for "feature-words" and "filtered-features" properties */ > static void x86_cpu_get_feature_words(Object *obj, Visitor *v, > const char *name, void *opaque, > @@ -4666,6 +4667,7 @@ static const char *x86_cpu_feature_name(FeatureWord w, int bitnr) > assert(bitnr < 32 || !(name && feature_word_info[w].type == CPUID_FEATURE_WORD)); > return name; > } > +#endif /* CONFIG_USER_ONLY */ > > /* > * Convert all '_' in a feature string option name to '-', to make feature > @@ -4783,6 +4785,7 @@ static void x86_cpu_parse_featurestr(const char *typename, char *features, > static void x86_cpu_expand_features(X86CPU *cpu, Error **errp); > static void x86_cpu_filter_features(X86CPU *cpu, bool verbose); > > +#if !defined(CONFIG_USER_ONLY) > /* Build a list with the name of all features on a feature word array */ > static void x86_cpu_list_feature_names(FeatureWordArray features, > strList **feat_names) > @@ -4853,6 +4856,7 @@ static void x86_cpu_class_check_missing_features(X86CPUClass *xcc, > > object_unref(OBJECT(xc)); > } > +#endif /* CONFIG_USER_ONLY */ > > /* Print all cpuid feature names in featureset > */ > @@ -4987,7 +4991,9 @@ static void x86_cpu_definition_entry(gpointer data, gpointer user_data) > > info = g_malloc0(sizeof(*info)); > info->name = x86_cpu_class_get_model_name(cc); > +#if !defined(CONFIG_USER_ONLY) > x86_cpu_class_check_missing_features(cc, &info->unavailable_features); > +#endif > info->has_unavailable_features = true; > info->q_typename = g_strdup(object_class_get_name(oc)); > info->migration_safe = cc->migration_safe; > @@ -6941,6 +6947,7 @@ static void x86_cpu_initfn(Object *obj) > object_property_add(obj, "tsc-frequency", "int", > x86_cpuid_get_tsc_freq, > x86_cpuid_set_tsc_freq, NULL, NULL); > +#if !defined(CONFIG_USER_ONLY) > object_property_add(obj, "feature-words", "X86CPUFeatureWordInfo", > x86_cpu_get_feature_words, > NULL, NULL, (void *)env->features); > @@ -6957,7 +6964,6 @@ static void x86_cpu_initfn(Object *obj) > x86_cpu_get_unavailable_features, > NULL, NULL, NULL); > > -#if !defined(CONFIG_USER_ONLY) > object_property_add(obj, "crash-information", "GuestPanicInformation", > x86_cpu_get_crash_info_qom, NULL, NULL, NULL); > #endif >
On 10/1/20 5:14 PM, Paolo Bonzini wrote: > On 01/10/20 16:41, Philippe Mathieu-Daudé wrote: >> The feature-words properties are not used in user-mode emulation, >> restrict it to system-mode. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > This seems a bit pointless honestly. It's going to be a few KBs of code > at most. I would end the work with the patches that have already been > queued. I'll let Eduardo and you discuss that, as he suggested the #ifdef instead of the stub. The two options are on the list, whichever you guys prefer :) > > Paolo > >> --- >> target/i386/cpu.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c >> index 2eec53ca22..9f72342506 100644 >> --- a/target/i386/cpu.c >> +++ b/target/i386/cpu.c >> @@ -4607,6 +4607,7 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, const char *name, >> cpu->env.tsc_khz = cpu->env.user_tsc_khz = value / 1000; >> } >> >> +#if !defined(CONFIG_USER_ONLY) >> /* Generic getter for "feature-words" and "filtered-features" properties */ >> static void x86_cpu_get_feature_words(Object *obj, Visitor *v, >> const char *name, void *opaque, >> @@ -4666,6 +4667,7 @@ static const char *x86_cpu_feature_name(FeatureWord w, int bitnr) >> assert(bitnr < 32 || !(name && feature_word_info[w].type == CPUID_FEATURE_WORD)); >> return name; >> } >> +#endif /* CONFIG_USER_ONLY */ >> >> /* >> * Convert all '_' in a feature string option name to '-', to make feature >> @@ -4783,6 +4785,7 @@ static void x86_cpu_parse_featurestr(const char *typename, char *features, >> static void x86_cpu_expand_features(X86CPU *cpu, Error **errp); >> static void x86_cpu_filter_features(X86CPU *cpu, bool verbose); >> >> +#if !defined(CONFIG_USER_ONLY) >> /* Build a list with the name of all features on a feature word array */ >> static void x86_cpu_list_feature_names(FeatureWordArray features, >> strList **feat_names) >> @@ -4853,6 +4856,7 @@ static void x86_cpu_class_check_missing_features(X86CPUClass *xcc, >> >> object_unref(OBJECT(xc)); >> } >> +#endif /* CONFIG_USER_ONLY */ >> >> /* Print all cpuid feature names in featureset >> */ >> @@ -4987,7 +4991,9 @@ static void x86_cpu_definition_entry(gpointer data, gpointer user_data) >> >> info = g_malloc0(sizeof(*info)); >> info->name = x86_cpu_class_get_model_name(cc); >> +#if !defined(CONFIG_USER_ONLY) >> x86_cpu_class_check_missing_features(cc, &info->unavailable_features); >> +#endif >> info->has_unavailable_features = true; >> info->q_typename = g_strdup(object_class_get_name(oc)); >> info->migration_safe = cc->migration_safe; >> @@ -6941,6 +6947,7 @@ static void x86_cpu_initfn(Object *obj) >> object_property_add(obj, "tsc-frequency", "int", >> x86_cpuid_get_tsc_freq, >> x86_cpuid_set_tsc_freq, NULL, NULL); >> +#if !defined(CONFIG_USER_ONLY) >> object_property_add(obj, "feature-words", "X86CPUFeatureWordInfo", >> x86_cpu_get_feature_words, >> NULL, NULL, (void *)env->features); >> @@ -6957,7 +6964,6 @@ static void x86_cpu_initfn(Object *obj) >> x86_cpu_get_unavailable_features, >> NULL, NULL, NULL); >> >> -#if !defined(CONFIG_USER_ONLY) >> object_property_add(obj, "crash-information", "GuestPanicInformation", >> x86_cpu_get_crash_info_qom, NULL, NULL, NULL); >> #endif >> >
On Thu, Oct 01, 2020 at 04:41:52PM +0200, Philippe Mathieu-Daudé wrote: > Only qemu-system-FOO and qemu-storage-daemon provide QMP > monitors, therefore such declarations and definitions are > irrelevant for user-mode emulation. > > Restricting the x86-specific commands to machine-target.json > pulls less QAPI-generated code into user-mode. Is this still true without "qapi: Restrict code generated for user-mode"? Markus, Eric: what's the difference between machine.json and machine-target.json? commit 7f7b4e7abef4 ("qapi: Split machine-target.json off target.json and misc.json") explains what but not why. > > Acked-by: Richard Henderson <richard.henderson@linaro.org> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Why the visit_type_X86CPUFeatureWordInfoList() stub was required in v3, but not in this version? > --- > qapi/machine-target.json | 45 ++++++++++++++++++++++++++++++++++++++++ > qapi/machine.json | 42 ------------------------------------- > target/i386/cpu.c | 2 +- > 3 files changed, 46 insertions(+), 43 deletions(-) > > diff --git a/qapi/machine-target.json b/qapi/machine-target.json > index 698850cc78..b4d769a53b 100644 > --- a/qapi/machine-target.json > +++ b/qapi/machine-target.json > @@ -4,6 +4,51 @@ > # This work is licensed under the terms of the GNU GPL, version 2 or later. > # See the COPYING file in the top-level directory. > > +## > +# @X86CPURegister32: > +# > +# A X86 32-bit register > +# > +# Since: 1.5 > +## > +{ 'enum': 'X86CPURegister32', > + 'data': [ 'EAX', 'EBX', 'ECX', 'EDX', 'ESP', 'EBP', 'ESI', 'EDI' ], > + 'if': 'defined(TARGET_I386)' } > + > +## > +# @X86CPUFeatureWordInfo: > +# > +# Information about a X86 CPU feature word > +# > +# @cpuid-input-eax: Input EAX value for CPUID instruction for that feature word > +# > +# @cpuid-input-ecx: Input ECX value for CPUID instruction for that > +# feature word > +# > +# @cpuid-register: Output register containing the feature bits > +# > +# @features: value of output register, containing the feature bits > +# > +# Since: 1.5 > +## > +{ 'struct': 'X86CPUFeatureWordInfo', > + 'data': { 'cpuid-input-eax': 'int', > + '*cpuid-input-ecx': 'int', > + 'cpuid-register': 'X86CPURegister32', > + 'features': 'int' }, > + 'if': 'defined(TARGET_I386)' } > + > +## > +# @DummyForceArrays: > +# > +# Not used by QMP; hack to let us use X86CPUFeatureWordInfoList internally > +# > +# Since: 2.5 > +## > +{ 'struct': 'DummyForceArrays', > + 'data': { 'unused': ['X86CPUFeatureWordInfo'] }, > + 'if': 'defined(TARGET_I386)' } > + > ## > # @CpuModelInfo: > # > diff --git a/qapi/machine.json b/qapi/machine.json > index 756dacb06f..995e972858 100644 > --- a/qapi/machine.json > +++ b/qapi/machine.json > @@ -574,48 +574,6 @@ > 'dst': 'uint16', > 'val': 'uint8' }} > > -## > -# @X86CPURegister32: > -# > -# A X86 32-bit register > -# > -# Since: 1.5 > -## > -{ 'enum': 'X86CPURegister32', > - 'data': [ 'EAX', 'EBX', 'ECX', 'EDX', 'ESP', 'EBP', 'ESI', 'EDI' ] } > - > -## > -# @X86CPUFeatureWordInfo: > -# > -# Information about a X86 CPU feature word > -# > -# @cpuid-input-eax: Input EAX value for CPUID instruction for that feature word > -# > -# @cpuid-input-ecx: Input ECX value for CPUID instruction for that > -# feature word > -# > -# @cpuid-register: Output register containing the feature bits > -# > -# @features: value of output register, containing the feature bits > -# > -# Since: 1.5 > -## > -{ 'struct': 'X86CPUFeatureWordInfo', > - 'data': { 'cpuid-input-eax': 'int', > - '*cpuid-input-ecx': 'int', > - 'cpuid-register': 'X86CPURegister32', > - 'features': 'int' } } > - > -## > -# @DummyForceArrays: > -# > -# Not used by QMP; hack to let us use X86CPUFeatureWordInfoList internally > -# > -# Since: 2.5 > -## > -{ 'struct': 'DummyForceArrays', > - 'data': { 'unused': ['X86CPUFeatureWordInfo'] } } > - > ## > # @NumaCpuOptions: > # > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index 9f72342506..848a65ded2 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -38,7 +38,7 @@ > #include "qemu/option.h" > #include "qemu/config-file.h" > #include "qapi/error.h" > -#include "qapi/qapi-visit-machine.h" > +#include "qapi/qapi-visit-machine-target.h" > #include "qapi/qapi-visit-run-state.h" > #include "qapi/qmp/qdict.h" > #include "qapi/qmp/qerror.h" > -- > 2.26.2 >
On Thu, Oct 01, 2020 at 05:27:32PM +0200, Philippe Mathieu-Daudé wrote: > On 10/1/20 5:14 PM, Paolo Bonzini wrote: > > On 01/10/20 16:41, Philippe Mathieu-Daudé wrote: > >> The feature-words properties are not used in user-mode emulation, > >> restrict it to system-mode. > >> > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > > > This seems a bit pointless honestly. It's going to be a few KBs of code > > at most. I would end the work with the patches that have already been > > queued. > > I'll let Eduardo and you discuss that, as he suggested the #ifdef > instead of the stub. The two options are on the list, whichever > you guys prefer :) Having neither the stub or the #ifdef is even better. :) I assumed the stub was necessary as a means for an end, but now I don't understand why the stub was necessary in the first place.
On 10/1/20 5:29 PM, Eduardo Habkost wrote: > On Thu, Oct 01, 2020 at 04:41:52PM +0200, Philippe Mathieu-Daudé wrote: >> Only qemu-system-FOO and qemu-storage-daemon provide QMP >> monitors, therefore such declarations and definitions are >> irrelevant for user-mode emulation. >> >> Restricting the x86-specific commands to machine-target.json >> pulls less QAPI-generated code into user-mode. > > Is this still true without "qapi: Restrict code generated for > user-mode"? The correct description so far is: "Restricting the x86-specific commands to machine-target.json pulls less QAPI-generated code into non-x86 targets.", as of this commit the X86CPURegister32 definitions are not built in the other ARM/S390/PPC/... targets. I still have some hope "qapi: Restrict code generated for user-mode" get merged some day. > > Markus, Eric: what's the difference between machine.json and > machine-target.json? commit 7f7b4e7abef4 ("qapi: Split > machine-target.json off target.json and misc.json") explains what > but not why. > >> >> Acked-by: Richard Henderson <richard.henderson@linaro.org> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > Why the visit_type_X86CPUFeatureWordInfoList() stub was required > in v3, but not in this version? Because it is ifdef'ed out in the previous (new) patch: "target/i386/cpu: Restrict some of feature-words uses to system-mode". > > >> --- >> qapi/machine-target.json | 45 ++++++++++++++++++++++++++++++++++++++++ >> qapi/machine.json | 42 ------------------------------------- >> target/i386/cpu.c | 2 +- >> 3 files changed, 46 insertions(+), 43 deletions(-) >> >> diff --git a/qapi/machine-target.json b/qapi/machine-target.json >> index 698850cc78..b4d769a53b 100644 >> --- a/qapi/machine-target.json >> +++ b/qapi/machine-target.json >> @@ -4,6 +4,51 @@ >> # This work is licensed under the terms of the GNU GPL, version 2 or later. >> # See the COPYING file in the top-level directory. >> >> +## >> +# @X86CPURegister32: >> +# >> +# A X86 32-bit register >> +# >> +# Since: 1.5 >> +## >> +{ 'enum': 'X86CPURegister32', >> + 'data': [ 'EAX', 'EBX', 'ECX', 'EDX', 'ESP', 'EBP', 'ESI', 'EDI' ], >> + 'if': 'defined(TARGET_I386)' } >> + >> +## >> +# @X86CPUFeatureWordInfo: >> +# >> +# Information about a X86 CPU feature word >> +# >> +# @cpuid-input-eax: Input EAX value for CPUID instruction for that feature word >> +# >> +# @cpuid-input-ecx: Input ECX value for CPUID instruction for that >> +# feature word >> +# >> +# @cpuid-register: Output register containing the feature bits >> +# >> +# @features: value of output register, containing the feature bits >> +# >> +# Since: 1.5 >> +## >> +{ 'struct': 'X86CPUFeatureWordInfo', >> + 'data': { 'cpuid-input-eax': 'int', >> + '*cpuid-input-ecx': 'int', >> + 'cpuid-register': 'X86CPURegister32', >> + 'features': 'int' }, >> + 'if': 'defined(TARGET_I386)' } >> + >> +## >> +# @DummyForceArrays: >> +# >> +# Not used by QMP; hack to let us use X86CPUFeatureWordInfoList internally >> +# >> +# Since: 2.5 >> +## >> +{ 'struct': 'DummyForceArrays', >> + 'data': { 'unused': ['X86CPUFeatureWordInfo'] }, >> + 'if': 'defined(TARGET_I386)' } >> + >> ## >> # @CpuModelInfo: >> # >> diff --git a/qapi/machine.json b/qapi/machine.json >> index 756dacb06f..995e972858 100644 >> --- a/qapi/machine.json >> +++ b/qapi/machine.json >> @@ -574,48 +574,6 @@ >> 'dst': 'uint16', >> 'val': 'uint8' }} >> >> -## >> -# @X86CPURegister32: >> -# >> -# A X86 32-bit register >> -# >> -# Since: 1.5 >> -## >> -{ 'enum': 'X86CPURegister32', >> - 'data': [ 'EAX', 'EBX', 'ECX', 'EDX', 'ESP', 'EBP', 'ESI', 'EDI' ] } >> - >> -## >> -# @X86CPUFeatureWordInfo: >> -# >> -# Information about a X86 CPU feature word >> -# >> -# @cpuid-input-eax: Input EAX value for CPUID instruction for that feature word >> -# >> -# @cpuid-input-ecx: Input ECX value for CPUID instruction for that >> -# feature word >> -# >> -# @cpuid-register: Output register containing the feature bits >> -# >> -# @features: value of output register, containing the feature bits >> -# >> -# Since: 1.5 >> -## >> -{ 'struct': 'X86CPUFeatureWordInfo', >> - 'data': { 'cpuid-input-eax': 'int', >> - '*cpuid-input-ecx': 'int', >> - 'cpuid-register': 'X86CPURegister32', >> - 'features': 'int' } } >> - >> -## >> -# @DummyForceArrays: >> -# >> -# Not used by QMP; hack to let us use X86CPUFeatureWordInfoList internally >> -# >> -# Since: 2.5 >> -## >> -{ 'struct': 'DummyForceArrays', >> - 'data': { 'unused': ['X86CPUFeatureWordInfo'] } } >> - >> ## >> # @NumaCpuOptions: >> # >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c >> index 9f72342506..848a65ded2 100644 >> --- a/target/i386/cpu.c >> +++ b/target/i386/cpu.c >> @@ -38,7 +38,7 @@ >> #include "qemu/option.h" >> #include "qemu/config-file.h" >> #include "qapi/error.h" >> -#include "qapi/qapi-visit-machine.h" >> +#include "qapi/qapi-visit-machine-target.h" >> #include "qapi/qapi-visit-run-state.h" >> #include "qapi/qmp/qdict.h" >> #include "qapi/qmp/qerror.h" >> -- >> 2.26.2 >> >
On Thu, Oct 01, 2020 at 05:37:26PM +0200, Philippe Mathieu-Daudé wrote: > On 10/1/20 5:29 PM, Eduardo Habkost wrote: > > On Thu, Oct 01, 2020 at 04:41:52PM +0200, Philippe Mathieu-Daudé wrote: > >> Only qemu-system-FOO and qemu-storage-daemon provide QMP > >> monitors, therefore such declarations and definitions are > >> irrelevant for user-mode emulation. > >> > >> Restricting the x86-specific commands to machine-target.json > >> pulls less QAPI-generated code into user-mode. > > > > Is this still true without "qapi: Restrict code generated for > > user-mode"? > > The correct description so far is: > > "Restricting the x86-specific commands to machine-target.json > pulls less QAPI-generated code into non-x86 targets.", as of > this commit the X86CPURegister32 definitions are not built in > the other ARM/S390/PPC/... targets. This seems to have other side effects in addition to remove the code from non-x86 targets and I don't understand why. > > I still have some hope "qapi: Restrict code generated for user-mode" > get merged some day. > > > > > Markus, Eric: what's the difference between machine.json and > > machine-target.json? commit 7f7b4e7abef4 ("qapi: Split > > machine-target.json off target.json and misc.json") explains what > > but not why. [1] > > > >> > >> Acked-by: Richard Henderson <richard.henderson@linaro.org> > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > > > Why the visit_type_X86CPUFeatureWordInfoList() stub was required > > in v3, but not in this version? > > Because it is ifdef'ed out in the previous (new) patch: > "target/i386/cpu: Restrict some of feature-words uses to system-mode". Sorry for the confusion, I thought this patch was _before_ "Restrict some of feature-words ..". Now, what I don't understand is: why moving the code from machine.json to machine-target.json makes patch 2/3 necessary? I assume the answer to the question above[1] would clarify this. > > > > > > >> --- > >> qapi/machine-target.json | 45 ++++++++++++++++++++++++++++++++++++++++ > >> qapi/machine.json | 42 ------------------------------------- > >> target/i386/cpu.c | 2 +- > >> 3 files changed, 46 insertions(+), 43 deletions(-) > >> > >> diff --git a/qapi/machine-target.json b/qapi/machine-target.json > >> index 698850cc78..b4d769a53b 100644 > >> --- a/qapi/machine-target.json > >> +++ b/qapi/machine-target.json > >> @@ -4,6 +4,51 @@ > >> # This work is licensed under the terms of the GNU GPL, version 2 or later. > >> # See the COPYING file in the top-level directory. > >> > >> +## > >> +# @X86CPURegister32: > >> +# > >> +# A X86 32-bit register > >> +# > >> +# Since: 1.5 > >> +## > >> +{ 'enum': 'X86CPURegister32', > >> + 'data': [ 'EAX', 'EBX', 'ECX', 'EDX', 'ESP', 'EBP', 'ESI', 'EDI' ], > >> + 'if': 'defined(TARGET_I386)' } > >> + > >> +## > >> +# @X86CPUFeatureWordInfo: > >> +# > >> +# Information about a X86 CPU feature word > >> +# > >> +# @cpuid-input-eax: Input EAX value for CPUID instruction for that feature word > >> +# > >> +# @cpuid-input-ecx: Input ECX value for CPUID instruction for that > >> +# feature word > >> +# > >> +# @cpuid-register: Output register containing the feature bits > >> +# > >> +# @features: value of output register, containing the feature bits > >> +# > >> +# Since: 1.5 > >> +## > >> +{ 'struct': 'X86CPUFeatureWordInfo', > >> + 'data': { 'cpuid-input-eax': 'int', > >> + '*cpuid-input-ecx': 'int', > >> + 'cpuid-register': 'X86CPURegister32', > >> + 'features': 'int' }, > >> + 'if': 'defined(TARGET_I386)' } > >> + > >> +## > >> +# @DummyForceArrays: > >> +# > >> +# Not used by QMP; hack to let us use X86CPUFeatureWordInfoList internally > >> +# > >> +# Since: 2.5 > >> +## > >> +{ 'struct': 'DummyForceArrays', > >> + 'data': { 'unused': ['X86CPUFeatureWordInfo'] }, > >> + 'if': 'defined(TARGET_I386)' } > >> + > >> ## > >> # @CpuModelInfo: > >> # > >> diff --git a/qapi/machine.json b/qapi/machine.json > >> index 756dacb06f..995e972858 100644 > >> --- a/qapi/machine.json > >> +++ b/qapi/machine.json > >> @@ -574,48 +574,6 @@ > >> 'dst': 'uint16', > >> 'val': 'uint8' }} > >> > >> -## > >> -# @X86CPURegister32: > >> -# > >> -# A X86 32-bit register > >> -# > >> -# Since: 1.5 > >> -## > >> -{ 'enum': 'X86CPURegister32', > >> - 'data': [ 'EAX', 'EBX', 'ECX', 'EDX', 'ESP', 'EBP', 'ESI', 'EDI' ] } > >> - > >> -## > >> -# @X86CPUFeatureWordInfo: > >> -# > >> -# Information about a X86 CPU feature word > >> -# > >> -# @cpuid-input-eax: Input EAX value for CPUID instruction for that feature word > >> -# > >> -# @cpuid-input-ecx: Input ECX value for CPUID instruction for that > >> -# feature word > >> -# > >> -# @cpuid-register: Output register containing the feature bits > >> -# > >> -# @features: value of output register, containing the feature bits > >> -# > >> -# Since: 1.5 > >> -## > >> -{ 'struct': 'X86CPUFeatureWordInfo', > >> - 'data': { 'cpuid-input-eax': 'int', > >> - '*cpuid-input-ecx': 'int', > >> - 'cpuid-register': 'X86CPURegister32', > >> - 'features': 'int' } } > >> - > >> -## > >> -# @DummyForceArrays: > >> -# > >> -# Not used by QMP; hack to let us use X86CPUFeatureWordInfoList internally > >> -# > >> -# Since: 2.5 > >> -## > >> -{ 'struct': 'DummyForceArrays', > >> - 'data': { 'unused': ['X86CPUFeatureWordInfo'] } } > >> - > >> ## > >> # @NumaCpuOptions: > >> # > >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c > >> index 9f72342506..848a65ded2 100644 > >> --- a/target/i386/cpu.c > >> +++ b/target/i386/cpu.c > >> @@ -38,7 +38,7 @@ > >> #include "qemu/option.h" > >> #include "qemu/config-file.h" > >> #include "qapi/error.h" > >> -#include "qapi/qapi-visit-machine.h" > >> +#include "qapi/qapi-visit-machine-target.h" > >> #include "qapi/qapi-visit-run-state.h" > >> #include "qapi/qmp/qdict.h" > >> #include "qapi/qmp/qerror.h" > >> -- > >> 2.26.2 > >> > > >
On 10/1/20 5:37 PM, Eduardo Habkost wrote: > On Thu, Oct 01, 2020 at 05:27:32PM +0200, Philippe Mathieu-Daudé wrote: >> On 10/1/20 5:14 PM, Paolo Bonzini wrote: >>> On 01/10/20 16:41, Philippe Mathieu-Daudé wrote: >>>> The feature-words properties are not used in user-mode emulation, >>>> restrict it to system-mode. >>>> >>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> >>> This seems a bit pointless honestly. It's going to be a few KBs of code >>> at most. I would end the work with the patches that have already been >>> queued. >> >> I'll let Eduardo and you discuss that, as he suggested the #ifdef >> instead of the stub. The two options are on the list, whichever >> you guys prefer :) > > Having neither the stub or the #ifdef is even better. :) > > I assumed the stub was necessary as a means for an end, but now I > don't understand why the stub was necessary in the first place. x86_cpu_get_feature_words() calls QAPI-generated visit_type_X86CPUFeatureWordInfoList(). Since "Only qemu-system-FOO and qemu-storage-daemon provide QMP monitors", this code is unreachable on user-mode. I tried to get a "no ifdef/no stub" codebase by not removing the QAPI uses from user-mode code, and build it without QAPI-generated files. I could only start the work and realized it is a huge effort. In particular since nobody sees the point of doing this cleanup... Regards, Phil.
On Thu, Oct 01, 2020 at 05:57:14PM +0200, Philippe Mathieu-Daudé wrote: > On 10/1/20 5:37 PM, Eduardo Habkost wrote: > > On Thu, Oct 01, 2020 at 05:27:32PM +0200, Philippe Mathieu-Daudé wrote: > >> On 10/1/20 5:14 PM, Paolo Bonzini wrote: > >>> On 01/10/20 16:41, Philippe Mathieu-Daudé wrote: > >>>> The feature-words properties are not used in user-mode emulation, > >>>> restrict it to system-mode. > >>>> > >>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >>> > >>> This seems a bit pointless honestly. It's going to be a few KBs of code > >>> at most. I would end the work with the patches that have already been > >>> queued. > >> > >> I'll let Eduardo and you discuss that, as he suggested the #ifdef > >> instead of the stub. The two options are on the list, whichever > >> you guys prefer :) > > > > Having neither the stub or the #ifdef is even better. :) > > > > I assumed the stub was necessary as a means for an end, but now I > > don't understand why the stub was necessary in the first place. > > x86_cpu_get_feature_words() calls QAPI-generated > visit_type_X86CPUFeatureWordInfoList(). > > Since "Only qemu-system-FOO and qemu-storage-daemon provide QMP > monitors", this code is unreachable on user-mode. This is true, but you are shaving a tiny fraction of target/i386/cpu.c code unreachable in user-mode. I agree with Paolo that it's pointless. > > I tried to get a "no ifdef/no stub" codebase by not removing the > QAPI uses from user-mode code, and build it without QAPI-generated > files. I could only start the work and realized it is a huge effort. > In particular since nobody sees the point of doing this cleanup... I don't see the point of removing QAPI-generated files from user-mode. Long term, I'd expect to see more QAPI usage in QEMU, which will probably include code used by user-mode. -- Eduardo
Eduardo Habkost <ehabkost@redhat.com> writes: > On Thu, Oct 01, 2020 at 04:41:52PM +0200, Philippe Mathieu-Daudé wrote: >> Only qemu-system-FOO and qemu-storage-daemon provide QMP >> monitors, therefore such declarations and definitions are >> irrelevant for user-mode emulation. >> >> Restricting the x86-specific commands to machine-target.json >> pulls less QAPI-generated code into user-mode. > > Is this still true without "qapi: Restrict code generated for > user-mode"? > > Markus, Eric: what's the difference between machine.json and > machine-target.json? commit 7f7b4e7abef4 ("qapi: Split > machine-target.json off target.json and misc.json") explains what > but not why. My bad. Short explanation: machine-target.json can use preprocessor symbols with 'if' that are only available in target-specific code: TARGET_S390X, TARGET_I386, TARGET_ARM, ... This requires compiling per target. machine.json can't use these, because it's compiled just once. Same for misc-target.json and misc.json. Now the long explanation. The clue is buried in the patch: diff --git a/qapi/Makefile.objs b/qapi/Makefile.objs index 01dced01aa..4e87bef6e1 100644 --- a/qapi/Makefile.objs +++ b/qapi/Makefile.objs @@ -9,7 +9,7 @@ QAPI_COMMON_MODULES = audio authz block-core block char common crypto QAPI_COMMON_MODULES += introspect job machine migration misc net QAPI_COMMON_MODULES += qdev qom rdma rocker run-state sockets tpm QAPI_COMMON_MODULES += trace transaction ui -QAPI_TARGET_MODULES = target +QAPI_TARGET_MODULES = machine-target target QAPI_MODULES = $(QAPI_COMMON_MODULES) $(QAPI_TARGET_MODULES) util-obj-y += qapi-builtin-types.o QAPI_TARGET_MODULES appears in commit 61eb9e80d5: qapi: New module target.json We can't add appropriate target-specific conditionals to misc.json, because that would make all of misc.json unusable in target-independent code. To keep misc.json target-independent, we need to split off target-dependent target.json. This commit doesn't actually split off anything, it merely creates the empty module. The next few patches will move stuff from misc.json there. Signed-off-by: Markus Armbruster <armbru@redhat.com> Aha: it's about compiling per target, to make target-specific conditionals available. This part of qapi/Makefile.objs has since morphed into qapi_inputs = [] qapi_specific_outputs = [] foreach module : qapi_all_modules qapi_inputs += [ files(module + '.json') ] qapi_module_outputs = [ ... ] if module.endswith('-target') qapi_specific_outputs += qapi_module_outputs else qapi_util_outputs += qapi_module_outputs endif endforeach in qapi/meson.build. Further down: # Now go through all the outputs and add them to the right sourceset. # These loops must be synchronized with the output of the above custom target. i = 0 foreach output : qapi_util_outputs if output.endswith('.h') genh += qapi_files[i] endif util_ss.add(qapi_files[i]) i = i + 1 endforeach foreach output : qapi_specific_outputs + qapi_nonmodule_outputs if output.endswith('.h') genh += qapi_files[i] endif specific_ss.add(when: 'CONFIG_SOFTMMU', if_true: qapi_files[i]) i = i + 1 endforeach The source files generated for the "common" modules are added to sourceset util_ss, and the ones for the "target" modules are added to sourceset specific_ss. The former gets compiled just once, and the .o get linked into pretty much everything. The latter gets compiled per target, and the .o get linked into target-specific executables. Clear as mud? [...]