[PATCH 00/13] single-binary: start make hw/arm/ common (boot.c)

[PATCH 00/13] single-binary: start make hw/arm/ common (boot.c)

[Pierrick Bouvier]

This series focuses on removing compilation units duplication in hw/arm. We
start with this architecture because it should not be too hard to transform it,
and should give us some good hints on the difficulties we'll meet later.

We first start by making changes in global headers to be able to not rely on
specific target defines. We then focus on removing those defines from
target/arm/cpu.h.

From there, we modify build system to create a new hw common library (per base
architecture, "arm" in this case), instead of compiling the same files for every
target.

Finally, we can declare hw/arm/boot.c common as a first step for this subsystem.

This series needs to be applied on top of
https://lore.kernel.org/qemu-devel/20250317183417.285700-19-pierrick.bouvier@linaro.org/
to compile.

Pierrick Bouvier (13):
  exec/cpu-all: restrict BSWAP_NEEDED to target specific code
  exec/cpu-all: restrict compile time assert to target specific code
  exec/target_page: runtime defintion for TARGET_PAGE_BITS_MIN
  exec/cpu-all: allow to include specific cpu
  target/arm/cpu: move KVM_HAVE_MCE_INJECTION to kvm-all.c file directly
  exec/poison: KVM_HAVE_MCE_INJECTION can now be poisoned
  target/arm/cpu: always define kvm related registers
  target/arm/cpu: flags2 is always uint64_t
  target/arm/cpu: define ARM_MAX_VQ once for aarch32 and aarch64
  target/arm/cpu: define same set of registers for aarch32 and aarch64
  target/arm/cpu: remove inline stubs for aarch32 emulation
  meson: add common hw files
  hw/arm/boot: make compilation unit hw common

 meson.build                | 36 +++++++++++++++++++++++++++++++++++-
 include/exec/cpu-all.h     | 12 ++++++++++--
 include/exec/poison.h      |  2 ++
 include/exec/target_page.h |  3 +++
 include/system/kvm.h       |  2 --
 target/arm/cpu.h           | 28 +++-------------------------
 accel/kvm/kvm-all.c        |  4 ++++
 hw/arm/boot.c              |  1 +
 target/arm/helper.c        |  6 ++++++
 target/arm/tcg/hflags.c    |  4 ++--
 hw/arm/meson.build         |  5 ++++-
 11 files changed, 70 insertions(+), 33 deletions(-)

-- 
2.39.5

[PATCH 01/13] exec/cpu-all: restrict BSWAP_NEEDED to target specific code

[Pierrick Bouvier]

This identifier is already poisoned, so it can't be used from common
code anyway.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 include/exec/cpu-all.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index 902ca1f3c7b..6dd71eb0de9 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -34,8 +34,10 @@
  * TARGET_BIG_ENDIAN : same for the target cpu
  */
 
-#if HOST_BIG_ENDIAN != TARGET_BIG_ENDIAN
-#define BSWAP_NEEDED
+#ifdef COMPILING_PER_TARGET
+# if HOST_BIG_ENDIAN != TARGET_BIG_ENDIAN
+#  define BSWAP_NEEDED
+# endif
 #endif
 
 /* page related stuff */
-- 
2.39.5

Re: [PATCH 01/13] exec/cpu-all: restrict BSWAP_NEEDED to target specific code

[Richard Henderson]

On 3/17/25 21:51, Pierrick Bouvier wrote:
> This identifier is already poisoned, so it can't be used from common
> code anyway.
> 
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   include/exec/cpu-all.h | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)

I'll give you a
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

because it's quick and correct.  However, there are only 8 actual uses within the entire 
tree (discounting comments), and all could be replaced by

> +# if HOST_BIG_ENDIAN != TARGET_BIG_ENDIAN


r~

Re: [PATCH 01/13] exec/cpu-all: restrict BSWAP_NEEDED to target specific code

[Pierrick Bouvier]

On 3/18/25 14:41, Richard Henderson wrote:
> On 3/17/25 21:51, Pierrick Bouvier wrote:
>> This identifier is already poisoned, so it can't be used from common
>> code anyway.
>>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>    include/exec/cpu-all.h | 6 ++++--
>>    1 file changed, 4 insertions(+), 2 deletions(-)
> 
> I'll give you a
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> because it's quick and correct.  However, there are only 8 actual uses within the entire
> tree (discounting comments), and all could be replaced by
> 

I hesitated to do it and get rid of BSWAP_NEEDED completely, so I'll do 
the replace.

>> +# if HOST_BIG_ENDIAN != TARGET_BIG_ENDIAN
> 
> 
> r~

[PATCH 02/13] exec/cpu-all: restrict compile time assert to target specific code

[Pierrick Bouvier]

TLB_FLAGS defines are based on TARGET_PAGE_BITS_MIN, which is defined
for every target.

In the next commit, we'll introduce a non-static define for
TARGET_PAGE_BITS_MIN in common code, thus, we can't check this at
compile time, except in target specific code.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 include/exec/cpu-all.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index 6dd71eb0de9..7c6c47c43ed 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -112,8 +112,10 @@ static inline int cpu_mmu_index(CPUState *cs, bool ifetch)
 
 #define TLB_SLOW_FLAGS_MASK  (TLB_BSWAP | TLB_WATCHPOINT | TLB_CHECK_ALIGNED)
 
+#ifdef COMPILING_PER_TARGET
 /* The two sets of flags must not overlap. */
 QEMU_BUILD_BUG_ON(TLB_FLAGS_MASK & TLB_SLOW_FLAGS_MASK);
+#endif
 
 #endif /* !CONFIG_USER_ONLY */
 
-- 
2.39.5

[PATCH 03/13] exec/target_page: runtime defintion for TARGET_PAGE_BITS_MIN

[Pierrick Bouvier]

We introduce later a mechanism to skip cpu definitions inclusion, so we
can detect it here, and call the correct runtime function instead.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 include/exec/target_page.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/exec/target_page.h b/include/exec/target_page.h
index 8e89e5cbe6f..aeddb25c743 100644
--- a/include/exec/target_page.h
+++ b/include/exec/target_page.h
@@ -40,6 +40,9 @@ extern const TargetPageBits target_page;
 #  define TARGET_PAGE_MASK   ((TARGET_PAGE_TYPE)target_page.mask)
 # endif
 # define TARGET_PAGE_SIZE    (-(int)TARGET_PAGE_MASK)
+# ifndef TARGET_PAGE_BITS_MIN
+#  define TARGET_PAGE_BITS_MIN qemu_target_page_bits_min()
+# endif
 #else
 # define TARGET_PAGE_BITS_MIN TARGET_PAGE_BITS
 # define TARGET_PAGE_SIZE    (1 << TARGET_PAGE_BITS)
-- 
2.39.5

[PATCH 04/13] exec/cpu-all: allow to include specific cpu

[Pierrick Bouvier]

Including "cpu.h" from code that is not compiled per target is ambiguous
by definition. Thus we introduce a conditional include, to allow every
architecture to set this, to point to the correct definition.

hw/X or target/X will now include directly "target/X/cpu.h", and
"target/X/cpu.h" will define CPU_INCLUDE to itself.
We already do this change for arm cpu as part of this commit.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 include/exec/cpu-all.h | 4 ++++
 target/arm/cpu.h       | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index 7c6c47c43ed..1a756c0cfb3 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -46,7 +46,11 @@
 
 CPUArchState *cpu_copy(CPUArchState *env);
 
+#ifdef CPU_INCLUDE
+#include CPU_INCLUDE
+#else
 #include "cpu.h"
+#endif
 
 #ifdef CONFIG_USER_ONLY
 
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index a8177c6c2e8..7aeb012428c 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -31,6 +31,8 @@
 #include "target/arm/multiprocessing.h"
 #include "target/arm/gtimer.h"
 
+#define CPU_INCLUDE "target/arm/cpu.h"
+
 #ifdef TARGET_AARCH64
 #define KVM_HAVE_MCE_INJECTION 1
 #endif
-- 
2.39.5

Re: [PATCH 04/13] exec/cpu-all: allow to include specific cpu

[Richard Henderson]

On 3/17/25 21:51, Pierrick Bouvier wrote:
> Including "cpu.h" from code that is not compiled per target is ambiguous
> by definition. Thus we introduce a conditional include, to allow every
> architecture to set this, to point to the correct definition.
> 
> hw/X or target/X will now include directly "target/X/cpu.h", and
> "target/X/cpu.h" will define CPU_INCLUDE to itself.
> We already do this change for arm cpu as part of this commit.
> 
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   include/exec/cpu-all.h | 4 ++++
>   target/arm/cpu.h       | 2 ++
>   2 files changed, 6 insertions(+)
> 
> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index 7c6c47c43ed..1a756c0cfb3 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -46,7 +46,11 @@
>   
>   CPUArchState *cpu_copy(CPUArchState *env);
>   
> +#ifdef CPU_INCLUDE
> +#include CPU_INCLUDE
> +#else
>   #include "cpu.h"
> +#endif
>   
>   #ifdef CONFIG_USER_ONLY
>   
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index a8177c6c2e8..7aeb012428c 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -31,6 +31,8 @@
>   #include "target/arm/multiprocessing.h"
>   #include "target/arm/gtimer.h"
>   
> +#define CPU_INCLUDE "target/arm/cpu.h"
> +
>   #ifdef TARGET_AARCH64
>   #define KVM_HAVE_MCE_INJECTION 1
>   #endif

