| Message ID | 1376537732-29300-1-git-send-email-aik@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
Am 15.08.2013 um 05:35 schrieb Alexey Kardashevskiy <aik@ozlabs.ru>: > IBM POWERPC processors encode PVR as a CPU family in higher 16 bits and > a CPU version in lower 16 bits. Since there is no significant change > in behavior between versions, there is no point to add every single CPU > version in QEMU's CPU list. Also, new CPU versions of already supported > CPU won't break the existing code. > > This adds a PVR mask support. POWER7, POWER7+ and POWER8 CPUs > definitions converted to use masks. How does the user select that he wants a v2.3 p7 cpu with this patch? Alex > > Cc: Andreas Färber <afaerber@suse.de> > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > > The patch does basically what the kernel does in arch/powerpc/kernel/cputable.c. > > --- > target-ppc/cpu-models.c | 26 +++++++++++++++----------- > target-ppc/cpu-models.h | 12 +++++++----- > target-ppc/cpu-qom.h | 1 + > target-ppc/translate_init.c | 2 +- > 4 files changed, 24 insertions(+), 17 deletions(-) > > diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c > index 8dea560..c40cf04 100644 > --- a/target-ppc/cpu-models.c > +++ b/target-ppc/cpu-models.c > @@ -35,7 +35,7 @@ > /* PowerPC CPU definitions */ > #define POWERPC_DEF_PREFIX(pvr, svr, type) \ > glue(glue(glue(glue(pvr, _), svr), _), type) > -#define POWERPC_DEF_SVR(_name, _desc, _pvr, _svr, _type) \ > +#define POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, _pvr_mask, _svr, _type) \ > static void \ > glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_class_init) \ > (ObjectClass *oc, void *data) \ > @@ -44,6 +44,7 @@ > PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc); \ > \ > pcc->pvr = _pvr; \ > + pcc->pvr_mask = _pvr_mask; \ > pcc->svr = _svr; \ > dc->desc = _desc; \ > } \ > @@ -66,9 +67,16 @@ > type_init( \ > glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_register_types)) > > +#define POWERPC_DEF_SVR(_name, _desc, _pvr, _svr, _type) \ > + POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, CPU_POWERPC_DEFAULT_MASK, \ > + _svr, _type) > + > #define POWERPC_DEF(_name, _pvr, _type, _desc) \ > POWERPC_DEF_SVR(_name, _desc, _pvr, POWERPC_SVR_NONE, _type) > > +#define POWERPC_DEF_MASK(_name, _pvr, _pvr_mask, _type, _desc) \ > + POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, _pvr_mask, POWERPC_SVR_NONE, _type) > + > /* Embedded PowerPC */ > /* PowerPC 401 family */ > POWERPC_DEF("401", CPU_POWERPC_401, 401, > @@ -1133,16 +1141,12 @@ > POWERPC_DEF("POWER6A", CPU_POWERPC_POWER6A, POWER6, > "POWER6A") > #endif > - POWERPC_DEF("POWER7_v2.0", CPU_POWERPC_POWER7_v20, POWER7, > - "POWER7 v2.0") > - POWERPC_DEF("POWER7_v2.1", CPU_POWERPC_POWER7_v21, POWER7, > - "POWER7 v2.1") > - POWERPC_DEF("POWER7_v2.3", CPU_POWERPC_POWER7_v23, POWER7, > - "POWER7 v2.3") > - POWERPC_DEF("POWER7+_v2.1", CPU_POWERPC_POWER7P_v21, POWER7, > - "POWER7+ v2.1") > - POWERPC_DEF("POWER8_v1.0", CPU_POWERPC_POWER8_v10, POWER8, > - "POWER8 v1.0") > + POWERPC_DEF_MASK("POWER7", CPU_POWERPC_POWER7, CPU_POWERPC_POWER7_MASK, > + POWER7, "POWER7") > + POWERPC_DEF_MASK("POWER7+", CPU_POWERPC_POWER7P, CPU_POWERPC_POWER7P_MASK, > + POWER7, "POWER7") > + POWERPC_DEF_MASK("POWER8", CPU_POWERPC_POWER8, CPU_POWERPC_POWER8_MASK, > + POWER8, "POWER8") > POWERPC_DEF("970", CPU_POWERPC_970, 970, > "PowerPC 970") > POWERPC_DEF("970fx_v1.0", CPU_POWERPC_970FX_v10, 970FX, > diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h > index d9145d1..b322063 100644 > --- a/target-ppc/cpu-models.h > +++ b/target-ppc/cpu-models.h > @@ -39,6 +39,7 @@ extern PowerPCCPUAlias ppc_cpu_aliases[]; > /*****************************************************************************/ > /* PVR definitions for most known PowerPC */ > enum { > + CPU_POWERPC_DEFAULT_MASK = 0xFFFFFFFF, > /* PowerPC 401 family */ > /* Generic PowerPC 401 */ > #define CPU_POWERPC_401 CPU_POWERPC_401G2 > @@ -552,11 +553,12 @@ enum { > CPU_POWERPC_POWER6 = 0x003E0000, > CPU_POWERPC_POWER6_5 = 0x0F000001, /* POWER6 in POWER5 mode */ > CPU_POWERPC_POWER6A = 0x0F000002, > - CPU_POWERPC_POWER7_v20 = 0x003F0200, > - CPU_POWERPC_POWER7_v21 = 0x003F0201, > - CPU_POWERPC_POWER7_v23 = 0x003F0203, > - CPU_POWERPC_POWER7P_v21 = 0x004A0201, > - CPU_POWERPC_POWER8_v10 = 0x004B0100, > + CPU_POWERPC_POWER7 = 0x003F0000, > + CPU_POWERPC_POWER7_MASK = 0xFFFF0000, > + CPU_POWERPC_POWER7P = 0x004A0000, > + CPU_POWERPC_POWER7P_MASK = 0xFFFF0000, > + CPU_POWERPC_POWER8 = 0x004B0000, > + CPU_POWERPC_POWER8_MASK = 0xFFFF0000, > CPU_POWERPC_970 = 0x00390202, > CPU_POWERPC_970FX_v10 = 0x00391100, > CPU_POWERPC_970FX_v20 = 0x003C0200, > diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h > index f3c710a..0ae8b09 100644 > --- a/target-ppc/cpu-qom.h > +++ b/target-ppc/cpu-qom.h > @@ -54,6 +54,7 @@ typedef struct PowerPCCPUClass { > void (*parent_reset)(CPUState *cpu); > > uint32_t pvr; > + uint32_t pvr_mask; > uint32_t svr; > uint64_t insns_flags; > uint64_t insns_flags2; > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c > index 13b290c..01dffa6 100644 > --- a/target-ppc/translate_init.c > +++ b/target-ppc/translate_init.c > @@ -8161,7 +8161,7 @@ static gint ppc_cpu_compare_class_pvr(gconstpointer a, gconstpointer b) > } > #endif > > - return pcc->pvr == pvr ? 0 : -1; > + return pcc->pvr == (pvr & pcc->pvr_mask) ? 0 : -1; > } > > PowerPCCPUClass *ppc_cpu_class_by_pvr(uint32_t pvr) > -- > 1.8.3.2 >
On 08/15/2013 03:21 PM, Alexander Graf wrote: > > > Am 15.08.2013 um 05:35 schrieb Alexey Kardashevskiy <aik@ozlabs.ru>: > >> IBM POWERPC processors encode PVR as a CPU family in higher 16 bits and >> a CPU version in lower 16 bits. Since there is no significant change >> in behavior between versions, there is no point to add every single CPU >> version in QEMU's CPU list. Also, new CPU versions of already supported >> CPU won't break the existing code. >> >> This adds a PVR mask support. POWER7, POWER7+ and POWER8 CPUs >> definitions converted to use masks. > > How does the user select that he wants a v2.3 p7 cpu with this patch? Why would he want that? The behaviour would not change because of the version - all definitions use the same POWERPC_FAMILY(POWER7) and PVR is not virtualized anyway. May be (may be) ppc_cpu_class_by_name() needs to be fixed to try to find the PPC CPU class with the biggest mask to choose (for example) 004a0201/ffffffff rather than more common 004a0000/ffff0000 (if 004a0201 is added to the list separately from the common definition for some reason). btw the patch is incomplete - aliases should be removed too. > > Alex > >> >> Cc: Andreas Färber <afaerber@suse.de> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> --- >> >> The patch does basically what the kernel does in arch/powerpc/kernel/cputable.c. >> >> --- >> target-ppc/cpu-models.c | 26 +++++++++++++++----------- >> target-ppc/cpu-models.h | 12 +++++++----- >> target-ppc/cpu-qom.h | 1 + >> target-ppc/translate_init.c | 2 +- >> 4 files changed, 24 insertions(+), 17 deletions(-) >> >> diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c >> index 8dea560..c40cf04 100644 >> --- a/target-ppc/cpu-models.c >> +++ b/target-ppc/cpu-models.c >> @@ -35,7 +35,7 @@ >> /* PowerPC CPU definitions */ >> #define POWERPC_DEF_PREFIX(pvr, svr, type) \ >> glue(glue(glue(glue(pvr, _), svr), _), type) >> -#define POWERPC_DEF_SVR(_name, _desc, _pvr, _svr, _type) \ >> +#define POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, _pvr_mask, _svr, _type) \ >> static void \ >> glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_class_init) \ >> (ObjectClass *oc, void *data) \ >> @@ -44,6 +44,7 @@ >> PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc); \ >> \ >> pcc->pvr = _pvr; \ >> + pcc->pvr_mask = _pvr_mask; \ >> pcc->svr = _svr; \ >> dc->desc = _desc; \ >> } \ >> @@ -66,9 +67,16 @@ >> type_init( \ >> glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_register_types)) >> >> +#define POWERPC_DEF_SVR(_name, _desc, _pvr, _svr, _type) \ >> + POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, CPU_POWERPC_DEFAULT_MASK, \ >> + _svr, _type) >> + >> #define POWERPC_DEF(_name, _pvr, _type, _desc) \ >> POWERPC_DEF_SVR(_name, _desc, _pvr, POWERPC_SVR_NONE, _type) >> >> +#define POWERPC_DEF_MASK(_name, _pvr, _pvr_mask, _type, _desc) \ >> + POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, _pvr_mask, POWERPC_SVR_NONE, _type) >> + >> /* Embedded PowerPC */ >> /* PowerPC 401 family */ >> POWERPC_DEF("401", CPU_POWERPC_401, 401, >> @@ -1133,16 +1141,12 @@ >> POWERPC_DEF("POWER6A", CPU_POWERPC_POWER6A, POWER6, >> "POWER6A") >> #endif >> - POWERPC_DEF("POWER7_v2.0", CPU_POWERPC_POWER7_v20, POWER7, >> - "POWER7 v2.0") >> - POWERPC_DEF("POWER7_v2.1", CPU_POWERPC_POWER7_v21, POWER7, >> - "POWER7 v2.1") >> - POWERPC_DEF("POWER7_v2.3", CPU_POWERPC_POWER7_v23, POWER7, >> - "POWER7 v2.3") >> - POWERPC_DEF("POWER7+_v2.1", CPU_POWERPC_POWER7P_v21, POWER7, >> - "POWER7+ v2.1") >> - POWERPC_DEF("POWER8_v1.0", CPU_POWERPC_POWER8_v10, POWER8, >> - "POWER8 v1.0") >> + POWERPC_DEF_MASK("POWER7", CPU_POWERPC_POWER7, CPU_POWERPC_POWER7_MASK, >> + POWER7, "POWER7") >> + POWERPC_DEF_MASK("POWER7+", CPU_POWERPC_POWER7P, CPU_POWERPC_POWER7P_MASK, >> + POWER7, "POWER7") >> + POWERPC_DEF_MASK("POWER8", CPU_POWERPC_POWER8, CPU_POWERPC_POWER8_MASK, >> + POWER8, "POWER8") >> POWERPC_DEF("970", CPU_POWERPC_970, 970, >> "PowerPC 970") >> POWERPC_DEF("970fx_v1.0", CPU_POWERPC_970FX_v10, 970FX, >> diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h >> index d9145d1..b322063 100644 >> --- a/target-ppc/cpu-models.h >> +++ b/target-ppc/cpu-models.h >> @@ -39,6 +39,7 @@ extern PowerPCCPUAlias ppc_cpu_aliases[]; >> /*****************************************************************************/ >> /* PVR definitions for most known PowerPC */ >> enum { >> + CPU_POWERPC_DEFAULT_MASK = 0xFFFFFFFF, >> /* PowerPC 401 family */ >> /* Generic PowerPC 401 */ >> #define CPU_POWERPC_401 CPU_POWERPC_401G2 >> @@ -552,11 +553,12 @@ enum { >> CPU_POWERPC_POWER6 = 0x003E0000, >> CPU_POWERPC_POWER6_5 = 0x0F000001, /* POWER6 in POWER5 mode */ >> CPU_POWERPC_POWER6A = 0x0F000002, >> - CPU_POWERPC_POWER7_v20 = 0x003F0200, >> - CPU_POWERPC_POWER7_v21 = 0x003F0201, >> - CPU_POWERPC_POWER7_v23 = 0x003F0203, >> - CPU_POWERPC_POWER7P_v21 = 0x004A0201, >> - CPU_POWERPC_POWER8_v10 = 0x004B0100, >> + CPU_POWERPC_POWER7 = 0x003F0000, >> + CPU_POWERPC_POWER7_MASK = 0xFFFF0000, >> + CPU_POWERPC_POWER7P = 0x004A0000, >> + CPU_POWERPC_POWER7P_MASK = 0xFFFF0000, >> + CPU_POWERPC_POWER8 = 0x004B0000, >> + CPU_POWERPC_POWER8_MASK = 0xFFFF0000, >> CPU_POWERPC_970 = 0x00390202, >> CPU_POWERPC_970FX_v10 = 0x00391100, >> CPU_POWERPC_970FX_v20 = 0x003C0200, >> diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h >> index f3c710a..0ae8b09 100644 >> --- a/target-ppc/cpu-qom.h >> +++ b/target-ppc/cpu-qom.h >> @@ -54,6 +54,7 @@ typedef struct PowerPCCPUClass { >> void (*parent_reset)(CPUState *cpu); >> >> uint32_t pvr; >> + uint32_t pvr_mask; >> uint32_t svr; >> uint64_t insns_flags; >> uint64_t insns_flags2; >> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c >> index 13b290c..01dffa6 100644 >> --- a/target-ppc/translate_init.c >> +++ b/target-ppc/translate_init.c >> @@ -8161,7 +8161,7 @@ static gint ppc_cpu_compare_class_pvr(gconstpointer a, gconstpointer b) >> } >> #endif >> >> - return pcc->pvr == pvr ? 0 : -1; >> + return pcc->pvr == (pvr & pcc->pvr_mask) ? 0 : -1; >> } >> >> PowerPCCPUClass *ppc_cpu_class_by_pvr(uint32_t pvr) >> -- >> 1.8.3.2 >>
On Thu, 2013-08-15 at 07:21 +0200, Alexander Graf wrote: > > Am 15.08.2013 um 05:35 schrieb Alexey Kardashevskiy <aik@ozlabs.ru>: > > > IBM POWERPC processors encode PVR as a CPU family in higher 16 bits and > > a CPU version in lower 16 bits. Since there is no significant change > > in behavior between versions, there is no point to add every single CPU > > version in QEMU's CPU list. Also, new CPU versions of already supported > > CPU won't break the existing code. > > > > This adds a PVR mask support. POWER7, POWER7+ and POWER8 CPUs > > definitions converted to use masks. > > How does the user select that he wants a v2.3 p7 cpu with this patch? It's a tradeoff. We are trading a useless feature (which you describe above) for a functional system that doesn't require to have an entry for every possible chip revision ever designed and to be designed in the future for things to work. I know the qemu folks is generally uninterested in making things actually work but heh .. :-) Cheers, Ben. > Alex > > > > > Cc: Andreas Färber <afaerber@suse.de> > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > --- > > > > The patch does basically what the kernel does in arch/powerpc/kernel/cputable.c. > > > > --- > > target-ppc/cpu-models.c | 26 +++++++++++++++----------- > > target-ppc/cpu-models.h | 12 +++++++----- > > target-ppc/cpu-qom.h | 1 + > > target-ppc/translate_init.c | 2 +- > > 4 files changed, 24 insertions(+), 17 deletions(-) > > > > diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c > > index 8dea560..c40cf04 100644 > > --- a/target-ppc/cpu-models.c > > +++ b/target-ppc/cpu-models.c > > @@ -35,7 +35,7 @@ > > /* PowerPC CPU definitions */ > > #define POWERPC_DEF_PREFIX(pvr, svr, type) \ > > glue(glue(glue(glue(pvr, _), svr), _), type) > > -#define POWERPC_DEF_SVR(_name, _desc, _pvr, _svr, _type) \ > > +#define POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, _pvr_mask, _svr, _type) \ > > static void \ > > glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_class_init) \ > > (ObjectClass *oc, void *data) \ > > @@ -44,6 +44,7 @@ > > PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc); \ > > \ > > pcc->pvr = _pvr; \ > > + pcc->pvr_mask = _pvr_mask; \ > > pcc->svr = _svr; \ > > dc->desc = _desc; \ > > } \ > > @@ -66,9 +67,16 @@ > > type_init( \ > > glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_register_types)) > > > > +#define POWERPC_DEF_SVR(_name, _desc, _pvr, _svr, _type) \ > > + POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, CPU_POWERPC_DEFAULT_MASK, \ > > + _svr, _type) > > + > > #define POWERPC_DEF(_name, _pvr, _type, _desc) \ > > POWERPC_DEF_SVR(_name, _desc, _pvr, POWERPC_SVR_NONE, _type) > > > > +#define POWERPC_DEF_MASK(_name, _pvr, _pvr_mask, _type, _desc) \ > > + POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, _pvr_mask, POWERPC_SVR_NONE, _type) > > + > > /* Embedded PowerPC */ > > /* PowerPC 401 family */ > > POWERPC_DEF("401", CPU_POWERPC_401, 401, > > @@ -1133,16 +1141,12 @@ > > POWERPC_DEF("POWER6A", CPU_POWERPC_POWER6A, POWER6, > > "POWER6A") > > #endif > > - POWERPC_DEF("POWER7_v2.0", CPU_POWERPC_POWER7_v20, POWER7, > > - "POWER7 v2.0") > > - POWERPC_DEF("POWER7_v2.1", CPU_POWERPC_POWER7_v21, POWER7, > > - "POWER7 v2.1") > > - POWERPC_DEF("POWER7_v2.3", CPU_POWERPC_POWER7_v23, POWER7, > > - "POWER7 v2.3") > > - POWERPC_DEF("POWER7+_v2.1", CPU_POWERPC_POWER7P_v21, POWER7, > > - "POWER7+ v2.1") > > - POWERPC_DEF("POWER8_v1.0", CPU_POWERPC_POWER8_v10, POWER8, > > - "POWER8 v1.0") > > + POWERPC_DEF_MASK("POWER7", CPU_POWERPC_POWER7, CPU_POWERPC_POWER7_MASK, > > + POWER7, "POWER7") > > + POWERPC_DEF_MASK("POWER7+", CPU_POWERPC_POWER7P, CPU_POWERPC_POWER7P_MASK, > > + POWER7, "POWER7") > > + POWERPC_DEF_MASK("POWER8", CPU_POWERPC_POWER8, CPU_POWERPC_POWER8_MASK, > > + POWER8, "POWER8") > > POWERPC_DEF("970", CPU_POWERPC_970, 970, > > "PowerPC 970") > > POWERPC_DEF("970fx_v1.0", CPU_POWERPC_970FX_v10, 970FX, > > diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h > > index d9145d1..b322063 100644 > > --- a/target-ppc/cpu-models.h > > +++ b/target-ppc/cpu-models.h > > @@ -39,6 +39,7 @@ extern PowerPCCPUAlias ppc_cpu_aliases[]; > > /*****************************************************************************/ > > /* PVR definitions for most known PowerPC */ > > enum { > > + CPU_POWERPC_DEFAULT_MASK = 0xFFFFFFFF, > > /* PowerPC 401 family */ > > /* Generic PowerPC 401 */ > > #define CPU_POWERPC_401 CPU_POWERPC_401G2 > > @@ -552,11 +553,12 @@ enum { > > CPU_POWERPC_POWER6 = 0x003E0000, > > CPU_POWERPC_POWER6_5 = 0x0F000001, /* POWER6 in POWER5 mode */ > > CPU_POWERPC_POWER6A = 0x0F000002, > > - CPU_POWERPC_POWER7_v20 = 0x003F0200, > > - CPU_POWERPC_POWER7_v21 = 0x003F0201, > > - CPU_POWERPC_POWER7_v23 = 0x003F0203, > > - CPU_POWERPC_POWER7P_v21 = 0x004A0201, > > - CPU_POWERPC_POWER8_v10 = 0x004B0100, > > + CPU_POWERPC_POWER7 = 0x003F0000, > > + CPU_POWERPC_POWER7_MASK = 0xFFFF0000, > > + CPU_POWERPC_POWER7P = 0x004A0000, > > + CPU_POWERPC_POWER7P_MASK = 0xFFFF0000, > > + CPU_POWERPC_POWER8 = 0x004B0000, > > + CPU_POWERPC_POWER8_MASK = 0xFFFF0000, > > CPU_POWERPC_970 = 0x00390202, > > CPU_POWERPC_970FX_v10 = 0x00391100, > > CPU_POWERPC_970FX_v20 = 0x003C0200, > > diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h > > index f3c710a..0ae8b09 100644 > > --- a/target-ppc/cpu-qom.h > > +++ b/target-ppc/cpu-qom.h > > @@ -54,6 +54,7 @@ typedef struct PowerPCCPUClass { > > void (*parent_reset)(CPUState *cpu); > > > > uint32_t pvr; > > + uint32_t pvr_mask; > > uint32_t svr; > > uint64_t insns_flags; > > uint64_t insns_flags2; > > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c > > index 13b290c..01dffa6 100644 > > --- a/target-ppc/translate_init.c > > +++ b/target-ppc/translate_init.c > > @@ -8161,7 +8161,7 @@ static gint ppc_cpu_compare_class_pvr(gconstpointer a, gconstpointer b) > > } > > #endif > > > > - return pcc->pvr == pvr ? 0 : -1; > > + return pcc->pvr == (pvr & pcc->pvr_mask) ? 0 : -1; > > } > > > > PowerPCCPUClass *ppc_cpu_class_by_pvr(uint32_t pvr) > > -- > > 1.8.3.2 > >
On 15.08.2013, at 07:21, Alexander Graf wrote: > > > Am 15.08.2013 um 05:35 schrieb Alexey Kardashevskiy <aik@ozlabs.ru>: > >> IBM POWERPC processors encode PVR as a CPU family in higher 16 bits and >> a CPU version in lower 16 bits. Since there is no significant change >> in behavior between versions, there is no point to add every single CPU >> version in QEMU's CPU list. Also, new CPU versions of already supported >> CPU won't break the existing code. >> >> This adds a PVR mask support. POWER7, POWER7+ and POWER8 CPUs >> definitions converted to use masks. > > How does the user select that he wants a v2.3 p7 cpu with this patch? I figured I should be a bit more verbose here ;). Today, we have a class hierarchy that looks like this: PowerPC |- POWER7 v2.2 `- POWER7 v2.3 with both POWER7 CPUs referencing to the same class_init function. With your patch, you're changing this to PowerPC `- POWER7 so we lose all information about major/minor versions. Now what I think you want is something like PowerPC `- POWER7 |- POWER7 v2.2 `- POWER7 v2.3 Here the POWER7 class can expose PVR mask values which -cpu host can use to match against it. -cpu host can then instantiate a "generic" POWER7 object with the host's PVR value if it matches. But we still keep the versioned CPU classes around for the user to chose from. This goes even further. We could implement something like -cpu POWER7,major=2,minor=3 which should also give us a POWER7 v2.3 CPU and essentially be the same as the POWER7 v2.3 class. Instantiating -cpu POWER7 without parameters should probably default to the latest revision that we're aware of. We can't just remove the POWER7_v23 type because users may depend on its availability. Maybe once we implement the above we can declare the explicitly versioned CPU types deprecated and remove them 1-2 years later. Alex
On 15.08.2013, at 07:44, Alexey Kardashevskiy wrote: > On 08/15/2013 03:21 PM, Alexander Graf wrote: >> >> >> Am 15.08.2013 um 05:35 schrieb Alexey Kardashevskiy <aik@ozlabs.ru>: >> >>> IBM POWERPC processors encode PVR as a CPU family in higher 16 bits and >>> a CPU version in lower 16 bits. Since there is no significant change >>> in behavior between versions, there is no point to add every single CPU >>> version in QEMU's CPU list. Also, new CPU versions of already supported >>> CPU won't break the existing code. >>> >>> This adds a PVR mask support. POWER7, POWER7+ and POWER8 CPUs >>> definitions converted to use masks. >> >> How does the user select that he wants a v2.3 p7 cpu with this patch? > > Why would he want that? The behaviour would not change because of the > version - all definitions use the same POWERPC_FAMILY(POWER7) and PVR is > not virtualized anyway. Quite frankly I don't know what to say here. Are you trying to play dumb or are you just one of those totally sloppy people who don't care about anything outside of their own scope of work? With HV KVM we can not trap PVR, yes. With PR KVM we do trap PVR and we emulate it. With TCG we do trap PVR and we emulate it. > May be (may be) ppc_cpu_class_by_name() needs to be fixed to try to find > the PPC CPU class with the biggest mask to choose (for example) > 004a0201/ffffffff rather than more common 004a0000/ffff0000 (if 004a0201 is > added to the list separately from the common definition for some reason). I think the class split as Linux has it should work just fine, no? Alex
On 15.08.2013, at 07:54, Benjamin Herrenschmidt wrote: > On Thu, 2013-08-15 at 07:21 +0200, Alexander Graf wrote: >> >> Am 15.08.2013 um 05:35 schrieb Alexey Kardashevskiy <aik@ozlabs.ru>: >> >>> IBM POWERPC processors encode PVR as a CPU family in higher 16 bits and >>> a CPU version in lower 16 bits. Since there is no significant change >>> in behavior between versions, there is no point to add every single CPU >>> version in QEMU's CPU list. Also, new CPU versions of already supported >>> CPU won't break the existing code. >>> >>> This adds a PVR mask support. POWER7, POWER7+ and POWER8 CPUs >>> definitions converted to use masks. >> >> How does the user select that he wants a v2.3 p7 cpu with this patch? > > It's a tradeoff. We are trading a useless feature (which you describe > above) for a functional system that doesn't require to have an entry for > every possible chip revision ever designed and to be designed in the > future for things to work. So you're saying it's good to remove a well established feature on 5% of the supported CPUs, leave the others inconsistent with the change and then declare the whole thing an improvement? > I know the qemu folks is generally uninterested in making things > actually work but heh .. :-) I disagree :). Alex
On Thu, 2013-08-15 at 08:10 +0200, Alexander Graf wrote: > So you're saying it's good to remove a well established feature on 5% > of the supported CPUs, leave the others inconsistent with the change > and then declare the whole thing an improvement? WTF are you talking about ? To need an exact PVR definition to the last bit means every time a minor chip rev comes out of IBM, KVM will stop working until qemu is updated to know about that revision. This is dumb. Being able to emulate a P7 2.1 vs a P7 2.3 is completely pointless since essentially they expose the same architecture and the bugs that are fixed between those revisions are for the most part not guest visible nor even emulated by qemu to begin with. Now there *might* be some value in being able to specify among "known supported" versions for things like P5 (but frankly, who gives a damn ? Who is actually going to *use* that ? Nobody really ....) In that case it's easy ... have a name match with the table entry. With the mask & value, you can do like the kernel, ie, have first in the list the specific entries you want to match against (ie, P7_2_1, P7_2_3, ...) and fallback to a generic "P7 any revision" entry. That way qemu will still work if IBM releases a P7 v2.4 you don't know about. As for selecting it, similarly, you can do an exact match on the name (or a partial match as a fallback, I don't care) and pickup the PVR value out of the table for emulation. Point is, what we have now is crap. This is the best fix I've seen so far. It's useful, cover the 99.9% of the possible use cases I can think of, but you seem to care more about hypothetical scenario that have no practical interest on the field. Ben.
On Thu, 2013-08-15 at 08:03 +0200, Alexander Graf wrote: > >> How does the user select that he wants a v2.3 p7 cpu with this > patch? > > > > Why would he want that? The behaviour would not change because of > the > > version - all definitions use the same POWERPC_FAMILY(POWER7) and > PVR is > > not virtualized anyway. > > Quite frankly I don't know what to say here. Are you trying to play > dumb or are you just one of those totally sloppy people who don't care > about anything outside of their own scope of work? Can you stop the bloody personal attacks on Alexey ? It's becoming tiresome. He makes a very valid point. The ability to specify a specific revision of the processor is pointless for pretty much any use case we have in mind at the moment, and is even more pointless as long as we emulate them all exactly the same way. Besides, we can probably still organize the table from "more precise" to "less precise" entries and match that way if you *really* want to have specific entries for obscure revision of the chip. Ben. > With HV KVM we can not trap PVR, yes. With PR KVM we do trap PVR and > we emulate it. With TCG we do trap PVR and we emulate it. > > > May be (may be) ppc_cpu_class_by_name() needs to be fixed to try to > find > > the PPC CPU class with the biggest mask to choose (for example) > > 004a0201/ffffffff rather than more common 004a0000/ffff0000 (if > 004a0201 is > > added to the list separately from the common definition for some > reason). > > I think the class split as Linux has it should work just fine, no? > > > Alex
On 15.08.2013, at 08:28, Benjamin Herrenschmidt wrote: > On Thu, 2013-08-15 at 08:10 +0200, Alexander Graf wrote: > >> So you're saying it's good to remove a well established feature on 5% >> of the supported CPUs, leave the others inconsistent with the change >> and then declare the whole thing an improvement? > > WTF are you talking about ? > > To need an exact PVR definition to the last bit means every time a minor > chip rev comes out of IBM, KVM will stop working until qemu is updated > to know about that revision. > > This is dumb. No disagreement here. > > Being able to emulate a P7 2.1 vs a P7 2.3 is completely pointless since > essentially they expose the same architecture and the bugs that are > fixed between those revisions are for the most part not guest visible > nor even emulated by qemu to begin with. > > Now there *might* be some value in being able to specify among "known > supported" versions for things like P5 (but frankly, who gives a damn ? > Who is actually going to *use* that ? Nobody really ....) > > In that case it's easy ... have a name match with the table entry. Have you read my previous reply? > With the mask & value, you can do like the kernel, ie, have first in the > list the specific entries you want to match against (ie, P7_2_1, > P7_2_3, ...) and fallback to a generic "P7 any revision" entry. That way > qemu will still work if IBM releases a P7 v2.4 you don't know about. > > As for selecting it, similarly, you can do an exact match on the name > (or a partial match as a fallback, I don't care) and pickup the PVR > value out of the table for emulation. > > Point is, what we have now is crap. This is the best fix I've seen so > far. It's useful, cover the 99.9% of the possible use cases I can think > of, but you seem to care more about hypothetical scenario that have no > practical interest on the field. The patch makes things inconsistent. It moves POWER7, POWER7+ and POWER8 to a different model from everything else and removes already existing -cpu names. This is just plain wrong. I want a consistent solution that's future proof. Alex
On 15.08.2013, at 08:30, Benjamin Herrenschmidt wrote: > On Thu, 2013-08-15 at 08:03 +0200, Alexander Graf wrote: > >>>> How does the user select that he wants a v2.3 p7 cpu with this >> patch? >>> >>> Why would he want that? The behaviour would not change because of >> the >>> version - all definitions use the same POWERPC_FAMILY(POWER7) and >> PVR is >>> not virtualized anyway. >> >> Quite frankly I don't know what to say here. Are you trying to play >> dumb or are you just one of those totally sloppy people who don't care >> about anything outside of their own scope of work? > > Can you stop the bloody personal attacks on Alexey ? It's becoming > tiresome. No, Alexey's ignorance is pretty tiresome. > He makes a very valid point. The ability to specify a specific revision > of the processor is pointless for pretty much any use case we have in > mind at the moment, and is even more pointless as long as we emulate > them all exactly the same way. The assessment that PVR does not get virtualized is simply not true. I agree that we need a way to match more generic CPU classes, as I've written in my previous reply. > Besides, we can probably still organize the table from "more precise" to > "less precise" entries and match that way if you *really* want to have > specific entries for obscure revision of the chip. So you really haven't read it. All we need is an intermediate class level that can match multiple CPUs and we should be good. Alex
Benjamin Herrenschmidt <benh@kernel.crashing.org> writes: > On Thu, 2013-08-15 at 08:03 +0200, Alexander Graf wrote: > >> >> How does the user select that he wants a v2.3 p7 cpu with this >> patch? >> > >> > Why would he want that? The behaviour would not change because of >> the >> > version - all definitions use the same POWERPC_FAMILY(POWER7) and >> PVR is >> > not virtualized anyway. >> >> Quite frankly I don't know what to say here. Are you trying to play >> dumb or are you just one of those totally sloppy people who don't care >> about anything outside of their own scope of work? > > Can you stop the bloody personal attacks on Alexey ? It's becoming > tiresome. First, everyone needs to tone it down a couple levels. This isn't LKML and Jonathan Corbet doesn't read qemu-devel so pithy remarks aren't going to get you a quote of the week. Everyone is talking past each other and no one is addressing the real problem. There are two distinct issues here: 1) We have two ABIs that cannot be changed unless there's a very good reason to. Alexey's original patch breaks both. The guest ABI cannot change given a fixed command line. IOW, the exposed PVR value for -cpu POWER7 cannot change across versions of QEMU or when running on different hardware. This breaks live migration and save/resume. We also cannot break the command line interface. If the last version of QEMU supported -cpu POWER7_v2.1, then we must continue to support that. If there's a good reason to break either of these, that's fine but that justification needs be up front in the patch commit message. 2) The only "-cpu" that makes sense is "-cpu host" for KVM on HV (or whatever ya'll call it). POWER does not have the ability to virtualize the hardware PVR value. There is a virtual PVR in the device tree but that's orthogonal to what we think of as the VCPU (it essentially means IIUC that the cpu is compatible with that PVR). We should explicitly disallow any -cpu value when KVM on HV is enabled other than host. The implementation of "-cpu host" is also goofy on PPC. -cpu host does a match on existing CPU models meaning that we have to define a CPU model for any possible CPU we run on. This would require having every possible CPU model implemented in QEMU which is silly. Instead, we should have a passthrough CPU model for use with "-cpu host" which is essentially what Alexey's patch turns -cpu POWER7 into. Regards, Anthony Liguori > > He makes a very valid point. The ability to specify a specific revision > of the processor is pointless for pretty much any use case we have in > mind at the moment, and is even more pointless as long as we emulate > them all exactly the same way. > > Besides, we can probably still organize the table from "more precise" to > "less precise" entries and match that way if you *really* want to have > specific entries for obscure revision of the chip. > > Ben. > >> With HV KVM we can not trap PVR, yes. With PR KVM we do trap PVR and >> we emulate it. With TCG we do trap PVR and we emulate it. >> >> > May be (may be) ppc_cpu_class_by_name() needs to be fixed to try to >> find >> > the PPC CPU class with the biggest mask to choose (for example) >> > 004a0201/ffffffff rather than more common 004a0000/ffff0000 (if >> 004a0201 is >> > added to the list separately from the common definition for some >> reason). >> >> I think the class split as Linux has it should work just fine, no? >> >> >> Alex
On 15.08.2013, at 15:12, Anthony Liguori wrote: > Benjamin Herrenschmidt <benh@kernel.crashing.org> writes: > >> On Thu, 2013-08-15 at 08:03 +0200, Alexander Graf wrote: >> >>>>> How does the user select that he wants a v2.3 p7 cpu with this >>> patch? >>>> >>>> Why would he want that? The behaviour would not change because of >>> the >>>> version - all definitions use the same POWERPC_FAMILY(POWER7) and >>> PVR is >>>> not virtualized anyway. >>> >>> Quite frankly I don't know what to say here. Are you trying to play >>> dumb or are you just one of those totally sloppy people who don't care >>> about anything outside of their own scope of work? >> >> Can you stop the bloody personal attacks on Alexey ? It's becoming >> tiresome. > > > First, everyone needs to tone it down a couple levels. This isn't LKML > and Jonathan Corbet doesn't read qemu-devel so pithy remarks aren't > going to get you a quote of the week. > > Everyone is talking past each other and no one is addressing the real > problem. There are two distinct issues here: > > 1) We have two ABIs that cannot be changed unless there's a very good > reason to. Alexey's original patch breaks both. The guest ABI > cannot change given a fixed command line. > > IOW, the exposed PVR value for -cpu POWER7 cannot change across > versions of QEMU or when running on different hardware. This breaks > live migration and save/resume. > > We also cannot break the command line interface. If the last version > of QEMU supported -cpu POWER7_v2.1, then we must continue to support > that. > > If there's a good reason to break either of these, that's fine but > that justification needs be up front in the patch commit message. > > 2) The only "-cpu" that makes sense is "-cpu host" for KVM on HV (or > whatever ya'll call it). POWER does not have the ability to > virtualize the hardware PVR value. There is a virtual PVR in the > device tree but that's orthogonal to what we think of as the VCPU (it > essentially means IIUC that the cpu is compatible with that PVR). > > We should explicitly disallow any -cpu value when KVM on HV is > enabled other than host. With Paulus' latest patches we can support HV and PR dynamically on the same host kernel, so there's no way to figure out whether we are in an HV or PR environment. The semantic we probably want there is that -cpu host (and anything that looks the same from KVM's point of view, basically guest_pvr == host_pvr) and PAPR enabled triggers HV KVM. If we ever support compat mode with HV KVM, that PVR will also enable HV KVM. What we need in addition is a query interface so that QEMU can find out whether we are in HV or PR mode. That way we can add a machine force option that allows us to bail out when we're falling back to PR mode in case that's not what the user wanted. > The implementation of "-cpu host" is also goofy on PPC. -cpu host > does a match on existing CPU models meaning that we have to define a > CPU model for any possible CPU we run on. This would require having > every possible CPU model implemented in QEMU which is silly. > Instead, we should have a passthrough CPU model for use with "-cpu > host" which is essentially what Alexey's patch turns -cpu POWER7 > into. The world isn't that easy unfortunately. We basically need one "passthrough CPU model" for each CPU class, since different CPUs can have different features and MMU implementations which QEMU does care about. That's basically what the conclusion of the discussion was too. That we need to be able to instantiate the CPU family class (which currently is abstract) which then serves as entry point for -cpu host (and -cpu <pvr> hopefully). Alex
Am 15.08.2013 15:12, schrieb Anthony Liguori: > Everyone is talking past each other and no one is addressing the real > problem. There are two distinct issues here: > > 1) We have two ABIs that cannot be changed unless there's a very good > reason to. Alexey's original patch breaks both. The guest ABI > cannot change given a fixed command line. > > IOW, the exposed PVR value for -cpu POWER7 cannot change across > versions of QEMU or when running on different hardware. This breaks > live migration and save/resume. > > We also cannot break the command line interface. If the last version > of QEMU supported -cpu POWER7_v2.1, then we must continue to support > that. 1a) How should -cpu 0xDEADBEEF or -cpu DEADBEEF behave. I expect it to error out as before rather than applying the same fuzz/mask that -cpu host might. That would let us implement our own fuzz logic in kvm.c, operating on a GSList of ObjectClasses to handle multiple matches. Regards, Andreas > > If there's a good reason to break either of these, that's fine but > that justification needs be up front in the patch commit message. > > 2) The only "-cpu" that makes sense is "-cpu host" for KVM on HV (or > whatever ya'll call it). POWER does not have the ability to > virtualize the hardware PVR value. There is a virtual PVR in the > device tree but that's orthogonal to what we think of as the VCPU (it > essentially means IIUC that the cpu is compatible with that PVR). > > We should explicitly disallow any -cpu value when KVM on HV is > enabled other than host. > > The implementation of "-cpu host" is also goofy on PPC. -cpu host > does a match on existing CPU models meaning that we have to define a > CPU model for any possible CPU we run on. This would require having > every possible CPU model implemented in QEMU which is silly. > Instead, we should have a passthrough CPU model for use with "-cpu > host" which is essentially what Alexey's patch turns -cpu POWER7 > into.
On 15.08.2013, at 17:11, Andreas Färber wrote: > Am 15.08.2013 15:12, schrieb Anthony Liguori: >> Everyone is talking past each other and no one is addressing the real >> problem. There are two distinct issues here: >> >> 1) We have two ABIs that cannot be changed unless there's a very good >> reason to. Alexey's original patch breaks both. The guest ABI >> cannot change given a fixed command line. >> >> IOW, the exposed PVR value for -cpu POWER7 cannot change across >> versions of QEMU or when running on different hardware. This breaks >> live migration and save/resume. >> >> We also cannot break the command line interface. If the last version >> of QEMU supported -cpu POWER7_v2.1, then we must continue to support >> that. > > 1a) How should -cpu 0xDEADBEEF or -cpu DEADBEEF behave. > > I expect it to error out as before > rather than applying the same fuzz/mask that -cpu host might. I actually think it'd make sense to apply the same fuzz/mask, don't you think? Alex
Am 15.08.2013 17:30, schrieb Alexander Graf: > > On 15.08.2013, at 17:11, Andreas Färber wrote: > >> Am 15.08.2013 15:12, schrieb Anthony Liguori: >>> Everyone is talking past each other and no one is addressing the real >>> problem. There are two distinct issues here: >>> >>> 1) We have two ABIs that cannot be changed unless there's a very good >>> reason to. Alexey's original patch breaks both. The guest ABI >>> cannot change given a fixed command line. >>> >>> IOW, the exposed PVR value for -cpu POWER7 cannot change across >>> versions of QEMU or when running on different hardware. This breaks >>> live migration and save/resume. >>> >>> We also cannot break the command line interface. If the last version >>> of QEMU supported -cpu POWER7_v2.1, then we must continue to support >>> that. >> >> 1a) How should -cpu 0xDEADBEEF or -cpu DEADBEEF behave. >> >> I expect it to error out as before >> rather than applying the same fuzz/mask that -cpu host might. > > I actually think it'd make sense to apply the same fuzz/mask, don't you think? I think "-cpu host" has the semantics of give-me-what-the-host-has. But -cpu 0xDEADBEEF is asking for PVR DEADBEEF and having it silently return a guest-visible DEADBEBE is going to be undesired. We could of course report our closest match on stderr for the user to decide. Andreas
On 15.08.2013, at 17:48, Andreas Färber wrote: > Am 15.08.2013 17:30, schrieb Alexander Graf: >> >> On 15.08.2013, at 17:11, Andreas Färber wrote: >> >>> Am 15.08.2013 15:12, schrieb Anthony Liguori: >>>> Everyone is talking past each other and no one is addressing the real >>>> problem. There are two distinct issues here: >>>> >>>> 1) We have two ABIs that cannot be changed unless there's a very good >>>> reason to. Alexey's original patch breaks both. The guest ABI >>>> cannot change given a fixed command line. >>>> >>>> IOW, the exposed PVR value for -cpu POWER7 cannot change across >>>> versions of QEMU or when running on different hardware. This breaks >>>> live migration and save/resume. >>>> >>>> We also cannot break the command line interface. If the last version >>>> of QEMU supported -cpu POWER7_v2.1, then we must continue to support >>>> that. >>> >>> 1a) How should -cpu 0xDEADBEEF or -cpu DEADBEEF behave. >>> >>> I expect it to error out as before >>> rather than applying the same fuzz/mask that -cpu host might. >> >> I actually think it'd make sense to apply the same fuzz/mask, don't you think? > > I think "-cpu host" has the semantics of give-me-what-the-host-has. But > -cpu 0xDEADBEEF is asking for PVR DEADBEEF and having it silently return > a guest-visible DEADBEBE is going to be undesired. -cpu host on 0xDEADBEEF should give us a 0xDEADBEEF cpu. -cpu 0xDEADBEEF should give us a 0xDEADBEEF cpu :). Alex
Andreas Färber <afaerber@suse.de> writes: > Am 15.08.2013 15:12, schrieb Anthony Liguori: >> Everyone is talking past each other and no one is addressing the real >> problem. There are two distinct issues here: >> >> 1) We have two ABIs that cannot be changed unless there's a very good >> reason to. Alexey's original patch breaks both. The guest ABI >> cannot change given a fixed command line. >> >> IOW, the exposed PVR value for -cpu POWER7 cannot change across >> versions of QEMU or when running on different hardware. This breaks >> live migration and save/resume. >> >> We also cannot break the command line interface. If the last version >> of QEMU supported -cpu POWER7_v2.1, then we must continue to support >> that. > > 1a) How should -cpu 0xDEADBEEF or -cpu DEADBEEF behave. > > I expect it to error out as before Correct although that can't be guaranteed. Maybe there is a 'DEADBEEF' cpu model in the future. This is the architecture of the ripvanwinkle and eieio instructions after all :-) > rather than applying the same fuzz/mask that -cpu host might. Fuzzing CPU models sounds like an awful idea to me. Regards, Anthony Liguori > That would let us implement our own fuzz logic in kvm.c, > operating on a GSList of ObjectClasses to handle multiple matches. > > Regards, > Andreas
Am 15.08.2013 17:58, schrieb Alexander Graf: > > On 15.08.2013, at 17:48, Andreas Färber wrote: > >> Am 15.08.2013 17:30, schrieb Alexander Graf: >>> >>> On 15.08.2013, at 17:11, Andreas Färber wrote: >>> >>>> Am 15.08.2013 15:12, schrieb Anthony Liguori: >>>>> Everyone is talking past each other and no one is addressing the real >>>>> problem. There are two distinct issues here: >>>>> >>>>> 1) We have two ABIs that cannot be changed unless there's a very good >>>>> reason to. Alexey's original patch breaks both. The guest ABI >>>>> cannot change given a fixed command line. >>>>> >>>>> IOW, the exposed PVR value for -cpu POWER7 cannot change across >>>>> versions of QEMU or when running on different hardware. This breaks >>>>> live migration and save/resume. >>>>> >>>>> We also cannot break the command line interface. If the last version >>>>> of QEMU supported -cpu POWER7_v2.1, then we must continue to support >>>>> that. >>>> >>>> 1a) How should -cpu 0xDEADBEEF or -cpu DEADBEEF behave. >>>> >>>> I expect it to error out as before >>>> rather than applying the same fuzz/mask that -cpu host might. >>> >>> I actually think it'd make sense to apply the same fuzz/mask, don't you think? >> >> I think "-cpu host" has the semantics of give-me-what-the-host-has. But >> -cpu 0xDEADBEEF is asking for PVR DEADBEEF and having it silently return >> a guest-visible DEADBEBE is going to be undesired. > > -cpu host on 0xDEADBEEF should give us a 0xDEADBEEF cpu. -cpu 0xDEADBEEF should give us a 0xDEADBEEF cpu :). Then we mustn't tweak translate_init.c:cpu_class_by_pvr() to return deviating results! Which is what the change to ppc_cpu_compare_class_pvr() is essentially resulting in if I am not completely off track. And therefore my calling to handle this at a higher level (KVM init), where the user's intentions are clear, rather than to blur our internal API. Otherwise the _by_pvr() function would need to create a new class or modify an existing one when the function can't know what the function call was actually intended for. Andreas
On 15.08.2013, at 18:22, Andreas Färber wrote: > Am 15.08.2013 17:58, schrieb Alexander Graf: >> >> On 15.08.2013, at 17:48, Andreas Färber wrote: >> >>> Am 15.08.2013 17:30, schrieb Alexander Graf: >>>> >>>> On 15.08.2013, at 17:11, Andreas Färber wrote: >>>> >>>>> Am 15.08.2013 15:12, schrieb Anthony Liguori: >>>>>> Everyone is talking past each other and no one is addressing the real >>>>>> problem. There are two distinct issues here: >>>>>> >>>>>> 1) We have two ABIs that cannot be changed unless there's a very good >>>>>> reason to. Alexey's original patch breaks both. The guest ABI >>>>>> cannot change given a fixed command line. >>>>>> >>>>>> IOW, the exposed PVR value for -cpu POWER7 cannot change across >>>>>> versions of QEMU or when running on different hardware. This breaks >>>>>> live migration and save/resume. >>>>>> >>>>>> We also cannot break the command line interface. If the last version >>>>>> of QEMU supported -cpu POWER7_v2.1, then we must continue to support >>>>>> that. >>>>> >>>>> 1a) How should -cpu 0xDEADBEEF or -cpu DEADBEEF behave. >>>>> >>>>> I expect it to error out as before >>>>> rather than applying the same fuzz/mask that -cpu host might. >>>> >>>> I actually think it'd make sense to apply the same fuzz/mask, don't you think? >>> >>> I think "-cpu host" has the semantics of give-me-what-the-host-has. But >>> -cpu 0xDEADBEEF is asking for PVR DEADBEEF and having it silently return >>> a guest-visible DEADBEBE is going to be undesired. >> >> -cpu host on 0xDEADBEEF should give us a 0xDEADBEEF cpu. -cpu 0xDEADBEEF should give us a 0xDEADBEEF cpu :). > > Then we mustn't tweak translate_init.c:cpu_class_by_pvr() to return > deviating results! Which is what the change to > ppc_cpu_compare_class_pvr() is essentially resulting in if I am not > completely off track. And therefore my calling to handle this at a Did anyone ever say the patch is correct? > higher level (KVM init), where the user's intentions are clear, rather > than to blur our internal API. Otherwise the _by_pvr() function would Yes. > need to create a new class or modify an existing one when the function > can't know what the function call was actually intended for. Yes :). Alex
diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c index 8dea560..c40cf04 100644 --- a/target-ppc/cpu-models.c +++ b/target-ppc/cpu-models.c @@ -35,7 +35,7 @@ /* PowerPC CPU definitions */ #define POWERPC_DEF_PREFIX(pvr, svr, type) \ glue(glue(glue(glue(pvr, _), svr), _), type) -#define POWERPC_DEF_SVR(_name, _desc, _pvr, _svr, _type) \ +#define POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, _pvr_mask, _svr, _type) \ static void \ glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_class_init) \ (ObjectClass *oc, void *data) \ @@ -44,6 +44,7 @@ PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc); \ \ pcc->pvr = _pvr; \ + pcc->pvr_mask = _pvr_mask; \ pcc->svr = _svr; \ dc->desc = _desc; \ } \ @@ -66,9 +67,16 @@ type_init( \ glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_register_types)) +#define POWERPC_DEF_SVR(_name, _desc, _pvr, _svr, _type) \ + POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, CPU_POWERPC_DEFAULT_MASK, \ + _svr, _type) + #define POWERPC_DEF(_name, _pvr, _type, _desc) \ POWERPC_DEF_SVR(_name, _desc, _pvr, POWERPC_SVR_NONE, _type) +#define POWERPC_DEF_MASK(_name, _pvr, _pvr_mask, _type, _desc) \ + POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, _pvr_mask, POWERPC_SVR_NONE, _type) + /* Embedded PowerPC */ /* PowerPC 401 family */ POWERPC_DEF("401", CPU_POWERPC_401, 401, @@ -1133,16 +1141,12 @@ POWERPC_DEF("POWER6A", CPU_POWERPC_POWER6A, POWER6, "POWER6A") #endif - POWERPC_DEF("POWER7_v2.0", CPU_POWERPC_POWER7_v20, POWER7, - "POWER7 v2.0") - POWERPC_DEF("POWER7_v2.1", CPU_POWERPC_POWER7_v21, POWER7, - "POWER7 v2.1") - POWERPC_DEF("POWER7_v2.3", CPU_POWERPC_POWER7_v23, POWER7, - "POWER7 v2.3") - POWERPC_DEF("POWER7+_v2.1", CPU_POWERPC_POWER7P_v21, POWER7, - "POWER7+ v2.1") - POWERPC_DEF("POWER8_v1.0", CPU_POWERPC_POWER8_v10, POWER8, - "POWER8 v1.0") + POWERPC_DEF_MASK("POWER7", CPU_POWERPC_POWER7, CPU_POWERPC_POWER7_MASK, + POWER7, "POWER7") + POWERPC_DEF_MASK("POWER7+", CPU_POWERPC_POWER7P, CPU_POWERPC_POWER7P_MASK, + POWER7, "POWER7") + POWERPC_DEF_MASK("POWER8", CPU_POWERPC_POWER8, CPU_POWERPC_POWER8_MASK, + POWER8, "POWER8") POWERPC_DEF("970", CPU_POWERPC_970, 970, "PowerPC 970") POWERPC_DEF("970fx_v1.0", CPU_POWERPC_970FX_v10, 970FX, diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h index d9145d1..b322063 100644 --- a/target-ppc/cpu-models.h +++ b/target-ppc/cpu-models.h @@ -39,6 +39,7 @@ extern PowerPCCPUAlias ppc_cpu_aliases[]; /*****************************************************************************/ /* PVR definitions for most known PowerPC */ enum { + CPU_POWERPC_DEFAULT_MASK = 0xFFFFFFFF, /* PowerPC 401 family */ /* Generic PowerPC 401 */ #define CPU_POWERPC_401 CPU_POWERPC_401G2 @@ -552,11 +553,12 @@ enum { CPU_POWERPC_POWER6 = 0x003E0000, CPU_POWERPC_POWER6_5 = 0x0F000001, /* POWER6 in POWER5 mode */ CPU_POWERPC_POWER6A = 0x0F000002, - CPU_POWERPC_POWER7_v20 = 0x003F0200, - CPU_POWERPC_POWER7_v21 = 0x003F0201, - CPU_POWERPC_POWER7_v23 = 0x003F0203, - CPU_POWERPC_POWER7P_v21 = 0x004A0201, - CPU_POWERPC_POWER8_v10 = 0x004B0100, + CPU_POWERPC_POWER7 = 0x003F0000, + CPU_POWERPC_POWER7_MASK = 0xFFFF0000, + CPU_POWERPC_POWER7P = 0x004A0000, + CPU_POWERPC_POWER7P_MASK = 0xFFFF0000, + CPU_POWERPC_POWER8 = 0x004B0000, + CPU_POWERPC_POWER8_MASK = 0xFFFF0000, CPU_POWERPC_970 = 0x00390202, CPU_POWERPC_970FX_v10 = 0x00391100, CPU_POWERPC_970FX_v20 = 0x003C0200, diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h index f3c710a..0ae8b09 100644 --- a/target-ppc/cpu-qom.h +++ b/target-ppc/cpu-qom.h @@ -54,6 +54,7 @@ typedef struct PowerPCCPUClass { void (*parent_reset)(CPUState *cpu); uint32_t pvr; + uint32_t pvr_mask; uint32_t svr; uint64_t insns_flags; uint64_t insns_flags2; diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c index 13b290c..01dffa6 100644 --- a/target-ppc/translate_init.c +++ b/target-ppc/translate_init.c @@ -8161,7 +8161,7 @@ static gint ppc_cpu_compare_class_pvr(gconstpointer a, gconstpointer b) } #endif - return pcc->pvr == pvr ? 0 : -1; + return pcc->pvr == (pvr & pcc->pvr_mask) ? 0 : -1; } PowerPCCPUClass *ppc_cpu_class_by_pvr(uint32_t pvr)
IBM POWERPC processors encode PVR as a CPU family in higher 16 bits and a CPU version in lower 16 bits. Since there is no significant change in behavior between versions, there is no point to add every single CPU version in QEMU's CPU list. Also, new CPU versions of already supported CPU won't break the existing code. This adds a PVR mask support. POWER7, POWER7+ and POWER8 CPUs definitions converted to use masks. Cc: Andreas Färber <afaerber@suse.de> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- The patch does basically what the kernel does in arch/powerpc/kernel/cputable.c. --- target-ppc/cpu-models.c | 26 +++++++++++++++----------- target-ppc/cpu-models.h | 12 +++++++----- target-ppc/cpu-qom.h | 1 + target-ppc/translate_init.c | 2 +- 4 files changed, 24 insertions(+), 17 deletions(-)