From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43391) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VQenU-0008AQ-1D for qemu-devel@nongnu.org; Mon, 30 Sep 2013 10:50:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VQenO-0007Ya-Ji for qemu-devel@nongnu.org; Mon, 30 Sep 2013 10:50:03 -0400 Message-ID: <52498F90.6020309@suse.de> Date: Mon, 30 Sep 2013 16:49:52 +0200 From: Alexander Graf MIME-Version: 1.0 References: <1380269200-8194-1-git-send-email-aik@ozlabs.ru> <67325DB7-EF06-49CD-BA79-6B44BCC20140@suse.de> <52497B00.6030706@ozlabs.ru> In-Reply-To: <52497B00.6030706@ozlabs.ru> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: Nikunj A Dadhania , qemu-devel@nongnu.org, qemu-ppc@nongnu.org, Paul Mackerras , =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= 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 force= s >>> QEMU to boot the guest with a logical PVR (CPU_POWERPC_LOGICAL_2_05 o= r >>> 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 >>> Cc: Andreas F=C3=A4rber >>> Signed-off-by: Alexey Kardashevskiy >>> --- >>> 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_server= s, int nr_irqs) >>> return icp; >>> } >>> >>> +static void spapr_compat_mode_init(sPAPREnvironment *spapr) >>> +{ >>> + QemuOpts *machine_opts =3D qemu_get_machine_opts(); >>> + uint64_t compat =3D qemu_opt_get_number(machine_opts, "compat", = 0); >>> + >>> + switch (compat) { >>> + case 0: >>> + break; >>> + case 205: >>> + spapr->arch_compat =3D CPU_POWERPC_LOGICAL_2_05; >>> + break; >>> + case 206: >>> + spapr->arch_compat =3D 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=3Dpower6" is >> easier to understand for a user than "-machine compat=3D205". > I just follow the PowerISA spec. It does not say anywhere (at least I d= o > not see it) that 2.05=3D=3Dpower6. 2.05 was released when power6 was > released and power6 supports 2.05 but these are not synonims. And > "compat=3Dpower6" 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 -=20 the ISA defines a lot more capabilities than a single core necessarily=20 supports, especially with the inclusion of booke into the generic ppc spe= c. > > > >>> + break; >>> + default: >>> + perror("Unsupported mode, only are 205, 206 supported\n"); >>> + break; >>> + } >>> +} >>> + >>> static int spapr_fixup_cpu_dt(void *fdt, sPAPREnvironment *spapr) >>> { >>> int ret =3D 0, offset; >>> @@ -206,6 +227,7 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPREnv= ironment *spapr) >>> >>> CPU_FOREACH(cpu) { >>> DeviceClass *dc =3D DEVICE_GET_CLASS(cpu); >>> + CPUPPCState *env =3D&(POWERPC_CPU(cpu)->env); >>> uint32_t associativity[] =3D {cpu_to_be32(0x5), >>> cpu_to_be32(0x0), >>> cpu_to_be32(0x0), >>> @@ -238,6 +260,14 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPREn= vironment *spapr) >>> if (ret< 0) { >>> return ret; >>> } >>> + >>> + if (env->arch_compat) { >>> + ret =3D 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 =3D g_malloc0(sizeof(*spapr)); >>> QLIST_INIT(&spapr->phbs); >>> >>> + spapr_compat_mode_init(spapr); >>> + >>> cpu_ppc_hypercall =3D 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 =3D 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 ca= n > 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 ca= n > 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 th= e > 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 w= e > did not disable some feature (really old guest will not use it and > that's it) 2) once created, PowerPCCPUState (or whatever the exact nam= e > 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=20 that we're currently not honoring them? Then fron the machine code point=20 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=20 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 =3D 0x00340000, >>> CPU_POWERPC_RS64III =3D 0x00360000, >>> CPU_POWERPC_RS64IV =3D 0x00370000, >>> + >>> + /* Logical CPUs */ >>> + CPU_POWERPC_LOGICAL_MASK =3D 0xFFFFFFFF, >>> + CPU_POWERPC_LOGICAL_2_04 =3D 0x0F000001, >>> + CPU_POWERPC_LOGICAL_2_05 =3D 0x0F000002, >>> + CPU_POWERPC_LOGICAL_2_06 =3D 0x0F000003, >>> + CPU_POWERPC_LOGICAL_2_06_PLUS =3D 0x0F100003, >>> + CPU_POWERPC_LOGICAL_2_07 =3D 0x0F000004, >>> + CPU_POWERPC_LOGICAL_2_08 =3D 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=20 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=20 compatibility setting or does that get reset as well? Alex