diff mbox series

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

Message ID 20250305005225.95051-2-philmd@linaro.org
State New
Headers show
Series qemu: Remove TARGET_NAME definition | expand

Commit Message

Philippe Mathieu-Daudé March 5, 2025, 12:52 a.m. UTC 
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.

[*] https://lore.kernel.org/qemu-devel/e514d6db-781d-4afe-b057-9046c70044dc@redhat.com/

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

Comments

Pierrick Bouvier March 5, 2025, 1:20 a.m. UTC | #1 
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.

> [*] https://lore.kernel.org/qemu-devel/e514d6db-781d-4afe-b057-9046c70044dc@redhat.com/
> 
> 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',
Philippe Mathieu-Daudé March 5, 2025, 1:33 a.m. UTC | #2 
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.

>> 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',
>
Pierrick Bouvier March 5, 2025, 1:41 a.m. UTC | #3 
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',
>>
>
diff mbox series

Patch

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',