All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/3] target/i386: Restrict 'feature-words' property to x86 machines
@ 2020-10-01 14:41 Philippe Mathieu-Daudé
  2020-10-01 14:41 ` [PATCH v4 1/3] target/i386/cpu: Trivial code movement Philippe Mathieu-Daudé
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-01 14:41 UTC (permalink / raw)
  To: Laurent Vivier, qemu-devel
  Cc: Eduardo Habkost, qemu-trivial, Philippe Mathieu-Daudé,
	Michael Tokarev, Markus Armbruster, Paolo Bonzini,
	Richard Henderson

This series restrict the 'feature-words' property to the x86
architecture (other archs don't have it), and to system-mode
(user-mode doesn't use it).

Extracted from "user-mode: Prune build dependencies (part 3)" v3:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg746423.html

Philippe Mathieu-Daudé (3):
  target/i386/cpu: Trivial code movement
  target/i386/cpu: Restrict some of feature-words uses to system-mode
  target/i386: Restrict X86CPUFeatureWord to X86 targets

 qapi/machine-target.json | 45 ++++++++++++++++++++++++++++++++++++++++
 qapi/machine.json        | 42 -------------------------------------
 target/i386/cpu.c        | 31 ++++++++++++++++-----------
 3 files changed, 64 insertions(+), 54 deletions(-)

-- 
2.26.2



^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v4 1/3] target/i386/cpu: Trivial code movement
  2020-10-01 14:41 [PATCH v4 0/3] target/i386: Restrict 'feature-words' property to x86 machines Philippe Mathieu-Daudé
@ 2020-10-01 14:41 ` Philippe Mathieu-Daudé
  2020-10-01 14:41 ` [PATCH v4 2/3] target/i386/cpu: Restrict some of feature-words uses to system-mode Philippe Mathieu-Daudé
  2020-10-01 14:41 ` [PATCH v4 3/3] target/i386: Restrict X86CPUFeatureWord to X86 targets Philippe Mathieu-Daudé
  2 siblings, 0 replies; 14+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-01 14:41 UTC (permalink / raw)
  To: Laurent Vivier, qemu-devel
  Cc: Eduardo Habkost, qemu-trivial, Philippe Mathieu-Daudé,
	Michael Tokarev, Markus Armbruster, Paolo Bonzini,
	Richard Henderson

Trivial code movement to reduce #ifdef'ry in the next commit.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 target/i386/cpu.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 3ffd877dd5..2eec53ca22 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4643,16 +4643,6 @@ static void x86_cpu_get_feature_words(Object *obj, Visitor *v,
     visit_type_X86CPUFeatureWordInfoList(v, "feature-words", &list, errp);
 }
 
-/* Convert all '_' in a feature string option name to '-', to make feature
- * name conform to QOM property naming rule, which uses '-' instead of '_'.
- */
-static inline void feat2prop(char *s)
-{
-    while ((s = strchr(s, '_'))) {
-        *s = '-';
-    }
-}
-
 /* Return the feature property name for a feature flag bit */
 static const char *x86_cpu_feature_name(FeatureWord w, int bitnr)
 {
@@ -4677,6 +4667,17 @@ static const char *x86_cpu_feature_name(FeatureWord w, int bitnr)
     return name;
 }
 