This doesn't make any sense to me.  CPU_INCLUDE is defined within the very file that 
you're trying to include by avoiding "cpu.h".


r~

Re: [PATCH 04/13] exec/cpu-all: allow to include specific cpu

[Pierrick Bouvier]

On 3/18/25 15:11, Richard Henderson wrote:
> On 3/17/25 21:51, Pierrick Bouvier wrote:
>> Including "cpu.h" from code that is not compiled per target is ambiguous
>> by definition. Thus we introduce a conditional include, to allow every
>> architecture to set this, to point to the correct definition.
>>
>> hw/X or target/X will now include directly "target/X/cpu.h", and
>> "target/X/cpu.h" will define CPU_INCLUDE to itself.
>> We already do this change for arm cpu as part of this commit.
>>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>    include/exec/cpu-all.h | 4 ++++
>>    target/arm/cpu.h       | 2 ++
>>    2 files changed, 6 insertions(+)
>>
>> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
>> index 7c6c47c43ed..1a756c0cfb3 100644
>> --- a/include/exec/cpu-all.h
>> +++ b/include/exec/cpu-all.h
>> @@ -46,7 +46,11 @@
>>    
>>    CPUArchState *cpu_copy(CPUArchState *env);
>>    
>> +#ifdef CPU_INCLUDE
>> +#include CPU_INCLUDE
>> +#else
>>    #include "cpu.h"
>> +#endif
>>    
>>    #ifdef CONFIG_USER_ONLY
>>    
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index a8177c6c2e8..7aeb012428c 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -31,6 +31,8 @@
>>    #include "target/arm/multiprocessing.h"
>>    #include "target/arm/gtimer.h"
>>    
>> +#define CPU_INCLUDE "target/arm/cpu.h"
>> +
>>    #ifdef TARGET_AARCH64
>>    #define KVM_HAVE_MCE_INJECTION 1
>>    #endif
> 
> This doesn't make any sense to me.  CPU_INCLUDE is defined within the very file that
> you're trying to include by avoiding "cpu.h".
> 

Every target/X/cpu.h includes cpu-all.h, which includes "cpu.h" itself, 
relying on per target include path set by build system. Now we have 
common code, there is no "per target include path".

The other solutions are:
- build hw common libraries with per target include path, but I thought 
it was a good way to cleanup this, and not rely on this hidden 
dependency on the build system
- remove cpu.h inclusion from cpu-all.h, but it requires more 
modifications in other places.

I'm not sure which is the more desirable, compare to having this weird 
CPU_INCLUDE trick.

> 
> r~

Re: [PATCH 04/13] exec/cpu-all: allow to include specific cpu

[Richard Henderson]

On 3/18/25 15:16, Pierrick Bouvier wrote:
>> This doesn't make any sense to me.  CPU_INCLUDE is defined within the very file that
>> you're trying to include by avoiding "cpu.h".
>>
> 
> Every target/X/cpu.h includes cpu-all.h, which includes "cpu.h" itself, relying on per 
> target include path set by build system.

So, another solution would be to fix the silly include loop?


r~

Re: [PATCH 04/13] exec/cpu-all: allow to include specific cpu

[Pierrick Bouvier]

On 3/18/25 15:21, Richard Henderson wrote:
> On 3/18/25 15:16, Pierrick Bouvier wrote:
>>> This doesn't make any sense to me.  CPU_INCLUDE is defined within the very file that
>>> you're trying to include by avoiding "cpu.h".
>>>
>>
>> Every target/X/cpu.h includes cpu-all.h, which includes "cpu.h" itself, relying on per
>> target include path set by build system.
> 
> So, another solution would be to fix the silly include loop?
>

If you're ok with it, I'm willing to remove cpu-all.h completely (moving 
tlb flags bits in a new header), and fixing missing includes everywhere.

I just wanted to make sure it's an acceptable path before spending too 
much time on it.

> 
> r~

Re: [PATCH 04/13] exec/cpu-all: allow to include specific cpu

[Richard Henderson]

On 3/18/25 15:25, Pierrick Bouvier wrote:
> On 3/18/25 15:21, Richard Henderson wrote:
>> On 3/18/25 15:16, Pierrick Bouvier wrote:
>>>> This doesn't make any sense to me.  CPU_INCLUDE is defined within the very file that
>>>> you're trying to include by avoiding "cpu.h".
>>>>
>>>
>>> Every target/X/cpu.h includes cpu-all.h, which includes "cpu.h" itself, relying on per
>>> target include path set by build system.
>>
>> So, another solution would be to fix the silly include loop?
>>
> 
> If you're ok with it, I'm willing to remove cpu-all.h completely (moving tlb flags bits in 
> a new header), and fixing missing includes everywhere.
> 
> I just wanted to make sure it's an acceptable path before spending too much time on it.

I would very much like cpu-all.h to go away.

It looks like we have, on tcg-next:

(1) cpu_copy is linux-user only, and should go in linux-user/qemu.h.

(2) the TLB flags certainly deserve their own header.

(3) The QEMU_BUILD_BUG_ON assertions need not be done in a header,
     so long as there is *some* file that won't build if the assertions fail.
     Perhaps cpu-target.c is as good as any.


r~

Re: [PATCH 04/13] exec/cpu-all: allow to include specific cpu

[Pierrick Bouvier]

On 3/18/25 15:36, Richard Henderson wrote:
> On 3/18/25 15:25, Pierrick Bouvier wrote:
>> On 3/18/25 15:21, Richard Henderson wrote:
>>> On 3/18/25 15:16, Pierrick Bouvier wrote:
>>>>> This doesn't make any sense to me.  CPU_INCLUDE is defined within the very file that
>>>>> you're trying to include by avoiding "cpu.h".
>>>>>
>>>>
>>>> Every target/X/cpu.h includes cpu-all.h, which includes "cpu.h" itself, relying on per
>>>> target include path set by build system.
>>>
>>> So, another solution would be to fix the silly include loop?
>>>
>>
>> If you're ok with it, I'm willing to remove cpu-all.h completely (moving tlb flags bits in
>> a new header), and fixing missing includes everywhere.
>>
>> I just wanted to make sure it's an acceptable path before spending too much time on it.
> 
> I would very much like cpu-all.h to go away.
> 

Deal, I will complete the work, while being based on your current series 
(v2).

> It looks like we have, on tcg-next:
> 
> (1) cpu_copy is linux-user only, and should go in linux-user/qemu.h.
> 
> (2) the TLB flags certainly deserve their own header.
> 
> (3) The QEMU_BUILD_BUG_ON assertions need not be done in a header,
>       so long as there is *some* file that won't build if the assertions fail.
>       Perhaps cpu-target.c is as good as any.
> 

Yes, I noticed it, and chose #ifdef COMPILING_PER_TARGET workaround to 
not make a choice of where to move it.

> 
> r~

[PATCH 05/13] target/arm/cpu: move KVM_HAVE_MCE_INJECTION to kvm-all.c file directly

[Pierrick Bouvier]

This define is used only in accel/kvm/kvm-all.c, so we push directly the
definition there. Add more visibility to kvm_arch_on_sigbus_vcpu() to
allow removing this define from any header.

The only other architecture defining KVM_HAVE_MCE_INJECTION is i386,
which we can cleanup later.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 include/system/kvm.h | 2 --
 target/arm/cpu.h     | 4 ----
 accel/kvm/kvm-all.c  | 4 ++++
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/include/system/kvm.h b/include/system/kvm.h
index 716c7dcdf6b..b690dda1370 100644
--- a/include/system/kvm.h
+++ b/include/system/kvm.h
@@ -392,9 +392,7 @@ bool kvm_vcpu_id_is_valid(int vcpu_id);
 /* Returns VCPU ID to be used on KVM_CREATE_VCPU ioctl() */
 unsigned long kvm_arch_vcpu_id(CPUState *cpu);
 
-#ifdef KVM_HAVE_MCE_INJECTION
 void kvm_arch_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
-#endif
 
 void kvm_arch_init_irq_routing(KVMState *s);
 
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 7aeb012428c..23c2293f7d1 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -33,10 +33,6 @@
 
 #define CPU_INCLUDE "target/arm/cpu.h"
 
-#ifdef TARGET_AARCH64
-#define KVM_HAVE_MCE_INJECTION 1
-#endif
-
 #define EXCP_UDEF            1   /* undefined instruction */
 #define EXCP_SWI             2   /* software interrupt */
 #define EXCP_PREFETCH_ABORT  3
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index f89568bfa39..28de3990699 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -13,6 +13,10 @@
  *
  */
 
+#ifdef TARGET_AARCH64
+#define KVM_HAVE_MCE_INJECTION 1
+#endif
+
 #include "qemu/osdep.h"
 #include <sys/ioctl.h>
 #include <poll.h>
-- 
2.39.5

Re: [PATCH 05/13] target/arm/cpu: move KVM_HAVE_MCE_INJECTION to kvm-all.c file directly

[Richard Henderson]

