[Qemu-devel] [qemu-devel] [PATCH v2] target-i386: Fix segment cache dump

[Qemu-devel] [qemu-devel] [PATCH v2] target-i386: Fix segment cache dump

[Tobias Markus]

When in Long Mode, cpu_x86_seg_cache() logs "DS16" because the Default operation size bit (D/B bit) is not set for Long Mode Data Segments since there are only Data Segments in Long Mode and no explicit 16/32/64-bit Descriptors.
This patch fixes this by checking the Long Mode Active bit of the hidden flags variable and logging "DS" if it is set. (I.e. in Long Mode all Data Segments are logged as "DS")

Signed-off-by: Tobias Markus <tobias@markus-regensburg.de>
---
v2: * Fix line wrapping as suggested in IRC
    * Break the line
Note that alternatively, Data Segments in Long Mode could be logged as "DS64" to avoid confusion about the missing 64 postfix. (But that would be, strictly speaking, wrong because there are only DSs and no DS16/32/64 in Long Mode.)
PS: This is my first contribution to an Open Source Project and I would be very happy about constructive feedback, especially about possible wrong line wrapping.
  target-i386/helper.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/target-i386/helper.c b/target-i386/helper.c
index bf3e2ac..db2f04f 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -147,7 +147,9 @@ cpu_x86_dump_seg_cache(CPUX86State *env, FILE *f, fprintf_function cpu_fprintf,
             cpu_fprintf(f, " [%c%c", (sc->flags & DESC_C_MASK) ? 'C' : '-',
                         (sc->flags & DESC_R_MASK) ? 'R' : '-');
         } else {
-            cpu_fprintf(f, (sc->flags & DESC_B_MASK) ? "DS  " : "DS16");
+            cpu_fprintf(f, (sc->flags & DESC_B_MASK ||
+                        env->hflags & HF_LMA_MASK)
+                        ? "DS  " : "DS16");
             cpu_fprintf(f, " [%c%c", (sc->flags & DESC_E_MASK) ? 'E' : '-',
                         (sc->flags & DESC_W_MASK) ? 'W' : '-');
         }
-- 
1.8.3.4

Re: [Qemu-devel] [qemu-devel] [PATCH v2] target-i386: Fix segment cache dump

[Richard Henderson]

On 08/23/2013 12:09 PM, Tobias Markus wrote:
> When in Long Mode, cpu_x86_seg_cache() logs "DS16" because the Default operation size bit (D/B bit) is not set for Long Mode Data Segments since there are only Data Segments in Long Mode and no explicit 16/32/64-bit Descriptors.
> This patch fixes this by checking the Long Mode Active bit of the hidden flags variable and logging "DS" if it is set. (I.e. in Long Mode all Data Segments are logged as "DS")
> 
> Signed-off-by: Tobias Markus <tobias@markus-regensburg.de>

Reviewed-by: Richard Henderson <rth@twiddle.net>

> +            cpu_fprintf(f, (sc->flags & DESC_B_MASK ||
> +                        env->hflags & HF_LMA_MASK)
> +                        ? "DS  " : "DS16");

Though we don't have anything in CODING_STYLE that mandates this,
IMO expressions shouldn't "unindent" in the middle like this.

Better as

           cpu_fprintf(f, (sc->flags & DESC_B_MASK
                           || env->hflags & HF_LMA_MASK
                           ? "DS  " : "DS16"));

but that's just me and emacs...


r~

Re: [Qemu-devel] [PATCH v2] target-i386: Fix segment cache dump

[Tobias Markus]

On 08/23/2013 10:01 PM, Richard Henderson wrote:
> On 08/23/2013 12:09 PM, Tobias Markus wrote:
>> When in Long Mode, cpu_x86_seg_cache() logs "DS16" because the Default operation size bit (D/B bit) is not set for Long Mode Data Segments since there are only Data Segments in Long Mode and no explicit 16/32/64-bit Descriptors.
>> This patch fixes this by checking the Long Mode Active bit of the hidden flags variable and logging "DS" if it is set. (I.e. in Long Mode all Data Segments are logged as "DS")
>>
>> Signed-off-by: Tobias Markus <tobias@markus-regensburg.de>
> 
> Reviewed-by: Richard Henderson <rth@twiddle.net>
> 
>> +            cpu_fprintf(f, (sc->flags & DESC_B_MASK ||
>> +                        env->hflags & HF_LMA_MASK)
>> +                        ? "DS  " : "DS16");
> 
> Though we don't have anything in CODING_STYLE that mandates this,
> IMO expressions shouldn't "unindent" in the middle like this.
Other similar cpu_fprintf()s also "unindent"ed, so I aimed for consistency.
I could submit a seperate patch to properly indent the relevant lines, but that seems like a seperate discussion. (Would be better to add it to CODING_STYLE, but that is obviously non-trivial and there are possibly many more files affected.)
> 
> Better as
> 
>            cpu_fprintf(f, (sc->flags & DESC_B_MASK
>                            || env->hflags & HF_LMA_MASK)
>                            ? "DS  " : "DS16");
> 
> but that's just me and emacs...
> 
> 
> r~
> 
Alternatively, for readability:
+            cpu_fprintf(f, (sc->flags & DESC_B_MASK || env->hflags & HF_LMA_MASK)
+                            ? "DS  " : "DS16");
The upper line would be 82 characters long. I'm not sure how strictly line width is enforced.
(Other lines in that file (target-i386/helper.c) also exceed the width limit.)

Re: [Qemu-devel] [PATCH v2] target-i386: Fix segment cache dump

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 725 bytes --]

On 08/23/2013 03:39 PM, Tobias Markus wrote:
> Alternatively, for readability:
> +            cpu_fprintf(f, (sc->flags & DESC_B_MASK || env->hflags & HF_LMA_MASK)
> +                            ? "DS  " : "DS16");
> The upper line would be 82 characters long. I'm not sure how strictly line width is enforced.
> (Other lines in that file (target-i386/helper.c) also exceed the width limit.)

This instance can get below 80 with one more line wrap:
+            cpu_fprintf(f,
+                        (sc->flags & DESC_B_MASK || env->hflags &
HF_LMA_MASK)
+                        ? "DS  " : "DS16");

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [qemu-devel] [PATCH v2] target-i386: Fix segment cache dump

[Peter Maydell]

On 23 August 2013 20:09, Tobias Markus <tobias@markus-regensburg.de> wrote:
> When in Long Mode, cpu_x86_seg_cache() logs "DS16" because the Default operation size bit (D/B bit) is not set for Long Mode Data Segments since there are only Data Segments in Long Mode and no explicit 16/32/64-bit Descriptors.
> This patch fixes this by checking the Long Mode Active bit of the hidden flags variable and logging "DS" if it is set. (I.e. in Long Mode all Data Segments are logged as "DS")

If you need to do another version of this patch, it would be nice
to (manually) wrap the commit message and your below-the-"---"
comments at about column 75. Otherwise whoever commits
the patch could maybe fix up the message.

thanks
-- PMM