[Qemu-devel] [PATCH] spapr: add "compat" machine option

[Qemu-devel] [PATCH] spapr: add "compat" machine option

[Alexey Kardashevskiy]

To be able to boot on newer hardware that the software support,
PowerISA defines a logical PVR, one per every PowerISA specification
version from 2.04.

This adds the "compat" option which takes values 205 or 206 and forces
QEMU to boot the guest with a logical PVR (CPU_POWERPC_LOGICAL_2_05 or
CPU_POWERPC_LOGICAL_2_06).

The guest reads the logical PVR value from "cpu-version" property of
a CPU device node.

Cc: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
Cc: Andreas Färber <afaerber@suse.de>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/ppc/spapr.c          | 40 ++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h  |  2 ++
 target-ppc/cpu-models.h | 10 ++++++++++
 target-ppc/cpu.h        |  3 +++
 target-ppc/kvm.c        |  2 ++
 vl.c                    |  4 ++++
 6 files changed, 61 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index a09a1d9..737452d 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -33,6 +33,7 @@
 #include "sysemu/kvm.h"
 #include "kvm_ppc.h"
 #include "mmu-hash64.h"
+#include "cpu-models.h"
 
 #include "hw/boards.h"
 #include "hw/ppc/ppc.h"
@@ -196,6 +197,26 @@ static XICSState *xics_system_init(int nr_servers, int nr_irqs)
     return icp;
 }
 
+static void spapr_compat_mode_init(sPAPREnvironment *spapr)
+{
+    QemuOpts *machine_opts = qemu_get_machine_opts();
+    uint64_t compat = qemu_opt_get_number(machine_opts, "compat", 0);
+
+    switch (compat) {
+    case 0:
+        break;
+    case 205:
+        spapr->arch_compat = CPU_POWERPC_LOGICAL_2_05;
+        break;
+    case 206:
+        spapr->arch_compat = CPU_POWERPC_LOGICAL_2_06;
+        break;
+    default:
+        perror("Unsupported mode, only are 205, 206 supported\n");
+        break;
+    }
+}
+
 static int spapr_fixup_cpu_dt(void *fdt, sPAPREnvironment *spapr)
 {
     int ret = 0, offset;
@@ -206,6 +227,7 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPREnvironment *spapr)
 
     CPU_FOREACH(cpu) {
         DeviceClass *dc = DEVICE_GET_CLASS(cpu);
+        CPUPPCState *env = &(POWERPC_CPU(cpu)->env);
         uint32_t associativity[] = {cpu_to_be32(0x5),
                                     cpu_to_be32(0x0),
                                     cpu_to_be32(0x0),
@@ -238,6 +260,14 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPREnvironment *spapr)
         if (ret < 0) {
             return ret;
         }
+
+        if (env->arch_compat) {
+            ret = fdt_setprop(fdt, offset, "cpu-version",
+                              &env->arch_compat, sizeof(env->arch_compat));
+            if (ret < 0) {
+                return ret;
+            }
+        }
     }
     return ret;
 }
@@ -1145,6 +1175,8 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
     spapr = g_malloc0(sizeof(*spapr));
     QLIST_INIT(&spapr->phbs);
 
+    spapr_compat_mode_init(spapr);
+
     cpu_ppc_hypercall = emulate_spapr_hypercall;
 
     /* Allocate RMA if necessary */
@@ -1226,6 +1258,14 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
 
         xics_cpu_setup(spapr->icp, cpu);
 
+        /*
+         * If compat mode is set in the command line, pass it to CPU so KVM
+         * will be able to set it in the host kernel.
+         */
+        if (spapr->arch_compat) {
+            env->arch_compat = spapr->arch_compat;
+        }
+
         qemu_register_reset(spapr_cpu_reset, cpu);
     }
 
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index ca175b0..201c578 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -34,6 +34,8 @@ typedef struct sPAPREnvironment {
     uint32_t epow_irq;
     Notifier epow_notifier;
 
+    uint32_t arch_compat;        /* Compatible PVR from the command line */
+
     /* Migration state */
     int htab_save_index;
     bool htab_first_pass;
diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
index 49ba4a4..d7c033c 100644
--- a/target-ppc/cpu-models.h
+++ b/target-ppc/cpu-models.h
@@ -583,6 +583,16 @@ enum {
     CPU_POWERPC_RS64II             = 0x00340000,
     CPU_POWERPC_RS64III            = 0x00360000,
     CPU_POWERPC_RS64IV             = 0x00370000,
+
+    /* Logical CPUs */
+    CPU_POWERPC_LOGICAL_MASK       = 0xFFFFFFFF,
+    CPU_POWERPC_LOGICAL_2_04       = 0x0F000001,
+    CPU_POWERPC_LOGICAL_2_05       = 0x0F000002,
+    CPU_POWERPC_LOGICAL_2_06       = 0x0F000003,
+    CPU_POWERPC_LOGICAL_2_06_PLUS  = 0x0F100003,
+    CPU_POWERPC_LOGICAL_2_07       = 0x0F000004,
+    CPU_POWERPC_LOGICAL_2_08       = 0x0F000005,
+
 #endif /* defined(TARGET_PPC64) */
     /* Original POWER */
     /* XXX: should be POWER (RIOS), RSC3308, RSC4608,
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 422a6bb..fc837c1 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -999,6 +999,9 @@ struct CPUPPCState {
     /* Device control registers */
     ppc_dcr_t *dcr_env;
 
+    /* Architecture compatibility mode */
+    uint32_t arch_compat;
+
     int dcache_line_size;
     int icache_line_size;
 
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 4bc4496..7b853a3 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -872,6 +872,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
                 DPRINTF("Warning: Unable to set VPA information to KVM\n");
             }
         }
+        kvm_set_one_reg(cs, KVM_REG_PPC_ARCH_COMPAT, &env->arch_compat);
 #endif /* TARGET_PPC64 */
     }
 
@@ -1083,6 +1084,7 @@ int kvm_arch_get_registers(CPUState *cs)
                 DPRINTF("Warning: Unable to get VPA information from KVM\n");
             }
         }
+        kvm_get_one_reg(cs, KVM_REG_PPC_ARCH_COMPAT, &env->arch_compat);
 #endif
     }
 
diff --git a/vl.c b/vl.c
index 4e709d5..90dad7b 100644
--- a/vl.c
+++ b/vl.c
@@ -427,6 +427,10 @@ static QemuOptsList qemu_machine_opts = {
             .name = "usb",
             .type = QEMU_OPT_BOOL,
             .help = "Set on/off to enable/disable usb",
+        }, {
+            .name = "compat",
+            .type = QEMU_OPT_NUMBER,
+            .help = "Selects compatibility mode on CPU",
         },
         { /* End of list */ }
     },
-- 
1.8.4.rc4

Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option

[Alexander Graf]

On 27.09.2013, at 10:06, Alexey Kardashevskiy wrote:

> To be able to boot on newer hardware that the software support,
> PowerISA defines a logical PVR, one per every PowerISA specification
> version from 2.04.
> 
> This adds the "compat" option which takes values 205 or 206 and forces
> QEMU to boot the guest with a logical PVR (CPU_POWERPC_LOGICAL_2_05 or
> CPU_POWERPC_LOGICAL_2_06).
> 
> The guest reads the logical PVR value from "cpu-version" property of
> a CPU device node.
> 
> Cc: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> Cc: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> hw/ppc/spapr.c          | 40 ++++++++++++++++++++++++++++++++++++++++
> include/hw/ppc/spapr.h  |  2 ++
> target-ppc/cpu-models.h | 10 ++++++++++
> target-ppc/cpu.h        |  3 +++
> target-ppc/kvm.c        |  2 ++
> vl.c                    |  4 ++++
> 6 files changed, 61 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a09a1d9..737452d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -33,6 +33,7 @@
> #include "sysemu/kvm.h"
> #include "kvm_ppc.h"
> #include "mmu-hash64.h"
> +#include "cpu-models.h"
> 
> #include "hw/boards.h"
> #include "hw/ppc/ppc.h"
> @@ -196,6 +197,26 @@ static XICSState *xics_system_init(int nr_servers, int nr_irqs)
>     return icp;
> }
> 
> +static void spapr_compat_mode_init(sPAPREnvironment *spapr)
> +{
> +    QemuOpts *machine_opts = qemu_get_machine_opts();
> +    uint64_t compat = qemu_opt_get_number(machine_opts, "compat", 0);
> +
> +    switch (compat) {
> +    case 0:
> +        break;
> +    case 205:
> +        spapr->arch_compat = CPU_POWERPC_LOGICAL_2_05;
> +        break;
> +    case 206:
> +        spapr->arch_compat = CPU_POWERPC_LOGICAL_2_06;

Does it make sense to declare compat mode a number or would a string be easier for users? I can imagine that "-machine compat=power6" is easier to understand for a user than "-machine compat=205".

> +        break;
> +    default:
> +        perror("Unsupported mode, only are 205, 206 supported\n");
> +        break;
> +    }
> +}
> +
> static int spapr_fixup_cpu_dt(void *fdt, sPAPREnvironment *spapr)
> {
>     int ret = 0, offset;
> @@ -206,6 +227,7 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPREnvironment *spapr)
> 
>     CPU_FOREACH(cpu) {
>         DeviceClass *dc = DEVICE_GET_CLASS(cpu);
> +        CPUPPCState *env = &(POWERPC_CPU(cpu)->env);
>         uint32_t associativity[] = {cpu_to_be32(0x5),
>                                     cpu_to_be32(0x0),
>                                     cpu_to_be32(0x0),
> @@ -238,6 +260,14 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPREnvironment *spapr)
>         if (ret < 0) {
>             return ret;
>         }
> +
> +        if (env->arch_compat) {
> +            ret = fdt_setprop(fdt, offset, "cpu-version",
> +                              &env->arch_compat, sizeof(env->arch_compat));
> +            if (ret < 0) {
> +                return ret;
> +            }
> +        }
>     }
>     return ret;
> }
> @@ -1145,6 +1175,8 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>     spapr = g_malloc0(sizeof(*spapr));
>     QLIST_INIT(&spapr->phbs);
> 
> +    spapr_compat_mode_init(spapr);
> +
>     cpu_ppc_hypercall = emulate_spapr_hypercall;
> 
>     /* Allocate RMA if necessary */
> @@ -1226,6 +1258,14 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
> 
>         xics_cpu_setup(spapr->icp, cpu);
> 
> +        /*
> +         * If compat mode is set in the command line, pass it to CPU so KVM
> +         * will be able to set it in the host kernel.
> +         */
> +        if (spapr->arch_compat) {
> +            env->arch_compat = spapr->arch_compat;

You should set the compat mode in KVM here, rather than doing it in the put_registers call which gets invoked on every register sync. Or can the guest change the mode?

Also, we need to handle failure. If the kernel can not set the CPU to 2.05 mode for example (IIRC POWER8 doesn't allow you to) we should bail out here.

And then there's the TCG question. We either have to disable CPU features similar to how we handle it in KVM (by setting and honoring the respective bits in PCR) or we need to bail out too and declare compat mode unsupported for TCG.

And then there's the fact that the kernel interface isn't upstream in a way that

> +        }
> +
>         qemu_register_reset(spapr_cpu_reset, cpu);
>     }
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index ca175b0..201c578 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -34,6 +34,8 @@ typedef struct sPAPREnvironment {
>     uint32_t epow_irq;
>     Notifier epow_notifier;
> 
> +    uint32_t arch_compat;        /* Compatible PVR from the command line */
> +
>     /* Migration state */
>     int htab_save_index;
>     bool htab_first_pass;
> diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
> index 49ba4a4..d7c033c 100644
> --- a/target-ppc/cpu-models.h
> +++ b/target-ppc/cpu-models.h
> @@ -583,6 +583,16 @@ enum {
>     CPU_POWERPC_RS64II             = 0x00340000,
>     CPU_POWERPC_RS64III            = 0x00360000,
>     CPU_POWERPC_RS64IV             = 0x00370000,
> +
> +    /* Logical CPUs */
> +    CPU_POWERPC_LOGICAL_MASK       = 0xFFFFFFFF,
> +    CPU_POWERPC_LOGICAL_2_04       = 0x0F000001,
> +    CPU_POWERPC_LOGICAL_2_05       = 0x0F000002,
> +    CPU_POWERPC_LOGICAL_2_06       = 0x0F000003,
> +    CPU_POWERPC_LOGICAL_2_06_PLUS  = 0x0F100003,
> +    CPU_POWERPC_LOGICAL_2_07       = 0x0F000004,
> +    CPU_POWERPC_LOGICAL_2_08       = 0x0F000005,

These don't really belong here.

> +
> #endif /* defined(TARGET_PPC64) */
>     /* Original POWER */
>     /* XXX: should be POWER (RIOS), RSC3308, RSC4608,
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 422a6bb..fc837c1 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -999,6 +999,9 @@ struct CPUPPCState {
>     /* Device control registers */
>     ppc_dcr_t *dcr_env;
> 
> +    /* Architecture compatibility mode */
> +    uint32_t arch_compat;

Do we really need to carry this in the vcpu struct? Or can we just fire-and-forget about it? If we want to preserve anything, it should be the PCR register.


Alex

> +
>     int dcache_line_size;
>     int icache_line_size;
> 
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 4bc4496..7b853a3 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -872,6 +872,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>                 DPRINTF("Warning: Unable to set VPA information to KVM\n");
>             }
>         }
> +        kvm_set_one_reg(cs, KVM_REG_PPC_ARCH_COMPAT, &env->arch_compat);
> #endif /* TARGET_PPC64 */
>     }
> 
> @@ -1083,6 +1084,7 @@ int kvm_arch_get_registers(CPUState *cs)
>                 DPRINTF("Warning: Unable to get VPA information from KVM\n");
>             }
>         }
> +        kvm_get_one_reg(cs, KVM_REG_PPC_ARCH_COMPAT, &env->arch_compat);
> #endif
>     }
> 
> diff --git a/vl.c b/vl.c
> index 4e709d5..90dad7b 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -427,6 +427,10 @@ static QemuOptsList qemu_machine_opts = {
>             .name = "usb",
>             .type = QEMU_OPT_BOOL,
>             .help = "Set on/off to enable/disable usb",
> +        }, {
> +            .name = "compat",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "Selects compatibility mode on CPU",
>         },
>         { /* End of list */ }
>     },
> -- 
> 1.8.4.rc4
> 

Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option

[Paolo Bonzini]

Il 30/09/2013 13:25, Alexander Graf ha scritto:
> On 27.09.2013, at 10:06, Alexey Kardashevskiy wrote:
> 
>> > To be able to boot on newer hardware that the software support,
>> > PowerISA defines a logical PVR, one per every PowerISA specification
>> > version from 2.04.
>> > 
>> > This adds the "compat" option which takes values 205 or 206 and forces
>> > QEMU to boot the guest with a logical PVR (CPU_POWERPC_LOGICAL_2_05 or
>> > CPU_POWERPC_LOGICAL_2_06).
>> > 
>> > The guest reads the logical PVR value from "cpu-version" property of
>> > a CPU device node.
>> > 

Why is the option under -machine instead of -cpu?

Paolo

Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option

[Alexey Kardashevskiy]

On 30.09.2013 21:52, Paolo Bonzini wrote:
> Il 30/09/2013 13:25, Alexander Graf ha scritto:
>> On 27.09.2013, at 10:06, Alexey Kardashevskiy wrote:
>>
>>>> To be able to boot on newer hardware that the software support,
>>>> PowerISA defines a logical PVR, one per every PowerISA specification
>>>> version from 2.04.
>>>>
>>>> This adds the "compat" option which takes values 205 or 206 and forces
>>>> QEMU to boot the guest with a logical PVR (CPU_POWERPC_LOGICAL_2_05 or
>>>> CPU_POWERPC_LOGICAL_2_06).
>>>>
>>>> The guest reads the logical PVR value from "cpu-version" property of
>>>> a CPU device node.
>>>>
> 
> Why is the option under -machine instead of -cpu?

Because it is still the same CPU and the guest will still read the real
PVR from the hardware (which it may not support but this is why we need
compatibility mode).

It is possible to run QEMU with -cpu POWER8 with some old distro which
does not know about power8, it will switch to power6-compat mode, the
the user does "yum update" (or analog) and then the new kernel (with
power8 support) will do H_CAS again and this time "raw" power8 should be
selected.


-- 
With best regards

Alexey Kardashevskiy -- icq: 52150396

Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option

[Alexey Kardashevskiy]

