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

[Claudio Fontana]

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