Re: [Qemu-ppc] [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<address@hidden>
>>>> Cc: Andreas Färber<address@hidden>
>>>> Signed-off-by: Alexey Kardashevskiy<address@hidden>
>>>> ---
>>>> 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