On 30.09.2013 21:25, Alexander Graf wrote:
> 
> On 27.09.2013, at 10:06, Alexey Kardashevskiy wrote:
> 
>> To be able to boot on newer hardware that the software support,
>> PowerISA defines a logical PVR, one per every PowerISA specification
>> version from 2.04.
>>
>> This adds the "compat" option which takes values 205 or 206 and forces
>> QEMU to boot the guest with a logical PVR (CPU_POWERPC_LOGICAL_2_05 or
>> CPU_POWERPC_LOGICAL_2_06).
>>
>> The guest reads the logical PVR value from "cpu-version" property of
>> a CPU device node.
>>
>> Cc: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>> Cc: Andreas Färber <afaerber@suse.de>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> hw/ppc/spapr.c          | 40 ++++++++++++++++++++++++++++++++++++++++
>> include/hw/ppc/spapr.h  |  2 ++
>> target-ppc/cpu-models.h | 10 ++++++++++
>> target-ppc/cpu.h        |  3 +++
>> target-ppc/kvm.c        |  2 ++
>> vl.c                    |  4 ++++
>> 6 files changed, 61 insertions(+)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index a09a1d9..737452d 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -33,6 +33,7 @@
>> #include "sysemu/kvm.h"
>> #include "kvm_ppc.h"
>> #include "mmu-hash64.h"
>> +#include "cpu-models.h"
>>
>> #include "hw/boards.h"
>> #include "hw/ppc/ppc.h"
>> @@ -196,6 +197,26 @@ static XICSState *xics_system_init(int nr_servers, int nr_irqs)
>>     return icp;
>> }
>>
>> +static void spapr_compat_mode_init(sPAPREnvironment *spapr)
>> +{
>> +    QemuOpts *machine_opts = qemu_get_machine_opts();
>> +    uint64_t compat = qemu_opt_get_number(machine_opts, "compat", 0);
>> +
>> +    switch (compat) {
>> +    case 0:
>> +        break;
>> +    case 205:
>> +        spapr->arch_compat = CPU_POWERPC_LOGICAL_2_05;
>> +        break;
>> +    case 206:
>> +        spapr->arch_compat = CPU_POWERPC_LOGICAL_2_06;
> 

> Does it make sense to declare compat mode a number or would a string
> be easier for users? I can imagine that "-machine compat=power6" is
> easier to understand for a user than "-machine compat=205".

I just follow the PowerISA spec. It does not say anywhere (at least I do
not see it) that 2.05==power6. 2.05 was released when power6 was
released and power6 supports 2.05 but these are not synonims. And
"compat=power6" would not set "cpu-version" to any of power6 PVRs, it
still will be a logical PVR. It confuses me too to tell qemu "205"
instead of "power6" but it is the spec to blame :)



>> +        break;
>> +    default:
>> +        perror("Unsupported mode, only are 205, 206 supported\n");
>> +        break;
>> +    }
>> +}
>> +
>> static int spapr_fixup_cpu_dt(void *fdt, sPAPREnvironment *spapr)
>> {
>>     int ret = 0, offset;
>> @@ -206,6 +227,7 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPREnvironment *spapr)
>>
>>     CPU_FOREACH(cpu) {
>>         DeviceClass *dc = DEVICE_GET_CLASS(cpu);
>> +        CPUPPCState *env = &(POWERPC_CPU(cpu)->env);
>>         uint32_t associativity[] = {cpu_to_be32(0x5),
>>                                     cpu_to_be32(0x0),
>>                                     cpu_to_be32(0x0),
>> @@ -238,6 +260,14 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPREnvironment *spapr)
>>         if (ret < 0) {
>>             return ret;
>>         }
>> +
>> +        if (env->arch_compat) {
>> +            ret = fdt_setprop(fdt, offset, "cpu-version",
>> +                              &env->arch_compat, sizeof(env->arch_compat));
>> +            if (ret < 0) {
>> +                return ret;
>> +            }
>> +        }
>>     }
>>     return ret;
>> }
>> @@ -1145,6 +1175,8 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>>     spapr = g_malloc0(sizeof(*spapr));
>>     QLIST_INIT(&spapr->phbs);
>>
>> +    spapr_compat_mode_init(spapr);
>> +
>>     cpu_ppc_hypercall = emulate_spapr_hypercall;
>>
>>     /* Allocate RMA if necessary */
>> @@ -1226,6 +1258,14 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>>
>>         xics_cpu_setup(spapr->icp, cpu);
>>
>> +        /*
>> +         * If compat mode is set in the command line, pass it to CPU so KVM
>> +         * will be able to set it in the host kernel.
>> +         */
>> +        if (spapr->arch_compat) {
>> +            env->arch_compat = spapr->arch_compat;
> 

> You should set the compat mode in KVM here, rather than doing it in
the put_registers call which gets invoked on every register sync. Or can
the guest change the mode?


I will change it here in the next patch (which requires kernel changes
which are not there yet). The guest cannot change it directly but it can
indirectly via "client-architecture-support".


> Also, we need to handle failure. If the kernel can not set the CPU
> to
2.05 mode for example (IIRC POWER8 doesn't allow you to) we should bail
out here.

Yep, I'll add this easy check :)

> And then there's the TCG question. We either have to disable CPU
features similar to how we handle it in KVM (by setting and honoring the
respective bits in PCR) or we need to bail out too and declare compat
mode unsupported for TCG.

At the moment we want to run old distro on new CPUs. This patch would
make more sense with the "ibm,client-architecture-support" and "power8
registers migration" patches which I did not post yet.

Disabling "unsupported" bits in TCG would be really nice indeed and we
should eventually do this but 1) it will not really hurt the guest if we
did not disable some feature (really old guest will not use it and
that's it)  2) once created, PowerPCCPUState (or whatever the exact name
is, I am writing this mail on windows machine :) ) is not very flexible
to reconfigure...


> And then there's the fact that the kernel interface isn't upstream in a way that

? unfinished sentence? What is missing in the kernel right now?


>> +        }
>> +
>>         qemu_register_reset(spapr_cpu_reset, cpu);
>>     }
>>
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index ca175b0..201c578 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -34,6 +34,8 @@ typedef struct sPAPREnvironment {
>>     uint32_t epow_irq;
>>     Notifier epow_notifier;
>>
>> +    uint32_t arch_compat;        /* Compatible PVR from the command line */
>> +
>>     /* Migration state */
>>     int htab_save_index;
>>     bool htab_first_pass;
>> diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
>> index 49ba4a4..d7c033c 100644
>> --- a/target-ppc/cpu-models.h
>> +++ b/target-ppc/cpu-models.h
>> @@ -583,6 +583,16 @@ enum {
>>     CPU_POWERPC_RS64II             = 0x00340000,
>>     CPU_POWERPC_RS64III            = 0x00360000,
>>     CPU_POWERPC_RS64IV             = 0x00370000,
>> +
>> +    /* Logical CPUs */
>> +    CPU_POWERPC_LOGICAL_MASK       = 0xFFFFFFFF,
>> +    CPU_POWERPC_LOGICAL_2_04       = 0x0F000001,
>> +    CPU_POWERPC_LOGICAL_2_05       = 0x0F000002,
>> +    CPU_POWERPC_LOGICAL_2_06       = 0x0F000003,
>> +    CPU_POWERPC_LOGICAL_2_06_PLUS  = 0x0F100003,
>> +    CPU_POWERPC_LOGICAL_2_07       = 0x0F000004,
>> +    CPU_POWERPC_LOGICAL_2_08       = 0x0F000005,
> 
> These don't really belong here.


Sorry, I do not understand. These are PVRs which are used in
"cpu-version" DT property. They are logical PVRs but still PVRs - i.e.
the guest has PVR masks for them too.


>> +
>> #endif /* defined(TARGET_PPC64) */
>>     /* Original POWER */
>>     /* XXX: should be POWER (RIOS), RSC3308, RSC4608,
>> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
>> index 422a6bb..fc837c1 100644
>> --- a/target-ppc/cpu.h
>> +++ b/target-ppc/cpu.h
>> @@ -999,6 +999,9 @@ struct CPUPPCState {
>>     /* Device control registers */
>>     ppc_dcr_t *dcr_env;
>>
>> +    /* Architecture compatibility mode */
>> +    uint32_t arch_compat;
> 


> Do we really need to carry this in the vcpu struct? Or can we just
> fire-and-forget about it? If we want to preserve anything, it should
> be the PCR register.

This is the current PVR value aka "cpu-version" property. It may change
during reboots as every reboot does the "client-architecture-support"
call so the logical PVR can change.


> 
> 
> Alex
> 
>> +
>>     int dcache_line_size;
>>     int icache_line_size;
>>
>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>> index 4bc4496..7b853a3 100644
>> --- a/target-ppc/kvm.c
>> +++ b/target-ppc/kvm.c
>> @@ -872,6 +872,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>>                 DPRINTF("Warning: Unable to set VPA information to KVM\n");
>>             }
>>         }
>> +        kvm_set_one_reg(cs, KVM_REG_PPC_ARCH_COMPAT, &env->arch_compat);
>> #endif /* TARGET_PPC64 */
>>     }
>>
>> @@ -1083,6 +1084,7 @@ int kvm_arch_get_registers(CPUState *cs)
>>                 DPRINTF("Warning: Unable to get VPA information from KVM\n");
>>             }
>>         }
>> +        kvm_get_one_reg(cs, KVM_REG_PPC_ARCH_COMPAT, &env->arch_compat);
>> #endif
>>     }
>>
>> diff --git a/vl.c b/vl.c
>> index 4e709d5..90dad7b 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -427,6 +427,10 @@ static QemuOptsList qemu_machine_opts = {
>>             .name = "usb",
>>             .type = QEMU_OPT_BOOL,
>>             .help = "Set on/off to enable/disable usb",
>> +        }, {
>> +            .name = "compat",
>> +            .type = QEMU_OPT_NUMBER,
>> +            .help = "Selects compatibility mode on CPU",
>>         },
>>         { /* End of list */ }
>>     },
>> -- 
>> 1.8.4.rc4
>>
> 


-- 
With best regards

Alexey Kardashevskiy -- icq: 52150396

Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option

[Alexander Graf]

On 09/30/2013 03:22 PM, Alexey Kardashevskiy wrote:
> On 30.09.2013 21:25, Alexander Graf wrote:
>> On 27.09.2013, at 10:06, Alexey Kardashevskiy wrote:
>>
>>> To be able to boot on newer hardware that the software support,
>>> PowerISA defines a logical PVR, one per every PowerISA specification
>>> version from 2.04.
>>>
>>> This adds the "compat" option which takes values 205 or 206 and forces
>>> QEMU to boot the guest with a logical PVR (CPU_POWERPC_LOGICAL_2_05 or
>>> CPU_POWERPC_LOGICAL_2_06).
>>>
>>> The guest reads the logical PVR value from "cpu-version" property of
>>> a CPU device node.
>>>
>>> Cc: Nikunj A Dadhania<nikunj@linux.vnet.ibm.com>
>>> Cc: Andreas Färber<afaerber@suse.de>
>>> Signed-off-by: Alexey Kardashevskiy<aik@ozlabs.ru>
>>> ---
>>> hw/ppc/spapr.c          | 40 ++++++++++++++++++++++++++++++++++++++++
>>> include/hw/ppc/spapr.h  |  2 ++
>>> target-ppc/cpu-models.h | 10 ++++++++++
>>> target-ppc/cpu.h        |  3 +++
>>> target-ppc/kvm.c        |  2 ++
>>> vl.c                    |  4 ++++
>>> 6 files changed, 61 insertions(+)
>>>
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index a09a1d9..737452d 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -33,6 +33,7 @@
>>> #include "sysemu/kvm.h"
>>> #include "kvm_ppc.h"
>>> #include "mmu-hash64.h"
>>> +#include "cpu-models.h"
>>>
>>> #include "hw/boards.h"
>>> #include "hw/ppc/ppc.h"
>>> @@ -196,6 +197,26 @@ static XICSState *xics_system_init(int nr_servers, int nr_irqs)
>>>      return icp;
>>> }
>>>
>>> +static void spapr_compat_mode_init(sPAPREnvironment *spapr)
>>> +{
>>> +    QemuOpts *machine_opts = qemu_get_machine_opts();
>>> +    uint64_t compat = qemu_opt_get_number(machine_opts, "compat", 0);
>>> +
>>> +    switch (compat) {
>>> +    case 0:
>>> +        break;
>>> +    case 205:
>>> +        spapr->arch_compat = CPU_POWERPC_LOGICAL_2_05;
>>> +        break;
>>> +    case 206:
>>> +        spapr->arch_compat = CPU_POWERPC_LOGICAL_2_06;
>> Does it make sense to declare compat mode a number or would a string
>> be easier for users? I can imagine that "-machine compat=power6" is
>> easier to understand for a user than "-machine compat=205".
> I just follow the PowerISA spec. It does not say anywhere (at least I do
> not see it) that 2.05==power6. 2.05 was released when power6 was
> released and power6 supports 2.05 but these are not synonims. And
> "compat=power6" would not set "cpu-version" to any of power6 PVRs, it
> still will be a logical PVR. It confuses me too to tell qemu "205"
> instead of "power6" but it is the spec to blame :)

So what is "2_06 plus" then? :)

To me it really sounds like a 1:1 mapping to cores rather than specs - 
the ISA defines a lot more capabilities than a single core necessarily 
supports, especially with the inclusion of booke into the generic ppc spec.

>
>
>
>>> +        break;
>>> +    default:
>>> +        perror("Unsupported mode, only are 205, 206 supported\n");
>>> +        break;
>>> +    }
>>> +}
>>> +
>>> static int spapr_fixup_cpu_dt(void *fdt, sPAPREnvironment *spapr)
>>> {
>>>      int ret = 0, offset;
>>> @@ -206,6 +227,7 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPREnvironment *spapr)
>>>
>>>      CPU_FOREACH(cpu) {
>>>          DeviceClass *dc = DEVICE_GET_CLASS(cpu);
>>> +        CPUPPCState *env =&(POWERPC_CPU(cpu)->env);
>>>          uint32_t associativity[] = {cpu_to_be32(0x5),
>>>                                      cpu_to_be32(0x0),
>>>                                      cpu_to_be32(0x0),
>>> @@ -238,6 +260,14 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPREnvironment *spapr)
>>>          if (ret<  0) {
>>>              return ret;
>>>          }
>>> +
>>> +        if (env->arch_compat) {
>>> +            ret = fdt_setprop(fdt, offset, "cpu-version",
>>> +&env->arch_compat, sizeof(env->arch_compat));
>>> +            if (ret<  0) {
>>> +                return ret;
>>> +            }
>>> +        }
>>>      }
>>>      return ret;
>>> }
>>> @@ -1145,6 +1175,8 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>>>      spapr = g_malloc0(sizeof(*spapr));
>>>      QLIST_INIT(&spapr->phbs);
>>>
>>> +    spapr_compat_mode_init(spapr);
>>> +
>>>      cpu_ppc_hypercall = emulate_spapr_hypercall;
>>>
>>>      /* Allocate RMA if necessary */
>>> @@ -1226,6 +1258,14 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>>>
>>>          xics_cpu_setup(spapr->icp, cpu);
>>>
>>> +        /*
>>> +         * If compat mode is set in the command line, pass it to CPU so KVM
>>> +         * will be able to set it in the host kernel.
>>> +         */
>>> +        if (spapr->arch_compat) {
>>> +            env->arch_compat = spapr->arch_compat;
>> You should set the compat mode in KVM here, rather than doing it in
> the put_registers call which gets invoked on every register sync. Or can
> the guest change the mode?
>
>
> I will change it here in the next patch (which requires kernel changes
> which are not there yet). The guest cannot change it directly but it can
> indirectly via "client-architecture-support".

They probably want a generic callback then. What happens on reset?

>
>
>> Also, we need to handle failure. If the kernel can not set the CPU
>> to
> 2.05 mode for example (IIRC POWER8 doesn't allow you to) we should bail
> out here.
>
> Yep, I'll add this easy check :)
>
>> And then there's the TCG question. We either have to disable CPU
> features similar to how we handle it in KVM (by setting and honoring the
> respective bits in PCR) or we need to bail out too and declare compat
> mode unsupported for TCG.
>
> At the moment we want to run old distro on new CPUs. This patch would
> make more sense with the "ibm,client-architecture-support" and "power8
> registers migration" patches which I did not post yet.
>
> Disabling "unsupported" bits in TCG would be really nice indeed and we
> should eventually do this but 1) it will not really hurt the guest if we
> did not disable some feature (really old guest will not use it and
> that's it)  2) once created, PowerPCCPUState (or whatever the exact name
> is, I am writing this mail on windows machine :) ) is not very flexible
> to reconfigure...

Can't you just set the bits in PCR and add an XXX comment indicating 
that we're currently not honoring them? Then fron the machine code point 
of view, everything is implemented.

>
>
>> And then there's the fact that the kernel interface isn't upstream in a way that
> ? unfinished sentence? What is missing in the kernel right now?

Eh. I think I wanted to say that this depends on in-kernel patches that 
are not upstream yet :).

