Re: [RFC PATCH 01/11] system: Extract target-specific globals to their own compilation unit

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 3/4/25 17:33, Philippe Mathieu-Daudé wrote:
> On 5/3/25 02:20, Pierrick Bouvier wrote:
>> On 3/4/25 16:52, Philippe Mathieu-Daudé wrote:
>>> We shouldn't use target specific globals for machine properties.
>>> These ones could be desugarized, as explained in [*]. While
>>> certainly doable, not trivial nor my priority for now. Just move
>>> them to a different file to clarify they are *globals*, like the
>>> generic globals residing in system/globals.c.
>>>
>>
>> Maybe those could be set globally to the default:
>> int graphic_width = 800;
>> int graphic_height = 600;
>> int graphic_depth = 32;
>>
>> And override them for sparc and m68k in qemu_init_arch_modules function,
>> using qemu_arch_name().
>> This way, no need to have a new file to hold them.
> 
> This is not what Paolo explained ...
> 
>>
>>> [*] https://lore.kernel.org/qemu-devel/e514d6db-781d-4afe-
>>> b057-9046c70044dc@redhat.com/
> 
> ... here ^, but maybe I misunderstood him.
> 
> Doing the '-g' CLI change is not my priority, and I welcome
> any developer willing to help :) Here I'm just trying to make
> sense of that arch_init.c file.
> 

It's not what I suggested, and the final state (globals are set with a 
value), should be the same.

It's just
Eventually, the
if (sparc) {
    graphic_* = ...'
} else if (m86k) {
    graphic_* = ...'
}

Can be moved somewhere else (sooner) if needed.
It's just to get rid of TARGET_SPARC and TARGET_M68K on the way, not to 
change anything else.

>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>    system/arch_init.c      | 14 --------------
>>>    system/globals-target.c | 24 ++++++++++++++++++++++++
>>>    system/meson.build      |  1 +
>>>    3 files changed, 25 insertions(+), 14 deletions(-)
>>>    create mode 100644 system/globals-target.c
>>>
>>> diff --git a/system/arch_init.c b/system/arch_init.c
>>> index d2c32f84887..ea0842b7410 100644
>>> --- a/system/arch_init.c
>>> +++ b/system/arch_init.c
>>> @@ -25,20 +25,6 @@
>>>    #include "qemu/module.h"
>>>    #include "system/arch_init.h"
>>> -#ifdef TARGET_SPARC
>>> -int graphic_width = 1024;
>>> -int graphic_height = 768;
>>> -int graphic_depth = 8;
>>> -#elif defined(TARGET_M68K)
>>> -int graphic_width = 800;
>>> -int graphic_height = 600;
>>> -int graphic_depth = 8;
>>> -#else
>>> -int graphic_width = 800;
>>> -int graphic_height = 600;
>>> -int graphic_depth = 32;
>>> -#endif
>>> -
>>>    const uint32_t arch_type = QEMU_ARCH;
>>>    void qemu_init_arch_modules(void)
>>> diff --git a/system/globals-target.c b/system/globals-target.c
>>> new file mode 100644
>>> index 00000000000..989720591e7
>>> --- /dev/null
>>> +++ b/system/globals-target.c
>>> @@ -0,0 +1,24 @@
>>> +/*
>>> + * Global variables that should not exist (target specific)
>>> + *
>>> + * Copyright (c) 2003-2008 Fabrice Bellard
>>> + *
>>> + * SPDX-License-Identifier: MIT
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "system/system.h"
>>> +
>>> +#ifdef TARGET_SPARC
>>> +int graphic_width = 1024;
>>> +int graphic_height = 768;
>>> +int graphic_depth = 8;
>>> +#elif defined(TARGET_M68K)
>>> +int graphic_width = 800;
>>> +int graphic_height = 600;
>>> +int graphic_depth = 8;
>>> +#else
>>> +int graphic_width = 800;
>>> +int graphic_height = 600;
>>> +int graphic_depth = 32;
>>> +#endif
>>> diff --git a/system/meson.build b/system/meson.build
>>> index 4952f4b2c7d..181195410fb 100644
>>> --- a/system/meson.build
>>> +++ b/system/meson.build
>>> @@ -1,6 +1,7 @@
>>>    specific_ss.add(when: 'CONFIG_SYSTEM_ONLY', if_true: [files(
>>>      'arch_init.c',
>>>      'ioport.c',
>>> +  'globals-target.c',
>>>      'memory.c',
>>>      'physmem.c',
>>>      'watchpoint.c',
>>
>