On 3/17/25 21:51, Pierrick Bouvier wrote:
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index f89568bfa39..28de3990699 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -13,6 +13,10 @@
>    *
>    */
>   
> +#ifdef TARGET_AARCH64
> +#define KVM_HAVE_MCE_INJECTION 1
> +#endif
> +
>   #include "qemu/osdep.h"
>   #include <sys/ioctl.h>
>   #include <poll.h>

I think this define should go after all #includes, emphasizing that it only affects this file.

I think the #ifdef should use __aarch64__.  KVM is explicitly only for the host, so 
TARGET_AARCH64 really means the host is also AArch64.

I think you should go ahead and adjust x86_64 either with the same patch or immediately 
afterward.  There are only two users after all.


r~

Re: [PATCH 05/13] target/arm/cpu: move KVM_HAVE_MCE_INJECTION to kvm-all.c file directly

[Pierrick Bouvier]

On 3/18/25 15:19, Richard Henderson wrote:
> On 3/17/25 21:51, Pierrick Bouvier wrote:
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index f89568bfa39..28de3990699 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -13,6 +13,10 @@
>>     *
>>     */
>>    
>> +#ifdef TARGET_AARCH64
>> +#define KVM_HAVE_MCE_INJECTION 1
>> +#endif
>> +
>>    #include "qemu/osdep.h"
>>    #include <sys/ioctl.h>
>>    #include <poll.h>
> 
> I think this define should go after all #includes, emphasizing that it only affects this file.
> 
> I think the #ifdef should use __aarch64__.  KVM is explicitly only for the host, so
> TARGET_AARCH64 really means the host is also AArch64.
> 
> I think you should go ahead and adjust x86_64 either with the same patch or immediately
> afterward.  There are only two users after all.
> 

I'll adjust this for x86_64 in the same patch.

> 
> r~

[PATCH 06/13] exec/poison: KVM_HAVE_MCE_INJECTION can now be poisoned

[Pierrick Bouvier]

We prevent common code to use this define by mistake.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 include/exec/poison.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/exec/poison.h b/include/exec/poison.h
index 8ed04b31083..816f6f99d16 100644
--- a/include/exec/poison.h
+++ b/include/exec/poison.h
@@ -67,4 +67,6 @@
 #pragma GCC poison CONFIG_WHPX
 #pragma GCC poison CONFIG_XEN
 
+#pragma GCC poison KVM_HAVE_MCE_INJECTION
+
 #endif
-- 
2.39.5

Re: [PATCH 06/13] exec/poison: KVM_HAVE_MCE_INJECTION can now be poisoned

[Richard Henderson]

On 3/17/25 21:51, Pierrick Bouvier wrote:
> We prevent common code to use this define by mistake.
> 
> Signed-off-by: Pierrick Bouvier<pierrick.bouvier@linaro.org>
> ---
>   include/exec/poison.h | 2 ++
>   1 file changed, 2 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 07/13] target/arm/cpu: always define kvm related registers

[Pierrick Bouvier]

