[Qemu-devel] [PATCH 0/2] target-i386: Small code cleanup

[Qemu-devel] [PATCH 0/2] target-i386: Small code cleanup

[Eduardo Habkost]

Just two small code cleanups in target-i386/cpu.c.

Patch 1/2 changes behavior of "-cpu ?" to not print feature names in reverse
order anymore. Patch 2/2 doesn't introduce any behavior changes.

Eduardo Habkost (2):
  target-i386: Simplify listflags() function
  target-i386: Eliminate unnecessary get_cpuid_vendor() function

 target-i386/cpu.c | 62 ++++++++++++++++++-------------------------------------
 1 file changed, 20 insertions(+), 42 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/2] target-i386: Simplify listflags() function

[Eduardo Habkost]

listflags() had lots of unnecessary complexity. Instead of printing to a
buffer that will be immediately printed, simply call the printing
function directly. Also, remove the fbits and flags arguments that were
always set to the same value. Also, there's no need to list the flags in
reverse order.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 42 ++++++++++++++----------------------------
 1 file changed, 14 insertions(+), 28 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3a9b32e..39d2fda 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1911,34 +1911,19 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
     }
 }
 
-/* generate a composite string into buf of all cpuid names in featureset
- * selected by fbits.  indicate truncation at bufsize in the event of overflow.
- * if flags, suppress names undefined in featureset.
+/* Print all cpuid feature names in featureset
  */
-static void listflags(char *buf, int bufsize, uint32_t fbits,
-                      const char **featureset, uint32_t flags)
-{
-    const char **p = &featureset[31];
-    char *q, *b, bit;
-    int nc;
-
-    b = 4 <= bufsize ? buf + (bufsize -= 3) - 1 : NULL;
-    *buf = '\0';
-    for (q = buf, bit = 31; fbits && bufsize; --p, fbits &= ~(1 << bit), --bit)
-        if (fbits & 1 << bit && (*p || !flags)) {
-            if (*p)
-                nc = snprintf(q, bufsize, "%s%s", q == buf ? "" : " ", *p);
-            else
-                nc = snprintf(q, bufsize, "%s[%d]", q == buf ? "" : " ", bit);
-            if (bufsize <= nc) {
-                if (b) {
-                    memcpy(b, "...", sizeof("..."));
-                }
-                return;
-            }
-            q += nc;
-            bufsize -= nc;
+static void listflags(FILE *f, fprintf_function print, const char **featureset)
+{
+    int bit;
+    bool first = true;
+
+    for (bit = 0; bit < 32; bit++) {
+        if (featureset[bit]) {
+            print(f, "%s%s", first ? "" : " ", featureset[bit]);
+            first = false;
         }
+    }
 }
 
 /* generate CPU information. */
@@ -1963,8 +1948,9 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
     for (i = 0; i < ARRAY_SIZE(feature_word_info); i++) {
         FeatureWordInfo *fw = &feature_word_info[i];
 
-        listflags(buf, sizeof(buf), (uint32_t)~0, fw->feat_names, 1);
-        (*cpu_fprintf)(f, "  %s\n", buf);
+        (*cpu_fprintf)(f, "  ");
+        listflags(f, cpu_fprintf, fw->feat_names);
+        (*cpu_fprintf)(f, "\n");
     }
 }
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH 1/2] target-i386: Simplify listflags() function

[Paolo Bonzini]

On 03/02/2015 19:08, Eduardo Habkost wrote:
> listflags() had lots of unnecessary complexity. Instead of printing to a
> buffer that will be immediately printed, simply call the printing
> function directly. Also, remove the fbits and flags arguments that were
> always set to the same value. Also, there's no need to list the flags in
> reverse order.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c | 42 ++++++++++++++----------------------------
>  1 file changed, 14 insertions(+), 28 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 3a9b32e..39d2fda 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1911,34 +1911,19 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
>      }
>  }
>  
> -/* generate a composite string into buf of all cpuid names in featureset
> - * selected by fbits.  indicate truncation at bufsize in the event of overflow.
> - * if flags, suppress names undefined in featureset.
> +/* Print all cpuid feature names in featureset
>   */
> -static void listflags(char *buf, int bufsize, uint32_t fbits,
> -                      const char **featureset, uint32_t flags)
> -{
> -    const char **p = &featureset[31];
> -    char *q, *b, bit;
> -    int nc;
> -
> -    b = 4 <= bufsize ? buf + (bufsize -= 3) - 1 : NULL;
> -    *buf = '\0';
> -    for (q = buf, bit = 31; fbits && bufsize; --p, fbits &= ~(1 << bit), --bit)
> -        if (fbits & 1 << bit && (*p || !flags)) {
> -            if (*p)
> -                nc = snprintf(q, bufsize, "%s%s", q == buf ? "" : " ", *p);
> -            else
> -                nc = snprintf(q, bufsize, "%s[%d]", q == buf ? "" : " ", bit);
> -            if (bufsize <= nc) {
> -                if (b) {
> -                    memcpy(b, "...", sizeof("..."));
> -                }
> -                return;
> -            }
> -            q += nc;
> -            bufsize -= nc;
> +static void listflags(FILE *f, fprintf_function print, const char **featureset)
> +{
> +    int bit;
> +    bool first = true;
> +
> +    for (bit = 0; bit < 32; bit++) {
> +        if (featureset[bit]) {
> +            print(f, "%s%s", first ? "" : " ", featureset[bit]);
> +            first = false;
>          }
> +    }
>  }
>  
>  /* generate CPU information. */
> @@ -1963,8 +1948,9 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
>      for (i = 0; i < ARRAY_SIZE(feature_word_info); i++) {
>          FeatureWordInfo *fw = &feature_word_info[i];
>  
> -        listflags(buf, sizeof(buf), (uint32_t)~0, fw->feat_names, 1);
> -        (*cpu_fprintf)(f, "  %s\n", buf);
> +        (*cpu_fprintf)(f, "  ");
> +        listflags(f, cpu_fprintf, fw->feat_names);
> +        (*cpu_fprintf)(f, "\n");
>      }
>  }
>  
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

[Qemu-devel] [PATCH 2/2] target-i386: Eliminate unnecessary get_cpuid_vendor() function

[Eduardo Habkost]

The function was used in only two places. In one of them, the function
made the code less readable by requiring temporary te[bcd]x variables.
In the other one we can simply inline the existing code.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 39d2fda..8c44e5b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2214,14 +2214,6 @@ void x86_cpudef_setup(void)
     }
 }
 