>
>
>>> +        }
>>> +
>>>          qemu_register_reset(spapr_cpu_reset, cpu);
>>>      }
>>>
>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>>> index ca175b0..201c578 100644
>>> --- a/include/hw/ppc/spapr.h
>>> +++ b/include/hw/ppc/spapr.h
>>> @@ -34,6 +34,8 @@ typedef struct sPAPREnvironment {
>>>      uint32_t epow_irq;
>>>      Notifier epow_notifier;
>>>
>>> +    uint32_t arch_compat;        /* Compatible PVR from the command line */
>>> +
>>>      /* Migration state */
>>>      int htab_save_index;
>>>      bool htab_first_pass;
>>> diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
>>> index 49ba4a4..d7c033c 100644
>>> --- a/target-ppc/cpu-models.h
>>> +++ b/target-ppc/cpu-models.h
>>> @@ -583,6 +583,16 @@ enum {
>>>      CPU_POWERPC_RS64II             = 0x00340000,
>>>      CPU_POWERPC_RS64III            = 0x00360000,
>>>      CPU_POWERPC_RS64IV             = 0x00370000,
>>> +
>>> +    /* Logical CPUs */
>>> +    CPU_POWERPC_LOGICAL_MASK       = 0xFFFFFFFF,
>>> +    CPU_POWERPC_LOGICAL_2_04       = 0x0F000001,
>>> +    CPU_POWERPC_LOGICAL_2_05       = 0x0F000002,
>>> +    CPU_POWERPC_LOGICAL_2_06       = 0x0F000003,
>>> +    CPU_POWERPC_LOGICAL_2_06_PLUS  = 0x0F100003,
>>> +    CPU_POWERPC_LOGICAL_2_07       = 0x0F000004,
>>> +    CPU_POWERPC_LOGICAL_2_08       = 0x0F000005,
>> These don't really belong here.
>
> Sorry, I do not understand. These are PVRs which are used in
> "cpu-version" DT property. They are logical PVRs but still PVRs - i.e.
> the guest has PVR masks for them too.

But they are specific to PAPR, not PowerPC in general, no? And you would 
never find one in the PVR register of a guest.

>
>
>>> +
>>> #endif /* defined(TARGET_PPC64) */
>>>      /* Original POWER */
>>>      /* XXX: should be POWER (RIOS), RSC3308, RSC4608,
>>> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
>>> index 422a6bb..fc837c1 100644
>>> --- a/target-ppc/cpu.h
>>> +++ b/target-ppc/cpu.h
>>> @@ -999,6 +999,9 @@ struct CPUPPCState {
>>>      /* Device control registers */
>>>      ppc_dcr_t *dcr_env;
>>>
>>> +    /* Architecture compatibility mode */
>>> +    uint32_t arch_compat;
>
>> Do we really need to carry this in the vcpu struct? Or can we just
>> fire-and-forget about it? If we want to preserve anything, it should
>> be the PCR register.
> This is the current PVR value aka "cpu-version" property. It may change
> during reboots as every reboot does the "client-architecture-support"
> call so the logical PVR can change.

Ok, so again: What happens on reset / reboot? Do we want to preserve the 
compatibility setting or does that get reset as well?


Alex

Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option

[Alexey Kardashevskiy]

On 10/01/2013 12:49 AM, Alexander Graf wrote:
> On 09/30/2013 03:22 PM, Alexey Kardashevskiy wrote:
>> On 30.09.2013 21:25, Alexander Graf wrote:
>>> On 27.09.2013, at 10:06, Alexey Kardashevskiy wrote:


I realized it has been a while since I got your response and did not answer
:) Sorry for the delay.


>>>
>>>> To be able to boot on newer hardware that the software support,
>>>> PowerISA defines a logical PVR, one per every PowerISA specification
>>>> version from 2.04.
>>>>
>>>> This adds the "compat" option which takes values 205 or 206 and forces
>>>> QEMU to boot the guest with a logical PVR (CPU_POWERPC_LOGICAL_2_05 or
>>>> CPU_POWERPC_LOGICAL_2_06).
>>>>
>>>> The guest reads the logical PVR value from "cpu-version" property of
>>>> a CPU device node.
>>>>
>>>> Cc: Nikunj A Dadhania<nikunj@linux.vnet.ibm.com>
>>>> Cc: Andreas Färber<afaerber@suse.de>
>>>> Signed-off-by: Alexey Kardashevskiy<aik@ozlabs.ru>
>>>> ---
>>>> hw/ppc/spapr.c          | 40 ++++++++++++++++++++++++++++++++++++++++
>>>> include/hw/ppc/spapr.h  |  2 ++
>>>> target-ppc/cpu-models.h | 10 ++++++++++
>>>> target-ppc/cpu.h        |  3 +++
>>>> target-ppc/kvm.c        |  2 ++
>>>> vl.c                    |  4 ++++
>>>> 6 files changed, 61 insertions(+)
>>>>
>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>> index a09a1d9..737452d 100644
>>>> --- a/hw/ppc/spapr.c
>>>> +++ b/hw/ppc/spapr.c
>>>> @@ -33,6 +33,7 @@
>>>> #include "sysemu/kvm.h"
>>>> #include "kvm_ppc.h"
>>>> #include "mmu-hash64.h"
>>>> +#include "cpu-models.h"
>>>>
>>>> #include "hw/boards.h"
>>>> #include "hw/ppc/ppc.h"
>>>> @@ -196,6 +197,26 @@ static XICSState *xics_system_init(int nr_servers,
>>>> int nr_irqs)
>>>>      return icp;
>>>> }
>>>>
>>>> +static void spapr_compat_mode_init(sPAPREnvironment *spapr)
>>>> +{
>>>> +    QemuOpts *machine_opts = qemu_get_machine_opts();
>>>> +    uint64_t compat = qemu_opt_get_number(machine_opts, "compat", 0);
>>>> +
>>>> +    switch (compat) {
>>>> +    case 0:
>>>> +        break;
>>>> +    case 205:
>>>> +        spapr->arch_compat = CPU_POWERPC_LOGICAL_2_05;
>>>> +        break;
>>>> +    case 206:
>>>> +        spapr->arch_compat = CPU_POWERPC_LOGICAL_2_06;
>>> Does it make sense to declare compat mode a number or would a string
>>> be easier for users? I can imagine that "-machine compat=power6" is
>>> easier to understand for a user than "-machine compat=205".
>> I just follow the PowerISA spec. It does not say anywhere (at least I do
>> not see it) that 2.05==power6. 2.05 was released when power6 was
>> released and power6 supports 2.05 but these are not synonims. And
>> "compat=power6" would not set "cpu-version" to any of power6 PVRs, it
>> still will be a logical PVR. It confuses me too to tell qemu "205"
>> instead of "power6" but it is the spec to blame :)
> 
> So what is "2_06 plus" then? :)

No idea. Why does it matter here?


> To me it really sounds like a 1:1 mapping to cores rather than specs - the
> ISA defines a lot more capabilities than a single core necessarily
> supports, especially with the inclusion of booke into the generic ppc spec.


Sounds - may be. But still not the same. The guest kernel has different
descriptors for power6(raw) and power6(architected) with different flags
and (slightly?) different behavior.


>>>> +        break;
>>>> +    default:
>>>> +        perror("Unsupported mode, only are 205, 206 supported\n");
>>>> +        break;
>>>> +    }
>>>> +}
>>>> +
>>>> static int spapr_fixup_cpu_dt(void *fdt, sPAPREnvironment *spapr)
>>>> {
>>>>      int ret = 0, offset;
>>>> @@ -206,6 +227,7 @@ static int spapr_fixup_cpu_dt(void *fdt,
>>>> sPAPREnvironment *spapr)
>>>>
>>>>      CPU_FOREACH(cpu) {
>>>>          DeviceClass *dc = DEVICE_GET_CLASS(cpu);
>>>> +        CPUPPCState *env =&(POWERPC_CPU(cpu)->env);
>>>>          uint32_t associativity[] = {cpu_to_be32(0x5),
>>>>                                      cpu_to_be32(0x0),
>>>>                                      cpu_to_be32(0x0),
>>>> @@ -238,6 +260,14 @@ static int spapr_fixup_cpu_dt(void *fdt,
>>>> sPAPREnvironment *spapr)
>>>>          if (ret<  0) {
>>>>              return ret;
>>>>          }
>>>> +
>>>> +        if (env->arch_compat) {
>>>> +            ret = fdt_setprop(fdt, offset, "cpu-version",
>>>> +&env->arch_compat, sizeof(env->arch_compat));
>>>> +            if (ret<  0) {
>>>> +                return ret;
>>>> +            }
>>>> +        }
>>>>      }
>>>>      return ret;
>>>> }
>>>> @@ -1145,6 +1175,8 @@ static void ppc_spapr_init(QEMUMachineInitArgs
>>>> *args)
>>>>      spapr = g_malloc0(sizeof(*spapr));
>>>>      QLIST_INIT(&spapr->phbs);
>>>>
>>>> +    spapr_compat_mode_init(spapr);
>>>> +
>>>>      cpu_ppc_hypercall = emulate_spapr_hypercall;
>>>>
>>>>      /* Allocate RMA if necessary */
>>>> @@ -1226,6 +1258,14 @@ static void ppc_spapr_init(QEMUMachineInitArgs
>>>> *args)
>>>>
>>>>          xics_cpu_setup(spapr->icp, cpu);
>>>>
>>>> +        /*
>>>> +         * If compat mode is set in the command line, pass it to CPU
>>>> so KVM
>>>> +         * will be able to set it in the host kernel.
>>>> +         */
>>>> +        if (spapr->arch_compat) {
>>>> +            env->arch_compat = spapr->arch_compat;
>>> You should set the compat mode in KVM here, rather than doing it in
>> the put_registers call which gets invoked on every register sync. Or can
>> the guest change the mode?
>>
>>
>> I will change it here in the next patch (which requires kernel changes
>> which are not there yet). The guest cannot change it directly but it can
>> indirectly via "client-architecture-support".
> 
> They probably want a generic callback then.


"They"? What callback on what? :) PCR change is privileged instruction so
the guest is not supposed to change it directly and AFAIK (sorry for my
ignorance) "ibm,client-architecture-support" RTAS call is the only way to
set it and it is spapr-only. How do other PPC machines do that change?


> What happens on reset?


PCR has to be reset I believe.


>>> Also, we need to handle failure. If the kernel can not set the CPU
>>> to
>> 2.05 mode for example (IIRC POWER8 doesn't allow you to) we should bail
>> out here.
>>
>> Yep, I'll add this easy check :)
>>
>>> And then there's the TCG question. We either have to disable CPU
>> features similar to how we handle it in KVM (by setting and honoring the
>> respective bits in PCR) or we need to bail out too and declare compat
>> mode unsupported for TCG.
>>
>> At the moment we want to run old distro on new CPUs. This patch would
>> make more sense with the "ibm,client-architecture-support" and "power8
>> registers migration" patches which I did not post yet.
>>
>> Disabling "unsupported" bits in TCG would be really nice indeed and we
>> should eventually do this but 1) it will not really hurt the guest if we
>> did not disable some feature (really old guest will not use it and
>> that's it)  2) once created, PowerPCCPUState (or whatever the exact name
>> is, I am writing this mail on windows machine :) ) is not very flexible
>> to reconfigure...
> 
> Can't you just set the bits in PCR and add an XXX comment indicating that
> we're currently not honoring them? Then fron the machine code point of
> view, everything is implemented.


Is not it what the patch does at the moment to TCG (except missing comment)?



>>> And then there's the fact that the kernel interface isn't upstream in a
>>> way that
>> ? unfinished sentence? What is missing in the kernel right now?
> 
> Eh. I think I wanted to say that this depends on in-kernel patches that are
> not upstream yet :).
> 
>>
>>
>>>> +        }
>>>> +
>>>>          qemu_register_reset(spapr_cpu_reset, cpu);
>>>>      }
>>>>
>>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>>>> index ca175b0..201c578 100644
>>>> --- a/include/hw/ppc/spapr.h
>>>> +++ b/include/hw/ppc/spapr.h
>>>> @@ -34,6 +34,8 @@ typedef struct sPAPREnvironment {
>>>>      uint32_t epow_irq;
>>>>      Notifier epow_notifier;
>>>>
>>>> +    uint32_t arch_compat;        /* Compatible PVR from the command
>>>> line */
>>>> +
>>>>      /* Migration state */
>>>>      int htab_save_index;
>>>>      bool htab_first_pass;
>>>> diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
>>>> index 49ba4a4..d7c033c 100644
>>>> --- a/target-ppc/cpu-models.h
>>>> +++ b/target-ppc/cpu-models.h
>>>> @@ -583,6 +583,16 @@ enum {
>>>>      CPU_POWERPC_RS64II             = 0x00340000,
>>>>      CPU_POWERPC_RS64III            = 0x00360000,
>>>>      CPU_POWERPC_RS64IV             = 0x00370000,
>>>> +
>>>> +    /* Logical CPUs */
>>>> +    CPU_POWERPC_LOGICAL_MASK       = 0xFFFFFFFF,
>>>> +    CPU_POWERPC_LOGICAL_2_04       = 0x0F000001,
>>>> +    CPU_POWERPC_LOGICAL_2_05       = 0x0F000002,
>>>> +    CPU_POWERPC_LOGICAL_2_06       = 0x0F000003,
>>>> +    CPU_POWERPC_LOGICAL_2_06_PLUS  = 0x0F100003,
>>>> +    CPU_POWERPC_LOGICAL_2_07       = 0x0F000004,
>>>> +    CPU_POWERPC_LOGICAL_2_08       = 0x0F000005,
>>> These don't really belong here.
>>
>> Sorry, I do not understand. These are PVRs which are used in
>> "cpu-version" DT property. They are logical PVRs but still PVRs - i.e.
>> the guest has PVR masks for them too.
> 
> But they are specific to PAPR, not PowerPC in general, no? And you would
> never find one in the PVR register of a guest.


Yes, never. But they all are in the same array in
arch/powerpc/kernel/cputable.c. How is that different?


>>>> +
>>>> #endif /* defined(TARGET_PPC64) */
>>>>      /* Original POWER */
>>>>      /* XXX: should be POWER (RIOS), RSC3308, RSC4608,
>>>> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
>>>> index 422a6bb..fc837c1 100644
>>>> --- a/target-ppc/cpu.h
>>>> +++ b/target-ppc/cpu.h
>>>> @@ -999,6 +999,9 @@ struct CPUPPCState {
>>>>      /* Device control registers */
>>>>      ppc_dcr_t *dcr_env;
>>>>
>>>> +    /* Architecture compatibility mode */
>>>> +    uint32_t arch_compat;
>>
>>> Do we really need to carry this in the vcpu struct? Or can we just
>>> fire-and-forget about it? If we want to preserve anything, it should
>>> be the PCR register.
>> This is the current PVR value aka "cpu-version" property. It may change
>> during reboots as every reboot does the "client-architecture-support"
>> call so the logical PVR can change.
> 
> Ok, so again: What happens on reset / reboot? Do we want to preserve the
> compatibility setting or does that get reset as well?

The setting must be preserved on reboot ("client-architecture-support"
definitely expects it to be preserved). I am not sure about reset, I guess
it can/should be reset.


-- 
Alexey

Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option

[Paolo Bonzini]

Il 30/09/2013 14:57, Alexey Kardashevskiy ha scritto:
>> > Why is the option under -machine instead of -cpu?
> Because it is still the same CPU and the guest will still read the real
> PVR from the hardware (which it may not support but this is why we need
> compatibility mode).

How do you support migration from a newer to an older CPU then?  I think
the guest should never see anything about the hardware CPU model.

Paolo

> It is possible to run QEMU with -cpu POWER8 with some old distro which
> does not know about power8, it will switch to power6-compat mode, the
> the user does "yum update" (or analog) and then the new kernel (with
> power8 support) will do H_CAS again and this time "raw" power8 should be
> selected.

Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option

[Alexander Graf]

On 05.11.2013, at 10:06, Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 30/09/2013 14:57, Alexey Kardashevskiy ha scritto:
>>>> Why is the option under -machine instead of -cpu?
>> Because it is still the same CPU and the guest will still read the real
>> PVR from the hardware (which it may not support but this is why we need
>> compatibility mode).
> 
> How do you support migration from a newer to an older CPU then?  I think
> the guest should never see anything about the hardware CPU model.

POWER can't model that. It always leaks the host CPU information into the guest. It's the guest kernel's responsibility to not expose that change to user space.

Yes, it's broken :). I'm not even sure there is any sensible way to do live migration between different CPU types.


Alex

Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option

[Alexander Graf]

On 05.11.2013, at 03:19, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> On 10/01/2013 12:49 AM, Alexander Graf wrote:
>> On 09/30/2013 03:22 PM, Alexey Kardashevskiy wrote:
>>> On 30.09.2013 21:25, Alexander Graf wrote:
>>>> On 27.09.2013, at 10:06, Alexey Kardashevskiy wrote:
> 
> 
> I realized it has been a while since I got your response and did not answer
> :) Sorry for the delay.
> 
> 
>>>> 
>>>>> To be able to boot on newer hardware that the software support,
>>>>> PowerISA defines a logical PVR, one per every PowerISA specification
>>>>> version from 2.04.
>>>>> 
>>>>> This adds the "compat" option which takes values 205 or 206 and forces
>>>>> QEMU to boot the guest with a logical PVR (CPU_POWERPC_LOGICAL_2_05 or
>>>>> CPU_POWERPC_LOGICAL_2_06).
>>>>> 
>>>>> The guest reads the logical PVR value from "cpu-version" property of
>>>>> a CPU device node.
>>>>> 
>>>>> Cc: Nikunj A Dadhania<nikunj@linux.vnet.ibm.com>
>>>>> Cc: Andreas Färber<afaerber@suse.de>
>>>>> Signed-off-by: Alexey Kardashevskiy<aik@ozlabs.ru>
>>>>> ---
>>>>> hw/ppc/spapr.c          | 40 ++++++++++++++++++++++++++++++++++++++++
>>>>> include/hw/ppc/spapr.h  |  2 ++
>>>>> target-ppc/cpu-models.h | 10 ++++++++++
>>>>> target-ppc/cpu.h        |  3 +++
>>>>> target-ppc/kvm.c        |  2 ++
>>>>> vl.c                    |  4 ++++
>>>>> 6 files changed, 61 insertions(+)
>>>>> 
>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>>> index a09a1d9..737452d 100644
>>>>> --- a/hw/ppc/spapr.c
>>>>> +++ b/hw/ppc/spapr.c
>>>>> @@ -33,6 +33,7 @@
>>>>> #include "sysemu/kvm.h"
>>>>> #include "kvm_ppc.h"
>>>>> #include "mmu-hash64.h"
>>>>> +#include "cpu-models.h"
>>>>> 
>>>>> #include "hw/boards.h"
>>>>> #include "hw/ppc/ppc.h"
>>>>> @@ -196,6 +197,26 @@ static XICSState *xics_system_init(int nr_servers,
>>>>> int nr_irqs)
>>>>>     return icp;
>>>>> }
>>>>> 
>>>>> +static void spapr_compat_mode_init(sPAPREnvironment *spapr)
>>>>> +{
>>>>> +    QemuOpts *machine_opts = qemu_get_machine_opts();
>>>>> +    uint64_t compat = qemu_opt_get_number(machine_opts, "compat", 0);
>>>>> +
>>>>> +    switch (compat) {
>>>>> +    case 0:
>>>>> +        break;
>>>>> +    case 205:
>>>>> +        spapr->arch_compat = CPU_POWERPC_LOGICAL_2_05;
>>>>> +        break;
>>>>> +    case 206:
>>>>> +        spapr->arch_compat = CPU_POWERPC_LOGICAL_2_06;
>>>> Does it make sense to declare compat mode a number or would a string
>>>> be easier for users? I can imagine that "-machine compat=power6" is
>>>> easier to understand for a user than "-machine compat=205".
>>> I just follow the PowerISA spec. It does not say anywhere (at least I do
>>> not see it) that 2.05==power6. 2.05 was released when power6 was
>>> released and power6 supports 2.05 but these are not synonims. And
>>> "compat=power6" would not set "cpu-version" to any of power6 PVRs, it
>>> still will be a logical PVR. It confuses me too to tell qemu "205"
>>> instead of "power6" but it is the spec to blame :)
>> 
>> So what is "2_06 plus" then? :)
> 
> No idea. Why does it matter here?
> 
> 
>> To me it really sounds like a 1:1 mapping to cores rather than specs - the
>> ISA defines a lot more capabilities than a single core necessarily
>> supports, especially with the inclusion of booke into the generic ppc spec.
> 
> 
> Sounds - may be. But still not the same. The guest kernel has different
> descriptors for power6(raw) and power6(architected) with different flags
> and (slightly?) different behavior.

