* [PATCH 2/2] drivers: cci: add missing CCI port availability firmware check
2016-09-23 13:09 [PATCH 1/2] ARM: vexpress: refine MCPM smp operations override criteria Lorenzo Pieralisi
@ 2016-09-23 13:09 ` Lorenzo Pieralisi
2016-09-23 15:49 ` Nicolas Pitre
2016-09-26 12:30 ` Jon Medhurst (Tixy)
2016-09-23 14:03 ` [PATCH 1/2] ARM: vexpress: refine MCPM smp operations override criteria Lorenzo Pieralisi
` (3 subsequent siblings)
4 siblings, 2 replies; 11+ messages in thread
From: Lorenzo Pieralisi @ 2016-09-23 13:09 UTC (permalink / raw)
To: linux-arm-kernel
The CCI ports programming interface is available to the kernel
only when booted in secure mode (or when firmware enables
non-secure access to override CCI ports control). In both cases,
firmware reports the CCI ports availability through the device
tree CCI ports nodes, which must be parsed and their status checked
by the kernel probing path.
This check is currently missing and may cause the kernel to
erroneously believe it is free to take control of CCI ports
where in practice CCI ports control is forbidden.
Add the missing CCI port availability check to the CCI driver
in order to guarantee sane CCI usage.
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
---
drivers/bus/arm-cci.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
index 5755907f..3c7a1c7 100644
--- a/drivers/bus/arm-cci.c
+++ b/drivers/bus/arm-cci.c
@@ -2199,6 +2199,9 @@ static int cci_probe_ports(struct device_node *np)
if (!of_match_node(arm_cci_ctrl_if_matches, cp))
continue;
+ if (!of_device_is_available(cp))
+ continue;
+
i = nb_ace + nb_ace_lite;
if (i >= nb_cci_ports)
@@ -2241,6 +2244,13 @@ static int cci_probe_ports(struct device_node *np)
ports[i].dn = cp;
}
+ /*
+ * If there is no CCI port that is under kernel control
+ * return early and report probe status.
+ */
+ if (!nb_ace && !nb_ace_lite)
+ return -ENODEV;
+
/* initialize a stashed array of ACE ports to speed-up look-up */
cci_ace_init_ports();
--
2.10.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 2/2] drivers: cci: add missing CCI port availability firmware check
2016-09-23 13:09 ` [PATCH 2/2] drivers: cci: add missing CCI port availability firmware check Lorenzo Pieralisi
@ 2016-09-23 15:49 ` Nicolas Pitre
2016-09-26 12:30 ` Jon Medhurst (Tixy)
1 sibling, 0 replies; 11+ messages in thread
From: Nicolas Pitre @ 2016-09-23 15:49 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, 23 Sep 2016, Lorenzo Pieralisi wrote:
> The CCI ports programming interface is available to the kernel
> only when booted in secure mode (or when firmware enables
> non-secure access to override CCI ports control). In both cases,
> firmware reports the CCI ports availability through the device
> tree CCI ports nodes, which must be parsed and their status checked
> by the kernel probing path.
>
> This check is currently missing and may cause the kernel to
> erroneously believe it is free to take control of CCI ports
> where in practice CCI ports control is forbidden.
>
> Add the missing CCI port availability check to the CCI driver
> in order to guarantee sane CCI usage.
>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
Acked-by: Nicolas Pitre <nico@linaro.org>
> ---
> drivers/bus/arm-cci.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> index 5755907f..3c7a1c7 100644
> --- a/drivers/bus/arm-cci.c
> +++ b/drivers/bus/arm-cci.c
> @@ -2199,6 +2199,9 @@ static int cci_probe_ports(struct device_node *np)
> if (!of_match_node(arm_cci_ctrl_if_matches, cp))
> continue;
>
> + if (!of_device_is_available(cp))
> + continue;
> +
> i = nb_ace + nb_ace_lite;
>
> if (i >= nb_cci_ports)
> @@ -2241,6 +2244,13 @@ static int cci_probe_ports(struct device_node *np)
> ports[i].dn = cp;
> }
>
> + /*
> + * If there is no CCI port that is under kernel control
> + * return early and report probe status.
> + */
> + if (!nb_ace && !nb_ace_lite)
> + return -ENODEV;
> +
> /* initialize a stashed array of ACE ports to speed-up look-up */
> cci_ace_init_ports();
>
> --
> 2.10.0
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] drivers: cci: add missing CCI port availability firmware check
2016-09-23 13:09 ` [PATCH 2/2] drivers: cci: add missing CCI port availability firmware check Lorenzo Pieralisi
2016-09-23 15:49 ` Nicolas Pitre
@ 2016-09-26 12:30 ` Jon Medhurst (Tixy)
1 sibling, 0 replies; 11+ messages in thread
From: Jon Medhurst (Tixy) @ 2016-09-26 12:30 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, 2016-09-23 at 14:09 +0100, Lorenzo Pieralisi wrote:
> The CCI ports programming interface is available to the kernel
> only when booted in secure mode (or when firmware enables
> non-secure access to override CCI ports control). In both cases,
> firmware reports the CCI ports availability through the device
> tree CCI ports nodes, which must be parsed and their status checked
> by the kernel probing path.
>
> This check is currently missing and may cause the kernel to
> erroneously believe it is free to take control of CCI ports
> where in practice CCI ports control is forbidden.
>
> Add the missing CCI port availability check to the CCI driver
> in order to guarantee sane CCI usage.
>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> ---
Tested-by: Jon Medhurst <tixy@linaro.org>
> drivers/bus/arm-cci.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> index 5755907f..3c7a1c7 100644
> --- a/drivers/bus/arm-cci.c
> +++ b/drivers/bus/arm-cci.c
> @@ -2199,6 +2199,9 @@ static int cci_probe_ports(struct device_node *np)
> if (!of_match_node(arm_cci_ctrl_if_matches, cp))
> continue;
>
> + if (!of_device_is_available(cp))
> + continue;
> +
> i = nb_ace + nb_ace_lite;
>
> if (i >= nb_cci_ports)
> @@ -2241,6 +2244,13 @@ static int cci_probe_ports(struct device_node *np)
> ports[i].dn = cp;
> }
>
> + /*
> + * If there is no CCI port that is under kernel control
> + * return early and report probe status.
> + */
> + if (!nb_ace && !nb_ace_lite)
> + return -ENODEV;
> +
> /* initialize a stashed array of ACE ports to speed-up look-up */
> cci_ace_init_ports();
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] ARM: vexpress: refine MCPM smp operations override criteria
2016-09-23 13:09 [PATCH 1/2] ARM: vexpress: refine MCPM smp operations override criteria Lorenzo Pieralisi
2016-09-23 13:09 ` [PATCH 2/2] drivers: cci: add missing CCI port availability firmware check Lorenzo Pieralisi
@ 2016-09-23 14:03 ` Lorenzo Pieralisi
2016-10-17 17:31 ` Sudeep Holla
2016-09-23 15:00 ` Liviu Dudau
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Lorenzo Pieralisi @ 2016-09-23 14:03 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Sep 23, 2016 at 02:09:06PM +0100, Lorenzo Pieralisi wrote:
> Current vexpress smp init code detects whether to override the
> default smp ops with MCPM smp ops by matching the "cci-400"
> compatible string, in that MCPM requires control over CCI ports
> to manage low-power states entry/exit.
>
> The "cci-400" compatible string check is a necessary but not
> sufficient condition for MCPM to work, because the cci-400
> can be made visible to the kernel, but firmware can nonetheless
> disable non-secure CCI ports control, while still allowing PMU
> access; if booted in non-secure world, the kernel would still
> blindly override smp operations with MCPM operations, resulting
> in kernel faults when the CCI ports programming interface is
> accessed from non-secure world.
>
> This means that the "cci-400" compatible string check would
> result in a false positive in systems that eg boot in HYP mode,
> where CCI ports non-secure access is explicitly not allowed,
> and it is reported in the respective device tree nodes with
> CCI ports marked as disabled.
>
> Refactor the smp operations initialization to make sure that
> the kernel is actually allowed to take control over CCI ports
> (by enabling MCPM smp operations) before overriding default
> vexpress smp operations.
>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> ---
> arch/arm/mach-vexpress/platsmp.c | 31 ++++++++++++++++++++++++-------
> 1 file changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm/mach-vexpress/platsmp.c b/arch/arm/mach-vexpress/platsmp.c
> index 8b8d072..6cfd782 100644
> --- a/arch/arm/mach-vexpress/platsmp.c
> +++ b/arch/arm/mach-vexpress/platsmp.c
> @@ -26,17 +26,34 @@
> bool __init vexpress_smp_init_ops(void)
> {
> #ifdef CONFIG_MCPM
> + int cpu;
> + struct device_node *cpu_node, *cci_node;
> +
> /*
> - * The best way to detect a multi-cluster configuration at the moment
> - * is to look for the presence of a CCI in the system.
> + * The best way to detect a multi-cluster configuration
> + * is to detect if the kernel can take over CCI ports
> + * control. Loop over possible CPUs and check if CCI
> + * port control is available.
> * Override the default vexpress_smp_ops if so.
> */
> - struct device_node *node;
> - node = of_find_compatible_node(NULL, NULL, "arm,cci-400");
> - if (node && of_device_is_available(node)) {
> - mcpm_smp_set_ops();
> - return true;
> + for_each_possible_cpu(cpu) {
> + bool available;
> +
> + cpu_node = of_get_cpu_node(cpu, NULL);
> + if (WARN(!cpu_node, "Missing cpu device node!"))
> + return false;
> +
> + cci_node = of_parse_phandle(cpu_node, "cci-control-port", 0);
> + available = cci_node && of_device_is_available(cci_node);
> + of_node_put(cci_node);
> + of_node_put(cpu_node);
> +
> + if (!available)
> + return false;
> }
> +
> + mcpm_smp_set_ops();
> + return true;
> #endif
> return false;
For the records, while moving the code around I missed I was ending
up with this idiotic double return, I have already reworked the patch
and will squash changes in the final version if we agree on the bulk of
the code.
Lorenzo
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/2] ARM: vexpress: refine MCPM smp operations override criteria
2016-09-23 14:03 ` [PATCH 1/2] ARM: vexpress: refine MCPM smp operations override criteria Lorenzo Pieralisi
@ 2016-10-17 17:31 ` Sudeep Holla
2016-10-19 8:38 ` Lorenzo Pieralisi
0 siblings, 1 reply; 11+ messages in thread
From: Sudeep Holla @ 2016-10-17 17:31 UTC (permalink / raw)
To: linux-arm-kernel
On 23/09/16 15:03, Lorenzo Pieralisi wrote:
> On Fri, Sep 23, 2016 at 02:09:06PM +0100, Lorenzo Pieralisi wrote:
>> Current vexpress smp init code detects whether to override the
>> default smp ops with MCPM smp ops by matching the "cci-400"
>> compatible string, in that MCPM requires control over CCI ports
>> to manage low-power states entry/exit.
>>
>> The "cci-400" compatible string check is a necessary but not
>> sufficient condition for MCPM to work, because the cci-400
>> can be made visible to the kernel, but firmware can nonetheless
>> disable non-secure CCI ports control, while still allowing PMU
>> access; if booted in non-secure world, the kernel would still
>> blindly override smp operations with MCPM operations, resulting
>> in kernel faults when the CCI ports programming interface is
>> accessed from non-secure world.
>>
>> This means that the "cci-400" compatible string check would
>> result in a false positive in systems that eg boot in HYP mode,
>> where CCI ports non-secure access is explicitly not allowed,
>> and it is reported in the respective device tree nodes with
>> CCI ports marked as disabled.
>>
>> Refactor the smp operations initialization to make sure that
>> the kernel is actually allowed to take control over CCI ports
>> (by enabling MCPM smp operations) before overriding default
>> vexpress smp operations.
>>
>> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Cc: Liviu Dudau <liviu.dudau@arm.com>
>> Cc: Sudeep Holla <sudeep.holla@arm.com>
>> Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>> arch/arm/mach-vexpress/platsmp.c | 31 ++++++++++++++++++++++++-------
>> 1 file changed, 24 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/arm/mach-vexpress/platsmp.c b/arch/arm/mach-vexpress/platsmp.c
>> index 8b8d072..6cfd782 100644
>> --- a/arch/arm/mach-vexpress/platsmp.c
>> +++ b/arch/arm/mach-vexpress/platsmp.c
>> @@ -26,17 +26,34 @@
>> bool __init vexpress_smp_init_ops(void)
>> {
>> #ifdef CONFIG_MCPM
>> + int cpu;
>> + struct device_node *cpu_node, *cci_node;
>> +
>> /*
>> - * The best way to detect a multi-cluster configuration at the moment
>> - * is to look for the presence of a CCI in the system.
>> + * The best way to detect a multi-cluster configuration
>> + * is to detect if the kernel can take over CCI ports
>> + * control. Loop over possible CPUs and check if CCI
>> + * port control is available.
>> * Override the default vexpress_smp_ops if so.
>> */
>> - struct device_node *node;
>> - node = of_find_compatible_node(NULL, NULL, "arm,cci-400");
>> - if (node && of_device_is_available(node)) {
>> - mcpm_smp_set_ops();
>> - return true;
>> + for_each_possible_cpu(cpu) {
>> + bool available;
>> +
>> + cpu_node = of_get_cpu_node(cpu, NULL);
>> + if (WARN(!cpu_node, "Missing cpu device node!"))
>> + return false;
>> +
>> + cci_node = of_parse_phandle(cpu_node, "cci-control-port", 0);
>> + available = cci_node && of_device_is_available(cci_node);
>> + of_node_put(cci_node);
>> + of_node_put(cpu_node);
>> +
>> + if (!available)
>> + return false;
>> }
>> +
>> + mcpm_smp_set_ops();
>> + return true;
>> #endif
>> return false;
>
> For the records, while moving the code around I missed I was ending
> up with this idiotic double return, I have already reworked the patch
> and will squash changes in the final version if we agree on the bulk of
> the code.
>
I applied both patches to [1] with the fix for the above issue. Let me
know if that's fine. I have tested both hyp mode boot and SVC mode +
MCPM boot with latest u-boot by just fliping a bit in the firmware
(board.txt) without recompiling the kernel.
--
Regards,
Sudeep
[1] git.kernel.org/sudeep.holla/linux/h/vexpress/for-next
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/2] ARM: vexpress: refine MCPM smp operations override criteria
2016-10-17 17:31 ` Sudeep Holla
@ 2016-10-19 8:38 ` Lorenzo Pieralisi
0 siblings, 0 replies; 11+ messages in thread
From: Lorenzo Pieralisi @ 2016-10-19 8:38 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Oct 17, 2016 at 06:31:58PM +0100, Sudeep Holla wrote:
>
>
> On 23/09/16 15:03, Lorenzo Pieralisi wrote:
> >On Fri, Sep 23, 2016 at 02:09:06PM +0100, Lorenzo Pieralisi wrote:
> >>Current vexpress smp init code detects whether to override the
> >>default smp ops with MCPM smp ops by matching the "cci-400"
> >>compatible string, in that MCPM requires control over CCI ports
> >>to manage low-power states entry/exit.
> >>
> >>The "cci-400" compatible string check is a necessary but not
> >>sufficient condition for MCPM to work, because the cci-400
> >>can be made visible to the kernel, but firmware can nonetheless
> >>disable non-secure CCI ports control, while still allowing PMU
> >>access; if booted in non-secure world, the kernel would still
> >>blindly override smp operations with MCPM operations, resulting
> >>in kernel faults when the CCI ports programming interface is
> >>accessed from non-secure world.
> >>
> >>This means that the "cci-400" compatible string check would
> >>result in a false positive in systems that eg boot in HYP mode,
> >>where CCI ports non-secure access is explicitly not allowed,
> >>and it is reported in the respective device tree nodes with
> >>CCI ports marked as disabled.
> >>
> >>Refactor the smp operations initialization to make sure that
> >>the kernel is actually allowed to take control over CCI ports
> >>(by enabling MCPM smp operations) before overriding default
> >>vexpress smp operations.
> >>
> >>Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> >>Cc: Liviu Dudau <liviu.dudau@arm.com>
> >>Cc: Sudeep Holla <sudeep.holla@arm.com>
> >>Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> >>Cc: Marc Zyngier <marc.zyngier@arm.com>
> >>---
> >> arch/arm/mach-vexpress/platsmp.c | 31 ++++++++++++++++++++++++-------
> >> 1 file changed, 24 insertions(+), 7 deletions(-)
> >>
> >>diff --git a/arch/arm/mach-vexpress/platsmp.c b/arch/arm/mach-vexpress/platsmp.c
> >>index 8b8d072..6cfd782 100644
> >>--- a/arch/arm/mach-vexpress/platsmp.c
> >>+++ b/arch/arm/mach-vexpress/platsmp.c
> >>@@ -26,17 +26,34 @@
> >> bool __init vexpress_smp_init_ops(void)
> >> {
> >> #ifdef CONFIG_MCPM
> >>+ int cpu;
> >>+ struct device_node *cpu_node, *cci_node;
> >>+
> >> /*
> >>- * The best way to detect a multi-cluster configuration at the moment
> >>- * is to look for the presence of a CCI in the system.
> >>+ * The best way to detect a multi-cluster configuration
> >>+ * is to detect if the kernel can take over CCI ports
> >>+ * control. Loop over possible CPUs and check if CCI
> >>+ * port control is available.
> >> * Override the default vexpress_smp_ops if so.
> >> */
> >>- struct device_node *node;
> >>- node = of_find_compatible_node(NULL, NULL, "arm,cci-400");
> >>- if (node && of_device_is_available(node)) {
> >>- mcpm_smp_set_ops();
> >>- return true;
> >>+ for_each_possible_cpu(cpu) {
> >>+ bool available;
> >>+
> >>+ cpu_node = of_get_cpu_node(cpu, NULL);
> >>+ if (WARN(!cpu_node, "Missing cpu device node!"))
> >>+ return false;
> >>+
> >>+ cci_node = of_parse_phandle(cpu_node, "cci-control-port", 0);
> >>+ available = cci_node && of_device_is_available(cci_node);
> >>+ of_node_put(cci_node);
> >>+ of_node_put(cpu_node);
> >>+
> >>+ if (!available)
> >>+ return false;
> >> }
> >>+
> >>+ mcpm_smp_set_ops();
> >>+ return true;
> >> #endif
> >> return false;
> >
> >For the records, while moving the code around I missed I was ending
> >up with this idiotic double return, I have already reworked the patch
> >and will squash changes in the final version if we agree on the bulk of
> >the code.
> >
>
> I applied both patches to [1] with the fix for the above issue. Let me
> know if that's fine. I have tested both hyp mode boot and SVC mode +
> MCPM boot with latest u-boot by just fliping a bit in the firmware
> (board.txt) without recompiling the kernel.
I updated the patch to remove the double return will send an updated
patch today.
Thanks !
Lorenzo
>
> --
> Regards,
> Sudeep
>
> [1] git.kernel.org/sudeep.holla/linux/h/vexpress/for-next
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] ARM: vexpress: refine MCPM smp operations override criteria
2016-09-23 13:09 [PATCH 1/2] ARM: vexpress: refine MCPM smp operations override criteria Lorenzo Pieralisi
2016-09-23 13:09 ` [PATCH 2/2] drivers: cci: add missing CCI port availability firmware check Lorenzo Pieralisi
2016-09-23 14:03 ` [PATCH 1/2] ARM: vexpress: refine MCPM smp operations override criteria Lorenzo Pieralisi
@ 2016-09-23 15:00 ` Liviu Dudau
2016-09-23 15:01 ` Sudeep Holla
2016-09-23 15:46 ` Nicolas Pitre
2016-09-26 12:29 ` Jon Medhurst (Tixy)
4 siblings, 1 reply; 11+ messages in thread
From: Liviu Dudau @ 2016-09-23 15:00 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Sep 23, 2016 at 02:09:06PM +0100, Lorenzo Pieralisi wrote:
> Current vexpress smp init code detects whether to override the
> default smp ops with MCPM smp ops by matching the "cci-400"
> compatible string, in that MCPM requires control over CCI ports
> to manage low-power states entry/exit.
>
> The "cci-400" compatible string check is a necessary but not
> sufficient condition for MCPM to work, because the cci-400
> can be made visible to the kernel, but firmware can nonetheless
> disable non-secure CCI ports control, while still allowing PMU
> access; if booted in non-secure world, the kernel would still
> blindly override smp operations with MCPM operations, resulting
> in kernel faults when the CCI ports programming interface is
> accessed from non-secure world.
>
> This means that the "cci-400" compatible string check would
> result in a false positive in systems that eg boot in HYP mode,
> where CCI ports non-secure access is explicitly not allowed,
> and it is reported in the respective device tree nodes with
> CCI ports marked as disabled.
I remember seeing a patch from Marc this week exactly on this
subject, but I can't find it again today. However, I remember that
his patch was explicitly testing for the HYP presence.
>
> Refactor the smp operations initialization to make sure that
> the kernel is actually allowed to take control over CCI ports
> (by enabling MCPM smp operations) before overriding default
> vexpress smp operations.
>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> ---
> arch/arm/mach-vexpress/platsmp.c | 31 ++++++++++++++++++++++++-------
> 1 file changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm/mach-vexpress/platsmp.c b/arch/arm/mach-vexpress/platsmp.c
> index 8b8d072..6cfd782 100644
> --- a/arch/arm/mach-vexpress/platsmp.c
> +++ b/arch/arm/mach-vexpress/platsmp.c
> @@ -26,17 +26,34 @@
> bool __init vexpress_smp_init_ops(void)
> {
> #ifdef CONFIG_MCPM
> + int cpu;
> + struct device_node *cpu_node, *cci_node;
> +
> /*
> - * The best way to detect a multi-cluster configuration at the moment
> - * is to look for the presence of a CCI in the system.
> + * The best way to detect a multi-cluster configuration
> + * is to detect if the kernel can take over CCI ports
> + * control. Loop over possible CPUs and check if CCI
> + * port control is available.
> * Override the default vexpress_smp_ops if so.
> */
> - struct device_node *node;
> - node = of_find_compatible_node(NULL, NULL, "arm,cci-400");
> - if (node && of_device_is_available(node)) {
> - mcpm_smp_set_ops();
> - return true;
> + for_each_possible_cpu(cpu) {
> + bool available;
> +
> + cpu_node = of_get_cpu_node(cpu, NULL);
> + if (WARN(!cpu_node, "Missing cpu device node!"))
> + return false;
> +
> + cci_node = of_parse_phandle(cpu_node, "cci-control-port", 0);
> + available = cci_node && of_device_is_available(cci_node);
of_device_is_available() only checks the DT node for status = "enabled";
Does the HYP modify the DT to disable the cci-control-port? If not, then I guess
this patch is not enough?
Best regards,
Liviu
> + of_node_put(cci_node);
> + of_node_put(cpu_node);
> +
> + if (!available)
> + return false;
> }
> +
> + mcpm_smp_set_ops();
> + return true;
> #endif
> return false;
> }
> --
> 2.10.0
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
?\_(?)_/?
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/2] ARM: vexpress: refine MCPM smp operations override criteria
2016-09-23 15:00 ` Liviu Dudau
@ 2016-09-23 15:01 ` Sudeep Holla
0 siblings, 0 replies; 11+ messages in thread
From: Sudeep Holla @ 2016-09-23 15:01 UTC (permalink / raw)
To: linux-arm-kernel
On 23/09/16 16:00, Liviu Dudau wrote:
> On Fri, Sep 23, 2016 at 02:09:06PM +0100, Lorenzo Pieralisi wrote:
>> Current vexpress smp init code detects whether to override the
>> default smp ops with MCPM smp ops by matching the "cci-400"
>> compatible string, in that MCPM requires control over CCI ports
>> to manage low-power states entry/exit.
>>
>> The "cci-400" compatible string check is a necessary but not
>> sufficient condition for MCPM to work, because the cci-400
>> can be made visible to the kernel, but firmware can nonetheless
>> disable non-secure CCI ports control, while still allowing PMU
>> access; if booted in non-secure world, the kernel would still
>> blindly override smp operations with MCPM operations, resulting
>> in kernel faults when the CCI ports programming interface is
>> accessed from non-secure world.
>>
>> This means that the "cci-400" compatible string check would
>> result in a false positive in systems that eg boot in HYP mode,
>> where CCI ports non-secure access is explicitly not allowed,
>> and it is reported in the respective device tree nodes with
>> CCI ports marked as disabled.
>
> I remember seeing a patch from Marc this week exactly on this
> subject, but I can't find it again today. However, I remember that
> his patch was explicitly testing for the HYP presence.
>
>>
>> Refactor the smp operations initialization to make sure that
>> the kernel is actually allowed to take control over CCI ports
>> (by enabling MCPM smp operations) before overriding default
>> vexpress smp operations.
>>
>> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Cc: Liviu Dudau <liviu.dudau@arm.com>
>> Cc: Sudeep Holla <sudeep.holla@arm.com>
>> Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>> arch/arm/mach-vexpress/platsmp.c | 31 ++++++++++++++++++++++++-------
>> 1 file changed, 24 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/arm/mach-vexpress/platsmp.c b/arch/arm/mach-vexpress/platsmp.c
>> index 8b8d072..6cfd782 100644
>> --- a/arch/arm/mach-vexpress/platsmp.c
>> +++ b/arch/arm/mach-vexpress/platsmp.c
>> @@ -26,17 +26,34 @@
>> bool __init vexpress_smp_init_ops(void)
>> {
>> #ifdef CONFIG_MCPM
>> + int cpu;
>> + struct device_node *cpu_node, *cci_node;
>> +
>> /*
>> - * The best way to detect a multi-cluster configuration at the moment
>> - * is to look for the presence of a CCI in the system.
>> + * The best way to detect a multi-cluster configuration
>> + * is to detect if the kernel can take over CCI ports
>> + * control. Loop over possible CPUs and check if CCI
>> + * port control is available.
>> * Override the default vexpress_smp_ops if so.
>> */
>> - struct device_node *node;
>> - node = of_find_compatible_node(NULL, NULL, "arm,cci-400");
>> - if (node && of_device_is_available(node)) {
>> - mcpm_smp_set_ops();
>> - return true;
>> + for_each_possible_cpu(cpu) {
>> + bool available;
>> +
>> + cpu_node = of_get_cpu_node(cpu, NULL);
>> + if (WARN(!cpu_node, "Missing cpu device node!"))
>> + return false;
>> +
>> + cci_node = of_parse_phandle(cpu_node, "cci-control-port", 0);
>> + available = cci_node && of_device_is_available(cci_node);
>
> of_device_is_available() only checks the DT node for status = "enabled";
>
> Does the HYP modify the DT to disable the cci-control-port? If not, then I guess
> this patch is not enough?
>
Correct, I will send out the u-boot patch to disable the cci ports soon.
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] ARM: vexpress: refine MCPM smp operations override criteria
2016-09-23 13:09 [PATCH 1/2] ARM: vexpress: refine MCPM smp operations override criteria Lorenzo Pieralisi
` (2 preceding siblings ...)
2016-09-23 15:00 ` Liviu Dudau
@ 2016-09-23 15:46 ` Nicolas Pitre
2016-09-26 12:29 ` Jon Medhurst (Tixy)
4 siblings, 0 replies; 11+ messages in thread
From: Nicolas Pitre @ 2016-09-23 15:46 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, 23 Sep 2016, Lorenzo Pieralisi wrote:
> Current vexpress smp init code detects whether to override the
> default smp ops with MCPM smp ops by matching the "cci-400"
> compatible string, in that MCPM requires control over CCI ports
> to manage low-power states entry/exit.
>
> The "cci-400" compatible string check is a necessary but not
> sufficient condition for MCPM to work, because the cci-400
> can be made visible to the kernel, but firmware can nonetheless
> disable non-secure CCI ports control, while still allowing PMU
> access; if booted in non-secure world, the kernel would still
> blindly override smp operations with MCPM operations, resulting
> in kernel faults when the CCI ports programming interface is
> accessed from non-secure world.
>
> This means that the "cci-400" compatible string check would
> result in a false positive in systems that eg boot in HYP mode,
> where CCI ports non-secure access is explicitly not allowed,
> and it is reported in the respective device tree nodes with
> CCI ports marked as disabled.
>
> Refactor the smp operations initialization to make sure that
> the kernel is actually allowed to take control over CCI ports
> (by enabling MCPM smp operations) before overriding default
> vexpress smp operations.
>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
Acked-by: Nicolas Pitre <nico@linaro.org>
> ---
> arch/arm/mach-vexpress/platsmp.c | 31 ++++++++++++++++++++++++-------
> 1 file changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm/mach-vexpress/platsmp.c b/arch/arm/mach-vexpress/platsmp.c
> index 8b8d072..6cfd782 100644
> --- a/arch/arm/mach-vexpress/platsmp.c
> +++ b/arch/arm/mach-vexpress/platsmp.c
> @@ -26,17 +26,34 @@
> bool __init vexpress_smp_init_ops(void)
> {
> #ifdef CONFIG_MCPM
> + int cpu;
> + struct device_node *cpu_node, *cci_node;
> +
> /*
> - * The best way to detect a multi-cluster configuration at the moment
> - * is to look for the presence of a CCI in the system.
> + * The best way to detect a multi-cluster configuration
> + * is to detect if the kernel can take over CCI ports
> + * control. Loop over possible CPUs and check if CCI
> + * port control is available.
> * Override the default vexpress_smp_ops if so.
> */
> - struct device_node *node;
> - node = of_find_compatible_node(NULL, NULL, "arm,cci-400");
> - if (node && of_device_is_available(node)) {
> - mcpm_smp_set_ops();
> - return true;
> + for_each_possible_cpu(cpu) {
> + bool available;
> +
> + cpu_node = of_get_cpu_node(cpu, NULL);
> + if (WARN(!cpu_node, "Missing cpu device node!"))
> + return false;
> +
> + cci_node = of_parse_phandle(cpu_node, "cci-control-port", 0);
> + available = cci_node && of_device_is_available(cci_node);
> + of_node_put(cci_node);
> + of_node_put(cpu_node);
> +
> + if (!available)
> + return false;
> }
> +
> + mcpm_smp_set_ops();
> + return true;
> #endif
> return false;
> }
> --
> 2.10.0
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/2] ARM: vexpress: refine MCPM smp operations override criteria
2016-09-23 13:09 [PATCH 1/2] ARM: vexpress: refine MCPM smp operations override criteria Lorenzo Pieralisi
` (3 preceding siblings ...)
2016-09-23 15:46 ` Nicolas Pitre
@ 2016-09-26 12:29 ` Jon Medhurst (Tixy)
4 siblings, 0 replies; 11+ messages in thread
From: Jon Medhurst (Tixy) @ 2016-09-26 12:29 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, 2016-09-23 at 14:09 +0100, Lorenzo Pieralisi wrote:
> Current vexpress smp init code detects whether to override the
> default smp ops with MCPM smp ops by matching the "cci-400"
> compatible string, in that MCPM requires control over CCI ports
> to manage low-power states entry/exit.
>
> The "cci-400" compatible string check is a necessary but not
> sufficient condition for MCPM to work, because the cci-400
> can be made visible to the kernel, but firmware can nonetheless
> disable non-secure CCI ports control, while still allowing PMU
> access; if booted in non-secure world, the kernel would still
> blindly override smp operations with MCPM operations, resulting
> in kernel faults when the CCI ports programming interface is
> accessed from non-secure world.
>
> This means that the "cci-400" compatible string check would
> result in a false positive in systems that eg boot in HYP mode,
> where CCI ports non-secure access is explicitly not allowed,
> and it is reported in the respective device tree nodes with
> CCI ports marked as disabled.
>
> Refactor the smp operations initialization to make sure that
> the kernel is actually allowed to take control over CCI ports
> (by enabling MCPM smp operations) before overriding default
> vexpress smp operations.
>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> ---
Tested-by: Jon Medhurst <tixy@linaro.org>
> arch/arm/mach-vexpress/platsmp.c | 31 ++++++++++++++++++++++++-------
> 1 file changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm/mach-vexpress/platsmp.c b/arch/arm/mach-vexpress/platsmp.c
> index 8b8d072..6cfd782 100644
> --- a/arch/arm/mach-vexpress/platsmp.c
> +++ b/arch/arm/mach-vexpress/platsmp.c
> @@ -26,17 +26,34 @@
> bool __init vexpress_smp_init_ops(void)
> {
> #ifdef CONFIG_MCPM
> + int cpu;
> + struct device_node *cpu_node, *cci_node;
> +
> /*
> - * The best way to detect a multi-cluster configuration at the moment
> - * is to look for the presence of a CCI in the system.
> + * The best way to detect a multi-cluster configuration
> + * is to detect if the kernel can take over CCI ports
> + * control. Loop over possible CPUs and check if CCI
> + * port control is available.
> * Override the default vexpress_smp_ops if so.
> */
> - struct device_node *node;
> - node = of_find_compatible_node(NULL, NULL, "arm,cci-400");
> - if (node && of_device_is_available(node)) {
> - mcpm_smp_set_ops();
> - return true;
> + for_each_possible_cpu(cpu) {
> + bool available;
> +
> + cpu_node = of_get_cpu_node(cpu, NULL);
> + if (WARN(!cpu_node, "Missing cpu device node!"))
> + return false;
> +
> + cci_node = of_parse_phandle(cpu_node, "cci-control-port", 0);
> + available = cci_node && of_device_is_available(cci_node);
> + of_node_put(cci_node);
> + of_node_put(cpu_node);
> +
> + if (!available)
> + return false;
> }
> +
> + mcpm_smp_set_ops();
> + return true;
> #endif
> return false;
> }
^ permalink raw reply [flat|nested] 11+ messages in thread