Re: [PATCH v22 16/17] i386: gdbstub: only write CR0/CR2/CR3/EFER for SOFTMMU

[Claudio Fontana]

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