* [PATCH V8 3/7] perf tools: adding coresight etm PMU record capabilities
From: Mathieu Poirier @ 2016-09-16 15:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474041004-13956-1-git-send-email-mathieu.poirier@linaro.org>
Coresight ETMs are IP blocks used to perform HW assisted tracing
on a CPU core. This patch introduce the required auxiliary API
functions allowing the perf core to interact with a tracer.
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
---
MAINTAINERS | 4 +
tools/perf/arch/arm/util/Build | 2 +-
tools/perf/arch/arm/util/auxtrace.c | 54 ++++
tools/perf/arch/arm/util/cs-etm.c | 559 ++++++++++++++++++++++++++++++++++++
tools/perf/arch/arm/util/cs-etm.h | 23 ++
tools/perf/arch/arm64/util/Build | 4 +-
tools/perf/util/auxtrace.c | 1 +
tools/perf/util/auxtrace.h | 1 +
tools/perf/util/cs-etm.h | 74 +++++
9 files changed, 720 insertions(+), 2 deletions(-)
create mode 100644 tools/perf/arch/arm/util/auxtrace.c
create mode 100644 tools/perf/arch/arm/util/cs-etm.c
create mode 100644 tools/perf/arch/arm/util/cs-etm.h
create mode 100644 tools/perf/util/cs-etm.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 3ff44a5aa539..e8443349fb7d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1124,6 +1124,10 @@ F: Documentation/trace/coresight.txt
F: Documentation/devicetree/bindings/arm/coresight.txt
F: Documentation/ABI/testing/sysfs-bus-coresight-devices-*
F: tools/perf/arch/arm/util/pmu.c
+F: tools/perf/arch/arm/util/auxtrace.c
+F: tools/perf/arch/arm/util/cs-etm.c
+F: tools/perf/arch/arm/util/cs-etm.h
+F: tools/perf/util/cs-etm.h
ARM/CORGI MACHINE SUPPORT
M: Richard Purdie <rpurdie@rpsys.net>
diff --git a/tools/perf/arch/arm/util/Build b/tools/perf/arch/arm/util/Build
index 4093fd146f46..e64c5f216448 100644
--- a/tools/perf/arch/arm/util/Build
+++ b/tools/perf/arch/arm/util/Build
@@ -3,4 +3,4 @@ libperf-$(CONFIG_DWARF) += dwarf-regs.o
libperf-$(CONFIG_LOCAL_LIBUNWIND) += unwind-libunwind.o
libperf-$(CONFIG_LIBDW_DWARF_UNWIND) += unwind-libdw.o
-libperf-$(CONFIG_AUXTRACE) += pmu.o
+libperf-$(CONFIG_AUXTRACE) += pmu.o auxtrace.o cs-etm.o
diff --git a/tools/perf/arch/arm/util/auxtrace.c b/tools/perf/arch/arm/util/auxtrace.c
new file mode 100644
index 000000000000..8edf2cb71564
--- /dev/null
+++ b/tools/perf/arch/arm/util/auxtrace.c
@@ -0,0 +1,54 @@
+/*
+ * Copyright(C) 2015 Linaro Limited. All rights reserved.
+ * Author: Mathieu Poirier <mathieu.poirier@linaro.org>
+ *
+ * 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/>.
+ */
+
+#include <stdbool.h>
+#include <linux/coresight-pmu.h>
+
+#include "../../util/auxtrace.h"
+#include "../../util/evlist.h"
+#include "../../util/pmu.h"
+#include "cs-etm.h"
+
+struct auxtrace_record
+*auxtrace_record__init(struct perf_evlist *evlist, int *err)
+{
+ struct perf_pmu *cs_etm_pmu;
+ struct perf_evsel *evsel;
+ bool found_etm = false;
+
+ cs_etm_pmu = perf_pmu__find(CORESIGHT_ETM_PMU_NAME);
+
+ if (evlist) {
+ evlist__for_each_entry(evlist, evsel) {
+ if (cs_etm_pmu &&
+ evsel->attr.type == cs_etm_pmu->type)
+ found_etm = true;
+ }
+ }
+
+ if (found_etm)
+ return cs_etm_record_init(err);
+
+ /*
+ * Clear 'err' even if we haven't found a cs_etm event - that way perf
+ * record can still be used even if tracers aren't present. The NULL
+ * return value will take care of telling the infrastructure HW tracing
+ * isn't available.
+ */
+ *err = 0;
+ return NULL;
+}
diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
new file mode 100644
index 000000000000..829c479614f1
--- /dev/null
+++ b/tools/perf/arch/arm/util/cs-etm.c
@@ -0,0 +1,559 @@
+/*
+ * Copyright(C) 2015 Linaro Limited. All rights reserved.
+ * Author: Mathieu Poirier <mathieu.poirier@linaro.org>
+ *
+ * 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/>.
+ */
+
+#include <api/fs/fs.h>
+#include <linux/bitops.h>
+#include <linux/coresight-pmu.h>
+#include <linux/kernel.h>
+#include <linux/log2.h>
+#include <linux/types.h>
+
+#include "cs-etm.h"
+#include "../../perf.h"
+#include "../../util/auxtrace.h"
+#include "../../util/cpumap.h"
+#include "../../util/evlist.h"
+#include "../../util/pmu.h"
+#include "../../util/thread_map.h"
+#include "../../util/cs-etm.h"
+
+#include <stdlib.h>
+
+struct cs_etm_recording {
+ struct auxtrace_record itr;
+ struct perf_pmu *cs_etm_pmu;
+ struct perf_evlist *evlist;
+ bool snapshot_mode;
+ size_t snapshot_size;
+};
+
+static bool cs_etm_is_etmv4(struct auxtrace_record *itr, int cpu);
+
+static int cs_etm_parse_snapshot_options(struct auxtrace_record *itr,
+ struct record_opts *opts,
+ const char *str)
+{
+ struct cs_etm_recording *ptr =
+ container_of(itr, struct cs_etm_recording, itr);
+ unsigned long long snapshot_size = 0;
+ char *endptr;
+
+ if (str) {
+ snapshot_size = strtoull(str, &endptr, 0);
+ if (*endptr || snapshot_size > SIZE_MAX)
+ return -1;
+ }
+
+ opts->auxtrace_snapshot_mode = true;
+ opts->auxtrace_snapshot_size = snapshot_size;
+ ptr->snapshot_size = snapshot_size;
+
+ return 0;
+}
+
+static int cs_etm_recording_options(struct auxtrace_record *itr,
+ struct perf_evlist *evlist,
+ struct record_opts *opts)
+{
+ struct cs_etm_recording *ptr =
+ container_of(itr, struct cs_etm_recording, itr);
+ struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu;
+ struct perf_evsel *evsel, *cs_etm_evsel = NULL;
+ const struct cpu_map *cpus = evlist->cpus;
+ bool privileged = (geteuid() == 0 || perf_event_paranoid() < 0);
+
+ ptr->evlist = evlist;
+ ptr->snapshot_mode = opts->auxtrace_snapshot_mode;
+
+ evlist__for_each_entry(evlist, evsel) {
+ if (evsel->attr.type == cs_etm_pmu->type) {
+ if (cs_etm_evsel) {
+ pr_err("There may be only one %s event\n",
+ CORESIGHT_ETM_PMU_NAME);
+ return -EINVAL;
+ }
+ evsel->attr.freq = 0;
+ evsel->attr.sample_period = 1;
+ cs_etm_evsel = evsel;
+ opts->full_auxtrace = true;
+ }
+ }
+
+ /* no need to continue if at least one event of interest was found */
+ if (!cs_etm_evsel)
+ return 0;
+
+ if (opts->use_clockid) {
+ pr_err("Cannot use clockid (-k option) with %s\n",
+ CORESIGHT_ETM_PMU_NAME);
+ return -EINVAL;
+ }
+
+ /* we are in snapshot mode */
+ if (opts->auxtrace_snapshot_mode) {
+ /*
+ * No size were given to '-S' or '-m,', so go with
+ * the default
+ */
+ if (!opts->auxtrace_snapshot_size &&
+ !opts->auxtrace_mmap_pages) {
+ if (privileged) {
+ opts->auxtrace_mmap_pages = MiB(4) / page_size;
+ } else {
+ opts->auxtrace_mmap_pages =
+ KiB(128) / page_size;
+ if (opts->mmap_pages == UINT_MAX)
+ opts->mmap_pages = KiB(256) / page_size;
+ }
+ } else if (!opts->auxtrace_mmap_pages && !privileged &&
+ opts->mmap_pages == UINT_MAX) {
+ opts->mmap_pages = KiB(256) / page_size;
+ }
+
+ /*
+ * '-m,xyz' was specified but no snapshot size, so make the
+ * snapshot size as big as the auxtrace mmap area.
+ */
+ if (!opts->auxtrace_snapshot_size) {
+ opts->auxtrace_snapshot_size =
+ opts->auxtrace_mmap_pages * (size_t)page_size;
+ }
+
+ /*
+ * -Sxyz was specified but no auxtrace mmap area, so make the
+ * auxtrace mmap area big enough to fit the requested snapshot
+ * size.
+ */
+ if (!opts->auxtrace_mmap_pages) {
+ size_t sz = opts->auxtrace_snapshot_size;
+
+ sz = round_up(sz, page_size) / page_size;
+ opts->auxtrace_mmap_pages = roundup_pow_of_two(sz);
+ }
+
+ /* Snapshost size can't be bigger than the auxtrace area */
+ if (opts->auxtrace_snapshot_size >
+ opts->auxtrace_mmap_pages * (size_t)page_size) {
+ pr_err("Snapshot size %zu must not be greater than AUX area tracing mmap size %zu\n",
+ opts->auxtrace_snapshot_size,
+ opts->auxtrace_mmap_pages * (size_t)page_size);
+ return -EINVAL;
+ }
+
+ /* Something went wrong somewhere - this shouldn't happen */
+ if (!opts->auxtrace_snapshot_size ||
+ !opts->auxtrace_mmap_pages) {
+ pr_err("Failed to calculate default snapshot size and/or AUX area tracing mmap pages\n");
+ return -EINVAL;
+ }
+ }
+
+ /* We are in full trace mode but '-m,xyz' wasn't specified */
+ if (opts->full_auxtrace && !opts->auxtrace_mmap_pages) {
+ if (privileged) {
+ opts->auxtrace_mmap_pages = MiB(4) / page_size;
+ } else {
+ opts->auxtrace_mmap_pages = KiB(128) / page_size;
+ if (opts->mmap_pages == UINT_MAX)
+ opts->mmap_pages = KiB(256) / page_size;
+ }
+
+ }
+
+ /* Validate auxtrace_mmap_pages provided by user */
+ if (opts->auxtrace_mmap_pages) {
+ unsigned int max_page = (KiB(128) / page_size);
+ size_t sz = opts->auxtrace_mmap_pages * (size_t)page_size;
+
+ if (!privileged &&
+ opts->auxtrace_mmap_pages > max_page) {
+ opts->auxtrace_mmap_pages = max_page;
+ pr_err("auxtrace too big, truncating to %d\n",
+ max_page);
+ }
+
+ if (!is_power_of_2(sz)) {
+ pr_err("Invalid mmap size for %s: must be a power of 2\n",
+ CORESIGHT_ETM_PMU_NAME);
+ return -EINVAL;
+ }
+ }
+
+ if (opts->auxtrace_snapshot_mode)
+ pr_debug2("%s snapshot size: %zu\n", CORESIGHT_ETM_PMU_NAME,
+ opts->auxtrace_snapshot_size);
+
+ if (cs_etm_evsel) {
+ /*
+ * To obtain the auxtrace buffer file descriptor, the auxtrace
+ * event must come first.
+ */
+ perf_evlist__to_front(evlist, cs_etm_evsel);
+ /*
+ * In the case of per-cpu mmaps, we need the CPU on the
+ * AUX event.
+ */
+ if (!cpu_map__empty(cpus))
+ perf_evsel__set_sample_bit(cs_etm_evsel, CPU);
+ }
+
+ /* Add dummy event to keep tracking */
+ if (opts->full_auxtrace) {
+ struct perf_evsel *tracking_evsel;
+ int err;
+
+ err = parse_events(evlist, "dummy:u", NULL);
+ if (err)
+ return err;
+
+ tracking_evsel = perf_evlist__last(evlist);
+ perf_evlist__set_tracking_event(evlist, tracking_evsel);
+
+ tracking_evsel->attr.freq = 0;
+ tracking_evsel->attr.sample_period = 1;
+
+ /* In per-cpu case, always need the time of mmap events etc */
+ if (!cpu_map__empty(cpus))
+ perf_evsel__set_sample_bit(tracking_evsel, TIME);
+ }
+
+ return 0;
+}
+
+static u64 cs_etm_get_config(struct auxtrace_record *itr)
+{
+ u64 config = 0;
+ struct cs_etm_recording *ptr =
+ container_of(itr, struct cs_etm_recording, itr);
+ struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu;
+ struct perf_evlist *evlist = ptr->evlist;
+ struct perf_evsel *evsel;
+
+ evlist__for_each_entry(evlist, evsel) {
+ if (evsel->attr.type == cs_etm_pmu->type) {
+ /*
+ * Variable perf_event_attr::config is assigned to
+ * ETMv3/PTM. The bit fields have been made to match
+ * the ETMv3.5 ETRMCR register specification. See the
+ * PMU_FORMAT_ATTR() declarations in
+ * drivers/hwtracing/coresight/coresight-perf.c for
+ * details.
+ */
+ config = evsel->attr.config;
+ break;
+ }
+ }
+
+ return config;
+}
+
+static size_t
+cs_etm_info_priv_size(struct auxtrace_record *itr __maybe_unused,
+ struct perf_evlist *evlist __maybe_unused)
+{
+ int i;
+ int etmv3 = 0, etmv4 = 0;
+ const struct cpu_map *cpus = evlist->cpus;
+
+ /* cpu map is not empty, we have specific CPUs to work with */
+ if (!cpu_map__empty(cpus)) {
+ for (i = 0; i < cpu_map__nr(cpus); i++) {
+ if (cs_etm_is_etmv4(itr, cpus->map[i]))
+ etmv4++;
+ else
+ etmv3++;
+ }
+ } else {
+ /* get configuration for all CPUs in the system */
+ for (i = 0; i < cpu__max_cpu(); i++) {
+ if (cs_etm_is_etmv4(itr, i))
+ etmv4++;
+ else
+ etmv3++;
+ }
+ }
+
+ return (CS_ETM_HEADER_SIZE +
+ (etmv4 * CS_ETMV4_PRIV_SIZE) +
+ (etmv3 * CS_ETMV3_PRIV_SIZE));
+}
+
+static const char *metadata_etmv3_ro[CS_ETM_PRIV_MAX] = {
+ [CS_ETM_ETMCCER] = "mgmt/etmccer",
+ [CS_ETM_ETMIDR] = "mgmt/etmidr",
+};
+
+static const char *metadata_etmv4_ro[CS_ETMV4_PRIV_MAX] = {
+ [CS_ETMV4_TRCIDR0] = "trcidr/trcidr0",
+ [CS_ETMV4_TRCIDR1] = "trcidr/trcidr1",
+ [CS_ETMV4_TRCIDR2] = "trcidr/trcidr2",
+ [CS_ETMV4_TRCIDR8] = "trcidr/trcidr8",
+ [CS_ETMV4_TRCAUTHSTATUS] = "mgmt/trcauthstatus",
+};
+
+static bool cs_etm_is_etmv4(struct auxtrace_record *itr, int cpu)
+{
+ bool ret = false;
+ char path[PATH_MAX];
+ int scan;
+ unsigned int val;
+ struct cs_etm_recording *ptr =
+ container_of(itr, struct cs_etm_recording, itr);
+ struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu;
+
+ /* Take any of the RO files for ETMv4 and see if it present */
+ snprintf(path, PATH_MAX, "cpu%d/%s",
+ cpu, metadata_etmv4_ro[CS_ETMV4_TRCIDR0]);
+ scan = perf_pmu__scan_file(cs_etm_pmu, path, "%x", &val);
+
+ /* The file was read successfully, we have a winner */
+ if (scan == 1)
+ ret = true;
+
+ return ret;
+}
+
+static int cs_etm_get_ro(struct perf_pmu *pmu, int cpu, const char *path)
+{
+ char pmu_path[PATH_MAX];
+ int scan;
+ unsigned int val = 0;
+
+ /* Get RO metadata from sysfs */
+ snprintf(pmu_path, PATH_MAX, "cpu%d/%s", cpu, path);
+
+ scan = perf_pmu__scan_file(pmu, pmu_path, "%x", &val);
+ if (scan != 1)
+ pr_err("%s: error reading: %s\n", __func__, pmu_path);
+
+ return val;
+}
+
+static void cs_etm_get_metadata(int cpu, u32 *offset,
+ struct auxtrace_record *itr,
+ struct auxtrace_info_event *info)
+{
+ u32 increment;
+ u64 magic;
+ struct cs_etm_recording *ptr =
+ container_of(itr, struct cs_etm_recording, itr);
+ struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu;
+
+ /* first see what kind of tracer this cpu is affined to */
+ if (cs_etm_is_etmv4(itr, cpu)) {
+ magic = __perf_cs_etmv4_magic;
+ /* Get trace configuration register */
+ info->priv[*offset + CS_ETMV4_TRCCONFIGR] =
+ cs_etm_get_config(itr);
+ /* Get traceID from the framework */
+ info->priv[*offset + CS_ETMV4_TRCTRACEIDR] =
+ coresight_get_trace_id(cpu);
+ /* Get read-only information from sysFS */
+ info->priv[*offset + CS_ETMV4_TRCIDR0] =
+ cs_etm_get_ro(cs_etm_pmu, cpu,
+ metadata_etmv4_ro[CS_ETMV4_TRCIDR0]);
+ info->priv[*offset + CS_ETMV4_TRCIDR1] =
+ cs_etm_get_ro(cs_etm_pmu, cpu,
+ metadata_etmv4_ro[CS_ETMV4_TRCIDR1]);
+ info->priv[*offset + CS_ETMV4_TRCIDR2] =
+ cs_etm_get_ro(cs_etm_pmu, cpu,
+ metadata_etmv4_ro[CS_ETMV4_TRCIDR2]);
+ info->priv[*offset + CS_ETMV4_TRCIDR8] =
+ cs_etm_get_ro(cs_etm_pmu, cpu,
+ metadata_etmv4_ro[CS_ETMV4_TRCIDR8]);
+ info->priv[*offset + CS_ETMV4_TRCAUTHSTATUS] =
+ cs_etm_get_ro(cs_etm_pmu, cpu,
+ metadata_etmv4_ro
+ [CS_ETMV4_TRCAUTHSTATUS]);
+
+ /* How much space was used */
+ increment = CS_ETMV4_PRIV_MAX;
+ } else {
+ magic = __perf_cs_etmv3_magic;
+ /* Get configuration register */
+ info->priv[*offset + CS_ETM_ETMCR] = cs_etm_get_config(itr);
+ /* Get traceID from the framework */
+ info->priv[*offset + CS_ETM_ETMTRACEIDR] =
+ coresight_get_trace_id(cpu);
+ /* Get read-only information from sysFS */
+ info->priv[*offset + CS_ETM_ETMCCER] =
+ cs_etm_get_ro(cs_etm_pmu, cpu,
+ metadata_etmv3_ro[CS_ETM_ETMCCER]);
+ info->priv[*offset + CS_ETM_ETMIDR] =
+ cs_etm_get_ro(cs_etm_pmu, cpu,
+ metadata_etmv3_ro[CS_ETM_ETMIDR]);
+
+ /* How much space was used */
+ increment = CS_ETM_PRIV_MAX;
+ }
+
+ /* Build generic header portion */
+ info->priv[*offset + CS_ETM_MAGIC] = magic;
+ info->priv[*offset + CS_ETM_CPU] = cpu;
+ /* Where the next CPU entry should start from */
+ *offset += increment;
+}
+
+static int cs_etm_info_fill(struct auxtrace_record *itr,
+ struct perf_session *session,
+ struct auxtrace_info_event *info,
+ size_t priv_size)
+{
+ int i;
+ u32 offset;
+ u64 nr_cpu, type;
+ const struct cpu_map *cpus = session->evlist->cpus;
+ struct cs_etm_recording *ptr =
+ container_of(itr, struct cs_etm_recording, itr);
+ struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu;
+
+ if (priv_size != cs_etm_info_priv_size(itr, session->evlist))
+ return -EINVAL;
+
+ if (!session->evlist->nr_mmaps)
+ return -EINVAL;
+
+ /* If the cpu_map is empty all CPUs are involved */
+ nr_cpu = cpu_map__empty(cpus) ? cpu__max_cpu() : cpu_map__nr(cpus);
+ /* Get PMU type as dynamically assigned by the core */
+ type = cs_etm_pmu->type;
+
+ /* First fill out the session header */
+ info->type = PERF_AUXTRACE_CS_ETM;
+ info->priv[CS_HEADER_VERSION_0] = 0;
+ info->priv[CS_PMU_TYPE_CPUS] = type << 32;
+ info->priv[CS_PMU_TYPE_CPUS] |= nr_cpu;
+ info->priv[CS_ETM_SNAPSHOT] = ptr->snapshot_mode;
+
+ offset = CS_ETM_SNAPSHOT + 1;
+
+ /* cpu map is not empty, we have specific CPUs to work with */
+ if (!cpu_map__empty(cpus)) {
+ for (i = 0; i < cpu_map__nr(cpus) && offset < priv_size; i++)
+ cs_etm_get_metadata(cpus->map[i], &offset, itr, info);
+ } else {
+ /* get configuration for all CPUs in the system */
+ for (i = 0; i < cpu__max_cpu(); i++)
+ cs_etm_get_metadata(i, &offset, itr, info);
+ }
+
+ return 0;
+}
+
+static int cs_etm_find_snapshot(struct auxtrace_record *itr __maybe_unused,
+ int idx, struct auxtrace_mmap *mm,
+ unsigned char *data __maybe_unused,
+ u64 *head, u64 *old)
+{
+ pr_debug3("%s: mmap index %d old head %zu new head %zu size %zu\n",
+ __func__, idx, (size_t)*old, (size_t)*head, mm->len);
+
+ *old = *head;
+ *head += mm->len;
+
+ return 0;
+}
+
+static int cs_etm_snapshot_start(struct auxtrace_record *itr)
+{
+ struct cs_etm_recording *ptr =
+ container_of(itr, struct cs_etm_recording, itr);
+ struct perf_evsel *evsel;
+
+ evlist__for_each_entry(ptr->evlist, evsel) {
+ if (evsel->attr.type == ptr->cs_etm_pmu->type)
+ return perf_evsel__disable(evsel);
+ }
+ return -EINVAL;
+}
+
+static int cs_etm_snapshot_finish(struct auxtrace_record *itr)
+{
+ struct cs_etm_recording *ptr =
+ container_of(itr, struct cs_etm_recording, itr);
+ struct perf_evsel *evsel;
+
+ evlist__for_each_entry(ptr->evlist, evsel) {
+ if (evsel->attr.type == ptr->cs_etm_pmu->type)
+ return perf_evsel__enable(evsel);
+ }
+ return -EINVAL;
+}
+
+static u64 cs_etm_reference(struct auxtrace_record *itr __maybe_unused)
+{
+ return (((u64) rand() << 0) & 0x00000000FFFFFFFFull) |
+ (((u64) rand() << 32) & 0xFFFFFFFF00000000ull);
+}
+
+static void cs_etm_recording_free(struct auxtrace_record *itr)
+{
+ struct cs_etm_recording *ptr =
+ container_of(itr, struct cs_etm_recording, itr);
+ free(ptr);
+}
+
+static int cs_etm_read_finish(struct auxtrace_record *itr, int idx)
+{
+ struct cs_etm_recording *ptr =
+ container_of(itr, struct cs_etm_recording, itr);
+ struct perf_evsel *evsel;
+
+ evlist__for_each_entry(ptr->evlist, evsel) {
+ if (evsel->attr.type == ptr->cs_etm_pmu->type)
+ return perf_evlist__enable_event_idx(ptr->evlist,
+ evsel, idx);
+ }
+
+ return -EINVAL;
+}
+
+struct auxtrace_record *cs_etm_record_init(int *err)
+{
+ struct perf_pmu *cs_etm_pmu;
+ struct cs_etm_recording *ptr;
+
+ cs_etm_pmu = perf_pmu__find(CORESIGHT_ETM_PMU_NAME);
+
+ if (!cs_etm_pmu) {
+ *err = -EINVAL;
+ goto out;
+ }
+
+ ptr = zalloc(sizeof(struct cs_etm_recording));
+ if (!ptr) {
+ *err = -ENOMEM;
+ goto out;
+ }
+
+ ptr->cs_etm_pmu = cs_etm_pmu;
+ ptr->itr.parse_snapshot_options = cs_etm_parse_snapshot_options;
+ ptr->itr.recording_options = cs_etm_recording_options;
+ ptr->itr.info_priv_size = cs_etm_info_priv_size;
+ ptr->itr.info_fill = cs_etm_info_fill;
+ ptr->itr.find_snapshot = cs_etm_find_snapshot;
+ ptr->itr.snapshot_start = cs_etm_snapshot_start;
+ ptr->itr.snapshot_finish = cs_etm_snapshot_finish;
+ ptr->itr.reference = cs_etm_reference;
+ ptr->itr.free = cs_etm_recording_free;
+ ptr->itr.read_finish = cs_etm_read_finish;
+
+ *err = 0;
+ return &ptr->itr;
+out:
+ return NULL;
+}
diff --git a/tools/perf/arch/arm/util/cs-etm.h b/tools/perf/arch/arm/util/cs-etm.h
new file mode 100644
index 000000000000..909f486d02d1
--- /dev/null
+++ b/tools/perf/arch/arm/util/cs-etm.h
@@ -0,0 +1,23 @@
+/*
+ * Copyright(C) 2015 Linaro Limited. All rights reserved.
+ * Author: Mathieu Poirier <mathieu.poirier@linaro.org>
+ *
+ * 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 INCLUDE__PERF_CS_ETM_H__
+#define INCLUDE__PERF_CS_ETM_H__
+
+struct auxtrace_record *cs_etm_record_init(int *err);
+
+#endif
diff --git a/tools/perf/arch/arm64/util/Build b/tools/perf/arch/arm64/util/Build
index 3876dd05bb8b..cef6fb38d17e 100644
--- a/tools/perf/arch/arm64/util/Build
+++ b/tools/perf/arch/arm64/util/Build
@@ -1,4 +1,6 @@
libperf-$(CONFIG_DWARF) += dwarf-regs.o
libperf-$(CONFIG_LOCAL_LIBUNWIND) += unwind-libunwind.o
-libperf-$(CONFIG_AUXTRACE) += ../../arm/util/pmu.o
+libperf-$(CONFIG_AUXTRACE) += ../../arm/util/pmu.o \
+ ../../arm/util/auxtrace.o \
+ ../../arm/util/cs-etm.o
diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
index c9169011e55e..c0aba8e839aa 100644
--- a/tools/perf/util/auxtrace.c
+++ b/tools/perf/util/auxtrace.c
@@ -892,6 +892,7 @@ int perf_event__process_auxtrace_info(struct perf_tool *tool __maybe_unused,
return intel_pt_process_auxtrace_info(event, session);
case PERF_AUXTRACE_INTEL_BTS:
return intel_bts_process_auxtrace_info(event, session);
+ case PERF_AUXTRACE_CS_ETM:
case PERF_AUXTRACE_UNKNOWN:
default:
return -EINVAL;
diff --git a/tools/perf/util/auxtrace.h b/tools/perf/util/auxtrace.h
index ac5f0d7167e6..09286f193532 100644
--- a/tools/perf/util/auxtrace.h
+++ b/tools/perf/util/auxtrace.h
@@ -41,6 +41,7 @@ enum auxtrace_type {
PERF_AUXTRACE_UNKNOWN,
PERF_AUXTRACE_INTEL_PT,
PERF_AUXTRACE_INTEL_BTS,
+ PERF_AUXTRACE_CS_ETM,
};
enum itrace_period_type {
diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h
new file mode 100644
index 000000000000..3cc6bc3263fe
--- /dev/null
+++ b/tools/perf/util/cs-etm.h
@@ -0,0 +1,74 @@
+/*
+ * Copyright(C) 2015 Linaro Limited. All rights reserved.
+ * Author: Mathieu Poirier <mathieu.poirier@linaro.org>
+ *
+ * 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 INCLUDE__UTIL_PERF_CS_ETM_H__
+#define INCLUDE__UTIL_PERF_CS_ETM_H__
+
+/* Versionning header in case things need tro change in the future. That way
+ * decoding of old snapshot is still possible.
+ */
+enum {
+ /* Starting with 0x0 */
+ CS_HEADER_VERSION_0,
+ /* PMU->type (32 bit), total # of CPUs (32 bit) */
+ CS_PMU_TYPE_CPUS,
+ CS_ETM_SNAPSHOT,
+ CS_HEADER_VERSION_0_MAX,
+};
+
+/* Beginning of header common to both ETMv3 and V4 */
+enum {
+ CS_ETM_MAGIC,
+ CS_ETM_CPU,
+};
+
+/* ETMv3/PTM metadata */
+enum {
+ /* Dynamic, configurable parameters */
+ CS_ETM_ETMCR = CS_ETM_CPU + 1,
+ CS_ETM_ETMTRACEIDR,
+ /* RO, taken from sysFS */
+ CS_ETM_ETMCCER,
+ CS_ETM_ETMIDR,
+ CS_ETM_PRIV_MAX,
+};
+
+/* ETMv4 metadata */
+enum {
+ /* Dynamic, configurable parameters */
+ CS_ETMV4_TRCCONFIGR = CS_ETM_CPU + 1,
+ CS_ETMV4_TRCTRACEIDR,
+ /* RO, taken from sysFS */
+ CS_ETMV4_TRCIDR0,
+ CS_ETMV4_TRCIDR1,
+ CS_ETMV4_TRCIDR2,
+ CS_ETMV4_TRCIDR8,
+ CS_ETMV4_TRCAUTHSTATUS,
+ CS_ETMV4_PRIV_MAX,
+};
+
+#define KiB(x) ((x) * 1024)
+#define MiB(x) ((x) * 1024 * 1024)
+
+#define CS_ETM_HEADER_SIZE (CS_HEADER_VERSION_0_MAX * sizeof(u64))
+
+static const u64 __perf_cs_etmv3_magic = 0x3030303030303030ULL;
+static const u64 __perf_cs_etmv4_magic = 0x4040404040404040ULL;
+#define CS_ETMV3_PRIV_SIZE (CS_ETM_PRIV_MAX * sizeof(u64))
+#define CS_ETMV4_PRIV_SIZE (CS_ETMV4_PRIV_MAX * sizeof(u64))
+
+#endif
--
2.7.4
^ permalink raw reply related
* [PATCH V8 4/7] perf tools: add infrastructure for PMU specific configuration
From: Mathieu Poirier @ 2016-09-16 15:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474041004-13956-1-git-send-email-mathieu.poirier@linaro.org>
This patch adds PMU driver specific configuration to the parser
infrastructure by preceding any term with the '@' letter. As such
doing something like:
perf record -e some_event/@cfg1, at cfg2=config/ ...
will see 'cfg1' and 'cfg2=config' being added to the list of evsel config
terms. Token 'cfg1' and 'cfg2=config' are not processed in user space
and are meant to be interpreted by the PMU driver.
First the lexer/parser are supplemented with the required definitions to
recognise the driver specific configuration. From there they are simply
added to the list of event terms. The bulk of the work is done in
function "parse_events_add_pmu()" where driver config event terms are
added to a new list of driver config terms, which in turn spliced with
the event's new driver configuration list.
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Acked-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/Documentation/perf-record.txt | 12 ++++++++++++
tools/perf/util/evsel.h | 2 ++
tools/perf/util/parse-events.c | 7 ++++++-
tools/perf/util/parse-events.h | 1 +
tools/perf/util/parse-events.l | 22 ++++++++++++++++++++++
tools/perf/util/parse-events.y | 11 +++++++++++
6 files changed, 54 insertions(+), 1 deletion(-)
diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
index 379a2bed07c0..1a24f4d64328 100644
--- a/tools/perf/Documentation/perf-record.txt
+++ b/tools/perf/Documentation/perf-record.txt
@@ -60,6 +60,18 @@ OPTIONS
Note: If user explicitly sets options which conflict with the params,
the value set by the params will be overridden.
+ Also not defined in .../<pmu>/format/* are PMU driver specific
+ configuration parameters. Any configuration parameter preceded by
+ the letter '@' is not interpreted in user space and sent down directly
+ to the PMU driver. For example:
+
+ perf record -e some_event/@cfg1, at cfg2=config/ ...
+
+ will see 'cfg1' and 'cfg2=config' pushed to the PMU driver associated
+ with the event for further processing. There is no restriction on
+ what the configuration parameters are, as long as their semantic is
+ understood and supported by the PMU driver.
+
- a hardware breakpoint event in the form of '\mem:addr[/len][:access]'
where addr is the address in memory you want to break in.
Access is the memory access type (read, write, execute) it can
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 49c51fb3d05c..b1503b0ecdff 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -46,6 +46,7 @@ enum {
PERF_EVSEL__CONFIG_TERM_INHERIT,
PERF_EVSEL__CONFIG_TERM_MAX_STACK,
PERF_EVSEL__CONFIG_TERM_OVERWRITE,
+ PERF_EVSEL__CONFIG_TERM_DRV_CFG,
PERF_EVSEL__CONFIG_TERM_MAX,
};
@@ -57,6 +58,7 @@ struct perf_evsel_config_term {
u64 freq;
bool time;
char *callgraph;
+ char *drv_cfg;
u64 stack_user;
int max_stack;
bool inherit;
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index c23f2d5fc134..33546c3ac1fe 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -904,6 +904,7 @@ static const char *config_term_names[__PARSE_EVENTS__TERM_TYPE_NR] = {
[PARSE_EVENTS__TERM_TYPE_MAX_STACK] = "max-stack",
[PARSE_EVENTS__TERM_TYPE_OVERWRITE] = "overwrite",
[PARSE_EVENTS__TERM_TYPE_NOOVERWRITE] = "no-overwrite",
+ [PARSE_EVENTS__TERM_TYPE_DRV_CFG] = "driver-config",
};
static bool config_term_shrinked;
@@ -1034,7 +1035,8 @@ static int config_term_pmu(struct perf_event_attr *attr,
struct parse_events_term *term,
struct parse_events_error *err)
{
- if (term->type_term == PARSE_EVENTS__TERM_TYPE_USER)
+ if (term->type_term == PARSE_EVENTS__TERM_TYPE_USER ||
+ term->type_term == PARSE_EVENTS__TERM_TYPE_DRV_CFG)
/*
* Always succeed for sysfs terms, as we dont know
* at this point what type they need to have.
@@ -1134,6 +1136,9 @@ do { \
case PARSE_EVENTS__TERM_TYPE_NOOVERWRITE:
ADD_CONFIG_TERM(OVERWRITE, overwrite, term->val.num ? 0 : 1);
break;
+ case PARSE_EVENTS__TERM_TYPE_DRV_CFG:
+ ADD_CONFIG_TERM(DRV_CFG, drv_cfg, term->val.str);
+ break;
default:
break;
}
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index d1edbf8cc66a..8d09a976fca8 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -71,6 +71,7 @@ enum {
PARSE_EVENTS__TERM_TYPE_MAX_STACK,
PARSE_EVENTS__TERM_TYPE_NOOVERWRITE,
PARSE_EVENTS__TERM_TYPE_OVERWRITE,
+ PARSE_EVENTS__TERM_TYPE_DRV_CFG,
__PARSE_EVENTS__TERM_TYPE_NR,
};
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 7a2519435da0..9f43fda2570f 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -53,6 +53,26 @@ static int str(yyscan_t scanner, int token)
return token;
}
+/*
+ * This function is called when the parser gets two kind of input:
+ *
+ * @cfg1 or @cfg2=config
+ *
+ * The leading '@' is stripped off before 'cfg1' and 'cfg2=config' are given to
+ * bison. In the latter case it is necessary to keep the string intact so that
+ * the PMU kernel driver can determine what configurable is associated to
+ * 'config'.
+ */
+static int drv_str(yyscan_t scanner, int token)
+{
+ YYSTYPE *yylval = parse_events_get_lval(scanner);
+ char *text = parse_events_get_text(scanner);
+
+ /* Strip off the '@' */
+ yylval->str = strdup(text + 1);
+ return token;
+}
+
#define REWIND(__alloc) \
do { \
YYSTYPE *__yylval = parse_events_get_lval(yyscanner); \
@@ -124,6 +144,7 @@ num_hex 0x[a-fA-F0-9]+
num_raw_hex [a-fA-F0-9]+
name [a-zA-Z_*?][a-zA-Z0-9_*?.]*
name_minus [a-zA-Z_*?][a-zA-Z0-9\-_*?.:]*
+drv_cfg_term [a-zA-Z0-9_\.]+(=[a-zA-Z0-9_*?\.:]+)?
/* If you add a modifier you need to update check_modifier() */
modifier_event [ukhpPGHSDI]+
modifier_bp [rwx]{1,3}
@@ -209,6 +230,7 @@ no-overwrite { return term(yyscanner, PARSE_EVENTS__TERM_TYPE_NOOVERWRITE); }
{name_minus} { return str(yyscanner, PE_NAME); }
\[all\] { return PE_ARRAY_ALL; }
"[" { BEGIN(array); return '['; }
+@{drv_cfg_term} { return drv_str(yyscanner, PE_DRV_CFG_TERM); }
}
<mem>{
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 5be4a5f216d6..879115f93edc 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -49,6 +49,7 @@ static void inc_group_count(struct list_head *list,
%token PE_ERROR
%token PE_PMU_EVENT_PRE PE_PMU_EVENT_SUF PE_KERNEL_PMU_EVENT
%token PE_ARRAY_ALL PE_ARRAY_RANGE
+%token PE_DRV_CFG_TERM
%type <num> PE_VALUE
%type <num> PE_VALUE_SYM_HW
%type <num> PE_VALUE_SYM_SW
@@ -63,6 +64,7 @@ static void inc_group_count(struct list_head *list,
%type <str> PE_MODIFIER_BP
%type <str> PE_EVENT_NAME
%type <str> PE_PMU_EVENT_PRE PE_PMU_EVENT_SUF PE_KERNEL_PMU_EVENT
+%type <str> PE_DRV_CFG_TERM
%type <num> value_sym
%type <head> event_config
%type <head> opt_event_config
@@ -599,6 +601,15 @@ PE_NAME array '=' PE_VALUE
term->array = $2;
$$ = term;
}
+|
+PE_DRV_CFG_TERM
+{
+ struct parse_events_term *term;
+
+ ABORT_ON(parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_DRV_CFG,
+ $1, $1, &@1, NULL));
+ $$ = term;
+}
array:
'[' array_terms ']'
--
2.7.4
^ permalink raw reply related
* [PATCH V8 5/7] perf tools: pushing configuration down to PMU driver
From: Mathieu Poirier @ 2016-09-16 15:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474041004-13956-1-git-send-email-mathieu.poirier@linaro.org>
This patch adds a PMU callback and the required mechanic so that
drivers can process the command line configuration elements found
in evsel::config_terms.
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Acked-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/util/Build | 1 +
tools/perf/util/drv_configs.c | 77 +++++++++++++++++++++++++++++++++++++++++++
tools/perf/util/drv_configs.h | 26 +++++++++++++++
tools/perf/util/pmu.h | 2 ++
4 files changed, 106 insertions(+)
create mode 100644 tools/perf/util/drv_configs.c
create mode 100644 tools/perf/util/drv_configs.h
diff --git a/tools/perf/util/Build b/tools/perf/util/Build
index 91c5f6e1af59..53cbbbb6ddb9 100644
--- a/tools/perf/util/Build
+++ b/tools/perf/util/Build
@@ -85,6 +85,7 @@ libperf-y += term.o
libperf-y += help-unknown-cmd.o
libperf-y += mem-events.o
libperf-y += vsprintf.o
+libperf-y += drv_configs.o
libperf-$(CONFIG_LIBBPF) += bpf-loader.o
libperf-$(CONFIG_BPF_PROLOGUE) += bpf-prologue.o
diff --git a/tools/perf/util/drv_configs.c b/tools/perf/util/drv_configs.c
new file mode 100644
index 000000000000..1647f285c629
--- /dev/null
+++ b/tools/perf/util/drv_configs.c
@@ -0,0 +1,77 @@
+/*
+ * drv_configs.h: Interface to apply PMU specific configuration
+ * Copyright (c) 2016-2018, Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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.
+ *
+ */
+
+#include "drv_configs.h"
+#include "evlist.h"
+#include "evsel.h"
+#include "pmu.h"
+
+static int
+perf_evsel__apply_drv_configs(struct perf_evsel *evsel,
+ struct perf_evsel_config_term **err_term)
+{
+ bool found = false;
+ int err = 0;
+ struct perf_evsel_config_term *term;
+ struct perf_pmu *pmu = NULL;
+
+ while ((pmu = perf_pmu__scan(pmu)) != NULL)
+ if (pmu->type == evsel->attr.type) {
+ found = true;
+ break;
+ }
+
+ list_for_each_entry(term, &evsel->config_terms, list) {
+ if (term->type != PERF_EVSEL__CONFIG_TERM_DRV_CFG)
+ continue;
+
+ /*
+ * We have a configuration term, report an error if we
+ * can't find the PMU or if the PMU driver doesn't support
+ * cmd line driver configuration.
+ */
+ if (!found || !pmu->set_drv_config) {
+ err = -EINVAL;
+ *err_term = term;
+ break;
+ }
+
+ err = pmu->set_drv_config(term);
+ if (err) {
+ *err_term = term;
+ break;
+ }
+ }
+
+ return err;
+}
+
+int perf_evlist__apply_drv_configs(struct perf_evlist *evlist,
+ struct perf_evsel **err_evsel,
+ struct perf_evsel_config_term **err_term)
+{
+ struct perf_evsel *evsel;
+ int err = 0;
+
+ evlist__for_each_entry(evlist, evsel) {
+ err = perf_evsel__apply_drv_configs(evsel, err_term);
+ if (err) {
+ *err_evsel = evsel;
+ break;
+ }
+ }
+
+ return err;
+}
diff --git a/tools/perf/util/drv_configs.h b/tools/perf/util/drv_configs.h
new file mode 100644
index 000000000000..32bc9babc2e0
--- /dev/null
+++ b/tools/perf/util/drv_configs.h
@@ -0,0 +1,26 @@
+/*
+ * drv_configs.h: Interface to apply PMU specific configuration
+ * Copyright (c) 2016-2018, Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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.
+ *
+ */
+
+#ifndef __PERF_DRV_CONFIGS_H
+#define __PERF_DRV_CONFIGS_H
+
+#include "drv_configs.h"
+#include "evlist.h"
+#include "evsel.h"
+
+int perf_evlist__apply_drv_configs(struct perf_evlist *evlist,
+ struct perf_evsel **err_evsel,
+ struct perf_evsel_config_term **term);
+#endif
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 5d7e84466bee..743422ad900b 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -4,6 +4,7 @@
#include <linux/bitmap.h>
#include <linux/perf_event.h>
#include <stdbool.h>
+#include "evsel.h"
#include "parse-events.h"
enum {
@@ -25,6 +26,7 @@ struct perf_pmu {
struct list_head format; /* HEAD struct perf_pmu_format -> list */
struct list_head aliases; /* HEAD struct perf_pmu_alias -> list */
struct list_head list; /* ELEM */
+ int (*set_drv_config) (struct perf_evsel_config_term *term);
};
struct perf_pmu_info {
--
2.7.4
^ permalink raw reply related
* [PATCH V8 6/7] perf tools: adding PMU configuration to tools
From: Mathieu Poirier @ 2016-09-16 15:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474041004-13956-1-git-send-email-mathieu.poirier@linaro.org>
Now that the required mechanic is there to deal with PMU specific
configuration, add the functionality to the tools where events
can be selected.
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Acked-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/builtin-record.c | 10 ++++++++++
tools/perf/builtin-stat.c | 9 +++++++++
tools/perf/builtin-top.c | 13 +++++++++++++
3 files changed, 32 insertions(+)
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 6355902fbfc8..ada7fa29ec3f 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -22,6 +22,7 @@
#include "util/evlist.h"
#include "util/evsel.h"
#include "util/debug.h"
+#include "util/drv_configs.h"
#include "util/session.h"
#include "util/tool.h"
#include "util/symbol.h"
@@ -383,6 +384,7 @@ static int record__open(struct record *rec)
struct perf_evlist *evlist = rec->evlist;
struct perf_session *session = rec->session;
struct record_opts *opts = &rec->opts;
+ struct perf_evsel_config_term *err_term;
int rc = 0;
perf_evlist__config(evlist, opts, &callchain_param);
@@ -412,6 +414,14 @@ try_again:
goto out;
}
+ if (perf_evlist__apply_drv_configs(evlist, &pos, &err_term)) {
+ error("failed to set config \"%s\" on event %s with %d (%s)\n",
+ err_term->val.drv_cfg, perf_evsel__name(pos), errno,
+ strerror_r(errno, msg, sizeof(msg)));
+ rc = -1;
+ goto out;
+ }
+
rc = record__mmap(rec);
if (rc)
goto out;
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 3c7452b39f57..6b85eecdd1bc 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -52,6 +52,7 @@
#include "util/evlist.h"
#include "util/evsel.h"
#include "util/debug.h"
+#include "util/drv_configs.h"
#include "util/color.h"
#include "util/stat.h"
#include "util/header.h"
@@ -539,6 +540,7 @@ static int __run_perf_stat(int argc, const char **argv)
int status = 0;
const bool forks = (argc > 0);
bool is_pipe = STAT_RECORD ? perf_stat.file.is_pipe : false;
+ struct perf_evsel_config_term *err_term;
if (interval) {
ts.tv_sec = interval / 1000;
@@ -610,6 +612,13 @@ try_again:
return -1;
}
+ if (perf_evlist__apply_drv_configs(evsel_list, &counter, &err_term)) {
+ error("failed to set config \"%s\" on event %s with %d (%s)\n",
+ err_term->val.drv_cfg, perf_evsel__name(counter), errno,
+ strerror_r(errno, msg, sizeof(msg)));
+ return -1;
+ }
+
if (STAT_RECORD) {
int err, fd = perf_data_file__fd(&perf_stat.file);
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 418ed94756d3..0ce3852daaa0 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -24,6 +24,7 @@
#include "util/annotate.h"
#include "util/config.h"
#include "util/color.h"
+#include "util/drv_configs.h"
#include "util/evlist.h"
#include "util/evsel.h"
#include "util/machine.h"
@@ -940,6 +941,10 @@ static int callchain_param__setup_sample_type(struct callchain_param *callchain)
static int __cmd_top(struct perf_top *top)
{
+ char msg[512];
+ struct perf_evsel *pos;
+ struct perf_evsel_config_term *err_term;
+ struct perf_evlist *evlist = top->evlist;
struct record_opts *opts = &top->record_opts;
pthread_t thread;
int ret;
@@ -976,6 +981,14 @@ static int __cmd_top(struct perf_top *top)
if (ret)
goto out_delete;
+ ret = perf_evlist__apply_drv_configs(evlist, &pos, &err_term);
+ if (ret) {
+ error("failed to set config \"%s\" on event %s with %d (%s)\n",
+ err_term->val.drv_cfg, perf_evsel__name(pos), errno,
+ strerror_r(errno, msg, sizeof(msg)));
+ goto out_delete;
+ }
+
top->session->evlist = top->evlist;
perf_session__set_id_hdr_size(top->session);
--
2.7.4
^ permalink raw reply related
* [PATCH V8 7/7] perf tools: adding sink configuration for cs_etm PMU
From: Mathieu Poirier @ 2016-09-16 15:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474041004-13956-1-git-send-email-mathieu.poirier@linaro.org>
Using the PMU::set_drv_config() callback to enable the CoreSight
sink that will be used for the trace session.
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Acked-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/arch/arm/util/cs-etm.c | 58 +++++++++++++++++++++++++++++++++++++++
tools/perf/arch/arm/util/cs-etm.h | 3 ++
tools/perf/arch/arm/util/pmu.c | 2 ++
3 files changed, 63 insertions(+)
diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
index 829c479614f1..47d584da5819 100644
--- a/tools/perf/arch/arm/util/cs-etm.c
+++ b/tools/perf/arch/arm/util/cs-etm.c
@@ -27,12 +27,16 @@
#include "../../util/auxtrace.h"
#include "../../util/cpumap.h"
#include "../../util/evlist.h"
+#include "../../util/evsel.h"
#include "../../util/pmu.h"
#include "../../util/thread_map.h"
#include "../../util/cs-etm.h"
#include <stdlib.h>
+#define ENABLE_SINK_MAX 128
+#define CS_BUS_DEVICE_PATH "/bus/coresight/devices/"
+
struct cs_etm_recording {
struct auxtrace_record itr;
struct perf_pmu *cs_etm_pmu;
@@ -557,3 +561,57 @@ struct auxtrace_record *cs_etm_record_init(int *err)
out:
return NULL;
}
+
+static FILE *cs_device__open_file(const char *name)
+{
+ struct stat st;
+ char path[PATH_MAX];
+ const char *sysfs;
+
+ sysfs = sysfs__mountpoint();
+ if (!sysfs)
+ return NULL;
+
+ snprintf(path, PATH_MAX,
+ "%s" CS_BUS_DEVICE_PATH "%s", sysfs, name);
+
+ printf("path: %s\n", path);
+
+ if (stat(path, &st) < 0)
+ return NULL;
+
+ return fopen(path, "w");
+
+}
+
+static __attribute__((format(printf, 2, 3)))
+int cs_device__print_file(const char *name, const char *fmt, ...)
+{
+ va_list args;
+ FILE *file;
+ int ret = -EINVAL;
+
+ va_start(args, fmt);
+ file = cs_device__open_file(name);
+ if (file) {
+ ret = vfprintf(file, fmt, args);
+ fclose(file);
+ }
+ va_end(args);
+ return ret;
+}
+
+int cs_etm_set_drv_config(struct perf_evsel_config_term *term)
+{
+ int ret;
+ char enable_sink[ENABLE_SINK_MAX];
+
+ snprintf(enable_sink, ENABLE_SINK_MAX, "%s/%s",
+ term->val.drv_cfg, "enable_sink");
+
+ ret = cs_device__print_file(enable_sink, "%d", 1);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
diff --git a/tools/perf/arch/arm/util/cs-etm.h b/tools/perf/arch/arm/util/cs-etm.h
index 909f486d02d1..5256741be549 100644
--- a/tools/perf/arch/arm/util/cs-etm.h
+++ b/tools/perf/arch/arm/util/cs-etm.h
@@ -18,6 +18,9 @@
#ifndef INCLUDE__PERF_CS_ETM_H__
#define INCLUDE__PERF_CS_ETM_H__
+#include "../../util/evsel.h"
+
struct auxtrace_record *cs_etm_record_init(int *err);
+int cs_etm_set_drv_config(struct perf_evsel_config_term *term);
#endif
diff --git a/tools/perf/arch/arm/util/pmu.c b/tools/perf/arch/arm/util/pmu.c
index af9fb666b44f..98d67399a0d6 100644
--- a/tools/perf/arch/arm/util/pmu.c
+++ b/tools/perf/arch/arm/util/pmu.c
@@ -19,6 +19,7 @@
#include <linux/coresight-pmu.h>
#include <linux/perf_event.h>
+#include "cs-etm.h"
#include "../../util/pmu.h"
struct perf_event_attr
@@ -28,6 +29,7 @@ struct perf_event_attr
if (!strcmp(pmu->name, CORESIGHT_ETM_PMU_NAME)) {
/* add ETM default config here */
pmu->selectable = true;
+ pmu->set_drv_config = cs_etm_set_drv_config;
}
#endif
return NULL;
--
2.7.4
^ permalink raw reply related
* [PATCH] musb: Export musb_root_disconnect for use in modules
From: Mark Brown @ 2016-09-16 15:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160916154701.GA20742@kroah.com>
On Fri, Sep 16, 2016 at 05:47:01PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Sep 16, 2016 at 04:59:36PM +0200, Hans de Goede wrote:
> > +EXPORT_SYMBOL_GPL(musb_root_disconnect);
> Does this fix a build error somehow? Who reported it?
Yes, the sunxi driver uses this symbol and the build bots reported it
yesterday.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160916/1c94a40d/attachment-0001.sig>
^ permalink raw reply
* [PATCH 07/19] ahci: st: Remove STiH416 dt example
From: Tejun Heo @ 2016-09-16 15:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1473859677-9231-8-git-send-email-peter.griffin@linaro.org>
On Wed, Sep 14, 2016 at 02:27:45PM +0100, Peter Griffin wrote:
> This platform is being removed from the kernel so remove
> the dt example.
>
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> Cc: <tj@kernel.org>
> Cc: <robh+dt@kernel.org>
> Cc: <linux-ide@vger.kernel.org>
This is gonna go through dt tree, right? If it should go through
libata, please let me know.
Thanks.
--
tejun
^ permalink raw reply
* [PATCH v3 5/7] arm64: Handle faults caused by inadvertent user access with PAN enabled
From: Catalin Marinas @ 2016-09-16 15:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160916113325.GB21702@leverpostej>
On Fri, Sep 16, 2016 at 12:33:25PM +0100, Mark Rutland wrote:
> On Tue, Sep 13, 2016 at 06:46:35PM +0100, Catalin Marinas wrote:
> > When TTBR0_EL1 is set to the reserved page, an erroneous kernel access
> > to user space would generate a translation fault. This patch adds the
> > checks for the software-set PSR_PAN_BIT to emulate a permission fault
> > and report it accordingly.
>
> Why do we need to treat this case as a permission fault? It looks like a
> fault that wasn't a deliberate uaccess (which thus doesn't have a fixup
> handler) should have do_page_fault call __do_kernel_fault, where we
> should die().
We don't always check the exception table for a dedicated uaccess. The
only cases where the exception table is checked is when
down_read_trylock(mmap_sem) fails or CONFIG_DEBUG_VM is enabled. This is
a slight optimisation of the fast path of the page fault handling. So,
without is_permission_fault() (which much quicker than
search_exception_tables()) the kernel would keep restarting the same
instruction.
> > -static inline bool is_permission_fault(unsigned int esr)
> > +static inline bool is_permission_fault(unsigned int esr, struct pt_regs *regs)
> > {
> > unsigned int ec = ESR_ELx_EC(esr);
> > unsigned int fsc_type = esr & ESR_ELx_FSC_TYPE;
> >
> > - return (ec == ESR_ELx_EC_DABT_CUR && fsc_type == ESR_ELx_FSC_PERM) ||
> > - (ec == ESR_ELx_EC_IABT_CUR && fsc_type == ESR_ELx_FSC_PERM);
> > + if (ec != ESR_ELx_EC_DABT_CUR && ec != ESR_ELx_EC_IABT_CUR)
> > + return false;
> > +
> > + if (system_uses_ttbr0_pan())
> > + return fsc_type == ESR_ELx_FSC_FAULT &&
> > + (regs->pstate & PSR_PAN_BIT);
> > + else
> > + return fsc_type == ESR_ELx_FSC_PERM;
> > }
>
> Since the entry code will clear the PAN bit in the SPSR when we see the
> reserved ASID,
It actually sets the PAN bit in regs->pstate when it sees the reserved
ASID.
> faults in EFI runtime services will still be correctly reported,
> though that's somewhat subtle (and it took me a while to convince
> myself that was the case).
The EFI runtime services use a non-reserved ASID, so a fault taken while
executing EFI services would clear the PAN bit in regs->pstate. Such
faults would not trigger is_permission_fault() == true above.
> Also, I think that faults elsewhere may be misreported, e.g. in cold
> entry to kernel paths (idle, hotplug) where we have a global mapping in
> TTBR0, and it looks like we're using the reserved ASID.
>
> I'm not immediately sure how to distinguish those cases.
We don't normally expect faults on those paths. If we get one, they are
usually fatal, so the kernel still dies but with a slightly misleading
message.
We could improve this I think if we can distinguished between
reserved_ttbr0 after swapper (set on exception entry) and the reserved
TTBR0_EL1 pointing to empty_zero_page (and not merging Ard's patch). Is
it worth having such distinction? I'm not convinced, the only thing you
probably save is not printing "Accessing user space memory outside
uaccess.h routines" during fault (there may be other ways to mark these
special cases maybe by checking the current thread_info but it needs
more thinking).
--
Catalin
^ permalink raw reply
* [PATCH] musb: Export musb_root_disconnect for use in modules
From: Greg Kroah-Hartman @ 2016-09-16 15:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160916155317.GE27974@sirena.org.uk>
On Fri, Sep 16, 2016 at 04:53:17PM +0100, Mark Brown wrote:
> On Fri, Sep 16, 2016 at 05:47:01PM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Sep 16, 2016 at 04:59:36PM +0200, Hans de Goede wrote:
>
> > > +EXPORT_SYMBOL_GPL(musb_root_disconnect);
>
> > Does this fix a build error somehow? Who reported it?
>
> Yes, the sunxi driver uses this symbol and the build bots reported it
> yesterday.
then all of that should have been in this patch :(
So much for me trying to be subtle...
^ permalink raw reply
* [PATCH v26 0/7] arm64: add kdump support
From: James Morse @ 2016-09-16 16:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160907042908.6232-1-takahiro.akashi@linaro.org>
(Cc: Ard),
Mark, Ard, how does/will reserved-memory work on an APCI only system?
On 07/09/16 05:29, AKASHI Takahiro wrote:
> v26-specific note: After a comment from Rob[0], an idea of adding
> "linux,usable-memory-range" was dropped. Instead, an existing
> "reserved-memory" node will be used to limit usable memory ranges
> on crash dump kernel.
> This works not only on UEFI/ACPI systems but also on DT-only systems,
> but if he really insists on using DT-specific "usable-memory" property,
> I will post additional patches for kexec-tools. Those would be
> redundant, though.
> Even in that case, the kernel will not have to be changed.
Some narrative on how the old memory ranges get reserved, as there is no longer
any code in the series doing this, (which is pretty neat!):
kexec-tools parses the list of memory ranges in /proc/iomem, and adds a node to
the /reserved-memory for System RAM ranges that don't cover the crash kernel.
Decompiling the crash-kernel DT from Seattle, it looks roughly like this:
reserved-memory {
ranges;
#size-cells = <0x2>;
#address-cells = <0x2>;
crash_dump at 83ffe50000 {
no-map;
reg = <0x83 0xffe50000 0x0 0x1b0000>;
};
[ ... ]
};
'no-map' means its doing the same thing to memblock as
'linux,usable-memory-range' did in earlier versions,
early_init_dt_reserve_memory_arch() takes no-map to mean memblock_remove().
We trigger the removing via early_init_fdt_scan_reserved_mem() in
arch/arm64/mm/init.c. This happens later than before, but its before the
crashkernel and cma ranges get reserved.
One difference I can see is that before we avoided memblock_remove()ing ranges
that were also in memblock.nomap. This was to avoid the ACPI tables getting
mapped as device memory by mistake, this is fixed by [1]. Now these ranges are
published in /proc/iomem as 'reserved' and won't get covered by a
reserved-memory node, and so we don't need to check memblock.nomap when
memblock_remove()ing.
The only odd thing I can see is for a (mythical?) pure-ACPI system. The EFI stub
will create a DT with a chosen node containing pointers to the memory map and
the efi command line. Now such as system may also grow a /reserved-memory node
after kdump. I don't think this is a problem, but it may not match how an
acpi-only system reserves memory. (how does that work?)
> [1] "arm64: mark reserved memblock regions explicitly in iomem"
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-August/450433.html
This is queued in Will's arm64/for-next/core,
> [2] "efi: arm64: treat regions with WT/WC set but WB cleared as memory"
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-August/451491.html
This is queued in tip, but I can't see why kdump depends on it. It only has an
effect if the uefi memory map has !WB regions that linux needs to use.
Thanks,
James
^ permalink raw reply
* [PATCH v26 4/7] arm64: kdump: add VMCOREINFO's for user-space coredump tools
From: James Morse @ 2016-09-16 16:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160907042908.6232-5-takahiro.akashi@linaro.org>
On 07/09/16 05:29, AKASHI Takahiro wrote:
> For the current crash utility, we need to know, at least,
> - kimage_voffset
> - PHYS_OFFSET
> to handle the contents of core dump file (/proc/vmcore) correctly due to
> the introduction of KASLR (CONFIG_RANDOMIZE_BASE) in v4.6.
> This patch puts them as VMCOREINFO's into the file.
>
> - VA_BITS
> is also added for makedumpfile command.
> More VMCOREINFO's may be added later.
Reviewed-by: James Morse <james.morse@arm.com>
Thanks,
James
^ permalink raw reply
* [PATCH v26 6/7] arm64: kdump: update a kernel doc
From: James Morse @ 2016-09-16 16:08 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160907042908.6232-7-takahiro.akashi@linaro.org>
Hi Akashi,
On 07/09/16 05:29, AKASHI Takahiro wrote:
> This patch adds arch specific descriptions about kdump usage on arm64
> to kdump.txt.
> diff --git a/Documentation/kdump/kdump.txt b/Documentation/kdump/kdump.txt
> @@ -249,6 +249,13 @@ Dump-capture kernel config options (Arch Dependent, arm)
>
> AUTO_ZRELADDR=y
>
> +Dump-capture kernel config options (Arch Dependent, arm64)
> +----------------------------------------------------------
> +
> +- Please note that kvm of the dump-capture kernel will not be enabled
> + on non-VHE systems even if it is configured. This is because the CPU
> + cannot be reset to EL2 on panic.
Nit:
cannot be -> will not be
We could try to do this, but its more code that could prevent us reaching the
kdump kernel, so we choose not to.
> @@ -370,6 +381,9 @@ For s390x:
> For arm:
> "1 maxcpus=1 reset_devices"
>
> +For arm64:
> + "1 maxcpus=1 reset_devices"
> +
'maxcpus=1' is a bit fragile. Since 44dbcc93ab67145 ("arm64: Fix behavior of
maxcpus=N") udev on ubuntu vivid (running on Juno) has taken it upon itself to
bring the secondary cores online, even when booted with 'maxcpus=1'.
Can we change the recomendation to "1 nosmp reset_devices"?
Thanks,
James
^ permalink raw reply
* [PATCH v7 00/22] Generic DT bindings for PCI IOMMUs and ARM SMMU
From: Robin Murphy @ 2016-09-16 16:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1838c65d-5944-8946-781c-b420bea1acab@redhat.com>
Hi Eric,
On 15/09/16 17:46, Auger Eric wrote:
[...]
> Hum OK; thanks for the explanation. With that implementation however,
> don't we face back the issue we encountered in early stage of default
> domain implementation:
>
> With this sample config (AMD overdrive + I350-T2 + 2VFs per PF) I fill
> the 8 context banks. Whereas practically we didn't need them before?
>
> 00:00.0 0600: 1022:1a00
> Subsystem: 1022:1a00
> 00:02.0 0600: 1022:1a01
> 00:02.2 0604: 1022:1a02
> Kernel driver in use: pcieport
> 01:00.0 0200: 8086:1521 (rev 01)
> Subsystem: 8086:0002
> Kernel driver in use: igb
> 01:00.1 0200: 8086:1521 (rev 01)
> Subsystem: 8086:0002
> Kernel driver in use: igb
> 01:10.0 0200: 8086:1520 (rev 01) -> context 5
> Subsystem: 8086:0002
> Kernel driver in use: vfio-pci
> 01:10.1 0200: 8086:1520 (rev 01) -> context 7
> Subsystem: 8086:0002
> Kernel driver in use: igbvf
> 01:10.4 0200: 8086:1520 (rev 01) -> context 6
> Subsystem: 8086:0002
> Kernel driver in use: igbvf
> 01:10.5 0200: 8086:1520 (rev 01) -> shortage
> Subsystem: 8086:0002
> Kernel driver in use: igbvf
>
> So I can't even do passthrough anymore with that config. Is there
> anything wrong in my setup/understanding?
It's kind of hard to avoid, really - people want DMA ops support (try
plugging a card which can only do 32-bit DMA into that Seattle, for
instance); DMA ops need default domains; default domains are allocated
per group; each domain requires a context bank to back it; thus if you
have 9 groups and 8 context banks you're in a corner. It would relieve
the pressure to have a single default domain per SMMU, but we've no way
to do that due to the way the iommu_domain_alloc() API is intended to work.
Ultimately, it's a hardware limitation of that platform - plug in a card
with 16 VFs with ACS, and either way you're stuck. There are a number of
bodges I can think of that would make your specific situation work, but
none of them are really sufficiently general to consider upstreaming.
The most logical thing to do right now, if you were happy without DMA
ops using the old binding before, is to keep using the old binding (just
fixing your DT with #stream-id-cells=0 on the host controller so as not
to create the fake aliasing problem). Hopefully future platforms will be
in a position to couple their PCI host controllers to an IOMMU which is
actually designed to support a PCI host controller.
What I probably will do, though, since we have the functionality in
place for the sake of the old binding, and I think there are other folks
who want PCI iommu-map support but would prefer not to bother with DMA
ops on the host, is add a command-line option to disable DMA domains
even for the generic bindings.
Robin.
^ permalink raw reply
* [PATCH v7 0/5] mfd: tps65218: Clean ups
From: Keerthy @ 2016-09-16 16:21 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160916133153.GB27974@sirena.org.uk>
On Friday 16 September 2016 07:01 PM, Mark Brown wrote:
> On Fri, Sep 16, 2016 at 05:12:38PM +0530, Keerthy wrote:
>
>> Should i repost this series? I do not see this series in linux-next yet.
>
> Please don't send content free pings and please allow a reasonable time
> for review. People get busy, go on holiday, attend conferences and so
> on so unless there is some reason for urgency (like critical bug fixes)
> please allow at least a couple of weeks for review. If there have been
> review comments then people may be waiting for those to be addressed.
I should have been clearer.
The last Lee Jones conveyed that:
"I can't take this series yet, since it relies on a change which was
taken into Mark's Regulator tree"
https://lkml.org/lkml/2016/8/31/312
So wanted to check if i needed to re-base/repost this series again.
Sorry about the confusion i think that is because of $Subject goof up in
the 0th patch of the series.
>
> Sending content free pings adds to the mail volume (if they are seen at
> all) which is often the problem and since they can't be reviewed
> directly if something has gone wrong you'll have to resend the patches
> anyway, though there are some other maintainers who like them - if in
> doubt look at how patches for the subsystem are normally handled.
>
^ permalink raw reply
* [PATCH v5 1/6] arm/arm64: vgic-new: Implement support for userspace access
From: Marc Zyngier @ 2016-09-16 16:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474028453-29132-2-git-send-email-vijay.kilari@gmail.com>
On 16/09/16 13:20, vijay.kilari at gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>
> Read and write of some registers like ISPENDR and ICPENDR
> from userspace requires special handling when compared to
> guest access for these registers.
>
> Refer to Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> for handling of ISPENDR, ICPENDR registers handling.
>
> Add infrastructure to support guest and userspace read
> and write for the required registers
> Also moved vgic_uaccess from vgic-mmio-v2.c to vgic-mmio.c
>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> ---
> virt/kvm/arm/vgic/vgic-mmio-v2.c | 25 ----------
> virt/kvm/arm/vgic/vgic-mmio-v3.c | 98 ++++++++++++++++++++++++++++++++--------
> virt/kvm/arm/vgic/vgic-mmio.c | 78 ++++++++++++++++++++++++++++----
> virt/kvm/arm/vgic/vgic-mmio.h | 19 ++++++++
> 4 files changed, 169 insertions(+), 51 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index b44b359..0b32f40 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -406,31 +406,6 @@ int vgic_v2_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
> return -ENXIO;
> }
>
> -/*
> - * When userland tries to access the VGIC register handlers, we need to
> - * create a usable struct vgic_io_device to be passed to the handlers and we
> - * have to set up a buffer similar to what would have happened if a guest MMIO
> - * access occurred, including doing endian conversions on BE systems.
> - */
> -static int vgic_uaccess(struct kvm_vcpu *vcpu, struct vgic_io_device *dev,
> - bool is_write, int offset, u32 *val)
> -{
> - unsigned int len = 4;
> - u8 buf[4];
> - int ret;
> -
> - if (is_write) {
> - vgic_data_host_to_mmio_bus(buf, len, *val);
> - ret = kvm_io_gic_ops.write(vcpu, &dev->dev, offset, len, buf);
> - } else {
> - ret = kvm_io_gic_ops.read(vcpu, &dev->dev, offset, len, buf);
> - if (!ret)
> - *val = vgic_data_mmio_bus_to_host(buf, len);
> - }
> -
> - return ret;
> -}
> -
> int vgic_v2_cpuif_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> int offset, u32 *val)
> {
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index 0d3c76a..edd3d40 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -209,6 +209,62 @@ static unsigned long vgic_mmio_read_v3_idregs(struct kvm_vcpu *vcpu,
> return 0;
> }
>
> +static unsigned long vgic_uaccess_read_v3_pending(struct kvm_vcpu *vcpu,
Maming is completely inconsistent. This should read "vgic_v3_uaccess_read_pending".
> + gpa_t addr, unsigned int len)
> +{
> + u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> + u32 value = 0;
> + int i;
> +
> + /*
> + * A level triggerred interrupt pending state is latched in both
> + * "soft_pending" and "line_level" variables. Userspace will save
> + * and restore soft_pending and line_level separately.
> + * Refer to Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> + * handling of ISPENDR and ICPENDR.
> + */
> + for (i = 0; i < len * 8; i++) {
> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> +
> + if (irq->config == VGIC_CONFIG_LEVEL && irq->soft_pending)
> + value |= (1U << i);
> + if (irq->config == VGIC_CONFIG_EDGE && irq->pending)
> + value |= (1U << i);
> +
> + vgic_put_irq(vcpu->kvm, irq);
> + }
> +
> + return value;
> +}
> +
> +static void vgic_uaccess_write_v3_pending(struct kvm_vcpu *vcpu,
Same here.
> + gpa_t addr, unsigned int len,
> + unsigned long val)
> +{
> + u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> + int i;
> +
> + for (i = 0; i < len * 8; i++) {
> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> +
> + spin_lock(&irq->irq_lock);
> + if (test_bit(i, &val)) {
> + irq->pending = true;
> + irq->soft_pending = true;
> + vgic_queue_irq_unlock(vcpu->kvm, irq);
> + } else {
> + irq->soft_pending = false;
> + if (irq->config == VGIC_CONFIG_EDGE ||
> + (irq->config == VGIC_CONFIG_LEVEL &&
> + !irq->line_level))
> + irq->pending = false;
> + spin_unlock(&irq->irq_lock);
> + }
> +
> + vgic_put_irq(vcpu->kvm, irq);
> + }
> +}
> +
> /* We want to avoid outer shareable. */
> u64 vgic_sanitise_shareability(u64 field)
> {
> @@ -358,7 +414,7 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu,
> * We take some special care here to fix the calculation of the register
> * offset.
> */
> -#define REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(off, rd, wr, bpi, acc) \
> +#define REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(off, rd, wr, ur, uw, bpi, acc) \
> { \
> .reg_offset = off, \
> .bits_per_irq = bpi, \
> @@ -373,6 +429,8 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu,
> .access_flags = acc, \
> .read = rd, \
> .write = wr, \
> + .uaccess_read = ur, \
> + .uaccess_write = uw, \
> }
>
> static const struct vgic_register_region vgic_v3_dist_registers[] = {
> @@ -380,40 +438,42 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
> vgic_mmio_read_v3_misc, vgic_mmio_write_v3_misc, 16,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IGROUPR,
> - vgic_mmio_read_rao, vgic_mmio_write_wi, 1,
> + vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISENABLER,
> - vgic_mmio_read_enable, vgic_mmio_write_senable, 1,
> + vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICENABLER,
> - vgic_mmio_read_enable, vgic_mmio_write_cenable, 1,
> + vgic_mmio_read_enable, vgic_mmio_write_cenable, NULL, NULL, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISPENDR,
> - vgic_mmio_read_pending, vgic_mmio_write_spending, 1,
> + vgic_mmio_read_pending, vgic_mmio_write_spending,
> + vgic_uaccess_read_v3_pending, vgic_uaccess_write_v3_pending, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICPENDR,
> - vgic_mmio_read_pending, vgic_mmio_write_cpending, 1,
> + vgic_mmio_read_pending, vgic_mmio_write_cpending,
> + vgic_uaccess_read_v3_pending, vgic_mmio_write_wi, 1,
>From the spec you quoted in the commit message:
+ Accesses to GICD_ICPENDR register region and GICR_ICPENDR0 registers have
+ RAZ/WI semantics, meaning that reads always return 0 and writes are always
+ ignored.
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISACTIVER,
> - vgic_mmio_read_active, vgic_mmio_write_sactive, 1,
> + vgic_mmio_read_active, vgic_mmio_write_sactive, NULL, NULL, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICACTIVER,
> - vgic_mmio_read_active, vgic_mmio_write_cactive, 1,
> + vgic_mmio_read_active, vgic_mmio_write_cactive, NULL, NULL, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IPRIORITYR,
> - vgic_mmio_read_priority, vgic_mmio_write_priority, 8,
> - VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),
> + vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
> + 8, VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ITARGETSR,
> - vgic_mmio_read_raz, vgic_mmio_write_wi, 8,
> + vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 8,
> VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICFGR,
> - vgic_mmio_read_config, vgic_mmio_write_config, 2,
> + vgic_mmio_read_config, vgic_mmio_write_config, NULL, NULL, 2,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IGRPMODR,
> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
> + vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IROUTER,
> - vgic_mmio_read_irouter, vgic_mmio_write_irouter, 64,
> + vgic_mmio_read_irouter, vgic_mmio_write_irouter, NULL, NULL, 64,
> VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_LENGTH(GICD_IDREGS,
> vgic_mmio_read_v3_idregs, vgic_mmio_write_wi, 48,
> @@ -451,11 +511,13 @@ static const struct vgic_register_region vgic_v3_sgibase_registers[] = {
> REGISTER_DESC_WITH_LENGTH(GICR_ICENABLER0,
> vgic_mmio_read_enable, vgic_mmio_write_cenable, 4,
> VGIC_ACCESS_32bit),
> - REGISTER_DESC_WITH_LENGTH(GICR_ISPENDR0,
> - vgic_mmio_read_pending, vgic_mmio_write_spending, 4,
> + REGISTER_DESC_WITH_LENGTH_UACCESS(GICR_ISPENDR0,
> + vgic_mmio_read_pending, vgic_mmio_write_spending,
> + vgic_uaccess_read_v3_pending, vgic_uaccess_write_v3_pending, 4,
> VGIC_ACCESS_32bit),
> - REGISTER_DESC_WITH_LENGTH(GICR_ICPENDR0,
> - vgic_mmio_read_pending, vgic_mmio_write_cpending, 4,
> + REGISTER_DESC_WITH_LENGTH_UACCESS(GICR_ICPENDR0,
> + vgic_mmio_read_pending, vgic_mmio_write_cpending,
> + vgic_uaccess_read_v3_pending, vgic_mmio_write_wi, 4,
Same issue.
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_LENGTH(GICR_ISACTIVER0,
> vgic_mmio_read_active, vgic_mmio_write_sactive, 4,
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> index e18b30d..31f85df 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -468,6 +468,73 @@ static bool check_region(const struct vgic_register_region *region,
> return false;
> }
>
> +static const struct vgic_register_region *
> +vgic_get_mmio_region(struct vgic_io_device *iodev, gpa_t addr, int len)
> +{
> + const struct vgic_register_region *region;
> +
> + region = vgic_find_mmio_region(iodev->regions, iodev->nr_regions,
> + addr - iodev->base_addr);
> + if (!region || !check_region(region, addr, len))
> + return NULL;
> +
> + return region;
> +}
> +
> +static int vgic_uaccess_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
> + gpa_t addr, u32 *val)
> +{
> + struct vgic_io_device *iodev = kvm_to_vgic_iodev(dev);
> + const struct vgic_register_region *region;
> + struct kvm_vcpu *r_vcpu;
> +
> + region = vgic_get_mmio_region(iodev, addr, sizeof(u32));
> + if (!region) {
> + *val = 0;
> + return 0;
> + }
> +
> + r_vcpu = iodev->redist_vcpu ? iodev->redist_vcpu : vcpu;
> + if (region->uaccess_read)
> + *val = region->uaccess_read(r_vcpu, addr, sizeof(u32));
> + else
> + *val = region->read(r_vcpu, addr, sizeof(u32));
> +
> + return 0;
> +}
> +
> +static int vgic_uaccess_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
> + gpa_t addr, const u32 *val)
> +{
> + struct vgic_io_device *iodev = kvm_to_vgic_iodev(dev);
> + const struct vgic_register_region *region;
> + struct kvm_vcpu *r_vcpu;
> +
> + region = vgic_get_mmio_region(iodev, addr, sizeof(u32));
> + if (!region)
> + return 0;
> +
> + r_vcpu = iodev->redist_vcpu ? iodev->redist_vcpu : vcpu;
> + if (region->uaccess_write)
> + region->uaccess_write(r_vcpu, addr, sizeof(u32), *val);
> + else
> + region->write(r_vcpu, addr, sizeof(u32), *val);
> +
> + return 0;
> +}
> +
> +/*
> + * Userland access to VGIC registers.
> + */
> +int vgic_uaccess(struct kvm_vcpu *vcpu, struct vgic_io_device *dev,
> + bool is_write, int offset, u32 *val)
> +{
> + if (is_write)
> + return vgic_uaccess_write(vcpu, &dev->dev, offset, val);
> + else
> + return vgic_uaccess_read(vcpu, &dev->dev, offset, val);
> +}
> +
> static int dispatch_mmio_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
> gpa_t addr, int len, void *val)
> {
> @@ -475,9 +542,8 @@ static int dispatch_mmio_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
> const struct vgic_register_region *region;
> unsigned long data = 0;
>
> - region = vgic_find_mmio_region(iodev->regions, iodev->nr_regions,
> - addr - iodev->base_addr);
> - if (!region || !check_region(region, addr, len)) {
> + region = vgic_get_mmio_region(iodev, addr, len);
> + if (!region) {
> memset(val, 0, len);
> return 0;
> }
> @@ -508,14 +574,10 @@ static int dispatch_mmio_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
> const struct vgic_register_region *region;
> unsigned long data = vgic_data_mmio_bus_to_host(val, len);
>
> - region = vgic_find_mmio_region(iodev->regions, iodev->nr_regions,
> - addr - iodev->base_addr);
> + region = vgic_get_mmio_region(iodev, addr, len);
> if (!region)
> return 0;
>
> - if (!check_region(region, addr, len))
> - return 0;
> -
> switch (iodev->iodev_type) {
> case IODEV_CPUIF:
> region->write(vcpu, addr, len, data);
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h
> index 4c34d39..97e6df7 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.h
> +++ b/virt/kvm/arm/vgic/vgic-mmio.h
> @@ -34,6 +34,10 @@ struct vgic_register_region {
> gpa_t addr, unsigned int len,
> unsigned long val);
> };
> + unsigned long (*uaccess_read)(struct kvm_vcpu *vcpu, gpa_t addr,
> + unsigned int len);
> + void (*uaccess_write)(struct kvm_vcpu *vcpu, gpa_t addr,
> + unsigned int len, unsigned long val);
> };
>
> extern struct kvm_io_device_ops kvm_io_gic_ops;
> @@ -86,6 +90,18 @@ extern struct kvm_io_device_ops kvm_io_gic_ops;
> .write = wr, \
> }
>
> +#define REGISTER_DESC_WITH_LENGTH_UACCESS(off, rd, wr, urd, uwr, length, acc) \
> + { \
> + .reg_offset = off, \
> + .bits_per_irq = 0, \
> + .len = length, \
> + .access_flags = acc, \
> + .read = rd, \
> + .write = wr, \
> + .uaccess_read = urd, \
> + .uaccess_write = uwr, \
> + }
> +
> int kvm_vgic_register_mmio_region(struct kvm *kvm, struct kvm_vcpu *vcpu,
> struct vgic_register_region *reg_desc,
> struct vgic_io_device *region,
> @@ -158,6 +174,9 @@ void vgic_mmio_write_config(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len,
> unsigned long val);
>
> +int vgic_uaccess(struct kvm_vcpu *vcpu, struct vgic_io_device *dev,
> + bool is_write, int offset, u32 *val);
> +
> unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev);
>
> unsigned int vgic_v3_init_dist_iodev(struct vgic_io_device *dev);
>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH v9 08/10] arm64: pmu: Detect and enable multiple PMUs in an ACPI system
From: Jeremy Linton @ 2016-09-16 16:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <87y42st1e3.fsf@e105922-lin.cambridge.arm.com>
Hi,
On 09/16/2016 08:33 AM, Punit Agrawal wrote:
> Jeremy Linton <jeremy.linton@arm.com> writes:
>
>> Its possible that an ACPI system has multiple CPU types in it
>> with differing PMU counters. Iterate the CPU's and make a determination
>> about how many of each type exist in the system. Then take and create
>> a PMU platform device for each type, and assign it the interrupts parsed
>> from the MADT. Creating a platform device is necessary because the PMUs
>> are not described as devices in the DSDT table.
>>
>> This code is loosely based on earlier work by Mark Salter.
(trimming)
>> +
>> + list_for_each_entry_safe(pmu, safe_temp, &pmus, list) {
>> + if (unused_madt_entries)
>> + pmu->cpu_count = num_possible_cpus();
>
> So if there is any unbooted cpu ...
>
>> +
>> + res = kcalloc(pmu->cpu_count,
>> + sizeof(struct resource), GFP_KERNEL);
>
> ... we allocate potentially large number (num_possible_cpus()) of
> resources for each PMU.
>
> This is needlessly wasteful. Under what conditions have you found
> reg_midr to be 0?
Unused MADT entries, in place for potentially unbooted/hotplug CPUs. In
those cases you don't know for sure which PMU the CPU belongs to until
it comes online and the MIDR can be read. I'm open to suggestions on how
to deal with this, outside of pushing my luck and further breaking the
platform device encapsulation by trying to reallocate the resource
structure while its active. Besides its only wasteful for
ACPI+big.little, which at the moment only applies to a development platform.
^ permalink raw reply
* [PATCH v9 07/10] arm: arm64: pmu: Assign platform PMU CPU affinity
From: Punit Agrawal @ 2016-09-16 16:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1473892358-22574-8-git-send-email-jeremy.linton@arm.com>
Hi Jeremy,
One comment below.
Jeremy Linton <jeremy.linton@arm.com> writes:
> On systems with multiple PMU types the PMU to CPU affinity
> needs to be detected and set. The CPU to interrupt affinity
> should also be set.
>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
> drivers/perf/arm_pmu.c | 63 ++++++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 53 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> index 58117d7..63f16a5 100644
> --- a/drivers/perf/arm_pmu.c
> +++ b/drivers/perf/arm_pmu.c
> @@ -11,6 +11,7 @@
> */
> #define pr_fmt(fmt) "hw perfevents: " fmt
>
> +#include <linux/acpi.h>
> #include <linux/bitmap.h>
> #include <linux/cpumask.h>
> #include <linux/cpu_pm.h>
> @@ -24,6 +25,7 @@
> #include <linux/irq.h>
> #include <linux/irqdesc.h>
>
> +#include <asm/cpu.h>
> #include <asm/cputype.h>
> #include <asm/irq_regs.h>
>
> @@ -876,25 +878,67 @@ static void cpu_pmu_destroy(struct arm_pmu *cpu_pmu)
> }
>
> /*
> - * CPU PMU identification and probing.
> + * CPU PMU identification and probing. Its possible to have
> + * multiple CPU types in an ARM machine. Assure that we are
> + * picking the right PMU types based on the CPU in question
> */
> -static int probe_current_pmu(struct arm_pmu *pmu,
> - const struct pmu_probe_info *info)
> +static int probe_plat_pmu(struct arm_pmu *pmu,
> + const struct pmu_probe_info *info,
> + unsigned int pmuid)
> {
> - int cpu = get_cpu();
> - unsigned int cpuid = read_cpuid_id();
> int ret = -ENODEV;
> + int cpu;
> + int aff_ctr = 0;
> + static int duplicate_pmus;
> + struct platform_device *pdev = pmu->plat_device;
> + int irq = platform_get_irq(pdev, 0);
>
> - pr_info("probing PMU on CPU %d\n", cpu);
> + if (irq >= 0 && !irq_is_percpu(irq)) {
Marc's got a patch[0] changing all instances of "irq >= 0" in this file to
"irq > 0" on the basis that irq 0 is an error.
If this version doesn't get merged, please drop the "=" for the next
version.
[0] http://marc.info/?l=linux-arm-kernel&m=147318284923863
[...]
^ permalink raw reply
* [PATCH v3 1/3] tty: amba-pl011: define flag register bits for ZTE device
From: Russell King - ARM Linux @ 2016-09-16 16:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <b549cd51-a81f-b693-e3c2-9e3c598d3d9e@arm.com>
On Fri, Sep 16, 2016 at 03:23:57PM +0100, Sudeep Holla wrote:
> Hi Shawn, Russell,
>
> I noticed this change is in linux-next and but I happen to hit an issue
> with this as I tested it with earlycon enabled today for the first time.
>
> On 08/07/16 10:00, Shawn Guo wrote:
> >For some reason we do not really understand, ZTE hardware designers
> >choose to define PL011 Flag Register bit positions differently from
> >standard ones as below.
> >
> > Bit Standard ZTE
> > -----------------------------------
> > CTS 0 1
> > DSR 1 3
> > BUSY 3 8
> > RI 8 0
> >
> >Let's define these bits into vendor data and get ZTE PL011 supported
> >properly.
> >
> >Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
>
> [...]
>
> >@@ -2303,13 +2325,16 @@ static struct console amba_console = {
> >
> > static void pl011_putc(struct uart_port *port, int c)
> > {
> >+ struct uart_amba_port *uap =
> >+ container_of(port, struct uart_amba_port, port);
> >+
> > while (readl(port->membase + UART01x_FR) & UART01x_FR_TXFF)
> > cpu_relax();
> > if (port->iotype == UPIO_MEM32)
> > writel(c, port->membase + UART01x_DR);
> > else
> > writeb(c, port->membase + UART01x_DR);
> >- while (readl(port->membase + UART01x_FR) & UART01x_FR_BUSY)
> >+ while (readl(port->membase + UART01x_FR) & uap->vendor->fr_busy)
> > cpu_relax();
> > }
>
> The above hunk won't work for early console devices. The earlycon_device
> just has uart_port and is not uart_amba_port. I don't know how to fix
> this properly but I thought we could reuse private_data in uart_port for
> early_con devices. Something like below(incomplete for other vendors,
> works only for ARM)
Unfortunate - I'm getting rather annoyed with the way vendors create
these derivatives of ARM hardware which then cause problems for the
kernel, springing up all these vendor specific hacks all over the
place.
Maybe what we should've done with ZTE is insisted that they implement
a complete new driver, rather than trying to shoe-horn it into PL011
even though it is in theory PL011.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* [PATCH v4 2/2] soc: qcom: add l2 cache perf events driver
From: Mark Rutland @ 2016-09-16 16:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <27f91809-f968-e278-7887-d0f9af275502@codeaurora.org>
On Fri, Sep 16, 2016 at 11:33:39AM -0400, Neil Leeder wrote:
> Hi Mark,
> Thank you for the thorough review. I will post an updated patchset
> which addresses all of your comments. There is just one outstanding
> comment which I have a question about:
[...]
> On 9/1/2016 12:30 PM, Mark Rutland wrote:
> > On Tue, Aug 30, 2016 at 01:01:33PM -0400, Neil Leeder wrote:
> >> + /* Don't allow groups with mixed PMUs, except for s/w events */
> >> + if (event->group_leader->pmu != event->pmu &&
> >> + !is_software_event(event->group_leader)) {
> >> + dev_warn(&l2cache_pmu->pdev->dev,
> >> + "Can't create mixed PMU group\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + list_for_each_entry(sibling, &event->group_leader->sibling_list,
> >> + group_entry)
> >> + if (sibling->pmu != event->pmu &&
> >> + !is_software_event(sibling)) {
> >> + dev_warn(&l2cache_pmu->pdev->dev,
> >> + "Can't create mixed PMU group\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + hwc->idx = -1;
> >> + hwc->config_base = event->attr.config;
> >> +
> >> + /*
> >> + * Ensure all events are on the same cpu so all events are in the
> >> + * same cpu context, to avoid races on pmu_enable etc.
> >> + */
> >> + slice = get_hml2_pmu(event->cpu);
> >> + event->cpu = slice->on_cpu;
> >
> > This could put an event on a different CPU to its group siblings, which
> > is broken.
>
> This is the same logic as in arm-ccn.c:arm_ccn_pmu_event_init(), where there
> is a single CPU designated as the CPU to be used for all events.
>
> All events for this slice are forced to slice->on_cpu which is the CPU
> set in the cpumask for this slice.
The CCN is a little different. For the CCN, a single CPU is designated
to handle *all* events.
For this driver, a CPU is designated per-slice, judging by the existence
of hml2_pmu::on_cpu (unless that's superfluous). We've only verified
that the events are all for this PMU, not the same slice, and thus each
event->cpu may differ.
> I'm not sure how this can put an event on a different CPU to its group
> siblings?
In practice today, we'll try to schedule the event on it's group
leader's CPU, but accounting and subsequent manipulation could go wrong.
Thanks,
Mark.
^ permalink raw reply
* [PATCH v3 6/6] ARM: dts: bcm283x: add pinctrl group to &sdhci, drop pins from &gpio
From: Stefan Wahren @ 2016-09-16 16:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1473841415-937-7-git-send-email-kraxel@redhat.com>
Hi Gerd,
> Gerd Hoffmann <kraxel@redhat.com> hat am 14. September 2016 um 10:23
> geschrieben:
>
>
> As the alt3 group has no pins left after this
> update drop the whole alt3 group from &gpio.
sorry it won't be that simple. The references needs to removed too :-(
> grep alt3 *.dts*
bcm2835-rpi-a.dts: pinctrl-0 = <&gpioout &alt0 &i2s_alt2 &alt3>;
bcm2835-rpi-a-plus.dts: pinctrl-0 = <&gpioout &alt0 &i2s_alt0 &alt3>;
bcm2835-rpi-b.dts: pinctrl-0 = <&gpioout &alt0 &alt3>;
bcm2835-rpi-b-plus.dts: pinctrl-0 = <&gpioout &alt0 &i2s_alt0 &alt3>;
bcm2835-rpi-b-rev2.dts: pinctrl-0 = <&gpioout &alt0 &i2s_alt2 &alt3>;
bcm2835-rpi.dtsi: alt3: alt3 {
bcm2835-rpi-zero.dts: pinctrl-0 = <&gpioout &alt0 &i2s_alt0 &alt3>;
bcm2836-rpi-2-b.dts: pinctrl-0 = <&gpioout &alt0 &i2s_alt0 &alt3>;
Regards
Stefan
^ permalink raw reply
* [PATCH 5/6] arm/arm64: vgic-new: Implement VGICv3 CPU interface access
From: Vijay Kilari @ 2016-09-16 16:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <57DC0366.8060304@arm.com>
On Fri, Sep 16, 2016 at 8:06 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 16/09/16 13:20, vijay.kilari at gmail.com wrote:
>> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>>
>> VGICv3 CPU interface registers are accessed using
>> KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
>> as 64-bit. The cpu MPIDR value is passed along with register id.
>> is used to identify the cpu for registers access.
>>
>> The version of VGIC v3 specification is define here
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
>>
>> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>> ---
>> arch/arm64/include/uapi/asm/kvm.h | 3 +
>> arch/arm64/kvm/Makefile | 1 +
>> include/linux/irqchip/arm-gic-v3.h | 30 ++++
>> virt/kvm/arm/vgic/vgic-kvm-device.c | 27 ++++
>> virt/kvm/arm/vgic/vgic-mmio-v3.c | 18 +++
>> virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 296 ++++++++++++++++++++++++++++++++++++
>> virt/kvm/arm/vgic/vgic.h | 10 ++
>> 7 files changed, 385 insertions(+)
>>
>> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
>> index 56dc08d..91c7137 100644
>> --- a/arch/arm64/include/uapi/asm/kvm.h
>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>> @@ -206,9 +206,12 @@ struct kvm_arch_memory_slot {
>> (0xffffffffULL << KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT)
>> #define KVM_DEV_ARM_VGIC_OFFSET_SHIFT 0
>> #define KVM_DEV_ARM_VGIC_OFFSET_MASK (0xffffffffULL << KVM_DEV_ARM_VGIC_OFFSET_SHIFT)
>> +#define KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK (0xffff)
>> #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS 3
>> #define KVM_DEV_ARM_VGIC_GRP_CTRL 4
>> #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
>> +#define KVM_DEV_ARM_VGIC_CPU_SYSREGS 6
>> +
>> #define KVM_DEV_ARM_VGIC_CTRL_INIT 0
>>
>> /* Device Control API on vcpu fd */
>> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
>> index d50a82a..1a14e29 100644
>> --- a/arch/arm64/kvm/Makefile
>> +++ b/arch/arm64/kvm/Makefile
>> @@ -32,5 +32,6 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio-v3.o
>> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-kvm-device.o
>> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-its.o
>> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/irqchip.o
>> +kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-sys-reg-v3.o
>> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arch_timer.o
>> kvm-$(CONFIG_KVM_ARM_PMU) += $(KVM)/arm/pmu.o
>> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
>> index 88d83d3..d4e9c7d 100644
>> --- a/include/linux/irqchip/arm-gic-v3.h
>> +++ b/include/linux/irqchip/arm-gic-v3.h
>> @@ -355,6 +355,27 @@
>> #define ICC_CTLR_EL1_EOImode_SHIFT (1)
>> #define ICC_CTLR_EL1_EOImode_drop_dir (0U << ICC_CTLR_EL1_EOImode_SHIFT)
>> #define ICC_CTLR_EL1_EOImode_drop (1U << ICC_CTLR_EL1_EOImode_SHIFT)
>> +#define ICC_CTLR_EL1_EOImode_MASK (1 << ICC_CTLR_EL1_EOImode_SHIFT)
>> +#define ICC_CTLR_EL1_CBPR_SHIFT (0)
>> +#define ICC_CTLR_EL1_CBPR_MASK (1 << ICC_CTLR_EL1_CBPR_SHIFT)
>> +#define ICC_CTLR_EL1_PRI_BITS_SHIFT (8)
>> +#define ICC_CTLR_EL1_PRI_BITS_MASK (0x7 << ICC_CTLR_EL1_PRI_BITS_SHIFT)
>> +#define ICC_CTLR_EL1_ID_BITS_SHIFT (11)
>> +#define ICC_CTLR_EL1_ID_BITS_MASK (0x7 << ICC_CTLR_EL1_ID_BITS_SHIFT)
>> +#define ICC_CTLR_EL1_SEIS_SHIFT (14)
>> +#define ICC_CTLR_EL1_SEIS_MASK (0x1 << ICC_CTLR_EL1_SEIS_SHIFT)
>> +#define ICC_CTLR_EL1_A3V_SHIFT (15)
>> +#define ICC_CTLR_EL1_A3V_MASK (0x1 << ICC_CTLR_EL1_A3V_SHIFT)
>> +#define ICC_PMR_EL1_SHIFT (0)
>> +#define ICC_PMR_EL1_MASK (0xff << ICC_PMR_EL1_SHIFT)
>> +#define ICC_BPR0_EL1_SHIFT (0)
>> +#define ICC_BPR0_EL1_MASK (0x7 << ICC_BPR0_EL1_SHIFT)
>> +#define ICC_BPR1_EL1_SHIFT (0)
>> +#define ICC_BPR1_EL1_MASK (0x7 << ICC_BPR1_EL1_SHIFT)
>> +#define ICC_IGRPEN0_EL1_SHIFT (0)
>> +#define ICC_IGRPEN0_EL1_MASK (1 << ICC_IGRPEN0_EL1_SHIFT)
>> +#define ICC_IGRPEN1_EL1_SHIFT (0)
>> +#define ICC_IGRPEN1_EL1_MASK (1 << ICC_IGRPEN1_EL1_SHIFT)
>> #define ICC_SRE_EL1_SRE (1U << 0)
>>
>> /*
>> @@ -398,6 +419,15 @@
>> #define ICH_VMCR_ENG1_SHIFT 1
>> #define ICH_VMCR_ENG1_MASK (1 << ICH_VMCR_ENG1_SHIFT)
>>
>> +#define ICH_VTR_PRI_BITS_SHIFT 29
>> +#define ICH_VTR_PRI_BITS_MASK (7 << ICH_VTR_PRI_BITS_SHIFT)
>> +#define ICH_VTR_ID_BITS_SHIFT 23
>> +#define ICH_VTR_ID_BITS_MASK (7 << ICH_VTR_ID_BITS_SHIFT)
>> +#define ICH_VTR_SEIS_SHIFT 22
>> +#define ICH_VTR_SEIS_MASK (1 << ICH_VTR_SEIS_SHIFT)
>> +#define ICH_VTR_A3V_SHIFT 21
>> +#define ICH_VTR_A3V_MASK (1 << ICH_VTR_A3V_SHIFT)
>> +
>
> Same thing here. Please group all the additions to this file into a
> single, independent patch.
>
>> #define ICC_IAR1_EL1_SPURIOUS 0x3ff
>>
>> #define ICC_SRE_EL2_SRE (1 << 0)
>> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
>> index a4656fc..a48101f 100644
>> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
>> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
>> @@ -506,6 +506,14 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev,
>> if (!is_write)
>> *reg = tmp32;
>> break;
>> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
>> + u64 regid;
>> +
>> + regid = (attr->attr & KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK);
>> + ret = vgic_v3_cpu_sysregs_uaccess(vcpu, is_write,
>> + regid, reg);
>> + break;
>> + }
>> default:
>> ret = -EINVAL;
>> break;
>> @@ -539,6 +547,15 @@ static int vgic_v3_set_attr(struct kvm_device *dev,
>> reg = tmp32;
>> return vgic_v3_attr_regs_access(dev, attr, ®, true);
>> }
>> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
>> + u64 __user *uaddr = (u64 __user *)(long)attr->addr;
>> + u64 reg;
>> +
>> + if (get_user(reg, uaddr))
>> + return -EFAULT;
>> +
>> + return vgic_v3_attr_regs_access(dev, attr, ®, true);
>> + }
>> }
>> return -ENXIO;
>> }
>> @@ -565,6 +582,15 @@ static int vgic_v3_get_attr(struct kvm_device *dev,
>> tmp32 = reg;
>> return put_user(tmp32, uaddr);
>> }
>> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
>> + u64 __user *uaddr = (u64 __user *)(long)attr->addr;
>> + u64 reg;
>> +
>> + ret = vgic_v3_attr_regs_access(dev, attr, ®, false);
>> + if (ret)
>> + return ret;
>> + return put_user(reg, uaddr);
>> + }
>> }
>>
>> return -ENXIO;
>> @@ -583,6 +609,7 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
>> break;
>> case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
>> case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
>> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS:
>> return vgic_v3_has_attr_regs(dev, attr);
>> case KVM_DEV_ARM_VGIC_GRP_NR_IRQS:
>> return 0;
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> index 83dece8..af748d7 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> @@ -23,6 +23,7 @@
>>
>> #include "vgic.h"
>> #include "vgic-mmio.h"
>> +#include "sys_regs.h"
>>
>> /* extract @num bytes at @offset bytes offset in data */
>> unsigned long extract_bytes(u64 data, unsigned int offset,
>> @@ -639,6 +640,23 @@ int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
>> nr_regions = ARRAY_SIZE(vgic_v3_rdbase_registers);
>> break;
>> }
>> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
>> + u64 reg, id;
>> + unsigned long mpidr;
>> + struct kvm_vcpu *vcpu;
>> +
>> + mpidr = (attr->attr & KVM_DEV_ARM_VGIC_V3_MPIDR_MASK) >>
>> + KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT;
>> +
>> + vcpu = kvm_mpidr_to_vcpu(dev->kvm, mpidr);
>
> No. Your "mpidr" variable is not an MPIDR. It represents the same
> thing, but not with the right format.
Ok. I will format in the form of MPIDR reg.
>
>> + if (!vcpu)
>> + return -EINVAL;
>> + if (vcpu->vcpu_id >= atomic_read(&dev->kvm->online_vcpus))
>
> How can that happen?
OK. This check can be dropped as kvm_for_each_vcpu() already checks
for only online_vcpus.
>
>> + return -EINVAL;
>> +
>> + id = (attr->attr & KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK);
>> + return vgic_v3_has_cpu_sysregs_attr(vcpu, 0, id, ®);
>> + }
>> default:
>> return -ENXIO;
>> }
>> diff --git a/virt/kvm/arm/vgic/vgic-sys-reg-v3.c b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
>> new file mode 100644
>> index 0000000..8e4f403
>> --- /dev/null
>> +++ b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
>> @@ -0,0 +1,296 @@
>> +#include <linux/irqchip/arm-gic-v3.h>
>> +#include <linux/kvm.h>
>> +#include <linux/kvm_host.h>
>> +#include <kvm/iodev.h>
>> +#include <kvm/arm_vgic.h>
>> +#include <asm/kvm_emulate.h>
>> +#include <asm/kvm_arm.h>
>> +#include <asm/kvm_mmu.h>
>> +
>> +#include "vgic.h"
>> +#include "vgic-mmio.h"
>> +#include "sys_regs.h"
>> +
>> +static bool access_gic_ctlr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>> + const struct sys_reg_desc *r)
>> +{
>> + struct vgic_vmcr vmcr;
>> + u64 val;
>> + u32 ich_vtr;
>> +
>> + vgic_get_vmcr(vcpu, &vmcr);
>> + if (p->is_write) {
>> + val = p->regval;
>> + vmcr.ctlr &= ~(ICH_VMCR_CBPR_MASK | ICH_VMCR_EOIM_MASK);
>> + vmcr.ctlr |= ((val & ICC_CTLR_EL1_CBPR_MASK) >>
>> + ICC_CTLR_EL1_CBPR_SHIFT) << ICH_VMCR_CBPR_SHIFT;
>> + vmcr.ctlr |= ((val & ICC_CTLR_EL1_EOImode_MASK) >>
>> + ICC_CTLR_EL1_EOImode_SHIFT) << ICH_VMCR_EOIM_SHIFT;
>> + vgic_set_vmcr(vcpu, &vmcr);
>
> You've ignored my comments again: "What if userspace writes something
> that is incompatible with the current configuration? Wrong number of ID
> bits, or number of priorities?"
IMO, In case of incompatibility,
If ID bits and PRI bits are less than HW supported, it is ok.
If ID bits and PRI bits are greater than HW supported, then warn would be good
enough. Please suggest the behaviour that you think it should be.
>
>> + } else {
>> + ich_vtr = kvm_call_hyp(__vgic_v3_get_ich_vtr_el2);
>> +
>> + val = 0;
>> + val |= ((ich_vtr & ICH_VTR_PRI_BITS_MASK) >>
>> + ICH_VTR_PRI_BITS_SHIFT) << ICC_CTLR_EL1_PRI_BITS_SHIFT;
>> + val |= ((ich_vtr & ICH_VTR_ID_BITS_MASK) >>
>> + ICH_VTR_ID_BITS_SHIFT) << ICC_CTLR_EL1_ID_BITS_SHIFT;
>> + val |= ((ich_vtr & ICH_VTR_SEIS_MASK) >> ICH_VTR_SEIS_SHIFT)
>> + << ICC_CTLR_EL1_SEIS_SHIFT;
>> + val |= ((ich_vtr & ICH_VTR_A3V_MASK) >> ICH_VTR_A3V_SHIFT)
>> + << ICC_CTLR_EL1_A3V_SHIFT;
>> + val |= ((vmcr.ctlr & ICH_VMCR_CBPR_MASK) >>
>> + ICH_VMCR_CBPR_SHIFT) << ICC_CTLR_EL1_CBPR_SHIFT;
>> + val |= ((vmcr.ctlr & ICH_VMCR_EOIM_MASK) >>
>> + ICH_VMCR_EOIM_SHIFT) << ICC_CTLR_EL1_EOImode_SHIFT;
>> +
>> + p->regval = val;
>> + }
>> +
>> + return true;
>> +}
>> +
>> +static bool access_gic_pmr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>> + const struct sys_reg_desc *r)
>> +{
>> + struct vgic_vmcr vmcr;
>> +
>> + vgic_get_vmcr(vcpu, &vmcr);
>> + if (p->is_write) {
>> + vmcr.pmr = (p->regval & ICC_PMR_EL1_MASK) >> ICC_PMR_EL1_SHIFT;
>> + vgic_set_vmcr(vcpu, &vmcr);
>> + } else {
>> + p->regval = (vmcr.pmr << ICC_PMR_EL1_SHIFT) & ICC_PMR_EL1_MASK;
>> + }
>> +
>> + return true;
>> +}
>> +
>> +static bool access_gic_bpr0(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>> + const struct sys_reg_desc *r)
>> +{
>> + struct vgic_vmcr vmcr;
>> +
>> + vgic_get_vmcr(vcpu, &vmcr);
>> + if (p->is_write) {
>> + vmcr.bpr = (p->regval & ICC_BPR0_EL1_MASK) >>
>> + ICC_BPR0_EL1_SHIFT;
>> + vgic_set_vmcr(vcpu, &vmcr);
>> + } else {
>> + p->regval = (vmcr.bpr << ICC_BPR0_EL1_SHIFT) &
>> + ICC_BPR0_EL1_MASK;
>> + }
>> +
>> + return true;
>> +}
>> +
>> +static bool access_gic_bpr1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>> + const struct sys_reg_desc *r)
>> +{
>> + struct vgic_vmcr vmcr;
>> +
>> + vgic_get_vmcr(vcpu, &vmcr);
>> + if (p->is_write) {
>> + vmcr.abpr = (p->regval & ICC_BPR1_EL1_MASK) >>
>> + ICC_BPR1_EL1_SHIFT;
>> + vgic_set_vmcr(vcpu, &vmcr);
>> + } else {
>> + p->regval = (vmcr.abpr << ICC_BPR1_EL1_SHIFT) &
>> + ICC_BPR1_EL1_MASK;
>> + }
>> +
>> + return true;
>> +}
>> +
>> +static bool access_gic_grpen0(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>> + const struct sys_reg_desc *r)
>> +{
>> + struct vgic_vmcr vmcr;
>> +
>> + vgic_get_vmcr(vcpu, &vmcr);
>> + if (p->is_write) {
>> + vmcr.grpen0 = (p->regval & ICC_IGRPEN0_EL1_MASK) >>
>> + ICC_IGRPEN0_EL1_SHIFT;
>> + vgic_set_vmcr(vcpu, &vmcr);
>> + } else {
>> + p->regval = (vmcr.grpen0 << ICC_IGRPEN0_EL1_SHIFT) &
>> + ICC_IGRPEN0_EL1_MASK;
>> + }
>> +
>> + return true;
>> +}
>> +
>> +static bool access_gic_grpen1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>> + const struct sys_reg_desc *r)
>> +{
>> + struct vgic_vmcr vmcr;
>> +
>> + vgic_get_vmcr(vcpu, &vmcr);
>> + if (p->is_write) {
>> + vmcr.grpen1 = (p->regval & ICC_IGRPEN1_EL1_MASK) >>
>> + ICC_IGRPEN1_EL1_SHIFT;
>> + vgic_set_vmcr(vcpu, &vmcr);
>> + } else {
>> + p->regval = (vmcr.grpen1 << ICC_IGRPEN1_EL1_SHIFT) &
>> + ICC_IGRPEN1_EL1_MASK;
>
> From the previous review comments: "Shouldn't this account for the
> ICC_CTLR_EL1.CBPR setting?"
Ok. I think this comment is for ICC_BPR1_EL1 access.
I will make a check on ICC_CTLR.EL1.CBPR for accessing ICC_BPR1_EL1.
>
>> + }
>> +
>> + return true;
>> +}
>> +
>> +static void vgic_v3_access_apr_reg(struct kvm_vcpu *vcpu,
>> + struct sys_reg_params *p, u8 apr, u8 idx)
>> +{
>> + struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
>> + uint32_t *ap_reg;
>> +
>> + if (apr)
>> + ap_reg = &vgicv3->vgic_ap1r[idx];
>> + else
>> + ap_reg = &vgicv3->vgic_ap0r[idx];
>> +
>> + if (p->is_write)
>> + *ap_reg = p->regval;
>> + else
>> + p->regval = *ap_reg;
>> +}
>> +
>> +static void access_gic_aprn(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>> + const struct sys_reg_desc *r, u8 apr)
>> +{
>> + u8 num_pri_bits, idx;
>> + u32 ich_vtr = kvm_call_hyp(__vgic_v3_get_ich_vtr_el2);
>
> Maybe you should cache this once and for all? It is fairly unlikely to
> change over time...
Is it ok to cache in vgic_global struct?
^ permalink raw reply
* [PATCH 5/6] arm/arm64: vgic-new: Implement VGICv3 CPU interface access
From: Marc Zyngier @ 2016-09-16 17:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CALicx6u-uYLY3DtDGMh3n6NbNH3kD9fn9aYHHJC=LVUQfFjTsQ@mail.gmail.com>
On 16/09/16 17:57, Vijay Kilari wrote:
> On Fri, Sep 16, 2016 at 8:06 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> On 16/09/16 13:20, vijay.kilari at gmail.com wrote:
>>> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>>>
>>> VGICv3 CPU interface registers are accessed using
>>> KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
>>> as 64-bit. The cpu MPIDR value is passed along with register id.
>>> is used to identify the cpu for registers access.
>>>
>>> The version of VGIC v3 specification is define here
>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
>>>
>>> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
>>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>>> ---
>>> arch/arm64/include/uapi/asm/kvm.h | 3 +
>>> arch/arm64/kvm/Makefile | 1 +
>>> include/linux/irqchip/arm-gic-v3.h | 30 ++++
>>> virt/kvm/arm/vgic/vgic-kvm-device.c | 27 ++++
>>> virt/kvm/arm/vgic/vgic-mmio-v3.c | 18 +++
>>> virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 296 ++++++++++++++++++++++++++++++++++++
>>> virt/kvm/arm/vgic/vgic.h | 10 ++
>>> 7 files changed, 385 insertions(+)
[...]
>>> diff --git a/virt/kvm/arm/vgic/vgic-sys-reg-v3.c b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
>>> new file mode 100644
>>> index 0000000..8e4f403
>>> --- /dev/null
>>> +++ b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
>>> @@ -0,0 +1,296 @@
>>> +#include <linux/irqchip/arm-gic-v3.h>
>>> +#include <linux/kvm.h>
>>> +#include <linux/kvm_host.h>
>>> +#include <kvm/iodev.h>
>>> +#include <kvm/arm_vgic.h>
>>> +#include <asm/kvm_emulate.h>
>>> +#include <asm/kvm_arm.h>
>>> +#include <asm/kvm_mmu.h>
>>> +
>>> +#include "vgic.h"
>>> +#include "vgic-mmio.h"
>>> +#include "sys_regs.h"
>>> +
>>> +static bool access_gic_ctlr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>>> + const struct sys_reg_desc *r)
>>> +{
>>> + struct vgic_vmcr vmcr;
>>> + u64 val;
>>> + u32 ich_vtr;
>>> +
>>> + vgic_get_vmcr(vcpu, &vmcr);
>>> + if (p->is_write) {
>>> + val = p->regval;
>>> + vmcr.ctlr &= ~(ICH_VMCR_CBPR_MASK | ICH_VMCR_EOIM_MASK);
>>> + vmcr.ctlr |= ((val & ICC_CTLR_EL1_CBPR_MASK) >>
>>> + ICC_CTLR_EL1_CBPR_SHIFT) << ICH_VMCR_CBPR_SHIFT;
>>> + vmcr.ctlr |= ((val & ICC_CTLR_EL1_EOImode_MASK) >>
>>> + ICC_CTLR_EL1_EOImode_SHIFT) << ICH_VMCR_EOIM_SHIFT;
>>> + vgic_set_vmcr(vcpu, &vmcr);
>>
>> You've ignored my comments again: "What if userspace writes something
>> that is incompatible with the current configuration? Wrong number of ID
>> bits, or number of priorities?"
>
> IMO, In case of incompatibility,
> If ID bits and PRI bits are less than HW supported, it is ok.
Yes. But you also need to track of what the guest has programmed in
order to be able to migrate it back to its original configuration.
> If ID bits and PRI bits are greater than HW supported, then warn would be good
> enough. Please suggest the behaviour that you think it should be.
No, it is an error, plain and simple. You cannot run in this condition.
>
>>
>>> + } else {
>>> + ich_vtr = kvm_call_hyp(__vgic_v3_get_ich_vtr_el2);
>>> +
>>> + val = 0;
>>> + val |= ((ich_vtr & ICH_VTR_PRI_BITS_MASK) >>
>>> + ICH_VTR_PRI_BITS_SHIFT) << ICC_CTLR_EL1_PRI_BITS_SHIFT;
>>> + val |= ((ich_vtr & ICH_VTR_ID_BITS_MASK) >>
>>> + ICH_VTR_ID_BITS_SHIFT) << ICC_CTLR_EL1_ID_BITS_SHIFT;
>>> + val |= ((ich_vtr & ICH_VTR_SEIS_MASK) >> ICH_VTR_SEIS_SHIFT)
>>> + << ICC_CTLR_EL1_SEIS_SHIFT;
>>> + val |= ((ich_vtr & ICH_VTR_A3V_MASK) >> ICH_VTR_A3V_SHIFT)
>>> + << ICC_CTLR_EL1_A3V_SHIFT;
>>> + val |= ((vmcr.ctlr & ICH_VMCR_CBPR_MASK) >>
>>> + ICH_VMCR_CBPR_SHIFT) << ICC_CTLR_EL1_CBPR_SHIFT;
>>> + val |= ((vmcr.ctlr & ICH_VMCR_EOIM_MASK) >>
>>> + ICH_VMCR_EOIM_SHIFT) << ICC_CTLR_EL1_EOImode_SHIFT;
>>> +
>>> + p->regval = val;
>>> + }
>>> +
>>> + return true;
>>> +}
>>> +
>>> +static bool access_gic_pmr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>>> + const struct sys_reg_desc *r)
>>> +{
>>> + struct vgic_vmcr vmcr;
>>> +
>>> + vgic_get_vmcr(vcpu, &vmcr);
>>> + if (p->is_write) {
>>> + vmcr.pmr = (p->regval & ICC_PMR_EL1_MASK) >> ICC_PMR_EL1_SHIFT;
>>> + vgic_set_vmcr(vcpu, &vmcr);
>>> + } else {
>>> + p->regval = (vmcr.pmr << ICC_PMR_EL1_SHIFT) & ICC_PMR_EL1_MASK;
>>> + }
>>> +
>>> + return true;
>>> +}
>>> +
>>> +static bool access_gic_bpr0(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>>> + const struct sys_reg_desc *r)
>>> +{
>>> + struct vgic_vmcr vmcr;
>>> +
>>> + vgic_get_vmcr(vcpu, &vmcr);
>>> + if (p->is_write) {
>>> + vmcr.bpr = (p->regval & ICC_BPR0_EL1_MASK) >>
>>> + ICC_BPR0_EL1_SHIFT;
>>> + vgic_set_vmcr(vcpu, &vmcr);
>>> + } else {
>>> + p->regval = (vmcr.bpr << ICC_BPR0_EL1_SHIFT) &
>>> + ICC_BPR0_EL1_MASK;
>>> + }
>>> +
>>> + return true;
>>> +}
>>> +
>>> +static bool access_gic_bpr1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>>> + const struct sys_reg_desc *r)
>>> +{
>>> + struct vgic_vmcr vmcr;
>>> +
>>> + vgic_get_vmcr(vcpu, &vmcr);
>>> + if (p->is_write) {
>>> + vmcr.abpr = (p->regval & ICC_BPR1_EL1_MASK) >>
>>> + ICC_BPR1_EL1_SHIFT;
>>> + vgic_set_vmcr(vcpu, &vmcr);
>>> + } else {
>>> + p->regval = (vmcr.abpr << ICC_BPR1_EL1_SHIFT) &
>>> + ICC_BPR1_EL1_MASK;
>>> + }
>>> +
>>> + return true;
>>> +}
>>> +
>>> +static bool access_gic_grpen0(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>>> + const struct sys_reg_desc *r)
>>> +{
>>> + struct vgic_vmcr vmcr;
>>> +
>>> + vgic_get_vmcr(vcpu, &vmcr);
>>> + if (p->is_write) {
>>> + vmcr.grpen0 = (p->regval & ICC_IGRPEN0_EL1_MASK) >>
>>> + ICC_IGRPEN0_EL1_SHIFT;
>>> + vgic_set_vmcr(vcpu, &vmcr);
>>> + } else {
>>> + p->regval = (vmcr.grpen0 << ICC_IGRPEN0_EL1_SHIFT) &
>>> + ICC_IGRPEN0_EL1_MASK;
>>> + }
>>> +
>>> + return true;
>>> +}
>>> +
>>> +static bool access_gic_grpen1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>>> + const struct sys_reg_desc *r)
>>> +{
>>> + struct vgic_vmcr vmcr;
>>> +
>>> + vgic_get_vmcr(vcpu, &vmcr);
>>> + if (p->is_write) {
>>> + vmcr.grpen1 = (p->regval & ICC_IGRPEN1_EL1_MASK) >>
>>> + ICC_IGRPEN1_EL1_SHIFT;
>>> + vgic_set_vmcr(vcpu, &vmcr);
>>> + } else {
>>> + p->regval = (vmcr.grpen1 << ICC_IGRPEN1_EL1_SHIFT) &
>>> + ICC_IGRPEN1_EL1_MASK;
>>
>> From the previous review comments: "Shouldn't this account for the
>> ICC_CTLR_EL1.CBPR setting?"
>
> Ok. I think this comment is for ICC_BPR1_EL1 access.
Yes, sorry about the misplaced comment.
> I will make a check on ICC_CTLR.EL1.CBPR for accessing ICC_BPR1_EL1.
The reverse is also true: you also need to account the value of
ICC_BPR1_EL1 when accessing ICC_CTLR_EL1.
>>
>>> + }
>>> +
>>> + return true;
>>> +}
>>> +
>>> +static void vgic_v3_access_apr_reg(struct kvm_vcpu *vcpu,
>>> + struct sys_reg_params *p, u8 apr, u8 idx)
>>> +{
>>> + struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
>>> + uint32_t *ap_reg;
>>> +
>>> + if (apr)
>>> + ap_reg = &vgicv3->vgic_ap1r[idx];
>>> + else
>>> + ap_reg = &vgicv3->vgic_ap0r[idx];
>>> +
>>> + if (p->is_write)
>>> + *ap_reg = p->regval;
>>> + else
>>> + p->regval = *ap_reg;
>>> +}
>>> +
>>> +static void access_gic_aprn(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>>> + const struct sys_reg_desc *r, u8 apr)
>>> +{
>>> + u8 num_pri_bits, idx;
>>> + u32 ich_vtr = kvm_call_hyp(__vgic_v3_get_ich_vtr_el2);
>>
>> Maybe you should cache this once and for all? It is fairly unlikely to
>> change over time...
>
> Is it ok to cache in vgic_global struct?
I can't see why not.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH] coresight: tmc: Cleanup operation mode handling
From: Mathieu Poirier @ 2016-09-16 17:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1473861230-5666-1-git-send-email-suzuki.poulose@arm.com>
On 14 September 2016 at 07:53, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> The mode of operation of the TMC tracked in drvdata->mode is defined
> as a local_t type. This is always checked and modified under the
> drvdata->spinlock and hence we don't need local_t for it and the
> unnecessary synchronisation instructions that comes with it. This
> change makes the code a bit more cleaner.
>
> Also fixes the order in which we update the drvdata->mode to
> CS_MODE_DISABLED. i.e, in tmc_disable_etX_sink we change the
> mode to CS_MODE_DISABLED before invoking tmc_disable_etX_hw()
> which in turn depends on the mode to decide whether to dump the
> trace to a buffer.
Thank you for the patch - just a few comments below.
Regards,
Mathieu
>
> Applies on mathieu's coresight/next tree [1]
>
> https://git.linaro.org/kernel/coresight.git next
>
> Reported-by: Venkatesh Vivekanandan <venkatesh.vivekanandan@broadcom.com>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
> drivers/hwtracing/coresight/coresight-tmc-etf.c | 32 +++++++++++--------------
> drivers/hwtracing/coresight/coresight-tmc-etr.c | 30 ++++++++++-------------
> drivers/hwtracing/coresight/coresight-tmc.h | 2 +-
> 3 files changed, 28 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> index d6941ea..c51ce45 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> @@ -70,7 +70,7 @@ static void tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
> * When operating in sysFS mode the content of the buffer needs to be
> * read before the TMC is disabled.
> */
> - if (local_read(&drvdata->mode) == CS_MODE_SYSFS)
> + if (drvdata->mode == CS_MODE_SYSFS)
> tmc_etb_dump_hw(drvdata);
> tmc_disable_hw(drvdata);
>
> @@ -108,7 +108,6 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev, u32 mode)
> int ret = 0;
> bool used = false;
> char *buf = NULL;
> - long val;
> unsigned long flags;
> struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>
> @@ -138,13 +137,12 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev, u32 mode)
> goto out;
> }
>
> - val = local_xchg(&drvdata->mode, mode);
> /*
> * In sysFS mode we can have multiple writers per sink. Since this
> * sink is already enabled no memory is needed and the HW need not be
> * touched.
> */
> - if (val == CS_MODE_SYSFS)
> + if (drvdata->mode == CS_MODE_SYSFS)
> goto out;
>
> /*
> @@ -163,6 +161,7 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev, u32 mode)
> drvdata->buf = buf;
> }
>
> + drvdata->mode = CS_MODE_SYSFS;
> tmc_etb_enable_hw(drvdata);
> out:
> spin_unlock_irqrestore(&drvdata->spinlock, flags);
> @@ -180,7 +179,6 @@ out:
> static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, u32 mode)
> {
> int ret = 0;
> - long val;
> unsigned long flags;
> struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>
> @@ -194,17 +192,17 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, u32 mode)
> goto out;
> }
>
> - val = local_xchg(&drvdata->mode, mode);
> /*
> * In Perf mode there can be only one writer per sink. There
> * is also no need to continue if the ETB/ETR is already operated
> * from sysFS.
> */
> - if (val != CS_MODE_DISABLED) {
> + if (drvdata->mode != CS_MODE_DISABLED) {
> ret = -EINVAL;
> goto out;
> }
>
> + drvdata->mode = mode;
Given the way tmc_enable_etf_sink_perf() is called in
tmc_enable_etf_sink(), I think it is time to get rid of the 'mode'
parameter - it doesn't do anything nowadays. Same thing for
tmc_enable_etf_sink_sysfs() and ETR.
> tmc_etb_enable_hw(drvdata);
> out:
> spin_unlock_irqrestore(&drvdata->spinlock, flags);
> @@ -227,7 +225,6 @@ static int tmc_enable_etf_sink(struct coresight_device *csdev, u32 mode)
>
> static void tmc_disable_etf_sink(struct coresight_device *csdev)
> {
> - long val;
> unsigned long flags;
> struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>
> @@ -237,10 +234,11 @@ static void tmc_disable_etf_sink(struct coresight_device *csdev)
> return;
> }
>
> - val = local_xchg(&drvdata->mode, CS_MODE_DISABLED);
> /* Disable the TMC only if it needs to */
> - if (val != CS_MODE_DISABLED)
> + if (drvdata->mode != CS_MODE_DISABLED) {
> tmc_etb_disable_hw(drvdata);
> + drvdata->mode = CS_MODE_DISABLED;
> + }
>
> spin_unlock_irqrestore(&drvdata->spinlock, flags);
>
> @@ -260,7 +258,7 @@ static int tmc_enable_etf_link(struct coresight_device *csdev,
> }
>
> tmc_etf_enable_hw(drvdata);
> - local_set(&drvdata->mode, CS_MODE_SYSFS);
> + drvdata->mode = CS_MODE_SYSFS;
> spin_unlock_irqrestore(&drvdata->spinlock, flags);
>
> dev_info(drvdata->dev, "TMC-ETF enabled\n");
> @@ -279,8 +277,8 @@ static void tmc_disable_etf_link(struct coresight_device *csdev,
> return;
> }
>
> + drvdata->mode = CS_MODE_DISABLED;
> tmc_etf_disable_hw(drvdata);
> - local_set(&drvdata->mode, CS_MODE_DISABLED);
I think setting the mode should come after tmc_etf_disable_hw(), as it
was before.
> spin_unlock_irqrestore(&drvdata->spinlock, flags);
>
> dev_info(drvdata->dev, "TMC disabled\n");
> @@ -383,7 +381,7 @@ static void tmc_update_etf_buffer(struct coresight_device *csdev,
> return;
>
> /* This shouldn't happen */
> - if (WARN_ON_ONCE(local_read(&drvdata->mode) != CS_MODE_PERF))
> + if (WARN_ON_ONCE(drvdata->mode != CS_MODE_PERF))
> return;
>
> CS_UNLOCK(drvdata->base);
> @@ -504,7 +502,6 @@ const struct coresight_ops tmc_etf_cs_ops = {
>
> int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
> {
> - long val;
> enum tmc_mode mode;
> int ret = 0;
> unsigned long flags;
> @@ -528,9 +525,8 @@ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
> goto out;
> }
>
> - val = local_read(&drvdata->mode);
> /* Don't interfere if operated from Perf */
> - if (val == CS_MODE_PERF) {
> + if (drvdata->mode == CS_MODE_PERF) {
> ret = -EINVAL;
> goto out;
> }
> @@ -542,7 +538,7 @@ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
> }
>
> /* Disable the TMC if need be */
> - if (val == CS_MODE_SYSFS)
> + if (drvdata->mode == CS_MODE_SYSFS)
> tmc_etb_disable_hw(drvdata);
>
> drvdata->reading = true;
> @@ -573,7 +569,7 @@ int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata)
> }
>
> /* Re-enable the TMC if need be */
> - if (local_read(&drvdata->mode) == CS_MODE_SYSFS) {
> + if (drvdata->mode == CS_MODE_SYSFS) {
> /*
> * The trace run will continue with the same allocated trace
> * buffer. As such zero-out the buffer so that we don't end
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index 886ea83..cf2bf60 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> @@ -86,7 +86,7 @@ static void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
> * When operating in sysFS mode the content of the buffer needs to be
> * read before the TMC is disabled.
> */
> - if (local_read(&drvdata->mode) == CS_MODE_SYSFS)
> + if (drvdata->mode == CS_MODE_SYSFS)
> tmc_etr_dump_hw(drvdata);
> tmc_disable_hw(drvdata);
>
> @@ -97,7 +97,6 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev, u32 mode)
> {
> int ret = 0;
> bool used = false;
> - long val;
> unsigned long flags;
> void __iomem *vaddr = NULL;
> dma_addr_t paddr;
> @@ -134,13 +133,12 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev, u32 mode)
> goto out;
> }
>
> - val = local_xchg(&drvdata->mode, mode);
> /*
> * In sysFS mode we can have multiple writers per sink. Since this
> * sink is already enabled no memory is needed and the HW need not be
> * touched.
> */
> - if (val == CS_MODE_SYSFS)
> + if (drvdata->mode == CS_MODE_SYSFS)
> goto out;
>
> /*
> @@ -157,6 +155,7 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev, u32 mode)
>
> memset(drvdata->vaddr, 0, drvdata->size);
>
> + drvdata->mode = CS_MODE_SYSFS;
> tmc_etr_enable_hw(drvdata);
> out:
> spin_unlock_irqrestore(&drvdata->spinlock, flags);
> @@ -174,7 +173,6 @@ out:
> static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, u32 mode)
> {
> int ret = 0;
> - long val;
> unsigned long flags;
> struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>
> @@ -188,18 +186,18 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, u32 mode)
> goto out;
> }
>
> - val = local_xchg(&drvdata->mode, mode);
> /*
> * In Perf mode there can be only one writer per sink. There
> * is also no need to continue if the ETR is already operated
> * from sysFS.
> */
> - if (val != CS_MODE_DISABLED) {
> + if (drvdata->mode == CS_MODE_DISABLED) {
> + drvdata->mode = CS_MODE_PERF;
> + tmc_etr_enable_hw(drvdata);
> + } else {
> ret = -EINVAL;
> - goto out;
> }
>
> - tmc_etr_enable_hw(drvdata);
Is there a reason for not proceeding the same way as in
tmc_enable_etr_sink_perf()? I thought that was a better approach.
> out:
> spin_unlock_irqrestore(&drvdata->spinlock, flags);
>
> @@ -221,7 +219,6 @@ static int tmc_enable_etr_sink(struct coresight_device *csdev, u32 mode)
>
> static void tmc_disable_etr_sink(struct coresight_device *csdev)
> {
> - long val;
> unsigned long flags;
> struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>
> @@ -231,10 +228,11 @@ static void tmc_disable_etr_sink(struct coresight_device *csdev)
> return;
> }
>
> - val = local_xchg(&drvdata->mode, CS_MODE_DISABLED);
> /* Disable the TMC only if it needs to */
> - if (val != CS_MODE_DISABLED)
> + if (drvdata->mode != CS_MODE_DISABLED) {
> tmc_etr_disable_hw(drvdata);
> + drvdata->mode = CS_MODE_DISABLED;
> + }
>
> spin_unlock_irqrestore(&drvdata->spinlock, flags);
>
> @@ -253,7 +251,6 @@ const struct coresight_ops tmc_etr_cs_ops = {
> int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)
> {
> int ret = 0;
> - long val;
> unsigned long flags;
>
> /* config types are set a boot time and never change */
> @@ -266,9 +263,8 @@ int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)
> goto out;
> }
>
> - val = local_read(&drvdata->mode);
> /* Don't interfere if operated from Perf */
> - if (val == CS_MODE_PERF) {
> + if (drvdata->mode == CS_MODE_PERF) {
> ret = -EINVAL;
> goto out;
> }
> @@ -280,7 +276,7 @@ int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)
> }
>
> /* Disable the TMC if need be */
> - if (val == CS_MODE_SYSFS)
> + if (drvdata->mode == CS_MODE_SYSFS)
> tmc_etr_disable_hw(drvdata);
>
> drvdata->reading = true;
> @@ -303,7 +299,7 @@ int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata)
> spin_lock_irqsave(&drvdata->spinlock, flags);
>
> /* RE-enable the TMC if need be */
> - if (local_read(&drvdata->mode) == CS_MODE_SYSFS) {
> + if (drvdata->mode == CS_MODE_SYSFS) {
> /*
> * The trace run will continue with the same allocated trace
> * buffer. The trace buffer is cleared in tmc_etr_enable_hw(),
> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
> index 44b3ae3..51c0185 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc.h
> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
> @@ -117,7 +117,7 @@ struct tmc_drvdata {
> void __iomem *vaddr;
> u32 size;
> u32 len;
> - local_t mode;
> + u32 mode;
> enum tmc_config_type config_type;
> enum tmc_mem_intf_width memwidth;
> u32 trigger_cntr;
> --
> 2.7.4
>
^ permalink raw reply
* [PATCH v9 08/10] arm64: pmu: Detect and enable multiple PMUs in an ACPI system
From: Punit Agrawal @ 2016-09-16 17:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <6e0b08db-2e13-be49-5208-c396bf51fe68@arm.com>
Jeremy Linton <jeremy.linton@arm.com> writes:
> Hi,
>
> On 09/16/2016 08:33 AM, Punit Agrawal wrote:
>> Jeremy Linton <jeremy.linton@arm.com> writes:
>>
>>> Its possible that an ACPI system has multiple CPU types in it
>>> with differing PMU counters. Iterate the CPU's and make a determination
>>> about how many of each type exist in the system. Then take and create
>>> a PMU platform device for each type, and assign it the interrupts parsed
>>> from the MADT. Creating a platform device is necessary because the PMUs
>>> are not described as devices in the DSDT table.
>>>
>>> This code is loosely based on earlier work by Mark Salter.
>
> (trimming)
>
>>> +
>>> + list_for_each_entry_safe(pmu, safe_temp, &pmus, list) {
>>> + if (unused_madt_entries)
>>> + pmu->cpu_count = num_possible_cpus();
>>
>> So if there is any unbooted cpu ...
>>
>>> +
>>> + res = kcalloc(pmu->cpu_count,
>>> + sizeof(struct resource), GFP_KERNEL);
>>
>> ... we allocate potentially large number (num_possible_cpus()) of
>> resources for each PMU.
>>
>> This is needlessly wasteful. Under what conditions have you found
>> reg_midr to be 0?
>
> Unused MADT entries, in place for potentially unbooted/hotplug
> CPUs.
Is linux able to deal with booting secondaries that have unused MADT
entries?
> In those cases you don't know for sure which PMU the CPU belongs
> to until it comes online and the MIDR can be read. I'm open to
> suggestions on how to deal with this, outside of pushing my luck and
> further breaking the platform device encapsulation by trying to
> reallocate the resource structure while its active. Besides its only
> wasteful for ACPI+big.little, which at the moment only applies to a
> development platform.
I don't have any ideas to solve this problem, but in the interest of
helping review, please move all the changes arising from hotplug/unused
MADT entries in this patch to the next one.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v3 1/3] tty: amba-pl011: define flag register bits for ZTE device
From: Timur Tabi @ 2016-09-16 17:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160916163952.GH1041@n2100.armlinux.org.uk>
Russell King - ARM Linux wrote:
> Maybe what we should've done with ZTE is insisted that they implement
> a complete new driver, rather than trying to shoe-horn it into PL011
> even though it is in theory PL011.
When I suggested that a year ago, I was shot down:
http://www.spinics.net/lists/arm-kernel/msg455888.html
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).