From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Suzuki K. Poulose" Subject: Re: [PATCH 2/4] arm-cci: Get rid of secure transactions for PMU driver Date: Wed, 25 Feb 2015 10:20:10 +0000 Message-ID: <54EDA1DA.6040508@arm.com> References: <1424783863-9894-1-git-send-email-suzuki.poulose@arm.com> <1424783863-9894-3-git-send-email-suzuki.poulose@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=WINDOWS-1252; format=flowed Content-Transfer-Encoding: 8BIT Return-path: In-Reply-To: Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Nicolas Pitre Cc: "linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" , Bartlomiej Zolnierkiewicz , Kukjin Kim , Abhilash Kesavan , Arnd Bergmann , "devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Liviu Dudau , Lorenzo Pieralisi , Olof Johansson , Pawel Moll , Punit Agrawal , Sudeep Holla , Will Deacon , Catalin Marinas List-Id: devicetree@vger.kernel.org On 24/02/15 21:53, Nicolas Pitre wrote: > On Tue, 24 Feb 2015, Suzuki K. Poulose wrote: > >> From: "Suzuki K. Poulose" >> >> Avoid secure transactions while probing the CCI PMU. The >> existing code makes use of the Peripheral ID2 (PID2) register >> to determine the revision of the CCI400, which requires a >> secure transaction. This puts a limitation on the usage of the >> driver on systems running non-secure Linux(e.g, ARM64). >> >> Updated the device-tree binding for cci pmu node to add the explicit >> revision number for the compatible field. >> >> The supported strings are : >> arm,cci-400-pmu,r0 >> arm,cci-400-pmu,r1 >> arm,cci-400-pmu - DEPRECATED. See NOTE below >> >> NOTE: If the revision is not mentioned, we need to probe the cci revision, >> which could be fatal on a platform running non-secure. We need a reliable way >> to know if we can poke the CCI registers at runtime on ARM32. We depend on >> 'mcpm_is_available()' when it is available. mcpm_is_available() returns true >> only when there is a registered driver for mcpm. Otherwise, we assume that we >> don't have secure access, and skips probing the revision number(ARM64 case). >> >> The MCPM should figure out if it is safe to access the CCI. Unfortunately >> there isn't a reliable way to indicate the same via dtb. This patch doesn't >> address/change the current situation. It only deals with the CCI-PMU, leaving >> the assumptions about the secure access as it has been, prior to this patch. > > This is an extensive commit log about secure access issues which is nice > and appreciated. However the patch does quite some more code reorg not > mentioned here. Could you please move this code reorg to a separate > patch and then have a patch on top introducing these probing changes? > This should make the implication of what is said above clearer. Sure, I will do that in the next revision. What I missed in the commit follows (which will be added in the next version): "This patch abstracts the representation of the CCI400 chipset PMU specific definitions, so that we can avoid probing the revision for any details. The new device-tree bindings helps to get the revision, without poking the CCI, and initialises the pmu with specific model details." Thanks Suzuki > >> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org >> Cc: Punit Agrawal >> Signed-off-by: Suzuki K. Poulose >> --- >> Documentation/devicetree/bindings/arm/cci.txt | 7 +- >> arch/arm/include/asm/arm-cci.h | 42 ++++++++ >> arch/arm64/include/asm/arm-cci.h | 27 +++++ >> drivers/bus/arm-cci.c | 138 ++++++++++++++++--------- >> include/linux/arm-cci.h | 2 + >> 5 files changed, 166 insertions(+), 50 deletions(-) >> create mode 100644 arch/arm/include/asm/arm-cci.h >> create mode 100644 arch/arm64/include/asm/arm-cci.h >> >> diff --git a/Documentation/devicetree/bindings/arm/cci.txt b/Documentation/devicetree/bindings/arm/cci.txt >> index f28d82b..0e4b6a7 100644 >> --- a/Documentation/devicetree/bindings/arm/cci.txt >> +++ b/Documentation/devicetree/bindings/arm/cci.txt >> @@ -94,8 +94,11 @@ specific to ARM. >> - compatible >> Usage: required >> Value type: >> - Definition: must be "arm,cci-400-pmu" >> - >> + Definition: Supported strings are : >> + "arm,cci-400-pmu,r0" >> + "arm,cci-400-pmu,r1" >> + "arm,cci-400-pmu" - DEPRECATED, permitted only where OS has >> + secure acces to CCI registers >> - reg: >> Usage: required >> Value type: Integer cells. A register entry, expressed >> diff --git a/arch/arm/include/asm/arm-cci.h b/arch/arm/include/asm/arm-cci.h >> new file mode 100644 >> index 0000000..b828d7a >> --- /dev/null >> +++ b/arch/arm/include/asm/arm-cci.h >> @@ -0,0 +1,42 @@ >> +/* >> + * arch/arm/include/asm/arm-cci.h >> + * >> + * Copyright (C) 2015 ARM Ltd. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program. If not, see . >> + */ >> + >> +#ifndef __ASM_ARM_CCI_H >> +#define __ASM_ARM_CCI_H >> + >> +#ifdef CONFIG_MCPM >> +#include >> + >> +/* >> + * We don't have a reliable way of detecting, whether >> + * we have access to secure-only registers, unless >> + * mcpm is registered. >> + */ >> +static inline int platform_has_secure_cci_access(void) >> +{ >> + return mcpm_is_available(); >> +} >> + >> +#else >> +static inline int platform_has_secure_cci_access(void) >> +{ >> + return 0; >> +} >> +#endif >> + >> +#endif >> diff --git a/arch/arm64/include/asm/arm-cci.h b/arch/arm64/include/asm/arm-cci.h >> new file mode 100644 >> index 0000000..37bbe37 >> --- /dev/null >> +++ b/arch/arm64/include/asm/arm-cci.h >> @@ -0,0 +1,27 @@ >> +/* >> + * arch/arm64/include/asm/arm-cci.h >> + * >> + * Copyright (C) 2015 ARM Ltd. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program. If not, see . >> + */ >> + >> +#ifndef __ASM_ARM_CCI_H >> +#define __ASM_ARM_CCI_H >> + >> +static inline int platform_has_secure_cci_access(void) >> +{ >> + return 0; >> +} >> + >> +#endif >> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c >> index f27cf56..fe9fa46 100644 >> --- a/drivers/bus/arm-cci.c >> +++ b/drivers/bus/arm-cci.c >> @@ -79,19 +79,38 @@ static const struct of_device_id arm_cci_matches[] = { >> >> #define CCI_PMU_MAX_HW_EVENTS 5 /* CCI PMU has 4 counters + 1 cycle counter */ >> >> +/* Types of interfaces that can generate events */ >> +enum { >> + CCI_IF_SLAVE, >> + CCI_IF_MASTER, >> + CCI_IF_MAX, >> +}; >> + >> +struct event_range { >> + u32 min; >> + u32 max; >> +}; >> + >> struct cci_pmu_hw_events { >> struct perf_event *events[CCI_PMU_MAX_HW_EVENTS]; >> unsigned long used_mask[BITS_TO_LONGS(CCI_PMU_MAX_HW_EVENTS)]; >> raw_spinlock_t pmu_lock; >> }; >> >> +struct cci_pmu_model { >> + char *name; >> + struct event_range event_ranges[CCI_IF_MAX]; >> +}; >> + >> +static struct cci_pmu_model cci_pmu_models[]; >> + >> struct cci_pmu { >> void __iomem *base; >> struct pmu pmu; >> int nr_irqs; >> int irqs[CCI_PMU_MAX_HW_EVENTS]; >> unsigned long active_irqs; >> - struct pmu_port_event_ranges *port_ranges; >> + const struct cci_pmu_model *model; >> struct cci_pmu_hw_events hw_events; >> struct platform_device *plat_device; >> int num_events; >> @@ -152,47 +171,16 @@ enum cci400_perf_events { >> #define CCI_REV_R1_MASTER_PORT_MIN_EV 0x00 >> #define CCI_REV_R1_MASTER_PORT_MAX_EV 0x11 >> >> -struct pmu_port_event_ranges { >> - u8 slave_min; >> - u8 slave_max; >> - u8 master_min; >> - u8 master_max; >> -}; >> - >> -static struct pmu_port_event_ranges port_event_range[] = { >> - [CCI_REV_R0] = { >> - .slave_min = CCI_REV_R0_SLAVE_PORT_MIN_EV, >> - .slave_max = CCI_REV_R0_SLAVE_PORT_MAX_EV, >> - .master_min = CCI_REV_R0_MASTER_PORT_MIN_EV, >> - .master_max = CCI_REV_R0_MASTER_PORT_MAX_EV, >> - }, >> - [CCI_REV_R1] = { >> - .slave_min = CCI_REV_R1_SLAVE_PORT_MIN_EV, >> - .slave_max = CCI_REV_R1_SLAVE_PORT_MAX_EV, >> - .master_min = CCI_REV_R1_MASTER_PORT_MIN_EV, >> - .master_max = CCI_REV_R1_MASTER_PORT_MAX_EV, >> - }, >> -}; >> - >> -/* >> - * Export different PMU names for the different revisions so userspace knows >> - * because the event ids are different >> - */ >> -static char *const pmu_names[] = { >> - [CCI_REV_R0] = "CCI_400", >> - [CCI_REV_R1] = "CCI_400_r1", >> -}; >> - >> static int pmu_is_valid_slave_event(u8 ev_code) >> { >> - return pmu->port_ranges->slave_min <= ev_code && >> - ev_code <= pmu->port_ranges->slave_max; >> + return pmu->model->event_ranges[CCI_IF_SLAVE].min <= ev_code && >> + ev_code <= pmu->model->event_ranges[CCI_IF_SLAVE].max; >> } >> >> static int pmu_is_valid_master_event(u8 ev_code) >> { >> - return pmu->port_ranges->master_min <= ev_code && >> - ev_code <= pmu->port_ranges->master_max; >> + return pmu->model->event_ranges[CCI_IF_MASTER].min <= ev_code && >> + ev_code <= pmu->model->event_ranges[CCI_IF_MASTER].max; >> } >> >> static int pmu_validate_hw_event(u8 hw_event) >> @@ -234,11 +222,11 @@ static int probe_cci_revision(void) >> return CCI_REV_R1; >> } >> >> -static struct pmu_port_event_ranges *port_range_by_rev(void) >> +static const struct cci_pmu_model *probe_cci_model(struct platform_device *pdev) >> { >> - int rev = probe_cci_revision(); >> - >> - return &port_event_range[rev]; >> + if (platform_has_secure_cci_access()) >> + return &cci_pmu_models[probe_cci_revision()]; >> + return NULL; >> } >> >> static int pmu_is_valid_counter(struct cci_pmu *cci_pmu, int idx) >> @@ -807,9 +795,9 @@ static const struct attribute_group *pmu_attr_groups[] = { >> >> static int cci_pmu_init(struct cci_pmu *cci_pmu, struct platform_device *pdev) >> { >> - char *name = pmu_names[probe_cci_revision()]; >> + char *name = cci_pmu->model->name; >> cci_pmu->pmu = (struct pmu) { >> - .name = pmu_names[probe_cci_revision()], >> + .name = cci_pmu->model->name, >> .task_ctx_nr = perf_invalid_context, >> .pmu_enable = cci_pmu_enable, >> .pmu_disable = cci_pmu_disable, >> @@ -862,13 +850,65 @@ static struct notifier_block cci_pmu_cpu_nb = { >> .priority = CPU_PRI_PERF + 1, >> }; >> >> +static struct cci_pmu_model cci_pmu_models[] = { >> + [CCI_REV_R0] = { >> + .name = "CCI_400", >> + .event_ranges = { >> + [CCI_IF_SLAVE] = { >> + CCI_REV_R0_SLAVE_PORT_MIN_EV, >> + CCI_REV_R0_SLAVE_PORT_MAX_EV, >> + }, >> + [CCI_IF_MASTER] = { >> + CCI_REV_R0_MASTER_PORT_MIN_EV, >> + CCI_REV_R0_MASTER_PORT_MAX_EV, >> + }, >> + }, >> + }, >> + [CCI_REV_R1] = { >> + .name = "CCI_400_r1", >> + .event_ranges = { >> + [CCI_IF_SLAVE] = { >> + CCI_REV_R1_SLAVE_PORT_MIN_EV, >> + CCI_REV_R1_SLAVE_PORT_MAX_EV, >> + }, >> + [CCI_IF_MASTER] = { >> + CCI_REV_R1_MASTER_PORT_MIN_EV, >> + CCI_REV_R1_MASTER_PORT_MAX_EV, >> + }, >> + }, >> + }, >> +}; >> + >> static const struct of_device_id arm_cci_pmu_matches[] = { >> { >> .compatible = "arm,cci-400-pmu", >> + .data = NULL, >> + }, >> + { >> + .compatible = "arm,cci-400-pmu,r0", >> + .data = &cci_pmu_models[CCI_REV_R0], >> + }, >> + { >> + .compatible = "arm,cci-400-pmu,r1", >> + .data = &cci_pmu_models[CCI_REV_R1], >> }, >> {}, >> }; >> >> +static inline const struct cci_pmu_model *get_cci_model(struct platform_device *pdev) >> +{ >> + const struct of_device_id *match = of_match_node(arm_cci_pmu_matches, >> + pdev->dev.of_node); >> + if (!match) >> + return NULL; >> + if (match->data) >> + return match->data; >> + >> + dev_warn(&pdev->dev, "DEPERCATED compatible property," >> + "requires secure access to CCI registers"); >> + return probe_cci_model(pdev); >> +} >> + >> static bool is_duplicate_irq(int irq, int *irqs, int nr_irqs) >> { >> int i; >> @@ -884,11 +924,19 @@ static int cci_pmu_probe(struct platform_device *pdev) >> { >> struct resource *res; >> int i, ret, irq; >> + const struct cci_pmu_model *model; >> + >> + model = get_cci_model(pdev); >> + if (!model) { >> + dev_warn(&pdev->dev, "CCI PMU version not supported\n"); >> + return -EINVAL; >> + } >> >> pmu = devm_kzalloc(&pdev->dev, sizeof(*pmu), GFP_KERNEL); >> if (!pmu) >> return -ENOMEM; >> >> + pmu->model = model; >> res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> pmu->base = devm_ioremap_resource(&pdev->dev, res); >> if (IS_ERR(pmu->base)) >> @@ -920,12 +968,6 @@ static int cci_pmu_probe(struct platform_device *pdev) >> return -EINVAL; >> } >> >> - pmu->port_ranges = port_range_by_rev(); >> - if (!pmu->port_ranges) { >> - dev_warn(&pdev->dev, "CCI PMU version not supported\n"); >> - return -EINVAL; >> - } >> - >> raw_spin_lock_init(&pmu->hw_events.pmu_lock); >> mutex_init(&pmu->reserve_mutex); >> atomic_set(&pmu->active_events, 0); >> diff --git a/include/linux/arm-cci.h b/include/linux/arm-cci.h >> index 79d6edf..aede5c7 100644 >> --- a/include/linux/arm-cci.h >> +++ b/include/linux/arm-cci.h >> @@ -24,6 +24,8 @@ >> #include >> #include >> >> +#include >> + >> struct device_node; >> >> #ifdef CONFIG_ARM_CCI >> -- >> 1.7.9.5 >> >> >> > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html