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

[Richard Henderson]

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~