So even the guest kernel calls it "power6" then? Why shouldn't we?

> 
> 
>>>>> +        break;
>>>>> +    default:
>>>>> +        perror("Unsupported mode, only are 205, 206 supported\n");
>>>>> +        break;
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> static int spapr_fixup_cpu_dt(void *fdt, sPAPREnvironment *spapr)
>>>>> {
>>>>>     int ret = 0, offset;
>>>>> @@ -206,6 +227,7 @@ static int spapr_fixup_cpu_dt(void *fdt,
>>>>> sPAPREnvironment *spapr)
>>>>> 
>>>>>     CPU_FOREACH(cpu) {
>>>>>         DeviceClass *dc = DEVICE_GET_CLASS(cpu);
>>>>> +        CPUPPCState *env =&(POWERPC_CPU(cpu)->env);
>>>>>         uint32_t associativity[] = {cpu_to_be32(0x5),
>>>>>                                     cpu_to_be32(0x0),
>>>>>                                     cpu_to_be32(0x0),
>>>>> @@ -238,6 +260,14 @@ static int spapr_fixup_cpu_dt(void *fdt,
>>>>> sPAPREnvironment *spapr)
>>>>>         if (ret<  0) {
>>>>>             return ret;
>>>>>         }
>>>>> +
>>>>> +        if (env->arch_compat) {
>>>>> +            ret = fdt_setprop(fdt, offset, "cpu-version",
>>>>> +&env->arch_compat, sizeof(env->arch_compat));
>>>>> +            if (ret<  0) {
>>>>> +                return ret;
>>>>> +            }
>>>>> +        }
>>>>>     }
>>>>>     return ret;
>>>>> }
>>>>> @@ -1145,6 +1175,8 @@ static void ppc_spapr_init(QEMUMachineInitArgs
>>>>> *args)
>>>>>     spapr = g_malloc0(sizeof(*spapr));
>>>>>     QLIST_INIT(&spapr->phbs);
>>>>> 
>>>>> +    spapr_compat_mode_init(spapr);
>>>>> +
>>>>>     cpu_ppc_hypercall = emulate_spapr_hypercall;
>>>>> 
>>>>>     /* Allocate RMA if necessary */
>>>>> @@ -1226,6 +1258,14 @@ static void ppc_spapr_init(QEMUMachineInitArgs
>>>>> *args)
>>>>> 
>>>>>         xics_cpu_setup(spapr->icp, cpu);
>>>>> 
>>>>> +        /*
>>>>> +         * If compat mode is set in the command line, pass it to CPU
>>>>> so KVM
>>>>> +         * will be able to set it in the host kernel.
>>>>> +         */
>>>>> +        if (spapr->arch_compat) {
>>>>> +            env->arch_compat = spapr->arch_compat;
>>>> You should set the compat mode in KVM here, rather than doing it in
>>> the put_registers call which gets invoked on every register sync. Or can
>>> the guest change the mode?
>>> 
>>> 
>>> I will change it here in the next patch (which requires kernel changes
>>> which are not there yet). The guest cannot change it directly but it can
>>> indirectly via "client-architecture-support".
>> 
>> They probably want a generic callback then.
> 
> 
> "They"? What callback on what? :) PCR change is privileged instruction so
> the guest is not supposed to change it directly and AFAIK (sorry for my
> ignorance) "ibm,client-architecture-support" RTAS call is the only way to
> set it and it is spapr-only. How do other PPC machines do that change?

No idea what I was referring to here. Just call a generic helper that changes the value of PCR in the QEMU SPR array as well as KVM.

> 
>> What happens on reset?
> 
> 
> PCR has to be reset I believe.

That needs to be modeled manually somewhere. Please make sure to do this.

> 
> 
>>>> Also, we need to handle failure. If the kernel can not set the CPU
>>>> to
>>> 2.05 mode for example (IIRC POWER8 doesn't allow you to) we should bail
>>> out here.
>>> 
>>> Yep, I'll add this easy check :)
>>> 
>>>> And then there's the TCG question. We either have to disable CPU
>>> features similar to how we handle it in KVM (by setting and honoring the
>>> respective bits in PCR) or we need to bail out too and declare compat
>>> mode unsupported for TCG.
>>> 
>>> At the moment we want to run old distro on new CPUs. This patch would
>>> make more sense with the "ibm,client-architecture-support" and "power8
>>> registers migration" patches which I did not post yet.
>>> 
>>> Disabling "unsupported" bits in TCG would be really nice indeed and we
>>> should eventually do this but 1) it will not really hurt the guest if we
>>> did not disable some feature (really old guest will not use it and
>>> that's it)  2) once created, PowerPCCPUState (or whatever the exact name
>>> is, I am writing this mail on windows machine :) ) is not very flexible
>>> to reconfigure...
>> 
>> Can't you just set the bits in PCR and add an XXX comment indicating that
>> we're currently not honoring them? Then fron the machine code point of
>> view, everything is implemented.
> 
> 
> Is not it what the patch does at the moment to TCG (except missing comment)?

You're modifying env->arch_compat. You should be modifying env->sprs[SPRN_PCR].

