| Message ID | 20140428112947.26464.41276.stgit@bahia.lab.toulouse-stg.fr.ibm.com |
|---|---|
| State | New |
| Headers | show |
[fixing Bharata's address] Am 28.04.2014 13:29, schrieb Greg Kurz: > POWER7, POWER7+ and POWER8 families use the ILE bit of the LPCR > special purpose register to decide the endianness to use when > entering interrupt handlers. When running a linux guest, this > provides a hint on the endianness used by the kernel. From a > qemu point of view, the information is needed for legacy virtio "QEMU" :) - and "Linux" while at it? > support and crash dump support as well. > > Suggested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Suggested-by: Alexander Graf <agraf@suse.de> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > --- > target-ppc/cpu-qom.h | 2 ++ > target-ppc/misc_helper.c | 7 +++++++ > target-ppc/translate_init.c | 16 ++++++++++++++++ > 3 files changed, 25 insertions(+) > > diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h > index 47dc8e6..c20473a 100644 > --- a/target-ppc/cpu-qom.h > +++ b/target-ppc/cpu-qom.h > @@ -76,6 +76,7 @@ typedef struct PowerPCCPUClass { > int (*handle_mmu_fault)(PowerPCCPU *cpu, target_ulong eaddr, int rwx, > int mmu_idx); > #endif > + bool (*interrupts_big_endian)(CPUPPCState *env); Please do not add *any* new functions with a CPUPPCState argument, especially not in my shiny new PowerPCCPUClass. ;) > } PowerPCCPUClass; > > /** > @@ -118,6 +119,7 @@ int ppc64_cpu_write_elf64_qemunote(WriteCoreDumpFunction f, > CPUState *cpu, void *opaque); > int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs, > int cpuid, void *opaque); > +bool ppc_cpu_interrupts_big_endian(CPUState *cs); > #ifndef CONFIG_USER_ONLY > extern const struct VMStateDescription vmstate_ppc_cpu; > #endif > diff --git a/target-ppc/misc_helper.c b/target-ppc/misc_helper.c > index 2eb2fa6..0e548ff 100644 > --- a/target-ppc/misc_helper.c > +++ b/target-ppc/misc_helper.c > @@ -120,3 +120,10 @@ void ppc_store_msr(CPUPPCState *env, target_ulong value) > { > hreg_store_msr(env, value, 0); > } > + > +bool ppc_cpu_interrupts_big_endian(CPUState *cs) > +{ > + PowerPCCPU *cpu = POWERPC_CPU(cs); > + PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); White line after variable declaration block, please. > + return pcc->interrupts_big_endian(&cpu->env); > +} > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c > index 823c63c..0d5492f 100644 > --- a/target-ppc/translate_init.c > +++ b/target-ppc/translate_init.c > @@ -3102,6 +3102,18 @@ static int check_pow_hid0_74xx (CPUPPCState *env) > return 0; > } > > +static bool interrupts_big_endian_always(CPUPPCState *env) ppc_cpu_... as a convention for a function accepting PowerPCCPU *cpu as main argument. Further, note that we are working towards compiling multiple targets into one binary, so at least ppc_... for non-static ones please. > +{ > + return true; > +} > + > +#ifdef TARGET_PPC64 > +static bool interrupts_big_endian_POWER7(CPUPPCState *env) Alex, do we prefer lowercase for new code? Otherwise looks okay to me - we had a lengthy discussion about the hook naming, but I admit some name is better than no functionality. I would like to re-raise the question though of whether it may make sense to do this at CPUClass level for the benefit of ARM? Could be refactored as follow-up, of course. Regards, Andreas > +{ > + return !(env->spr[SPR_LPCR] & LPCR_ILE); > +} > +#endif > + > /*****************************************************************************/ > /* PowerPC implementations definitions */ > > @@ -7097,6 +7109,7 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data) > POWERPC_FLAG_VSX; > pcc->l1_dcache_size = 0x8000; > pcc->l1_icache_size = 0x8000; > + pcc->interrupts_big_endian = interrupts_big_endian_POWER7; > } > > POWERPC_FAMILY(POWER7P)(ObjectClass *oc, void *data) > @@ -7140,6 +7153,7 @@ POWERPC_FAMILY(POWER7P)(ObjectClass *oc, void *data) > POWERPC_FLAG_VSX; > pcc->l1_dcache_size = 0x8000; > pcc->l1_icache_size = 0x8000; > + pcc->interrupts_big_endian = interrupts_big_endian_POWER7; > } > > static void init_proc_POWER8(CPUPPCState *env) > @@ -7197,6 +7211,7 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data) > POWERPC_FLAG_VSX; > pcc->l1_dcache_size = 0x8000; > pcc->l1_icache_size = 0x8000; > + pcc->interrupts_big_endian = interrupts_big_endian_POWER7; > } > #endif /* defined (TARGET_PPC64) */ > > @@ -8523,6 +8538,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data) > pcc->parent_realize = dc->realize; > pcc->pvr = CPU_POWERPC_DEFAULT_MASK; > pcc->pvr_mask = CPU_POWERPC_DEFAULT_MASK; > + pcc->interrupts_big_endian = interrupts_big_endian_always; > dc->realize = ppc_cpu_realizefn; > dc->unrealize = ppc_cpu_unrealizefn; >
On 28.04.14 14:47, Andreas Färber wrote: > [fixing Bharata's address] > > Am 28.04.2014 13:29, schrieb Greg Kurz: >> POWER7, POWER7+ and POWER8 families use the ILE bit of the LPCR >> special purpose register to decide the endianness to use when >> entering interrupt handlers. When running a linux guest, this >> provides a hint on the endianness used by the kernel. From a >> qemu point of view, the information is needed for legacy virtio > "QEMU" :) - and "Linux" while at it? > >> support and crash dump support as well. >> >> Suggested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> >> Suggested-by: Alexander Graf <agraf@suse.de> >> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> >> --- >> target-ppc/cpu-qom.h | 2 ++ >> target-ppc/misc_helper.c | 7 +++++++ >> target-ppc/translate_init.c | 16 ++++++++++++++++ >> 3 files changed, 25 insertions(+) >> >> diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h >> index 47dc8e6..c20473a 100644 >> --- a/target-ppc/cpu-qom.h >> +++ b/target-ppc/cpu-qom.h >> @@ -76,6 +76,7 @@ typedef struct PowerPCCPUClass { >> int (*handle_mmu_fault)(PowerPCCPU *cpu, target_ulong eaddr, int rwx, >> int mmu_idx); >> #endif >> + bool (*interrupts_big_endian)(CPUPPCState *env); > Please do not add *any* new functions with a CPUPPCState argument, > especially not in my shiny new PowerPCCPUClass. ;) > >> } PowerPCCPUClass; >> >> /** >> @@ -118,6 +119,7 @@ int ppc64_cpu_write_elf64_qemunote(WriteCoreDumpFunction f, >> CPUState *cpu, void *opaque); >> int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs, >> int cpuid, void *opaque); >> +bool ppc_cpu_interrupts_big_endian(CPUState *cs); >> #ifndef CONFIG_USER_ONLY >> extern const struct VMStateDescription vmstate_ppc_cpu; >> #endif >> diff --git a/target-ppc/misc_helper.c b/target-ppc/misc_helper.c >> index 2eb2fa6..0e548ff 100644 >> --- a/target-ppc/misc_helper.c >> +++ b/target-ppc/misc_helper.c >> @@ -120,3 +120,10 @@ void ppc_store_msr(CPUPPCState *env, target_ulong value) >> { >> hreg_store_msr(env, value, 0); >> } >> + >> +bool ppc_cpu_interrupts_big_endian(CPUState *cs) >> +{ >> + PowerPCCPU *cpu = POWERPC_CPU(cs); >> + PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); > White line after variable declaration block, please. > >> + return pcc->interrupts_big_endian(&cpu->env); >> +} >> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c >> index 823c63c..0d5492f 100644 >> --- a/target-ppc/translate_init.c >> +++ b/target-ppc/translate_init.c >> @@ -3102,6 +3102,18 @@ static int check_pow_hid0_74xx (CPUPPCState *env) >> return 0; >> } >> >> +static bool interrupts_big_endian_always(CPUPPCState *env) > ppc_cpu_... as a convention for a function accepting PowerPCCPU *cpu as > main argument. > > Further, note that we are working towards compiling multiple targets > into one binary, so at least ppc_... for non-static ones please. > >> +{ >> + return true; >> +} >> + >> +#ifdef TARGET_PPC64 >> +static bool interrupts_big_endian_POWER7(CPUPPCState *env) > Alex, do we prefer lowercase for new code? If this was a POWER7 only function I think the naming would make sense. But it isn't - it works on POWER8 just as well. Why not call it interrupts_big_endian_lpcr()? Alex
On Tue, 29 Apr 2014 11:14:12 +0200 Alexander Graf <agraf@suse.de> wrote: > > On 28.04.14 14:47, Andreas Färber wrote: > > [fixing Bharata's address] > > > > Am 28.04.2014 13:29, schrieb Greg Kurz: > >> POWER7, POWER7+ and POWER8 families use the ILE bit of the LPCR > >> special purpose register to decide the endianness to use when > >> entering interrupt handlers. When running a linux guest, this > >> provides a hint on the endianness used by the kernel. From a > >> qemu point of view, the information is needed for legacy virtio > > "QEMU" :) - and "Linux" while at it? > > > >> support and crash dump support as well. > >> > >> Suggested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > >> Suggested-by: Alexander Graf <agraf@suse.de> > >> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > >> --- > >> target-ppc/cpu-qom.h | 2 ++ > >> target-ppc/misc_helper.c | 7 +++++++ > >> target-ppc/translate_init.c | 16 ++++++++++++++++ > >> 3 files changed, 25 insertions(+) > >> > >> diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h > >> index 47dc8e6..c20473a 100644 > >> --- a/target-ppc/cpu-qom.h > >> +++ b/target-ppc/cpu-qom.h > >> @@ -76,6 +76,7 @@ typedef struct PowerPCCPUClass { > >> int (*handle_mmu_fault)(PowerPCCPU *cpu, target_ulong eaddr, int rwx, > >> int mmu_idx); > >> #endif > >> + bool (*interrupts_big_endian)(CPUPPCState *env); > > Please do not add *any* new functions with a CPUPPCState argument, > > especially not in my shiny new PowerPCCPUClass. ;) > > > >> } PowerPCCPUClass; > >> > >> /** > >> @@ -118,6 +119,7 @@ int ppc64_cpu_write_elf64_qemunote(WriteCoreDumpFunction f, > >> CPUState *cpu, void *opaque); > >> int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs, > >> int cpuid, void *opaque); > >> +bool ppc_cpu_interrupts_big_endian(CPUState *cs); > >> #ifndef CONFIG_USER_ONLY > >> extern const struct VMStateDescription vmstate_ppc_cpu; > >> #endif > >> diff --git a/target-ppc/misc_helper.c b/target-ppc/misc_helper.c > >> index 2eb2fa6..0e548ff 100644 > >> --- a/target-ppc/misc_helper.c > >> +++ b/target-ppc/misc_helper.c > >> @@ -120,3 +120,10 @@ void ppc_store_msr(CPUPPCState *env, target_ulong value) > >> { > >> hreg_store_msr(env, value, 0); > >> } > >> + > >> +bool ppc_cpu_interrupts_big_endian(CPUState *cs) > >> +{ > >> + PowerPCCPU *cpu = POWERPC_CPU(cs); > >> + PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); > > White line after variable declaration block, please. > > > >> + return pcc->interrupts_big_endian(&cpu->env); > >> +} > >> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c > >> index 823c63c..0d5492f 100644 > >> --- a/target-ppc/translate_init.c > >> +++ b/target-ppc/translate_init.c > >> @@ -3102,6 +3102,18 @@ static int check_pow_hid0_74xx (CPUPPCState *env) > >> return 0; > >> } > >> > >> +static bool interrupts_big_endian_always(CPUPPCState *env) > > ppc_cpu_... as a convention for a function accepting PowerPCCPU *cpu as > > main argument. > > > > Further, note that we are working towards compiling multiple targets > > into one binary, so at least ppc_... for non-static ones please. > > > >> +{ > >> + return true; > >> +} > >> + > >> +#ifdef TARGET_PPC64 > >> +static bool interrupts_big_endian_POWER7(CPUPPCState *env) > > Alex, do we prefer lowercase for new code? > > If this was a POWER7 only function I think the naming would make sense. > But it isn't - it works on POWER8 just as well. Why not call it > interrupts_big_endian_lpcr()? > Sure, I'll do this right now. > > Alex >
diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h index 47dc8e6..c20473a 100644 --- a/target-ppc/cpu-qom.h +++ b/target-ppc/cpu-qom.h @@ -76,6 +76,7 @@ typedef struct PowerPCCPUClass { int (*handle_mmu_fault)(PowerPCCPU *cpu, target_ulong eaddr, int rwx, int mmu_idx); #endif + bool (*interrupts_big_endian)(CPUPPCState *env); } PowerPCCPUClass; /** @@ -118,6 +119,7 @@ int ppc64_cpu_write_elf64_qemunote(WriteCoreDumpFunction f, CPUState *cpu, void *opaque); int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs, int cpuid, void *opaque); +bool ppc_cpu_interrupts_big_endian(CPUState *cs); #ifndef CONFIG_USER_ONLY extern const struct VMStateDescription vmstate_ppc_cpu; #endif diff --git a/target-ppc/misc_helper.c b/target-ppc/misc_helper.c index 2eb2fa6..0e548ff 100644 --- a/target-ppc/misc_helper.c +++ b/target-ppc/misc_helper.c @@ -120,3 +120,10 @@ void ppc_store_msr(CPUPPCState *env, target_ulong value) { hreg_store_msr(env, value, 0); } + +bool ppc_cpu_interrupts_big_endian(CPUState *cs) +{ + PowerPCCPU *cpu = POWERPC_CPU(cs); + PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); + return pcc->interrupts_big_endian(&cpu->env); +} diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c index 823c63c..0d5492f 100644 --- a/target-ppc/translate_init.c +++ b/target-ppc/translate_init.c @@ -3102,6 +3102,18 @@ static int check_pow_hid0_74xx (CPUPPCState *env) return 0; } +static bool interrupts_big_endian_always(CPUPPCState *env) +{ + return true; +} + +#ifdef TARGET_PPC64 +static bool interrupts_big_endian_POWER7(CPUPPCState *env) +{ + return !(env->spr[SPR_LPCR] & LPCR_ILE); +} +#endif + /*****************************************************************************/ /* PowerPC implementations definitions */ @@ -7097,6 +7109,7 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data) POWERPC_FLAG_VSX; pcc->l1_dcache_size = 0x8000; pcc->l1_icache_size = 0x8000; + pcc->interrupts_big_endian = interrupts_big_endian_POWER7; } POWERPC_FAMILY(POWER7P)(ObjectClass *oc, void *data) @@ -7140,6 +7153,7 @@ POWERPC_FAMILY(POWER7P)(ObjectClass *oc, void *data) POWERPC_FLAG_VSX; pcc->l1_dcache_size = 0x8000; pcc->l1_icache_size = 0x8000; + pcc->interrupts_big_endian = interrupts_big_endian_POWER7; } static void init_proc_POWER8(CPUPPCState *env) @@ -7197,6 +7211,7 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data) POWERPC_FLAG_VSX; pcc->l1_dcache_size = 0x8000; pcc->l1_icache_size = 0x8000; + pcc->interrupts_big_endian = interrupts_big_endian_POWER7; } #endif /* defined (TARGET_PPC64) */ @@ -8523,6 +8538,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data) pcc->parent_realize = dc->realize; pcc->pvr = CPU_POWERPC_DEFAULT_MASK; pcc->pvr_mask = CPU_POWERPC_DEFAULT_MASK; + pcc->interrupts_big_endian = interrupts_big_endian_always; dc->realize = ppc_cpu_realizefn; dc->unrealize = ppc_cpu_unrealizefn;
POWER7, POWER7+ and POWER8 families use the ILE bit of the LPCR special purpose register to decide the endianness to use when entering interrupt handlers. When running a linux guest, this provides a hint on the endianness used by the kernel. From a qemu point of view, the information is needed for legacy virtio support and crash dump support as well. Suggested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> Suggested-by: Alexander Graf <agraf@suse.de> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> --- target-ppc/cpu-qom.h | 2 ++ target-ppc/misc_helper.c | 7 +++++++ target-ppc/translate_init.c | 16 ++++++++++++++++ 3 files changed, 25 insertions(+)