Re: [PATCH 03/10] accel/tcg: move i386 halt handling to sysemu_ops

[Claudio Fontana <cfontana@suse.de>]

On 3/20/23 16:34, Philippe Mathieu-Daudé wrote:
> On 20/3/23 16:23, Claudio Fontana wrote:
>> Hi Alex, all,
>>
>> again, this moves TCG-only code to common code, no?
> 
> Oh, good point.
> 
>> Even if this happens to work, the idea is to avoid adding unneeded accel TCG code to a KVM-only binary.
> 
> Could yet another AccelSysemuCPUOps *accel struct in SysemuCPUOps
> help being stricter? ...

Just a thought, in general I wonder if we could devise a less error prone way to keep things in the right place.
Just thinking out loud here, something like a QEMU_ATTRIBUTE_TCG, _KVM, ... to add to symbols to avoid ending up in the wrong binary.

Keeping in mind all these dimensions is probably very taxing, maybe getting some support from the build system would be beneficial,
checking that a build requested with specific features contains only compatible objects.

Any ideas?

Ciao,

C

> 
>> We need to keep in mind all dimensions when we do refactorings:
>>
>> user-mode vs sysemu,
>> the architecture,
>> the accel, in particular tcg, non-tcg (which could be not compiled in, built-in, or loaded as separate module).
>>
>> In many cases, testing with --disable-tcg --enable-kvm helps to avoid breakages,
>> but it is possible also to move in unneeded code in a way that does not generate compile or link-time errors, so we need to be a bit alert to that.
>>
>> Ciao,
>>
>> C
>>
>>
>> On 3/20/23 11:10, Alex Bennée wrote:
>>> We don't want to be polluting the core run loop code with target
>>> specific handling, punt it to sysemu_ops where it belongs.
>>>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> ---
>>>   include/hw/core/sysemu-cpu-ops.h |  5 +++++
>>>   target/i386/cpu-internal.h       |  1 +
>>>   accel/tcg/cpu-exec.c             | 14 +++-----------
>>>   target/i386/cpu-sysemu.c         | 12 ++++++++++++
>>>   target/i386/cpu.c                |  1 +
>>>   5 files changed, 22 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/include/hw/core/sysemu-cpu-ops.h b/include/hw/core/sysemu-cpu-ops.h
>>> index ee169b872c..c9d30172c4 100644
>>> --- a/include/hw/core/sysemu-cpu-ops.h
>>> +++ b/include/hw/core/sysemu-cpu-ops.h
>>> @@ -48,6 +48,11 @@ typedef struct SysemuCPUOps {
>>>        * GUEST_PANICKED events.
>>>        */
>>>       GuestPanicInformation* (*get_crash_info)(CPUState *cpu);
>>> +    /**
>>> +     * @handle_cpu_halt: Callback for special handling during cpu_handle_halt()
>>> +     * @cs: The CPUState
>>> +     */
> 
> Perhaps insert within a 'tcg' structure for now.
> 
>      #ifdef CONFIG_TCG
>      struct {
> 
>>> +    void (*handle_cpu_halt)(CPUState *cpu);
> 
>      } tcg;
>      #endif
> 
> Then we could extract as accel.
> 
>>>       /**
>>>        * @write_elf32_note: Callback for writing a CPU-specific ELF note to a
>>>        * 32-bit VM coredump.
> 
>