-static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
-                             uint32_t *ecx, uint32_t *edx)
-{
-    *ebx = env->cpuid_vendor1;
-    *edx = env->cpuid_vendor2;
-    *ecx = env->cpuid_vendor3;
-}
-
 void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                    uint32_t *eax, uint32_t *ebx,
                    uint32_t *ecx, uint32_t *edx)
@@ -2255,7 +2247,9 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
     switch(index) {
     case 0:
         *eax = env->cpuid_level;
-        get_cpuid_vendor(env, ebx, ecx, edx);
+        *ebx = env->cpuid_vendor1;
+        *edx = env->cpuid_vendor2;
+        *ecx = env->cpuid_vendor3;
         break;
     case 1:
         *eax = env->cpuid_version;
@@ -2448,11 +2442,9 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
          * So dont set it here for Intel to make Linux guests happy.
          */
         if (cs->nr_cores * cs->nr_threads > 1) {
-            uint32_t tebx, tecx, tedx;
-            get_cpuid_vendor(env, &tebx, &tecx, &tedx);
-            if (tebx != CPUID_VENDOR_INTEL_1 ||
-                tedx != CPUID_VENDOR_INTEL_2 ||
-                tecx != CPUID_VENDOR_INTEL_3) {
+            if (env->cpuid_vendor1 != CPUID_VENDOR_INTEL_1 ||
+                env->cpuid_vendor2 != CPUID_VENDOR_INTEL_2 ||
+                env->cpuid_vendor3 != CPUID_VENDOR_INTEL_3) {
                 *ecx |= 1 << 1;    /* CmpLegacy bit */
             }
         }
-- 
2.1.0

Re: [Qemu-devel] [PATCH 2/2] target-i386: Eliminate unnecessary get_cpuid_vendor() function

[Andreas Färber]

Am 03.02.2015 um 19:08 schrieb Eduardo Habkost:
> The function was used in only two places. In one of them, the function
> made the code less readable by requiring temporary te[bcd]x variables.
> In the other one we can simply inline the existing code.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Reviewed-by: Andreas Färber <afaerber@suse.de>

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 0/2] target-i386: Small code cleanup

[Eduardo Habkost]

On Tue, Feb 03, 2015 at 04:08:56PM -0200, Eduardo Habkost wrote:
> Just two small code cleanups in target-i386/cpu.c.
> 
> Patch 1/2 changes behavior of "-cpu ?" to not print feature names in reverse
> order anymore. Patch 2/2 doesn't introduce any behavior changes.

Andreas, just to confirm: is this series in your queue?

> 
> Eduardo Habkost (2):
>   target-i386: Simplify listflags() function
>   target-i386: Eliminate unnecessary get_cpuid_vendor() function
> 
>  target-i386/cpu.c | 62 ++++++++++++++++++-------------------------------------
>  1 file changed, 20 insertions(+), 42 deletions(-)
> 
> -- 
> 2.1.0
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/2] target-i386: Small code cleanup

[Andreas Färber]

Am 04.03.2015 um 13:28 schrieb Eduardo Habkost:
> On Tue, Feb 03, 2015 at 04:08:56PM -0200, Eduardo Habkost wrote:
>> Just two small code cleanups in target-i386/cpu.c.
>>
>> Patch 1/2 changes behavior of "-cpu ?" to not print feature names in reverse
>> order anymore. Patch 2/2 doesn't introduce any behavior changes.
> 
> Andreas, just to confirm: is this series in your queue?

No, this is perfectly within target-i386 scope, so please keep it in
your queue for resubmission.

Thanks,
Andreas

>> Eduardo Habkost (2):
>>   target-i386: Simplify listflags() function
>>   target-i386: Eliminate unnecessary get_cpuid_vendor() function
>>
>>  target-i386/cpu.c | 62 ++++++++++++++++++-------------------------------------
>>  1 file changed, 20 insertions(+), 42 deletions(-)

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)