single-binary: start make hw/arm/ common (boot.c)

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

Posted by Pierrick Bouvier 10 months, 4 weeks ago

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

Posted by Richard Henderson 10 months, 4 weeks ago

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~

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

Posted by Philippe Mathieu-Daudé 10 months, 4 weeks ago

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

Posted by Pierrick Bouvier 10 months, 4 weeks ago

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

Posted by Philippe Mathieu-Daudé 10 months, 4 weeks ago

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

Posted by Pierrick Bouvier 10 months, 3 weeks ago

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);
>>>
>>
>