Signed-off-by: Claudio Fontana <cfontana@suse.de>
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/gdbstub.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/target/i386/gdbstub.c b/target/i386/gdbstub.c
index 41e265fc67..9f505d6ee3 100644
--- a/target/i386/gdbstub.c
+++ b/target/i386/gdbstub.c
@@ -383,26 +383,38 @@ int x86_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
 
         case IDX_CTL_CR0_REG:
             if (env->hflags & HF_CS64_MASK) {
+#ifdef CONFIG_SOFTMMU
                 cpu_x86_update_cr0(env, ldq_p(mem_buf));
+#endif
                 return 8;
             }
+#ifdef CONFIG_SOFTMMU
             cpu_x86_update_cr0(env, ldl_p(mem_buf));
+#endif
             return 4;
 
         case IDX_CTL_CR2_REG:
             if (env->hflags & HF_CS64_MASK) {
+#ifdef CONFIG_SOFTMMU
                 env->cr[2] = ldq_p(mem_buf);
+#endif
                 return 8;
             }
+#ifdef CONFIG_SOFTMMU
             env->cr[2] = ldl_p(mem_buf);
+#endif
             return 4;
 
         case IDX_CTL_CR3_REG:
             if (env->hflags & HF_CS64_MASK) {
+#ifdef CONFIG_SOFTMMU
                 cpu_x86_update_cr3(env, ldq_p(mem_buf));
+#endif
                 return 8;
             }
+#ifdef CONFIG_SOFTMMU
             cpu_x86_update_cr3(env, ldl_p(mem_buf));
+#endif
             return 4;
 
         case IDX_CTL_CR4_REG:
@@ -427,10 +439,14 @@ int x86_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
 
         case IDX_CTL_EFER_REG:
             if (env->hflags & HF_CS64_MASK) {
+#ifdef CONFIG_SOFTMMU
                 cpu_load_efer(env, ldq_p(mem_buf));
+#endif
                 return 8;
             }
+#ifdef CONFIG_SOFTMMU
             cpu_load_efer(env, ldl_p(mem_buf));
+#endif
             return 4;
 
         }
-- 
2.26.2

On 2/24/21 5:34 AM, Claudio Fontana wrote:
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target/i386/gdbstub.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/target/i386/gdbstub.c b/target/i386/gdbstub.c
> index 41e265fc67..9f505d6ee3 100644
> --- a/target/i386/gdbstub.c
> +++ b/target/i386/gdbstub.c
> @@ -383,26 +383,38 @@ int x86_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
>  
>          case IDX_CTL_CR0_REG:
>              if (env->hflags & HF_CS64_MASK) {
> +#ifdef CONFIG_SOFTMMU
>                  cpu_x86_update_cr0(env, ldq_p(mem_buf));
> +#endif
>                  return 8;
>              }
> +#ifdef CONFIG_SOFTMMU
>              cpu_x86_update_cr0(env, ldl_p(mem_buf));
> +#endif
>              return 4;

It would be nice to do all these with rather less ifdefs.
And let's correctly use !CONFIG_USER_ONLY.

Without adding more stubs, may I suggest a new helper:

static target_ulong read_long_cs64(env, buf, len)
{
#ifdef TARGET_X86_64
    if (env->hflags & HF_CS64_MASK) {
        *len = 8;
        return ldq_p(buf);
    }
#endif
    *len = 4;
    return ldl_p(buf);
}

which, even by itself allows some cleanup in this function.
Then:

    case IDX_CTL_CR2_REG:
       tmp = read_long_cs64(env, mem_buf, &len);
#ifndef CONFIG_USER_ONLY
       env->cr[2] = tmp;
#endif
       return len;

which still has one ifdef, but not 2.


r~

