| Message ID | 1409044926-3744-5-git-send-email-anup.patel@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Anup, On 26/08/14 10:22, Anup Patel wrote: > If in-kernel KVM support PSCI-0.2 emulation then we should set > KVM_ARM_VCPU_PSCI_0_2 feature for each guest VCPU and also > provide "arm,psci-0.2","arm,psci" as PSCI compatible string. > > This patch updates kvm_cpu__arch_init() and setup_fdt() as > per above. > > Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> > Signed-off-by: Anup Patel <anup.patel@linaro.org> > --- > tools/kvm/arm/fdt.c | 39 +++++++++++++++++++++++++++++++++------ > tools/kvm/arm/kvm-cpu.c | 5 +++++ > 2 files changed, 38 insertions(+), 6 deletions(-) > > diff --git a/tools/kvm/arm/fdt.c b/tools/kvm/arm/fdt.c > index 186a718..93849cf2 100644 > --- a/tools/kvm/arm/fdt.c > +++ b/tools/kvm/arm/fdt.c > @@ -13,6 +13,7 @@ > #include <linux/byteorder.h> > #include <linux/kernel.h> > #include <linux/sizes.h> > +#include <linux/psci.h> > > static char kern_cmdline[COMMAND_LINE_SIZE]; > > @@ -162,12 +163,38 @@ static int setup_fdt(struct kvm *kvm) > > /* PSCI firmware */ > _FDT(fdt_begin_node(fdt, "psci")); > - _FDT(fdt_property_string(fdt, "compatible", "arm,psci")); > - _FDT(fdt_property_string(fdt, "method", "hvc")); > - _FDT(fdt_property_cell(fdt, "cpu_suspend", KVM_PSCI_FN_CPU_SUSPEND)); > - _FDT(fdt_property_cell(fdt, "cpu_off", KVM_PSCI_FN_CPU_OFF)); > - _FDT(fdt_property_cell(fdt, "cpu_on", KVM_PSCI_FN_CPU_ON)); > - _FDT(fdt_property_cell(fdt, "migrate", KVM_PSCI_FN_MIGRATE)); > + if (kvm__supports_extension(kvm, KVM_CAP_ARM_PSCI_0_2)) { > + const char compatible[] = "arm,psci-0.2\0arm,psci"; > + _FDT(fdt_property(fdt, "compatible", > + compatible, sizeof(compatible))); > + _FDT(fdt_property_string(fdt, "method", "hvc")); > + if (kvm->cfg.arch.aarch32_guest) { > + _FDT(fdt_property_cell(fdt, "cpu_suspend", > + PSCI_0_2_FN_CPU_SUSPEND)); > + _FDT(fdt_property_cell(fdt, "cpu_off", > + PSCI_0_2_FN_CPU_OFF)); > + _FDT(fdt_property_cell(fdt, "cpu_on", > + PSCI_0_2_FN_CPU_ON)); > + _FDT(fdt_property_cell(fdt, "migrate", > + PSCI_0_2_FN_MIGRATE)); > + } else { > + _FDT(fdt_property_cell(fdt, "cpu_suspend", > + PSCI_0_2_FN64_CPU_SUSPEND)); > + _FDT(fdt_property_cell(fdt, "cpu_off", > + PSCI_0_2_FN_CPU_OFF)); > + _FDT(fdt_property_cell(fdt, "cpu_on", > + PSCI_0_2_FN64_CPU_ON)); > + _FDT(fdt_property_cell(fdt, "migrate", > + PSCI_0_2_FN64_MIGRATE)); > + } > + } else { > + _FDT(fdt_property_string(fdt, "compatible", "arm,psci")); > + _FDT(fdt_property_string(fdt, "method", "hvc")); > + _FDT(fdt_property_cell(fdt, "cpu_suspend", KVM_PSCI_FN_CPU_SUSPEND)); > + _FDT(fdt_property_cell(fdt, "cpu_off", KVM_PSCI_FN_CPU_OFF)); > + _FDT(fdt_property_cell(fdt, "cpu_on", KVM_PSCI_FN_CPU_ON)); > + _FDT(fdt_property_cell(fdt, "migrate", KVM_PSCI_FN_MIGRATE)); > + } I guess this could be simplified much by defining three arrays with the respective function IDs and setting a pointer to the right one here. Then there would still be only one set of _FDT() calls, which reference this pointer. Like: uint32_t *psci_fn_ids; ... if (KVM_CAP_ARM_PSCI_0_2) { if (aarch32_guest) psci_fn_ids = psci_0_2_fn_ids; else psci_fn_ids = psci_0_2_fn64_ids; } else psci_fn_ids = psci_0_1_fn_ids; _FDT(fdt_property_cell(fdt, "cpu_suspend", psci_fn_ids[0])); _FDT(fdt_property_cell(fdt, "cpu_off", psci_fn_ids[1])); ... Also I wonder if we actually need those different IDs. The binding doc says that Linux' PSCI 0.2 code ignores them altogether, only using them if the "arm,psci" branch of the compatible string is actually used (on kernels not supporting PSCI 0.2) So can't we just always pass the PSCI 0.1 numbers in here? That would restrict this whole patch to just changing the compatible string, right? Regards, Andre. > _FDT(fdt_end_node(fdt)); > > /* Finalise. */ > diff --git a/tools/kvm/arm/kvm-cpu.c b/tools/kvm/arm/kvm-cpu.c > index c010e9c..0637e9a 100644 > --- a/tools/kvm/arm/kvm-cpu.c > +++ b/tools/kvm/arm/kvm-cpu.c > @@ -56,6 +56,11 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id) > if (vcpu->kvm_run == MAP_FAILED) > die("unable to mmap vcpu fd"); > > + /* Set KVM_ARM_VCPU_PSCI_0_2 if available */ > + if (kvm__supports_extension(kvm, KVM_CAP_ARM_PSCI_0_2)) { > + vcpu_init.features[0] |= (1UL << KVM_ARM_VCPU_PSCI_0_2); > + } > + > /* > * If preferred target ioctl successful then use preferred target > * else try each and every target type. > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andre, On 29 August 2014 14:41, Andre Przywara <andre.przywara@arm.com> wrote: > Hi Anup, > > On 26/08/14 10:22, Anup Patel wrote: >> If in-kernel KVM support PSCI-0.2 emulation then we should set >> KVM_ARM_VCPU_PSCI_0_2 feature for each guest VCPU and also >> provide "arm,psci-0.2","arm,psci" as PSCI compatible string. >> >> This patch updates kvm_cpu__arch_init() and setup_fdt() as >> per above. >> >> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> >> Signed-off-by: Anup Patel <anup.patel@linaro.org> >> --- >> tools/kvm/arm/fdt.c | 39 +++++++++++++++++++++++++++++++++------ >> tools/kvm/arm/kvm-cpu.c | 5 +++++ >> 2 files changed, 38 insertions(+), 6 deletions(-) >> >> diff --git a/tools/kvm/arm/fdt.c b/tools/kvm/arm/fdt.c >> index 186a718..93849cf2 100644 >> --- a/tools/kvm/arm/fdt.c >> +++ b/tools/kvm/arm/fdt.c >> @@ -13,6 +13,7 @@ >> #include <linux/byteorder.h> >> #include <linux/kernel.h> >> #include <linux/sizes.h> >> +#include <linux/psci.h> >> >> static char kern_cmdline[COMMAND_LINE_SIZE]; >> >> @@ -162,12 +163,38 @@ static int setup_fdt(struct kvm *kvm) >> >> /* PSCI firmware */ >> _FDT(fdt_begin_node(fdt, "psci")); >> - _FDT(fdt_property_string(fdt, "compatible", "arm,psci")); >> - _FDT(fdt_property_string(fdt, "method", "hvc")); >> - _FDT(fdt_property_cell(fdt, "cpu_suspend", KVM_PSCI_FN_CPU_SUSPEND)); >> - _FDT(fdt_property_cell(fdt, "cpu_off", KVM_PSCI_FN_CPU_OFF)); >> - _FDT(fdt_property_cell(fdt, "cpu_on", KVM_PSCI_FN_CPU_ON)); >> - _FDT(fdt_property_cell(fdt, "migrate", KVM_PSCI_FN_MIGRATE)); >> + if (kvm__supports_extension(kvm, KVM_CAP_ARM_PSCI_0_2)) { >> + const char compatible[] = "arm,psci-0.2\0arm,psci"; >> + _FDT(fdt_property(fdt, "compatible", >> + compatible, sizeof(compatible))); >> + _FDT(fdt_property_string(fdt, "method", "hvc")); >> + if (kvm->cfg.arch.aarch32_guest) { >> + _FDT(fdt_property_cell(fdt, "cpu_suspend", >> + PSCI_0_2_FN_CPU_SUSPEND)); >> + _FDT(fdt_property_cell(fdt, "cpu_off", >> + PSCI_0_2_FN_CPU_OFF)); >> + _FDT(fdt_property_cell(fdt, "cpu_on", >> + PSCI_0_2_FN_CPU_ON)); >> + _FDT(fdt_property_cell(fdt, "migrate", >> + PSCI_0_2_FN_MIGRATE)); >> + } else { >> + _FDT(fdt_property_cell(fdt, "cpu_suspend", >> + PSCI_0_2_FN64_CPU_SUSPEND)); >> + _FDT(fdt_property_cell(fdt, "cpu_off", >> + PSCI_0_2_FN_CPU_OFF)); >> + _FDT(fdt_property_cell(fdt, "cpu_on", >> + PSCI_0_2_FN64_CPU_ON)); >> + _FDT(fdt_property_cell(fdt, "migrate", >> + PSCI_0_2_FN64_MIGRATE)); >> + } >> + } else { >> + _FDT(fdt_property_string(fdt, "compatible", "arm,psci")); >> + _FDT(fdt_property_string(fdt, "method", "hvc")); >> + _FDT(fdt_property_cell(fdt, "cpu_suspend", KVM_PSCI_FN_CPU_SUSPEND)); >> + _FDT(fdt_property_cell(fdt, "cpu_off", KVM_PSCI_FN_CPU_OFF)); >> + _FDT(fdt_property_cell(fdt, "cpu_on", KVM_PSCI_FN_CPU_ON)); >> + _FDT(fdt_property_cell(fdt, "migrate", KVM_PSCI_FN_MIGRATE)); >> + } > > I guess this could be simplified much by defining three arrays with the > respective function IDs and setting a pointer to the right one here. > Then there would still be only one set of _FDT() calls, which reference > this pointer. Like: > uint32_t *psci_fn_ids; > ... > if (KVM_CAP_ARM_PSCI_0_2) { > if (aarch32_guest) > psci_fn_ids = psci_0_2_fn_ids; > else > psci_fn_ids = psci_0_2_fn64_ids; > } else > psci_fn_ids = psci_0_1_fn_ids; > _FDT(fdt_property_cell(fdt, "cpu_suspend", psci_fn_ids[0])); > _FDT(fdt_property_cell(fdt, "cpu_off", psci_fn_ids[1])); > ... > > Also I wonder if we actually need those different IDs. The binding doc > says that Linux' PSCI 0.2 code ignores them altogether, only using them > if the "arm,psci" branch of the compatible string is actually used (on > kernels not supporting PSCI 0.2) Your suggestion looks good to me. I will rework this patch accordingly. > So can't we just always pass the PSCI 0.1 numbers in here? > That would restrict this whole patch to just changing the compatible > string, right? If we always pass PSCI 0.1 numbers irrespective to compatible string then it breaks the case where we have latest host kernel with PSCI 0.2, latest KVMTOOL with PSCI 0.2, and older guest kernel with only PSCI 0.1 support. There was a issue in QEMU and Christoffer had send-out a patch to fix this in QEMU. (Refer, https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg01311.html) Regards, Anup > > Regards, > Andre. > >> _FDT(fdt_end_node(fdt)); >> >> /* Finalise. */ >> diff --git a/tools/kvm/arm/kvm-cpu.c b/tools/kvm/arm/kvm-cpu.c >> index c010e9c..0637e9a 100644 >> --- a/tools/kvm/arm/kvm-cpu.c >> +++ b/tools/kvm/arm/kvm-cpu.c >> @@ -56,6 +56,11 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id) >> if (vcpu->kvm_run == MAP_FAILED) >> die("unable to mmap vcpu fd"); >> >> + /* Set KVM_ARM_VCPU_PSCI_0_2 if available */ >> + if (kvm__supports_extension(kvm, KVM_CAP_ARM_PSCI_0_2)) { >> + vcpu_init.features[0] |= (1UL << KVM_ARM_VCPU_PSCI_0_2); >> + } >> + >> /* >> * If preferred target ioctl successful then use preferred target >> * else try each and every target type. >> -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/tools/kvm/arm/fdt.c b/tools/kvm/arm/fdt.c index 186a718..93849cf2 100644 --- a/tools/kvm/arm/fdt.c +++ b/tools/kvm/arm/fdt.c @@ -13,6 +13,7 @@ #include <linux/byteorder.h> #include <linux/kernel.h> #include <linux/sizes.h> +#include <linux/psci.h> static char kern_cmdline[COMMAND_LINE_SIZE]; @@ -162,12 +163,38 @@ static int setup_fdt(struct kvm *kvm) /* PSCI firmware */ _FDT(fdt_begin_node(fdt, "psci")); - _FDT(fdt_property_string(fdt, "compatible", "arm,psci")); - _FDT(fdt_property_string(fdt, "method", "hvc")); - _FDT(fdt_property_cell(fdt, "cpu_suspend", KVM_PSCI_FN_CPU_SUSPEND)); - _FDT(fdt_property_cell(fdt, "cpu_off", KVM_PSCI_FN_CPU_OFF)); - _FDT(fdt_property_cell(fdt, "cpu_on", KVM_PSCI_FN_CPU_ON)); - _FDT(fdt_property_cell(fdt, "migrate", KVM_PSCI_FN_MIGRATE)); + if (kvm__supports_extension(kvm, KVM_CAP_ARM_PSCI_0_2)) { + const char compatible[] = "arm,psci-0.2\0arm,psci"; + _FDT(fdt_property(fdt, "compatible", + compatible, sizeof(compatible))); + _FDT(fdt_property_string(fdt, "method", "hvc")); + if (kvm->cfg.arch.aarch32_guest) { + _FDT(fdt_property_cell(fdt, "cpu_suspend", + PSCI_0_2_FN_CPU_SUSPEND)); + _FDT(fdt_property_cell(fdt, "cpu_off", + PSCI_0_2_FN_CPU_OFF)); + _FDT(fdt_property_cell(fdt, "cpu_on", + PSCI_0_2_FN_CPU_ON)); + _FDT(fdt_property_cell(fdt, "migrate", + PSCI_0_2_FN_MIGRATE)); + } else { + _FDT(fdt_property_cell(fdt, "cpu_suspend", + PSCI_0_2_FN64_CPU_SUSPEND)); + _FDT(fdt_property_cell(fdt, "cpu_off", + PSCI_0_2_FN_CPU_OFF)); + _FDT(fdt_property_cell(fdt, "cpu_on", + PSCI_0_2_FN64_CPU_ON)); + _FDT(fdt_property_cell(fdt, "migrate", + PSCI_0_2_FN64_MIGRATE)); + } + } else { + _FDT(fdt_property_string(fdt, "compatible", "arm,psci")); + _FDT(fdt_property_string(fdt, "method", "hvc")); + _FDT(fdt_property_cell(fdt, "cpu_suspend", KVM_PSCI_FN_CPU_SUSPEND)); + _FDT(fdt_property_cell(fdt, "cpu_off", KVM_PSCI_FN_CPU_OFF)); + _FDT(fdt_property_cell(fdt, "cpu_on", KVM_PSCI_FN_CPU_ON)); + _FDT(fdt_property_cell(fdt, "migrate", KVM_PSCI_FN_MIGRATE)); + } _FDT(fdt_end_node(fdt)); /* Finalise. */ diff --git a/tools/kvm/arm/kvm-cpu.c b/tools/kvm/arm/kvm-cpu.c index c010e9c..0637e9a 100644 --- a/tools/kvm/arm/kvm-cpu.c +++ b/tools/kvm/arm/kvm-cpu.c @@ -56,6 +56,11 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id) if (vcpu->kvm_run == MAP_FAILED) die("unable to mmap vcpu fd"); + /* Set KVM_ARM_VCPU_PSCI_0_2 if available */ + if (kvm__supports_extension(kvm, KVM_CAP_ARM_PSCI_0_2)) { + vcpu_init.features[0] |= (1UL << KVM_ARM_VCPU_PSCI_0_2); + } + /* * If preferred target ioctl successful then use preferred target * else try each and every target type.