+/*
+ * Convert all '_' in a feature string option name to '-', to make feature
+ * name conform to QOM property naming rule, which uses '-' instead of '_'.
+ */
+static inline void feat2prop(char *s)
+{
+    while ((s = strchr(s, '_'))) {
+        *s = '-';
+    }
+}
+
 /* Compatibily hack to maintain legacy +-feat semantic,
  * where +-feat overwrites any feature set by
  * feat=on|feat even if the later is parsed after +-feat
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v4 2/3] target/i386/cpu: Restrict some of feature-words uses to system-mode
  2020-10-01 14:41 [PATCH v4 0/3] target/i386: Restrict 'feature-words' property to x86 machines Philippe Mathieu-Daudé
  2020-10-01 14:41 ` [PATCH v4 1/3] target/i386/cpu: Trivial code movement Philippe Mathieu-Daudé
@ 2020-10-01 14:41 ` Philippe Mathieu-Daudé
  2020-10-01 14:44   ` Philippe Mathieu-Daudé
  2020-10-01 15:14   ` Paolo Bonzini
  2020-10-01 14:41 ` [PATCH v4 3/3] target/i386: Restrict X86CPUFeatureWord to X86 targets Philippe Mathieu-Daudé
  2 siblings, 2 replies; 14+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-01 14:41 UTC (permalink / raw)
  To: Laurent Vivier, qemu-devel
  Cc: Eduardo Habkost, qemu-trivial, Philippe Mathieu-Daudé,
	Michael Tokarev, Markus Armbruster, Paolo Bonzini,
	Richard Henderson

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>
---
 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
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v4 3/3] target/i386: Restrict X86CPUFeatureWord to X86 targets
  2020-10-01 14:41 [PATCH v4 0/3] target/i386: Restrict 'feature-words' property to x86 machines Philippe Mathieu-Daudé
  2020-10-01 14:41 ` [PATCH v4 1/3] target/i386/cpu: Trivial code movement Philippe Mathieu-Daudé
  2020-10-01 14:41 ` [PATCH v4 2/3] target/i386/cpu: Restrict some of feature-words uses to system-mode Philippe Mathieu-Daudé
@ 2020-10-01 14:41 ` Philippe Mathieu-Daudé
  2020-10-01 15:29   ` Eduardo Habkost
  2 siblings, 1 reply; 14+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-01 14:41 UTC (permalink / raw)
  To: Laurent Vivier, qemu-devel
  Cc: Eduardo Habkost, qemu-trivial, Philippe Mathieu-Daudé,
	Michael Tokarev, Markus Armbruster, Paolo Bonzini,
	Richard Henderson, Richard Henderson

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.

Acked-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 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



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v4 2/3] target/i386/cpu: Restrict some of feature-words uses to system-mode
  2020-10-01 14:41 ` [PATCH v4 2/3] target/i386/cpu: Restrict some of feature-words uses to system-mode Philippe Mathieu-Daudé
@ 2020-10-01 14:44   ` Philippe Mathieu-Daudé
  2020-10-01 15:14   ` Paolo Bonzini
  1 sibling, 0 replies; 14+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-01 14:44 UTC (permalink / raw)
  To: Laurent Vivier, qemu-devel
  Cc: Eduardo Habkost, qemu-trivial, Michael Tokarev,
	Markus Armbruster, Paolo Bonzini, Richard Henderson

On 10/1/20 4:41 PM, 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>
> ---
>  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

Buh...

>      info->has_unavailable_features = true;

... I guess here goes the:

   #endif

>      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
> 



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v4 2/3] target/i386/cpu: Restrict some of feature-words uses to system-mode
  2020-10-01 14:41 ` [PATCH v4 2/3] target/i386/cpu: Restrict some of feature-words uses to system-mode Philippe Mathieu-Daudé
  2020-10-01 14:44   ` Philippe Mathieu-Daudé
@ 2020-10-01 15:14   ` Paolo Bonzini
  2020-10-01 15:27     ` Philippe Mathieu-Daudé
  1 sibling, 1 reply; 14+ messages in thread
From: Paolo Bonzini @ 2020-10-01 15:14 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, Laurent Vivier, qemu-devel
  Cc: Eduardo Habkost, qemu-trivial, Michael Tokarev,
	Markus Armbruster, Richard Henderson

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
> 



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v4 2/3] target/i386/cpu: Restrict some of feature-words uses to system-mode
  2020-10-01 15:14   ` Paolo Bonzini
@ 2020-10-01 15:27     ` Philippe Mathieu-Daudé
  2020-10-01 15:37       ` Eduardo Habkost
  0 siblings, 1 reply; 14+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-01 15:27 UTC (permalink / raw)
  To: Paolo Bonzini, Laurent Vivier, qemu-devel
  Cc: Eduardo Habkost, qemu-trivial, Michael Tokarev,
	Markus Armbruster, Richard Henderson

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
>>
> 



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v4 3/3] target/i386: Restrict X86CPUFeatureWord to X86 targets
  2020-10-01 14:41 ` [PATCH v4 3/3] target/i386: Restrict X86CPUFeatureWord to X86 targets Philippe Mathieu-Daudé
@ 2020-10-01 15:29   ` Eduardo Habkost
  2020-10-01 15:37     ` Philippe Mathieu-Daudé
  2020-10-02  6:55     ` Markus Armbruster
  0 siblings, 2 replies; 14+ messages in thread
From: Eduardo Habkost @ 2020-10-01 15:29 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-devel, Richard Henderson, Michael Tokarev,
	Markus Armbruster, Laurent Vivier, qemu-trivial, Paolo Bonzini,
	Richard Henderson

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
> 

-- 
Eduardo



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v4 2/3] target/i386/cpu: Restrict some of feature-words uses to system-mode
  2020-10-01 15:27     ` Philippe Mathieu-Daudé
@ 2020-10-01 15:37       ` Eduardo Habkost
  2020-10-01 15:57         ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 14+ messages in thread
From: Eduardo Habkost @ 2020-10-01 15:37 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-devel, Michael Tokarev, Laurent Vivier, Markus Armbruster,
	qemu-trivial, Paolo Bonzini, Richard Henderson

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.

-- 
Eduardo



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v4 3/3] target/i386: Restrict X86CPUFeatureWord to X86 targets
  2020-10-01 15:29   ` Eduardo Habkost
@ 2020-10-01 15:37     ` Philippe Mathieu-Daudé
  2020-10-01 15:53       ` Eduardo Habkost
  2020-10-02  6:55     ` Markus Armbruster
  1 sibling, 1 reply; 14+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-01 15:37 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: qemu-devel, Richard Henderson, Michael Tokarev,
	Markus Armbruster, Laurent Vivier, qemu-trivial, Paolo Bonzini,
	Richard Henderson

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
>>
> 



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v4 3/3] target/i386: Restrict X86CPUFeatureWord to X86 targets
  2020-10-01 15:37     ` Philippe Mathieu-Daudé
@ 2020-10-01 15:53       ` Eduardo Habkost
  0 siblings, 0 replies; 14+ messages in thread
From: Eduardo Habkost @ 2020-10-01 15:53 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-devel, Richard Henderson, Michael Tokarev,
	Markus Armbruster, Laurent Vivier, qemu-trivial, Paolo Bonzini,
	Richard Henderson

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
> >>
> > 
> 

-- 
Eduardo



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v4 2/3] target/i386/cpu: Restrict some of feature-words uses to system-mode
  2020-10-01 15:37       ` Eduardo Habkost
@ 2020-10-01 15:57         ` Philippe Mathieu-Daudé
  2020-10-01 16:15           ` Eduardo Habkost
  0 siblings, 1 reply; 14+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-01 15:57 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: qemu-devel, Michael Tokarev, Laurent Vivier, Markus Armbruster,
	qemu-trivial, Paolo Bonzini, Richard Henderson

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.



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v4 2/3] target/i386/cpu: Restrict some of feature-words uses to system-mode
  2020-10-01 15:57         ` Philippe Mathieu-Daudé
@ 2020-10-01 16:15           ` Eduardo Habkost
  0 siblings, 0 replies; 14+ messages in thread
From: Eduardo Habkost @ 2020-10-01 16:15 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-devel, Michael Tokarev, Laurent Vivier, Markus Armbruster,
	qemu-trivial, Paolo Bonzini, Richard Henderson

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



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v4 3/3] target/i386: Restrict X86CPUFeatureWord to X86 targets
  2020-10-01 15:29   ` Eduardo Habkost
  2020-10-01 15:37     ` Philippe Mathieu-Daudé
@ 2020-10-02  6:55     ` Markus Armbruster
  1 sibling, 0 replies; 14+ messages in thread
From: Markus Armbruster @ 2020-10-02  6:55 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: qemu-trivial, Michael Tokarev, Richard Henderson, qemu-devel,
	Laurent Vivier, Paolo Bonzini, Philippe Mathieu-Daudé,
	Richard Henderson

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?

[...]



^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2020-10-02  6:56 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-01 14:41 [PATCH v4 0/3] target/i386: Restrict 'feature-words' property to x86 machines Philippe Mathieu-Daudé
2020-10-01 14:41 ` [PATCH v4 1/3] target/i386/cpu: Trivial code movement Philippe Mathieu-Daudé
2020-10-01 14:41 ` [PATCH v4 2/3] target/i386/cpu: Restrict some of feature-words uses to system-mode Philippe Mathieu-Daudé
2020-10-01 14:44   ` Philippe Mathieu-Daudé
2020-10-01 15:14   ` Paolo Bonzini
2020-10-01 15:27     ` Philippe Mathieu-Daudé
2020-10-01 15:37       ` Eduardo Habkost
2020-10-01 15:57         ` Philippe Mathieu-Daudé
2020-10-01 16:15           ` Eduardo Habkost
2020-10-01 14:41 ` [PATCH v4 3/3] target/i386: Restrict X86CPUFeatureWord to X86 targets Philippe Mathieu-Daudé
2020-10-01 15:29   ` Eduardo Habkost
2020-10-01 15:37     ` Philippe Mathieu-Daudé
2020-10-01 15:53       ` Eduardo Habkost
2020-10-02  6:55     ` Markus Armbruster

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.