Re: [Qemu-devel] [PATCH v3 8/9] target/ppc: move FP and VMX registers into aligned vsr register array

[Mark Cave-Ayland]

On 20/12/2018 17:57, Richard Henderson wrote:

> On 12/20/18 8:31 AM, Mark Cave-Ayland wrote:
>> The VSX register array is a block of 64 128-bit registers where the first 32
>> registers consist of the existing 64-bit FP registers extended to 128-bit
>> using new VSR registers, and the last 32 registers are the VMX 128-bit
>> registers as show below:
>>
>>             64-bit               64-bit
>>     +--------------------+--------------------+
>>     |        FP0         |                    |  VSR0
>>     +--------------------+--------------------+
>>     |        FP1         |                    |  VSR1
>>     +--------------------+--------------------+
>>     |        ...         |        ...         |  ...
>>     +--------------------+--------------------+
>>     |        FP30        |                    |  VSR30
>>     +--------------------+--------------------+
>>     |        FP31        |                    |  VSR31
>>     +--------------------+--------------------+
>>     |                  VMX0                   |  VSR32
>>     +-----------------------------------------+
>>     |                  VMX1                   |  VSR33
>>     +-----------------------------------------+
>>     |                  ...                    |  ...
>>     +-----------------------------------------+
>>     |                  VMX30                  |  VSR62
>>     +-----------------------------------------+
>>     |                  VMX31                  |  VSR63
>>     +-----------------------------------------+
>>
>> In order to allow for future conversion of VSX instructions to use TCG vector
>> operations, recreate the same layout using an aligned version of the existing
>> vsr register array.
>>
>> Since the old fpr and avr register arrays are removed, the existing callers
>> must also be updated to use the correct offset in the vsr register array. 
>> This
>> also includes switching the relevant VMState fields over to using subarrays
>> to make sure that migration is preserved.
>>
>> Signed-off-by: Mark Cave-Ayland <address@hidden>
>> Reviewed-by: Richard Henderson <address@hidden>
>> Acked-by: David Gibson <address@hidden>
>> ---
>>  linux-user/ppc/signal.c             | 24 ++++++-------
>>  target/ppc/arch_dump.c              | 12 +++----
>>  target/ppc/cpu.h                    |  9 ++---
>>  target/ppc/gdbstub.c                |  8 ++---
>>  target/ppc/internal.h               | 18 +++-------
>>  target/ppc/machine.c                | 72 
>> ++++++++++++++++++++++++++++++++++---
>>  target/ppc/monitor.c                |  4 +--
>>  target/ppc/translate.c              | 14 ++++----
>>  target/ppc/translate/dfp-impl.inc.c |  2 +-
>>  target/ppc/translate/vmx-impl.inc.c |  7 +++-
>>  target/ppc/translate/vsx-impl.inc.c |  4 +--
>>  target/ppc/translate_init.inc.c     | 24 ++++++-------
>>  12 files changed, 126 insertions(+), 72 deletions(-)
>>
>> diff --git a/linux-user/ppc/signal.c b/linux-user/ppc/signal.c
>> index 2ae120a2bc..a053dd5b84 100644
>> --- a/linux-user/ppc/signal.c
>> +++ b/linux-user/ppc/signal.c
>> @@ -258,8 +258,8 @@ static void save_user_regs(CPUPPCState *env, struct 
>> target_mcontext *frame)
>>      /* Save Altivec registers if necessary.  */
>>      if (env->insns_flags & PPC_ALTIVEC) {
>>          uint32_t *vrsave;
>> -        for (i = 0; i < ARRAY_SIZE(env->avr); i++) {
>> -            ppc_avr_t *avr = &env->avr[i];
>> +        for (i = 0; i < 32; i++) {
>> +            ppc_avr_t *avr = &env->vsr[32 + i];
> 
> Because of our subsequent discussion re endianness within these vectors, I
> think it would be helpful add some helpers here.
> 
> static inline ppc_avr_t *cpu_avr_ptr(CPUPPCState *env, int i)
> {
>     return &env->vsr[32 + i];
> }
> 
>>      /* Save VSX second halves */
>>      if (env->insns_flags2 & PPC2_VSX) {
>>          uint64_t *vsregs = (uint64_t *)&frame->mc_vregs.altivec[34];
>> -        for (i = 0; i < ARRAY_SIZE(env->vsr); i++) {
>> -            __put_user(env->vsr[i], &vsregs[i]);
>> +        for (i = 0; i < 32; i++) {
>> +            __put_user(env->vsr[i].u64[1], &vsregs[i]);
> 
> static inline uint64_t *cpu_vsrl_ptr(CPUPPCState *env, int i)
> {
>     return &env->vsr[i].u64[1];
> }
> 
>>      /* Save floating point registers.  */
>>      if (env->insns_flags & PPC_FLOAT) {
>> -        for (i = 0; i < ARRAY_SIZE(env->fpr); i++) {
>> -            __put_user(env->fpr[i], &frame->mc_fregs[i]);
>> +        for (i = 0; i < 32; i++) {
>> +            __put_user(env->vsr[i].u64[0], &frame->mc_fregs[i]);
> 
> static inline uint64_t *cpu_fpr_ptr(CPUPPCState *env, int i)
> {
>     return &env->vsr[i].u64[0];
> }
> 
> 
> Eventually, we will want to make these last two functions be dependent on host
> endianness, so that we can remove getVSR and putVSR.  At which point VSR and
> AVX registers will have the same representation.  Because at present they
> don't, which IMO is, if not a bug, at least a severe mis-feature.

Okay I can do that. Do you want these helpers used just within
linux-user/ppc/signal.c or also within the other files touched by this patch 
e.g.
arch_dump.c, gdbstub.c etc.?


ATB,

Mark.