Re: [PATCH 2/4] arm-cci: Get rid of secure transactions for PMU driver

[Suzuki K. Poulose]

On 24/02/15 21:53, Nicolas Pitre wrote:
> On Tue, 24 Feb 2015, Suzuki K. Poulose wrote:
>
> > From: "Suzuki K. Poulose" <suzuki.poulose@xxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx
> > Cc: Punit Agrawal <punit.agrawal@xxxxxxx>
> > Signed-off-by: Suzuki K. Poulose <suzuki.poulose@xxxxxxx>
> > ---
> >   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: <string>
> > -                     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 <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#ifndef __ASM_ARM_CCI_H
> > +#define __ASM_ARM_CCI_H
> > +
> > +#ifdef CONFIG_MCPM
> > +#include <asm/mcpm.h>
> > +
> > +/*
> > + * 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 <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#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 <linux/errno.h>
> >   #include <linux/types.h>
> >
> > +#include <asm/arm-cci.h>
> > +
> >   struct device_node;
> >
> >   #ifdef CONFIG_ARM_CCI
> > --
> > 1.7.9.5


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/