> 
> 
> 
>>>> And then there's the fact that the kernel interface isn't upstream in a
>>>> way that
>>> ? unfinished sentence? What is missing in the kernel right now?
>> 
>> Eh. I think I wanted to say that this depends on in-kernel patches that are
>> not upstream yet :).
>> 
>>> 
>>> 
>>>>> +        }
>>>>> +
>>>>>         qemu_register_reset(spapr_cpu_reset, cpu);
>>>>>     }
>>>>> 
>>>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>>>>> index ca175b0..201c578 100644
>>>>> --- a/include/hw/ppc/spapr.h
>>>>> +++ b/include/hw/ppc/spapr.h
>>>>> @@ -34,6 +34,8 @@ typedef struct sPAPREnvironment {
>>>>>     uint32_t epow_irq;
>>>>>     Notifier epow_notifier;
>>>>> 
>>>>> +    uint32_t arch_compat;        /* Compatible PVR from the command
>>>>> line */
>>>>> +
>>>>>     /* Migration state */
>>>>>     int htab_save_index;
>>>>>     bool htab_first_pass;
>>>>> diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
>>>>> index 49ba4a4..d7c033c 100644
>>>>> --- a/target-ppc/cpu-models.h
>>>>> +++ b/target-ppc/cpu-models.h
>>>>> @@ -583,6 +583,16 @@ enum {
>>>>>     CPU_POWERPC_RS64II             = 0x00340000,
>>>>>     CPU_POWERPC_RS64III            = 0x00360000,
>>>>>     CPU_POWERPC_RS64IV             = 0x00370000,
>>>>> +
>>>>> +    /* Logical CPUs */
>>>>> +    CPU_POWERPC_LOGICAL_MASK       = 0xFFFFFFFF,
>>>>> +    CPU_POWERPC_LOGICAL_2_04       = 0x0F000001,
>>>>> +    CPU_POWERPC_LOGICAL_2_05       = 0x0F000002,
>>>>> +    CPU_POWERPC_LOGICAL_2_06       = 0x0F000003,
>>>>> +    CPU_POWERPC_LOGICAL_2_06_PLUS  = 0x0F100003,
>>>>> +    CPU_POWERPC_LOGICAL_2_07       = 0x0F000004,
>>>>> +    CPU_POWERPC_LOGICAL_2_08       = 0x0F000005,
>>>> These don't really belong here.
>>> 
>>> Sorry, I do not understand. These are PVRs which are used in
>>> "cpu-version" DT property. They are logical PVRs but still PVRs - i.e.
>>> the guest has PVR masks for them too.
>> 
>> But they are specific to PAPR, not PowerPC in general, no? And you would
>> never find one in the PVR register of a guest.
> 
> 
> Yes, never. But they all are in the same array in
> arch/powerpc/kernel/cputable.c. How is that different?

They are not CPUs. That's what's different. These numbers don't exist on any real silicon and will never get returned my mfpvr.

> 
> 
>>>>> +
>>>>> #endif /* defined(TARGET_PPC64) */
>>>>>     /* Original POWER */
>>>>>     /* XXX: should be POWER (RIOS), RSC3308, RSC4608,
>>>>> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
>>>>> index 422a6bb..fc837c1 100644
>>>>> --- a/target-ppc/cpu.h
>>>>> +++ b/target-ppc/cpu.h
>>>>> @@ -999,6 +999,9 @@ struct CPUPPCState {
>>>>>     /* Device control registers */
>>>>>     ppc_dcr_t *dcr_env;
>>>>> 
>>>>> +    /* Architecture compatibility mode */
>>>>> +    uint32_t arch_compat;
>>> 
>>>> Do we really need to carry this in the vcpu struct? Or can we just
>>>> fire-and-forget about it? If we want to preserve anything, it should
>>>> be the PCR register.
>>> This is the current PVR value aka "cpu-version" property. It may change
>>> during reboots as every reboot does the "client-architecture-support"
>>> call so the logical PVR can change.
>> 
>> Ok, so again: What happens on reset / reboot? Do we want to preserve the
>> compatibility setting or does that get reset as well?
> 
> The setting must be preserved on reboot ("client-architecture-support"
> definitely expects it to be preserved). I am not sure about reset, I guess
> it can/should be reset.

Please find out for sure. It's critical we get this right and model it accordingly.


Alex

Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option

[Paolo Bonzini]

Il 05/11/2013 10:16, Alexander Graf ha scritto:
> 
> On 05.11.2013, at 10:06, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> Il 30/09/2013 14:57, Alexey Kardashevskiy ha scritto:
>>>>> Why is the option under -machine instead of -cpu?
>>> Because it is still the same CPU and the guest will still read the real
>>> PVR from the hardware (which it may not support but this is why we need
>>> compatibility mode).
>>
>> How do you support migration from a newer to an older CPU then?  I think
>> the guest should never see anything about the hardware CPU model.
> 
> POWER can't model that. It always leaks the host CPU information into the guest. It's the guest kernel's responsibility to not expose that change to user space.
> 
> Yes, it's broken :). I'm not even sure there is any sensible way to do live migration between different CPU types.

Still in my opinion it should be "-cpu", not "-machine".  Even if it's
just a "virtual" CPU model.

Paolo

Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option

[Alexander Graf]

On 05.11.2013, at 10:52, Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 05/11/2013 10:16, Alexander Graf ha scritto:
>> 
>> On 05.11.2013, at 10:06, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> 
>>> Il 30/09/2013 14:57, Alexey Kardashevskiy ha scritto:
>>>>>> Why is the option under -machine instead of -cpu?
>>>> Because it is still the same CPU and the guest will still read the real
>>>> PVR from the hardware (which it may not support but this is why we need
>>>> compatibility mode).
>>> 
>>> How do you support migration from a newer to an older CPU then?  I think
>>> the guest should never see anything about the hardware CPU model.
>> 
>> POWER can't model that. It always leaks the host CPU information into the guest. It's the guest kernel's responsibility to not expose that change to user space.
>> 
>> Yes, it's broken :). I'm not even sure there is any sensible way to do live migration between different CPU types.
> 
> Still in my opinion it should be "-cpu", not "-machine".  Even if it's
> just a "virtual" CPU model.

The only thing that this really changes is an SPR (MSR in x86 speech) on an existing cpu model. It's definitely not a new CPU type. If anything it'd be an option to an existing type.


Alex

Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option

[Paolo Bonzini]

Il 05/11/2013 11:27, Alexander Graf ha scritto:
> 
> On 05.11.2013, at 10:52, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> Il 05/11/2013 10:16, Alexander Graf ha scritto:
>>>
>>> On 05.11.2013, at 10:06, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>
>>>> Il 30/09/2013 14:57, Alexey Kardashevskiy ha scritto:
>>>>>>> Why is the option under -machine instead of -cpu?
>>>>> Because it is still the same CPU and the guest will still read the real
>>>>> PVR from the hardware (which it may not support but this is why we need
>>>>> compatibility mode).
>>>>
>>>> How do you support migration from a newer to an older CPU then?  I think
>>>> the guest should never see anything about the hardware CPU model.
>>>
>>> POWER can't model that. It always leaks the host CPU information into the guest. It's the guest kernel's responsibility to not expose that change to user space.
>>>
>>> Yes, it's broken :). I'm not even sure there is any sensible way to do live migration between different CPU types.
>>
>> Still in my opinion it should be "-cpu", not "-machine".  Even if it's
>> just a "virtual" CPU model.
> 
> The only thing that this really changes is an SPR (MSR in x86 speech)
> on an existing cpu model. It's definitely not a new CPU type. If
> anything it'd be an option to an existing type.

Agreed.

Paolo

Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option

[Alexey Kardashevskiy]

On 11/05/2013 08:52 PM, Paolo Bonzini wrote:
> Il 05/11/2013 10:16, Alexander Graf ha scritto:
>>
>> On 05.11.2013, at 10:06, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>> Il 30/09/2013 14:57, Alexey Kardashevskiy ha scritto:
>>>>>> Why is the option under -machine instead of -cpu?
>>>> Because it is still the same CPU and the guest will still read the real
>>>> PVR from the hardware (which it may not support but this is why we need
>>>> compatibility mode).
>>>
>>> How do you support migration from a newer to an older CPU then?  I think
>>> the guest should never see anything about the hardware CPU model.
>>
>> POWER can't model that. It always leaks the host CPU information into the guest. It's the guest kernel's responsibility to not expose that change to user space.
>>
>> Yes, it's broken :). I'm not even sure there is any sensible way to do live migration between different CPU types.
> 
> Still in my opinion it should be "-cpu", not "-machine".  Even if it's
> just a "virtual" CPU model.

The compat option itself does not make much sense (yes we could just add
yet another CPU class and that's it) but with the
ibm,client-architecture-support we will have to implement this
compatibility mode anyway. Since the guest can ask for a compatibility mode
change, we either have to support compat option or hot-unplug all (all) CPU
objects in QEMU and hotplug CPUs of the requested model. Or always reset
the guest if it asked for a compatibility mode and recreate CPUs in QEMU
during reset. As for me, the compat option seems simpler.


-- 
Alexey

Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option

[Paolo Bonzini]

Il 05/11/2013 11:45, Alexey Kardashevskiy ha scritto:
>> > Still in my opinion it should be "-cpu", not "-machine".  Even if it's
>> > just a "virtual" CPU model.
> The compat option itself does not make much sense (yes we could just add
> yet another CPU class and that's it) but with the
> ibm,client-architecture-support we will have to implement this
> compatibility mode anyway. Since the guest can ask for a compatibility mode
> change, we either have to support compat option or hot-unplug all (all) CPU
> objects in QEMU and hotplug CPUs of the requested model. Or always reset
> the guest if it asked for a compatibility mode and recreate CPUs in QEMU
> during reset. As for me, the compat option seems simpler.

Sure, just make it a suboption of "-cpu" rather than "-machine".

Paolo

Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option

[Andreas Färber]

Am 05.11.2013 10:52, schrieb Paolo Bonzini:
> Il 05/11/2013 10:16, Alexander Graf ha scritto:
>>
>> On 05.11.2013, at 10:06, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>> Il 30/09/2013 14:57, Alexey Kardashevskiy ha scritto:
>>>>>> Why is the option under -machine instead of -cpu?
>>>> Because it is still the same CPU and the guest will still read the real
>>>> PVR from the hardware (which it may not support but this is why we need
>>>> compatibility mode).
>>>
>>> How do you support migration from a newer to an older CPU then?  I think
>>> the guest should never see anything about the hardware CPU model.
>>
>> POWER can't model that. It always leaks the host CPU information into the guest. It's the guest kernel's responsibility to not expose that change to user space.
>>
>> Yes, it's broken :). I'm not even sure there is any sensible way to do live migration between different CPU types.
> 
> Still in my opinion it should be "-cpu", not "-machine".  Even if it's
> just a "virtual" CPU model.

PowerPC currently does not have -cpu option parsing. If you need to
implement it, I would ask for a generic hook in CPUClass set by
TYPE_POWERPC_CPU, so that the logic does not get hardcoded in cpu_init,
and for the p=v parsing logic to be so generic as to just set property p
to value v on the CPU instance. I.e. please make the compatibility
settings a static property or dynamic property of the CPU.

Maybe the parsing code could even live in generic qom/cpu.c, overridden
by x86/sparc and reused for arm? Somewhere down my to-do list but
patches appreciated...

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option

[Paul Mackerras]

On Tue, Nov 05, 2013 at 10:06:04AM +0100, Paolo Bonzini wrote:
> Il 30/09/2013 14:57, Alexey Kardashevskiy ha scritto:
> >> > Why is the option under -machine instead of -cpu?
> > Because it is still the same CPU and the guest will still read the real
> > PVR from the hardware (which it may not support but this is why we need
> > compatibility mode).
> 
> How do you support migration from a newer to an older CPU then? 

PowerVM has supported this for years, so there are well-established
interfaces that existing guest kernels use for this.

Basically, the hypervisor can put the CPU into a mode where the
processor behaves when in user mode according to an earlier version of
the architecture.  POWER8 conforms to architecture 2.07, and it has a
register with bits that allow it to be put into an architecture 2.05
(POWER6) mode or an architecture 2.06 (POWER7) mode.  In 2.05 mode,
none of the instructions and registers that are new in POWER7 and
POWER8 are available in user mode, and in 2.06 mode, none of the
facilities that are new in POWER8 are available in user mode.

The other thing the hypervisor does is tell the guest which
architecture mode the CPU is running in via a property in the device
tree.  Linux kernels use that to determine the list of hardware
features that they can use.

Under PowerVM, if you are running a guest on a POWER8 (say) and you
want to be able to be able to migrate to a POWER6 later on, you have
to run the guest in architecture 2.05 mode, even if the guest knows
about later architecture versions.

> I think
> the guest should never see anything about the hardware CPU model.

The architecture doesn't let us do that, but our guest kernels are
accustomed to largely ignoring the actual PVR and operating on the
basis of the architecture level that we tell them via the device
tree.

Regards,
Paul.

Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option

[Paul Mackerras]

On Mon, Sep 30, 2013 at 01:25:32PM +0200, Alexander Graf wrote:
> 
> On 27.09.2013, at 10:06, Alexey Kardashevskiy wrote:
> 
> > To be able to boot on newer hardware that the software support,
> > PowerISA defines a logical PVR, one per every PowerISA specification
> > version from 2.04.
[snip]
> > +    case 205:
> > +        spapr->arch_compat = CPU_POWERPC_LOGICAL_2_05;
> > +        break;
> > +    case 206:
> > +        spapr->arch_compat = CPU_POWERPC_LOGICAL_2_06;
> 
> Does it make sense to declare compat mode a number or would a string be easier for users? I can imagine that "-machine compat=power6" is easier to understand for a user than "-machine compat=205".

That's probably true.  I don't mind either way.

> Also, we need to handle failure. If the kernel can not set the CPU to 2.05 mode for example (IIRC POWER8 doesn't allow you to) we should bail out here.

POWER8 does have 2.05 (POWER6) and 2.06 (POWER7) compatibility modes.

> > +    /* Architecture compatibility mode */
> > +    uint32_t arch_compat;
> 
> Do we really need to carry this in the vcpu struct? Or can we just fire-and-forget about it? If we want to preserve anything, it should be the PCR register.

There are two relevant values here; the compatibility mode (if any)
that the user has requested via the command line, and the mode that
has been negotiated with the ibm,client-architecture-support (CAS)
call.  CAS should select the latest mode supported by the guest that
is not later than the mode requested on the command line, and is
supported by QEMU, and is not later than the architecture of the
host.  Both values should be sent across to the destination VM on
migration AFAICS.

On reset/reboot, the compatibility mode should not change.  The device
tree that is supplied to the new SLOF instance should reflect the
current compatibility mode.

Regards,
Paul.

Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option

[Igor Mammedov]

On Tue, 05 Nov 2013 14:53:14 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 05.11.2013 10:52, schrieb Paolo Bonzini:
> > Il 05/11/2013 10:16, Alexander Graf ha scritto:
> >>
> >> On 05.11.2013, at 10:06, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >>
> >>> Il 30/09/2013 14:57, Alexey Kardashevskiy ha scritto:
> >>>>>> Why is the option under -machine instead of -cpu?
> >>>> Because it is still the same CPU and the guest will still read the real
> >>>> PVR from the hardware (which it may not support but this is why we need
> >>>> compatibility mode).
> >>>
> >>> How do you support migration from a newer to an older CPU then?  I think
> >>> the guest should never see anything about the hardware CPU model.
> >>
> >> POWER can't model that. It always leaks the host CPU information into the guest. It's the guest kernel's responsibility to not expose that change to user space.
> >>
> >> Yes, it's broken :). I'm not even sure there is any sensible way to do live migration between different CPU types.
> > 
> > Still in my opinion it should be "-cpu", not "-machine".  Even if it's
> > just a "virtual" CPU model.
> 
> PowerPC currently does not have -cpu option parsing. If you need to
> implement it, I would ask for a generic hook in CPUClass set by
> TYPE_POWERPC_CPU, so that the logic does not get hardcoded in cpu_init,
> and for the p=v parsing logic to be so generic as to just set property p
> to value v on the CPU instance. I.e. please make the compatibility
> settings a static property or dynamic property of the CPU.
Seconded, that's what is on my to-do list for x86 once x86 properties series
is applied.

moreover, all hook has to do is to translate -cpu to a set of global options,
adding [CPUtype.foo, val] pairs to global option list. Then options will be
automatically applied to any new instance of CPUtype and there won't be any
need for accessing/storing cpu_model string during cpu creation and hotplug.

> 
> Maybe the parsing code could even live in generic qom/cpu.c, overridden
> by x86/sparc and reused for arm? Somewhere down my to-do list but
> patches appreciated...
> 
> Andreas
> 

Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option

[Alexander Graf]

On 06.11.2013, at 06:48, Paul Mackerras <paulus@samba.org> wrote:

> On Mon, Sep 30, 2013 at 01:25:32PM +0200, Alexander Graf wrote:
>> 
>> On 27.09.2013, at 10:06, Alexey Kardashevskiy wrote:
>> 
>>> To be able to boot on newer hardware that the software support,
>>> PowerISA defines a logical PVR, one per every PowerISA specification
>>> version from 2.04.
> [snip]
>>> +    case 205:
>>> +        spapr->arch_compat = CPU_POWERPC_LOGICAL_2_05;
>>> +        break;
>>> +    case 206:
>>> +        spapr->arch_compat = CPU_POWERPC_LOGICAL_2_06;
>> 
>> Does it make sense to declare compat mode a number or would a string be easier for users? I can imagine that "-machine compat=power6" is easier to understand for a user than "-machine compat=205".
> 
> That's probably true.  I don't mind either way.
> 
>> Also, we need to handle failure. If the kernel can not set the CPU to 2.05 mode for example (IIRC POWER8 doesn't allow you to) we should bail out here.
> 
> POWER8 does have 2.05 (POWER6) and 2.06 (POWER7) compatibility modes.
> 
>>> +    /* Architecture compatibility mode */
>>> +    uint32_t arch_compat;
>> 
>> Do we really need to carry this in the vcpu struct? Or can we just fire-and-forget about it? If we want to preserve anything, it should be the PCR register.
> 
> There are two relevant values here; the compatibility mode (if any)
> that the user has requested via the command line, and the mode that
> has been negotiated with the ibm,client-architecture-support (CAS)
> call.  CAS should select the latest mode supported by the guest that
> is not later than the mode requested on the command line, and is
> supported by QEMU, and is not later than the architecture of the
> host.  Both values should be sent across to the destination VM on
> migration AFAICS.

So how does this work on pHyp? I thought the guest always comes up in full native compat mode and downgrades itself then?

> On reset/reboot, the compatibility mode should not change.  The device
> tree that is supplied to the new SLOF instance should reflect the
> current compatibility mode.

Ok, this should work if we just don't touch PCR across reset.


Alex

Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option

[Alexey Kardashevskiy]

On 11/06/2013 12:53 AM, Andreas Färber wrote:> Am 05.11.2013 10:52, schrieb Paolo Bonzini:
>> Il 05/11/2013 10:16, Alexander Graf ha scritto:
>>>
>>> On 05.11.2013, at 10:06, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>
>>>> Il 30/09/2013 14:57, Alexey Kardashevskiy ha scritto:
>>>>>>> Why is the option under -machine instead of -cpu?
>>>>> Because it is still the same CPU and the guest will still read the real
>>>>> PVR from the hardware (which it may not support but this is why we need
>>>>> compatibility mode).
>>>>
>>>> How do you support migration from a newer to an older CPU then?  I think
>>>> the guest should never see anything about the hardware CPU model.
>>>
>>> POWER can't model that. It always leaks the host CPU information into the guest. It's the guest kernel's responsibility to not expose that change to user space.
>>>
>>> Yes, it's broken :). I'm not even sure there is any sensible way to do live migration between different CPU types.
>>
>> Still in my opinion it should be "-cpu", not "-machine".  Even if it's
>> just a "virtual" CPU model.
>
> PowerPC currently does not have -cpu option parsing. If you need to
> implement it, I would ask for a generic hook in CPUClass set by
> TYPE_POWERPC_CPU, so that the logic does not get hardcoded in cpu_init,
> and for the p=v parsing logic to be so generic as to just set property p
> to value v on the CPU instance. I.e. please make the compatibility
> settings a static property or dynamic property of the CPU.
>
> Maybe the parsing code could even live in generic qom/cpu.c, overridden
> by x86/sparc and reused for arm? Somewhere down my to-do list but
> patches appreciated...


I spent some time today trying to digest what you said, still having problems
with understanding of what you meant and what Igor meant about global variables
(I just do not see the point in them).

Below is the result of my excercise. At the moment I would just like to know
if I am going in the right direction or not.

And few question while we are here:
1. the proposed common code handles both static and dynamic properties.
What is the current QEMU trend about choosing static vs. dynamic? Can do
both in POWERPC, both have benifits.

2. The static powerpc_properties array only works if defined with POWER7 family
but not POWER family. Both families are abstract so I did not expect any
difference but it is there. Any clue before I continue debugging? :)

Thanks!

---

This adds suboptions support for -cpu and demonstrates the use ot this
by adding a static "compat1" and a dynamic "compat" options to POWERPC
CPUs.

---
 include/qom/cpu.h           |  6 ++++++
 include/sysemu/sysemu.h     |  1 +
 qom/cpu.c                   | 24 ++++++++++++++++++++++++
 target-ppc/translate_init.c | 40 ++++++++++++++++++++++++++++++++++++++++
 vl.c                        | 35 ++++++++++++++++++++++++++++++++++-
 5 files changed, 105 insertions(+), 1 deletion(-)

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 7739e00..a17cd73 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -327,6 +327,12 @@ static inline hwaddr cpu_get_phys_page_debug(CPUState *cpu, vaddr addr)
 #endif
 
 /**
+ * cpu_common_set_properties:
+ * @cpu: The CPU whose properties to initialize from the command line.
+ */
+int cpu_common_set_properties(Object *obj);
+
+/**
  * cpu_reset:
  * @cpu: The CPU whose state is to be reset.
  */
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index b27a1df..9007e44 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -192,6 +192,7 @@ DeviceState *get_boot_device_ex(uint32_t position, char **suffix);
 DeviceState *get_boot_device(uint32_t position);
 
 QemuOpts *qemu_get_machine_opts(void);
+QemuOpts *qemu_get_cpu_opts(void);
 
 bool usb_enabled(bool default_usb);
 
diff --git a/qom/cpu.c b/qom/cpu.c
index 818fb26..6516c63 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -24,6 +24,8 @@
 #include "qemu/notify.h"
 #include "qemu/log.h"
 #include "sysemu/sysemu.h"
+#include "hw/qdev-properties.h"
+#include "qapi/qmp/qerror.h"
 
 bool cpu_exists(int64_t id)
 {
@@ -186,6 +188,28 @@ void cpu_reset(CPUState *cpu)
     }
 }
 
+static int cpu_set_property(const char *name, const char *value, void *opaque)
+{
+    DeviceState *dev = opaque;
+    Error *err = NULL;
+
+    if (strcmp(name, "type") == 0)
+        return 0;
+
+    qdev_prop_parse(dev, name, value, &err);
+    if (err != NULL) {
+        qerror_report_err(err);
+        error_free(err);
+        return -1;
+    }
+    return 0;
+}
+
+int cpu_common_set_properties(Object *obj)
+{
+    return qemu_opt_foreach(qemu_get_cpu_opts(), cpu_set_property, obj, 1);
+}
+
 static void cpu_common_reset(CPUState *cpu)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 93f7cba..d357b1f 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -28,6 +28,8 @@
 #include "mmu-hash32.h"
 #include "mmu-hash64.h"
 #include "qemu/error-report.h"
+#include "hw/qdev-properties.h"
+#include "qapi/visitor.h"
 
 //#define PPC_DUMP_CPU
 //#define PPC_DEBUG_SPR
@@ -4733,6 +4735,11 @@ POWERPC_FAMILY(e5500)(ObjectClass *oc, void *data)
 
 /* Non-embedded PowerPC                                                      */
 
+static Property powerpc_properties[] = {
+    DEFINE_PROP_INT32("compat1", PowerPCCPU, env.compat, -1),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 /* POWER : same as 601, without mfmsr, mfsr                                  */
 POWERPC_FAMILY(POWER)(ObjectClass *oc, void *data)
 {
@@ -4743,6 +4750,7 @@ POWERPC_FAMILY(POWER)(ObjectClass *oc, void *data)
     /* pcc->insns_flags = XXX_TODO; */
     /* POWER RSC (from RAD6000) */
     pcc->msr_mask = 0x00000000FEF0ULL;
+    dc->props = powerpc_properties;
 }
 
 #define POWERPC_MSRR_601     (0x0000000000001040ULL)
@@ -7262,6 +7270,7 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data)
                  POWERPC_FLAG_BUS_CLK | POWERPC_FLAG_CFAR;
     pcc->l1_dcache_size = 0x8000;
     pcc->l1_icache_size = 0x8000;
+    dc->props = powerpc_properties;
 }
 
 POWERPC_FAMILY(POWER7P)(ObjectClass *oc, void *data)
@@ -8390,6 +8399,10 @@ PowerPCCPU *cpu_ppc_init(const char *cpu_model)
 
     cpu = POWERPC_CPU(object_new(object_class_get_name(oc)));
 
+    if (cpu_common_set_properties(OBJECT(cpu))) {
+        return NULL;
+    }
+
     object_property_set_bool(OBJECT(cpu), true, "realized", &err);
     if (err != NULL) {
         error_report("%s", error_get_pretty(err));
@@ -8611,6 +8624,30 @@ static void ppc_cpu_reset(CPUState *s)
     tlb_flush(env, 1);
 }
 
+static void powerpc_get_compat(Object *obj, Visitor *v,
+                               void *opaque, const char *name, Error **errp)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(obj);
+    int64_t value = cpu->env.compat;
+
+    visit_type_int(v, &value, name, errp);
+}
+
+static void powerpc_set_compat(Object *obj, Visitor *v,
+                               void *opaque, const char *name, Error **errp)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(obj);
+    Error *error = NULL;
+    int64_t value;
+
+    visit_type_int(v, &value, name, &error);
+    if (error) {
+        error_propagate(errp, error);
+        return;
+    }
+    cpu->env.compat = value;
+}
+
 static void ppc_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -8618,6 +8655,9 @@ static void ppc_cpu_initfn(Object *obj)
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
     CPUPPCState *env = &cpu->env;
 
+    object_property_add(obj, "compat", "int",
+                        powerpc_get_compat, powerpc_set_compat,
+                        NULL, NULL, NULL);
     cs->env_ptr = env;
     cpu_exec_init(env);
 
diff --git a/vl.c b/vl.c
index d5c5d8c..a5fbc38 100644
--- a/vl.c
+++ b/vl.c
@@ -437,6 +437,16 @@ static QemuOptsList qemu_machine_opts = {
     },
 };
 
