Re: [PATCH 08/10] hw/core/cpu: Pass CPUArchState to set/get_pc() handlers

[Paolo Bonzini <pbonzini@redhat.com>]

On 11/15/24 16:54, Peter Maydell wrote:
> On Fri, 15 Nov 2024 at 15:22, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> CPUClass set_pc() and get_pc() handlers are target specific.
>> Rather than passing a generic CPUState and forcing QOM casts,
>> we can directly pass the target CPUArchState, simplifying.
> 
>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
>> index db8a6fbc6e..70f5f8c3bf 100644
>> --- a/include/hw/core/cpu.h
>> +++ b/include/hw/core/cpu.h
>> @@ -160,8 +160,8 @@ struct CPUClass {
>>       int64_t (*get_arch_id)(CPUState *cpu);
>>       bool (*cpu_persistent_status)(CPUState *cpu);
>>       bool (*cpu_enabled_status)(CPUState *cpu);
>> -    void (*set_pc)(CPUState *cpu, vaddr value);
>> -    vaddr (*get_pc)(CPUState *cpu);
>> +    void (*set_pc)(CPUArchState *env, vaddr value);
>> +    vaddr (*get_pc)(CPUArchState *env);
>>       int (*gdb_read_register)(CPUState *cpu, GByteArray *buf, int reg);
>>       int (*gdb_write_register)(CPUState *cpu, uint8_t *buf, int reg);
>>       vaddr (*gdb_adjust_breakpoint)(CPUState *cpu, vaddr addr);
> 
> This is effectively the table of methods for the CPUClass
> class. I think that methods on class A should take a pointer to the
> object of that type, not to something else.

Yeah, the diffstat is enticing but I agree.  It's particularly confusing 
because there isn't a single type called CPUArchState*, it's different 
depending on which target/ subdirectory you're compiling. In a 
multi-target binary it'd have to be void*, and then you get back the casts.


Paolo