On 2/25/21 5:19 AM, Richard Henderson wrote:
> On 2/24/21 5:34 AM, Claudio Fontana wrote:
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  target/i386/gdbstub.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/target/i386/gdbstub.c b/target/i386/gdbstub.c
>> index 41e265fc67..9f505d6ee3 100644
>> --- a/target/i386/gdbstub.c
>> +++ b/target/i386/gdbstub.c
>> @@ -383,26 +383,38 @@ int x86_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
>>  
>>          case IDX_CTL_CR0_REG:
>>              if (env->hflags & HF_CS64_MASK) {
>> +#ifdef CONFIG_SOFTMMU
>>                  cpu_x86_update_cr0(env, ldq_p(mem_buf));
>> +#endif
>>                  return 8;
>>              }
>> +#ifdef CONFIG_SOFTMMU
>>              cpu_x86_update_cr0(env, ldl_p(mem_buf));
>> +#endif
>>              return 4;
> 
> It would be nice to do all these with rather less ifdefs.
> And let's correctly use !CONFIG_USER_ONLY.
> 
> Without adding more stubs, may I suggest a new helper:
> 
> static target_ulong read_long_cs64(env, buf, len)
> {
> #ifdef TARGET_X86_64
>     if (env->hflags & HF_CS64_MASK) {
>         *len = 8;
>         return ldq_p(buf);
>     }
> #endif
>     *len = 4;
>     return ldl_p(buf);
> }

in the current code the

#ifdef TARGET_x86_64 is not there. Is it safe to use everywhere?

should we do a matching:

static int gdb_read_reg_cs64(CPUX86State *env, GByteArray *buf, target_ulong val)
{
    if ((env->hflags & HF_CS64_MASK) || GDB_FORCE_64) {
        return gdb_get_reg64(buf, val);
    }
    return gdb_get_reg32(buf, val);
}

?

Should we add the #ifdef TARGET_X86_64 here as well?

Thanks,

Claudio

> 
> which, even by itself allows some cleanup in this function.
> Then:
> 
>     case IDX_CTL_CR2_REG:
>        tmp = read_long_cs64(env, mem_buf, &len);
> #ifndef CONFIG_USER_ONLY
>        env->cr[2] = tmp;
> #endif
>        return len;
> 
> which still has one ifdef, but not 2.
> 
> 
> r~
> 

On 2/25/21 12:55 AM, Claudio Fontana wrote:
> On 2/25/21 5:19 AM, Richard Henderson wrote:
>> On 2/24/21 5:34 AM, Claudio Fontana wrote:
>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>  target/i386/gdbstub.c | 16 ++++++++++++++++
>>>  1 file changed, 16 insertions(+)
>>>
>>> diff --git a/target/i386/gdbstub.c b/target/i386/gdbstub.c
>>> index 41e265fc67..9f505d6ee3 100644
>>> --- a/target/i386/gdbstub.c
>>> +++ b/target/i386/gdbstub.c
>>> @@ -383,26 +383,38 @@ int x86_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
>>>  
>>>          case IDX_CTL_CR0_REG:
>>>              if (env->hflags & HF_CS64_MASK) {
>>> +#ifdef CONFIG_SOFTMMU
>>>                  cpu_x86_update_cr0(env, ldq_p(mem_buf));
>>> +#endif
>>>                  return 8;
>>>              }
>>> +#ifdef CONFIG_SOFTMMU
>>>              cpu_x86_update_cr0(env, ldl_p(mem_buf));
>>> +#endif
>>>              return 4;
>>
>> It would be nice to do all these with rather less ifdefs.
>> And let's correctly use !CONFIG_USER_ONLY.
>>
>> Without adding more stubs, may I suggest a new helper:
>>
>> static target_ulong read_long_cs64(env, buf, len)
>> {
>> #ifdef TARGET_X86_64
>>     if (env->hflags & HF_CS64_MASK) {
>>         *len = 8;
>>         return ldq_p(buf);
>>     }
>> #endif
>>     *len = 4;
>>     return ldl_p(buf);
>> }
> 
> in the current code the
> 
> #ifdef TARGET_x86_64 is not there. Is it safe to use everywhere?

It'll never be set unless TARGET_X86_64.  Also, it *is* used in other tests for
the mask.  I do wonder if we should have CS64_MASK defined to 0 for
!TARGET_X86_64, so that (X & 0) -> 0.