+static QemuOptsList qemu_cpu_opts = {
+    .name = "cpu",
+    .implied_opt_name = "type",
+    .merge_lists = true,
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_cpu_opts.head),
+    .desc = {
+        { /* End of list */ }
+    },
+};
+
 static QemuOptsList qemu_boot_opts = {
     .name = "boot-opts",
     .implied_opt_name = "order",
@@ -552,6 +562,20 @@ QemuOpts *qemu_get_machine_opts(void)
     return opts;
 }
 
+QemuOpts *qemu_get_cpu_opts(void)
+{
+    QemuOptsList *list;
+    QemuOpts *opts;
+
+    list = qemu_find_opts("cpu");
+    assert(list);
+    opts = qemu_opts_find(list, NULL);
+    if (!opts) {
+        opts = qemu_opts_create_nofail(list);
+    }
+    return opts;
+}
+
 const char *qemu_get_vm_name(void)
 {
     return qemu_name;
@@ -2942,6 +2966,7 @@ int main(int argc, char **argv, char **envp)
     qemu_add_opts(&qemu_trace_opts);
     qemu_add_opts(&qemu_option_rom_opts);
     qemu_add_opts(&qemu_machine_opts);
+    qemu_add_opts(&qemu_cpu_opts);
     qemu_add_opts(&qemu_smp_opts);
     qemu_add_opts(&qemu_boot_opts);
     qemu_add_opts(&qemu_sandbox_opts);
@@ -3037,7 +3062,15 @@ int main(int argc, char **argv, char **envp)
             }
             case QEMU_OPTION_cpu:
                 /* hw initialization will check this */
-                cpu_model = optarg;
+                olist = qemu_find_opts("cpu");
+                opts = qemu_opts_parse(olist, optarg, 1);
+                if (!opts) {
+                    exit(1);
+                }
+                optarg = qemu_opt_get(opts, "type");
+                if (optarg) {
+                    cpu_model = optarg;
+                }
                 break;
             case QEMU_OPTION_hda:
                 {
-- 
1.8.4.rc4

Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option

[Igor Mammedov]

On Thu,  7 Nov 2013 20:11:51 +1100
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> On 11/06/2013 12:53 AM, Andreas Färber wrote:> Am 05.11.2013 10:52, schrieb Paolo Bonzini:
> >> Il 05/11/2013 10:16, Alexander Graf ha scritto:
> >>>
> >>> On 05.11.2013, at 10:06, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >>>
> >>>> Il 30/09/2013 14:57, Alexey Kardashevskiy ha scritto:
> >>>>>>> Why is the option under -machine instead of -cpu?
> >>>>> Because it is still the same CPU and the guest will still read the real
> >>>>> PVR from the hardware (which it may not support but this is why we need
> >>>>> compatibility mode).
> >>>>
> >>>> How do you support migration from a newer to an older CPU then?  I think
> >>>> the guest should never see anything about the hardware CPU model.
> >>>
> >>> POWER can't model that. It always leaks the host CPU information into the guest. It's the guest kernel's responsibility to not expose that change to user space.
> >>>
> >>> Yes, it's broken :). I'm not even sure there is any sensible way to do live migration between different CPU types.
> >>
> >> Still in my opinion it should be "-cpu", not "-machine".  Even if it's
> >> just a "virtual" CPU model.
> >
> > PowerPC currently does not have -cpu option parsing. If you need to
> > implement it, I would ask for a generic hook in CPUClass set by
> > TYPE_POWERPC_CPU, so that the logic does not get hardcoded in cpu_init,
> > and for the p=v parsing logic to be so generic as to just set property p
> > to value v on the CPU instance. I.e. please make the compatibility
> > settings a static property or dynamic property of the CPU.
> >
> > Maybe the parsing code could even live in generic qom/cpu.c, overridden
> > by x86/sparc and reused for arm? Somewhere down my to-do list but
> > patches appreciated...
> 
> 
> I spent some time today trying to digest what you said, still having problems
> with understanding of what you meant and what Igor meant about global variables
> (I just do not see the point in them).
> 
> Below is the result of my excercise. At the moment I would just like to know
> if I am going in the right direction or not.

what I've had in mind was a bit simpler and more implicit approach instead of
setting properties on each CPU instance explicitly. It could done using
existing "global properties" mechanism.

in current code -cpu type,foo1=x,foo2=y... are saved into cpu_model string
which is parsed by target specific cpu_init() effectively parsing cpu_model
each time when creating a CPU.

So to avoid fixing every target I suggest to leave cpu_model be as it is and
add translation hook that will convert "type,foo1=x,foo2=y..." virtually
into a set of following options:
 "-global type.foo1=x -global type.foo2=y ..."
these options when registered are transparently applied to each new CPU
instance (see device_post_init() for details).

now why do we need translation hook,
 * not every target is able to handle "type,foo1=x,foo2=y" in terms of
   properties yet
 * legacy feature string might be in non canonical format, like
   "+foo1,-foo2..." so for compatibility reasons with CLI we need target
   specific code to convert to canonical format when it becomes available.
 * for targets that don't have feature string handling and implementing
   new features as properties we can implement generic default hook that
   will convert canonical feature string into global properties.

as result we eventually would be able drop cpu_model and use global
properties to store CPU features.

see comments below for pseudo code:
> And few question while we are here:
> 1. the proposed common code handles both static and dynamic properties.
> What is the current QEMU trend about choosing static vs. dynamic? Can do
> both in POWERPC, both have benifits.
I prefer static, since it's usually less boilerplate code.


[...]

> 
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 7739e00..a17cd73 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -327,6 +327,12 @@ static inline hwaddr cpu_get_phys_page_debug(CPUState *cpu, vaddr addr)
>  #endif
>  
>  /**
> + * cpu_common_set_properties:
> + * @cpu: The CPU whose properties to initialize from the command line.
> + */
> +int cpu_common_set_properties(Object *obj);

cpu_translate_features_compat(classname, cpu_model_str)


> diff --git a/qom/cpu.c b/qom/cpu.c
> index 818fb26..6516c63 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -24,6 +24,8 @@
>  #include "qemu/notify.h"
>  #include "qemu/log.h"
>  #include "sysemu/sysemu.h"
> +#include "hw/qdev-properties.h"
> +#include "qapi/qmp/qerror.h"
>  
>  bool cpu_exists(int64_t id)
>  {
> @@ -186,6 +188,28 @@ void cpu_reset(CPUState *cpu)
>      }
>  }
>  
> +static int cpu_set_property(const char *name, const char *value, void *opaque)
> +{
> +    DeviceState *dev = opaque;
> +    Error *err = NULL;
> +
> +    if (strcmp(name, "type") == 0)
> +        return 0;
> +
> +    qdev_prop_parse(dev, name, value, &err);
> +    if (err != NULL) {
> +        qerror_report_err(err);
> +        error_free(err);
> +        return -1;
> +    }
> +    return 0;
> +}
> +
> +int cpu_common_set_properties(Object *obj)
> +{
> +    return qemu_opt_foreach(qemu_get_cpu_opts(), cpu_set_property, obj, 1);
> +}
replace ^^^ with:

void cpu_translate_features_compat(classname, cpu_model_str) {
    for_each_foo in cpu_model_str {
        qdev_prop_register_global(classname.foo=val)
    }
}

and set default hook to NULL for every target that does custom
parsing (i.e. x86/sparc) so hook will be NOP there.


> diff --git a/vl.c b/vl.c
> index d5c5d8c..a5fbc38 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -437,6 +437,16 @@ static QemuOptsList qemu_machine_opts = {
>      },
>  };
>  

>              case QEMU_OPTION_cpu:
>                  /* hw initialization will check this */

cpu_model = optarg;
classname = GET_CLASS_NAME_FROM_TYPE(cpu_model) <= not sure how implement it since naming is target specific, maybe Andreas has an idea
CPUClass = get_cpu_class_by_name(classname)
class->cpu_translate_features_compat(classname, cpu_model_str)

>                  break;
>              case QEMU_OPTION_hda:
>                  {

Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option

[Andreas Färber]

Am 07.11.2013 10:11, schrieb Alexey Kardashevskiy:
> On 11/06/2013 12:53 AM, Andreas Färber wrote:> Am 05.11.2013 10:52, schrieb Paolo Bonzini:
>>> Il 05/11/2013 10:16, Alexander Graf ha scritto:
>>>>
>>>> On 05.11.2013, at 10:06, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>
>>>>> Il 30/09/2013 14:57, Alexey Kardashevskiy ha scritto:
>>>>>>>> Why is the option under -machine instead of -cpu?
>>>>>> Because it is still the same CPU and the guest will still read the real
>>>>>> PVR from the hardware (which it may not support but this is why we need
>>>>>> compatibility mode).
>>>>>
>>>>> How do you support migration from a newer to an older CPU then?  I think
>>>>> the guest should never see anything about the hardware CPU model.
>>>>
>>>> POWER can't model that. It always leaks the host CPU information into the guest. It's the guest kernel's responsibility to not expose that change to user space.
>>>>
>>>> Yes, it's broken :). I'm not even sure there is any sensible way to do live migration between different CPU types.
>>>
>>> Still in my opinion it should be "-cpu", not "-machine".  Even if it's
>>> just a "virtual" CPU model.
>>
>> PowerPC currently does not have -cpu option parsing. If you need to
>> implement it, I would ask for a generic hook in CPUClass set by
>> TYPE_POWERPC_CPU, so that the logic does not get hardcoded in cpu_init,
>> and for the p=v parsing logic to be so generic as to just set property p
>> to value v on the CPU instance. I.e. please make the compatibility
>> settings a static property or dynamic property of the CPU.
>>
>> Maybe the parsing code could even live in generic qom/cpu.c, overridden
>> by x86/sparc and reused for arm? Somewhere down my to-do list but
>> patches appreciated...
> 
> 
> I spent some time today trying to digest what you said, still having problems
> with understanding of what you meant and what Igor meant about global variables
> (I just do not see the point in them).
> 
> Below is the result of my excercise. At the moment I would just like to know
> if I am going in the right direction or not.

The overall direction is good ... see below.

> 
> And few question while we are here:
> 1. the proposed common code handles both static and dynamic properties.
> What is the current QEMU trend about choosing static vs. dynamic? Can do
> both in POWERPC, both have benifits.

Static properties have mostly served to set a field to a value before
the object is realized. You can set a default value there. The setters
are usually no-op (error out) for realized objects.

Dynamic properties allow you (more easily) to implement any logic for
storing/retrieving the value and can serve to inspect or set a value at
runtime.

We were told on a KVM call that discovery of properties should not be a
decision factor towards static properties - management tools need to
inspect an object instance via QMP (and handle a property getting
dropped or renamed).

> 2. The static powerpc_properties array only works if defined with POWER7 family
> but not POWER family. Both families are abstract so I did not expect any
> difference but it is there. Any clue before I continue debugging? :)

There is no hierarchy among families. So POWER7 is not a POWER, it's a
powerpc at the bottom of the file. If you want power_properties rather
than powerpc_properties then you need to assign them individually for
POWER, ..., POWER5, ..., POWER7, POWER8 - or tweak the type hierarchy.

> 
> Thanks!
> 
> ---
> 
> This adds suboptions support for -cpu and demonstrates the use ot this
> by adding a static "compat1" and a dynamic "compat" options to POWERPC
> CPUs.

Unfortunately that approach won't work. Both x86 and sparc, as I
mentioned, need special handling, so you can't generalize it. Either we
need #ifdef'fery to rule out the exceptions, or better, what I suggested
was something along the lines of

struct CPUClass {
...
void (*parse_options)(CPUState *cpu, const char *str);
}

with cpu_common_parse_options() as the default implementation assigned
via cc->parse_options = cpu_common_parse_options; rather than called out
of common code.

You could have a trivial (inline?) function to obtain cc and call
cc->parse_options though, for use in cpu_ppc_init().

I also think you can use the object_property_* API rather than
qdev_prop_* for parsing and setting the value, compare Igor's code in
target-i386/cpu.c.

Please do separate these global preparations from the actual new ppc
property. Elsewhere it was discussed whether to use a readable string
value, which might hint at a dynamic property of type string or maybe
towards an enum (/me no experience with those yet and whether that works
better with dynamic or static).

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option

[Alexey Kardashevskiy]

On 11/08/2013 12:36 AM, Igor Mammedov wrote:
> On Thu,  7 Nov 2013 20:11:51 +1100
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> 
>> On 11/06/2013 12:53 AM, Andreas Färber wrote:> Am 05.11.2013 10:52, schrieb Paolo Bonzini:
>>>> Il 05/11/2013 10:16, Alexander Graf ha scritto:
>>>>>
>>>>> On 05.11.2013, at 10:06, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>>
>>>>>> Il 30/09/2013 14:57, Alexey Kardashevskiy ha scritto:
>>>>>>>>> Why is the option under -machine instead of -cpu?
>>>>>>> Because it is still the same CPU and the guest will still read the real
>>>>>>> PVR from the hardware (which it may not support but this is why we need
>>>>>>> compatibility mode).
>>>>>>
>>>>>> How do you support migration from a newer to an older CPU then?  I think
>>>>>> the guest should never see anything about the hardware CPU model.
>>>>>
>>>>> POWER can't model that. It always leaks the host CPU information into the guest. It's the guest kernel's responsibility to not expose that change to user space.
>>>>>
>>>>> Yes, it's broken :). I'm not even sure there is any sensible way to do live migration between different CPU types.
>>>>
>>>> Still in my opinion it should be "-cpu", not "-machine".  Even if it's
>>>> just a "virtual" CPU model.
>>>
>>> PowerPC currently does not have -cpu option parsing. If you need to
>>> implement it, I would ask for a generic hook in CPUClass set by
>>> TYPE_POWERPC_CPU, so that the logic does not get hardcoded in cpu_init,
>>> and for the p=v parsing logic to be so generic as to just set property p
>>> to value v on the CPU instance. I.e. please make the compatibility
>>> settings a static property or dynamic property of the CPU.
>>>
>>> Maybe the parsing code could even live in generic qom/cpu.c, overridden
>>> by x86/sparc and reused for arm? Somewhere down my to-do list but
>>> patches appreciated...
>>
>>
>> I spent some time today trying to digest what you said, still having problems
>> with understanding of what you meant and what Igor meant about global variables
>> (I just do not see the point in them).
>>
>> Below is the result of my excercise. At the moment I would just like to know
>> if I am going in the right direction or not.
> 
> what I've had in mind was a bit simpler and more implicit approach instead of
> setting properties on each CPU instance explicitly. It could done using
> existing "global properties" mechanism.
> 
> in current code -cpu type,foo1=x,foo2=y... are saved into cpu_model string
> which is parsed by target specific cpu_init() effectively parsing cpu_model
> each time when creating a CPU.
> 
> So to avoid fixing every target I suggest to leave cpu_model be as it is and
>
> add translation hook that will convert "type,foo1=x,foo2=y..." virtually
> into a set of following options:
>  "-global type.foo1=x -global type.foo2=y ..."
> these options when registered are transparently applied to each new CPU
> instance (see device_post_init() for details).
> 
> now why do we need translation hook,
>  * not every target is able to handle "type,foo1=x,foo2=y" in terms of
>    properties yet
>  * legacy feature string might be in non canonical format, like
>    "+foo1,-foo2..." so for compatibility reasons with CLI we need target
>    specific code to convert to canonical format when it becomes available.
>  * for targets that don't have feature string handling and implementing
>    new features as properties we can implement generic default hook that
>    will convert canonical feature string into global properties.
> 
> as result we eventually would be able drop cpu_model and use global
> properties to store CPU features.


