[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v22 16/17] i386: gdbstub: only write CR0/CR2/CR3/EFER for SOF
From: |
Claudio Fontana |
Subject: |
Re: [PATCH v22 16/17] i386: gdbstub: only write CR0/CR2/CR3/EFER for SOFTMMU |
Date: |
Fri, 26 Feb 2021 10:22:46 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 |
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~
>
- Re: [PATCH v22 14/17] i386: split seg_helper into user-only and sysemu parts, (continued)
[PATCH v22 15/17] i386: split off sysemu part of cpu.c, Claudio Fontana, 2021/02/24
Re: [PATCH v22 00/17] i386 cleanup PART 2, no-reply, 2021/02/24
Re: [PATCH v22 00/17] i386 cleanup PART 2, Philippe Mathieu-Daudé, 2021/02/24