| Message ID | 20250318045125.759259-10-pierrick.bouvier@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | single-binary: start make hw/arm/ common (boot.c) | expand |
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);
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); >
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~
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); >> >
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); >>> >> >
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);
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(-)