On 05.11.2013, at 03:19, Alexey Kardashevskiy <a...@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<nik...@linux.vnet.ibm.com>
>>>>> Cc: Andreas Färber<afaer...@suse.de>
>>>>> Signed-off-by: Alexey Kardashevskiy<a...@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