> should we do a matching:
> 
> static int gdb_read_reg_cs64(CPUX86State *env, GByteArray *buf, target_ulong val)
> {
>     if ((env->hflags & HF_CS64_MASK) || GDB_FORCE_64) {

I should think so, except...  that FORCE_64 makes the previous test useless.  I
have no idea what's going on here.


r~

On 2/26/21 5:05 AM, Richard Henderson wrote:
> On 2/25/21 12:55 AM, Claudio Fontana wrote:
>> On 2/25/21 5:19 AM, Richard Henderson wrote:
>>> On 2/24/21 5:34 AM, Claudio Fontana wrote:
>>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>> ---
>>>>  target/i386/gdbstub.c | 16 ++++++++++++++++
>>>>  1 file changed, 16 insertions(+)
>>>>
>>>> diff --git a/target/i386/gdbstub.c b/target/i386/gdbstub.c
>>>> index 41e265fc67..9f505d6ee3 100644
>>>> --- a/target/i386/gdbstub.c
>>>> +++ b/target/i386/gdbstub.c
>>>> @@ -383,26 +383,38 @@ int x86_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
>>>>  
>>>>          case IDX_CTL_CR0_REG:
>>>>              if (env->hflags & HF_CS64_MASK) {
>>>> +#ifdef CONFIG_SOFTMMU
>>>>                  cpu_x86_update_cr0(env, ldq_p(mem_buf));
>>>> +#endif
>>>>                  return 8;
>>>>              }
>>>> +#ifdef CONFIG_SOFTMMU
>>>>              cpu_x86_update_cr0(env, ldl_p(mem_buf));
>>>> +#endif
>>>>              return 4;
>>>
>>> It would be nice to do all these with rather less ifdefs.
>>> And let's correctly use !CONFIG_USER_ONLY.
>>>
>>> Without adding more stubs, may I suggest a new helper:
>>>
>>> static target_ulong read_long_cs64(env, buf, len)
>>> {
>>> #ifdef TARGET_X86_64
>>>     if (env->hflags & HF_CS64_MASK) {
>>>         *len = 8;
>>>         return ldq_p(buf);
>>>     }
>>> #endif
>>>     *len = 4;
>>>     return ldl_p(buf);
>>> }
>>
>> in the current code the
>>
>> #ifdef TARGET_x86_64 is not there. Is it safe to use everywhere?
> 
> It'll never be set unless TARGET_X86_64.  Also, it *is* used in other tests for

Right, there might be a reason for it (some instances are with the #ifdef TARGET_x86_64, some without)..?

> the mask.  I do wonder if we should have CS64_MASK defined to 0 for
> !TARGET_X86_64, so that (X & 0) -> 0.> 
>> should we do a matching:
>>
>> static int gdb_read_reg_cs64(CPUX86State *env, GByteArray *buf, target_ulong val)
>> {
>>     if ((env->hflags & HF_CS64_MASK) || GDB_FORCE_64) {
> 
> I should think so, except...  that FORCE_64 makes the previous test useless.  I
> have no idea what's going on here.

#ifdef TARGET_X86_64
#define GDB_FORCE_64 1
#else
#define GDB_FORCE_64 0
#endif

So for TARGET_X86_64, GDB_FORCE_64 is always 1.

Maybe the flags is there for when a cpu switches between modes? (32vs64 bit)?

I'll make a conservative patch that does not risk changing the behavior (at least in the intention).

> 
> 
> r~
> 

On 2/26/21 1:22 AM, Claudio Fontana wrote:
>> I should think so, except...  that FORCE_64 makes the previous test useless.  I
>> have no idea what's going on here.
> 
> #ifdef TARGET_X86_64
> #define GDB_FORCE_64 1
> #else
> #define GDB_FORCE_64 0
> #endif
> 
> So for TARGET_X86_64, GDB_FORCE_64 is always 1.
> 
> Maybe the flags is there for when a cpu switches between modes? (32vs64 bit)?

But of course the only cpu that could switch is TARGET_X86_64.
So... as I said, I have no idea what's going on.

> I'll make a conservative patch that does not risk changing the behavior (at least in the intention).

Sure.


r~