What is wrong doing it in the way the "-machine" switch does it now?
qemu_get_machine_opts() returns a global list which I can iterate through
via qemu_opt_foreach() and set every property to a CPU, this will check if
a property exists and assigns it => happy Aik :)



> see comments below for pseudo code:
>> And few question while we are here:
>> 1. the proposed common code handles both static and dynamic properties.
>> What is the current QEMU trend about choosing static vs. dynamic? Can do
>> both in POWERPC, both have benifits.
> I prefer static, since it's usually less boilerplate code.
> 
> 
> [...]
> 
>>
>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>> index 7739e00..a17cd73 100644
>> --- a/include/qom/cpu.h
>> +++ b/include/qom/cpu.h
>> @@ -327,6 +327,12 @@ static inline hwaddr cpu_get_phys_page_debug(CPUState *cpu, vaddr addr)
>>  #endif
>>  
>>  /**
>> + * cpu_common_set_properties:
>> + * @cpu: The CPU whose properties to initialize from the command line.
>> + */
>> +int cpu_common_set_properties(Object *obj);
> 
> cpu_translate_features_compat(classname, cpu_model_str)


Here I lost you. I am looking to a generic way of adding any number of
properties to "-cpu", not just "compat".


>> diff --git a/qom/cpu.c b/qom/cpu.c
>> index 818fb26..6516c63 100644
>> --- a/qom/cpu.c
>> +++ b/qom/cpu.c
>> @@ -24,6 +24,8 @@
>>  #include "qemu/notify.h"
>>  #include "qemu/log.h"
>>  #include "sysemu/sysemu.h"
>> +#include "hw/qdev-properties.h"
>> +#include "qapi/qmp/qerror.h"
>>  
>>  bool cpu_exists(int64_t id)
>>  {
>> @@ -186,6 +188,28 @@ void cpu_reset(CPUState *cpu)
>>      }
>>  }
>>  
>> +static int cpu_set_property(const char *name, const char *value, void *opaque)
>> +{
>> +    DeviceState *dev = opaque;
>> +    Error *err = NULL;
>> +
>> +    if (strcmp(name, "type") == 0)
>> +        return 0;
>> +
>> +    qdev_prop_parse(dev, name, value, &err);
>> +    if (err != NULL) {
>> +        qerror_report_err(err);
>> +        error_free(err);
>> +        return -1;
>> +    }
>> +    return 0;
>> +}
>> +
>> +int cpu_common_set_properties(Object *obj)
>> +{
>> +    return qemu_opt_foreach(qemu_get_cpu_opts(), cpu_set_property, obj, 1);
>> +}
> replace ^^^ with:
> 
> void cpu_translate_features_compat(classname, cpu_model_str) {
>     for_each_foo in cpu_model_str {
>         qdev_prop_register_global(classname.foo=val)
>     }
> }
> 
> and set default hook to NULL for every target that does custom
> parsing (i.e. x86/sparc) so hook will be NOP there.
> 
> 
>> diff --git a/vl.c b/vl.c
>> index d5c5d8c..a5fbc38 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -437,6 +437,16 @@ static QemuOptsList qemu_machine_opts = {
>>      },
>>  };
>>  
> 
>>              case QEMU_OPTION_cpu:
>>                  /* hw initialization will check this */
> 
> cpu_model = optarg;
> classname = GET_CLASS_NAME_FROM_TYPE(cpu_model) <= not sure how
> implement it since naming is target specific, maybe Andreas has an idea

Heh. We cannot do this as on ppc we have to use ppc_cpu_class_by_name() and
we definitely do not want to call it from vl.c.


Thanks for comments but I'll try what Andreas suggested if I understood it
all right and that suggestion is any different from yours :)



> CPUClass = get_cpu_class_by_name(classname)
> class->cpu_translate_features_compat(classname, cpu_model_str)
> 
>>                  break;
>>              case QEMU_OPTION_hda:
>>                  {
> 




-- 
Alexey

[Qemu-devel] [PATCH v2 0/2] spapr: add "compat" machine option

[Alexey Kardashevskiy]

On 11/08/2013 01:01 AM, Andreas Färber wrote:> Am 07.11.2013 10:11, schrieb Alexey Kardashevskiy:
>> On 11/06/2013 12:53 AM, Andreas Färber wrote:> Am 05.11.2013 10:52, schrieb Paolo Bonzini:
>>>> Il 05/11/2013 10:16, Alexander Graf ha scritto:
>>>>>
>>>>> On 05.11.2013, at 10:06, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>>
>>>>>> Il 30/09/2013 14:57, Alexey Kardashevskiy ha scritto:
>>>>>>>>> Why is the option under -machine instead of -cpu?
>>>>>>> Because it is still the same CPU and the guest will still read the real
>>>>>>> PVR from the hardware (which it may not support but this is why we need
>>>>>>> compatibility mode).
>>>>>>
>>>>>> How do you support migration from a newer to an older CPU then?  I think
>>>>>> the guest should never see anything about the hardware CPU model.
>>>>>
>>>>> POWER can't model that. It always leaks the host CPU information into the guest. It's the guest kernel's responsibility to not expose that change to user space.
>>>>>
>>>>> Yes, it's broken :). I'm not even sure there is any sensible way to do live migration between different CPU types.
>>>>
>>>> Still in my opinion it should be "-cpu", not "-machine".  Even if it's
>>>> just a "virtual" CPU model.
>>>
>>> PowerPC currently does not have -cpu option parsing. If you need to
>>> implement it, I would ask for a generic hook in CPUClass set by
>>> TYPE_POWERPC_CPU, so that the logic does not get hardcoded in cpu_init,
>>> and for the p=v parsing logic to be so generic as to just set property p
>>> to value v on the CPU instance. I.e. please make the compatibility
>>> settings a static property or dynamic property of the CPU.
>>>
>>> Maybe the parsing code could even live in generic qom/cpu.c, overridden
>>> by x86/sparc and reused for arm? Somewhere down my to-do list but
>>> patches appreciated...
>>
>>
>> I spent some time today trying to digest what you said, still having problems
>> with understanding of what you meant and what Igor meant about global variables
>> (I just do not see the point in them).
>>
>> Below is the result of my excercise. At the moment I would just like to know
>> if I am going in the right direction or not.
> 
> The overall direction is good ... see below.

Good. Thanks for comments.


> 
>>
>> And few question while we are here:
>> 1. the proposed common code handles both static and dynamic properties.
>> What is the current QEMU trend about choosing static vs. dynamic? Can do
>> both in POWERPC, both have benifits.
> 
> Static properties have mostly served to set a field to a value before
> the object is realized. You can set a default value there. The setters
> are usually no-op (error out) for realized objects.
> 
> Dynamic properties allow you (more easily) to implement any logic for
> storing/retrieving the value and can serve to inspect or set a value at
> runtime.
> 
> We were told on a KVM call that discovery of properties should not be a
> decision factor towards static properties - management tools need to
> inspect an object instance via QMP (and handle a property getting
> dropped or renamed).
> 
>> 2. The static powerpc_properties array only works if defined with POWER7 family
>> but not POWER family. Both families are abstract so I did not expect any
>> difference but it is there. Any clue before I continue debugging? :)
> 
> There is no hierarchy among families. So POWER7 is not a POWER, it's a
> powerpc at the bottom of the file. If you want power_properties rather
> than powerpc_properties then you need to assign them individually for
> POWER, ..., POWER5, ..., POWER7, POWER8 - or tweak the type hierarchy.
>
>>
>> Thanks!
>>
>> ---
>>
>> This adds suboptions support for -cpu and demonstrates the use ot this
>> by adding a static "compat1" and a dynamic "compat" options to POWERPC
>> CPUs.
> 
> Unfortunately that approach won't work. Both x86 and sparc, as I
> mentioned, need special handling, so you can't generalize it. Either we
> need #ifdef'fery to rule out the exceptions, or better, what I suggested
> was something along the lines of
> 
> struct CPUClass {
> ...
> void (*parse_options)(CPUState *cpu, const char *str);
> }
> 
> with cpu_common_parse_options() as the default implementation assigned
> via cc->parse_options = cpu_common_parse_options; rather than called out
> of common code.
> 
> You could have a trivial (inline?) function to obtain cc and call
> cc->parse_options though, for use in cpu_ppc_init().
> 
> I also think you can use the object_property_* API rather than
> qdev_prop_* for parsing and setting the value, compare Igor's code in
> target-i386/cpu.c.

I did all of this (I hope) so here is another try.

> Please do separate these global preparations from the actual new ppc
> property. Elsewhere it was discussed whether to use a readable string
> value, which might hint at a dynamic property of type string or maybe
> towards an enum (/me no experience with those yet and whether that works
> better with dynamic or static).


Ufff... I did not get the part about a hint. Everything is string
in the command line :)


---

Alexey Kardashevskiy (2):
  cpu: add suboptions support
  target-ppc: add "compat" CPU option

 hw/ppc/spapr.c              | 12 +++++++++-
 include/qom/cpu.h           | 29 +++++++++++++++++++++++
 include/sysemu/sysemu.h     |  1 +
 qom/cpu.c                   | 27 ++++++++++++++++++++++
 target-ppc/cpu-models.h     | 16 +++++++++++++
 target-ppc/cpu.h            |  4 ++++
 target-ppc/translate_init.c | 56 +++++++++++++++++++++++++++++++++++++++++++++
 vl.c                        | 42 ++++++++++++++++++++++++++++++++++
 8 files changed, 186 insertions(+), 1 deletion(-)

-- 
1.8.4.rc4

[Qemu-devel] [PATCH v2 1/2] cpu: add suboptions support

[Alexey Kardashevskiy]

This adds suboptions support for -cpu.

Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Andreas Färber <afaerber@suse.de>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 include/qom/cpu.h       | 29 +++++++++++++++++++++++++++++
 include/sysemu/sysemu.h |  1 +
 qom/cpu.c               | 27 +++++++++++++++++++++++++++
 vl.c                    | 42 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 99 insertions(+)

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 7739e00..7d3b043 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -124,6 +124,7 @@ typedef struct CPUClass {
                             int cpuid, void *opaque);
     int (*write_elf32_qemunote)(WriteCoreDumpFunction f, CPUState *cpu,
                                 void *opaque);
+    void (*parse_options)(CPUState *cpu, Error **errp);
 
     const struct VMStateDescription *vmsd;
     int gdb_num_core_regs;
@@ -327,6 +328,34 @@ static inline hwaddr cpu_get_phys_page_debug(CPUState *cpu, vaddr addr)
 #endif
 
 /**
+ * cpu_parse_options:
+ * @cpu: The CPU to set options for.
+ */
+static inline int cpu_parse_options(CPUState *cpu)
+{
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+    Error *err = NULL;
+
+    if (cc->parse_options) {
+        cc->parse_options(cpu, &err);
+        if (err) {
+            return -1;
+        }
+    }
+
+    /* No callback, let arch do it the old way */
+    return 0;
+}
+
+/**
+ * cpu_default_parse_options_func:
+ * The default handler for CPUClass::parse_options
+ * @cpu: the CPU to set option for.
+ * @errp: the handling error descriptor.
+ */
+void cpu_default_parse_options_func(CPUState *cpu, Error **errp);
+
+/**
  * cpu_reset:
  * @cpu: The CPU whose state is to be reset.
  */
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index cd5791e..c6e3ea0 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -190,6 +190,7 @@ char *get_boot_devices_list(size_t *size);
 DeviceState *get_boot_device(uint32_t position);
 
 QemuOpts *qemu_get_machine_opts(void);
+QemuOpts *qemu_get_cpu_opts(void);
 
 bool usb_enabled(bool default_usb);
 
diff --git a/qom/cpu.c b/qom/cpu.c
index 818fb26..231dec5 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -24,6 +24,8 @@
 #include "qemu/notify.h"
 #include "qemu/log.h"
 #include "sysemu/sysemu.h"
+#include "qapi/qmp/qerror.h"
+#include "qemu/config-file.h"
 
 bool cpu_exists(int64_t id)
 {
@@ -186,6 +188,31 @@ void cpu_reset(CPUState *cpu)
     }
 }
 
+static int cpu_set_property(const char *name, const char *value, void *opaque)
+{
+    Error *err = NULL;
+
+    if (strcmp(name, "type") == 0) {
+        return 0;
+    }
+
+    object_property_parse(opaque, value, name, &err);
+    if (err != NULL) {
+        qerror_report_err(err);
+        error_free(err);
+        return -1;
+    }
+
+    return 0;
+}
+
+void cpu_default_parse_options_func(CPUState *cpu, Error **errp)
+{
+    if (qemu_opt_foreach(qemu_get_cpu_opts(), cpu_set_property, cpu, 1)) {
+        error_setg(errp, "Bad option");
+    }
+}
+
 static void cpu_common_reset(CPUState *cpu)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
diff --git a/vl.c b/vl.c
index 4ad15b8..09f8e8b 100644
--- a/vl.c
+++ b/vl.c
@@ -433,6 +433,16 @@ static QemuOptsList qemu_machine_opts = {
     },
 };
 
+static QemuOptsList qemu_cpu_opts = {
+    .name = "cpu",
+    .implied_opt_name = "type",
+    .merge_lists = true,
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_cpu_opts.head),
+    .desc = {
+        { /* End of list */ }
+    },
+};
+
 static QemuOptsList qemu_boot_opts = {
     .name = "boot-opts",
     .implied_opt_name = "order",
@@ -548,6 +558,25 @@ QemuOpts *qemu_get_machine_opts(void)
     return opts;
 }
 
+/**
+ * Get CPU options
+ *
+ * Returns: CPU options (never null).
+ */
+QemuOpts *qemu_get_cpu_opts(void)
+{
+    QemuOptsList *list;
+    QemuOpts *opts;
+
+    list = qemu_find_opts("cpu");
+    assert(list);
+    opts = qemu_opts_find(list, NULL);
+    if (!opts) {
+        opts = qemu_opts_create_nofail(list);
+    }
+    return opts;
+}
+
 const char *qemu_get_vm_name(void)
 {
     return qemu_name;
@@ -2877,6 +2906,7 @@ int main(int argc, char **argv, char **envp)
     qemu_add_opts(&qemu_trace_opts);
     qemu_add_opts(&qemu_option_rom_opts);
     qemu_add_opts(&qemu_machine_opts);
+    qemu_add_opts(&qemu_cpu_opts);
     qemu_add_opts(&qemu_smp_opts);
     qemu_add_opts(&qemu_boot_opts);
     qemu_add_opts(&qemu_sandbox_opts);
@@ -2972,7 +3002,19 @@ int main(int argc, char **argv, char **envp)
             }
             case QEMU_OPTION_cpu:
                 /* hw initialization will check this */
+
+                /* Store cpu_model for old style parser */
                 cpu_model = optarg;
+
+                /*
+                 * Parse and save options in the cpu options list
+                 * for a new parser
+                 */
+                olist = qemu_find_opts("cpu");
+                opts = qemu_opts_parse(olist, optarg, 1);
+                if (!opts) {
+                    exit(1);
+                }
                 break;
             case QEMU_OPTION_hda:
                 {
-- 
1.8.4.rc4

[Qemu-devel] [PATCH v2 2/2] target-ppc: add "compat" CPU option

[Alexey Kardashevskiy]

To be able to boot on newer hardware that the software support,
PowerISA defines a logical PVR, one per every PowerISA specification
version from 2.04.

This adds the "compat" option which takes values 205 or 206 and forces
QEMU to boot the guest with a logical PVR (CPU_POWERPC_LOGICAL_2_05 or
CPU_POWERPC_LOGICAL_2_06).

The guest reads the logical PVR value from "cpu-version" property of
a CPU device node.

Cc: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
Cc: Andreas Färber <afaerber@suse.de>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/ppc/spapr.c              | 13 ++++++++-
 target-ppc/cpu-models.h     | 16 +++++++++++
 target-ppc/cpu.h            |  4 +++
 target-ppc/translate_init.c | 67 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 99 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 7e53a5f..9fcbd96 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -206,6 +206,7 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPREnvironment *spapr)
 
     CPU_FOREACH(cpu) {
         DeviceClass *dc = DEVICE_GET_CLASS(cpu);
+        CPUPPCState *env = &POWERPC_CPU(cpu)->env;
         uint32_t associativity[] = {cpu_to_be32(0x5),
                                     cpu_to_be32(0x0),
                                     cpu_to_be32(0x0),
@@ -238,6 +239,14 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPREnvironment *spapr)
         if (ret < 0) {
             return ret;
         }
+
+        if (env->compat) {
+            ret = fdt_setprop(fdt, offset, "cpu-version",
+                              &env->compat, sizeof(env->compat));
+            if (ret < 0) {
+                return ret;
+            }
+        }
     }
     return ret;
 }
