Re: [Qemu-devel] [PATCH] arm/virt: Use PSCI v0.2 function IDs in the DT when KVM uses PSCI v0.2

[Christoffer Dall]

On Thu, Aug 07, 2014 at 07:06:12PM +0100, Peter Maydell wrote:
> On 7 August 2014 17:30, Christoffer Dall <address@hidden> wrote:
> > The current code supplies the PSCI v0.1 funciton IDs in the DT even when
> 
> "function"
> 
> > KVM uses PSCI v0.2.
> >
> > This will break guest kernels that only support PSCI v0.1 as they will
> > use the IDs provided in the DT.  Guest kernels with PSCI v0.2 support
> > are not affected by this patch, because they ignore the function IDs in
> > the device tree and rely on the architecture definition.
> >
> > Define QEMU versions of the constants and check that they correspond to
> > the Linux defines on Linux build hosts.  After this patch, both guest
> > kernels with PSCI v0.1 support and guest kernels with PSCI v0.2 should
> > work.
> >
> > Tested on TC2 for 32-bit and APM Mustang for 64-bit (aarch64 guest
> > only).  Both cases tested with 3.14 and linus/master and verified I
> > could bring up 2 cpus with both guest kernels.
> >
> > Signed-off-by: Christoffer Dall <address@hidden>
> > ---
> >  hw/arm/virt.c           | 25 ++++++++++++++++++++++---
> >  target-arm/kvm-consts.h | 23 +++++++++++++++++++++++
> >  2 files changed, 45 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index ba94298..4e882bc 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -194,20 +194,39 @@ static void fdt_add_psci_node(const VirtBoardInfo 
> > *vbi)
> >
> >      /* No PSCI for TCG yet */
> >      if (kvm_enabled()) {
> > +        uint32_t cpu_off_fn;
> > +        uint32_t cpu_on_fn;
> > +        uint32_t migrate_fn;
> > +
> >          qemu_fdt_add_subnode(fdt, "/psci");
> >          if (armcpu->psci_version == 2) {
> >              const char comp[] = "arm,psci-0.2\0arm,psci";
> >              qemu_fdt_setprop(fdt, "/psci", "compatible", comp, 
> > sizeof(comp));
> > +
> > +            if (arm_feature(&armcpu->env, ARM_FEATURE_AARCH64)) {
> > +                cpu_off_fn = QEMU_PSCI_0_2_FN_CPU_OFF;
> > +                cpu_on_fn = QEMU_PSCI_0_2_FN64_CPU_ON;
> > +                migrate_fn = QEMU_PSCI_0_2_FN64_MIGRATE;
> > +            } else {
> > +                cpu_off_fn = QEMU_PSCI_0_2_FN_CPU_OFF;
> > +                cpu_on_fn = QEMU_PSCI_0_2_FN_CPU_ON;
> > +                migrate_fn = QEMU_PSCI_0_2_FN_MIGRATE;
> > +            }
> >          } else {
> >              qemu_fdt_setprop_string(fdt, "/psci", "compatible", 
> > "arm,psci");
> > +
> > +            cpu_off_fn = PSCI_FN_CPU_OFF;
> > +            cpu_on_fn = PSCI_FN_CPU_ON;
> > +            migrate_fn = PSCI_FN_MIGRATE;
> 
> 
> I see that linux/psci.h defines PSCI_0_2_FN64_CPU_SUSPEND
> and PSCI_0_2_FN_CPU_SUSPEND, neither of which are
> the same as PSCI_FN_CPU_SUSPEND. Shouldn't we be
> using the 0.2 values for SUSPEND too?

we should, I'm an idiot.

> 
> (Sharing the CPU_OFF value is correct by the spec I see,
> but it would probably be better outside the if() and with a comment,
> since it otherwise looks a lot like an error.)

good point, fixed for v2.  Will give it a quick test tomorrow and then
send it out.

-Christoffer

> 
> >          }
> >
> >          qemu_fdt_setprop_string(fdt, "/psci", "method", "hvc");
> >          qemu_fdt_setprop_cell(fdt, "/psci", "cpu_suspend",
> >                                    PSCI_FN_CPU_SUSPEND);
> > -        qemu_fdt_setprop_cell(fdt, "/psci", "cpu_off", PSCI_FN_CPU_OFF);
> > -        qemu_fdt_setprop_cell(fdt, "/psci", "cpu_on", PSCI_FN_CPU_ON);
> > -        qemu_fdt_setprop_cell(fdt, "/psci", "migrate", PSCI_FN_MIGRATE);
> > +
> > +        qemu_fdt_setprop_cell(fdt, "/psci", "cpu_off", cpu_off_fn);
> > +        qemu_fdt_setprop_cell(fdt, "/psci", "cpu_on", cpu_on_fn);
> > +        qemu_fdt_setprop_cell(fdt, "/psci", "migrate", migrate_fn);
> >      }
> >  }
> >
> > diff --git a/target-arm/kvm-consts.h b/target-arm/kvm-consts.h
> > index 6009a33..a8df135 100644
> > --- a/target-arm/kvm-consts.h
> > +++ b/target-arm/kvm-consts.h
> > @@ -17,6 +17,7 @@
> >  #ifdef CONFIG_KVM
> >  #include "qemu/compiler.h"
> >  #include <linux/kvm.h>
> > +#include <linux/psci.h>
> >
> >  #define MISMATCH_CHECK(X, Y) QEMU_BUILD_BUG_ON(X != Y)
> >
> > @@ -50,6 +51,28 @@ MISMATCH_CHECK(PSCI_FN_CPU_OFF, KVM_PSCI_FN_CPU_OFF)
> >  MISMATCH_CHECK(PSCI_FN_CPU_ON, KVM_PSCI_FN_CPU_ON)
> >  MISMATCH_CHECK(PSCI_FN_MIGRATE, KVM_PSCI_FN_MIGRATE)
> >
> > +#define QEMU_PSCI_0_2_FN_BASE 0x84000000
> > +#define QEMU_PSCI_0_2_FN(n) (QEMU_PSCI_0_2_FN_BASE + (n))
> > +
> > +#define QEMU_PSCI_0_2_64BIT 0x40000000
> > +#define QEMU_PSCI_0_2_FN64_BASE \
> > +        (QEMU_PSCI_0_2_FN_BASE + QEMU_PSCI_0_2_64BIT)
> > +#define QEMU_PSCI_0_2_FN64(n) (QEMU_PSCI_0_2_FN64_BASE + (n))
> > +
> > +#define QEMU_PSCI_0_2_FN_CPU_OFF QEMU_PSCI_0_2_FN(2)
> > +#define QEMU_PSCI_0_2_FN_CPU_ON QEMU_PSCI_0_2_FN(3)
> > +#define QEMU_PSCI_0_2_FN_MIGRATE QEMU_PSCI_0_2_FN(5)
> > +
> > +#define QEMU_PSCI_0_2_FN64_CPU_OFF QEMU_PSCI_0_2_FN64(2)
> > +#define QEMU_PSCI_0_2_FN64_CPU_ON QEMU_PSCI_0_2_FN64(3)
> > +#define QEMU_PSCI_0_2_FN64_MIGRATE QEMU_PSCI_0_2_FN64(5)
> > +
> > +MISMATCH_CHECK(QEMU_PSCI_0_2_FN_CPU_OFF, PSCI_0_2_FN_CPU_OFF)
> > +MISMATCH_CHECK(QEMU_PSCI_0_2_FN_CPU_ON, PSCI_0_2_FN_CPU_ON)
> > +MISMATCH_CHECK(QEMU_PSCI_0_2_FN_MIGRATE, PSCI_0_2_FN_MIGRATE)
> > +MISMATCH_CHECK(QEMU_PSCI_0_2_FN64_CPU_ON, PSCI_0_2_FN64_CPU_ON)
> > +MISMATCH_CHECK(QEMU_PSCI_0_2_FN64_MIGRATE, PSCI_0_2_FN64_MIGRATE)
> > +
> 
> This now clashes awkwardly with the way we previously had
> PSCI_* for the QEMU versions and KVM_PSCI_* for the
> kernel header versions, because the kernel's moved over
> to PSCI_*. We should probably (in a separate patch) rename
> the PSCI_FN* constants in kvm-consts.h to QEMU_PSCI_0_1_*.
> 

Yep, will send out two patches in the morning.

Thanks!
-Christoffer