This does not hurt, even if they are not used.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 target/arm/cpu.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 23c2293f7d1..96f7801a239 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -971,7 +971,6 @@ struct ArchCPU {
      */
     uint32_t kvm_target;
 
-#ifdef CONFIG_KVM
     /* KVM init features for this CPU */
     uint32_t kvm_init_features[7];
 
@@ -984,7 +983,6 @@ struct ArchCPU {
 
     /* KVM steal time */
     OnOffAuto kvm_steal_time;
-#endif /* CONFIG_KVM */
 
     /* Uniprocessor system with MP extensions */
     bool mp_is_up;
-- 
2.39.5

Re: [PATCH 07/13] target/arm/cpu: always define kvm related registers

[Philippe Mathieu-Daudé]

On 18/3/25 05:51, Pierrick Bouvier wrote:
> This does not hurt, even if they are not used.

I'm not convinced by the rationale.

> 
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   target/arm/cpu.h | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 23c2293f7d1..96f7801a239 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -971,7 +971,6 @@ struct ArchCPU {
>        */
>       uint32_t kvm_target;
>   
> -#ifdef CONFIG_KVM
>       /* KVM init features for this CPU */
>       uint32_t kvm_init_features[7];
>   
> @@ -984,7 +983,6 @@ struct ArchCPU {
>   
>       /* KVM steal time */
>       OnOffAuto kvm_steal_time;
> -#endif /* CONFIG_KVM */

Maybe we need an opaque ArchAccelCpuState structure...

>   
>       /* Uniprocessor system with MP extensions */
>       bool mp_is_up;

Re: [PATCH 07/13] target/arm/cpu: always define kvm related registers

[Pierrick Bouvier]

On 3/18/25 11:14, Philippe Mathieu-Daudé wrote:
> On 18/3/25 05:51, Pierrick Bouvier wrote:
>> This does not hurt, even if they are not used.
> 
> I'm not convinced by the rationale.
> 
>>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>    target/arm/cpu.h | 2 --
>>    1 file changed, 2 deletions(-)
>>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index 23c2293f7d1..96f7801a239 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -971,7 +971,6 @@ struct ArchCPU {
>>         */
>>        uint32_t kvm_target;
>>    
>> -#ifdef CONFIG_KVM
>>        /* KVM init features for this CPU */
>>        uint32_t kvm_init_features[7];
>>    
>> @@ -984,7 +983,6 @@ struct ArchCPU {
>>    
>>        /* KVM steal time */
>>        OnOffAuto kvm_steal_time;
>> -#endif /* CONFIG_KVM */
> 
> Maybe we need an opaque ArchAccelCpuState structure...
> 

It's similar to the interesting question of how to expose some registers 
conditionnally.

We could put this in another struct, allocate if only if needed 
(kvm_enabled()), or just let it be present anytime like it is done with 
this patch.

I don't have a strong opinion, but having conditional presence here is 
just making things complicated without introducing any benefit. It does 
not prevent "unauthorized" access to it.

Now, if we start to have something more clean, implemented in another 
compilation units, only related to kvm, well, that could be useful. But 
we have to define the interface for that, add it to other architectures, 
and probably spend a few months in the middle where things are stuck here.

>>    
>>        /* Uniprocessor system with MP extensions */
>>        bool mp_is_up;
> 

[PATCH 08/13] target/arm/cpu: flags2 is always uint64_t

[Pierrick Bouvier]

Do not rely on target dependent type, but use a fixed type instead.
Since the original type is unsigned, it should be safe to extend its
size without any side effect.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 target/arm/cpu.h        | 2 +-
 target/arm/tcg/hflags.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 96f7801a239..27a0d4550f2 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -194,7 +194,7 @@ typedef struct ARMPACKey {
 /* See the commentary above the TBFLAG field definitions.  */
 typedef struct CPUARMTBFlags {
     uint32_t flags;
-    target_ulong flags2;
+    uint64_t flags2;
 } CPUARMTBFlags;
 
 typedef struct ARMMMUFaultInfo ARMMMUFaultInfo;
diff --git a/target/arm/tcg/hflags.c b/target/arm/tcg/hflags.c
index 8d79b8b7ae1..e51d9f7b159 100644
--- a/target/arm/tcg/hflags.c
+++ b/target/arm/tcg/hflags.c
@@ -506,8 +506,8 @@ void assert_hflags_rebuild_correctly(CPUARMState *env)
 
     if (unlikely(c.flags != r.flags || c.flags2 != r.flags2)) {
         fprintf(stderr, "TCG hflags mismatch "
-                        "(current:(0x%08x,0x" TARGET_FMT_lx ")"
-                        " rebuilt:(0x%08x,0x" TARGET_FMT_lx ")\n",
+                        "(current:(0x%08x,0x%016" PRIx64 ")"
+                        " rebuilt:(0x%08x,0x%016" PRIx64 ")\n",
                 c.flags, c.flags2, r.flags, r.flags2);
         abort();
     }
-- 
2.39.5

Re: [PATCH 08/13] target/arm/cpu: flags2 is always uint64_t

[Richard Henderson]

On 3/17/25 21:51, Pierrick Bouvier wrote:
> Do not rely on target dependent type, but use a fixed type instead.
> Since the original type is unsigned, it should be safe to extend its
> size without any side effect.
> 
> Signed-off-by: Pierrick Bouvier<pierrick.bouvier@linaro.org>
> ---
>   target/arm/cpu.h        | 2 +-
>   target/arm/tcg/hflags.c | 4 ++--
>   2 files changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

We can also remove the comment lower down about cs_base being
based on target_ulong, since that was changed some time ago:

exec/translation-block.h:    uint64_t cs_base;



r~

Re: [PATCH 08/13] target/arm/cpu: flags2 is always uint64_t

[Pierrick Bouvier]

On 3/18/25 15:40, Richard Henderson wrote:
> On 3/17/25 21:51, Pierrick Bouvier wrote:
>> Do not rely on target dependent type, but use a fixed type instead.
>> Since the original type is unsigned, it should be safe to extend its
>> size without any side effect.
>>
>> Signed-off-by: Pierrick Bouvier<pierrick.bouvier@linaro.org>
>> ---
>>    target/arm/cpu.h        | 2 +-
>>    target/arm/tcg/hflags.c | 4 ++--
>>    2 files changed, 3 insertions(+), 3 deletions(-)
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> We can also remove the comment lower down about cs_base being
> based on target_ulong, since that was changed some time ago:
> 
> exec/translation-block.h:    uint64_t cs_base;
> 
>

Sure.

I updated the comment to this:

   * We collect these two parts in CPUARMTBFlags where they are named
   * flags and flags2 respectively.
   *
- * The flags that are shared between all execution modes, TBFLAG_ANY,
- * are stored in flags.  The flags that are specific to a given mode
- * are stores in flags2.  Since cs_base is sized on the configured
- * address size, flags2 always has 64-bits for A64, and a minimum of
- * 32-bits for A32 and M32.
+ * The flags that are shared between all execution modes, TBFLAG_ANY, 
are stored
+ * in flags. The flags that are specific to a given mode are stored in 
flags2.
+ * flags2 always has 64-bits, even though only 32-bits are used for A32 
and M32.
   *
   * The bits for 32-bit A-profile and M-profile partially overlap:
   *


> 
> r~

[PATCH 09/13] target/arm/cpu: define ARM_MAX_VQ once for aarch32 and aarch64

[Pierrick Bouvier]

This will affect zregs field for aarch32.
This field is used for MVE and SVE implementations. MVE implementation
is clipping index value to 0 or 1 for zregs[*].d[],
so we should not touch the rest of data in this case anyway.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 target/arm/cpu.h | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 27a0d4550f2..00f78d64bd8 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -169,11 +169,7 @@ typedef struct ARMGenericTimer {
  * Align the data for use with TCG host vector operations.
  */
 
-#ifdef TARGET_AARCH64
-# define ARM_MAX_VQ    16
-#else
-# define ARM_MAX_VQ    1
-#endif
+#define ARM_MAX_VQ    16
 
 typedef struct ARMVectorReg {
     uint64_t d[2 * ARM_MAX_VQ] QEMU_ALIGNED(16);
-- 
2.39.5

Re: [PATCH 09/13] target/arm/cpu: define ARM_MAX_VQ once for aarch32 and aarch64

[Philippe Mathieu-Daudé]

On 18/3/25 05:51, Pierrick Bouvier wrote:
> This will affect zregs field for aarch32.
> This field is used for MVE and SVE implementations. MVE implementation
> is clipping index value to 0 or 1 for zregs[*].d[],
> so we should not touch the rest of data in this case anyway.

We should describe why it is safe for migration.

I.e. vmstate_za depends on za_needed() -> SME, not included in 32-bit
cpus, etc.

Should we update target/arm/machine.c in this same patch, or a
preliminary one?

> 
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   target/arm/cpu.h | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 27a0d4550f2..00f78d64bd8 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -169,11 +169,7 @@ typedef struct ARMGenericTimer {
>    * Align the data for use with TCG host vector operations.
>    */
>   
> -#ifdef TARGET_AARCH64
> -# define ARM_MAX_VQ    16
> -#else
> -# define ARM_MAX_VQ    1
> -#endif
> +#define ARM_MAX_VQ    16
>   
>   typedef struct ARMVectorReg {
>       uint64_t d[2 * ARM_MAX_VQ] QEMU_ALIGNED(16);

Re: [PATCH 09/13] target/arm/cpu: define ARM_MAX_VQ once for aarch32 and aarch64

[Pierrick Bouvier]

On 3/18/25 11:50, Philippe Mathieu-Daudé wrote:
> On 18/3/25 05:51, Pierrick Bouvier wrote:
>> This will affect zregs field for aarch32.
>> This field is used for MVE and SVE implementations. MVE implementation
>> is clipping index value to 0 or 1 for zregs[*].d[],
>> so we should not touch the rest of data in this case anyway.
> 
> We should describe why it is safe for migration.
> 
> I.e. vmstate_za depends on za_needed() -> SME, not included in 32-bit
> cpus, etc.
> 
> Should we update target/arm/machine.c in this same patch, or a
> preliminary one?
> 

vmstate_za definition and inclusion in vmstate_arm_cpu is under #ifdef 
TARGET_AARCH64. In this case (TARGET_AARCH64), ARM_MAX_VQ was already 
defined as 16, so there should not be any change.

Other values depending on ARM_MAX_VQ, for migration, are as well under 
TARGET_AARCH64 ifdefs (vmstate_zreg_hi_reg, vmstate_preg_reg, vmstate_vreg).

And for vmstate_vfp, which is present for aarch32 as well, the size of 
data under each register is specifically set to 2.
VMSTATE_UINT64_SUB_ARRAY(env.vfp.zregs[0].d, ARMCPU, 0, 2)

So even if storage has more space, it should not impact any usage of it.

Even though this change is trivial, I didn't do it blindly to "make it 
compile" and I checked the various usages of ARM_MAX_VQ and zregs, and I 
didn't see anything that seems to be a problem.

>>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>    target/arm/cpu.h | 6 +-----
>>    1 file changed, 1 insertion(+), 5 deletions(-)
>>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index 27a0d4550f2..00f78d64bd8 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -169,11 +169,7 @@ typedef struct ARMGenericTimer {
>>     * Align the data for use with TCG host vector operations.
>>     */
>>    
>> -#ifdef TARGET_AARCH64
>> -# define ARM_MAX_VQ    16
>> -#else
>> -# define ARM_MAX_VQ    1
>> -#endif
>> +#define ARM_MAX_VQ    16
>>    
>>    typedef struct ARMVectorReg {
>>        uint64_t d[2 * ARM_MAX_VQ] QEMU_ALIGNED(16);
> 

Re: [PATCH 09/13] target/arm/cpu: define ARM_MAX_VQ once for aarch32 and aarch64

[Philippe Mathieu-Daudé]

On 18/3/25 23:02, Pierrick Bouvier wrote:
> On 3/18/25 11:50, Philippe Mathieu-Daudé wrote:
>> On 18/3/25 05:51, Pierrick Bouvier wrote:
>>> This will affect zregs field for aarch32.
>>> This field is used for MVE and SVE implementations. MVE implementation
>>> is clipping index value to 0 or 1 for zregs[*].d[],
>>> so we should not touch the rest of data in this case anyway.
>>
>> We should describe why it is safe for migration.
>>
>> I.e. vmstate_za depends on za_needed() -> SME, not included in 32-bit
>> cpus, etc.
>>
>> Should we update target/arm/machine.c in this same patch, or a
>> preliminary one?
>>
> 
> vmstate_za definition and inclusion in vmstate_arm_cpu is under #ifdef 
> TARGET_AARCH64. In this case (TARGET_AARCH64), ARM_MAX_VQ was already 
> defined as 16, so there should not be any change.

I'm not saying this is invalid, I'm trying to say we need to document
why it is safe.

> Other values depending on ARM_MAX_VQ, for migration, are as well under 
> TARGET_AARCH64 ifdefs (vmstate_zreg_hi_reg, vmstate_preg_reg, 
> vmstate_vreg).
> 
> And for vmstate_vfp, which is present for aarch32 as well, the size of 
> data under each register is specifically set to 2.
> VMSTATE_UINT64_SUB_ARRAY(env.vfp.zregs[0].d, ARMCPU, 0, 2)
> 
> So even if storage has more space, it should not impact any usage of it.
> 
> Even though this change is trivial, I didn't do it blindly to "make it 
> compile" and I checked the various usages of ARM_MAX_VQ and zregs, and I 
> didn't see anything that seems to be a problem.

You did the analysis once, let's add it in the commit description so
other developers looking at this commit won't have to do it again.

> 
>>>
>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>> ---
>>>    target/arm/cpu.h | 6 +-----
>>>    1 file changed, 1 insertion(+), 5 deletions(-)
>>>
>>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>>> index 27a0d4550f2..00f78d64bd8 100644
>>> --- a/target/arm/cpu.h
>>> +++ b/target/arm/cpu.h
>>> @@ -169,11 +169,7 @@ typedef struct ARMGenericTimer {
>>>     * Align the data for use with TCG host vector operations.
>>>     */
>>> -#ifdef TARGET_AARCH64
>>> -# define ARM_MAX_VQ    16
>>> -#else
>>> -# define ARM_MAX_VQ    1
>>> -#endif
>>> +#define ARM_MAX_VQ    16
>>>    typedef struct ARMVectorReg {
>>>        uint64_t d[2 * ARM_MAX_VQ] QEMU_ALIGNED(16);
>>
> 

Re: [PATCH 09/13] target/arm/cpu: define ARM_MAX_VQ once for aarch32 and aarch64

[Pierrick Bouvier]

On 3/19/25 00:03, Philippe Mathieu-Daudé wrote:
> On 18/3/25 23:02, Pierrick Bouvier wrote:
>> On 3/18/25 11:50, Philippe Mathieu-Daudé wrote:
>>> On 18/3/25 05:51, Pierrick Bouvier wrote:
>>>> This will affect zregs field for aarch32.
>>>> This field is used for MVE and SVE implementations. MVE implementation
>>>> is clipping index value to 0 or 1 for zregs[*].d[],
>>>> so we should not touch the rest of data in this case anyway.
>>>
>>> We should describe why it is safe for migration.
>>>
>>> I.e. vmstate_za depends on za_needed() -> SME, not included in 32-bit
>>> cpus, etc.
>>>
>>> Should we update target/arm/machine.c in this same patch, or a
>>> preliminary one?
>>>
>>
>> vmstate_za definition and inclusion in vmstate_arm_cpu is under #ifdef
>> TARGET_AARCH64. In this case (TARGET_AARCH64), ARM_MAX_VQ was already
>> defined as 16, so there should not be any change.
> 
> I'm not saying this is invalid, I'm trying to say we need to document
> why it is safe.
> 
>> Other values depending on ARM_MAX_VQ, for migration, are as well under
>> TARGET_AARCH64 ifdefs (vmstate_zreg_hi_reg, vmstate_preg_reg,
>> vmstate_vreg).
>>
>> And for vmstate_vfp, which is present for aarch32 as well, the size of
>> data under each register is specifically set to 2.
>> VMSTATE_UINT64_SUB_ARRAY(env.vfp.zregs[0].d, ARMCPU, 0, 2)
>>
>> So even if storage has more space, it should not impact any usage of it.
>>
>> Even though this change is trivial, I didn't do it blindly to "make it
>> compile" and I checked the various usages of ARM_MAX_VQ and zregs, and I
>> didn't see anything that seems to be a problem.
> 
> You did the analysis once, let's add it in the commit description so
> other developers looking at this commit won't have to do it again.
> 

Sure, I'll add this to the commit message.

>>
>>>>
>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>> ---
>>>>     target/arm/cpu.h | 6 +-----
>>>>     1 file changed, 1 insertion(+), 5 deletions(-)
>>>>
>>>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>>>> index 27a0d4550f2..00f78d64bd8 100644
>>>> --- a/target/arm/cpu.h
>>>> +++ b/target/arm/cpu.h
>>>> @@ -169,11 +169,7 @@ typedef struct ARMGenericTimer {
>>>>      * Align the data for use with TCG host vector operations.
>>>>      */
>>>> -#ifdef TARGET_AARCH64
>>>> -# define ARM_MAX_VQ    16
>>>> -#else
>>>> -# define ARM_MAX_VQ    1
>>>> -#endif
>>>> +#define ARM_MAX_VQ    16
>>>>     typedef struct ARMVectorReg {
>>>>         uint64_t d[2 * ARM_MAX_VQ] QEMU_ALIGNED(16);
>>>
>>
> 

Re: [PATCH 09/13] target/arm/cpu: define ARM_MAX_VQ once for aarch32 and aarch64

[Richard Henderson]

On 3/17/25 21:51, Pierrick Bouvier wrote:
> This will affect zregs field for aarch32.
> This field is used for MVE and SVE implementations. MVE implementation
> is clipping index value to 0 or 1 for zregs[*].d[],
> so we should not touch the rest of data in this case anyway.
> 
> Signed-off-by: Pierrick Bouvier<pierrick.bouvier@linaro.org>
> ---
>   target/arm/cpu.h | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 10/13] target/arm/cpu: define same set of registers for aarch32 and aarch64

[Pierrick Bouvier]

To eliminate TARGET_AARCH64, we need to make various definitions common
between 32 and 64 bit Arm targets.
Added registers are used only by aarch64 code, and the only impact is on
the size of CPUARMState, and added zarray
(ARMVectorReg zarray[ARM_MAX_VQ * 16]) member (+64KB)

It could be eventually possible to allocate this array only for aarch64
emulation, but I'm not sure it's worth the hassle to save a few KB per
vcpu. Running qemu-system takes already several hundreds of MB of
(resident) memory, and qemu-user takes dozens of MB of (resident) memory
anyway.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 target/arm/cpu.h | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 00f78d64bd8..51b6428cfec 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -175,7 +175,6 @@ typedef struct ARMVectorReg {
     uint64_t d[2 * ARM_MAX_VQ] QEMU_ALIGNED(16);
 } ARMVectorReg;
 
-#ifdef TARGET_AARCH64
 /* In AArch32 mode, predicate registers do not exist at all.  */
 typedef struct ARMPredicateReg {
     uint64_t p[DIV_ROUND_UP(2 * ARM_MAX_VQ, 8)] QEMU_ALIGNED(16);
@@ -185,7 +184,6 @@ typedef struct ARMPredicateReg {
 typedef struct ARMPACKey {
     uint64_t lo, hi;
 } ARMPACKey;
-#endif
 
 /* See the commentary above the TBFLAG field definitions.  */
 typedef struct CPUARMTBFlags {
@@ -656,13 +654,11 @@ typedef struct CPUArchState {
     struct {
         ARMVectorReg zregs[32];
 
-#ifdef TARGET_AARCH64
         /* Store FFR as pregs[16] to make it easier to treat as any other.  */
 #define FFR_PRED_NUM 16
         ARMPredicateReg pregs[17];
         /* Scratch space for aa64 sve predicate temporary.  */
         ARMPredicateReg preg_tmp;
-#endif
 
         /* We store these fpcsr fields separately for convenience.  */
         uint32_t qc[4] QEMU_ALIGNED(16);
@@ -707,7 +703,6 @@ typedef struct CPUArchState {
         uint32_t cregs[16];
     } iwmmxt;
 
-#ifdef TARGET_AARCH64
     struct {
         ARMPACKey apia;
         ARMPACKey apib;
@@ -739,7 +734,6 @@ typedef struct CPUArchState {
      * to keep the offsets into the rest of the structure smaller.
      */
     ARMVectorReg zarray[ARM_MAX_VQ * 16];
-#endif
 
     struct CPUBreakpoint *cpu_breakpoint[16];
     struct CPUWatchpoint *cpu_watchpoint[16];
-- 
2.39.5

Re: [PATCH 10/13] target/arm/cpu: define same set of registers for aarch32 and aarch64

[Richard Henderson]

On 3/17/25 21:51, Pierrick Bouvier wrote:
> To eliminate TARGET_AARCH64, we need to make various definitions common
> between 32 and 64 bit Arm targets.
> Added registers are used only by aarch64 code, and the only impact is on
> the size of CPUARMState, and added zarray
> (ARMVectorReg zarray[ARM_MAX_VQ * 16]) member (+64KB)
> 
> It could be eventually possible to allocate this array only for aarch64
> emulation, but I'm not sure it's worth the hassle to save a few KB per
> vcpu. Running qemu-system takes already several hundreds of MB of
> (resident) memory, and qemu-user takes dozens of MB of (resident) memory
> anyway.
> 
> Signed-off-by: Pierrick Bouvier<pierrick.bouvier@linaro.org>
> ---
>   target/arm/cpu.h | 6 ------
>   1 file changed, 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

I think this could easily squash with ARM_MAX_VQ, since the
rationale is better spelled out here.


r~

Re: [PATCH 10/13] target/arm/cpu: define same set of registers for aarch32 and aarch64

[Pierrick Bouvier]

On 3/18/25 15:45, Richard Henderson wrote:
> On 3/17/25 21:51, Pierrick Bouvier wrote:
>> To eliminate TARGET_AARCH64, we need to make various definitions common
>> between 32 and 64 bit Arm targets.
>> Added registers are used only by aarch64 code, and the only impact is on
>> the size of CPUARMState, and added zarray
>> (ARMVectorReg zarray[ARM_MAX_VQ * 16]) member (+64KB)
>>
>> It could be eventually possible to allocate this array only for aarch64
>> emulation, but I'm not sure it's worth the hassle to save a few KB per
>> vcpu. Running qemu-system takes already several hundreds of MB of
>> (resident) memory, and qemu-user takes dozens of MB of (resident) memory
>> anyway.
>>
>> Signed-off-by: Pierrick Bouvier<pierrick.bouvier@linaro.org>
>> ---
>>    target/arm/cpu.h | 6 ------
>>    1 file changed, 6 deletions(-)
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> I think this could easily squash with ARM_MAX_VQ, since the
> rationale is better spelled out here.
> 

Yes, makes sense. I'll squash it.

> 
> r~

[PATCH 11/13] target/arm/cpu: remove inline stubs for aarch32 emulation

[Pierrick Bouvier]

Directly condition associated calls in target/arm/helper.c for now.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 target/arm/cpu.h    | 8 --------
 target/arm/helper.c | 6 ++++++
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 51b6428cfec..9205cbdec43 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1222,7 +1222,6 @@ int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
  */
 void arm_emulate_firmware_reset(CPUState *cpustate, int target_el);
 
-#ifdef TARGET_AARCH64
 int aarch64_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
 int aarch64_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
 void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq);
@@ -1254,13 +1253,6 @@ static inline uint64_t *sve_bswap64(uint64_t *dst, uint64_t *src, int nr)
 #endif
 }
 
-#else
-static inline void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq) { }
-static inline void aarch64_sve_change_el(CPUARMState *env, int o,
-                                         int n, bool a)
-{ }
-#endif
-
 void aarch64_sync_32_to_64(CPUARMState *env);
 void aarch64_sync_64_to_32(CPUARMState *env);
 
diff --git a/target/arm/helper.c b/target/arm/helper.c
index b46b2bffcf3..774e1ee0245 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6562,7 +6562,9 @@ static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
      */
     new_len = sve_vqm1_for_el(env, cur_el);
     if (new_len < old_len) {
+#ifdef TARGET_AARCH64
         aarch64_sve_narrow_vq(env, new_len + 1);
+#endif
     }
 }
 
@@ -10646,7 +10648,9 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
          * Note that new_el can never be 0.  If cur_el is 0, then
          * el0_a64 is is_a64(), else el0_a64 is ignored.
          */
+#ifdef TARGET_AARCH64
         aarch64_sve_change_el(env, cur_el, new_el, is_a64(env));
+#endif
     }
 
     if (cur_el < new_el) {
@@ -11663,7 +11667,9 @@ void aarch64_sve_change_el(CPUARMState *env, int old_el,
 
     /* When changing vector length, clear inaccessible state.  */
     if (new_len < old_len) {
+#ifdef TARGET_AARCH64
         aarch64_sve_narrow_vq(env, new_len + 1);
+#endif
     }
 }
 #endif
-- 
2.39.5

Re: [PATCH 11/13] target/arm/cpu: remove inline stubs for aarch32 emulation

[Philippe Mathieu-Daudé]

On 18/3/25 05:51, Pierrick Bouvier wrote:
> Directly condition associated calls in target/arm/helper.c for now.
> 
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   target/arm/cpu.h    | 8 --------
>   target/arm/helper.c | 6 ++++++
>   2 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 51b6428cfec..9205cbdec43 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1222,7 +1222,6 @@ int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
>    */
>   void arm_emulate_firmware_reset(CPUState *cpustate, int target_el);
>   
> -#ifdef TARGET_AARCH64
>   int aarch64_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
>   int aarch64_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
>   void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq);
> @@ -1254,13 +1253,6 @@ static inline uint64_t *sve_bswap64(uint64_t *dst, uint64_t *src, int nr)
>   #endif
>   }
>   
> -#else
> -static inline void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq) { }
> -static inline void aarch64_sve_change_el(CPUARMState *env, int o,
> -                                         int n, bool a)
> -{ }
> -#endif
> -
>   void aarch64_sync_32_to_64(CPUARMState *env);
>   void aarch64_sync_64_to_32(CPUARMState *env);
>   
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index b46b2bffcf3..774e1ee0245 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6562,7 +6562,9 @@ static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>        */
>       new_len = sve_vqm1_for_el(env, cur_el);
>       if (new_len < old_len) {
> +#ifdef TARGET_AARCH64

What about using runtime check instead?

  if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) && new_len < old_len) {

>           aarch64_sve_narrow_vq(env, new_len + 1);
> +#endif
>       }
>   }

Re: [PATCH 11/13] target/arm/cpu: remove inline stubs for aarch32 emulation

[Peter Maydell]

On Tue, 18 Mar 2025 at 17:42, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 18/3/25 05:51, Pierrick Bouvier wrote:
> > Directly condition associated calls in target/arm/helper.c for now.
> >
> > Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> > ---
> >   target/arm/cpu.h    | 8 --------
> >   target/arm/helper.c | 6 ++++++
> >   2 files changed, 6 insertions(+), 8 deletions(-)
> >
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index 51b6428cfec..9205cbdec43 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -1222,7 +1222,6 @@ int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
> >    */
> >   void arm_emulate_firmware_reset(CPUState *cpustate, int target_el);
> >
> > -#ifdef TARGET_AARCH64
> >   int aarch64_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
> >   int aarch64_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
> >   void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq);
> > @@ -1254,13 +1253,6 @@ static inline uint64_t *sve_bswap64(uint64_t *dst, uint64_t *src, int nr)
> >   #endif
> >   }
> >
> > -#else
> > -static inline void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq) { }
> > -static inline void aarch64_sve_change_el(CPUARMState *env, int o,
> > -                                         int n, bool a)
> > -{ }
> > -#endif
> > -
> >   void aarch64_sync_32_to_64(CPUARMState *env);
> >   void aarch64_sync_64_to_32(CPUARMState *env);
> >
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index b46b2bffcf3..774e1ee0245 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -6562,7 +6562,9 @@ static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> >        */
> >       new_len = sve_vqm1_for_el(env, cur_el);
> >       if (new_len < old_len) {
> > +#ifdef TARGET_AARCH64
>
> What about using runtime check instead?
>
>   if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) && new_len < old_len) {
>

That would be a dead check: it is not possible to get here
unless ARM_FEATURE_AARCH64 is set.

thanks
-- PMM

Re: [PATCH 11/13] target/arm/cpu: remove inline stubs for aarch32 emulation

[Pierrick Bouvier]

On 3/18/25 10:50, Peter Maydell wrote:
> On Tue, 18 Mar 2025 at 17:42, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> On 18/3/25 05:51, Pierrick Bouvier wrote:
>>> Directly condition associated calls in target/arm/helper.c for now.
>>>
>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>> ---
>>>    target/arm/cpu.h    | 8 --------
>>>    target/arm/helper.c | 6 ++++++
>>>    2 files changed, 6 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>>> index 51b6428cfec..9205cbdec43 100644
>>> --- a/target/arm/cpu.h
>>> +++ b/target/arm/cpu.h
>>> @@ -1222,7 +1222,6 @@ int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
>>>     */
>>>    void arm_emulate_firmware_reset(CPUState *cpustate, int target_el);
>>>
>>> -#ifdef TARGET_AARCH64
>>>    int aarch64_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
>>>    int aarch64_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
>>>    void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq);
>>> @@ -1254,13 +1253,6 @@ static inline uint64_t *sve_bswap64(uint64_t *dst, uint64_t *src, int nr)
>>>    #endif
>>>    }
>>>
>>> -#else
>>> -static inline void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq) { }
>>> -static inline void aarch64_sve_change_el(CPUARMState *env, int o,
>>> -                                         int n, bool a)
>>> -{ }
>>> -#endif
>>> -
>>>    void aarch64_sync_32_to_64(CPUARMState *env);
>>>    void aarch64_sync_64_to_32(CPUARMState *env);
>>>
>>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>>> index b46b2bffcf3..774e1ee0245 100644
>>> --- a/target/arm/helper.c
>>> +++ b/target/arm/helper.c
>>> @@ -6562,7 +6562,9 @@ static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>>         */
>>>        new_len = sve_vqm1_for_el(env, cur_el);
>>>        if (new_len < old_len) {
>>> +#ifdef TARGET_AARCH64
>>
>> What about using runtime check instead?
>>
>>    if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) && new_len < old_len) {
>>
> 
> That would be a dead check: it is not possible to get here
> unless ARM_FEATURE_AARCH64 is set.
> 

We can then assert it, to make sure there is no regression around that.

We now have another conversation and something to decide in another 
file, and that's why I chose to do the minimal change ("ifdef the 
issue") instead of trying to do any change.

> thanks
> -- PMM

Re: [PATCH 11/13] target/arm/cpu: remove inline stubs for aarch32 emulation

[Peter Maydell]

On Tue, 18 Mar 2025 at 17:52, Pierrick Bouvier
<pierrick.bouvier@linaro.org> wrote:
>
> On 3/18/25 10:50, Peter Maydell wrote:
> > On Tue, 18 Mar 2025 at 17:42, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >>
> >> On 18/3/25 05:51, Pierrick Bouvier wrote:
> >>> Directly condition associated calls in target/arm/helper.c for now.
> >>>
> >>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> >>> ---
> >>>    target/arm/cpu.h    | 8 --------
> >>>    target/arm/helper.c | 6 ++++++
> >>>    2 files changed, 6 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> >>> index 51b6428cfec..9205cbdec43 100644
> >>> --- a/target/arm/cpu.h
> >>> +++ b/target/arm/cpu.h
> >>> @@ -1222,7 +1222,6 @@ int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
> >>>     */
> >>>    void arm_emulate_firmware_reset(CPUState *cpustate, int target_el);
> >>>
> >>> -#ifdef TARGET_AARCH64
> >>>    int aarch64_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
> >>>    int aarch64_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
> >>>    void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq);
> >>> @@ -1254,13 +1253,6 @@ static inline uint64_t *sve_bswap64(uint64_t *dst, uint64_t *src, int nr)
> >>>    #endif
> >>>    }
> >>>
> >>> -#else
> >>> -static inline void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq) { }
> >>> -static inline void aarch64_sve_change_el(CPUARMState *env, int o,
> >>> -                                         int n, bool a)
> >>> -{ }
> >>> -#endif
> >>> -
> >>>    void aarch64_sync_32_to_64(CPUARMState *env);
> >>>    void aarch64_sync_64_to_32(CPUARMState *env);
> >>>
> >>> diff --git a/target/arm/helper.c b/target/arm/helper.c
> >>> index b46b2bffcf3..774e1ee0245 100644
> >>> --- a/target/arm/helper.c
> >>> +++ b/target/arm/helper.c
> >>> @@ -6562,7 +6562,9 @@ static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> >>>         */
> >>>        new_len = sve_vqm1_for_el(env, cur_el);
> >>>        if (new_len < old_len) {
> >>> +#ifdef TARGET_AARCH64
> >>
> >> What about using runtime check instead?
> >>
> >>    if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) && new_len < old_len) {
> >>
> >
> > That would be a dead check: it is not possible to get here
> > unless ARM_FEATURE_AARCH64 is set.
> >
>
> We can then assert it, to make sure there is no regression around that.

We have a lot of write/read/access fns for AArch64-only sysregs, and
we don't need to assert in all of them that they're called only when
the CPU has AArch64 enabled.

> We now have another conversation and something to decide in another
> file, and that's why I chose to do the minimal change ("ifdef the
> issue") instead of trying to do any change.

I think we can fairly easily avoid ifdeffing the callsite of
aarch64_sve_narrow_vq(). Currently we have:
 * a real version of the function, whose definition is inside
   an ifdef TARGET_AARCH64 in target/arm/helper.c
 * a stub version, inline in the cpu.h header

If we don't want to have the stub version with ifdefs, then we can
move the real implementation of the function to not be inside the
ifdef (matching the fact that the prototype is no longer inside
an ifdef). The function doesn't call any other functions that are
TARGET_AARCH64 only, so it shouldn't be a "now we have to move
50 other things" problem, I hope.

thanks
-- PMM

Re: [PATCH 11/13] target/arm/cpu: remove inline stubs for aarch32 emulation

[Pierrick Bouvier]

On 3/18/25 11:06, Peter Maydell wrote:
> On Tue, 18 Mar 2025 at 17:52, Pierrick Bouvier
> <pierrick.bouvier@linaro.org> wrote:
>>
>> On 3/18/25 10:50, Peter Maydell wrote:
>>> On Tue, 18 Mar 2025 at 17:42, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>>>
>>>> On 18/3/25 05:51, Pierrick Bouvier wrote:
>>>>> Directly condition associated calls in target/arm/helper.c for now.
>>>>>
>>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>>> ---
>>>>>     target/arm/cpu.h    | 8 --------
>>>>>     target/arm/helper.c | 6 ++++++
>>>>>     2 files changed, 6 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>>>>> index 51b6428cfec..9205cbdec43 100644
>>>>> --- a/target/arm/cpu.h
>>>>> +++ b/target/arm/cpu.h
>>>>> @@ -1222,7 +1222,6 @@ int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
>>>>>      */
>>>>>     void arm_emulate_firmware_reset(CPUState *cpustate, int target_el);
>>>>>
>>>>> -#ifdef TARGET_AARCH64
>>>>>     int aarch64_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
>>>>>     int aarch64_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
>>>>>     void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq);
>>>>> @@ -1254,13 +1253,6 @@ static inline uint64_t *sve_bswap64(uint64_t *dst, uint64_t *src, int nr)
>>>>>     #endif
>>>>>     }
>>>>>
>>>>> -#else
>>>>> -static inline void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq) { }
>>>>> -static inline void aarch64_sve_change_el(CPUARMState *env, int o,
>>>>> -                                         int n, bool a)
>>>>> -{ }
>>>>> -#endif
>>>>> -
>>>>>     void aarch64_sync_32_to_64(CPUARMState *env);
>>>>>     void aarch64_sync_64_to_32(CPUARMState *env);
>>>>>
>>>>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>>>>> index b46b2bffcf3..774e1ee0245 100644
>>>>> --- a/target/arm/helper.c
>>>>> +++ b/target/arm/helper.c
>>>>> @@ -6562,7 +6562,9 @@ static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>>>>          */
>>>>>         new_len = sve_vqm1_for_el(env, cur_el);
>>>>>         if (new_len < old_len) {
>>>>> +#ifdef TARGET_AARCH64
>>>>
>>>> What about using runtime check instead?
>>>>
>>>>     if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) && new_len < old_len) {
>>>>
>>>
>>> That would be a dead check: it is not possible to get here
>>> unless ARM_FEATURE_AARCH64 is set.
>>>
>>
>> We can then assert it, to make sure there is no regression around that.
> 
> We have a lot of write/read/access fns for AArch64-only sysregs, and
> we don't need to assert in all of them that they're called only when
> the CPU has AArch64 enabled.
> 
>> We now have another conversation and something to decide in another
>> file, and that's why I chose to do the minimal change ("ifdef the
>> issue") instead of trying to do any change.
> 
> I think we can fairly easily avoid ifdeffing the callsite of
> aarch64_sve_narrow_vq(). Currently we have:
>   * a real version of the function, whose definition is inside
>     an ifdef TARGET_AARCH64 in target/arm/helper.c
>   * a stub version, inline in the cpu.h header
> 
> If we don't want to have the stub version with ifdefs, then we can
> move the real implementation of the function to not be inside the
> ifdef (matching the fact that the prototype is no longer inside
> an ifdef). The function doesn't call any other functions that are
> TARGET_AARCH64 only, so it shouldn't be a "now we have to move
> 50 other things" problem, I hope.
> 

I'll try to just let the call be done, and see what happens.
But if I meet a regression somewhere in target/arm/*, I'll simply
let the current ifdef for now.

I understand it "should be ok", but I just want to focus on hw/arm, and 
not start further changes in target/arm/helper.c as a side effect of 
simply moving an ifdef in a header.

> thanks
> -- PMM

Re: [PATCH 11/13] target/arm/cpu: remove inline stubs for aarch32 emulation

[Pierrick Bouvier]

On 3/18/25 11:13, Pierrick Bouvier wrote:
> On 3/18/25 11:06, Peter Maydell wrote:
>> On Tue, 18 Mar 2025 at 17:52, Pierrick Bouvier
>> <pierrick.bouvier@linaro.org> wrote:
>>>
>>> On 3/18/25 10:50, Peter Maydell wrote:
>>>> On Tue, 18 Mar 2025 at 17:42, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>>>>
>>>>> On 18/3/25 05:51, Pierrick Bouvier wrote:
>>>>>> Directly condition associated calls in target/arm/helper.c for now.
>>>>>>
>>>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>>>> ---
>>>>>>      target/arm/cpu.h    | 8 --------
>>>>>>      target/arm/helper.c | 6 ++++++
>>>>>>      2 files changed, 6 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>>>>>> index 51b6428cfec..9205cbdec43 100644
>>>>>> --- a/target/arm/cpu.h
>>>>>> +++ b/target/arm/cpu.h
>>>>>> @@ -1222,7 +1222,6 @@ int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
>>>>>>       */
>>>>>>      void arm_emulate_firmware_reset(CPUState *cpustate, int target_el);
>>>>>>
>>>>>> -#ifdef TARGET_AARCH64
>>>>>>      int aarch64_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
>>>>>>      int aarch64_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
>>>>>>      void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq);
>>>>>> @@ -1254,13 +1253,6 @@ static inline uint64_t *sve_bswap64(uint64_t *dst, uint64_t *src, int nr)
>>>>>>      #endif
>>>>>>      }
>>>>>>
>>>>>> -#else
>>>>>> -static inline void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq) { }
>>>>>> -static inline void aarch64_sve_change_el(CPUARMState *env, int o,
>>>>>> -                                         int n, bool a)
>>>>>> -{ }
>>>>>> -#endif
>>>>>> -
>>>>>>      void aarch64_sync_32_to_64(CPUARMState *env);
>>>>>>      void aarch64_sync_64_to_32(CPUARMState *env);
>>>>>>
>>>>>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>>>>>> index b46b2bffcf3..774e1ee0245 100644
>>>>>> --- a/target/arm/helper.c
>>>>>> +++ b/target/arm/helper.c
>>>>>> @@ -6562,7 +6562,9 @@ static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>>>>>           */
>>>>>>          new_len = sve_vqm1_for_el(env, cur_el);
>>>>>>          if (new_len < old_len) {
>>>>>> +#ifdef TARGET_AARCH64
>>>>>
>>>>> What about using runtime check instead?
>>>>>
>>>>>      if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) && new_len < old_len) {
>>>>>
>>>>
>>>> That would be a dead check: it is not possible to get here
>>>> unless ARM_FEATURE_AARCH64 is set.
>>>>
>>>
>>> We can then assert it, to make sure there is no regression around that.
>>
>> We have a lot of write/read/access fns for AArch64-only sysregs, and
>> we don't need to assert in all of them that they're called only when
>> the CPU has AArch64 enabled.
>>
>>> We now have another conversation and something to decide in another
>>> file, and that's why I chose to do the minimal change ("ifdef the
>>> issue") instead of trying to do any change.
>>
>> I think we can fairly easily avoid ifdeffing the callsite of
>> aarch64_sve_narrow_vq(). Currently we have:
>>    * a real version of the function, whose definition is inside
>>      an ifdef TARGET_AARCH64 in target/arm/helper.c
>>    * a stub version, inline in the cpu.h header
>>
>> If we don't want to have the stub version with ifdefs, then we can
>> move the real implementation of the function to not be inside the
>> ifdef (matching the fact that the prototype is no longer inside
>> an ifdef). The function doesn't call any other functions that are
>> TARGET_AARCH64 only, so it shouldn't be a "now we have to move
>> 50 other things" problem, I hope.
>>
> 
> I'll try to just let the call be done, and see what happens.
> But if I meet a regression somewhere in target/arm/*, I'll simply
> let the current ifdef for now.
> 
> I understand it "should be ok", but I just want to focus on hw/arm, and
> not start further changes in target/arm/helper.c as a side effect of
> simply moving an ifdef in a header.
> 

I tried to simply ifdef functions depending on those symbols. However, 
this adds much more ifdefs than this original patch.
Soon, we'll completely remove those ifdefs from target/arm/helper.c, so 
I'll just call it a day for now, without adding stubs that will be 
removed soon as well.

>> thanks
>> -- PMM
> 

Re: [PATCH 11/13] target/arm/cpu: remove inline stubs for aarch32 emulation

[Philippe Mathieu-Daudé]

On 18/3/25 18:50, Peter Maydell wrote:
> On Tue, 18 Mar 2025 at 17:42, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> On 18/3/25 05:51, Pierrick Bouvier wrote:
>>> Directly condition associated calls in target/arm/helper.c for now.
>>>
>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>> ---
>>>    target/arm/cpu.h    | 8 --------
>>>    target/arm/helper.c | 6 ++++++
>>>    2 files changed, 6 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>>> index 51b6428cfec..9205cbdec43 100644
>>> --- a/target/arm/cpu.h
>>> +++ b/target/arm/cpu.h
>>> @@ -1222,7 +1222,6 @@ int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
>>>     */
>>>    void arm_emulate_firmware_reset(CPUState *cpustate, int target_el);
>>>
>>> -#ifdef TARGET_AARCH64
>>>    int aarch64_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
>>>    int aarch64_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
>>>    void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq);
>>> @@ -1254,13 +1253,6 @@ static inline uint64_t *sve_bswap64(uint64_t *dst, uint64_t *src, int nr)
>>>    #endif
>>>    }
>>>
>>> -#else
>>> -static inline void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq) { }
>>> -static inline void aarch64_sve_change_el(CPUARMState *env, int o,
>>> -                                         int n, bool a)
>>> -{ }
>>> -#endif
>>> -
>>>    void aarch64_sync_32_to_64(CPUARMState *env);
>>>    void aarch64_sync_64_to_32(CPUARMState *env);
>>>
>>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>>> index b46b2bffcf3..774e1ee0245 100644
>>> --- a/target/arm/helper.c
>>> +++ b/target/arm/helper.c
>>> @@ -6562,7 +6562,9 @@ static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>>         */
>>>        new_len = sve_vqm1_for_el(env, cur_el);
>>>        if (new_len < old_len) {
>>> +#ifdef TARGET_AARCH64
>>
>> What about using runtime check instead?
>>
>>    if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) && new_len < old_len) {
>>
> 
> That would be a dead check: it is not possible to get here
> unless ARM_FEATURE_AARCH64 is set.

So checks in callees such:

-- >8 --
diff --git a/target/arm/helper.c b/target/arm/helper.c
index bb445e30cd1..8377eb0e710 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -11547,5 +11547,7 @@ void aarch64_sve_narrow_vq(CPUARMState *env, 
unsigned vq)
      uint64_t pmask;
+    ARMCPU *cpu = env_archcpu(env);

+    assert(cpu_isar_feature(aa64_sve, cpu));
      assert(vq >= 1 && vq <= ARM_MAX_VQ);
-    assert(vq <= env_archcpu(env)->sve_max_vq);
+    assert(vq <= cpu->sve_max_vq);

---

Re: [PATCH 11/13] target/arm/cpu: remove inline stubs for aarch32 emulation

[Pierrick Bouvier]

On 3/18/25 10:42, Philippe Mathieu-Daudé wrote:
> On 18/3/25 05:51, Pierrick Bouvier wrote:
>> Directly condition associated calls in target/arm/helper.c for now.
>>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>    target/arm/cpu.h    | 8 --------
>>    target/arm/helper.c | 6 ++++++
>>    2 files changed, 6 insertions(+), 8 deletions(-)
>>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index 51b6428cfec..9205cbdec43 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -1222,7 +1222,6 @@ int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
>>     */
>>    void arm_emulate_firmware_reset(CPUState *cpustate, int target_el);
>>    
>> -#ifdef TARGET_AARCH64
>>    int aarch64_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
>>    int aarch64_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
>>    void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq);
>> @@ -1254,13 +1253,6 @@ static inline uint64_t *sve_bswap64(uint64_t *dst, uint64_t *src, int nr)
>>    #endif
>>    }
>>    
>> -#else
>> -static inline void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq) { }
>> -static inline void aarch64_sve_change_el(CPUARMState *env, int o,
>> -                                         int n, bool a)
>> -{ }
>> -#endif
>> -
>>    void aarch64_sync_32_to_64(CPUARMState *env);
>>    void aarch64_sync_64_to_32(CPUARMState *env);
>>    
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index b46b2bffcf3..774e1ee0245 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -6562,7 +6562,9 @@ static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>         */
>>        new_len = sve_vqm1_for_el(env, cur_el);
>>        if (new_len < old_len) {
>> +#ifdef TARGET_AARCH64
> 
> What about using runtime check instead?
> 
>    if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) && new_len < old_len) {
> 

This is the right way to deal with it, but I would prefer to do it when 
the concerned file (target/arm/helper.c) will be modified, in a future 
series.

The current one focuses on hw/arm and "pushes back" dependencies in 
other places, that we can deal with later.

Nothing wrong with the change proposed, just trying to focus on the 
minimal set needed to reach the goal, without any detour.

>>            aarch64_sve_narrow_vq(env, new_len + 1);
>> +#endif
>>        }
>>    }
> 