@@ -1101,7 +1110,7 @@ static SaveVMHandlers savevm_htab_handlers = {
 static void ppc_spapr_init(QEMUMachineInitArgs *args)
 {
     ram_addr_t ram_size = args->ram_size;
-    const char *cpu_model = args->cpu_model;
+    const char *cpu_model;
     const char *kernel_filename = args->kernel_filename;
     const char *kernel_cmdline = args->kernel_cmdline;
     const char *initrd_filename = args->initrd_filename;
@@ -1121,6 +1130,8 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
 
     msi_supported = true;
 
+    cpu_model = qemu_opt_get(qemu_get_cpu_opts(), "type");
+
     spapr = g_malloc0(sizeof(*spapr));
     QLIST_INIT(&spapr->phbs);
 
diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
index 49ba4a4..38f38d1 100644
--- a/target-ppc/cpu-models.h
+++ b/target-ppc/cpu-models.h
@@ -583,6 +583,16 @@ enum {
     CPU_POWERPC_RS64II             = 0x00340000,
     CPU_POWERPC_RS64III            = 0x00360000,
     CPU_POWERPC_RS64IV             = 0x00370000,
+
+    /* Logical CPUs */
+    CPU_POWERPC_LOGICAL_MASK       = 0xFFFFFFFF,
+    CPU_POWERPC_LOGICAL_2_04       = 0x0F000001,
+    CPU_POWERPC_LOGICAL_2_05       = 0x0F000002,
+    CPU_POWERPC_LOGICAL_2_06       = 0x0F000003,
+    CPU_POWERPC_LOGICAL_2_06_PLUS  = 0x0F100003,
+    CPU_POWERPC_LOGICAL_2_07       = 0x0F000004,
+    CPU_POWERPC_LOGICAL_2_08       = 0x0F000005,
+
 #endif /* defined(TARGET_PPC64) */
     /* Original POWER */
     /* XXX: should be POWER (RIOS), RSC3308, RSC4608,
@@ -745,4 +755,10 @@ enum {
     POWERPC_SVR_8641D              = 0x80900121,
 };
 
+/* Processor Compatibility Register (PCR) */
+enum {
+    POWERPC_ISA_COMPAT_2_05 = 1ULL << 62,
+    POWERPC_ISA_COMPAT_2_06 = 1ULL << 61,
+};
+
 #endif
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index bb84767..8e30518 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -1006,6 +1006,9 @@ struct CPUPPCState {
     /* Device control registers */
     ppc_dcr_t *dcr_env;
 
+    /* Architecture compatibility mode PVR */
+    uint32_t compat;
+
     int dcache_line_size;
     int icache_line_size;
 
@@ -1330,6 +1333,7 @@ static inline int cpu_mmu_index (CPUPPCState *env)
 #define SPR_BOOKE_DVC1        (0x13E)
 #define SPR_BOOKE_DVC2        (0x13F)
 #define SPR_BOOKE_TSR         (0x150)
+#define SPR_PCR               (0x152)
 #define SPR_BOOKE_TCR         (0x154)
 #define SPR_BOOKE_TLB0PS      (0x158)
 #define SPR_BOOKE_TLB1PS      (0x159)
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index c030a20..b297fb3 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -28,6 +28,7 @@
 #include "mmu-hash32.h"
 #include "mmu-hash64.h"
 #include "qemu/error-report.h"
+#include "qapi/visitor.h"
 
 //#define PPC_DUMP_CPU
 //#define PPC_DEBUG_SPR
@@ -7201,6 +7202,10 @@ static void init_proc_POWER7 (CPUPPCState *env)
                  &spr_read_generic, &spr_write_generic,
                  &spr_read_generic, &spr_write_generic,
                  0x00000000);
+    spr_register(env, SPR_PCR, "PCR",
+                 &spr_read_generic, &spr_write_generic,
+                 &spr_read_generic, &spr_write_generic,
+                 0x00000000);
 #if !defined(CONFIG_USER_ONLY)
     env->slb_nr = 32;
 #endif
@@ -7330,6 +7335,57 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data)
 }
 #endif /* defined (TARGET_PPC64) */
 
+static void powerpc_get_compat(Object *obj, Visitor *v,
+                               void *opaque, const char *name, Error **errp)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(obj);
+    int64_t value = 0;
+
+    switch (cpu->env.compat) {
+    case CPU_POWERPC_LOGICAL_2_05:
+        value = 205;
+        break;
+    case CPU_POWERPC_LOGICAL_2_06:
+        value = 206;
+        break;
+    case 0:
+        break;
+    default:
+        perror("Unsupported mode, only are 205, 206 supported\n");
+        break;
+    }
+
+    visit_type_int(v, &value, name, errp);
+}
+
+static void powerpc_set_compat(Object *obj, Visitor *v,
+                               void *opaque, const char *name, Error **errp)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(obj);
+    Error *error = NULL;
+    int64_t value;
+
+    visit_type_int(v, &value, name, &error);
+    if (error) {
+        error_propagate(errp, error);
+        return;
+    }
+
+    switch (value) {
+    case 0:
+        cpu->env.compat = 0;
+        break;
+    case 205:
+        cpu->env.compat = CPU_POWERPC_LOGICAL_2_05;
+        break;
+    case 206:
+        cpu->env.compat = CPU_POWERPC_LOGICAL_2_06;
+        break;
+    default:
+        perror("Unsupported mode, only are 205, 206 supported\n");
+        return;
+    }
+}
 
 /*****************************************************************************/
 /* Generic CPU instantiation routine                                         */
@@ -7497,6 +7553,12 @@ static void init_ppc_proc(PowerPCCPU *cpu)
                 "registered.\n"
                 " Attempt QEMU to crash very soon !\n");
     }
+
+    if (env->spr_cb[SPR_PCR].oea_read) {
+        object_property_add(OBJECT(cpu), "compat", "int",
+                            powerpc_get_compat, powerpc_set_compat,
+                            NULL, NULL, NULL);
+    }
 }
 
 #if defined(PPC_DUMP_CPU)
@@ -8376,6 +8438,10 @@ PowerPCCPU *cpu_ppc_init(const char *cpu_model)
         return NULL;
     }
 
+    if (cpu_parse_options(CPU(cpu))) {
+        return NULL;
+    }
+
     return cpu;
 }
 
@@ -8674,6 +8740,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
 #endif
 
     dc->fw_name = "PowerPC,UNKNOWN";
+    cc->parse_options = cpu_default_parse_options_func;
 }
 
 static const TypeInfo ppc_cpu_type_info = {
-- 
1.8.4.rc4

Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option

[Andreas Färber]

Am 08.11.2013 09:22, schrieb Alexey Kardashevskiy:
> On 11/08/2013 12:36 AM, Igor Mammedov wrote:
>> On Thu,  7 Nov 2013 20:11:51 +1100
>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>
>>> On 11/06/2013 12:53 AM, Andreas Färber wrote:> Am 05.11.2013 10:52, schrieb Paolo Bonzini:
>>>>> Il 05/11/2013 10:16, Alexander Graf ha scritto:
>>>>>>
>>>>>> On 05.11.2013, at 10:06, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>>>
>>>>>>> Il 30/09/2013 14:57, Alexey Kardashevskiy ha scritto:
>>>>>>>>>> Why is the option under -machine instead of -cpu?
>>>>>>>> Because it is still the same CPU and the guest will still read the real
>>>>>>>> PVR from the hardware (which it may not support but this is why we need
>>>>>>>> compatibility mode).
>>>>>>>
>>>>>>> How do you support migration from a newer to an older CPU then?  I think
>>>>>>> the guest should never see anything about the hardware CPU model.
>>>>>>
>>>>>> POWER can't model that. It always leaks the host CPU information into the guest. It's the guest kernel's responsibility to not expose that change to user space.
>>>>>>
>>>>>> Yes, it's broken :). I'm not even sure there is any sensible way to do live migration between different CPU types.
>>>>>
>>>>> Still in my opinion it should be "-cpu", not "-machine".  Even if it's
>>>>> just a "virtual" CPU model.
>>>>
>>>> PowerPC currently does not have -cpu option parsing. If you need to
>>>> implement it, I would ask for a generic hook in CPUClass set by
>>>> TYPE_POWERPC_CPU, so that the logic does not get hardcoded in cpu_init,
>>>> and for the p=v parsing logic to be so generic as to just set property p
>>>> to value v on the CPU instance. I.e. please make the compatibility
>>>> settings a static property or dynamic property of the CPU.
>>>>
>>>> Maybe the parsing code could even live in generic qom/cpu.c, overridden
>>>> by x86/sparc and reused for arm? Somewhere down my to-do list but
>>>> patches appreciated...
>>>
>>>
>>> I spent some time today trying to digest what you said, still having problems
>>> with understanding of what you meant and what Igor meant about global variables
>>> (I just do not see the point in them).
>>>
>>> Below is the result of my excercise. At the moment I would just like to know
>>> if I am going in the right direction or not.
>>
>> what I've had in mind was a bit simpler and more implicit approach instead of
>> setting properties on each CPU instance explicitly. It could done using
>> existing "global properties" mechanism.
>>
>> in current code -cpu type,foo1=x,foo2=y... are saved into cpu_model string
>> which is parsed by target specific cpu_init() effectively parsing cpu_model
>> each time when creating a CPU.
>>
>> So to avoid fixing every target I suggest to leave cpu_model be as it is and
>>
>> add translation hook that will convert "type,foo1=x,foo2=y..." virtually
>> into a set of following options:
>>  "-global type.foo1=x -global type.foo2=y ..."
>> these options when registered are transparently applied to each new CPU
>> instance (see device_post_init() for details).
>>
>> now why do we need translation hook,
>>  * not every target is able to handle "type,foo1=x,foo2=y" in terms of
>>    properties yet
>>  * legacy feature string might be in non canonical format, like
>>    "+foo1,-foo2..." so for compatibility reasons with CLI we need target
>>    specific code to convert to canonical format when it becomes available.
>>  * for targets that don't have feature string handling and implementing
>>    new features as properties we can implement generic default hook that
>>    will convert canonical feature string into global properties.
>>
>> as result we eventually would be able drop cpu_model and use global
>> properties to store CPU features.
> 
> 
> What is wrong doing it in the way the "-machine" switch does it now?
> qemu_get_machine_opts() returns a global list which I can iterate through
> via qemu_opt_foreach() and set every property to a CPU, this will check if
> a property exists and assigns it => happy Aik :)

You are happy w/ ppc, but you would make x86 and sparc users unhappy! ;)
QemuOpts does not support the +feature,-feature notation, just [key=]value.

>> see comments below for pseudo code:
>>> And few question while we are here:
>>> 1. the proposed common code handles both static and dynamic properties.
>>> What is the current QEMU trend about choosing static vs. dynamic? Can do
>>> both in POWERPC, both have benifits.
>> I prefer static, since it's usually less boilerplate code.
>>
>>
>> [...]
>>
>>>
>>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>>> index 7739e00..a17cd73 100644
>>> --- a/include/qom/cpu.h
>>> +++ b/include/qom/cpu.h
>>> @@ -327,6 +327,12 @@ static inline hwaddr cpu_get_phys_page_debug(CPUState *cpu, vaddr addr)
>>>  #endif
>>>  
>>>  /**
>>> + * cpu_common_set_properties:
>>> + * @cpu: The CPU whose properties to initialize from the command line.
>>> + */
>>> +int cpu_common_set_properties(Object *obj);
>>
>> cpu_translate_features_compat(classname, cpu_model_str)
> 
> 
> Here I lost you. I am looking to a generic way of adding any number of
> properties to "-cpu", not just "compat".

That's just naming and doesn't rule each other out, important is the
string argument that you pass in.

>>> diff --git a/qom/cpu.c b/qom/cpu.c
>>> index 818fb26..6516c63 100644
>>> --- a/qom/cpu.c
>>> +++ b/qom/cpu.c
>>> @@ -24,6 +24,8 @@
>>>  #include "qemu/notify.h"
>>>  #include "qemu/log.h"
>>>  #include "sysemu/sysemu.h"
>>> +#include "hw/qdev-properties.h"
>>> +#include "qapi/qmp/qerror.h"
>>>  
>>>  bool cpu_exists(int64_t id)
>>>  {
>>> @@ -186,6 +188,28 @@ void cpu_reset(CPUState *cpu)
>>>      }
>>>  }
>>>  
>>> +static int cpu_set_property(const char *name, const char *value, void *opaque)
>>> +{
>>> +    DeviceState *dev = opaque;
>>> +    Error *err = NULL;
>>> +
>>> +    if (strcmp(name, "type") == 0)
>>> +        return 0;
>>> +
>>> +    qdev_prop_parse(dev, name, value, &err);
>>> +    if (err != NULL) {
>>> +        qerror_report_err(err);
>>> +        error_free(err);
>>> +        return -1;
>>> +    }
>>> +    return 0;
>>> +}
>>> +
>>> +int cpu_common_set_properties(Object *obj)
>>> +{
>>> +    return qemu_opt_foreach(qemu_get_cpu_opts(), cpu_set_property, obj, 1);
>>> +}
>> replace ^^^ with:
>>
>> void cpu_translate_features_compat(classname, cpu_model_str) {
>>     for_each_foo in cpu_model_str {
>>         qdev_prop_register_global(classname.foo=val)
>>     }
>> }
>>
>> and set default hook to NULL for every target that does custom
>> parsing (i.e. x86/sparc) so hook will be NOP there.
>>
>>
>>> diff --git a/vl.c b/vl.c
>>> index d5c5d8c..a5fbc38 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -437,6 +437,16 @@ static QemuOptsList qemu_machine_opts = {
>>>      },
>>>  };
>>>  
>>
>>>              case QEMU_OPTION_cpu:
>>>                  /* hw initialization will check this */
>>
>> cpu_model = optarg;
>> classname = GET_CLASS_NAME_FROM_TYPE(cpu_model) <= not sure how
>> implement it since naming is target specific, maybe Andreas has an idea
> 
> Heh. We cannot do this as on ppc we have to use ppc_cpu_class_by_name() and
> we definitely do not want to call it from vl.c.
> 
> 
> Thanks for comments but I'll try what Andreas suggested if I understood it
> all right and that suggestion is any different from yours :)

Actually it was more or less the same suggestion in parallel, modulo
whether to set properties on the instance (easier) or via global list.

In cpu_ppc_init() you have access to the instance and can use
object_get_typename(OBJECT(cpu)) to obtain the string class name to use.
Using a more specific type name than where the properties are defined
should work just okay, only the opposite wouldn't work. But it's gonna
be easier for you if you take one step a time. :)

When I am finally through with review of Igor's patches then he can
implement that for x86 and we/you can copy or adapt it for ppc. No need
to do big experiments for a concretely needed ppc feature.

Cheers,
Andreas

>> CPUClass = get_cpu_class_by_name(classname)
>> class->cpu_translate_features_compat(classname, cpu_model_str)
>>
>>>                  break;
>>>              case QEMU_OPTION_hda:
>>>                  {

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v2 0/2] spapr: add "compat" machine option

[Andreas Färber]

Am 08.11.2013 09:22, schrieb Alexey Kardashevskiy:
> On 11/08/2013 01:01 AM, Andreas Färber wrote:
>> Please do separate these global preparations from the actual new ppc
>> property. Elsewhere it was discussed whether to use a readable string
>> value, which might hint at a dynamic property of type string or maybe
>> towards an enum (/me no experience with those yet and whether that works
>> better with dynamic or static).
> 
> 
> Ufff... I did not get the part about a hint. Everything is string
> in the command line :)

Your "compat" and "compat1" properties in the previous patch were of
type int. :)

The question I raised was how to best implement Alex' suggestion of
using literal "power6" rather than 206 as value.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option

[Alexey Kardashevskiy]

On 11/09/2013 12:20 AM, Andreas Färber wrote:

> When I am finally through with review of Igor's patches then he can
> implement that for x86 and we/you can copy or adapt it for ppc. No need
> to do big experiments for a concretely needed ppc feature.

Does it mean I better stop wasting your time and relax and you guys do this
-cpu subptions thing yourselves? Thanks.


-- 
Alexey

Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option

[Andreas Färber]

Am 08.11.2013 15:57, schrieb Alexey Kardashevskiy:
> On 11/09/2013 12:20 AM, Andreas Färber wrote:
> 
>> When I am finally through with review of Igor's patches then he can
>> implement that for x86 and we/you can copy or adapt it for ppc. No need
>> to do big experiments for a concretely needed ppc feature.
> 
> Does it mean I better stop wasting your time and relax and you guys do this
> -cpu subptions thing yourselves? Thanks.

No, I meant don't try to mess with translating -cpu to -global and
looking up the class name that Igor mentioned. (Depending on when that
is being done it may introduce subtle bugs like real -global being
overridden.)

If you wait for us then you'll wait for some time...

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg