* [Qemu-devel] [RFC PATCH] powerpc: add PVR mask support
@ 2013-08-15 3:35 Alexey Kardashevskiy
2013-08-15 5:21 ` Alexander Graf
0 siblings, 1 reply; 50+ messages in thread
From: Alexey Kardashevskiy @ 2013-08-15 3:35 UTC (permalink / raw)
To: qemu-devel
Cc: Alexey Kardashevskiy, Alexander Graf, qemu-ppc, Paul Mackerras,
Andreas Färber
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(-)
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
^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH] powerpc: add PVR mask support
2013-08-15 3:35 [Qemu-devel] [RFC PATCH] powerpc: add PVR mask support Alexey Kardashevskiy
@ 2013-08-15 5:21 ` Alexander Graf
2013-08-15 5:44 ` Alexey Kardashevskiy
` (2 more replies)
0 siblings, 3 replies; 50+ messages in thread
From: Alexander Graf @ 2013-08-15 5:21 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: qemu-devel, qemu-ppc, Paul Mackerras, Andreas Färber
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
>
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH] powerpc: add PVR mask support
2013-08-15 5:21 ` Alexander Graf
@ 2013-08-15 5:44 ` Alexey Kardashevskiy
2013-08-15 6:03 ` Alexander Graf
2013-08-15 5:54 ` Benjamin Herrenschmidt
2013-08-15 6:00 ` [Qemu-devel] [Qemu-ppc] " Alexander Graf
2 siblings, 1 reply; 50+ messages in thread
From: Alexey Kardashevskiy @ 2013-08-15 5:44 UTC (permalink / raw)
To: Alexander Graf; +Cc: qemu-ppc, Paul Mackerras, qemu-devel, Andreas Färber
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
>>
--
Alexey
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH] powerpc: add PVR mask support
2013-08-15 5:21 ` Alexander Graf
2013-08-15 5:44 ` Alexey Kardashevskiy
@ 2013-08-15 5:54 ` Benjamin Herrenschmidt
2013-08-15 6:10 ` Alexander Graf
2013-08-15 6:00 ` [Qemu-devel] [Qemu-ppc] " Alexander Graf
2 siblings, 1 reply; 50+ messages in thread
From: Benjamin Herrenschmidt @ 2013-08-15 5:54 UTC (permalink / raw)
To: Alexander Graf
Cc: Alexey Kardashevskiy, qemu-ppc, Paul Mackerras, qemu-devel,
Andreas Färber
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
> >
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] powerpc: add PVR mask support
2013-08-15 5:21 ` Alexander Graf
2013-08-15 5:44 ` Alexey Kardashevskiy
2013-08-15 5:54 ` Benjamin Herrenschmidt
@ 2013-08-15 6:00 ` Alexander Graf
2013-08-15 6:43 ` [Qemu-devel] [_R_F_C_ PATCH v2] " Alexey Kardashevskiy
2 siblings, 1 reply; 50+ messages in thread
From: Alexander Graf @ 2013-08-15 6:00 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Paul Mackerras, qemu-ppc, qemu-devel, Andreas Färber
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH] powerpc: add PVR mask support
2013-08-15 5:44 ` Alexey Kardashevskiy
@ 2013-08-15 6:03 ` Alexander Graf
2013-08-15 6:30 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 50+ messages in thread
From: Alexander Graf @ 2013-08-15 6:03 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: qemu-ppc, Paul Mackerras, qemu-devel, Andreas Färber
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH] powerpc: add PVR mask support
2013-08-15 5:54 ` Benjamin Herrenschmidt
@ 2013-08-15 6:10 ` Alexander Graf
2013-08-15 6:28 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 50+ messages in thread
From: Alexander Graf @ 2013-08-15 6:10 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Alexey Kardashevskiy, qemu-ppc, Paul Mackerras, qemu-devel,
Andreas Färber
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH] powerpc: add PVR mask support
2013-08-15 6:10 ` Alexander Graf
@ 2013-08-15 6:28 ` Benjamin Herrenschmidt
2013-08-15 6:37 ` Alexander Graf
0 siblings, 1 reply; 50+ messages in thread
From: Benjamin Herrenschmidt @ 2013-08-15 6:28 UTC (permalink / raw)
To: Alexander Graf
Cc: Alexey Kardashevskiy, qemu-ppc, Paul Mackerras, qemu-devel,
Andreas Färber
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.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH] powerpc: add PVR mask support
2013-08-15 6:03 ` Alexander Graf
@ 2013-08-15 6:30 ` Benjamin Herrenschmidt
2013-08-15 6:39 ` Alexander Graf
2013-08-15 13:12 ` Anthony Liguori
0 siblings, 2 replies; 50+ messages in thread
From: Benjamin Herrenschmidt @ 2013-08-15 6:30 UTC (permalink / raw)
To: Alexander Graf
Cc: Alexey Kardashevskiy, qemu-ppc, Paul Mackerras, qemu-devel,
Andreas Färber
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH] powerpc: add PVR mask support
2013-08-15 6:28 ` Benjamin Herrenschmidt
@ 2013-08-15 6:37 ` Alexander Graf
0 siblings, 0 replies; 50+ messages in thread
From: Alexander Graf @ 2013-08-15 6:37 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Alexey Kardashevskiy, qemu-ppc, Paul Mackerras, qemu-devel,
Andreas Färber
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH] powerpc: add PVR mask support
2013-08-15 6:30 ` Benjamin Herrenschmidt
@ 2013-08-15 6:39 ` Alexander Graf
2013-08-15 13:12 ` Anthony Liguori
1 sibling, 0 replies; 50+ messages in thread
From: Alexander Graf @ 2013-08-15 6:39 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Alexey Kardashevskiy, qemu-ppc, Paul Mackerras, qemu-devel,
Andreas Färber
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* [Qemu-devel] [_R_F_C_ PATCH v2] powerpc: add PVR mask support
2013-08-15 6:00 ` [Qemu-devel] [Qemu-ppc] " Alexander Graf
@ 2013-08-15 6:43 ` Alexey Kardashevskiy
2013-08-15 6:57 ` Alexander Graf
0 siblings, 1 reply; 50+ messages in thread
From: Alexey Kardashevskiy @ 2013-08-15 6:43 UTC (permalink / raw)
To: qemu-devel
Cc: Alexey Kardashevskiy, Alexander Graf, qemu-ppc, Paul Mackerras,
Andreas Färber
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 which means that aliases are replaced with
another layer in POWERPC CPU class hierarchy. The patch adds intermediate
POWER7, POWER7+ and POWER8 CPU classes and makes use of those in
specific versioned POWERPC CPUs.
Cc: Andreas Färber <afaerber@suse.de>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v2:
* aliases are replaced with another level in class hierarchy
---
target-ppc/cpu-models.c | 46 ++++++++++++++++++++++++++++++---------------
target-ppc/cpu-models.h | 7 +++++++
target-ppc/cpu-qom.h | 1 +
target-ppc/translate_init.c | 2 +-
4 files changed, 40 insertions(+), 16 deletions(-)
diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c
index 8dea560..dfa6673 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, _parent) \
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; \
} \
@@ -51,7 +52,7 @@
static const TypeInfo \
glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_type_info) = { \
.name = _name "-" TYPE_POWERPC_CPU, \
- .parent = stringify(_type) "-family-" TYPE_POWERPC_CPU, \
+ .parent = _parent, \
.class_init = \
glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_class_init), \
}; \
@@ -66,9 +67,21 @@
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, stringify(_type) "-family-" TYPE_POWERPC_CPU)
+
#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, \
+ stringify(_type) "-family-" TYPE_POWERPC_CPU)
+
+#define POWERPC_DEF_2ND(_name, _pvr, _type, _desc, _parent) \
+ POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, CPU_POWERPC_DEFAULT_MASK, \
+ POWERPC_SVR_NONE, _type, _parent)
+
/* Embedded PowerPC */
/* PowerPC 401 family */
POWERPC_DEF("401", CPU_POWERPC_401, 401,
@@ -1133,16 +1146,22 @@
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_2ND("POWER7_v2.0", CPU_POWERPC_POWER7_v20, POWER7,
+ "POWER7 v2.0", "POWER7-" TYPE_POWERPC_CPU)
+ POWERPC_DEF_2ND("POWER7_v2.1", CPU_POWERPC_POWER7_v21, POWER7,
+ "POWER7 v2.1", "POWER7-" TYPE_POWERPC_CPU)
+ POWERPC_DEF_2ND("POWER7_v2.3", CPU_POWERPC_POWER7_v23, POWER7,
+ "POWER7 v2.3", "POWER7-" TYPE_POWERPC_CPU)
+ POWERPC_DEF_2ND("POWER7+_v2.1", CPU_POWERPC_POWER7P_v21, POWER7,
+ "POWER7+ v2.1", "POWER7+-" TYPE_POWERPC_CPU)
+ POWERPC_DEF_2ND("POWER8_v1.0", CPU_POWERPC_POWER8_v10, POWER8,
+ "POWER8 v1.0", "POWER8-" TYPE_POWERPC_CPU)
+ 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,
@@ -1389,9 +1408,6 @@ PowerPCCPUAlias ppc_cpu_aliases[] = {
{ "POWER3+", "631" },
{ "POWER5gr", "POWER5" },
{ "POWER5gs", "POWER5+" },
- { "POWER7", "POWER7_v2.3" },
- { "POWER7+", "POWER7+_v2.1" },
- { "POWER8", "POWER8_v1.0" },
{ "970fx", "970fx_v3.1" },
{ "970mp", "970mp_v1.1" },
{ "Apache", "RS64" },
diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
index d9145d1..2233053 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
@@ -557,6 +558,12 @@ enum {
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
^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [_R_F_C_ PATCH v2] powerpc: add PVR mask support
2013-08-15 6:43 ` [Qemu-devel] [_R_F_C_ PATCH v2] " Alexey Kardashevskiy
@ 2013-08-15 6:57 ` Alexander Graf
2013-08-15 7:45 ` [Qemu-devel] [RFC PATCH v3] " Alexey Kardashevskiy
0 siblings, 1 reply; 50+ messages in thread
From: Alexander Graf @ 2013-08-15 6:57 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: qemu-ppc, Paul Mackerras, qemu-devel, Andreas Färber
On 15.08.2013, at 08:43, Alexey Kardashevskiy wrote:
> 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 which means that aliases are replaced with
> another layer in POWERPC CPU class hierarchy. The patch adds intermediate
> POWER7, POWER7+ and POWER8 CPU classes and makes use of those in
> specific versioned POWERPC CPUs.
A lot better :)
>
> Cc: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>
> ---
> Changes:
> v2:
> * aliases are replaced with another level in class hierarchy
> ---
> target-ppc/cpu-models.c | 46 ++++++++++++++++++++++++++++++---------------
> target-ppc/cpu-models.h | 7 +++++++
> target-ppc/cpu-qom.h | 1 +
> target-ppc/translate_init.c | 2 +-
> 4 files changed, 40 insertions(+), 16 deletions(-)
>
> diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c
> index 8dea560..dfa6673 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, _parent) \
> 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; \
> } \
> @@ -51,7 +52,7 @@
> static const TypeInfo \
> glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_type_info) = { \
> .name = _name "-" TYPE_POWERPC_CPU, \
> - .parent = stringify(_type) "-family-" TYPE_POWERPC_CPU, \
> + .parent = _parent, \
> .class_init = \
> glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_class_init), \
> }; \
> @@ -66,9 +67,21 @@
> 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, stringify(_type) "-family-" TYPE_POWERPC_CPU)
> +
> #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, \
> + stringify(_type) "-family-" TYPE_POWERPC_CPU)
> +
> +#define POWERPC_DEF_2ND(_name, _pvr, _type, _desc, _parent) \
> + POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, CPU_POWERPC_DEFAULT_MASK, \
> + POWERPC_SVR_NONE, _type, _parent)
> +
> /* Embedded PowerPC */
> /* PowerPC 401 family */
> POWERPC_DEF("401", CPU_POWERPC_401, 401,
> @@ -1133,16 +1146,22 @@
> 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_2ND("POWER7_v2.0", CPU_POWERPC_POWER7_v20, POWER7,
> + "POWER7 v2.0", "POWER7-" TYPE_POWERPC_CPU)
> + POWERPC_DEF_2ND("POWER7_v2.1", CPU_POWERPC_POWER7_v21, POWER7,
> + "POWER7 v2.1", "POWER7-" TYPE_POWERPC_CPU)
> + POWERPC_DEF_2ND("POWER7_v2.3", CPU_POWERPC_POWER7_v23, POWER7,
> + "POWER7 v2.3", "POWER7-" TYPE_POWERPC_CPU)
> + POWERPC_DEF_2ND("POWER7+_v2.1", CPU_POWERPC_POWER7P_v21, POWER7,
> + "POWER7+ v2.1", "POWER7+-" TYPE_POWERPC_CPU)
> + POWERPC_DEF_2ND("POWER8_v1.0", CPU_POWERPC_POWER8_v10, POWER8,
> + "POWER8 v1.0", "POWER8-" TYPE_POWERPC_CPU)
> + 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,
> @@ -1389,9 +1408,6 @@ PowerPCCPUAlias ppc_cpu_aliases[] = {
> { "POWER3+", "631" },
> { "POWER5gr", "POWER5" },
> { "POWER5gs", "POWER5+" },
> - { "POWER7", "POWER7_v2.3" },
> - { "POWER7+", "POWER7+_v2.1" },
> - { "POWER8", "POWER8_v1.0" },
So here we had explicit versions for -cpu POWERx
> { "970fx", "970fx_v3.1" },
> { "970mp", "970mp_v1.1" },
> { "Apache", "RS64" },
> diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
> index d9145d1..2233053 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
> @@ -557,6 +558,12 @@ enum {
> CPU_POWERPC_POWER7_v23 = 0x003F0203,
> CPU_POWERPC_POWER7P_v21 = 0x004A0201,
> CPU_POWERPC_POWER8_v10 = 0x004B0100,
> + CPU_POWERPC_POWER7 = 0x003F0000,
But because we define this as the default PVR value we lose those. So when the user just says -cpu POWER7 he will get a POWER7 v0.0 chip rather than a POWER7 v2.3 chip, which guests might not like.
I'm not sure what the best way to solve this is. Maybe we shouldn't pass in CPU_POWERPC_POWER7 but instead CPU_POWERPC_POWER7_V23 and have the mask work its magic?
Alex
> + 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
>
^ permalink raw reply [flat|nested] 50+ messages in thread
* [Qemu-devel] [RFC PATCH v3] powerpc: add PVR mask support
2013-08-15 6:57 ` Alexander Graf
@ 2013-08-15 7:45 ` Alexey Kardashevskiy
2013-08-15 7:55 ` Alexander Graf
0 siblings, 1 reply; 50+ messages in thread
From: Alexey Kardashevskiy @ 2013-08-15 7:45 UTC (permalink / raw)
To: qemu-devel
Cc: Anthony Liguori, Alexey Kardashevskiy, Alexander Graf, qemu-ppc,
Paul Mackerras, Andreas Färber
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 which means that aliases are replaced with
another layer in POWERPC CPU class hierarchy. The patch adds intermediate
POWER7, POWER7+ and POWER8 CPU classes and makes use of those in
specific versioned POWERPC CPUs.
Cc: Andreas Färber <afaerber@suse.de>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v3:
* renamed macros to describe the functionality better
* added default PVR value for the powerpc cpu family (what alias used to do)
v2:
* aliases are replaced with another level in class hierarchy
---
target-ppc/cpu-models.c | 54 ++++++++++++++++++++++++++++++++-------------
target-ppc/cpu-models.h | 7 ++++++
target-ppc/cpu-qom.h | 2 ++
target-ppc/translate_init.c | 4 ++--
4 files changed, 50 insertions(+), 17 deletions(-)
diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c
index 8dea560..e48004b 100644
--- a/target-ppc/cpu-models.c
+++ b/target-ppc/cpu-models.c
@@ -35,7 +35,8 @@
/* 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, _pvr_default, \
+ _svr, _type, _parent) \
static void \
glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_class_init) \
(ObjectClass *oc, void *data) \
@@ -44,6 +45,8 @@
PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc); \
\
pcc->pvr = _pvr; \
+ pcc->pvr_default = _pvr_default; \
+ pcc->pvr_mask = _pvr_mask; \
pcc->svr = _svr; \
dc->desc = _desc; \
} \
@@ -51,7 +54,7 @@
static const TypeInfo \
glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_type_info) = { \
.name = _name "-" TYPE_POWERPC_CPU, \
- .parent = stringify(_type) "-family-" TYPE_POWERPC_CPU, \
+ .parent = _parent, \
.class_init = \
glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_class_init), \
}; \
@@ -66,9 +69,24 @@
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, 0, \
+ _svr, _type, \
+ stringify(_type) "-family-" TYPE_POWERPC_CPU)
+
#define POWERPC_DEF(_name, _pvr, _type, _desc) \
POWERPC_DEF_SVR(_name, _desc, _pvr, POWERPC_SVR_NONE, _type)
+#define POWERPC_DEF_FAMILY(_name, _pvr, _pvr_mask, _pvr_default, \
+ _type, _desc) \
+ POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, _pvr_mask, _pvr_default, \
+ POWERPC_SVR_NONE, _type, \
+ stringify(_type) "-family-" TYPE_POWERPC_CPU)
+
+#define POWERPC_DEF_FAMILY_MEMBER(_name, _pvr, _type, _desc, _parent) \
+ POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, CPU_POWERPC_DEFAULT_MASK, 0, \
+ POWERPC_SVR_NONE, _type, _parent)
+
/* Embedded PowerPC */
/* PowerPC 401 family */
POWERPC_DEF("401", CPU_POWERPC_401, 401,
@@ -1133,16 +1151,25 @@
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_FAMILY("POWER7", CPU_POWERPC_POWER7, CPU_POWERPC_POWER7_MASK,
+ CPU_POWERPC_POWER7_v23,
+ POWER7, "POWER7")
+ POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.0", CPU_POWERPC_POWER7_v20, POWER7,
+ "POWER7 v2.0", "POWER7-" TYPE_POWERPC_CPU)
+ POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.1", CPU_POWERPC_POWER7_v21, POWER7,
+ "POWER7 v2.1", "POWER7-" TYPE_POWERPC_CPU)
+ POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.3", CPU_POWERPC_POWER7_v23, POWER7,
+ "POWER7 v2.3", "POWER7-" TYPE_POWERPC_CPU)
+ POWERPC_DEF_FAMILY("POWER7+", CPU_POWERPC_POWER7P, CPU_POWERPC_POWER7P_MASK,
+ CPU_POWERPC_POWER7P_v21,
+ POWER7, "POWER7")
+ POWERPC_DEF_FAMILY_MEMBER("POWER7+_v2.1", CPU_POWERPC_POWER7P_v21, POWER7,
+ "POWER7+ v2.1", "POWER7+-" TYPE_POWERPC_CPU)
+ POWERPC_DEF_FAMILY("POWER8", CPU_POWERPC_POWER8, CPU_POWERPC_POWER8_MASK,
+ CPU_POWERPC_POWER8_v10,
+ POWER8, "POWER8")
+ POWERPC_DEF_FAMILY_MEMBER("POWER8_v1.0", CPU_POWERPC_POWER8_v10, POWER8,
+ "POWER8 v1.0", "POWER8-" TYPE_POWERPC_CPU)
POWERPC_DEF("970", CPU_POWERPC_970, 970,
"PowerPC 970")
POWERPC_DEF("970fx_v1.0", CPU_POWERPC_970FX_v10, 970FX,
@@ -1389,9 +1416,6 @@ PowerPCCPUAlias ppc_cpu_aliases[] = {
{ "POWER3+", "631" },
{ "POWER5gr", "POWER5" },
{ "POWER5gs", "POWER5+" },
- { "POWER7", "POWER7_v2.3" },
- { "POWER7+", "POWER7+_v2.1" },
- { "POWER8", "POWER8_v1.0" },
{ "970fx", "970fx_v3.1" },
{ "970mp", "970mp_v1.1" },
{ "Apache", "RS64" },
diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
index d9145d1..2233053 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
@@ -557,6 +558,12 @@ enum {
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..a1a712c 100644
--- a/target-ppc/cpu-qom.h
+++ b/target-ppc/cpu-qom.h
@@ -54,6 +54,8 @@ typedef struct PowerPCCPUClass {
void (*parent_reset)(CPUState *cpu);
uint32_t pvr;
+ uint32_t pvr_default;
+ 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..e73792d 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -7309,7 +7309,7 @@ static void init_ppc_proc(PowerPCCPU *cpu)
#endif
SPR_NOACCESS,
&spr_read_generic, SPR_NOACCESS,
- pcc->pvr);
+ pcc->pvr_default ? pcc->pvr_default : pcc->pvr);
/* Register SVR if it's defined to anything else than POWERPC_SVR_NONE */
if (pcc->svr != POWERPC_SVR_NONE) {
if (pcc->svr & POWERPC_SVR_E500) {
@@ -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
^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3] powerpc: add PVR mask support
2013-08-15 7:45 ` [Qemu-devel] [RFC PATCH v3] " Alexey Kardashevskiy
@ 2013-08-15 7:55 ` Alexander Graf
2013-08-15 8:06 ` Alexey Kardashevskiy
0 siblings, 1 reply; 50+ messages in thread
From: Alexander Graf @ 2013-08-15 7:55 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Anthony Liguori, qemu-devel, qemu-ppc, Paul Mackerras,
Andreas Färber
On 15.08.2013, at 09:45, Alexey Kardashevskiy wrote:
> 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 which means that aliases are replaced with
> another layer in POWERPC CPU class hierarchy. The patch adds intermediate
> POWER7, POWER7+ and POWER8 CPU classes and makes use of those in
> specific versioned POWERPC CPUs.
>
> Cc: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>
> ---
> Changes:
> v3:
> * renamed macros to describe the functionality better
> * added default PVR value for the powerpc cpu family (what alias used to do)
>
> v2:
> * aliases are replaced with another level in class hierarchy
> ---
> target-ppc/cpu-models.c | 54 ++++++++++++++++++++++++++++++++-------------
> target-ppc/cpu-models.h | 7 ++++++
> target-ppc/cpu-qom.h | 2 ++
> target-ppc/translate_init.c | 4 ++--
> 4 files changed, 50 insertions(+), 17 deletions(-)
>
> diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c
> index 8dea560..e48004b 100644
> --- a/target-ppc/cpu-models.c
> +++ b/target-ppc/cpu-models.c
> @@ -35,7 +35,8 @@
> /* 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, _pvr_default, \
> + _svr, _type, _parent) \
> static void \
> glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_class_init) \
> (ObjectClass *oc, void *data) \
> @@ -44,6 +45,8 @@
> PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc); \
> \
> pcc->pvr = _pvr; \
> + pcc->pvr_default = _pvr_default; \
> + pcc->pvr_mask = _pvr_mask; \
> pcc->svr = _svr; \
> dc->desc = _desc; \
> } \
> @@ -51,7 +54,7 @@
> static const TypeInfo \
> glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_type_info) = { \
> .name = _name "-" TYPE_POWERPC_CPU, \
> - .parent = stringify(_type) "-family-" TYPE_POWERPC_CPU, \
> + .parent = _parent, \
> .class_init = \
> glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_class_init), \
> }; \
> @@ -66,9 +69,24 @@
> 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, 0, \
> + _svr, _type, \
> + stringify(_type) "-family-" TYPE_POWERPC_CPU)
> +
> #define POWERPC_DEF(_name, _pvr, _type, _desc) \
> POWERPC_DEF_SVR(_name, _desc, _pvr, POWERPC_SVR_NONE, _type)
>
> +#define POWERPC_DEF_FAMILY(_name, _pvr, _pvr_mask, _pvr_default, \
> + _type, _desc) \
> + POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, _pvr_mask, _pvr_default, \
> + POWERPC_SVR_NONE, _type, \
> + stringify(_type) "-family-" TYPE_POWERPC_CPU)
> +
> +#define POWERPC_DEF_FAMILY_MEMBER(_name, _pvr, _type, _desc, _parent) \
> + POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, CPU_POWERPC_DEFAULT_MASK, 0, \
> + POWERPC_SVR_NONE, _type, _parent)
> +
> /* Embedded PowerPC */
> /* PowerPC 401 family */
> POWERPC_DEF("401", CPU_POWERPC_401, 401,
> @@ -1133,16 +1151,25 @@
> 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_FAMILY("POWER7", CPU_POWERPC_POWER7, CPU_POWERPC_POWER7_MASK,
> + CPU_POWERPC_POWER7_v23,
> + POWER7, "POWER7")
> + POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.0", CPU_POWERPC_POWER7_v20, POWER7,
> + "POWER7 v2.0", "POWER7-" TYPE_POWERPC_CPU)
> + POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.1", CPU_POWERPC_POWER7_v21, POWER7,
> + "POWER7 v2.1", "POWER7-" TYPE_POWERPC_CPU)
> + POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.3", CPU_POWERPC_POWER7_v23, POWER7,
> + "POWER7 v2.3", "POWER7-" TYPE_POWERPC_CPU)
> + POWERPC_DEF_FAMILY("POWER7+", CPU_POWERPC_POWER7P, CPU_POWERPC_POWER7P_MASK,
> + CPU_POWERPC_POWER7P_v21,
> + POWER7, "POWER7")
> + POWERPC_DEF_FAMILY_MEMBER("POWER7+_v2.1", CPU_POWERPC_POWER7P_v21, POWER7,
> + "POWER7+ v2.1", "POWER7+-" TYPE_POWERPC_CPU)
> + POWERPC_DEF_FAMILY("POWER8", CPU_POWERPC_POWER8, CPU_POWERPC_POWER8_MASK,
> + CPU_POWERPC_POWER8_v10,
> + POWER8, "POWER8")
> + POWERPC_DEF_FAMILY_MEMBER("POWER8_v1.0", CPU_POWERPC_POWER8_v10, POWER8,
> + "POWER8 v1.0", "POWER8-" TYPE_POWERPC_CPU)
> POWERPC_DEF("970", CPU_POWERPC_970, 970,
> "PowerPC 970")
> POWERPC_DEF("970fx_v1.0", CPU_POWERPC_970FX_v10, 970FX,
> @@ -1389,9 +1416,6 @@ PowerPCCPUAlias ppc_cpu_aliases[] = {
> { "POWER3+", "631" },
> { "POWER5gr", "POWER5" },
> { "POWER5gs", "POWER5+" },
> - { "POWER7", "POWER7_v2.3" },
> - { "POWER7+", "POWER7+_v2.1" },
> - { "POWER8", "POWER8_v1.0" },
> { "970fx", "970fx_v3.1" },
> { "970mp", "970mp_v1.1" },
> { "Apache", "RS64" },
> diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
> index d9145d1..2233053 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
> @@ -557,6 +558,12 @@ enum {
> 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..a1a712c 100644
> --- a/target-ppc/cpu-qom.h
> +++ b/target-ppc/cpu-qom.h
> @@ -54,6 +54,8 @@ typedef struct PowerPCCPUClass {
> void (*parent_reset)(CPUState *cpu);
>
> uint32_t pvr;
> + uint32_t pvr_default;
> + 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..e73792d 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -7309,7 +7309,7 @@ static void init_ppc_proc(PowerPCCPU *cpu)
> #endif
> SPR_NOACCESS,
> &spr_read_generic, SPR_NOACCESS,
> - pcc->pvr);
> + pcc->pvr_default ? pcc->pvr_default : pcc->pvr);
This means that -cpu host on a POWER7_v20 system will still return POWER7_v23 and thus expose a different CPU inside the guest than expected with PR KVM, no?
Is there any case where major=0 minor=0 is valid? If not, we could add a check in the class constructor and then default to sane values when we hit this case. We'll have to add logic there later anyways if we want to allow the user to manually specify major and minor numbers.
Alex
> /* Register SVR if it's defined to anything else than POWERPC_SVR_NONE */
> if (pcc->svr != POWERPC_SVR_NONE) {
> if (pcc->svr & POWERPC_SVR_E500) {
> @@ -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
>
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3] powerpc: add PVR mask support
2013-08-15 7:55 ` Alexander Graf
@ 2013-08-15 8:06 ` Alexey Kardashevskiy
2013-08-15 8:45 ` Alexander Graf
0 siblings, 1 reply; 50+ messages in thread
From: Alexey Kardashevskiy @ 2013-08-15 8:06 UTC (permalink / raw)
To: Alexander Graf
Cc: Anthony Liguori, qemu-devel, qemu-ppc, Paul Mackerras,
Andreas Färber
On 08/15/2013 05:55 PM, Alexander Graf wrote:
>
> On 15.08.2013, at 09:45, Alexey Kardashevskiy wrote:
>
>> 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 which means that aliases are replaced with
>> another layer in POWERPC CPU class hierarchy. The patch adds intermediate
>> POWER7, POWER7+ and POWER8 CPU classes and makes use of those in
>> specific versioned POWERPC CPUs.
>>
>> Cc: Andreas Färber <afaerber@suse.de>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>
>> ---
>> Changes:
>> v3:
>> * renamed macros to describe the functionality better
>> * added default PVR value for the powerpc cpu family (what alias used to do)
>>
>> v2:
>> * aliases are replaced with another level in class hierarchy
>> ---
>> target-ppc/cpu-models.c | 54 ++++++++++++++++++++++++++++++++-------------
>> target-ppc/cpu-models.h | 7 ++++++
>> target-ppc/cpu-qom.h | 2 ++
>> target-ppc/translate_init.c | 4 ++--
>> 4 files changed, 50 insertions(+), 17 deletions(-)
>>
>> diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c
>> index 8dea560..e48004b 100644
>> --- a/target-ppc/cpu-models.c
>> +++ b/target-ppc/cpu-models.c
>> @@ -35,7 +35,8 @@
>> /* 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, _pvr_default, \
>> + _svr, _type, _parent) \
>> static void \
>> glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_class_init) \
>> (ObjectClass *oc, void *data) \
>> @@ -44,6 +45,8 @@
>> PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc); \
>> \
>> pcc->pvr = _pvr; \
>> + pcc->pvr_default = _pvr_default; \
>> + pcc->pvr_mask = _pvr_mask; \
>> pcc->svr = _svr; \
>> dc->desc = _desc; \
>> } \
>> @@ -51,7 +54,7 @@
>> static const TypeInfo \
>> glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_type_info) = { \
>> .name = _name "-" TYPE_POWERPC_CPU, \
>> - .parent = stringify(_type) "-family-" TYPE_POWERPC_CPU, \
>> + .parent = _parent, \
>> .class_init = \
>> glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_class_init), \
>> }; \
>> @@ -66,9 +69,24 @@
>> 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, 0, \
>> + _svr, _type, \
>> + stringify(_type) "-family-" TYPE_POWERPC_CPU)
>> +
>> #define POWERPC_DEF(_name, _pvr, _type, _desc) \
>> POWERPC_DEF_SVR(_name, _desc, _pvr, POWERPC_SVR_NONE, _type)
>>
>> +#define POWERPC_DEF_FAMILY(_name, _pvr, _pvr_mask, _pvr_default, \
>> + _type, _desc) \
>> + POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, _pvr_mask, _pvr_default, \
>> + POWERPC_SVR_NONE, _type, \
>> + stringify(_type) "-family-" TYPE_POWERPC_CPU)
>> +
>> +#define POWERPC_DEF_FAMILY_MEMBER(_name, _pvr, _type, _desc, _parent) \
>> + POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, CPU_POWERPC_DEFAULT_MASK, 0, \
>> + POWERPC_SVR_NONE, _type, _parent)
>> +
>> /* Embedded PowerPC */
>> /* PowerPC 401 family */
>> POWERPC_DEF("401", CPU_POWERPC_401, 401,
>> @@ -1133,16 +1151,25 @@
>> 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_FAMILY("POWER7", CPU_POWERPC_POWER7, CPU_POWERPC_POWER7_MASK,
>> + CPU_POWERPC_POWER7_v23,
>> + POWER7, "POWER7")
>> + POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.0", CPU_POWERPC_POWER7_v20, POWER7,
>> + "POWER7 v2.0", "POWER7-" TYPE_POWERPC_CPU)
>> + POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.1", CPU_POWERPC_POWER7_v21, POWER7,
>> + "POWER7 v2.1", "POWER7-" TYPE_POWERPC_CPU)
>> + POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.3", CPU_POWERPC_POWER7_v23, POWER7,
>> + "POWER7 v2.3", "POWER7-" TYPE_POWERPC_CPU)
>> + POWERPC_DEF_FAMILY("POWER7+", CPU_POWERPC_POWER7P, CPU_POWERPC_POWER7P_MASK,
>> + CPU_POWERPC_POWER7P_v21,
>> + POWER7, "POWER7")
>> + POWERPC_DEF_FAMILY_MEMBER("POWER7+_v2.1", CPU_POWERPC_POWER7P_v21, POWER7,
>> + "POWER7+ v2.1", "POWER7+-" TYPE_POWERPC_CPU)
>> + POWERPC_DEF_FAMILY("POWER8", CPU_POWERPC_POWER8, CPU_POWERPC_POWER8_MASK,
>> + CPU_POWERPC_POWER8_v10,
>> + POWER8, "POWER8")
>> + POWERPC_DEF_FAMILY_MEMBER("POWER8_v1.0", CPU_POWERPC_POWER8_v10, POWER8,
>> + "POWER8 v1.0", "POWER8-" TYPE_POWERPC_CPU)
>> POWERPC_DEF("970", CPU_POWERPC_970, 970,
>> "PowerPC 970")
>> POWERPC_DEF("970fx_v1.0", CPU_POWERPC_970FX_v10, 970FX,
>> @@ -1389,9 +1416,6 @@ PowerPCCPUAlias ppc_cpu_aliases[] = {
>> { "POWER3+", "631" },
>> { "POWER5gr", "POWER5" },
>> { "POWER5gs", "POWER5+" },
>> - { "POWER7", "POWER7_v2.3" },
>> - { "POWER7+", "POWER7+_v2.1" },
>> - { "POWER8", "POWER8_v1.0" },
>> { "970fx", "970fx_v3.1" },
>> { "970mp", "970mp_v1.1" },
>> { "Apache", "RS64" },
>> diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
>> index d9145d1..2233053 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
>> @@ -557,6 +558,12 @@ enum {
>> 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..a1a712c 100644
>> --- a/target-ppc/cpu-qom.h
>> +++ b/target-ppc/cpu-qom.h
>> @@ -54,6 +54,8 @@ typedef struct PowerPCCPUClass {
>> void (*parent_reset)(CPUState *cpu);
>>
>> uint32_t pvr;
>> + uint32_t pvr_default;
>> + 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..e73792d 100644
>> --- a/target-ppc/translate_init.c
>> +++ b/target-ppc/translate_init.c
>> @@ -7309,7 +7309,7 @@ static void init_ppc_proc(PowerPCCPU *cpu)
>> #endif
>> SPR_NOACCESS,
>> &spr_read_generic, SPR_NOACCESS,
>> - pcc->pvr);
>> + pcc->pvr_default ? pcc->pvr_default : pcc->pvr);
>
> This means that -cpu host on a POWER7_v20 system will still return
> POWER7_v23 and thus expose a different CPU inside the guest than
> expected with PR KVM, no?
./qemu-system-ppc64 \
-cpu \
POWER7_v2.0 \
-S \
-m "1024" \
-machine "pseries,usb=off" \
-nographic \
-vga "none" \
-enable-kvm \
-kernel "host.vmlinux" \
-initrd "1.cpio"
QEMU 1.5.91 monitor - type 'help' for more information
(qemu) info registers
[...]
FPSCR 0000000000000000
SRR0 0000000000000000 SRR1 0000000000000000 PVR 00000000003f0200
[...]
(qemu)
PVR is 003f0200 which is correct.
>
> Is there any case where major=0 minor=0 is valid? If not, we could add a
> check in the class constructor and then default to sane values when we
> hit this case. We'll have to add logic there later anyways if we want to
> allow the user to manually specify major and minor numbers.
As I was told and then noticed in the kernel (and sorry for my ignorance if
I am wrong), the lower 16 bits are major/minor numbers only for IBM POWERPC
CPUs and other CPUs may use different number of bits for this purpose. So I
would not parse those numbers and carry them as major/minor version in QEMU
and rather use some fixed sane value for a whole family for the cases when
the user does not really care about the exact chip version.
>
>
> Alex
>
>> /* Register SVR if it's defined to anything else than POWERPC_SVR_NONE */
>> if (pcc->svr != POWERPC_SVR_NONE) {
>> if (pcc->svr & POWERPC_SVR_E500) {
>> @@ -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
>>
>
--
Alexey
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3] powerpc: add PVR mask support
2013-08-15 8:06 ` Alexey Kardashevskiy
@ 2013-08-15 8:45 ` Alexander Graf
2013-08-15 10:52 ` Andreas Färber
2013-08-15 14:43 ` [Qemu-devel] [RFC PATCH v3] " Alexey Kardashevskiy
0 siblings, 2 replies; 50+ messages in thread
From: Alexander Graf @ 2013-08-15 8:45 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Anthony Liguori, qemu-devel, qemu-ppc, Paul Mackerras,
Andreas Färber
On 15.08.2013, at 10:06, Alexey Kardashevskiy wrote:
> On 08/15/2013 05:55 PM, Alexander Graf wrote:
>>
>> On 15.08.2013, at 09:45, Alexey Kardashevskiy wrote:
>>
>>> 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 which means that aliases are replaced with
>>> another layer in POWERPC CPU class hierarchy. The patch adds intermediate
>>> POWER7, POWER7+ and POWER8 CPU classes and makes use of those in
>>> specific versioned POWERPC CPUs.
>>>
>>> Cc: Andreas Färber <afaerber@suse.de>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>
>>> ---
>>> Changes:
>>> v3:
>>> * renamed macros to describe the functionality better
>>> * added default PVR value for the powerpc cpu family (what alias used to do)
>>>
>>> v2:
>>> * aliases are replaced with another level in class hierarchy
>>> ---
>>> target-ppc/cpu-models.c | 54 ++++++++++++++++++++++++++++++++-------------
>>> target-ppc/cpu-models.h | 7 ++++++
>>> target-ppc/cpu-qom.h | 2 ++
>>> target-ppc/translate_init.c | 4 ++--
>>> 4 files changed, 50 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c
>>> index 8dea560..e48004b 100644
>>> --- a/target-ppc/cpu-models.c
>>> +++ b/target-ppc/cpu-models.c
>>> @@ -35,7 +35,8 @@
>>> /* 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, _pvr_default, \
>>> + _svr, _type, _parent) \
>>> static void \
>>> glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_class_init) \
>>> (ObjectClass *oc, void *data) \
>>> @@ -44,6 +45,8 @@
>>> PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc); \
>>> \
>>> pcc->pvr = _pvr; \
>>> + pcc->pvr_default = _pvr_default; \
>>> + pcc->pvr_mask = _pvr_mask; \
>>> pcc->svr = _svr; \
>>> dc->desc = _desc; \
>>> } \
>>> @@ -51,7 +54,7 @@
>>> static const TypeInfo \
>>> glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_type_info) = { \
>>> .name = _name "-" TYPE_POWERPC_CPU, \
>>> - .parent = stringify(_type) "-family-" TYPE_POWERPC_CPU, \
>>> + .parent = _parent, \
>>> .class_init = \
>>> glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_class_init), \
>>> }; \
>>> @@ -66,9 +69,24 @@
>>> 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, 0, \
>>> + _svr, _type, \
>>> + stringify(_type) "-family-" TYPE_POWERPC_CPU)
>>> +
>>> #define POWERPC_DEF(_name, _pvr, _type, _desc) \
>>> POWERPC_DEF_SVR(_name, _desc, _pvr, POWERPC_SVR_NONE, _type)
>>>
>>> +#define POWERPC_DEF_FAMILY(_name, _pvr, _pvr_mask, _pvr_default, \
>>> + _type, _desc) \
>>> + POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, _pvr_mask, _pvr_default, \
>>> + POWERPC_SVR_NONE, _type, \
>>> + stringify(_type) "-family-" TYPE_POWERPC_CPU)
>>> +
>>> +#define POWERPC_DEF_FAMILY_MEMBER(_name, _pvr, _type, _desc, _parent) \
>>> + POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, CPU_POWERPC_DEFAULT_MASK, 0, \
>>> + POWERPC_SVR_NONE, _type, _parent)
>>> +
>>> /* Embedded PowerPC */
>>> /* PowerPC 401 family */
>>> POWERPC_DEF("401", CPU_POWERPC_401, 401,
>>> @@ -1133,16 +1151,25 @@
>>> 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_FAMILY("POWER7", CPU_POWERPC_POWER7, CPU_POWERPC_POWER7_MASK,
>>> + CPU_POWERPC_POWER7_v23,
>>> + POWER7, "POWER7")
>>> + POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.0", CPU_POWERPC_POWER7_v20, POWER7,
>>> + "POWER7 v2.0", "POWER7-" TYPE_POWERPC_CPU)
>>> + POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.1", CPU_POWERPC_POWER7_v21, POWER7,
>>> + "POWER7 v2.1", "POWER7-" TYPE_POWERPC_CPU)
>>> + POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.3", CPU_POWERPC_POWER7_v23, POWER7,
>>> + "POWER7 v2.3", "POWER7-" TYPE_POWERPC_CPU)
>>> + POWERPC_DEF_FAMILY("POWER7+", CPU_POWERPC_POWER7P, CPU_POWERPC_POWER7P_MASK,
>>> + CPU_POWERPC_POWER7P_v21,
>>> + POWER7, "POWER7")
>>> + POWERPC_DEF_FAMILY_MEMBER("POWER7+_v2.1", CPU_POWERPC_POWER7P_v21, POWER7,
>>> + "POWER7+ v2.1", "POWER7+-" TYPE_POWERPC_CPU)
>>> + POWERPC_DEF_FAMILY("POWER8", CPU_POWERPC_POWER8, CPU_POWERPC_POWER8_MASK,
>>> + CPU_POWERPC_POWER8_v10,
>>> + POWER8, "POWER8")
>>> + POWERPC_DEF_FAMILY_MEMBER("POWER8_v1.0", CPU_POWERPC_POWER8_v10, POWER8,
>>> + "POWER8 v1.0", "POWER8-" TYPE_POWERPC_CPU)
>>> POWERPC_DEF("970", CPU_POWERPC_970, 970,
>>> "PowerPC 970")
>>> POWERPC_DEF("970fx_v1.0", CPU_POWERPC_970FX_v10, 970FX,
>>> @@ -1389,9 +1416,6 @@ PowerPCCPUAlias ppc_cpu_aliases[] = {
>>> { "POWER3+", "631" },
>>> { "POWER5gr", "POWER5" },
>>> { "POWER5gs", "POWER5+" },
>>> - { "POWER7", "POWER7_v2.3" },
>>> - { "POWER7+", "POWER7+_v2.1" },
>>> - { "POWER8", "POWER8_v1.0" },
>>> { "970fx", "970fx_v3.1" },
>>> { "970mp", "970mp_v1.1" },
>>> { "Apache", "RS64" },
>>> diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
>>> index d9145d1..2233053 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
>>> @@ -557,6 +558,12 @@ enum {
>>> 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..a1a712c 100644
>>> --- a/target-ppc/cpu-qom.h
>>> +++ b/target-ppc/cpu-qom.h
>>> @@ -54,6 +54,8 @@ typedef struct PowerPCCPUClass {
>>> void (*parent_reset)(CPUState *cpu);
>>>
>>> uint32_t pvr;
>>> + uint32_t pvr_default;
>>> + 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..e73792d 100644
>>> --- a/target-ppc/translate_init.c
>>> +++ b/target-ppc/translate_init.c
>>> @@ -7309,7 +7309,7 @@ static void init_ppc_proc(PowerPCCPU *cpu)
>>> #endif
>>> SPR_NOACCESS,
>>> &spr_read_generic, SPR_NOACCESS,
>>> - pcc->pvr);
>>> + pcc->pvr_default ? pcc->pvr_default : pcc->pvr);
>>
>
>
>> This means that -cpu host on a POWER7_v20 system will still return
>> POWER7_v23 and thus expose a different CPU inside the guest than
>> expected with PR KVM, no?
>
>
> ./qemu-system-ppc64 \
> -cpu \
> POWER7_v2.0 \
This is not -cpu host, is it? :)
> -S \
> -m "1024" \
> -machine "pseries,usb=off" \
> -nographic \
> -vga "none" \
> -enable-kvm \
> -kernel "host.vmlinux" \
> -initrd "1.cpio"
>
> QEMU 1.5.91 monitor - type 'help' for more information
> (qemu) info registers
> [...]
> FPSCR 0000000000000000
> SRR0 0000000000000000 SRR1 0000000000000000 PVR 00000000003f0200
> [...]
> (qemu)
>
> PVR is 003f0200 which is correct.
>
>
>>
>> Is there any case where major=0 minor=0 is valid? If not, we could add a
>> check in the class constructor and then default to sane values when we
>> hit this case. We'll have to add logic there later anyways if we want to
>> allow the user to manually specify major and minor numbers.
>
> As I was told and then noticed in the kernel (and sorry for my ignorance if
> I am wrong), the lower 16 bits are major/minor numbers only for IBM POWERPC
> CPUs and other CPUs may use different number of bits for this purpose. So I
> would not parse those numbers and carry them as major/minor version in QEMU
> and rather use some fixed sane value for a whole family for the cases when
> the user does not really care about the exact chip version.
Yes, I think it makes sense to keep the full PVR around when we want to be specific. What I'm referring to is class specific logic that can assemble major/minor numbers from the command line. So
-cpu POWER7,major=2,minor=0
would result in a PVR value that is identical to POWER7_v2.0. The assembly of this PVR value is class specific, because different classes of CPUs have different semantics for their major and minor numbers.
That way in the future we won't have to add any new version specific CPU types but instead the user can assemble those himself, making everyone's life a lot easier.
My point was that if we have that logic, we could at the same place just say "if my major/minor is 0, default to something reasonable".
But let's ask Andreas whether he has a better idea here :).
Alex
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3] powerpc: add PVR mask support
2013-08-15 8:45 ` Alexander Graf
@ 2013-08-15 10:52 ` Andreas Färber
2013-08-15 11:03 ` Alexander Graf
2013-08-15 14:43 ` [Qemu-devel] [RFC PATCH v3] " Alexey Kardashevskiy
1 sibling, 1 reply; 50+ messages in thread
From: Andreas Färber @ 2013-08-15 10:52 UTC (permalink / raw)
To: Alexander Graf
Cc: Anthony Liguori, Alexey Kardashevskiy, qemu-devel, qemu-ppc,
Paul Mackerras
Am 15.08.2013 10:45, schrieb Alexander Graf:
>
> On 15.08.2013, at 10:06, Alexey Kardashevskiy wrote:
>
>> On 08/15/2013 05:55 PM, Alexander Graf wrote:
>>>
>>> On 15.08.2013, at 09:45, Alexey Kardashevskiy wrote:
>>>
>>>> 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 which means that aliases are replaced with
>>>> another layer in POWERPC CPU class hierarchy. The patch adds intermediate
>>>> POWER7, POWER7+ and POWER8 CPU classes and makes use of those in
>>>> specific versioned POWERPC CPUs.
>>>>
>>>> Cc: Andreas Färber <afaerber@suse.de>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>
>>>> ---
>>>> Changes:
>>>> v3:
>>>> * renamed macros to describe the functionality better
>>>> * added default PVR value for the powerpc cpu family (what alias used to do)
>>>>
>>>> v2:
>>>> * aliases are replaced with another level in class hierarchy
>>>> ---
>>>> target-ppc/cpu-models.c | 54 ++++++++++++++++++++++++++++++++-------------
>>>> target-ppc/cpu-models.h | 7 ++++++
>>>> target-ppc/cpu-qom.h | 2 ++
>>>> target-ppc/translate_init.c | 4 ++--
>>>> 4 files changed, 50 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c
>>>> index 8dea560..e48004b 100644
>>>> --- a/target-ppc/cpu-models.c
>>>> +++ b/target-ppc/cpu-models.c
>>>> @@ -35,7 +35,8 @@
>>>> /* 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, _pvr_default, \
>>>> + _svr, _type, _parent) \
>>>> static void \
>>>> glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_class_init) \
>>>> (ObjectClass *oc, void *data) \
Quite obviously this heavily conflicts with my patchset for Prerna's
problem...
>>>> @@ -44,6 +45,8 @@
>>>> PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc); \
>>>> \
>>>> pcc->pvr = _pvr; \
>>>> + pcc->pvr_default = _pvr_default; \
>>>> + pcc->pvr_mask = _pvr_mask; \
>>>> pcc->svr = _svr; \
>>>> dc->desc = _desc; \
>>>> } \
>>>> @@ -51,7 +54,7 @@
>>>> static const TypeInfo \
>>>> glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_type_info) = { \
>>>> .name = _name "-" TYPE_POWERPC_CPU, \
>>>> - .parent = stringify(_type) "-family-" TYPE_POWERPC_CPU, \
>>>> + .parent = _parent, \
>>>> .class_init = \
>>>> glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_class_init), \
>>>> }; \
>>>> @@ -66,9 +69,24 @@
>>>> 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, 0, \
>>>> + _svr, _type, \
>>>> + stringify(_type) "-family-" TYPE_POWERPC_CPU)
>>>> +
>>>> #define POWERPC_DEF(_name, _pvr, _type, _desc) \
>>>> POWERPC_DEF_SVR(_name, _desc, _pvr, POWERPC_SVR_NONE, _type)
>>>>
>>>> +#define POWERPC_DEF_FAMILY(_name, _pvr, _pvr_mask, _pvr_default, \
>>>> + _type, _desc) \
>>>> + POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, _pvr_mask, _pvr_default, \
>>>> + POWERPC_SVR_NONE, _type, \
>>>> + stringify(_type) "-family-" TYPE_POWERPC_CPU)
>>>> +
>>>> +#define POWERPC_DEF_FAMILY_MEMBER(_name, _pvr, _type, _desc, _parent) \
>>>> + POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, CPU_POWERPC_DEFAULT_MASK, 0, \
>>>> + POWERPC_SVR_NONE, _type, _parent)
>>>> +
>>>> /* Embedded PowerPC */
>>>> /* PowerPC 401 family */
>>>> POWERPC_DEF("401", CPU_POWERPC_401, 401,
>>>> @@ -1133,16 +1151,25 @@
>>>> 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_FAMILY("POWER7", CPU_POWERPC_POWER7, CPU_POWERPC_POWER7_MASK,
>>>> + CPU_POWERPC_POWER7_v23,
>>>> + POWER7, "POWER7")
>>>> + POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.0", CPU_POWERPC_POWER7_v20, POWER7,
>>>> + "POWER7 v2.0", "POWER7-" TYPE_POWERPC_CPU)
No. Alex wanted these macros to abstract all the QOM business, so make
that "POWER7" if you need it here and assembly the type name inside the
macro. I'll have a closer look later. We already have the POWER7
argument so I find this redundant. If we need more families we can
simply add them, such as POWER7P.
>>>> + POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.1", CPU_POWERPC_POWER7_v21, POWER7,
>>>> + "POWER7 v2.1", "POWER7-" TYPE_POWERPC_CPU)
>>>> + POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.3", CPU_POWERPC_POWER7_v23, POWER7,
>>>> + "POWER7 v2.3", "POWER7-" TYPE_POWERPC_CPU)
>>>> + POWERPC_DEF_FAMILY("POWER7+", CPU_POWERPC_POWER7P, CPU_POWERPC_POWER7P_MASK,
>>>> + CPU_POWERPC_POWER7P_v21,
>>>> + POWER7, "POWER7")
>>>> + POWERPC_DEF_FAMILY_MEMBER("POWER7+_v2.1", CPU_POWERPC_POWER7P_v21, POWER7,
>>>> + "POWER7+ v2.1", "POWER7+-" TYPE_POWERPC_CPU)
>>>> + POWERPC_DEF_FAMILY("POWER8", CPU_POWERPC_POWER8, CPU_POWERPC_POWER8_MASK,
>>>> + CPU_POWERPC_POWER8_v10,
>>>> + POWER8, "POWER8")
>>>> + POWERPC_DEF_FAMILY_MEMBER("POWER8_v1.0", CPU_POWERPC_POWER8_v10, POWER8,
>>>> + "POWER8 v1.0", "POWER8-" TYPE_POWERPC_CPU)
>>>> POWERPC_DEF("970", CPU_POWERPC_970, 970,
>>>> "PowerPC 970")
>>>> POWERPC_DEF("970fx_v1.0", CPU_POWERPC_970FX_v10, 970FX,
>>>> @@ -1389,9 +1416,6 @@ PowerPCCPUAlias ppc_cpu_aliases[] = {
>>>> { "POWER3+", "631" },
>>>> { "POWER5gr", "POWER5" },
>>>> { "POWER5gs", "POWER5+" },
>>>> - { "POWER7", "POWER7_v2.3" },
>>>> - { "POWER7+", "POWER7+_v2.1" },
>>>> - { "POWER8", "POWER8_v1.0" },
>>>> { "970fx", "970fx_v3.1" },
>>>> { "970mp", "970mp_v1.1" },
>>>> { "Apache", "RS64" },
>>>> diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
>>>> index d9145d1..2233053 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
>>>> @@ -557,6 +558,12 @@ enum {
>>>> 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..a1a712c 100644
>>>> --- a/target-ppc/cpu-qom.h
>>>> +++ b/target-ppc/cpu-qom.h
>>>> @@ -54,6 +54,8 @@ typedef struct PowerPCCPUClass {
>>>> void (*parent_reset)(CPUState *cpu);
>>>>
>>>> uint32_t pvr;
>>>> + uint32_t pvr_default;
>>>> + 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..e73792d 100644
>>>> --- a/target-ppc/translate_init.c
>>>> +++ b/target-ppc/translate_init.c
>>>> @@ -7309,7 +7309,7 @@ static void init_ppc_proc(PowerPCCPU *cpu)
>>>> #endif
>>>> SPR_NOACCESS,
>>>> &spr_read_generic, SPR_NOACCESS,
>>>> - pcc->pvr);
>>>> + pcc->pvr_default ? pcc->pvr_default : pcc->pvr);
>>>
>>
>>
>>> This means that -cpu host on a POWER7_v20 system will still return
>>> POWER7_v23 and thus expose a different CPU inside the guest than
>>> expected with PR KVM, no?
>>
>>
>> ./qemu-system-ppc64 \
>> -cpu \
>> POWER7_v2.0 \
>
> This is not -cpu host, is it? :)
>
>> -S \
>> -m "1024" \
>> -machine "pseries,usb=off" \
>> -nographic \
>> -vga "none" \
>> -enable-kvm \
>> -kernel "host.vmlinux" \
>> -initrd "1.cpio"
>>
>> QEMU 1.5.91 monitor - type 'help' for more information
>> (qemu) info registers
>> [...]
>> FPSCR 0000000000000000
>> SRR0 0000000000000000 SRR1 0000000000000000 PVR 00000000003f0200
>> [...]
>> (qemu)
>>
>> PVR is 003f0200 which is correct.
>>
>>
>>>
>>> Is there any case where major=0 minor=0 is valid? If not, we could add a
>>> check in the class constructor and then default to sane values when we
>>> hit this case. We'll have to add logic there later anyways if we want to
>>> allow the user to manually specify major and minor numbers.
>>
>> As I was told and then noticed in the kernel (and sorry for my ignorance if
>> I am wrong), the lower 16 bits are major/minor numbers only for IBM POWERPC
>> CPUs and other CPUs may use different number of bits for this purpose. So I
>> would not parse those numbers and carry them as major/minor version in QEMU
>> and rather use some fixed sane value for a whole family for the cases when
>> the user does not really care about the exact chip version.
>
> Yes, I think it makes sense to keep the full PVR around when we want to be specific. What I'm referring to is class specific logic that can assemble major/minor numbers from the command line. So
>
> -cpu POWER7,major=2,minor=0
>
> would result in a PVR value that is identical to POWER7_v2.0. The assembly of this PVR value is class specific, because different classes of CPUs have different semantics for their major and minor numbers.
>
> That way in the future we won't have to add any new version specific CPU types but instead the user can assemble those himself, making everyone's life a lot easier.
>
> My point was that if we have that logic, we could at the same place just say "if my major/minor is 0, default to something reasonable".
>
> But let's ask Andreas whether he has a better idea here :).
If you read the previous discussion on the initial POWER7+ patch, I
believe I had proposed major-version / minor-version or so properties at
family level, to be able to use different implementations or none at all
where we don't see a scheme. However if we want to use that from -cpu as
in your example above, we would have to implement custom parsing code
for cpu_model, which I would rather avoid, given we want to replace it
with -device in the future.
But maybe I didn't fully catch the exact question. :)
The custom parenting strikes me as a wrong consequence of us not having
fully QOM'ified / cleaned up the family classes yet. We had discussed
two ways: Either have, e.g., POWER7+ inherit from POWER7 (which looks
like the only reason this is being done here) and/or have, e.g., POWER5+
copy and modify 970fx values via #defines.
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3] powerpc: add PVR mask support
2013-08-15 10:52 ` Andreas Färber
@ 2013-08-15 11:03 ` Alexander Graf
2013-08-15 11:48 ` Andreas Färber
0 siblings, 1 reply; 50+ messages in thread
From: Alexander Graf @ 2013-08-15 11:03 UTC (permalink / raw)
To: Andreas Färber
Cc: Anthony Liguori, Alexey Kardashevskiy, qemu-devel, qemu-ppc,
Paul Mackerras
On 15.08.2013, at 12:52, Andreas Färber wrote:
> Am 15.08.2013 10:45, schrieb Alexander Graf:
>>
>> On 15.08.2013, at 10:06, Alexey Kardashevskiy wrote:
>>
>>> On 08/15/2013 05:55 PM, Alexander Graf wrote:
>>>>
>>>> On 15.08.2013, at 09:45, Alexey Kardashevskiy wrote:
>>>>
>>>>> 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 which means that aliases are replaced with
>>>>> another layer in POWERPC CPU class hierarchy. The patch adds intermediate
>>>>> POWER7, POWER7+ and POWER8 CPU classes and makes use of those in
>>>>> specific versioned POWERPC CPUs.
>>>>>
>>>>> Cc: Andreas Färber <afaerber@suse.de>
>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>>
>>>>> ---
>>>>> Changes:
>>>>> v3:
>>>>> * renamed macros to describe the functionality better
>>>>> * added default PVR value for the powerpc cpu family (what alias used to do)
>>>>>
>>>>> v2:
>>>>> * aliases are replaced with another level in class hierarchy
>>>>> ---
>>>>> target-ppc/cpu-models.c | 54 ++++++++++++++++++++++++++++++++-------------
>>>>> target-ppc/cpu-models.h | 7 ++++++
>>>>> target-ppc/cpu-qom.h | 2 ++
>>>>> target-ppc/translate_init.c | 4 ++--
>>>>> 4 files changed, 50 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c
>>>>> index 8dea560..e48004b 100644
>>>>> --- a/target-ppc/cpu-models.c
>>>>> +++ b/target-ppc/cpu-models.c
>>>>> @@ -35,7 +35,8 @@
>>>>> /* 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, _pvr_default, \
>>>>> + _svr, _type, _parent) \
>>>>> static void \
>>>>> glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_class_init) \
>>>>> (ObjectClass *oc, void *data) \
>
> Quite obviously this heavily conflicts with my patchset for Prerna's
> problem...
>
>>>>> @@ -44,6 +45,8 @@
>>>>> PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc); \
>>>>> \
>>>>> pcc->pvr = _pvr; \
>>>>> + pcc->pvr_default = _pvr_default; \
>>>>> + pcc->pvr_mask = _pvr_mask; \
>>>>> pcc->svr = _svr; \
>>>>> dc->desc = _desc; \
>>>>> } \
>>>>> @@ -51,7 +54,7 @@
>>>>> static const TypeInfo \
>>>>> glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_type_info) = { \
>>>>> .name = _name "-" TYPE_POWERPC_CPU, \
>>>>> - .parent = stringify(_type) "-family-" TYPE_POWERPC_CPU, \
>>>>> + .parent = _parent, \
>>>>> .class_init = \
>>>>> glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_class_init), \
>>>>> }; \
>>>>> @@ -66,9 +69,24 @@
>>>>> 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, 0, \
>>>>> + _svr, _type, \
>>>>> + stringify(_type) "-family-" TYPE_POWERPC_CPU)
>>>>> +
>>>>> #define POWERPC_DEF(_name, _pvr, _type, _desc) \
>>>>> POWERPC_DEF_SVR(_name, _desc, _pvr, POWERPC_SVR_NONE, _type)
>>>>>
>>>>> +#define POWERPC_DEF_FAMILY(_name, _pvr, _pvr_mask, _pvr_default, \
>>>>> + _type, _desc) \
>>>>> + POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, _pvr_mask, _pvr_default, \
>>>>> + POWERPC_SVR_NONE, _type, \
>>>>> + stringify(_type) "-family-" TYPE_POWERPC_CPU)
>>>>> +
>>>>> +#define POWERPC_DEF_FAMILY_MEMBER(_name, _pvr, _type, _desc, _parent) \
>>>>> + POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, CPU_POWERPC_DEFAULT_MASK, 0, \
>>>>> + POWERPC_SVR_NONE, _type, _parent)
>>>>> +
>>>>> /* Embedded PowerPC */
>>>>> /* PowerPC 401 family */
>>>>> POWERPC_DEF("401", CPU_POWERPC_401, 401,
>>>>> @@ -1133,16 +1151,25 @@
>>>>> 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_FAMILY("POWER7", CPU_POWERPC_POWER7, CPU_POWERPC_POWER7_MASK,
>>>>> + CPU_POWERPC_POWER7_v23,
>>>>> + POWER7, "POWER7")
>>>>> + POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.0", CPU_POWERPC_POWER7_v20, POWER7,
>>>>> + "POWER7 v2.0", "POWER7-" TYPE_POWERPC_CPU)
>
> No. Alex wanted these macros to abstract all the QOM business, so make
> that "POWER7" if you need it here and assembly the type name inside the
> macro. I'll have a closer look later. We already have the POWER7
> argument so I find this redundant. If we need more families we can
> simply add them, such as POWER7P.
>
>>>>> + POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.1", CPU_POWERPC_POWER7_v21, POWER7,
>>>>> + "POWER7 v2.1", "POWER7-" TYPE_POWERPC_CPU)
>>>>> + POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.3", CPU_POWERPC_POWER7_v23, POWER7,
>>>>> + "POWER7 v2.3", "POWER7-" TYPE_POWERPC_CPU)
>>>>> + POWERPC_DEF_FAMILY("POWER7+", CPU_POWERPC_POWER7P, CPU_POWERPC_POWER7P_MASK,
>>>>> + CPU_POWERPC_POWER7P_v21,
>>>>> + POWER7, "POWER7")
>>>>> + POWERPC_DEF_FAMILY_MEMBER("POWER7+_v2.1", CPU_POWERPC_POWER7P_v21, POWER7,
>>>>> + "POWER7+ v2.1", "POWER7+-" TYPE_POWERPC_CPU)
>>>>> + POWERPC_DEF_FAMILY("POWER8", CPU_POWERPC_POWER8, CPU_POWERPC_POWER8_MASK,
>>>>> + CPU_POWERPC_POWER8_v10,
>>>>> + POWER8, "POWER8")
>>>>> + POWERPC_DEF_FAMILY_MEMBER("POWER8_v1.0", CPU_POWERPC_POWER8_v10, POWER8,
>>>>> + "POWER8 v1.0", "POWER8-" TYPE_POWERPC_CPU)
>>>>> POWERPC_DEF("970", CPU_POWERPC_970, 970,
>>>>> "PowerPC 970")
>>>>> POWERPC_DEF("970fx_v1.0", CPU_POWERPC_970FX_v10, 970FX,
>>>>> @@ -1389,9 +1416,6 @@ PowerPCCPUAlias ppc_cpu_aliases[] = {
>>>>> { "POWER3+", "631" },
>>>>> { "POWER5gr", "POWER5" },
>>>>> { "POWER5gs", "POWER5+" },
>>>>> - { "POWER7", "POWER7_v2.3" },
>>>>> - { "POWER7+", "POWER7+_v2.1" },
>>>>> - { "POWER8", "POWER8_v1.0" },
>>>>> { "970fx", "970fx_v3.1" },
>>>>> { "970mp", "970mp_v1.1" },
>>>>> { "Apache", "RS64" },
>>>>> diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
>>>>> index d9145d1..2233053 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
>>>>> @@ -557,6 +558,12 @@ enum {
>>>>> 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..a1a712c 100644
>>>>> --- a/target-ppc/cpu-qom.h
>>>>> +++ b/target-ppc/cpu-qom.h
>>>>> @@ -54,6 +54,8 @@ typedef struct PowerPCCPUClass {
>>>>> void (*parent_reset)(CPUState *cpu);
>>>>>
>>>>> uint32_t pvr;
>>>>> + uint32_t pvr_default;
>>>>> + 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..e73792d 100644
>>>>> --- a/target-ppc/translate_init.c
>>>>> +++ b/target-ppc/translate_init.c
>>>>> @@ -7309,7 +7309,7 @@ static void init_ppc_proc(PowerPCCPU *cpu)
>>>>> #endif
>>>>> SPR_NOACCESS,
>>>>> &spr_read_generic, SPR_NOACCESS,
>>>>> - pcc->pvr);
>>>>> + pcc->pvr_default ? pcc->pvr_default : pcc->pvr);
>>>>
>>>
>>>
>>>> This means that -cpu host on a POWER7_v20 system will still return
>>>> POWER7_v23 and thus expose a different CPU inside the guest than
>>>> expected with PR KVM, no?
>>>
>>>
>>> ./qemu-system-ppc64 \
>>> -cpu \
>>> POWER7_v2.0 \
>>
>> This is not -cpu host, is it? :)
>>
>>> -S \
>>> -m "1024" \
>>> -machine "pseries,usb=off" \
>>> -nographic \
>>> -vga "none" \
>>> -enable-kvm \
>>> -kernel "host.vmlinux" \
>>> -initrd "1.cpio"
>>>
>>> QEMU 1.5.91 monitor - type 'help' for more information
>>> (qemu) info registers
>>> [...]
>>> FPSCR 0000000000000000
>>> SRR0 0000000000000000 SRR1 0000000000000000 PVR 00000000003f0200
>>> [...]
>>> (qemu)
>>>
>>> PVR is 003f0200 which is correct.
>>>
>>>
>>>>
>>>> Is there any case where major=0 minor=0 is valid? If not, we could add a
>>>> check in the class constructor and then default to sane values when we
>>>> hit this case. We'll have to add logic there later anyways if we want to
>>>> allow the user to manually specify major and minor numbers.
>>>
>>> As I was told and then noticed in the kernel (and sorry for my ignorance if
>>> I am wrong), the lower 16 bits are major/minor numbers only for IBM POWERPC
>>> CPUs and other CPUs may use different number of bits for this purpose. So I
>>> would not parse those numbers and carry them as major/minor version in QEMU
>>> and rather use some fixed sane value for a whole family for the cases when
>>> the user does not really care about the exact chip version.
>>
>> Yes, I think it makes sense to keep the full PVR around when we want to be specific. What I'm referring to is class specific logic that can assemble major/minor numbers from the command line. So
>>
>> -cpu POWER7,major=2,minor=0
>>
>> would result in a PVR value that is identical to POWER7_v2.0. The assembly of this PVR value is class specific, because different classes of CPUs have different semantics for their major and minor numbers.
>>
>> That way in the future we won't have to add any new version specific CPU types but instead the user can assemble those himself, making everyone's life a lot easier.
>>
>> My point was that if we have that logic, we could at the same place just say "if my major/minor is 0, default to something reasonable".
>>
>> But let's ask Andreas whether he has a better idea here :).
>
> If you read the previous discussion on the initial POWER7+ patch, I
> believe I had proposed major-version / minor-version or so properties at
> family level, to be able to use different implementations or none at all
> where we don't see a scheme.
Sounds like a good idea.
> However if we want to use that from -cpu as
> in your example above, we would have to implement custom parsing code
> for cpu_model, which I would rather avoid, given we want to replace it
> with -device in the future.
Can't we make this generic QOM property parsing code?
-cpu POWER7,major-version=2,minor-version=0
would do
cpu = new POWER7(major-version = 2, minor_version = 0);
and then the POWER7 class can decide what to do with this additional information?
> But maybe I didn't fully catch the exact question. :)
>
> The custom parenting strikes me as a wrong consequence of us not having
> fully QOM'ified / cleaned up the family classes yet. We had discussed
> two ways: Either have, e.g., POWER7+ inherit from POWER7 (which looks
> like the only reason this is being done here) and/or have, e.g., POWER5+
> copy and modify 970fx values via #defines.
IIUC the family parenting is orthogonal to this. Here we're looking at having families as classes at all. Currently we don't - we only have explicit versioned implementations as classes. Whether we have
PowerPC
`- POWER7
`- POWER7+
`- POWER7+ v1.0
or
PowerPC
`- POWER7+
`- POWER7+ v1.0
is a different question I think.
Alex
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3] powerpc: add PVR mask support
2013-08-15 11:03 ` Alexander Graf
@ 2013-08-15 11:48 ` Andreas Färber
2013-08-15 11:59 ` Alexander Graf
2013-08-15 13:55 ` Alexey Kardashevskiy
0 siblings, 2 replies; 50+ messages in thread
From: Andreas Färber @ 2013-08-15 11:48 UTC (permalink / raw)
To: Alexander Graf
Cc: Anthony Liguori, Alexey Kardashevskiy, qemu-devel, qemu-ppc,
Paul Mackerras
Am 15.08.2013 13:03, schrieb Alexander Graf:
>
> On 15.08.2013, at 12:52, Andreas Färber wrote:
>
>> Am 15.08.2013 10:45, schrieb Alexander Graf:
>>>
>>> Yes, I think it makes sense to keep the full PVR around when we want to be specific. What I'm referring to is class specific logic that can assemble major/minor numbers from the command line. So
>>>
>>> -cpu POWER7,major=2,minor=0
>>>
>>> would result in a PVR value that is identical to POWER7_v2.0. The assembly of this PVR value is class specific, because different classes of CPUs have different semantics for their major and minor numbers.
>>>
>>> That way in the future we won't have to add any new version specific CPU types but instead the user can assemble those himself, making everyone's life a lot easier.
>>>
>>> My point was that if we have that logic, we could at the same place just say "if my major/minor is 0, default to something reasonable".
>>>
>>> But let's ask Andreas whether he has a better idea here :).
>>
>> If you read the previous discussion on the initial POWER7+ patch, I
>> believe I had proposed major-version / minor-version or so properties at
>> family level, to be able to use different implementations or none at all
>> where we don't see a scheme.
>
> Sounds like a good idea.
>
>> However if we want to use that from -cpu as
>> in your example above, we would have to implement custom parsing code
>> for cpu_model, which I would rather avoid, given we want to replace it
>> with -device in the future.
>
> Can't we make this generic QOM property parsing code?
>
> -cpu POWER7,major-version=2,minor-version=0
>
> would do
>
> cpu = new POWER7(major-version = 2, minor_version = 0);
>
> and then the POWER7 class can decide what to do with this additional information?
That is "custom parsing code for cpu_model" in target-ppc then. x86 has
its own implementation and so does sparc, both not fully QOM'ified yet,
so there is no one-size-fits-all.
>> But maybe I didn't fully catch the exact question. :)
>>
>> The custom parenting strikes me as a wrong consequence of us not having
>> fully QOM'ified / cleaned up the family classes yet. We had discussed
>> two ways: Either have, e.g., POWER7+ inherit from POWER7 (which looks
>> like the only reason this is being done here) and/or have, e.g., POWER5+
>> copy and modify 970fx values via #defines.
>
> IIUC the family parenting is orthogonal to this. Here we're looking at having families as classes at all. Currently we don't - we only have explicit versioned implementations as classes.
That's simply not true!!! All is hidden by macros as requested by you -
sounds as if that was a bad idea after all. :/
We do have the following:
"object"
+- "device"
+- "cpu"
+- "powerpc64-cpu"
+- "POWER7-family-powerpc64-cpu" -> POWERPC_FAMILY()
+- "POWER7_v2.0-powerpc64-cpu" -> POWERPC_DEF_SVR()
+- "host-powerpc64-cpu" (depending on host PVR)
That's why I was saying: If we need POWER7+-specific family code, we
need to have a POWER7P family and not reuse POWER7 as conveniently done
today. All is there to implement properties or whatever at that level.
And that's also why trying to do the parent tweaking in
POWERPC_DEF_FAMILY_MEMBER() is bogus. The existing infrastructure just
needs to be used the right way, sigh.
And to clean up the aliases business, we should simply move them into
the POWER7_v2.0-powerpc64-cpu level class as an array, I think. That
would greatly simplify -cpu ?, and the alias-to-type lookup would get
faster at the same time since we wouldn't be looking at unavailable
models anymore.
> Whether we have
>
> PowerPC
> `- POWER7
> `- POWER7+
> `- POWER7+ v1.0
>
> or
>
> PowerPC
> `- POWER7+
> `- POWER7+ v1.0
>
> is a different question I think.
My question is: Why are you guys trying to create yet another type for
"POWER7" when we already have one. The only plausible-to-me explanation
was that avoidance of separate POWER7P family was the core cause, but
apparently the core problem is that no one except me is actually
grasping the macro'fied code or at least you lost the overview during
your vacation... :(
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3] powerpc: add PVR mask support
2013-08-15 11:48 ` Andreas Färber
@ 2013-08-15 11:59 ` Alexander Graf
2013-08-19 17:13 ` Andreas Färber
2013-08-15 13:55 ` Alexey Kardashevskiy
1 sibling, 1 reply; 50+ messages in thread
From: Alexander Graf @ 2013-08-15 11:59 UTC (permalink / raw)
To: Andreas Färber
Cc: Anthony Liguori, Alexey Kardashevskiy, qemu-devel, qemu-ppc,
Paul Mackerras
On 15.08.2013, at 13:48, Andreas Färber wrote:
> Am 15.08.2013 13:03, schrieb Alexander Graf:
>>
>> On 15.08.2013, at 12:52, Andreas Färber wrote:
>>
>>> Am 15.08.2013 10:45, schrieb Alexander Graf:
>>>>
>>>> Yes, I think it makes sense to keep the full PVR around when we want to be specific. What I'm referring to is class specific logic that can assemble major/minor numbers from the command line. So
>>>>
>>>> -cpu POWER7,major=2,minor=0
>>>>
>>>> would result in a PVR value that is identical to POWER7_v2.0. The assembly of this PVR value is class specific, because different classes of CPUs have different semantics for their major and minor numbers.
>>>>
>>>> That way in the future we won't have to add any new version specific CPU types but instead the user can assemble those himself, making everyone's life a lot easier.
>>>>
>>>> My point was that if we have that logic, we could at the same place just say "if my major/minor is 0, default to something reasonable".
>>>>
>>>> But let's ask Andreas whether he has a better idea here :).
>>>
>>> If you read the previous discussion on the initial POWER7+ patch, I
>>> believe I had proposed major-version / minor-version or so properties at
>>> family level, to be able to use different implementations or none at all
>>> where we don't see a scheme.
>>
>> Sounds like a good idea.
>>
>>> However if we want to use that from -cpu as
>>> in your example above, we would have to implement custom parsing code
>>> for cpu_model, which I would rather avoid, given we want to replace it
>>> with -device in the future.
>>
>> Can't we make this generic QOM property parsing code?
>>
>> -cpu POWER7,major-version=2,minor-version=0
>>
>> would do
>>
>> cpu = new POWER7(major-version = 2, minor_version = 0);
>>
>> and then the POWER7 class can decide what to do with this additional information?
>
> That is "custom parsing code for cpu_model" in target-ppc then. x86 has
> its own implementation and so does sparc, both not fully QOM'ified yet,
> so there is no one-size-fits-all.
>
>>> But maybe I didn't fully catch the exact question. :)
>>>
>>> The custom parenting strikes me as a wrong consequence of us not having
>>> fully QOM'ified / cleaned up the family classes yet. We had discussed
>>> two ways: Either have, e.g., POWER7+ inherit from POWER7 (which looks
>>> like the only reason this is being done here) and/or have, e.g., POWER5+
>>> copy and modify 970fx values via #defines.
>>
>> IIUC the family parenting is orthogonal to this. Here we're looking at having families as classes at all. Currently we don't - we only have explicit versioned implementations as classes.
>
> That's simply not true!!! All is hidden by macros as requested by you -
> sounds as if that was a bad idea after all. :/
Maybe. I don't know - it's always a tradeoff of readability on different cases. Maybe we simply have too many CPU models too ;).
>
> We do have the following:
>
> "object"
> +- "device"
> +- "cpu"
> +- "powerpc64-cpu"
> +- "POWER7-family-powerpc64-cpu" -> POWERPC_FAMILY()
Ah, there is the family :).
> +- "POWER7_v2.0-powerpc64-cpu" -> POWERPC_DEF_SVR()
> +- "host-powerpc64-cpu" (depending on host PVR)
>
> That's why I was saying: If we need POWER7+-specific family code, we
> need to have a POWER7P family and not reuse POWER7 as conveniently done
> today. All is there to implement properties or whatever at that level.
Ah, so your point is that POWER7+ is-a POWER7 today? That's most likely wrong, yes. Who pulled in that code?
> And that's also why trying to do the parent tweaking in
> POWERPC_DEF_FAMILY_MEMBER() is bogus. The existing infrastructure just
> needs to be used the right way, sigh.
That's not what this patch is about. It's about making the family classes instantiable. Could we just remove the abstract = true piece from the POWER7 class and create that one as our generic POWER7 CPU?
> And to clean up the aliases business, we should simply move them into
> the POWER7_v2.0-powerpc64-cpu level class as an array, I think. That
> would greatly simplify -cpu ?, and the alias-to-type lookup would get
> faster at the same time since we wouldn't be looking at unavailable
> models anymore.
Not sure I follow you on this one :).
>
>> Whether we have
>>
>> PowerPC
>> `- POWER7
>> `- POWER7+
>> `- POWER7+ v1.0
>>
>> or
>>
>> PowerPC
>> `- POWER7+
>> `- POWER7+ v1.0
>>
>> is a different question I think.
>
> My question is: Why are you guys trying to create yet another type for
> "POWER7" when we already have one.
Because we're being dumb ;).
> The only plausible-to-me explanation
> was that avoidance of separate POWER7P family was the core cause, but
> apparently the core problem is that no one except me is actually
> grasping the macro'fied code or at least you lost the overview during
> your vacation... :(
That's the reason, yes :). There's just way too much complexity in this code. The macros were supposed to reduce that, but either they failed or the code is just still too complex despite them.
Alex
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH] powerpc: add PVR mask support
2013-08-15 6:30 ` Benjamin Herrenschmidt
2013-08-15 6:39 ` Alexander Graf
@ 2013-08-15 13:12 ` Anthony Liguori
2013-08-15 13:33 ` Alexander Graf
2013-08-15 15:11 ` Andreas Färber
1 sibling, 2 replies; 50+ messages in thread
From: Anthony Liguori @ 2013-08-15 13:12 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Alexander Graf
Cc: Alexey Kardashevskiy, Paul Mackerras, qemu-ppc, qemu-devel,
Andreas Färber
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH] powerpc: add PVR mask support
2013-08-15 13:12 ` Anthony Liguori
@ 2013-08-15 13:33 ` Alexander Graf
2013-08-15 15:11 ` Andreas Färber
1 sibling, 0 replies; 50+ messages in thread
From: Alexander Graf @ 2013-08-15 13:33 UTC (permalink / raw)
To: Anthony Liguori
Cc: Alexey Kardashevskiy, qemu-devel, qemu-ppc, Paul Mackerras,
Andreas Färber
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3] powerpc: add PVR mask support
2013-08-15 11:48 ` Andreas Färber
2013-08-15 11:59 ` Alexander Graf
@ 2013-08-15 13:55 ` Alexey Kardashevskiy
2013-08-15 14:47 ` Andreas Färber
1 sibling, 1 reply; 50+ messages in thread
From: Alexey Kardashevskiy @ 2013-08-15 13:55 UTC (permalink / raw)
To: Andreas Färber
Cc: Anthony Liguori, Alexander Graf, qemu-devel, qemu-ppc, Paul Mackerras
On 08/15/2013 09:48 PM, Andreas Färber wrote:
> Am 15.08.2013 13:03, schrieb Alexander Graf:
>>
>> On 15.08.2013, at 12:52, Andreas Färber wrote:
>>
>>> Am 15.08.2013 10:45, schrieb Alexander Graf:
>>>>
>>>> Yes, I think it makes sense to keep the full PVR around when we want to be specific. What I'm referring to is class specific logic that can assemble major/minor numbers from the command line. So
>>>>
>>>> -cpu POWER7,major=2,minor=0
>>>>
>>>> would result in a PVR value that is identical to POWER7_v2.0. The assembly of this PVR value is class specific, because different classes of CPUs have different semantics for their major and minor numbers.
>>>>
>>>> That way in the future we won't have to add any new version specific CPU types but instead the user can assemble those himself, making everyone's life a lot easier.
>>>>
>>>> My point was that if we have that logic, we could at the same place just say "if my major/minor is 0, default to something reasonable".
>>>>
>>>> But let's ask Andreas whether he has a better idea here :).
>>>
>>> If you read the previous discussion on the initial POWER7+ patch, I
>>> believe I had proposed major-version / minor-version or so properties at
>>> family level, to be able to use different implementations or none at all
>>> where we don't see a scheme.
>>
>> Sounds like a good idea.
>>
>>> However if we want to use that from -cpu as
>>> in your example above, we would have to implement custom parsing code
>>> for cpu_model, which I would rather avoid, given we want to replace it
>>> with -device in the future.
>>
>> Can't we make this generic QOM property parsing code?
>>
>> -cpu POWER7,major-version=2,minor-version=0
>>
>> would do
>>
>> cpu = new POWER7(major-version = 2, minor_version = 0);
>>
>> and then the POWER7 class can decide what to do with this additional information?
>
> That is "custom parsing code for cpu_model" in target-ppc then. x86 has
> its own implementation and so does sparc, both not fully QOM'ified yet,
> so there is no one-size-fits-all.
>
>>> But maybe I didn't fully catch the exact question. :)
>>>
>>> The custom parenting strikes me as a wrong consequence of us not having
>>> fully QOM'ified / cleaned up the family classes yet. We had discussed
>>> two ways: Either have, e.g., POWER7+ inherit from POWER7 (which looks
>>> like the only reason this is being done here) and/or have, e.g., POWER5+
>>> copy and modify 970fx values via #defines.
>>
>> IIUC the family parenting is orthogonal to this. Here we're looking at having families as classes at all. Currently we don't - we only have explicit versioned implementations as classes.
>
> That's simply not true!!! All is hidden by macros as requested by you -
> sounds as if that was a bad idea after all. :/
>
> We do have the following:
>
> "object"
> +- "device"
> +- "cpu"
> +- "powerpc64-cpu"
> +- "POWER7-family-powerpc64-cpu" -> POWERPC_FAMILY()
> +- "POWER7_v2.0-powerpc64-cpu" -> POWERPC_DEF_SVR()
> +- "host-powerpc64-cpu" (depending on host PVR)
>
> That's why I was saying: If we need POWER7+-specific family code, we
> need to have a POWER7P family and not reuse POWER7 as conveniently done
> today. All is there to implement properties or whatever at that level.
>
> And that's also why trying to do the parent tweaking in
> POWERPC_DEF_FAMILY_MEMBER() is bogus. The existing infrastructure just
> needs to be used the right way, sigh.
>
> And to clean up the aliases business, we should simply move them into
> the POWER7_v2.0-powerpc64-cpu level class as an array, I think. That
> would greatly simplify -cpu ?, and the alias-to-type lookup would get
> faster at the same time since we wouldn't be looking at unavailable
> models anymore.
>
>> Whether we have
>>
>> PowerPC
>> `- POWER7
>> `- POWER7+
>> `- POWER7+ v1.0
>>
>> or
>>
>> PowerPC
>> `- POWER7+
>> `- POWER7+ v1.0
>>
>> is a different question I think.
>
> My question is: Why are you guys trying to create yet another type for
> "POWER7" when we already have one. The only plausible-to-me explanation
> was that avoidance of separate POWER7P family was the core cause, but
> apparently the core problem is that no one except me is actually
> grasping the macro'fied code or at least you lost the overview during
> your vacation... :(
I am not trying to add any additional POWER7. We do not have POWER7 in QEMU
at all, just some approaching/approximation (whatever word suits, sorry for
my weak, terrible english). POWER7 (forget about POWER7+ and others) with
PVR=0x003FAABB would still be absolutely valid POWER7 everywhere but QEMU
(until we support the exact PVR with the specific patch which would add
_anything_ new but just definition). Sorry for my deep ignorance if I miss
the point. Thank you.
--
Alexey
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3] powerpc: add PVR mask support
2013-08-15 8:45 ` Alexander Graf
2013-08-15 10:52 ` Andreas Färber
@ 2013-08-15 14:43 ` Alexey Kardashevskiy
2013-08-15 15:17 ` Alexander Graf
1 sibling, 1 reply; 50+ messages in thread
From: Alexey Kardashevskiy @ 2013-08-15 14:43 UTC (permalink / raw)
To: Alexander Graf
Cc: Anthony Liguori, qemu-devel, qemu-ppc, Paul Mackerras,
Andreas Färber
On 08/15/2013 06:45 PM, Alexander Graf wrote:
>
> On 15.08.2013, at 10:06, Alexey Kardashevskiy wrote:
>
>> On 08/15/2013 05:55 PM, Alexander Graf wrote:
>>>
>>> On 15.08.2013, at 09:45, Alexey Kardashevskiy wrote:
>>>
>>>> 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 which means that aliases are replaced with
>>>> another layer in POWERPC CPU class hierarchy. The patch adds intermediate
>>>> POWER7, POWER7+ and POWER8 CPU classes and makes use of those in
>>>> specific versioned POWERPC CPUs.
>>>>
>>>> Cc: Andreas Färber <afaerber@suse.de>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>
>>>> ---
>>>> Changes:
>>>> v3:
>>>> * renamed macros to describe the functionality better
>>>> * added default PVR value for the powerpc cpu family (what alias used to do)
>>>>
>>>> v2:
>>>> * aliases are replaced with another level in class hierarchy
>>>> ---
>>>> target-ppc/cpu-models.c | 54 ++++++++++++++++++++++++++++++++-------------
>>>> target-ppc/cpu-models.h | 7 ++++++
>>>> target-ppc/cpu-qom.h | 2 ++
>>>> target-ppc/translate_init.c | 4 ++--
>>>> 4 files changed, 50 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c
>>>> index 8dea560..e48004b 100644
>>>> --- a/target-ppc/cpu-models.c
>>>> +++ b/target-ppc/cpu-models.c
>>>> @@ -35,7 +35,8 @@
>>>> /* 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, _pvr_default, \
>>>> + _svr, _type, _parent) \
>>>> static void \
>>>> glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_class_init) \
>>>> (ObjectClass *oc, void *data) \
>>>> @@ -44,6 +45,8 @@
>>>> PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc); \
>>>> \
>>>> pcc->pvr = _pvr; \
>>>> + pcc->pvr_default = _pvr_default; \
>>>> + pcc->pvr_mask = _pvr_mask; \
>>>> pcc->svr = _svr; \
>>>> dc->desc = _desc; \
>>>> } \
>>>> @@ -51,7 +54,7 @@
>>>> static const TypeInfo \
>>>> glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_type_info) = { \
>>>> .name = _name "-" TYPE_POWERPC_CPU, \
>>>> - .parent = stringify(_type) "-family-" TYPE_POWERPC_CPU, \
>>>> + .parent = _parent, \
>>>> .class_init = \
>>>> glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_class_init), \
>>>> }; \
>>>> @@ -66,9 +69,24 @@
>>>> 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, 0, \
>>>> + _svr, _type, \
>>>> + stringify(_type) "-family-" TYPE_POWERPC_CPU)
>>>> +
>>>> #define POWERPC_DEF(_name, _pvr, _type, _desc) \
>>>> POWERPC_DEF_SVR(_name, _desc, _pvr, POWERPC_SVR_NONE, _type)
>>>>
>>>> +#define POWERPC_DEF_FAMILY(_name, _pvr, _pvr_mask, _pvr_default, \
>>>> + _type, _desc) \
>>>> + POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, _pvr_mask, _pvr_default, \
>>>> + POWERPC_SVR_NONE, _type, \
>>>> + stringify(_type) "-family-" TYPE_POWERPC_CPU)
>>>> +
>>>> +#define POWERPC_DEF_FAMILY_MEMBER(_name, _pvr, _type, _desc, _parent) \
>>>> + POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, CPU_POWERPC_DEFAULT_MASK, 0, \
>>>> + POWERPC_SVR_NONE, _type, _parent)
>>>> +
>>>> /* Embedded PowerPC */
>>>> /* PowerPC 401 family */
>>>> POWERPC_DEF("401", CPU_POWERPC_401, 401,
>>>> @@ -1133,16 +1151,25 @@
>>>> 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_FAMILY("POWER7", CPU_POWERPC_POWER7, CPU_POWERPC_POWER7_MASK,
>>>> + CPU_POWERPC_POWER7_v23,
>>>> + POWER7, "POWER7")
>>>> + POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.0", CPU_POWERPC_POWER7_v20, POWER7,
>>>> + "POWER7 v2.0", "POWER7-" TYPE_POWERPC_CPU)
>>>> + POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.1", CPU_POWERPC_POWER7_v21, POWER7,
>>>> + "POWER7 v2.1", "POWER7-" TYPE_POWERPC_CPU)
>>>> + POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.3", CPU_POWERPC_POWER7_v23, POWER7,
>>>> + "POWER7 v2.3", "POWER7-" TYPE_POWERPC_CPU)
>>>> + POWERPC_DEF_FAMILY("POWER7+", CPU_POWERPC_POWER7P, CPU_POWERPC_POWER7P_MASK,
>>>> + CPU_POWERPC_POWER7P_v21,
>>>> + POWER7, "POWER7")
>>>> + POWERPC_DEF_FAMILY_MEMBER("POWER7+_v2.1", CPU_POWERPC_POWER7P_v21, POWER7,
>>>> + "POWER7+ v2.1", "POWER7+-" TYPE_POWERPC_CPU)
>>>> + POWERPC_DEF_FAMILY("POWER8", CPU_POWERPC_POWER8, CPU_POWERPC_POWER8_MASK,
>>>> + CPU_POWERPC_POWER8_v10,
>>>> + POWER8, "POWER8")
>>>> + POWERPC_DEF_FAMILY_MEMBER("POWER8_v1.0", CPU_POWERPC_POWER8_v10, POWER8,
>>>> + "POWER8 v1.0", "POWER8-" TYPE_POWERPC_CPU)
>>>> POWERPC_DEF("970", CPU_POWERPC_970, 970,
>>>> "PowerPC 970")
>>>> POWERPC_DEF("970fx_v1.0", CPU_POWERPC_970FX_v10, 970FX,
>>>> @@ -1389,9 +1416,6 @@ PowerPCCPUAlias ppc_cpu_aliases[] = {
>>>> { "POWER3+", "631" },
>>>> { "POWER5gr", "POWER5" },
>>>> { "POWER5gs", "POWER5+" },
>>>> - { "POWER7", "POWER7_v2.3" },
>>>> - { "POWER7+", "POWER7+_v2.1" },
>>>> - { "POWER8", "POWER8_v1.0" },
>>>> { "970fx", "970fx_v3.1" },
>>>> { "970mp", "970mp_v1.1" },
>>>> { "Apache", "RS64" },
>>>> diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
>>>> index d9145d1..2233053 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
>>>> @@ -557,6 +558,12 @@ enum {
>>>> 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..a1a712c 100644
>>>> --- a/target-ppc/cpu-qom.h
>>>> +++ b/target-ppc/cpu-qom.h
>>>> @@ -54,6 +54,8 @@ typedef struct PowerPCCPUClass {
>>>> void (*parent_reset)(CPUState *cpu);
>>>>
>>>> uint32_t pvr;
>>>> + uint32_t pvr_default;
>>>> + 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..e73792d 100644
>>>> --- a/target-ppc/translate_init.c
>>>> +++ b/target-ppc/translate_init.c
>>>> @@ -7309,7 +7309,7 @@ static void init_ppc_proc(PowerPCCPU *cpu)
>>>> #endif
>>>> SPR_NOACCESS,
>>>> &spr_read_generic, SPR_NOACCESS,
>>>> - pcc->pvr);
>>>> + pcc->pvr_default ? pcc->pvr_default : pcc->pvr);
>>>
>>
>>
>>> This means that -cpu host on a POWER7_v20 system will still return
>>> POWER7_v23 and thus expose a different CPU inside the guest than
>>> expected with PR KVM, no?
>>
>>
>> ./qemu-system-ppc64 \
>> -cpu \
>> POWER7_v2.0 \
>
> This is not -cpu host, is it? :)
Just at the start of a guest. Then the guest reads PVR and if it is HV-KVM
- it will be the actual one OR if it is emulated (PR KVM or TCG) - who
cares, the guest will get what QEMU tells it. Who is unhappy? Sorry for my
deep ignorance if I miss the point. Thank you.
--
Alexey
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3] powerpc: add PVR mask support
2013-08-15 13:55 ` Alexey Kardashevskiy
@ 2013-08-15 14:47 ` Andreas Färber
2013-08-15 15:29 ` Alexander Graf
` (3 more replies)
0 siblings, 4 replies; 50+ messages in thread
From: Andreas Färber @ 2013-08-15 14:47 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Anthony Liguori, Alexander Graf, qemu-devel, qemu-ppc, Paul Mackerras
Am 15.08.2013 15:55, schrieb Alexey Kardashevskiy:
> On 08/15/2013 09:48 PM, Andreas Färber wrote:
>> Am 15.08.2013 13:03, schrieb Alexander Graf:
>>>
>>> On 15.08.2013, at 12:52, Andreas Färber wrote:
>>>
>>>> Am 15.08.2013 10:45, schrieb Alexander Graf:
>>>>>
>>>>> Yes, I think it makes sense to keep the full PVR around when we want to be specific. What I'm referring to is class specific logic that can assemble major/minor numbers from the command line. So
>>>>>
>>>>> -cpu POWER7,major=2,minor=0
>>>>>
>>>>> would result in a PVR value that is identical to POWER7_v2.0. The assembly of this PVR value is class specific, because different classes of CPUs have different semantics for their major and minor numbers.
>>>>>
>>>>> That way in the future we won't have to add any new version specific CPU types but instead the user can assemble those himself, making everyone's life a lot easier.
>>>>>
>>>>> My point was that if we have that logic, we could at the same place just say "if my major/minor is 0, default to something reasonable".
>>>>>
>>>>> But let's ask Andreas whether he has a better idea here :).
>>>>
>>>> If you read the previous discussion on the initial POWER7+ patch, I
>>>> believe I had proposed major-version / minor-version or so properties at
>>>> family level, to be able to use different implementations or none at all
>>>> where we don't see a scheme.
>>>
>>> Sounds like a good idea.
>>>
>>>> However if we want to use that from -cpu as
>>>> in your example above, we would have to implement custom parsing code
>>>> for cpu_model, which I would rather avoid, given we want to replace it
>>>> with -device in the future.
>>>
>>> Can't we make this generic QOM property parsing code?
>>>
>>> -cpu POWER7,major-version=2,minor-version=0
>>>
>>> would do
>>>
>>> cpu = new POWER7(major-version = 2, minor_version = 0);
>>>
>>> and then the POWER7 class can decide what to do with this additional information?
>>
>> That is "custom parsing code for cpu_model" in target-ppc then. x86 has
>> its own implementation and so does sparc, both not fully QOM'ified yet,
>> so there is no one-size-fits-all.
>>
>>>> But maybe I didn't fully catch the exact question. :)
>>>>
>>>> The custom parenting strikes me as a wrong consequence of us not having
>>>> fully QOM'ified / cleaned up the family classes yet. We had discussed
>>>> two ways: Either have, e.g., POWER7+ inherit from POWER7 (which looks
>>>> like the only reason this is being done here) and/or have, e.g., POWER5+
>>>> copy and modify 970fx values via #defines.
>>>
>>> IIUC the family parenting is orthogonal to this. Here we're looking at having families as classes at all. Currently we don't - we only have explicit versioned implementations as classes.
>>
>> That's simply not true!!! All is hidden by macros as requested by you -
>> sounds as if that was a bad idea after all. :/
>>
>> We do have the following:
>>
>> "object"
>> +- "device"
>> +- "cpu"
>> +- "powerpc64-cpu"
>> +- "POWER7-family-powerpc64-cpu" -> POWERPC_FAMILY()
>> +- "POWER7_v2.0-powerpc64-cpu" -> POWERPC_DEF_SVR()
>> +- "host-powerpc64-cpu" (depending on host PVR)
>>
>> That's why I was saying: If we need POWER7+-specific family code, we
>> need to have a POWER7P family and not reuse POWER7 as conveniently done
>> today. All is there to implement properties or whatever at that level.
>>
>> And that's also why trying to do the parent tweaking in
>> POWERPC_DEF_FAMILY_MEMBER() is bogus. The existing infrastructure just
>> needs to be used the right way, sigh.
>>
>> And to clean up the aliases business, we should simply move them into
>> the POWER7_v2.0-powerpc64-cpu level class as an array, I think. That
>> would greatly simplify -cpu ?, and the alias-to-type lookup would get
>> faster at the same time since we wouldn't be looking at unavailable
>> models anymore.
>>
>>> Whether we have
>>>
>>> PowerPC
>>> `- POWER7
>>> `- POWER7+
>>> `- POWER7+ v1.0
>>>
>>> or
>>>
>>> PowerPC
>>> `- POWER7+
>>> `- POWER7+ v1.0
>>>
>>> is a different question I think.
>>
>> My question is: Why are you guys trying to create yet another type for
>> "POWER7" when we already have one. The only plausible-to-me explanation
>> was that avoidance of separate POWER7P family was the core cause, but
>> apparently the core problem is that no one except me is actually
>> grasping the macro'fied code or at least you lost the overview during
>> your vacation... :(
>
>
> I am not trying to add any additional POWER7. We do not have POWER7 in QEMU
> at all, just some approaching/approximation (whatever word suits, sorry for
> my weak, terrible english). POWER7 (forget about POWER7+ and others) with
> PVR=0x003FAABB would still be absolutely valid POWER7 everywhere but QEMU
> (until we support the exact PVR with the specific patch which would add
> _anything_ new but just definition). Sorry for my deep ignorance if I miss
> the point. Thank you.
There is nothing wrong with finding a mask or wildcard solution to that
problem, I already indicated so on the original POWER+ patch. The point
of the whole discussion is how to get there in the least invasive way.
Not whether, just how.
I think - unlike Alex apparently - that the least invasive way is to
leave models as they are and to add masking support to families and KVM
code only. I'm already trying to get away from extending the
POWERPC_DEF* macros for Prerna's fw_name, which are starting to get a
big conflict point these days and a future pain if everyone extends them
for the feature of the day. Note that I started with reading v3, not
everything from the start, and am therefore not pointing fingers at
anyone. It may be that you were given some unfortunate suggestions and
too quick in implementing them.
When we instantiate a -cpu POWER9 then having one POWER9_vX.Y around to
back it doesn't really hurt. Unlike ARM's MIDR there doesn't seem to be
an encoding of IBM vendor or POWER family in the PVR. The macros and
their new implementation are not the way they are because I consider
them the nicest thing in the world but because the name+pvr+svr+family
combination made them work for the whole zoo of models we carry around
and started to give us some inheritance through QOM. Making the POWER7
family non-abstract would require the same kind of macro "overloading"
for POWERPC_FAMILY that I'm trying to contain for POWERPC_DEF ATM. So
what I am still thinking about is how to handle there being multiple
matches for a PVR - I am considering putting them into a list and
comparing values for closest match. So that if you have a v2.4 and QEMU
knows v2.1 and v2.3 we take v2.3 and fill in the v2.4 PVR.
Cheers,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH] powerpc: add PVR mask support
2013-08-15 13:12 ` Anthony Liguori
2013-08-15 13:33 ` Alexander Graf
@ 2013-08-15 15:11 ` Andreas Färber
2013-08-15 15:30 ` Alexander Graf
2013-08-15 16:04 ` Anthony Liguori
1 sibling, 2 replies; 50+ messages in thread
From: Andreas Färber @ 2013-08-15 15:11 UTC (permalink / raw)
To: Anthony Liguori
Cc: Alexey Kardashevskiy, Alexander Graf, qemu-devel, qemu-ppc,
Paul Mackerras
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.
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3] powerpc: add PVR mask support
2013-08-15 14:43 ` [Qemu-devel] [RFC PATCH v3] " Alexey Kardashevskiy
@ 2013-08-15 15:17 ` Alexander Graf
0 siblings, 0 replies; 50+ messages in thread
From: Alexander Graf @ 2013-08-15 15:17 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Anthony Liguori, qemu-devel, qemu-ppc, Paul Mackerras,
Andreas Färber
On 15.08.2013, at 16:43, Alexey Kardashevskiy wrote:
> On 08/15/2013 06:45 PM, Alexander Graf wrote:
>>
>> On 15.08.2013, at 10:06, Alexey Kardashevskiy wrote:
>>
>>> On 08/15/2013 05:55 PM, Alexander Graf wrote:
>>>>
>>>> On 15.08.2013, at 09:45, Alexey Kardashevskiy wrote:
>>>>
>>>>> 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 which means that aliases are replaced with
>>>>> another layer in POWERPC CPU class hierarchy. The patch adds intermediate
>>>>> POWER7, POWER7+ and POWER8 CPU classes and makes use of those in
>>>>> specific versioned POWERPC CPUs.
>>>>>
>>>>> Cc: Andreas Färber <afaerber@suse.de>
>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>>
>>>>> ---
>>>>> Changes:
>>>>> v3:
>>>>> * renamed macros to describe the functionality better
>>>>> * added default PVR value for the powerpc cpu family (what alias used to do)
>>>>>
>>>>> v2:
>>>>> * aliases are replaced with another level in class hierarchy
>>>>> ---
>>>>> target-ppc/cpu-models.c | 54 ++++++++++++++++++++++++++++++++-------------
>>>>> target-ppc/cpu-models.h | 7 ++++++
>>>>> target-ppc/cpu-qom.h | 2 ++
>>>>> target-ppc/translate_init.c | 4 ++--
>>>>> 4 files changed, 50 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c
>>>>> index 8dea560..e48004b 100644
>>>>> --- a/target-ppc/cpu-models.c
>>>>> +++ b/target-ppc/cpu-models.c
>>>>> @@ -35,7 +35,8 @@
>>>>> /* 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, _pvr_default, \
>>>>> + _svr, _type, _parent) \
>>>>> static void \
>>>>> glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_class_init) \
>>>>> (ObjectClass *oc, void *data) \
>>>>> @@ -44,6 +45,8 @@
>>>>> PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc); \
>>>>> \
>>>>> pcc->pvr = _pvr; \
>>>>> + pcc->pvr_default = _pvr_default; \
>>>>> + pcc->pvr_mask = _pvr_mask; \
>>>>> pcc->svr = _svr; \
>>>>> dc->desc = _desc; \
>>>>> } \
>>>>> @@ -51,7 +54,7 @@
>>>>> static const TypeInfo \
>>>>> glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_type_info) = { \
>>>>> .name = _name "-" TYPE_POWERPC_CPU, \
>>>>> - .parent = stringify(_type) "-family-" TYPE_POWERPC_CPU, \
>>>>> + .parent = _parent, \
>>>>> .class_init = \
>>>>> glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_class_init), \
>>>>> }; \
>>>>> @@ -66,9 +69,24 @@
>>>>> 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, 0, \
>>>>> + _svr, _type, \
>>>>> + stringify(_type) "-family-" TYPE_POWERPC_CPU)
>>>>> +
>>>>> #define POWERPC_DEF(_name, _pvr, _type, _desc) \
>>>>> POWERPC_DEF_SVR(_name, _desc, _pvr, POWERPC_SVR_NONE, _type)
>>>>>
>>>>> +#define POWERPC_DEF_FAMILY(_name, _pvr, _pvr_mask, _pvr_default, \
>>>>> + _type, _desc) \
>>>>> + POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, _pvr_mask, _pvr_default, \
>>>>> + POWERPC_SVR_NONE, _type, \
>>>>> + stringify(_type) "-family-" TYPE_POWERPC_CPU)
>>>>> +
>>>>> +#define POWERPC_DEF_FAMILY_MEMBER(_name, _pvr, _type, _desc, _parent) \
>>>>> + POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, CPU_POWERPC_DEFAULT_MASK, 0, \
>>>>> + POWERPC_SVR_NONE, _type, _parent)
>>>>> +
>>>>> /* Embedded PowerPC */
>>>>> /* PowerPC 401 family */
>>>>> POWERPC_DEF("401", CPU_POWERPC_401, 401,
>>>>> @@ -1133,16 +1151,25 @@
>>>>> 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_FAMILY("POWER7", CPU_POWERPC_POWER7, CPU_POWERPC_POWER7_MASK,
>>>>> + CPU_POWERPC_POWER7_v23,
>>>>> + POWER7, "POWER7")
>>>>> + POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.0", CPU_POWERPC_POWER7_v20, POWER7,
>>>>> + "POWER7 v2.0", "POWER7-" TYPE_POWERPC_CPU)
>>>>> + POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.1", CPU_POWERPC_POWER7_v21, POWER7,
>>>>> + "POWER7 v2.1", "POWER7-" TYPE_POWERPC_CPU)
>>>>> + POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.3", CPU_POWERPC_POWER7_v23, POWER7,
>>>>> + "POWER7 v2.3", "POWER7-" TYPE_POWERPC_CPU)
>>>>> + POWERPC_DEF_FAMILY("POWER7+", CPU_POWERPC_POWER7P, CPU_POWERPC_POWER7P_MASK,
>>>>> + CPU_POWERPC_POWER7P_v21,
>>>>> + POWER7, "POWER7")
>>>>> + POWERPC_DEF_FAMILY_MEMBER("POWER7+_v2.1", CPU_POWERPC_POWER7P_v21, POWER7,
>>>>> + "POWER7+ v2.1", "POWER7+-" TYPE_POWERPC_CPU)
>>>>> + POWERPC_DEF_FAMILY("POWER8", CPU_POWERPC_POWER8, CPU_POWERPC_POWER8_MASK,
>>>>> + CPU_POWERPC_POWER8_v10,
>>>>> + POWER8, "POWER8")
>>>>> + POWERPC_DEF_FAMILY_MEMBER("POWER8_v1.0", CPU_POWERPC_POWER8_v10, POWER8,
>>>>> + "POWER8 v1.0", "POWER8-" TYPE_POWERPC_CPU)
>>>>> POWERPC_DEF("970", CPU_POWERPC_970, 970,
>>>>> "PowerPC 970")
>>>>> POWERPC_DEF("970fx_v1.0", CPU_POWERPC_970FX_v10, 970FX,
>>>>> @@ -1389,9 +1416,6 @@ PowerPCCPUAlias ppc_cpu_aliases[] = {
>>>>> { "POWER3+", "631" },
>>>>> { "POWER5gr", "POWER5" },
>>>>> { "POWER5gs", "POWER5+" },
>>>>> - { "POWER7", "POWER7_v2.3" },
>>>>> - { "POWER7+", "POWER7+_v2.1" },
>>>>> - { "POWER8", "POWER8_v1.0" },
>>>>> { "970fx", "970fx_v3.1" },
>>>>> { "970mp", "970mp_v1.1" },
>>>>> { "Apache", "RS64" },
>>>>> diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
>>>>> index d9145d1..2233053 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
>>>>> @@ -557,6 +558,12 @@ enum {
>>>>> 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..a1a712c 100644
>>>>> --- a/target-ppc/cpu-qom.h
>>>>> +++ b/target-ppc/cpu-qom.h
>>>>> @@ -54,6 +54,8 @@ typedef struct PowerPCCPUClass {
>>>>> void (*parent_reset)(CPUState *cpu);
>>>>>
>>>>> uint32_t pvr;
>>>>> + uint32_t pvr_default;
>>>>> + 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..e73792d 100644
>>>>> --- a/target-ppc/translate_init.c
>>>>> +++ b/target-ppc/translate_init.c
>>>>> @@ -7309,7 +7309,7 @@ static void init_ppc_proc(PowerPCCPU *cpu)
>>>>> #endif
>>>>> SPR_NOACCESS,
>>>>> &spr_read_generic, SPR_NOACCESS,
>>>>> - pcc->pvr);
>>>>> + pcc->pvr_default ? pcc->pvr_default : pcc->pvr);
>>>>
>>>
>>>
>>>> This means that -cpu host on a POWER7_v20 system will still return
>>>> POWER7_v23 and thus expose a different CPU inside the guest than
>>>> expected with PR KVM, no?
>>>
>>>
>>> ./qemu-system-ppc64 \
>>> -cpu \
>>> POWER7_v2.0 \
>>
>> This is not -cpu host, is it? :)
>
>
> Just at the start of a guest. Then the guest reads PVR and if it is HV-KVM
> - it will be the actual one OR if it is emulated (PR KVM or TCG) - who
> cares, the guest will get what QEMU tells it. Who is unhappy? Sorry for my
> deep ignorance if I miss the point. Thank you.
PR KVM supports -cpu host and honors what QEMU tells it, so it matters.
Alex
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3] powerpc: add PVR mask support
2013-08-15 14:47 ` Andreas Färber
@ 2013-08-15 15:29 ` Alexander Graf
2013-08-15 15:43 ` Andreas Färber
2013-08-16 0:20 ` Benjamin Herrenschmidt
` (2 subsequent siblings)
3 siblings, 1 reply; 50+ messages in thread
From: Alexander Graf @ 2013-08-15 15:29 UTC (permalink / raw)
To: Andreas Färber
Cc: Anthony Liguori, Alexey Kardashevskiy, qemu-devel, qemu-ppc,
Paul Mackerras
On 15.08.2013, at 16:47, Andreas Färber wrote:
> Am 15.08.2013 15:55, schrieb Alexey Kardashevskiy:
>> On 08/15/2013 09:48 PM, Andreas Färber wrote:
>>> Am 15.08.2013 13:03, schrieb Alexander Graf:
>>>>
>>>> On 15.08.2013, at 12:52, Andreas Färber wrote:
>>>>
>>>>> Am 15.08.2013 10:45, schrieb Alexander Graf:
>>>>>>
>>>>>> Yes, I think it makes sense to keep the full PVR around when we want to be specific. What I'm referring to is class specific logic that can assemble major/minor numbers from the command line. So
>>>>>>
>>>>>> -cpu POWER7,major=2,minor=0
>>>>>>
>>>>>> would result in a PVR value that is identical to POWER7_v2.0. The assembly of this PVR value is class specific, because different classes of CPUs have different semantics for their major and minor numbers.
>>>>>>
>>>>>> That way in the future we won't have to add any new version specific CPU types but instead the user can assemble those himself, making everyone's life a lot easier.
>>>>>>
>>>>>> My point was that if we have that logic, we could at the same place just say "if my major/minor is 0, default to something reasonable".
>>>>>>
>>>>>> But let's ask Andreas whether he has a better idea here :).
>>>>>
>>>>> If you read the previous discussion on the initial POWER7+ patch, I
>>>>> believe I had proposed major-version / minor-version or so properties at
>>>>> family level, to be able to use different implementations or none at all
>>>>> where we don't see a scheme.
>>>>
>>>> Sounds like a good idea.
>>>>
>>>>> However if we want to use that from -cpu as
>>>>> in your example above, we would have to implement custom parsing code
>>>>> for cpu_model, which I would rather avoid, given we want to replace it
>>>>> with -device in the future.
>>>>
>>>> Can't we make this generic QOM property parsing code?
>>>>
>>>> -cpu POWER7,major-version=2,minor-version=0
>>>>
>>>> would do
>>>>
>>>> cpu = new POWER7(major-version = 2, minor_version = 0);
>>>>
>>>> and then the POWER7 class can decide what to do with this additional information?
>>>
>>> That is "custom parsing code for cpu_model" in target-ppc then. x86 has
>>> its own implementation and so does sparc, both not fully QOM'ified yet,
>>> so there is no one-size-fits-all.
>>>
>>>>> But maybe I didn't fully catch the exact question. :)
>>>>>
>>>>> The custom parenting strikes me as a wrong consequence of us not having
>>>>> fully QOM'ified / cleaned up the family classes yet. We had discussed
>>>>> two ways: Either have, e.g., POWER7+ inherit from POWER7 (which looks
>>>>> like the only reason this is being done here) and/or have, e.g., POWER5+
>>>>> copy and modify 970fx values via #defines.
>>>>
>>>> IIUC the family parenting is orthogonal to this. Here we're looking at having families as classes at all. Currently we don't - we only have explicit versioned implementations as classes.
>>>
>>> That's simply not true!!! All is hidden by macros as requested by you -
>>> sounds as if that was a bad idea after all. :/
>>>
>>> We do have the following:
>>>
>>> "object"
>>> +- "device"
>>> +- "cpu"
>>> +- "powerpc64-cpu"
>>> +- "POWER7-family-powerpc64-cpu" -> POWERPC_FAMILY()
>>> +- "POWER7_v2.0-powerpc64-cpu" -> POWERPC_DEF_SVR()
>>> +- "host-powerpc64-cpu" (depending on host PVR)
>>>
>>> That's why I was saying: If we need POWER7+-specific family code, we
>>> need to have a POWER7P family and not reuse POWER7 as conveniently done
>>> today. All is there to implement properties or whatever at that level.
>>>
>>> And that's also why trying to do the parent tweaking in
>>> POWERPC_DEF_FAMILY_MEMBER() is bogus. The existing infrastructure just
>>> needs to be used the right way, sigh.
>>>
>>> And to clean up the aliases business, we should simply move them into
>>> the POWER7_v2.0-powerpc64-cpu level class as an array, I think. That
>>> would greatly simplify -cpu ?, and the alias-to-type lookup would get
>>> faster at the same time since we wouldn't be looking at unavailable
>>> models anymore.
>>>
>>>> Whether we have
>>>>
>>>> PowerPC
>>>> `- POWER7
>>>> `- POWER7+
>>>> `- POWER7+ v1.0
>>>>
>>>> or
>>>>
>>>> PowerPC
>>>> `- POWER7+
>>>> `- POWER7+ v1.0
>>>>
>>>> is a different question I think.
>>>
>>> My question is: Why are you guys trying to create yet another type for
>>> "POWER7" when we already have one. The only plausible-to-me explanation
>>> was that avoidance of separate POWER7P family was the core cause, but
>>> apparently the core problem is that no one except me is actually
>>> grasping the macro'fied code or at least you lost the overview during
>>> your vacation... :(
>>
>>
>> I am not trying to add any additional POWER7. We do not have POWER7 in QEMU
>> at all, just some approaching/approximation (whatever word suits, sorry for
>> my weak, terrible english). POWER7 (forget about POWER7+ and others) with
>> PVR=0x003FAABB would still be absolutely valid POWER7 everywhere but QEMU
>> (until we support the exact PVR with the specific patch which would add
>> _anything_ new but just definition). Sorry for my deep ignorance if I miss
>> the point. Thank you.
>
> There is nothing wrong with finding a mask or wildcard solution to that
> problem, I already indicated so on the original POWER+ patch. The point
> of the whole discussion is how to get there in the least invasive way.
> Not whether, just how.
>
> I think - unlike Alex apparently - that the least invasive way is to
> leave models as they are and to add masking support to families and KVM
> code only.
Not sure I understand. What is KVM specific about this?
> I'm already trying to get away from extending the
> POWERPC_DEF* macros for Prerna's fw_name, which are starting to get a
> big conflict point these days and a future pain if everyone extends them
> for the feature of the day. Note that I started with reading v3, not
> everything from the start, and am therefore not pointing fingers at
> anyone. It may be that you were given some unfortunate suggestions and
> too quick in implementing them.
>
> When we instantiate a -cpu POWER9 then having one POWER9_vX.Y around to
> back it doesn't really hurt. Unlike ARM's MIDR there doesn't seem to be
> an encoding of IBM vendor or POWER family in the PVR. The macros and
> their new implementation are not the way they are because I consider
> them the nicest thing in the world but because the name+pvr+svr+family
> combination made them work for the whole zoo of models we carry around
> and started to give us some inheritance through QOM. Making the POWER7
> family non-abstract would require the same kind of macro "overloading"
> for POWERPC_FAMILY that I'm trying to contain for POWERPC_DEF ATM. So
> what I am still thinking about is how to handle there being multiple
> matches for a PVR - I am considering putting them into a list and
> comparing values for closest match. So that if you have a v2.4 and QEMU
> knows v2.1 and v2.3 we take v2.3 and fill in the v2.4 PVR.
I think this goes into the wrong direction. We should have one single unified scheme to model core versions and -cpu host should be able to override them for a family, no? I don't see how instantiating a POWER7_v20 object on a POWER7_v23 system is any improvement over instantiating a POWER7 object.
Alex
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH] powerpc: add PVR mask support
2013-08-15 15:11 ` Andreas Färber
@ 2013-08-15 15:30 ` Alexander Graf
2013-08-15 15:48 ` Andreas Färber
2013-08-15 16:04 ` Anthony Liguori
1 sibling, 1 reply; 50+ messages in thread
From: Alexander Graf @ 2013-08-15 15:30 UTC (permalink / raw)
To: Andreas Färber
Cc: Alexey Kardashevskiy, qemu-devel, qemu-ppc, Anthony Liguori,
Paul Mackerras
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3] powerpc: add PVR mask support
2013-08-15 15:29 ` Alexander Graf
@ 2013-08-15 15:43 ` Andreas Färber
2013-08-15 15:51 ` Alexander Graf
0 siblings, 1 reply; 50+ messages in thread
From: Andreas Färber @ 2013-08-15 15:43 UTC (permalink / raw)
To: Alexander Graf
Cc: Anthony Liguori, Alexey Kardashevskiy, qemu-devel, qemu-ppc,
Paul Mackerras
Am 15.08.2013 17:29, schrieb Alexander Graf:
>
> On 15.08.2013, at 16:47, Andreas Färber wrote:
>
>> There is nothing wrong with finding a mask or wildcard solution to that
>> problem, I already indicated so on the original POWER+ patch. The point
>> of the whole discussion is how to get there in the least invasive way.
>> Not whether, just how.
>>
>> I think - unlike Alex apparently - that the least invasive way is to
>> leave models as they are and to add masking support to families and KVM
>> code only.
>
> Not sure I understand. What is KVM specific about this?
-cpu host is, it's in kvm.c.
These patches are changing sort comparison code in translate_ppc.c
though, which is used in multiple places.
>
>> I'm already trying to get away from extending the
>> POWERPC_DEF* macros for Prerna's fw_name, which are starting to get a
>> big conflict point these days and a future pain if everyone extends them
>> for the feature of the day. Note that I started with reading v3, not
>> everything from the start, and am therefore not pointing fingers at
>> anyone. It may be that you were given some unfortunate suggestions and
>> too quick in implementing them.
>>
>> When we instantiate a -cpu POWER9 then having one POWER9_vX.Y around to
>> back it doesn't really hurt. Unlike ARM's MIDR there doesn't seem to be
>> an encoding of IBM vendor or POWER family in the PVR. The macros and
>> their new implementation are not the way they are because I consider
>> them the nicest thing in the world but because the name+pvr+svr+family
>> combination made them work for the whole zoo of models we carry around
>> and started to give us some inheritance through QOM. Making the POWER7
>> family non-abstract would require the same kind of macro "overloading"
>> for POWERPC_FAMILY that I'm trying to contain for POWERPC_DEF ATM. So
>> what I am still thinking about is how to handle there being multiple
>> matches for a PVR - I am considering putting them into a list and
>> comparing values for closest match. So that if you have a v2.4 and QEMU
>> knows v2.1 and v2.3 we take v2.3 and fill in the v2.4 PVR.
>
> I think this goes into the wrong direction. We should have one single unified scheme to model core versions and -cpu host should be able to override them for a family, no? I don't see how instantiating a POWER7_v20 object on a POWER7_v23 system is any improvement over instantiating a POWER7 object.
There is no one unified scheme, as we have discussed in your absence.
My point is, a) -cpu POWER7 should result in valid values and b) you
asked to have a unified macro scheme that works for all CPUs, you got
it, now instead you are asking for something that is nice to POWERx, and
we cannot make POWERx family different from the rest wrt macros unless
we break the scheme, which you specifically wanted to have, to avoid
boilerplate QOM code you said. Now you want the full customization
goodness that you were against just before! :)
Andreas
P.S. Please configure your mail client to break lines. Replying is
really hard this way...
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH] powerpc: add PVR mask support
2013-08-15 15:30 ` Alexander Graf
@ 2013-08-15 15:48 ` Andreas Färber
2013-08-15 15:58 ` Alexander Graf
0 siblings, 1 reply; 50+ messages in thread
From: Andreas Färber @ 2013-08-15 15:48 UTC (permalink / raw)
To: Alexander Graf
Cc: Alexey Kardashevskiy, qemu-devel, qemu-ppc, Anthony Liguori,
Paul Mackerras
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
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3] powerpc: add PVR mask support
2013-08-15 15:43 ` Andreas Färber
@ 2013-08-15 15:51 ` Alexander Graf
2013-08-15 16:08 ` Andreas Färber
0 siblings, 1 reply; 50+ messages in thread
From: Alexander Graf @ 2013-08-15 15:51 UTC (permalink / raw)
To: Andreas Färber
Cc: Anthony Liguori, Alexey Kardashevskiy, qemu-devel, qemu-ppc,
Paul Mackerras
On 15.08.2013, at 17:43, Andreas Färber wrote:
> Am 15.08.2013 17:29, schrieb Alexander Graf:
>>
>> On 15.08.2013, at 16:47, Andreas Färber wrote:
>>
>>> There is nothing wrong with finding a mask or wildcard solution to that
>>> problem, I already indicated so on the original POWER+ patch. The point
>>> of the whole discussion is how to get there in the least invasive way.
>>> Not whether, just how.
>>>
>>> I think - unlike Alex apparently - that the least invasive way is to
>>> leave models as they are and to add masking support to families and KVM
>>> code only.
>>
>> Not sure I understand. What is KVM specific about this?
>
> -cpu host is, it's in kvm.c.
>
> These patches are changing sort comparison code in translate_ppc.c
> though, which is used in multiple places.
>
>>
>>> I'm already trying to get away from extending the
>>> POWERPC_DEF* macros for Prerna's fw_name, which are starting to get a
>>> big conflict point these days and a future pain if everyone extends them
>>> for the feature of the day. Note that I started with reading v3, not
>>> everything from the start, and am therefore not pointing fingers at
>>> anyone. It may be that you were given some unfortunate suggestions and
>>> too quick in implementing them.
>>>
>>> When we instantiate a -cpu POWER9 then having one POWER9_vX.Y around to
>>> back it doesn't really hurt. Unlike ARM's MIDR there doesn't seem to be
>>> an encoding of IBM vendor or POWER family in the PVR. The macros and
>>> their new implementation are not the way they are because I consider
>>> them the nicest thing in the world but because the name+pvr+svr+family
>>> combination made them work for the whole zoo of models we carry around
>>> and started to give us some inheritance through QOM. Making the POWER7
>>> family non-abstract would require the same kind of macro "overloading"
>>> for POWERPC_FAMILY that I'm trying to contain for POWERPC_DEF ATM. So
>>> what I am still thinking about is how to handle there being multiple
>>> matches for a PVR - I am considering putting them into a list and
>>> comparing values for closest match. So that if you have a v2.4 and QEMU
>>> knows v2.1 and v2.3 we take v2.3 and fill in the v2.4 PVR.
>>
>> I think this goes into the wrong direction. We should have one single unified scheme to model core versions and -cpu host should be able to override them for a family, no? I don't see how instantiating a POWER7_v20 object on a POWER7_v23 system is any improvement over instantiating a POWER7 object.
>
> There is no one unified scheme, as we have discussed in your absence.
>
> My point is, a) -cpu POWER7 should result in valid values
Yes :)
> and b) you
> asked to have a unified macro scheme that works for all CPUs, you got
> it, now instead you are asking for something that is nice to POWERx, and
> we cannot make POWERx family different from the rest wrt macros unless
> we break the scheme, which you specifically wanted to have, to avoid
> boilerplate QOM code you said. Now you want the full customization
> goodness that you were against just before! :)
Ah, nonono, I don't want POWER to be any different. I want things unified and consistent. Any time I mention "POWER7" I also mean "e500" or "440" or any other family class we have out there.
What I was proposing was to make _all_ families non-abstract and have _all_ families support major/minor parameters.
> Andreas
>
> P.S. Please configure your mail client to break lines. Replying is
> really hard this way...
Phew - no idea how :).
Alex
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH] powerpc: add PVR mask support
2013-08-15 15:48 ` Andreas Färber
@ 2013-08-15 15:58 ` Alexander Graf
2013-08-15 16:22 ` Andreas Färber
0 siblings, 1 reply; 50+ messages in thread
From: Alexander Graf @ 2013-08-15 15:58 UTC (permalink / raw)
To: Andreas Färber
Cc: Alexey Kardashevskiy, qemu-devel, qemu-ppc, Anthony Liguori,
Paul Mackerras
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH] powerpc: add PVR mask support
2013-08-15 15:11 ` Andreas Färber
2013-08-15 15:30 ` Alexander Graf
@ 2013-08-15 16:04 ` Anthony Liguori
1 sibling, 0 replies; 50+ messages in thread
From: Anthony Liguori @ 2013-08-15 16:04 UTC (permalink / raw)
To: Andreas Färber
Cc: Alexey Kardashevskiy, Alexander Graf, qemu-devel, qemu-ppc,
Paul Mackerras
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3] powerpc: add PVR mask support
2013-08-15 15:51 ` Alexander Graf
@ 2013-08-15 16:08 ` Andreas Färber
2013-08-15 16:17 ` Alexander Graf
0 siblings, 1 reply; 50+ messages in thread
From: Andreas Färber @ 2013-08-15 16:08 UTC (permalink / raw)
To: Alexander Graf
Cc: Anthony Liguori, Alexey Kardashevskiy, qemu-devel, qemu-ppc,
Paul Mackerras
Am 15.08.2013 17:51, schrieb Alexander Graf:
>
> On 15.08.2013, at 17:43, Andreas Färber wrote:
>
>> Am 15.08.2013 17:29, schrieb Alexander Graf:
>>>
>>> On 15.08.2013, at 16:47, Andreas Färber wrote:
>>>
>>>> There is nothing wrong with finding a mask or wildcard solution to that
>>>> problem, I already indicated so on the original POWER+ patch. The point
>>>> of the whole discussion is how to get there in the least invasive way.
>>>> Not whether, just how.
>>>>
>>>> I think - unlike Alex apparently - that the least invasive way is to
>>>> leave models as they are and to add masking support to families and KVM
>>>> code only.
>>>
>>> Not sure I understand. What is KVM specific about this?
>>
>> -cpu host is, it's in kvm.c.
>>
>> These patches are changing sort comparison code in translate_ppc.c
>> though, which is used in multiple places.
>>
>>>
>>>> I'm already trying to get away from extending the
>>>> POWERPC_DEF* macros for Prerna's fw_name, which are starting to get a
>>>> big conflict point these days and a future pain if everyone extends them
>>>> for the feature of the day. Note that I started with reading v3, not
>>>> everything from the start, and am therefore not pointing fingers at
>>>> anyone. It may be that you were given some unfortunate suggestions and
>>>> too quick in implementing them.
>>>>
>>>> When we instantiate a -cpu POWER9 then having one POWER9_vX.Y around to
>>>> back it doesn't really hurt. Unlike ARM's MIDR there doesn't seem to be
>>>> an encoding of IBM vendor or POWER family in the PVR. The macros and
>>>> their new implementation are not the way they are because I consider
>>>> them the nicest thing in the world but because the name+pvr+svr+family
>>>> combination made them work for the whole zoo of models we carry around
>>>> and started to give us some inheritance through QOM. Making the POWER7
>>>> family non-abstract would require the same kind of macro "overloading"
>>>> for POWERPC_FAMILY that I'm trying to contain for POWERPC_DEF ATM. So
>>>> what I am still thinking about is how to handle there being multiple
>>>> matches for a PVR - I am considering putting them into a list and
>>>> comparing values for closest match. So that if you have a v2.4 and QEMU
>>>> knows v2.1 and v2.3 we take v2.3 and fill in the v2.4 PVR.
>>>
>>> I think this goes into the wrong direction. We should have one single unified scheme to model core versions and -cpu host should be able to override them for a family, no? I don't see how instantiating a POWER7_v20 object on a POWER7_v23 system is any improvement over instantiating a POWER7 object.
>>
>> There is no one unified scheme, as we have discussed in your absence.
>>
>> My point is, a) -cpu POWER7 should result in valid values
>
> Yes :)
...which requires a specific vX.Y PVR in addition to the mask, i.e. a
model in our current terms. :)
Consider that there may be differences between models within one family,
otherwise there would be little point to distinguish them.
>> and b) you
>> asked to have a unified macro scheme that works for all CPUs, you got
>> it, now instead you are asking for something that is nice to POWERx, and
>> we cannot make POWERx family different from the rest wrt macros unless
>> we break the scheme, which you specifically wanted to have, to avoid
>> boilerplate QOM code you said. Now you want the full customization
>> goodness that you were against just before! :)
>
> Ah, nonono, I don't want POWER to be any different. I want things unified and consistent. Any time I mention "POWER7" I also mean "e500" or "440" or any other family class we have out there.
>
> What I was proposing was to make _all_ families non-abstract and have _all_ families support major/minor parameters.
Again, I pointed out looong ago on the POWER7+ patch
http://patchwork.ozlabs.org/patch/264176/
(which you really could've looked up yourself by now!)
that major/minor does not apply to all CPUs. It works for POWER and for
e500, but that's about it. I specifically gave 440 as an example where
it doesn't!
Note that there's no strict necessity for "host" to be derived from any
existing model, it seemed convenient to me at the time. It could just as
well be created in-place in KVM code iff you can figure out via ioctls
or assembly code what MMU, flags, etc. to fill in beyond PVR - not sure
which fields are even relevant for KVM, I just looked for patterns and
possible OOD / build-time optimizations in that code. :)
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3] powerpc: add PVR mask support
2013-08-15 16:08 ` Andreas Färber
@ 2013-08-15 16:17 ` Alexander Graf
0 siblings, 0 replies; 50+ messages in thread
From: Alexander Graf @ 2013-08-15 16:17 UTC (permalink / raw)
To: Andreas Färber
Cc: Anthony Liguori, Alexey Kardashevskiy, qemu-devel, qemu-ppc,
Paul Mackerras
On 15.08.2013, at 18:08, Andreas Färber wrote:
> Am 15.08.2013 17:51, schrieb Alexander Graf:
>>
>> On 15.08.2013, at 17:43, Andreas Färber wrote:
>>
>>> Am 15.08.2013 17:29, schrieb Alexander Graf:
>>>>
>>>> On 15.08.2013, at 16:47, Andreas Färber wrote:
>>>>
>>>>> There is nothing wrong with finding a mask or wildcard solution to that
>>>>> problem, I already indicated so on the original POWER+ patch. The point
>>>>> of the whole discussion is how to get there in the least invasive way.
>>>>> Not whether, just how.
>>>>>
>>>>> I think - unlike Alex apparently - that the least invasive way is to
>>>>> leave models as they are and to add masking support to families and KVM
>>>>> code only.
>>>>
>>>> Not sure I understand. What is KVM specific about this?
>>>
>>> -cpu host is, it's in kvm.c.
>>>
>>> These patches are changing sort comparison code in translate_ppc.c
>>> though, which is used in multiple places.
>>>
>>>>
>>>>> I'm already trying to get away from extending the
>>>>> POWERPC_DEF* macros for Prerna's fw_name, which are starting to get a
>>>>> big conflict point these days and a future pain if everyone extends them
>>>>> for the feature of the day. Note that I started with reading v3, not
>>>>> everything from the start, and am therefore not pointing fingers at
>>>>> anyone. It may be that you were given some unfortunate suggestions and
>>>>> too quick in implementing them.
>>>>>
>>>>> When we instantiate a -cpu POWER9 then having one POWER9_vX.Y around to
>>>>> back it doesn't really hurt. Unlike ARM's MIDR there doesn't seem to be
>>>>> an encoding of IBM vendor or POWER family in the PVR. The macros and
>>>>> their new implementation are not the way they are because I consider
>>>>> them the nicest thing in the world but because the name+pvr+svr+family
>>>>> combination made them work for the whole zoo of models we carry around
>>>>> and started to give us some inheritance through QOM. Making the POWER7
>>>>> family non-abstract would require the same kind of macro "overloading"
>>>>> for POWERPC_FAMILY that I'm trying to contain for POWERPC_DEF ATM. So
>>>>> what I am still thinking about is how to handle there being multiple
>>>>> matches for a PVR - I am considering putting them into a list and
>>>>> comparing values for closest match. So that if you have a v2.4 and QEMU
>>>>> knows v2.1 and v2.3 we take v2.3 and fill in the v2.4 PVR.
>>>>
>>>> I think this goes into the wrong direction. We should have one single unified scheme to model core versions and -cpu host should be able to override them for a family, no? I don't see how instantiating a POWER7_v20 object on a POWER7_v23 system is any improvement over instantiating a POWER7 object.
>>>
>>> There is no one unified scheme, as we have discussed in your absence.
>>>
>>> My point is, a) -cpu POWER7 should result in valid values
>>
>> Yes :)
>
> ...which requires a specific vX.Y PVR in addition to the mask, i.e. a
> model in our current terms. :)
>
> Consider that there may be differences between models within one family,
> otherwise there would be little point to distinguish them.
>
>>> and b) you
>>> asked to have a unified macro scheme that works for all CPUs, you got
>>> it, now instead you are asking for something that is nice to POWERx, and
>>> we cannot make POWERx family different from the rest wrt macros unless
>>> we break the scheme, which you specifically wanted to have, to avoid
>>> boilerplate QOM code you said. Now you want the full customization
>>> goodness that you were against just before! :)
>>
>> Ah, nonono, I don't want POWER to be any different. I want things unified and consistent. Any time I mention "POWER7" I also mean "e500" or "440" or any other family class we have out there.
>>
>> What I was proposing was to make _all_ families non-abstract and have _all_ families support major/minor parameters.
>
> Again, I pointed out looong ago on the POWER7+ patch
> http://patchwork.ozlabs.org/patch/264176/
> (which you really could've looked up yourself by now!)
> that major/minor does not apply to all CPUs. It works for POWER and for
> e500, but that's about it. I specifically gave 440 as an example where
> it doesn't!
Even 440 cores seem to have a matching mask, at least in Linux:
{
.pvr_mask = 0xf0000fff,
.pvr_value = 0x40000850,
.cpu_name = "440GR Rev. A",
.cpu_features = CPU_FTRS_44X,
.cpu_user_features = COMMON_USER_BOOKE,
.mmu_features = MMU_FTR_TYPE_44x,
.icache_bsize = 32,
.dcache_bsize = 32,
.machine_check = machine_check_4xx,
.platform = "ppc440",
},
But you're right, there isn't as much of a scheme to them as with the others. So for 440 we would need the intermediate level to allow for multiple PVR matches.
So you're saying you'd just always make families abstract and have the pvr mask matching happen on actual core revisions? That just feels gross somehow. Meh.
> Note that there's no strict necessity for "host" to be derived from any
> existing model, it seemed convenient to me at the time. It could just as
> well be created in-place in KVM code iff you can figure out via ioctls
> or assembly code what MMU, flags, etc. to fill in beyond PVR - not sure
> which fields are even relevant for KVM, I just looked for patterns and
> possible OOD / build-time optimizations in that code. :)
I think it makes a lot of sense to share the existing CPU table infrastructure.
Alex
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH] powerpc: add PVR mask support
2013-08-15 15:58 ` Alexander Graf
@ 2013-08-15 16:22 ` Andreas Färber
2013-08-15 17:01 ` Alexander Graf
0 siblings, 1 reply; 50+ messages in thread
From: Andreas Färber @ 2013-08-15 16:22 UTC (permalink / raw)
To: Alexander Graf
Cc: Alexey Kardashevskiy, qemu-devel, qemu-ppc, Anthony Liguori,
Paul Mackerras
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
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH] powerpc: add PVR mask support
2013-08-15 16:22 ` Andreas Färber
@ 2013-08-15 17:01 ` Alexander Graf
0 siblings, 0 replies; 50+ messages in thread
From: Alexander Graf @ 2013-08-15 17:01 UTC (permalink / raw)
To: Andreas Färber
Cc: Alexey Kardashevskiy, qemu-devel, qemu-ppc, Anthony Liguori,
Paul Mackerras
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3] powerpc: add PVR mask support
2013-08-15 14:47 ` Andreas Färber
2013-08-15 15:29 ` Alexander Graf
@ 2013-08-16 0:20 ` Benjamin Herrenschmidt
2013-08-16 0:28 ` Anthony Liguori
2013-08-19 17:34 ` Andreas Färber
2013-08-16 8:07 ` Alexey Kardashevskiy
2013-08-19 4:06 ` [Qemu-devel] [RFC PATCH v4] " Alexey Kardashevskiy
3 siblings, 2 replies; 50+ messages in thread
From: Benjamin Herrenschmidt @ 2013-08-16 0:20 UTC (permalink / raw)
To: Andreas Färber
Cc: Anthony Liguori, Alexey Kardashevskiy, Alexander Graf,
qemu-devel, qemu-ppc, Paul Mackerras
On Thu, 2013-08-15 at 16:47 +0200, Andreas Färber wrote:
> When we instantiate a -cpu POWER9 then having one POWER9_vX.Y around to
> back it doesn't really hurt. Unlike ARM's MIDR there doesn't seem to be
> an encoding of IBM vendor or POWER family in the PVR. The macros and
> their new implementation are not the way they are because I consider
> them the nicest thing in the world but because the name+pvr+svr+family
> combination made them work for the whole zoo of models we carry around
> and started to give us some inheritance through QOM. Making the POWER7
> family non-abstract would require the same kind of macro "overloading"
> for POWERPC_FAMILY that I'm trying to contain for POWERPC_DEF ATM. So
> what I am still thinking about is how to handle there being multiple
> matches for a PVR - I am considering putting them into a list and
> comparing values for closest match. So that if you have a v2.4 and QEMU
> knows v2.1 and v2.3 we take v2.3 and fill in the v2.4 PVR.
Another thing to keep in mind is that we will want eventually to support
POWER7 compatibility more on POWER8 with HV KVM. I am not certain what
the "right" way to do it via qemu command line is, whether we would
have a -cpu entry (-cpu POWER7_COMPAT ?) or such...
Additionally, the trick here is that qemu must be able to change its model
at runtime (a reset is permitted). This is how PAPR defines the reconfiguration
reboot (for that and other things).
IE. The guest kernel will call FW early on, while still operating under
OFW (from prom_init) indicating what it supports, and if that doesn't include
P8, we need to reconfigure the CPU model to be P7 compat (we are allowed to
reboot and reload the same kernel, which is generally what pHyp does, but
we'd like to try avoiding it as much as possible).
Cheers,
Ben.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3] powerpc: add PVR mask support
2013-08-16 0:20 ` Benjamin Herrenschmidt
@ 2013-08-16 0:28 ` Anthony Liguori
2013-08-16 0:30 ` Benjamin Herrenschmidt
2013-08-19 17:34 ` Andreas Färber
1 sibling, 1 reply; 50+ messages in thread
From: Anthony Liguori @ 2013-08-16 0:28 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Anthony Liguori, Alexey Kardashevskiy, qemu-devel,
Alexander Graf, open list:PowerPC, Paul Mackerras,
Andreas Färber
On Thu, Aug 15, 2013 at 7:20 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Thu, 2013-08-15 at 16:47 +0200, Andreas Färber wrote:
>> comparing values for closest match. So that if you have a v2.4 and QEMU
>> knows v2.1 and v2.3 we take v2.3 and fill in the v2.4 PVR.
>
> Another thing to keep in mind is that we will want eventually to support
> POWER7 compatibility more on POWER8 with HV KVM. I am not certain what
> the "right" way to do it via qemu command line is, whether we would
> have a -cpu entry (-cpu POWER7_COMPAT ?) or such...
But this doesn't change the hardware PVR value right, just the virtual
PVR that's in the device tree? Maybe you need to change some state of
the virtual VCPU but from a QEMU point of view, it's still a POWER8
VCPU (it's just in power7 compat mode).
> Additionally, the trick here is that qemu must be able to change its model
> at runtime (a reset is permitted). This is how PAPR defines the reconfiguration
> reboot (for that and other things).
I don't think the model changes. It's just a flag in the power8 vcpu state.
No different IMHO between jumping from real mode to protected mode to
long mode on x86.
Regards,
Anthony Liguori
> IE. The guest kernel will call FW early on, while still operating under
> OFW (from prom_init) indicating what it supports, and if that doesn't include
> P8, we need to reconfigure the CPU model to be P7 compat (we are allowed to
> reboot and reload the same kernel, which is generally what pHyp does, but
> we'd like to try avoiding it as much as possible).
>
> Cheers,
> Ben.
>
>
>
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3] powerpc: add PVR mask support
2013-08-16 0:28 ` Anthony Liguori
@ 2013-08-16 0:30 ` Benjamin Herrenschmidt
0 siblings, 0 replies; 50+ messages in thread
From: Benjamin Herrenschmidt @ 2013-08-16 0:30 UTC (permalink / raw)
To: Anthony Liguori
Cc: Anthony Liguori, Alexey Kardashevskiy, qemu-devel,
Alexander Graf, open list:PowerPC, Paul Mackerras,
Andreas Färber
On Thu, 2013-08-15 at 19:28 -0500, Anthony Liguori wrote:
> On Thu, Aug 15, 2013 at 7:20 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > On Thu, 2013-08-15 at 16:47 +0200, Andreas Färber wrote:
> >> comparing values for closest match. So that if you have a v2.4 and QEMU
> >> knows v2.1 and v2.3 we take v2.3 and fill in the v2.4 PVR.
> >
> > Another thing to keep in mind is that we will want eventually to support
> > POWER7 compatibility more on POWER8 with HV KVM. I am not certain what
> > the "right" way to do it via qemu command line is, whether we would
> > have a -cpu entry (-cpu POWER7_COMPAT ?) or such...
>
> But this doesn't change the hardware PVR value right, just the virtual
> PVR that's in the device tree?
Right.
> Maybe you need to change some state of the virtual VCPU but from a QEMU
> point of view, it's still a POWER8 VCPU (it's just in power7 compat mode).
Sure, as long as TCG does the right thing (some instructions must be
forbidden etc...). That's more what I had in mind...
> > Additionally, the trick here is that qemu must be able to change its model
> > at runtime (a reset is permitted). This is how PAPR defines the reconfiguration
> > reboot (for that and other things).
>
> I don't think the model changes. It's just a flag in the power8 vcpu state.
>
> No different IMHO between jumping from real mode to protected mode to
> long mode on x86.
Ok.
Cheers,
Ben.
> Regards,
>
> Anthony Liguori
>
> > IE. The guest kernel will call FW early on, while still operating under
> > OFW (from prom_init) indicating what it supports, and if that doesn't include
> > P8, we need to reconfigure the CPU model to be P7 compat (we are allowed to
> > reboot and reload the same kernel, which is generally what pHyp does, but
> > we'd like to try avoiding it as much as possible).
> >
> > Cheers,
> > Ben.
> >
> >
> >
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3] powerpc: add PVR mask support
2013-08-15 14:47 ` Andreas Färber
2013-08-15 15:29 ` Alexander Graf
2013-08-16 0:20 ` Benjamin Herrenschmidt
@ 2013-08-16 8:07 ` Alexey Kardashevskiy
2013-08-19 4:06 ` [Qemu-devel] [RFC PATCH v4] " Alexey Kardashevskiy
3 siblings, 0 replies; 50+ messages in thread
From: Alexey Kardashevskiy @ 2013-08-16 8:07 UTC (permalink / raw)
To: Andreas Färber
Cc: Anthony Liguori, Alexander Graf, qemu-devel, qemu-ppc, Paul Mackerras
On 08/16/2013 12:47 AM, Andreas Färber wrote:
> Am 15.08.2013 15:55, schrieb Alexey Kardashevskiy:
>> On 08/15/2013 09:48 PM, Andreas Färber wrote:
>>> Am 15.08.2013 13:03, schrieb Alexander Graf:
>>>>
>>>> On 15.08.2013, at 12:52, Andreas Färber wrote:
>>>>
>>>>> Am 15.08.2013 10:45, schrieb Alexander Graf:
>>>>>>
>>>>>> Yes, I think it makes sense to keep the full PVR around when we want to be specific. What I'm referring to is class specific logic that can assemble major/minor numbers from the command line. So
>>>>>>
>>>>>> -cpu POWER7,major=2,minor=0
>>>>>>
>>>>>> would result in a PVR value that is identical to POWER7_v2.0. The assembly of this PVR value is class specific, because different classes of CPUs have different semantics for their major and minor numbers.
>>>>>>
>>>>>> That way in the future we won't have to add any new version specific CPU types but instead the user can assemble those himself, making everyone's life a lot easier.
>>>>>>
>>>>>> My point was that if we have that logic, we could at the same place just say "if my major/minor is 0, default to something reasonable".
>>>>>>
>>>>>> But let's ask Andreas whether he has a better idea here :).
>>>>>
>>>>> If you read the previous discussion on the initial POWER7+ patch, I
>>>>> believe I had proposed major-version / minor-version or so properties at
>>>>> family level, to be able to use different implementations or none at all
>>>>> where we don't see a scheme.
>>>>
>>>> Sounds like a good idea.
>>>>
>>>>> However if we want to use that from -cpu as
>>>>> in your example above, we would have to implement custom parsing code
>>>>> for cpu_model, which I would rather avoid, given we want to replace it
>>>>> with -device in the future.
>>>>
>>>> Can't we make this generic QOM property parsing code?
>>>>
>>>> -cpu POWER7,major-version=2,minor-version=0
>>>>
>>>> would do
>>>>
>>>> cpu = new POWER7(major-version = 2, minor_version = 0);
>>>>
>>>> and then the POWER7 class can decide what to do with this additional information?
>>>
>>> That is "custom parsing code for cpu_model" in target-ppc then. x86 has
>>> its own implementation and so does sparc, both not fully QOM'ified yet,
>>> so there is no one-size-fits-all.
>>>
>>>>> But maybe I didn't fully catch the exact question. :)
>>>>>
>>>>> The custom parenting strikes me as a wrong consequence of us not having
>>>>> fully QOM'ified / cleaned up the family classes yet. We had discussed
>>>>> two ways: Either have, e.g., POWER7+ inherit from POWER7 (which looks
>>>>> like the only reason this is being done here) and/or have, e.g., POWER5+
>>>>> copy and modify 970fx values via #defines.
>>>>
>>>> IIUC the family parenting is orthogonal to this. Here we're looking at having families as classes at all. Currently we don't - we only have explicit versioned implementations as classes.
>>>
>>> That's simply not true!!! All is hidden by macros as requested by you -
>>> sounds as if that was a bad idea after all. :/
>>>
>>> We do have the following:
>>>
>>> "object"
>>> +- "device"
>>> +- "cpu"
>>> +- "powerpc64-cpu"
>>> +- "POWER7-family-powerpc64-cpu" -> POWERPC_FAMILY()
>>> +- "POWER7_v2.0-powerpc64-cpu" -> POWERPC_DEF_SVR()
>>> +- "host-powerpc64-cpu" (depending on host PVR)
>>>
>>> That's why I was saying: If we need POWER7+-specific family code, we
>>> need to have a POWER7P family and not reuse POWER7 as conveniently done
>>> today. All is there to implement properties or whatever at that level.
>>>
>>> And that's also why trying to do the parent tweaking in
>>> POWERPC_DEF_FAMILY_MEMBER() is bogus. The existing infrastructure just
>>> needs to be used the right way, sigh.
>>>
>>> And to clean up the aliases business, we should simply move them into
>>> the POWER7_v2.0-powerpc64-cpu level class as an array, I think. That
>>> would greatly simplify -cpu ?, and the alias-to-type lookup would get
>>> faster at the same time since we wouldn't be looking at unavailable
>>> models anymore.
>>>
>>>> Whether we have
>>>>
>>>> PowerPC
>>>> `- POWER7
>>>> `- POWER7+
>>>> `- POWER7+ v1.0
>>>>
>>>> or
>>>>
>>>> PowerPC
>>>> `- POWER7+
>>>> `- POWER7+ v1.0
>>>>
>>>> is a different question I think.
>>>
>>> My question is: Why are you guys trying to create yet another type for
>>> "POWER7" when we already have one. The only plausible-to-me explanation
>>> was that avoidance of separate POWER7P family was the core cause, but
>>> apparently the core problem is that no one except me is actually
>>> grasping the macro'fied code or at least you lost the overview during
>>> your vacation... :(
>>
>>
>> I am not trying to add any additional POWER7. We do not have POWER7 in QEMU
>> at all, just some approaching/approximation (whatever word suits, sorry for
>> my weak, terrible english). POWER7 (forget about POWER7+ and others) with
>> PVR=0x003FAABB would still be absolutely valid POWER7 everywhere but QEMU
>> (until we support the exact PVR with the specific patch which would add
>> _anything_ new but just definition). Sorry for my deep ignorance if I miss
>> the point. Thank you.
>
> There is nothing wrong with finding a mask or wildcard solution to that
> problem, I already indicated so on the original POWER+ patch. The point
> of the whole discussion is how to get there in the least invasive way.
> Not whether, just how.
>
> I think - unlike Alex apparently - that the least invasive way is to
> leave models as they are and to add masking support to families and KVM
> code only. I'm already trying to get away from extending the
> POWERPC_DEF* macros for Prerna's fw_name, which are starting to get a
> big conflict point these days and a future pain if everyone extends them
> for the feature of the day. Note that I started with reading v3, not
> everything from the start, and am therefore not pointing fingers at
> anyone. It may be that you were given some unfortunate suggestions and
> too quick in implementing them.
Sorry for my ignorance again, I overlooked that POWER7 is already a family
and it is abstract (did not see families in -cpu ? and got confused). And I
just did not expect families there since Alex Graf said they are not there yet.
I fixed my patch (which does not extend macros) but now I wonder if I
should keep posting it to the list. Do you have any plans to fix this thing
with masks any time soon? Thank you.
--
Alexey
^ permalink raw reply [flat|nested] 50+ messages in thread
* [Qemu-devel] [RFC PATCH v4] powerpc: add PVR mask support
2013-08-15 14:47 ` Andreas Färber
` (2 preceding siblings ...)
2013-08-16 8:07 ` Alexey Kardashevskiy
@ 2013-08-19 4:06 ` Alexey Kardashevskiy
2013-08-26 13:04 ` Alexander Graf
3 siblings, 1 reply; 50+ messages in thread
From: Alexey Kardashevskiy @ 2013-08-19 4:06 UTC (permalink / raw)
To: qemu-devel
Cc: Anthony Liguori, Alexey Kardashevskiy, Alexander Graf, qemu-ppc,
Paul Mackerras, Andreas Färber
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 does the following:
1. add a PVR mask support for a CPU family (in this patch for POWER7 only);
2. make CPU family not abstract;
3. add a @pvr_default (probably bad name) - the idea when QEMU instantiates
POWERPC CPU from a CPU family class, this value will be written to PVR
before the guest starts; KVM uses this to pass the actual PVR value to
the guest if QEMU was started with -cpu host.
As CPU family class name for POWER7 is "POWER7-family", there is no need
to touch aliases.
Cc: Andreas Färber <afaerber@suse.de>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
This does not pretend to be the final valid patch which can go to any tree
(at least because it does need a POWER7+ family difinition and some bits
for POWER8 family), it is me again asking stupid question in order to
reduce my ignorance and get some understanding if anyone going to care of
the PVR masks problem. Thank you for comments.
ps. :)
---
Changes:
v4:
* removed bogus layer from hierarchy
v3:
* renamed macros to describe the functionality better
* added default PVR value for the powerpc cpu family (what alias used to do)
v2:
* aliases are replaced with another level in class hierarchy
---
target-ppc/cpu-models.c | 2 ++
target-ppc/cpu-models.h | 7 +++++++
target-ppc/cpu-qom.h | 2 ++
target-ppc/kvm.c | 2 ++
target-ppc/translate_init.c | 9 +++++++--
5 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c
index 8dea560..5ae88a5 100644
--- a/target-ppc/cpu-models.c
+++ b/target-ppc/cpu-models.c
@@ -44,6 +44,8 @@
PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc); \
\
pcc->pvr = _pvr; \
+ pcc->pvr_default = 0; \
+ pcc->pvr_mask = CPU_POWERPC_DEFAULT_MASK; \
pcc->svr = _svr; \
dc->desc = _desc; \
} \
diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
index d9145d1..2233053 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
@@ -557,6 +558,12 @@ enum {
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..a1a712c 100644
--- a/target-ppc/cpu-qom.h
+++ b/target-ppc/cpu-qom.h
@@ -54,6 +54,8 @@ typedef struct PowerPCCPUClass {
void (*parent_reset)(CPUState *cpu);
uint32_t pvr;
+ uint32_t pvr_default;
+ uint32_t pvr_mask;
uint32_t svr;
uint64_t insns_flags;
uint64_t insns_flags2;
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 30a870e..315e499 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -1731,6 +1731,8 @@ static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
uint32_t dcache_size = kvmppc_read_int_cpu_dt("d-cache-size");
uint32_t icache_size = kvmppc_read_int_cpu_dt("i-cache-size");
+ pcc->pvr_default = mfpvr();
+
/* Now fix up the class with information we can query from the host */
if (vmx != -1) {
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 13b290c..dfe398d 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -3104,7 +3104,6 @@ static int check_pow_hid0_74xx (CPUPPCState *env)
glue(glue(ppc_, _name), _cpu_family_type_info) = { \
.name = stringify(_name) "-family-" TYPE_POWERPC_CPU, \
.parent = TYPE_POWERPC_CPU, \
- .abstract = true, \
.class_init = glue(glue(ppc_, _name), _cpu_family_class_init), \
}; \
\
@@ -7213,6 +7212,9 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data)
PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
dc->desc = "POWER7";
+ pcc->pvr = CPU_POWERPC_POWER7;
+ pcc->pvr_mask = CPU_POWERPC_POWER7_MASK;
+ pcc->pvr_default = CPU_POWERPC_POWER7_v23;
pcc->init_proc = init_proc_POWER7;
pcc->check_pow = check_pow_nocheck;
pcc->insns_flags = PPC_INSNS_BASE | PPC_ISEL | PPC_STRING | PPC_MFTB |
@@ -7309,7 +7311,7 @@ static void init_ppc_proc(PowerPCCPU *cpu)
#endif
SPR_NOACCESS,
&spr_read_generic, SPR_NOACCESS,
- pcc->pvr);
+ pcc->pvr_default ? pcc->pvr_default : pcc->pvr);
/* Register SVR if it's defined to anything else than POWERPC_SVR_NONE */
if (pcc->svr != POWERPC_SVR_NONE) {
if (pcc->svr & POWERPC_SVR_E500) {
@@ -8553,6 +8555,9 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
DeviceClass *dc = DEVICE_CLASS(oc);
pcc->parent_realize = dc->realize;
+ pcc->pvr = CPU_POWERPC_DEFAULT_MASK;
+ pcc->pvr_mask = 0;
+ pcc->pvr_default = 0;
dc->realize = ppc_cpu_realizefn;
dc->unrealize = ppc_cpu_unrealizefn;
--
1.8.3.2
^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3] powerpc: add PVR mask support
2013-08-15 11:59 ` Alexander Graf
@ 2013-08-19 17:13 ` Andreas Färber
0 siblings, 0 replies; 50+ messages in thread
From: Andreas Färber @ 2013-08-19 17:13 UTC (permalink / raw)
To: Alexander Graf
Cc: Alexey Kardashevskiy, Paul Mackerras, Anthony Liguori, qemu-ppc,
qemu-devel
Am 15.08.2013 13:59, schrieb Alexander Graf:
>
> On 15.08.2013, at 13:48, Andreas Färber wrote:
>
>> We do have the following:
>>
>> "object"
>> +- "device"
>> +- "cpu"
>> +- "powerpc64-cpu"
>> +- "POWER7-family-powerpc64-cpu" -> POWERPC_FAMILY()
>
> Ah, there is the family :).
>
>> +- "POWER7_v2.0-powerpc64-cpu" -> POWERPC_DEF_SVR()
>> +- "host-powerpc64-cpu" (depending on host PVR)
>>
>> That's why I was saying: If we need POWER7+-specific family code, we
>> need to have a POWER7P family and not reuse POWER7 as conveniently done
>> today. All is there to implement properties or whatever at that level.
>
> Ah, so your point is that POWER7+ is-a POWER7 today? That's most likely wrong, yes. Who pulled in that code?
Today POWER7+_v2.1-powerpc64-cpu is-a POWER7-family-powerpc64-cpu, yes.
http://git.qemu.org/?p=qemu.git;a=commit;h=03a15a5436ed7723f406f15cc3798aa9991e75b5
http://git.qemu.org/?p=qemu.git;a=commit;h=a7d6b9f084765a834110bb9a2a0329b1a96de792
>> And that's also why trying to do the parent tweaking in
>> POWERPC_DEF_FAMILY_MEMBER() is bogus. The existing infrastructure just
>> needs to be used the right way, sigh.
>
> That's not what this patch is about. It's about making the family classes instantiable. Could we just remove the abstract = true piece from the POWER7 class and create that one as our generic POWER7 CPU?
That seems unnecessary to me since the host type is already
instantiatable and we can set the host's PVR there, whether we derive
from an abstract or non-abstract type.
>> And to clean up the aliases business, we should simply move them into
>> the POWER7_v2.0-powerpc64-cpu level class as an array, I think. That
>> would greatly simplify -cpu ?, and the alias-to-type lookup would get
>> faster at the same time since we wouldn't be looking at unavailable
>> models anymore.
>
> Not sure I follow you on this one :).
You had complained about and uglily bandaided aliases for -cpu ?, some
versions of this patch here touched on aliases and so did the sPAPR
device tree patches I reworked by now to avoid exactly that.
Point is, let's keep separate aspects separate, then we can fix one
problem at a time and gradually move forward rather than constantly
ending up with colliding patches. :)
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3] powerpc: add PVR mask support
2013-08-16 0:20 ` Benjamin Herrenschmidt
2013-08-16 0:28 ` Anthony Liguori
@ 2013-08-19 17:34 ` Andreas Färber
1 sibling, 0 replies; 50+ messages in thread
From: Andreas Färber @ 2013-08-19 17:34 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Peter Maydell, Anthony Liguori, Alexey Kardashevskiy,
Alexander Graf, qemu-devel, Paul Mackerras, qemu-ppc
Am 16.08.2013 02:20, schrieb Benjamin Herrenschmidt:
> On Thu, 2013-08-15 at 16:47 +0200, Andreas Färber wrote:
>> When we instantiate a -cpu POWER9 then having one POWER9_vX.Y around to
>> back it doesn't really hurt. Unlike ARM's MIDR there doesn't seem to be
>> an encoding of IBM vendor or POWER family in the PVR. The macros and
>> their new implementation are not the way they are because I consider
>> them the nicest thing in the world but because the name+pvr+svr+family
>> combination made them work for the whole zoo of models we carry around
>> and started to give us some inheritance through QOM. Making the POWER7
>> family non-abstract would require the same kind of macro "overloading"
>> for POWERPC_FAMILY that I'm trying to contain for POWERPC_DEF ATM. So
>> what I am still thinking about is how to handle there being multiple
>> matches for a PVR - I am considering putting them into a list and
>> comparing values for closest match. So that if you have a v2.4 and QEMU
>> knows v2.1 and v2.3 we take v2.3 and fill in the v2.4 PVR.
>
> Another thing to keep in mind is that we will want eventually to support
> POWER7 compatibility more on POWER8 with HV KVM. I am not certain what
> the "right" way to do it via qemu command line is, whether we would
> have a -cpu entry (-cpu POWER7_COMPAT ?) or such...
There is precedence in having a POWER6_5 model for POWER6 in POWER5
mode, but that is not fully implemented.
Not saying that's how it needs to be done for POWER8.
Question is, when the host is POWER8 in POWER7 mode, should the -cpu
host guest be POWER8 in POWER7 mode or POWER7 or possibly even POWER8?
That question influences whether a simple mask does the job or whether
we need an am-I-compatible-with-this-host callback on the CPUClasses.
> Additionally, the trick here is that qemu must be able to change its model
> at runtime (a reset is permitted). This is how PAPR defines the reconfiguration
> reboot (for that and other things).
>
> IE. The guest kernel will call FW early on, while still operating under
> OFW (from prom_init) indicating what it supports, and if that doesn't include
> P8, we need to reconfigure the CPU model to be P7 compat (we are allowed to
> reboot and reload the same kernel, which is generally what pHyp does, but
> we'd like to try avoiding it as much as possible).
As Anthony indicated, this probably doesn't require to change the model
(in terms of QOM type) but to dynamically change what is exposed to the
guest by POWER8 through registers/fdt/whatever.
A related example in our tree is an ARM CPU model that changes its MIDR
(PVR equivalent) on register write, search for ARM_CPUID_TI915T.
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v4] powerpc: add PVR mask support
2013-08-19 4:06 ` [Qemu-devel] [RFC PATCH v4] " Alexey Kardashevskiy
@ 2013-08-26 13:04 ` Alexander Graf
2013-08-26 14:32 ` Andreas Färber
0 siblings, 1 reply; 50+ messages in thread
From: Alexander Graf @ 2013-08-26 13:04 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Anthony Liguori, qemu-devel, qemu-ppc, Paul Mackerras,
Andreas Färber
On 19.08.2013, at 06:06, Alexey Kardashevskiy wrote:
> 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 does the following:
> 1. add a PVR mask support for a CPU family (in this patch for POWER7 only);
> 2. make CPU family not abstract;
> 3. add a @pvr_default (probably bad name) - the idea when QEMU instantiates
> POWERPC CPU from a CPU family class, this value will be written to PVR
> before the guest starts; KVM uses this to pass the actual PVR value to
> the guest if QEMU was started with -cpu host.
>
> As CPU family class name for POWER7 is "POWER7-family", there is no need
> to touch aliases.
>
> Cc: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>
> ---
>
> This does not pretend to be the final valid patch which can go to any tree
> (at least because it does need a POWER7+ family difinition and some bits
> for POWER8 family), it is me again asking stupid question in order to
> reduce my ignorance and get some understanding if anyone going to care of
> the PVR masks problem. Thank you for comments.
>
> ps. :)
> ---
> Changes:
> v4:
> * removed bogus layer from hierarchy
>
> v3:
> * renamed macros to describe the functionality better
> * added default PVR value for the powerpc cpu family (what alias used to do)
>
> v2:
> * aliases are replaced with another level in class hierarchy
> ---
> target-ppc/cpu-models.c | 2 ++
> target-ppc/cpu-models.h | 7 +++++++
> target-ppc/cpu-qom.h | 2 ++
> target-ppc/kvm.c | 2 ++
> target-ppc/translate_init.c | 9 +++++++--
> 5 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c
> index 8dea560..5ae88a5 100644
> --- a/target-ppc/cpu-models.c
> +++ b/target-ppc/cpu-models.c
> @@ -44,6 +44,8 @@
> PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc); \
> \
> pcc->pvr = _pvr; \
> + pcc->pvr_default = 0; \
There is no need for pvr_default if you limit family instantiation to -cpu host. Just make sure to filter out from cpu ? (and the qmp equivalent) and we should be good.
> + pcc->pvr_mask = CPU_POWERPC_DEFAULT_MASK; \
> pcc->svr = _svr; \
> dc->desc = _desc; \
> } \
> diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
> index d9145d1..2233053 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
> @@ -557,6 +558,12 @@ enum {
> 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,
Please add support for all the other CPUs supported by PR and HV KVM at least on Book3S, but preferably everything Linux supports once this patch is out of RFC.
> 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..a1a712c 100644
> --- a/target-ppc/cpu-qom.h
> +++ b/target-ppc/cpu-qom.h
> @@ -54,6 +54,8 @@ typedef struct PowerPCCPUClass {
> void (*parent_reset)(CPUState *cpu);
>
> uint32_t pvr;
> + uint32_t pvr_default;
> + uint32_t pvr_mask;
> uint32_t svr;
> uint64_t insns_flags;
> uint64_t insns_flags2;
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 30a870e..315e499 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -1731,6 +1731,8 @@ static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
> uint32_t dcache_size = kvmppc_read_int_cpu_dt("d-cache-size");
> uint32_t icache_size = kvmppc_read_int_cpu_dt("i-cache-size");
>
> + pcc->pvr_default = mfpvr();
> +
> /* Now fix up the class with information we can query from the host */
>
> if (vmx != -1) {
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 13b290c..dfe398d 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -3104,7 +3104,6 @@ static int check_pow_hid0_74xx (CPUPPCState *env)
> glue(glue(ppc_, _name), _cpu_family_type_info) = { \
> .name = stringify(_name) "-family-" TYPE_POWERPC_CPU, \
> .parent = TYPE_POWERPC_CPU, \
> - .abstract = true, \
> .class_init = glue(glue(ppc_, _name), _cpu_family_class_init), \
> }; \
> \
> @@ -7213,6 +7212,9 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data)
> PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
>
> dc->desc = "POWER7";
> + pcc->pvr = CPU_POWERPC_POWER7;
> + pcc->pvr_mask = CPU_POWERPC_POWER7_MASK;
> + pcc->pvr_default = CPU_POWERPC_POWER7_v23;
> pcc->init_proc = init_proc_POWER7;
> pcc->check_pow = check_pow_nocheck;
> pcc->insns_flags = PPC_INSNS_BASE | PPC_ISEL | PPC_STRING | PPC_MFTB |
> @@ -7309,7 +7311,7 @@ static void init_ppc_proc(PowerPCCPU *cpu)
> #endif
> SPR_NOACCESS,
> &spr_read_generic, SPR_NOACCESS,
> - pcc->pvr);
> + pcc->pvr_default ? pcc->pvr_default : pcc->pvr);
The automatically generated host class should just take on the host PVR value, so there's no need for jumping through hoops here.
This patch is also missing the matching part :).
Alex
> /* Register SVR if it's defined to anything else than POWERPC_SVR_NONE */
> if (pcc->svr != POWERPC_SVR_NONE) {
> if (pcc->svr & POWERPC_SVR_E500) {
> @@ -8553,6 +8555,9 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
> DeviceClass *dc = DEVICE_CLASS(oc);
>
> pcc->parent_realize = dc->realize;
> + pcc->pvr = CPU_POWERPC_DEFAULT_MASK;
> + pcc->pvr_mask = 0;
> + pcc->pvr_default = 0;
> dc->realize = ppc_cpu_realizefn;
> dc->unrealize = ppc_cpu_unrealizefn;
>
> --
> 1.8.3.2
>
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v4] powerpc: add PVR mask support
2013-08-26 13:04 ` Alexander Graf
@ 2013-08-26 14:32 ` Andreas Färber
2013-08-28 10:31 ` Alexey Kardashevskiy
0 siblings, 1 reply; 50+ messages in thread
From: Andreas Färber @ 2013-08-26 14:32 UTC (permalink / raw)
To: Alexander Graf, Alexey Kardashevskiy
Cc: qemu-ppc, Anthony Liguori, Paul Mackerras, qemu-devel
Am 26.08.2013 15:04, schrieb Alexander Graf:
>
> On 19.08.2013, at 06:06, Alexey Kardashevskiy wrote:
>
>> 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 does the following:
>> 1. add a PVR mask support for a CPU family (in this patch for POWER7 only);
>> 2. make CPU family not abstract;
>> 3. add a @pvr_default (probably bad name) - the idea when QEMU instantiates
>> POWERPC CPU from a CPU family class, this value will be written to PVR
>> before the guest starts; KVM uses this to pass the actual PVR value to
>> the guest if QEMU was started with -cpu host.
>>
>> As CPU family class name for POWER7 is "POWER7-family", there is no need
>> to touch aliases.
>>
>> Cc: Andreas Färber <afaerber@suse.de>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>
>> ---
>>
>> This does not pretend to be the final valid patch which can go to any tree
>> (at least because it does need a POWER7+ family difinition and some bits
>> for POWER8 family), it is me again asking stupid question in order to
>> reduce my ignorance and get some understanding if anyone going to care of
>> the PVR masks problem. Thank you for comments.
>>
>> ps. :)
>> ---
>> Changes:
>> v4:
>> * removed bogus layer from hierarchy
>>
>> v3:
>> * renamed macros to describe the functionality better
>> * added default PVR value for the powerpc cpu family (what alias used to do)
>>
>> v2:
>> * aliases are replaced with another level in class hierarchy
>> ---
>> target-ppc/cpu-models.c | 2 ++
>> target-ppc/cpu-models.h | 7 +++++++
>> target-ppc/cpu-qom.h | 2 ++
>> target-ppc/kvm.c | 2 ++
>> target-ppc/translate_init.c | 9 +++++++--
>> 5 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c
>> index 8dea560..5ae88a5 100644
>> --- a/target-ppc/cpu-models.c
>> +++ b/target-ppc/cpu-models.c
>> @@ -44,6 +44,8 @@
>> PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc); \
>> \
>> pcc->pvr = _pvr; \
>> + pcc->pvr_default = 0; \
>
> There is no need for pvr_default if you limit family instantiation to -cpu host. Just make sure to filter out from cpu ? (and the qmp equivalent) and we should be good.
Matches what I was going to reply.
>> + pcc->pvr_mask = CPU_POWERPC_DEFAULT_MASK; \
>> pcc->svr = _svr; \
>> dc->desc = _desc; \
>> } \
>> diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
>> index d9145d1..2233053 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
>> @@ -557,6 +558,12 @@ enum {
>> 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,
>
> Please add support for all the other CPUs supported by PR and HV KVM at least on Book3S, but preferably everything Linux supports once this patch is out of RFC.
Personally I didn't see that as a hard requirement - better to start
somewhere where we are sure than touching everything and finding no one
to ack. ;)
What I would prefer here is to move the mask before the concrete PVRs
and possibly to come up with a more telling suffix for the base PVR so
that it doesn't get mixed up with a "real" PVR.
E.g.,
CPU_POWERPC_POWER7_BASE = 0x12340000,
CPU_POWERPC_POWER7_MASK = 0xffff0000,
CPU_POWERPC_POWER7_V21 = 0x12340201,
CPU_POWERPC_POWER8_...
>
>> 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..a1a712c 100644
>> --- a/target-ppc/cpu-qom.h
>> +++ b/target-ppc/cpu-qom.h
>> @@ -54,6 +54,8 @@ typedef struct PowerPCCPUClass {
>> void (*parent_reset)(CPUState *cpu);
>>
>> uint32_t pvr;
>> + uint32_t pvr_default;
>> + uint32_t pvr_mask;
>> uint32_t svr;
>> uint64_t insns_flags;
>> uint64_t insns_flags2;
>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>> index 30a870e..315e499 100644
>> --- a/target-ppc/kvm.c
>> +++ b/target-ppc/kvm.c
>> @@ -1731,6 +1731,8 @@ static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
>> uint32_t dcache_size = kvmppc_read_int_cpu_dt("d-cache-size");
>> uint32_t icache_size = kvmppc_read_int_cpu_dt("i-cache-size");
>>
>> + pcc->pvr_default = mfpvr();
>> +
>> /* Now fix up the class with information we can query from the host */
>>
>> if (vmx != -1) {
This should be moved under the "fix up ... from host" heading. :)
>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>> index 13b290c..dfe398d 100644
>> --- a/target-ppc/translate_init.c
>> +++ b/target-ppc/translate_init.c
>> @@ -3104,7 +3104,6 @@ static int check_pow_hid0_74xx (CPUPPCState *env)
>> glue(glue(ppc_, _name), _cpu_family_type_info) = { \
>> .name = stringify(_name) "-family-" TYPE_POWERPC_CPU, \
>> .parent = TYPE_POWERPC_CPU, \
>> - .abstract = true, \
>> .class_init = glue(glue(ppc_, _name), _cpu_family_class_init), \
>> }; \
>> \
This should no longer be necessary (once the fuzzy search is implemented
in KVM host type registration code path) and trivially solves the -cpu ?
/ QMP aspect Alex mentioned above.
>> @@ -7213,6 +7212,9 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data)
>> PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
>>
>> dc->desc = "POWER7";
>> + pcc->pvr = CPU_POWERPC_POWER7;
>> + pcc->pvr_mask = CPU_POWERPC_POWER7_MASK;
>> + pcc->pvr_default = CPU_POWERPC_POWER7_v23;
>> pcc->init_proc = init_proc_POWER7;
>> pcc->check_pow = check_pow_nocheck;
>> pcc->insns_flags = PPC_INSNS_BASE | PPC_ISEL | PPC_STRING | PPC_MFTB |
>> @@ -7309,7 +7311,7 @@ static void init_ppc_proc(PowerPCCPU *cpu)
>> #endif
>> SPR_NOACCESS,
>> &spr_read_generic, SPR_NOACCESS,
>> - pcc->pvr);
>> + pcc->pvr_default ? pcc->pvr_default : pcc->pvr);
>
> The automatically generated host class should just take on the host PVR value, so there's no need for jumping through hoops here.
>
> This patch is also missing the matching part :).
2x Yep.
But it's an RFC and I think we're steering into the right direction now. :)
Andreas
>
>
> Alex
>
>> /* Register SVR if it's defined to anything else than POWERPC_SVR_NONE */
>> if (pcc->svr != POWERPC_SVR_NONE) {
>> if (pcc->svr & POWERPC_SVR_E500) {
>> @@ -8553,6 +8555,9 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>> DeviceClass *dc = DEVICE_CLASS(oc);
>>
>> pcc->parent_realize = dc->realize;
>> + pcc->pvr = CPU_POWERPC_DEFAULT_MASK;
>> + pcc->pvr_mask = 0;
>> + pcc->pvr_default = 0;
>> dc->realize = ppc_cpu_realizefn;
>> dc->unrealize = ppc_cpu_unrealizefn;
>>
>> --
>> 1.8.3.2
>>
>
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v4] powerpc: add PVR mask support
2013-08-26 14:32 ` Andreas Färber
@ 2013-08-28 10:31 ` Alexey Kardashevskiy
2013-08-28 10:34 ` Andreas Färber
0 siblings, 1 reply; 50+ messages in thread
From: Alexey Kardashevskiy @ 2013-08-28 10:31 UTC (permalink / raw)
To: Andreas Färber
Cc: Anthony Liguori, Alexander Graf, qemu-devel, qemu-ppc, Paul Mackerras
On 08/27/2013 12:32 AM, Andreas Färber wrote:
> Am 26.08.2013 15:04, schrieb Alexander Graf:
>>
>> On 19.08.2013, at 06:06, Alexey Kardashevskiy wrote:
>>
>>> 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 does the following:
>>> 1. add a PVR mask support for a CPU family (in this patch for POWER7 only);
>>> 2. make CPU family not abstract;
>>> 3. add a @pvr_default (probably bad name) - the idea when QEMU instantiates
>>> POWERPC CPU from a CPU family class, this value will be written to PVR
>>> before the guest starts; KVM uses this to pass the actual PVR value to
>>> the guest if QEMU was started with -cpu host.
>>>
>>> As CPU family class name for POWER7 is "POWER7-family", there is no need
>>> to touch aliases.
>>>
>>> Cc: Andreas Färber <afaerber@suse.de>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>
>>> ---
>>>
>>> This does not pretend to be the final valid patch which can go to any tree
>>> (at least because it does need a POWER7+ family difinition and some bits
>>> for POWER8 family), it is me again asking stupid question in order to
>>> reduce my ignorance and get some understanding if anyone going to care of
>>> the PVR masks problem. Thank you for comments.
>>>
>>> ps. :)
>>> ---
>>> Changes:
>>> v4:
>>> * removed bogus layer from hierarchy
>>>
>>> v3:
>>> * renamed macros to describe the functionality better
>>> * added default PVR value for the powerpc cpu family (what alias used to do)
>>>
>>> v2:
>>> * aliases are replaced with another level in class hierarchy
>>> ---
>>> target-ppc/cpu-models.c | 2 ++
>>> target-ppc/cpu-models.h | 7 +++++++
>>> target-ppc/cpu-qom.h | 2 ++
>>> target-ppc/kvm.c | 2 ++
>>> target-ppc/translate_init.c | 9 +++++++--
>>> 5 files changed, 20 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c
>>> index 8dea560..5ae88a5 100644
>>> --- a/target-ppc/cpu-models.c
>>> +++ b/target-ppc/cpu-models.c
>>> @@ -44,6 +44,8 @@
>>> PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc); \
>>> \
>>> pcc->pvr = _pvr; \
>>> + pcc->pvr_default = 0; \
>>
>> There is no need for pvr_default if you limit family instantiation to -cpu host. Just make sure to filter out from cpu ? (and the qmp equivalent) and we should be good.
>
> Matches what I was going to reply.
>
>>> + pcc->pvr_mask = CPU_POWERPC_DEFAULT_MASK; \
>>> pcc->svr = _svr; \
>>> dc->desc = _desc; \
>>> } \
>>> diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
>>> index d9145d1..2233053 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
>>> @@ -557,6 +558,12 @@ enum {
>>> 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,
>>
>> Please add support for all the other CPUs supported by PR and HV KVM at least on Book3S, but preferably everything Linux supports once this patch is out of RFC.
>
> Personally I didn't see that as a hard requirement - better to start
> somewhere where we are sure than touching everything and finding no one
> to ack. ;)
>
> What I would prefer here is to move the mask before the concrete PVRs
> and possibly to come up with a more telling suffix for the base PVR so
> that it doesn't get mixed up with a "real" PVR.
>
> E.g.,
> CPU_POWERPC_POWER7_BASE = 0x12340000,
> CPU_POWERPC_POWER7_MASK = 0xffff0000,
> CPU_POWERPC_POWER7_V21 = 0x12340201,
> CPU_POWERPC_POWER8_...
>
>>
>>> 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..a1a712c 100644
>>> --- a/target-ppc/cpu-qom.h
>>> +++ b/target-ppc/cpu-qom.h
>>> @@ -54,6 +54,8 @@ typedef struct PowerPCCPUClass {
>>> void (*parent_reset)(CPUState *cpu);
>>>
>>> uint32_t pvr;
>>> + uint32_t pvr_default;
>>> + uint32_t pvr_mask;
>>> uint32_t svr;
>>> uint64_t insns_flags;
>>> uint64_t insns_flags2;
>>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>>> index 30a870e..315e499 100644
>>> --- a/target-ppc/kvm.c
>>> +++ b/target-ppc/kvm.c
>>> @@ -1731,6 +1731,8 @@ static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
>>> uint32_t dcache_size = kvmppc_read_int_cpu_dt("d-cache-size");
>>> uint32_t icache_size = kvmppc_read_int_cpu_dt("i-cache-size");
>>>
>>> + pcc->pvr_default = mfpvr();
>>> +
>>> /* Now fix up the class with information we can query from the host */
>>>
>>> if (vmx != -1) {
>
> This should be moved under the "fix up ... from host" heading. :)
>
>>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>>> index 13b290c..dfe398d 100644
>>> --- a/target-ppc/translate_init.c
>>> +++ b/target-ppc/translate_init.c
>>> @@ -3104,7 +3104,6 @@ static int check_pow_hid0_74xx (CPUPPCState *env)
>>> glue(glue(ppc_, _name), _cpu_family_type_info) = { \
>>> .name = stringify(_name) "-family-" TYPE_POWERPC_CPU, \
>>> .parent = TYPE_POWERPC_CPU, \
>>> - .abstract = true, \
>>> .class_init = glue(glue(ppc_, _name), _cpu_family_class_init), \
>>> }; \
>>> \
>
> This should no longer be necessary (once the fuzzy search is implemented
> in KVM host type registration code path) and trivially solves the -cpu ?
> / QMP aspect Alex mentioned above.
I do not really understand this part. Will we still need/want
.abstract==true? I'll post yet another version of my patch in next 3
minutes, there I skip PPC CPU classes which parent is TYPE_POWERPC_CPU,
pretty trivial already :)
--
Alexey
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v4] powerpc: add PVR mask support
2013-08-28 10:31 ` Alexey Kardashevskiy
@ 2013-08-28 10:34 ` Andreas Färber
0 siblings, 0 replies; 50+ messages in thread
From: Andreas Färber @ 2013-08-28 10:34 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Anthony Liguori, Alexander Graf, qemu-devel, qemu-ppc, Paul Mackerras
Am 28.08.2013 12:31, schrieb Alexey Kardashevskiy:
> On 08/27/2013 12:32 AM, Andreas Färber wrote:
>> Am 26.08.2013 15:04, schrieb Alexander Graf:
>>>
>>> On 19.08.2013, at 06:06, Alexey Kardashevskiy wrote:
>>>
>>>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>>>> index 13b290c..dfe398d 100644
>>>> --- a/target-ppc/translate_init.c
>>>> +++ b/target-ppc/translate_init.c
>>>> @@ -3104,7 +3104,6 @@ static int check_pow_hid0_74xx (CPUPPCState *env)
>>>> glue(glue(ppc_, _name), _cpu_family_type_info) = { \
>>>> .name = stringify(_name) "-family-" TYPE_POWERPC_CPU, \
>>>> .parent = TYPE_POWERPC_CPU, \
>>>> - .abstract = true, \
>>>> .class_init = glue(glue(ppc_, _name), _cpu_family_class_init), \
>>>> }; \
>>>> \
>>
>> This should no longer be necessary (once the fuzzy search is implemented
>> in KVM host type registration code path) and trivially solves the -cpu ?
>> / QMP aspect Alex mentioned above.
>
>
> I do not really understand this part. Will we still need/want
> .abstract==true?
No, we don't need/want it since the host CPU type is non-abstract
already and can inherit from abstract types without problem.
Andreas
> I'll post yet another version of my patch in next 3
> minutes, there I skip PPC CPU classes which parent is TYPE_POWERPC_CPU,
> pretty trivial already :)
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 50+ messages in thread
end of thread, other threads:[~2013-08-28 10:34 UTC | newest]
Thread overview: 50+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-15 3:35 [Qemu-devel] [RFC PATCH] powerpc: add PVR mask support Alexey Kardashevskiy
2013-08-15 5:21 ` Alexander Graf
2013-08-15 5:44 ` Alexey Kardashevskiy
2013-08-15 6:03 ` Alexander Graf
2013-08-15 6:30 ` Benjamin Herrenschmidt
2013-08-15 6:39 ` Alexander Graf
2013-08-15 13:12 ` Anthony Liguori
2013-08-15 13:33 ` Alexander Graf
2013-08-15 15:11 ` Andreas Färber
2013-08-15 15:30 ` Alexander Graf
2013-08-15 15:48 ` Andreas Färber
2013-08-15 15:58 ` Alexander Graf
2013-08-15 16:22 ` Andreas Färber
2013-08-15 17:01 ` Alexander Graf
2013-08-15 16:04 ` Anthony Liguori
2013-08-15 5:54 ` Benjamin Herrenschmidt
2013-08-15 6:10 ` Alexander Graf
2013-08-15 6:28 ` Benjamin Herrenschmidt
2013-08-15 6:37 ` Alexander Graf
2013-08-15 6:00 ` [Qemu-devel] [Qemu-ppc] " Alexander Graf
2013-08-15 6:43 ` [Qemu-devel] [_R_F_C_ PATCH v2] " Alexey Kardashevskiy
2013-08-15 6:57 ` Alexander Graf
2013-08-15 7:45 ` [Qemu-devel] [RFC PATCH v3] " Alexey Kardashevskiy
2013-08-15 7:55 ` Alexander Graf
2013-08-15 8:06 ` Alexey Kardashevskiy
2013-08-15 8:45 ` Alexander Graf
2013-08-15 10:52 ` Andreas Färber
2013-08-15 11:03 ` Alexander Graf
2013-08-15 11:48 ` Andreas Färber
2013-08-15 11:59 ` Alexander Graf
2013-08-19 17:13 ` Andreas Färber
2013-08-15 13:55 ` Alexey Kardashevskiy
2013-08-15 14:47 ` Andreas Färber
2013-08-15 15:29 ` Alexander Graf
2013-08-15 15:43 ` Andreas Färber
2013-08-15 15:51 ` Alexander Graf
2013-08-15 16:08 ` Andreas Färber
2013-08-15 16:17 ` Alexander Graf
2013-08-16 0:20 ` Benjamin Herrenschmidt
2013-08-16 0:28 ` Anthony Liguori
2013-08-16 0:30 ` Benjamin Herrenschmidt
2013-08-19 17:34 ` Andreas Färber
2013-08-16 8:07 ` Alexey Kardashevskiy
2013-08-19 4:06 ` [Qemu-devel] [RFC PATCH v4] " Alexey Kardashevskiy
2013-08-26 13:04 ` Alexander Graf
2013-08-26 14:32 ` Andreas Färber
2013-08-28 10:31 ` Alexey Kardashevskiy
2013-08-28 10:34 ` Andreas Färber
2013-08-15 14:43 ` [Qemu-devel] [RFC PATCH v3] " Alexey Kardashevskiy
2013-08-15 15:17 ` Alexander Graf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.