[PATCH 12/13] meson: add common hw files

[Pierrick Bouvier]

Those files will be compiled once per base architecture ("arm" in this
case), instead of being compiled for every variant/bitness of
architecture.

We make sure to not include target cpu definitions (exec/cpu-defs.h) by
defining header guard directly. This way, a given compilation unit can
access a specific cpu definition, but not access to compile time defines
associated.

Previous commits took care to clean up some headers to not rely on
cpu-defs.h content.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 meson.build | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index 672a0f79d11..0dec7d9750e 100644
--- a/meson.build
+++ b/meson.build
@@ -3689,6 +3689,7 @@ hw_arch = {}
 target_arch = {}
 target_system_arch = {}
 target_user_arch = {}
+hw_common_arch = {}
 
 # NOTE: the trace/ subdirectory needs the qapi_trace_events variable
 # that is filled in by qapi/.
@@ -4065,6 +4066,33 @@ common_all = static_library('common',
                             implicit_include_directories: false,
                             dependencies: common_ss.all_dependencies())
 
+# construct common libraries per base architecture
+hw_common_arch_libs = {}
+foreach target : target_dirs
+  config_target = config_target_mak[target]
+  target_base_arch = config_target['TARGET_BASE_ARCH']
+
+  # check if already generated
+  if target_base_arch in hw_common_arch_libs
+    continue
+  endif
+
+  if target_base_arch in hw_common_arch
+    src = hw_common_arch[target_base_arch]
+    lib = static_library(
+      'hw_' + target_base_arch,
+      build_by_default: false,
+      sources: src.all_sources() + genh,
+      include_directories: common_user_inc,
+      implicit_include_directories: false,
+      # prevent common code to access cpu compile time
+      # definition, but still allow access to cpu.h
+      c_args: ['-DCPU_DEFS_H', '-DCONFIG_SOFTMMU'],
+      dependencies: src.all_dependencies())
+    hw_common_arch_libs += {target_base_arch: lib}
+  endif
+endforeach
+
 if have_rust
   # We would like to use --generate-cstr, but it is only available
   # starting with bindgen 0.66.0.  The oldest supported versions
@@ -4230,8 +4258,14 @@ foreach target : target_dirs
   arch_deps += t.dependencies()
 
   target_common = common_ss.apply(config_target, strict: false)
-  objects = common_all.extract_objects(target_common.sources())
+  objects = [common_all.extract_objects(target_common.sources())]
   arch_deps += target_common.dependencies()
+  if target_type == 'system' and target_base_arch in hw_common_arch_libs
+    src = hw_common_arch[target_base_arch].apply(config_target, strict: false)
+    lib = hw_common_arch_libs[target_base_arch]
+    objects += lib.extract_objects(src.sources())
+    arch_deps += src.dependencies()
+  endif
 
   target_specific = specific_ss.apply(config_target, strict: false)
   arch_srcs += target_specific.sources()
-- 
2.39.5

[PATCH 13/13] hw/arm/boot: make compilation unit hw common

[Pierrick Bouvier]

Now we eliminated poisoned identifiers from headers, this file can now
be compiled once for all arm targets.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 hw/arm/boot.c      | 1 +
 hw/arm/meson.build | 5 ++++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index e296b62fa12..639f737aefe 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -23,6 +23,7 @@
 #include "hw/loader.h"
 #include "elf.h"
 #include "system/device_tree.h"
+#include "target/arm/cpu.h"
 #include "qemu/config-file.h"
 #include "qemu/option.h"
 #include "qemu/units.h"
diff --git a/hw/arm/meson.build b/hw/arm/meson.build
index ac473ce7cda..9e8c96059eb 100644
--- a/hw/arm/meson.build
+++ b/hw/arm/meson.build
@@ -1,5 +1,5 @@
 arm_ss = ss.source_set()
-arm_ss.add(files('boot.c'))
+arm_common_ss = ss.source_set()
 arm_ss.add(when: 'CONFIG_ARM_VIRT', if_true: files('virt.c'))
 arm_ss.add(when: 'CONFIG_ACPI', if_true: files('virt-acpi-build.c'))
 arm_ss.add(when: 'CONFIG_DIGIC', if_true: files('digic_boards.c'))
@@ -75,4 +75,7 @@ system_ss.add(when: 'CONFIG_SX1', if_true: files('omap_sx1.c'))
 system_ss.add(when: 'CONFIG_VERSATILE', if_true: files('versatilepb.c'))
 system_ss.add(when: 'CONFIG_VEXPRESS', if_true: files('vexpress.c'))
 
+arm_common_ss.add(fdt, files('boot.c'))
+
 hw_arch += {'arm': arm_ss}
+hw_common_arch += {'arm': arm_common_ss}
-- 
2.39.5