linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] arm64: smccc: Add trace events to SMC calls.
@ 2023-03-04 12:58 Rakesh Babu Saladi
  2023-03-04 17:38 ` kernel test robot
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Rakesh Babu Saladi @ 2023-03-04 12:58 UTC (permalink / raw)
  To: mark.rutland, lpieralisi, sudeep.holla, linux-arm-kernel,
	linux-kernel, sgoutham, gcherian, naveenm
  Cc: Rakesh Babu Saladi

This patch adds start and end trace events to an SMC call sent from
kernel to ATF. The start trace event prints the smc_id and the end
trace event prints the smc_id and the time taken to process the SMC
call.

Signed-off-by: Rakesh Babu Saladi <rsaladi2@marvell.com>
---
 drivers/firmware/smccc/Makefile      |  3 +-
 drivers/firmware/smccc/smccc.c       | 14 ++++++++++
 drivers/firmware/smccc/smccc_trace.c |  7 +++++
 drivers/firmware/smccc/smccc_trace.h | 41 ++++++++++++++++++++++++++++
 include/linux/arm-smccc.h            |  4 ++-
 kernel/time/timekeeping.c            |  7 +++++
 6 files changed, 74 insertions(+), 2 deletions(-)
 create mode 100644 drivers/firmware/smccc/smccc_trace.c
 create mode 100644 drivers/firmware/smccc/smccc_trace.h

diff --git a/drivers/firmware/smccc/Makefile b/drivers/firmware/smccc/Makefile
index 40d19144a860..e74c35191b49 100644
--- a/drivers/firmware/smccc/Makefile
+++ b/drivers/firmware/smccc/Makefile
@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
 #
-obj-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)	+= smccc.o kvm_guest.o
+ccflags-y += -I$(src)
+obj-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)	+= smccc_trace.o smccc.o kvm_guest.o
 obj-$(CONFIG_ARM_SMCCC_SOC_ID)	+= soc_id.o
diff --git a/drivers/firmware/smccc/smccc.c b/drivers/firmware/smccc/smccc.c
index 60ccf3e90d7d..7631a16479e9 100644
--- a/drivers/firmware/smccc/smccc.c
+++ b/drivers/firmware/smccc/smccc.c
@@ -11,6 +11,7 @@
 #include <linux/kernel.h>
 #include <linux/platform_device.h>
 #include <asm/archrandom.h>
+#include "smccc_trace.h"
 
 static u32 smccc_version = ARM_SMCCC_VERSION_1_0;
 static enum arm_smccc_conduit smccc_conduit = SMCCC_CONDUIT_NONE;
@@ -59,3 +60,16 @@ static int __init smccc_devices_init(void)
 	return 0;
 }
 device_initcall(smccc_devices_init);
+
+void arm_smccc_smc(unsigned long a0, unsigned long a1, unsigned long a2, unsigned long a3,
+		   unsigned long a4, unsigned long a5, unsigned long a6, unsigned long a7,
+		   struct arm_smccc_res *res)
+{
+	u64 start, elapsed;
+
+	trace_arm_smccc_smc_start(a0);
+	start = ktime_get_ns();
+	__arm_smccc_smc(a0, a1, a2, a3, a4, a5, a6, a7, res, NULL);
+	elapsed = ktime_get_ns() - start;
+	trace_arm_smccc_smc_end(a0, elapsed);
+}
diff --git a/drivers/firmware/smccc/smccc_trace.c b/drivers/firmware/smccc/smccc_trace.c
new file mode 100644
index 000000000000..6b94d5d9c0f4
--- /dev/null
+++ b/drivers/firmware/smccc/smccc_trace.c
@@ -0,0 +1,7 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define CREATE_TRACE_POINTS
+#include "smccc_trace.h"
+
+EXPORT_TRACEPOINT_SYMBOL(arm_smccc_smc_start);
+EXPORT_TRACEPOINT_SYMBOL(arm_smccc_smc_end);
diff --git a/drivers/firmware/smccc/smccc_trace.h b/drivers/firmware/smccc/smccc_trace.h
new file mode 100644
index 000000000000..c0ef836bc093
--- /dev/null
+++ b/drivers/firmware/smccc/smccc_trace.h
@@ -0,0 +1,41 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM smccc
+
+#if !defined(__SMCCC_TRACE_H) || defined(TRACE_HEADER_MULTI_READ)
+#define __SMCCC_TRACE_H
+
+#include <linux/types.h>
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(arm_smccc_smc_start,
+	    TP_PROTO(unsigned long smc_id),
+	    TP_ARGS(smc_id),
+	    TP_STRUCT__entry(__field(unsigned long, smc_id)),
+	    TP_fast_assign(__entry->smc_id = smc_id;),
+	    TP_printk("SMC ID: 0x%lx", __entry->smc_id)
+);
+
+TRACE_EVENT(arm_smccc_smc_end,
+	    TP_PROTO(unsigned long smc_id, u64 elapsed_time),
+	    TP_ARGS(smc_id, elapsed_time),
+	    TP_STRUCT__entry(__field(unsigned long, smc_id)
+			     __field(u64, elapsed_time)
+	    ),
+	    TP_fast_assign(__entry->smc_id = smc_id;
+			   __entry->elapsed_time = elapsed_time;
+	    ),
+	    TP_printk("SMC ID: 0x%lx time taken to process : %llu ns",
+		      __entry->smc_id, __entry->elapsed_time)
+);
+
+#endif /* __SMCCC_TRACE_H */
+
+#undef TRACE_INCLUDE_PATH
+#define TRACE_INCLUDE_PATH .
+
+#undef TRACE_INCLUDE_FILE
+#define TRACE_INCLUDE_FILE smccc_trace
+
+#include <trace/define_trace.h>
diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index 220c8c60e021..39588c3db486 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -358,7 +358,9 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
 			unsigned long a5, unsigned long a6, unsigned long a7,
 			struct arm_smccc_res *res, struct arm_smccc_quirk *quirk);
 
-#define arm_smccc_smc(...) __arm_smccc_smc(__VA_ARGS__, NULL)
+void arm_smccc_smc(unsigned long a0, unsigned long a1, unsigned long a2,
+		   unsigned long a3, unsigned long a4, unsigned long a5,
+		   unsigned long a6, unsigned long a7, struct arm_smccc_res *res);
 
 #define arm_smccc_smc_quirk(...) __arm_smccc_smc(__VA_ARGS__)
 
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 5579ead449f2..3a3bf2d674a3 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -192,6 +192,13 @@ static inline u64 tk_clock_read(const struct tk_read_base *tkr)
 {
 	struct clocksource *clock = READ_ONCE(tkr->clock);
 
+	/* At the time of kernel booting some SMC calls are called before the
+	 * clock is initialized, in such cases it would lead to kernel crash.
+	 * To prevent kernel crash in such cases this check is included.
+	 */
+	if (unlikely(!clock))
+		return 0;
+
 	return clock->read(clock);
 }
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] arm64: smccc: Add trace events to SMC calls.
  2023-03-04 12:58 [PATCH] arm64: smccc: Add trace events to SMC calls Rakesh Babu Saladi
@ 2023-03-04 17:38 ` kernel test robot
  2023-03-04 17:38 ` kernel test robot
  2023-03-06 14:47 ` Mark Rutland
  2 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2023-03-04 17:38 UTC (permalink / raw)
  To: Rakesh Babu Saladi, mark.rutland, lpieralisi, sudeep.holla,
	linux-arm-kernel, linux-kernel, sgoutham, gcherian, naveenm
  Cc: oe-kbuild-all, Rakesh Babu Saladi

Hi Rakesh,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tip/timers/core]
[also build test ERROR on arm/for-next arm/fixes arm64/for-next/core kvmarm/next soc/for-next xilinx-xlnx/master linus/master v6.2 next-20230303]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Rakesh-Babu-Saladi/arm64-smccc-Add-trace-events-to-SMC-calls/20230304-210103
patch link:    https://lore.kernel.org/r/20230304125850.32687-1-rsaladi2%40marvell.com
patch subject: [PATCH] arm64: smccc: Add trace events to SMC calls.
config: parisc-buildonly-randconfig-r003-20230302 (https://download.01.org/0day-ci/archive/20230305/202303050132.lgJCOelW-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/ebdc6e981b26c56978d65ad39d650fd2a7862d1a
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Rakesh-Babu-Saladi/arm64-smccc-Add-trace-events-to-SMC-calls/20230304-210103
        git checkout ebdc6e981b26c56978d65ad39d650fd2a7862d1a
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=parisc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=parisc SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303050132.lgJCOelW-lkp@intel.com/

All errors (new ones prefixed by >>):

   hppa-linux-ld: drivers/usb/host/ohci-at91.o: in function `ohci_at91_port_suspend.isra.0':
>> drivers/usb/host/ohci-at91.c:332: undefined reference to `arm_smccc_smc'
   hppa-linux-ld: drivers/memory/mtk-smi.o: in function `mtk_smi_larb_config_port_gen2_general':
   drivers/memory/mtk-smi.c:269: undefined reference to `arm_smccc_smc'
   hppa-linux-ld: drivers/firmware/qcom_scm-legacy.o: in function `scm_legacy_call':
   drivers/firmware/qcom_scm-legacy.c:114: undefined reference to `arm_smccc_smc'
   hppa-linux-ld: drivers/firmware/qcom_scm-legacy.o: in function `scm_legacy_call_atomic':
   drivers/firmware/qcom_scm-legacy.c:234: undefined reference to `arm_smccc_smc'


vim +332 drivers/usb/host/ohci-at91.c

aa6e52a35d388e Thomas Petazzoni 2011-07-13  322  
1e073e3ed9ff9e Clément Léger    2022-06-07  323  static int ohci_at91_port_suspend(struct ohci_at91_priv *ohci_at91, u8 set)
2e2aa1bc7eff90 Wenyou Yang      2016-08-23  324  {
1e073e3ed9ff9e Clément Léger    2022-06-07  325  	struct regmap *regmap = ohci_at91->sfr_regmap;
2e2aa1bc7eff90 Wenyou Yang      2016-08-23  326  	u32 regval;
2e2aa1bc7eff90 Wenyou Yang      2016-08-23  327  	int ret;
2e2aa1bc7eff90 Wenyou Yang      2016-08-23  328  
1e073e3ed9ff9e Clément Léger    2022-06-07  329  	if (ohci_at91->suspend_smc_id) {
1e073e3ed9ff9e Clément Léger    2022-06-07  330  		struct arm_smccc_res res;
2e2aa1bc7eff90 Wenyou Yang      2016-08-23  331  
1e073e3ed9ff9e Clément Léger    2022-06-07 @332  		arm_smccc_smc(ohci_at91->suspend_smc_id, set, 0, 0, 0, 0, 0, 0, &res);
1e073e3ed9ff9e Clément Léger    2022-06-07  333  		if (res.a0)
1e073e3ed9ff9e Clément Léger    2022-06-07  334  			return -EINVAL;
1e073e3ed9ff9e Clément Léger    2022-06-07  335  	} else if (regmap) {
2e2aa1bc7eff90 Wenyou Yang      2016-08-23  336  		ret = regmap_read(regmap, AT91_SFR_OHCIICR, &regval);
2e2aa1bc7eff90 Wenyou Yang      2016-08-23  337  		if (ret)
2e2aa1bc7eff90 Wenyou Yang      2016-08-23  338  			return ret;
2e2aa1bc7eff90 Wenyou Yang      2016-08-23  339  
2e2aa1bc7eff90 Wenyou Yang      2016-08-23  340  		if (set)
2e2aa1bc7eff90 Wenyou Yang      2016-08-23  341  			regval |= AT91_OHCIICR_USB_SUSPEND;
2e2aa1bc7eff90 Wenyou Yang      2016-08-23  342  		else
2e2aa1bc7eff90 Wenyou Yang      2016-08-23  343  			regval &= ~AT91_OHCIICR_USB_SUSPEND;
2e2aa1bc7eff90 Wenyou Yang      2016-08-23  344  
2e2aa1bc7eff90 Wenyou Yang      2016-08-23  345  		regmap_write(regmap, AT91_SFR_OHCIICR, regval);
1e073e3ed9ff9e Clément Léger    2022-06-07  346  	}
2e2aa1bc7eff90 Wenyou Yang      2016-08-23  347  
2e2aa1bc7eff90 Wenyou Yang      2016-08-23  348  	return 0;
2e2aa1bc7eff90 Wenyou Yang      2016-08-23  349  }
2e2aa1bc7eff90 Wenyou Yang      2016-08-23  350  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] arm64: smccc: Add trace events to SMC calls.
  2023-03-04 12:58 [PATCH] arm64: smccc: Add trace events to SMC calls Rakesh Babu Saladi
  2023-03-04 17:38 ` kernel test robot
@ 2023-03-04 17:38 ` kernel test robot
  2023-03-06 14:47 ` Mark Rutland
  2 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2023-03-04 17:38 UTC (permalink / raw)
  To: Rakesh Babu Saladi, mark.rutland, lpieralisi, sudeep.holla,
	linux-arm-kernel, linux-kernel, sgoutham, gcherian, naveenm
  Cc: oe-kbuild-all, Rakesh Babu Saladi

Hi Rakesh,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tip/timers/core]
[also build test ERROR on arm/for-next arm/fixes arm64/for-next/core kvmarm/next soc/for-next xilinx-xlnx/master linus/master v6.2 next-20230303]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Rakesh-Babu-Saladi/arm64-smccc-Add-trace-events-to-SMC-calls/20230304-210103
patch link:    https://lore.kernel.org/r/20230304125850.32687-1-rsaladi2%40marvell.com
patch subject: [PATCH] arm64: smccc: Add trace events to SMC calls.
config: m68k-randconfig-r032-20230302 (https://download.01.org/0day-ci/archive/20230305/202303050135.zEqZYkVJ-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/ebdc6e981b26c56978d65ad39d650fd2a7862d1a
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Rakesh-Babu-Saladi/arm64-smccc-Add-trace-events-to-SMC-calls/20230304-210103
        git checkout ebdc6e981b26c56978d65ad39d650fd2a7862d1a
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303050135.zEqZYkVJ-lkp@intel.com/

All errors (new ones prefixed by >>):

   m68k-linux-ld: section .rodata VMA [0000000000002000,000000000039d947] overlaps section .text VMA [0000000000000400,0000000000876c3f]
   m68k-linux-ld: drivers/firmware/qcom_scm-legacy.o: in function `scm_legacy_call':
>> drivers/firmware/qcom_scm-legacy.c:176: undefined reference to `arm_smccc_smc'
   m68k-linux-ld: drivers/firmware/qcom_scm-legacy.o: in function `scm_legacy_call_atomic':
   drivers/firmware/qcom_scm-legacy.c:234: undefined reference to `arm_smccc_smc'


vim +176 drivers/firmware/qcom_scm-legacy.c

b6a1dfbc7d5740 drivers/firmware/qcom_scm-32.c     Kumar Gala          2015-03-11  119  
b6a1dfbc7d5740 drivers/firmware/qcom_scm-32.c     Kumar Gala          2015-03-11  120  /**
e1cd92da0b3321 drivers/firmware/qcom_scm-legacy.c Stephen Boyd        2021-02-23  121   * scm_legacy_call() - Sends a command to the SCM and waits for the command to
efd2b15c21a043 drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  122   * finish processing.
ebf21bbc2ff56a drivers/firmware/qcom_scm-legacy.c Krzysztof Kozlowski 2022-05-19  123   * @dev:	device
ebf21bbc2ff56a drivers/firmware/qcom_scm-legacy.c Krzysztof Kozlowski 2022-05-19  124   * @desc:	descriptor structure containing arguments and return values
ebf21bbc2ff56a drivers/firmware/qcom_scm-legacy.c Krzysztof Kozlowski 2022-05-19  125   * @res:        results from SMC call
b6a1dfbc7d5740 drivers/firmware/qcom_scm-32.c     Kumar Gala          2015-03-11  126   *
b6a1dfbc7d5740 drivers/firmware/qcom_scm-32.c     Kumar Gala          2015-03-11  127   * A note on cache maintenance:
b6a1dfbc7d5740 drivers/firmware/qcom_scm-32.c     Kumar Gala          2015-03-11  128   * Note that any buffers that are expected to be accessed by the secure world
b6a1dfbc7d5740 drivers/firmware/qcom_scm-32.c     Kumar Gala          2015-03-11  129   * must be flushed before invoking qcom_scm_call and invalidated in the cache
b6a1dfbc7d5740 drivers/firmware/qcom_scm-32.c     Kumar Gala          2015-03-11  130   * immediately after qcom_scm_call returns. Cache maintenance on the command
b6a1dfbc7d5740 drivers/firmware/qcom_scm-32.c     Kumar Gala          2015-03-11  131   * and response buffers is taken care of by qcom_scm_call; however, callers are
b6a1dfbc7d5740 drivers/firmware/qcom_scm-32.c     Kumar Gala          2015-03-11  132   * responsible for any other cached buffers passed over to the secure world.
b6a1dfbc7d5740 drivers/firmware/qcom_scm-32.c     Kumar Gala          2015-03-11  133   */
9a434cee773ae1 drivers/firmware/qcom_scm-legacy.c Elliot Berman       2020-01-07  134  int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
efd2b15c21a043 drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  135  		    struct qcom_scm_res *res)
b6a1dfbc7d5740 drivers/firmware/qcom_scm-32.c     Kumar Gala          2015-03-11  136  {
efd2b15c21a043 drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  137  	u8 arglen = desc->arginfo & 0xf;
590e92809a58a5 drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  138  	int ret = 0, context_id;
efd2b15c21a043 drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  139  	unsigned int i;
e0aa153965041c drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  140  	struct scm_legacy_command *cmd;
e0aa153965041c drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  141  	struct scm_legacy_response *rsp;
590e92809a58a5 drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  142  	struct arm_smccc_args smc = {0};
590e92809a58a5 drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  143  	struct arm_smccc_res smc_res;
efd2b15c21a043 drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  144  	const size_t cmd_len = arglen * sizeof(__le32);
efd2b15c21a043 drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  145  	const size_t resp_len = MAX_QCOM_SCM_RETS * sizeof(__le32);
16e59467a44651 drivers/firmware/qcom_scm-32.c     Andy Gross          2016-06-03  146  	size_t alloc_len = sizeof(*cmd) + cmd_len + sizeof(*rsp) + resp_len;
16e59467a44651 drivers/firmware/qcom_scm-32.c     Andy Gross          2016-06-03  147  	dma_addr_t cmd_phys;
efd2b15c21a043 drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  148  	__le32 *arg_buf;
efd2b15c21a043 drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  149  	const __le32 *res_buf;
b6a1dfbc7d5740 drivers/firmware/qcom_scm-32.c     Kumar Gala          2015-03-11  150  
16e59467a44651 drivers/firmware/qcom_scm-32.c     Andy Gross          2016-06-03  151  	cmd = kzalloc(PAGE_ALIGN(alloc_len), GFP_KERNEL);
b6a1dfbc7d5740 drivers/firmware/qcom_scm-32.c     Kumar Gala          2015-03-11  152  	if (!cmd)
b6a1dfbc7d5740 drivers/firmware/qcom_scm-32.c     Kumar Gala          2015-03-11  153  		return -ENOMEM;
b6a1dfbc7d5740 drivers/firmware/qcom_scm-32.c     Kumar Gala          2015-03-11  154  
16e59467a44651 drivers/firmware/qcom_scm-32.c     Andy Gross          2016-06-03  155  	cmd->len = cpu_to_le32(alloc_len);
16e59467a44651 drivers/firmware/qcom_scm-32.c     Andy Gross          2016-06-03  156  	cmd->buf_offset = cpu_to_le32(sizeof(*cmd));
16e59467a44651 drivers/firmware/qcom_scm-32.c     Andy Gross          2016-06-03  157  	cmd->resp_hdr_offset = cpu_to_le32(sizeof(*cmd) + cmd_len);
efd2b15c21a043 drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  158  	cmd->id = cpu_to_le32(SCM_LEGACY_FNID(desc->svc, desc->cmd));
16e59467a44651 drivers/firmware/qcom_scm-32.c     Andy Gross          2016-06-03  159  
efd2b15c21a043 drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  160  	arg_buf = scm_legacy_get_command_buffer(cmd);
efd2b15c21a043 drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  161  	for (i = 0; i < arglen; i++)
efd2b15c21a043 drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  162  		arg_buf[i] = cpu_to_le32(desc->args[i]);
b6a1dfbc7d5740 drivers/firmware/qcom_scm-32.c     Kumar Gala          2015-03-11  163  
e0aa153965041c drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  164  	rsp = scm_legacy_command_to_response(cmd);
16e59467a44651 drivers/firmware/qcom_scm-32.c     Andy Gross          2016-06-03  165  
16e59467a44651 drivers/firmware/qcom_scm-32.c     Andy Gross          2016-06-03  166  	cmd_phys = dma_map_single(dev, cmd, alloc_len, DMA_TO_DEVICE);
16e59467a44651 drivers/firmware/qcom_scm-32.c     Andy Gross          2016-06-03  167  	if (dma_mapping_error(dev, cmd_phys)) {
16e59467a44651 drivers/firmware/qcom_scm-32.c     Andy Gross          2016-06-03  168  		kfree(cmd);
16e59467a44651 drivers/firmware/qcom_scm-32.c     Andy Gross          2016-06-03  169  		return -ENOMEM;
16e59467a44651 drivers/firmware/qcom_scm-32.c     Andy Gross          2016-06-03  170  	}
16e59467a44651 drivers/firmware/qcom_scm-32.c     Andy Gross          2016-06-03  171  
590e92809a58a5 drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  172  	smc.args[0] = 1;
590e92809a58a5 drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  173  	smc.args[1] = (unsigned long)&context_id;
590e92809a58a5 drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  174  	smc.args[2] = cmd_phys;
590e92809a58a5 drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  175  
b6a1dfbc7d5740 drivers/firmware/qcom_scm-32.c     Kumar Gala          2015-03-11 @176  	mutex_lock(&qcom_scm_lock);
590e92809a58a5 drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  177  	__scm_legacy_do(&smc, &smc_res);
590e92809a58a5 drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  178  	if (smc_res.a0)
590e92809a58a5 drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  179  		ret = qcom_scm_remap_error(smc_res.a0);
b6a1dfbc7d5740 drivers/firmware/qcom_scm-32.c     Kumar Gala          2015-03-11  180  	mutex_unlock(&qcom_scm_lock);
b6a1dfbc7d5740 drivers/firmware/qcom_scm-32.c     Kumar Gala          2015-03-11  181  	if (ret)
b6a1dfbc7d5740 drivers/firmware/qcom_scm-32.c     Kumar Gala          2015-03-11  182  		goto out;
b6a1dfbc7d5740 drivers/firmware/qcom_scm-32.c     Kumar Gala          2015-03-11  183  
b6a1dfbc7d5740 drivers/firmware/qcom_scm-32.c     Kumar Gala          2015-03-11  184  	do {
16e59467a44651 drivers/firmware/qcom_scm-32.c     Andy Gross          2016-06-03  185  		dma_sync_single_for_cpu(dev, cmd_phys + sizeof(*cmd) + cmd_len,
16e59467a44651 drivers/firmware/qcom_scm-32.c     Andy Gross          2016-06-03  186  					sizeof(*rsp), DMA_FROM_DEVICE);
b6a1dfbc7d5740 drivers/firmware/qcom_scm-32.c     Kumar Gala          2015-03-11  187  	} while (!rsp->is_complete);
b6a1dfbc7d5740 drivers/firmware/qcom_scm-32.c     Kumar Gala          2015-03-11  188  
16e59467a44651 drivers/firmware/qcom_scm-32.c     Andy Gross          2016-06-03  189  	dma_sync_single_for_cpu(dev, cmd_phys + sizeof(*cmd) + cmd_len +
16e59467a44651 drivers/firmware/qcom_scm-32.c     Andy Gross          2016-06-03  190  				le32_to_cpu(rsp->buf_offset),
16e59467a44651 drivers/firmware/qcom_scm-32.c     Andy Gross          2016-06-03  191  				resp_len, DMA_FROM_DEVICE);
efd2b15c21a043 drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  192  
efd2b15c21a043 drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  193  	if (res) {
efd2b15c21a043 drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  194  		res_buf = scm_legacy_get_response_buffer(rsp);
efd2b15c21a043 drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  195  		for (i = 0; i < MAX_QCOM_SCM_RETS; i++)
efd2b15c21a043 drivers/firmware/qcom_scm-32.c     Elliot Berman       2020-01-07  196  			res->result[i] = le32_to_cpu(res_buf[i]);
16e59467a44651 drivers/firmware/qcom_scm-32.c     Andy Gross          2016-06-03  197  	}
b6a1dfbc7d5740 drivers/firmware/qcom_scm-32.c     Kumar Gala          2015-03-11  198  out:
16e59467a44651 drivers/firmware/qcom_scm-32.c     Andy Gross          2016-06-03  199  	dma_unmap_single(dev, cmd_phys, alloc_len, DMA_TO_DEVICE);
16e59467a44651 drivers/firmware/qcom_scm-32.c     Andy Gross          2016-06-03  200  	kfree(cmd);
b6a1dfbc7d5740 drivers/firmware/qcom_scm-32.c     Kumar Gala          2015-03-11  201  	return ret;
b6a1dfbc7d5740 drivers/firmware/qcom_scm-32.c     Kumar Gala          2015-03-11  202  }
b6a1dfbc7d5740 drivers/firmware/qcom_scm-32.c     Kumar Gala          2015-03-11  203  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] arm64: smccc: Add trace events to SMC calls.
  2023-03-04 12:58 [PATCH] arm64: smccc: Add trace events to SMC calls Rakesh Babu Saladi
  2023-03-04 17:38 ` kernel test robot
  2023-03-04 17:38 ` kernel test robot
@ 2023-03-06 14:47 ` Mark Rutland
  2023-03-08 12:25   ` [EXT] " Rakesh Babu Saladi
  2 siblings, 1 reply; 7+ messages in thread
From: Mark Rutland @ 2023-03-06 14:47 UTC (permalink / raw)
  To: Rakesh Babu Saladi
  Cc: lpieralisi, sudeep.holla, linux-arm-kernel, linux-kernel,
	sgoutham, gcherian, naveenm

On Sat, Mar 04, 2023 at 06:28:50PM +0530, Rakesh Babu Saladi wrote:
> This patch adds start and end trace events to an SMC call sent from
> kernel to ATF. The start trace event prints the smc_id and the end
> trace event prints the smc_id and the time taken to process the SMC
> call.
> 
> Signed-off-by: Rakesh Babu Saladi <rsaladi2@marvell.com>

We've said no to this in the past:

  https://lore.kernel.org/lkml/20210923112058.GA14893@C02TD0UTHF1T.local/

I don't think anything has changed, and this has all the same problems as
before, so I do not think we should do this.

Which SMC calls do you want to trace, and why?

Thanks,
Mark.

> ---
>  drivers/firmware/smccc/Makefile      |  3 +-
>  drivers/firmware/smccc/smccc.c       | 14 ++++++++++
>  drivers/firmware/smccc/smccc_trace.c |  7 +++++
>  drivers/firmware/smccc/smccc_trace.h | 41 ++++++++++++++++++++++++++++
>  include/linux/arm-smccc.h            |  4 ++-
>  kernel/time/timekeeping.c            |  7 +++++
>  6 files changed, 74 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/firmware/smccc/smccc_trace.c
>  create mode 100644 drivers/firmware/smccc/smccc_trace.h
> 
> diff --git a/drivers/firmware/smccc/Makefile b/drivers/firmware/smccc/Makefile
> index 40d19144a860..e74c35191b49 100644
> --- a/drivers/firmware/smccc/Makefile
> +++ b/drivers/firmware/smccc/Makefile
> @@ -1,4 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0
>  #
> -obj-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)	+= smccc.o kvm_guest.o
> +ccflags-y += -I$(src)
> +obj-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)	+= smccc_trace.o smccc.o kvm_guest.o
>  obj-$(CONFIG_ARM_SMCCC_SOC_ID)	+= soc_id.o
> diff --git a/drivers/firmware/smccc/smccc.c b/drivers/firmware/smccc/smccc.c
> index 60ccf3e90d7d..7631a16479e9 100644
> --- a/drivers/firmware/smccc/smccc.c
> +++ b/drivers/firmware/smccc/smccc.c
> @@ -11,6 +11,7 @@
>  #include <linux/kernel.h>
>  #include <linux/platform_device.h>
>  #include <asm/archrandom.h>
> +#include "smccc_trace.h"
>  
>  static u32 smccc_version = ARM_SMCCC_VERSION_1_0;
>  static enum arm_smccc_conduit smccc_conduit = SMCCC_CONDUIT_NONE;
> @@ -59,3 +60,16 @@ static int __init smccc_devices_init(void)
>  	return 0;
>  }
>  device_initcall(smccc_devices_init);
> +
> +void arm_smccc_smc(unsigned long a0, unsigned long a1, unsigned long a2, unsigned long a3,
> +		   unsigned long a4, unsigned long a5, unsigned long a6, unsigned long a7,
> +		   struct arm_smccc_res *res)
> +{
> +	u64 start, elapsed;
> +
> +	trace_arm_smccc_smc_start(a0);
> +	start = ktime_get_ns();
> +	__arm_smccc_smc(a0, a1, a2, a3, a4, a5, a6, a7, res, NULL);
> +	elapsed = ktime_get_ns() - start;
> +	trace_arm_smccc_smc_end(a0, elapsed);
> +}
> diff --git a/drivers/firmware/smccc/smccc_trace.c b/drivers/firmware/smccc/smccc_trace.c
> new file mode 100644
> index 000000000000..6b94d5d9c0f4
> --- /dev/null
> +++ b/drivers/firmware/smccc/smccc_trace.c
> @@ -0,0 +1,7 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#define CREATE_TRACE_POINTS
> +#include "smccc_trace.h"
> +
> +EXPORT_TRACEPOINT_SYMBOL(arm_smccc_smc_start);
> +EXPORT_TRACEPOINT_SYMBOL(arm_smccc_smc_end);
> diff --git a/drivers/firmware/smccc/smccc_trace.h b/drivers/firmware/smccc/smccc_trace.h
> new file mode 100644
> index 000000000000..c0ef836bc093
> --- /dev/null
> +++ b/drivers/firmware/smccc/smccc_trace.h
> @@ -0,0 +1,41 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM smccc
> +
> +#if !defined(__SMCCC_TRACE_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define __SMCCC_TRACE_H
> +
> +#include <linux/types.h>
> +#include <linux/tracepoint.h>
> +
> +TRACE_EVENT(arm_smccc_smc_start,
> +	    TP_PROTO(unsigned long smc_id),
> +	    TP_ARGS(smc_id),
> +	    TP_STRUCT__entry(__field(unsigned long, smc_id)),
> +	    TP_fast_assign(__entry->smc_id = smc_id;),
> +	    TP_printk("SMC ID: 0x%lx", __entry->smc_id)
> +);
> +
> +TRACE_EVENT(arm_smccc_smc_end,
> +	    TP_PROTO(unsigned long smc_id, u64 elapsed_time),
> +	    TP_ARGS(smc_id, elapsed_time),
> +	    TP_STRUCT__entry(__field(unsigned long, smc_id)
> +			     __field(u64, elapsed_time)
> +	    ),
> +	    TP_fast_assign(__entry->smc_id = smc_id;
> +			   __entry->elapsed_time = elapsed_time;
> +	    ),
> +	    TP_printk("SMC ID: 0x%lx time taken to process : %llu ns",
> +		      __entry->smc_id, __entry->elapsed_time)
> +);
> +
> +#endif /* __SMCCC_TRACE_H */
> +
> +#undef TRACE_INCLUDE_PATH
> +#define TRACE_INCLUDE_PATH .
> +
> +#undef TRACE_INCLUDE_FILE
> +#define TRACE_INCLUDE_FILE smccc_trace
> +
> +#include <trace/define_trace.h>
> diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> index 220c8c60e021..39588c3db486 100644
> --- a/include/linux/arm-smccc.h
> +++ b/include/linux/arm-smccc.h
> @@ -358,7 +358,9 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
>  			unsigned long a5, unsigned long a6, unsigned long a7,
>  			struct arm_smccc_res *res, struct arm_smccc_quirk *quirk);
>  
> -#define arm_smccc_smc(...) __arm_smccc_smc(__VA_ARGS__, NULL)
> +void arm_smccc_smc(unsigned long a0, unsigned long a1, unsigned long a2,
> +		   unsigned long a3, unsigned long a4, unsigned long a5,
> +		   unsigned long a6, unsigned long a7, struct arm_smccc_res *res);
>  
>  #define arm_smccc_smc_quirk(...) __arm_smccc_smc(__VA_ARGS__)
>  
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index 5579ead449f2..3a3bf2d674a3 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -192,6 +192,13 @@ static inline u64 tk_clock_read(const struct tk_read_base *tkr)
>  {
>  	struct clocksource *clock = READ_ONCE(tkr->clock);
>  
> +	/* At the time of kernel booting some SMC calls are called before the
> +	 * clock is initialized, in such cases it would lead to kernel crash.
> +	 * To prevent kernel crash in such cases this check is included.
> +	 */
> +	if (unlikely(!clock))
> +		return 0;
> +
>  	return clock->read(clock);
>  }
>  
> -- 
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [EXT] Re: [PATCH] arm64: smccc: Add trace events to SMC calls.
  2023-03-06 14:47 ` Mark Rutland
@ 2023-03-08 12:25   ` Rakesh Babu Saladi
  2023-03-08 13:42     ` Sudeep Holla
  2023-03-08 13:58     ` Mark Rutland
  0 siblings, 2 replies; 7+ messages in thread
From: Rakesh Babu Saladi @ 2023-03-08 12:25 UTC (permalink / raw)
  To: Mark Rutland
  Cc: lpieralisi@kernel.org, sudeep.holla@arm.com,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Sunil Kovvuri Goutham,
	George Cherian, Naveen Mamindlapalli

Hi Mark, 

Please see comments in line.

Thanks,
Rakesh.

-----Original Message-----
From: Mark Rutland <mark.rutland@arm.com> 
Sent: Monday, March 6, 2023 8:18 PM
To: Rakesh Babu Saladi <rsaladi2@marvell.com>
Cc: lpieralisi@kernel.org; sudeep.holla@arm.com; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Sunil Kovvuri Goutham <sgoutham@marvell.com>; George Cherian <gcherian@marvell.com>; Naveen Mamindlapalli <naveenm@marvell.com>
Subject: [EXT] Re: [PATCH] arm64: smccc: Add trace events to SMC calls.

External Email

----------------------------------------------------------------------
On Sat, Mar 04, 2023 at 06:28:50PM +0530, Rakesh Babu Saladi wrote:
> This patch adds start and end trace events to an SMC call sent from 
> kernel to ATF. The start trace event prints the smc_id and the end 
> trace event prints the smc_id and the time taken to process the SMC 
> call.
> 
> Signed-off-by: Rakesh Babu Saladi <rsaladi2@marvell.com>

We've said no to this in the past:

  https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_lkml_20210923112058.GA14893-40C02TD0UTHF1T.local_&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=OrNjoyjPmqlDfiSr6RMDtAJU4gxiGfgzEtgbqDmoQPA&m=fj6aQCqc0xOYgRMw7BGCVRrOEabi2taRuKl63gCdHras-JfpWCGhjp6HqaLZYj9I&s=1_EJt65j5EoyHhcroJxJFMIkTqQY6jKtWypPIzmgYK8&e= 

I don't think anything has changed, and this has all the same problems as before, so I do not think we should do this.
Rakesh >> Can you please be more specific why the changes are not getting accepted?

Which SMC calls do you want to trace, and why?
Rakesh >> These traces we would like to have for debugging purposes while calling any SMC call.

Thanks,
Mark.

> ---
>  drivers/firmware/smccc/Makefile      |  3 +-
>  drivers/firmware/smccc/smccc.c       | 14 ++++++++++
>  drivers/firmware/smccc/smccc_trace.c |  7 +++++  
> drivers/firmware/smccc/smccc_trace.h | 41 ++++++++++++++++++++++++++++
>  include/linux/arm-smccc.h            |  4 ++-
>  kernel/time/timekeeping.c            |  7 +++++
>  6 files changed, 74 insertions(+), 2 deletions(-)  create mode 100644 
> drivers/firmware/smccc/smccc_trace.c
>  create mode 100644 drivers/firmware/smccc/smccc_trace.h
> 
> diff --git a/drivers/firmware/smccc/Makefile 
> b/drivers/firmware/smccc/Makefile index 40d19144a860..e74c35191b49 
> 100644
> --- a/drivers/firmware/smccc/Makefile
> +++ b/drivers/firmware/smccc/Makefile
> @@ -1,4 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0
>  #
> -obj-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)	+= smccc.o kvm_guest.o
> +ccflags-y += -I$(src)
> +obj-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)	+= smccc_trace.o smccc.o kvm_guest.o
>  obj-$(CONFIG_ARM_SMCCC_SOC_ID)	+= soc_id.o
> diff --git a/drivers/firmware/smccc/smccc.c 
> b/drivers/firmware/smccc/smccc.c index 60ccf3e90d7d..7631a16479e9 
> 100644
> --- a/drivers/firmware/smccc/smccc.c
> +++ b/drivers/firmware/smccc/smccc.c
> @@ -11,6 +11,7 @@
>  #include <linux/kernel.h>
>  #include <linux/platform_device.h>
>  #include <asm/archrandom.h>
> +#include "smccc_trace.h"
>  
>  static u32 smccc_version = ARM_SMCCC_VERSION_1_0;  static enum 
> arm_smccc_conduit smccc_conduit = SMCCC_CONDUIT_NONE; @@ -59,3 +60,16 
> @@ static int __init smccc_devices_init(void)
>  	return 0;
>  }
>  device_initcall(smccc_devices_init);
> +
> +void arm_smccc_smc(unsigned long a0, unsigned long a1, unsigned long a2, unsigned long a3,
> +		   unsigned long a4, unsigned long a5, unsigned long a6, unsigned long a7,
> +		   struct arm_smccc_res *res)
> +{
> +	u64 start, elapsed;
> +
> +	trace_arm_smccc_smc_start(a0);
> +	start = ktime_get_ns();
> +	__arm_smccc_smc(a0, a1, a2, a3, a4, a5, a6, a7, res, NULL);
> +	elapsed = ktime_get_ns() - start;
> +	trace_arm_smccc_smc_end(a0, elapsed); }
> diff --git a/drivers/firmware/smccc/smccc_trace.c 
> b/drivers/firmware/smccc/smccc_trace.c
> new file mode 100644
> index 000000000000..6b94d5d9c0f4
> --- /dev/null
> +++ b/drivers/firmware/smccc/smccc_trace.c
> @@ -0,0 +1,7 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#define CREATE_TRACE_POINTS
> +#include "smccc_trace.h"
> +
> +EXPORT_TRACEPOINT_SYMBOL(arm_smccc_smc_start);
> +EXPORT_TRACEPOINT_SYMBOL(arm_smccc_smc_end);
> diff --git a/drivers/firmware/smccc/smccc_trace.h 
> b/drivers/firmware/smccc/smccc_trace.h
> new file mode 100644
> index 000000000000..c0ef836bc093
> --- /dev/null
> +++ b/drivers/firmware/smccc/smccc_trace.h
> @@ -0,0 +1,41 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM smccc
> +
> +#if !defined(__SMCCC_TRACE_H) || defined(TRACE_HEADER_MULTI_READ) 
> +#define __SMCCC_TRACE_H
> +
> +#include <linux/types.h>
> +#include <linux/tracepoint.h>
> +
> +TRACE_EVENT(arm_smccc_smc_start,
> +	    TP_PROTO(unsigned long smc_id),
> +	    TP_ARGS(smc_id),
> +	    TP_STRUCT__entry(__field(unsigned long, smc_id)),
> +	    TP_fast_assign(__entry->smc_id = smc_id;),
> +	    TP_printk("SMC ID: 0x%lx", __entry->smc_id) );
> +
> +TRACE_EVENT(arm_smccc_smc_end,
> +	    TP_PROTO(unsigned long smc_id, u64 elapsed_time),
> +	    TP_ARGS(smc_id, elapsed_time),
> +	    TP_STRUCT__entry(__field(unsigned long, smc_id)
> +			     __field(u64, elapsed_time)
> +	    ),
> +	    TP_fast_assign(__entry->smc_id = smc_id;
> +			   __entry->elapsed_time = elapsed_time;
> +	    ),
> +	    TP_printk("SMC ID: 0x%lx time taken to process : %llu ns",
> +		      __entry->smc_id, __entry->elapsed_time) );
> +
> +#endif /* __SMCCC_TRACE_H */
> +
> +#undef TRACE_INCLUDE_PATH
> +#define TRACE_INCLUDE_PATH .
> +
> +#undef TRACE_INCLUDE_FILE
> +#define TRACE_INCLUDE_FILE smccc_trace
> +
> +#include <trace/define_trace.h>
> diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h 
> index 220c8c60e021..39588c3db486 100644
> --- a/include/linux/arm-smccc.h
> +++ b/include/linux/arm-smccc.h
> @@ -358,7 +358,9 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
>  			unsigned long a5, unsigned long a6, unsigned long a7,
>  			struct arm_smccc_res *res, struct arm_smccc_quirk *quirk);
>  
> -#define arm_smccc_smc(...) __arm_smccc_smc(__VA_ARGS__, NULL)
> +void arm_smccc_smc(unsigned long a0, unsigned long a1, unsigned long a2,
> +		   unsigned long a3, unsigned long a4, unsigned long a5,
> +		   unsigned long a6, unsigned long a7, struct arm_smccc_res *res);
>  
>  #define arm_smccc_smc_quirk(...) __arm_smccc_smc(__VA_ARGS__)
>  
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c 
> index 5579ead449f2..3a3bf2d674a3 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -192,6 +192,13 @@ static inline u64 tk_clock_read(const struct 
> tk_read_base *tkr)  {
>  	struct clocksource *clock = READ_ONCE(tkr->clock);
>  
> +	/* At the time of kernel booting some SMC calls are called before the
> +	 * clock is initialized, in such cases it would lead to kernel crash.
> +	 * To prevent kernel crash in such cases this check is included.
> +	 */
> +	if (unlikely(!clock))
> +		return 0;
> +
>  	return clock->read(clock);
>  }
>  
> --
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [EXT] Re: [PATCH] arm64: smccc: Add trace events to SMC calls.
  2023-03-08 12:25   ` [EXT] " Rakesh Babu Saladi
@ 2023-03-08 13:42     ` Sudeep Holla
  2023-03-08 13:58     ` Mark Rutland
  1 sibling, 0 replies; 7+ messages in thread
From: Sudeep Holla @ 2023-03-08 13:42 UTC (permalink / raw)
  To: Rakesh Babu Saladi
  Cc: Mark Rutland, Sudeep Holla, lpieralisi@kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Sunil Kovvuri Goutham,
	George Cherian, Naveen Mamindlapalli

On Wed, Mar 08, 2023 at 12:25:15PM +0000, Rakesh Babu Saladi wrote:
> Hi Mark, 
> 
> 
>   https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_lkml_20210923112058.GA14893-40C02TD0UTHF1T.local_&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=OrNjoyjPmqlDfiSr6RMDtAJU4gxiGfgzEtgbqDmoQPA&m=fj6aQCqc0xOYgRMw7BGCVRrOEabi2taRuKl63gCdHras-JfpWCGhjp6HqaLZYj9I&s=1_EJt65j5EoyHhcroJxJFMIkTqQY6jKtWypPIzmgYK8&e= 
>

(Nit: You need to fix your email setup, it is spoiling the links and also
flow for reading in general)


> I don't think anything has changed, and this has all the same problems as
> before, so I do not think we should do this.
>
> Rakesh >> Can you please be more specific why the changes are not getting accepted?
>

Have you gone through the link Mark pointed at ?
For starters, you have new v1.2 specific calls that has x0-x17 as the
input and output registers which neither this patch or the earlier one
addressed if we agree to allow tracing SMCCC calls this way.

> Which SMC calls do you want to trace, and why?
> Rakesh >> These traces we would like to have for debugging purposes while calling any SMC call.
>

The main point was why would you want to enable universal tracing of all SMCCC
calls. Remember that means you would trace the most common and fundamental
for boot PSCI calls which cover CPU_SUSPEND and it gets called a *lot* for
obvious reasons. So it is not a good idea to do so.

So, what are you using this trace for ? Is that a specific driver using
SMCCC ? Can't the trace be added in that driver as you could add specifics
there ?

Taking v1.2 SMCCC as example, though the spec and the API itself allows
18 registers to be input and output, it may not be used in most of the
cases and adding them to the trace is just useless. Hence the suggestion
of making it specific to your use-case.

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [EXT] Re: [PATCH] arm64: smccc: Add trace events to SMC calls.
  2023-03-08 12:25   ` [EXT] " Rakesh Babu Saladi
  2023-03-08 13:42     ` Sudeep Holla
@ 2023-03-08 13:58     ` Mark Rutland
  1 sibling, 0 replies; 7+ messages in thread
From: Mark Rutland @ 2023-03-08 13:58 UTC (permalink / raw)
  To: Rakesh Babu Saladi
  Cc: lpieralisi@kernel.org, sudeep.holla@arm.com,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Sunil Kovvuri Goutham,
	George Cherian, Naveen Mamindlapalli

On Wed, Mar 08, 2023 at 12:25:15PM +0000, Rakesh Babu Saladi wrote:
> Hi Mark, 

Hi Rakesh,

As a general thing, your mail setup isn't being helpful here. It has corrupted
links below, and this style of quoting without leading '>' characters is
incredibly painful to read.

The inline comment style you've used heere is also painful to read, but I
understand that's working around the mail client being unhelpful.

There's some guidance on how to configure mail clients at:

  https://docs.kernel.org/process/email-clients.html

> 
> Please see comments in line.
> 
> Thanks,
> Rakesh.
> 
> -----Original Message-----
> From: Mark Rutland <mark.rutland@arm.com> 
> Sent: Monday, March 6, 2023 8:18 PM
> To: Rakesh Babu Saladi <rsaladi2@marvell.com>
> Cc: lpieralisi@kernel.org; sudeep.holla@arm.com; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Sunil Kovvuri Goutham <sgoutham@marvell.com>; George Cherian <gcherian@marvell.com>; Naveen Mamindlapalli <naveenm@marvell.com>
> Subject: [EXT] Re: [PATCH] arm64: smccc: Add trace events to SMC calls.
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Sat, Mar 04, 2023 at 06:28:50PM +0530, Rakesh Babu Saladi wrote:
> > This patch adds start and end trace events to an SMC call sent from 
> > kernel to ATF. The start trace event prints the smc_id and the end 
> > trace event prints the smc_id and the time taken to process the SMC 
> > call.
> > 
> > Signed-off-by: Rakesh Babu Saladi <rsaladi2@marvell.com>
> 
> We've said no to this in the past:
> 

  [...
    garbled link, was: 
    https://lore.kernel.org/lkml/20210923112058.GA14893@C02TD0UTHF1T.local/
  ...]

> I don't think anything has changed, and this has all the same problems as
> before, so I do not think we should do this.
> Rakesh >> Can you please be more specific why the changes are not getting accepted?

I was very specific in the reply I linked to at:

  https://lore.kernel.org/lkml/20210923112058.GA14893@C02TD0UTHF1T.local/

The gist being:

* There are a tonne of SMCCC invocations that this doesn't capture.

* This doesn't handle SMCCC variants which can pass x0-17

* This is very low level, and so not good for consumers (e.g. most of the
  arguments may be meadningless UNKNOWN values).

* This is very low level, and so painful for things which must avoid
  instrumentation (e.g. noinstr code for ARCH_WORKAROUND_*).

* SMCCC may change again in future, and I don't want to get stuck in an ABI we
  cannot maintain.

Which all remain the same here, so NAK to this patch as it stands.

As before, I'd suggest placing tracepoints at a higher level in the specific
SMCCC callers you're interested in.

> Which SMC calls do you want to trace, and why?
> Rakesh >> These traces we would like to have for debugging purposes while calling any SMC call.

That's not a lot of information to go on. Consider the following:

* Are you trying to solve a particular problem, for which this would help?

* Are you trying to debug kernel code, or the firmware?

* Are you trying to debug a functional issue, or get some statistics for
  optimization?

I'll also note that if you really want to, you can place a kprobe or kretprobe
around the invocation and trace this without any kernel changes. So if you just
need some data right now, that should be sufficient.

Thanks,
Mark.

> Thanks,
> Mark.
> 
> > ---
> >  drivers/firmware/smccc/Makefile      |  3 +-
> >  drivers/firmware/smccc/smccc.c       | 14 ++++++++++
> >  drivers/firmware/smccc/smccc_trace.c |  7 +++++  
> > drivers/firmware/smccc/smccc_trace.h | 41 ++++++++++++++++++++++++++++
> >  include/linux/arm-smccc.h            |  4 ++-
> >  kernel/time/timekeeping.c            |  7 +++++
> >  6 files changed, 74 insertions(+), 2 deletions(-)  create mode 100644 
> > drivers/firmware/smccc/smccc_trace.c
> >  create mode 100644 drivers/firmware/smccc/smccc_trace.h
> > 
> > diff --git a/drivers/firmware/smccc/Makefile 
> > b/drivers/firmware/smccc/Makefile index 40d19144a860..e74c35191b49 
> > 100644
> > --- a/drivers/firmware/smccc/Makefile
> > +++ b/drivers/firmware/smccc/Makefile
> > @@ -1,4 +1,5 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  #
> > -obj-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)	+= smccc.o kvm_guest.o
> > +ccflags-y += -I$(src)
> > +obj-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)	+= smccc_trace.o smccc.o kvm_guest.o
> >  obj-$(CONFIG_ARM_SMCCC_SOC_ID)	+= soc_id.o
> > diff --git a/drivers/firmware/smccc/smccc.c 
> > b/drivers/firmware/smccc/smccc.c index 60ccf3e90d7d..7631a16479e9 
> > 100644
> > --- a/drivers/firmware/smccc/smccc.c
> > +++ b/drivers/firmware/smccc/smccc.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/kernel.h>
> >  #include <linux/platform_device.h>
> >  #include <asm/archrandom.h>
> > +#include "smccc_trace.h"
> >  
> >  static u32 smccc_version = ARM_SMCCC_VERSION_1_0;  static enum 
> > arm_smccc_conduit smccc_conduit = SMCCC_CONDUIT_NONE; @@ -59,3 +60,16 
> > @@ static int __init smccc_devices_init(void)
> >  	return 0;
> >  }
> >  device_initcall(smccc_devices_init);
> > +
> > +void arm_smccc_smc(unsigned long a0, unsigned long a1, unsigned long a2, unsigned long a3,
> > +		   unsigned long a4, unsigned long a5, unsigned long a6, unsigned long a7,
> > +		   struct arm_smccc_res *res)
> > +{
> > +	u64 start, elapsed;
> > +
> > +	trace_arm_smccc_smc_start(a0);
> > +	start = ktime_get_ns();
> > +	__arm_smccc_smc(a0, a1, a2, a3, a4, a5, a6, a7, res, NULL);
> > +	elapsed = ktime_get_ns() - start;
> > +	trace_arm_smccc_smc_end(a0, elapsed); }
> > diff --git a/drivers/firmware/smccc/smccc_trace.c 
> > b/drivers/firmware/smccc/smccc_trace.c
> > new file mode 100644
> > index 000000000000..6b94d5d9c0f4
> > --- /dev/null
> > +++ b/drivers/firmware/smccc/smccc_trace.c
> > @@ -0,0 +1,7 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#define CREATE_TRACE_POINTS
> > +#include "smccc_trace.h"
> > +
> > +EXPORT_TRACEPOINT_SYMBOL(arm_smccc_smc_start);
> > +EXPORT_TRACEPOINT_SYMBOL(arm_smccc_smc_end);
> > diff --git a/drivers/firmware/smccc/smccc_trace.h 
> > b/drivers/firmware/smccc/smccc_trace.h
> > new file mode 100644
> > index 000000000000..c0ef836bc093
> > --- /dev/null
> > +++ b/drivers/firmware/smccc/smccc_trace.h
> > @@ -0,0 +1,41 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#undef TRACE_SYSTEM
> > +#define TRACE_SYSTEM smccc
> > +
> > +#if !defined(__SMCCC_TRACE_H) || defined(TRACE_HEADER_MULTI_READ) 
> > +#define __SMCCC_TRACE_H
> > +
> > +#include <linux/types.h>
> > +#include <linux/tracepoint.h>
> > +
> > +TRACE_EVENT(arm_smccc_smc_start,
> > +	    TP_PROTO(unsigned long smc_id),
> > +	    TP_ARGS(smc_id),
> > +	    TP_STRUCT__entry(__field(unsigned long, smc_id)),
> > +	    TP_fast_assign(__entry->smc_id = smc_id;),
> > +	    TP_printk("SMC ID: 0x%lx", __entry->smc_id) );
> > +
> > +TRACE_EVENT(arm_smccc_smc_end,
> > +	    TP_PROTO(unsigned long smc_id, u64 elapsed_time),
> > +	    TP_ARGS(smc_id, elapsed_time),
> > +	    TP_STRUCT__entry(__field(unsigned long, smc_id)
> > +			     __field(u64, elapsed_time)
> > +	    ),
> > +	    TP_fast_assign(__entry->smc_id = smc_id;
> > +			   __entry->elapsed_time = elapsed_time;
> > +	    ),
> > +	    TP_printk("SMC ID: 0x%lx time taken to process : %llu ns",
> > +		      __entry->smc_id, __entry->elapsed_time) );
> > +
> > +#endif /* __SMCCC_TRACE_H */
> > +
> > +#undef TRACE_INCLUDE_PATH
> > +#define TRACE_INCLUDE_PATH .
> > +
> > +#undef TRACE_INCLUDE_FILE
> > +#define TRACE_INCLUDE_FILE smccc_trace
> > +
> > +#include <trace/define_trace.h>
> > diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h 
> > index 220c8c60e021..39588c3db486 100644
> > --- a/include/linux/arm-smccc.h
> > +++ b/include/linux/arm-smccc.h
> > @@ -358,7 +358,9 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
> >  			unsigned long a5, unsigned long a6, unsigned long a7,
> >  			struct arm_smccc_res *res, struct arm_smccc_quirk *quirk);
> >  
> > -#define arm_smccc_smc(...) __arm_smccc_smc(__VA_ARGS__, NULL)
> > +void arm_smccc_smc(unsigned long a0, unsigned long a1, unsigned long a2,
> > +		   unsigned long a3, unsigned long a4, unsigned long a5,
> > +		   unsigned long a6, unsigned long a7, struct arm_smccc_res *res);
> >  
> >  #define arm_smccc_smc_quirk(...) __arm_smccc_smc(__VA_ARGS__)
> >  
> > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c 
> > index 5579ead449f2..3a3bf2d674a3 100644
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> > @@ -192,6 +192,13 @@ static inline u64 tk_clock_read(const struct 
> > tk_read_base *tkr)  {
> >  	struct clocksource *clock = READ_ONCE(tkr->clock);
> >  
> > +	/* At the time of kernel booting some SMC calls are called before the
> > +	 * clock is initialized, in such cases it would lead to kernel crash.
> > +	 * To prevent kernel crash in such cases this check is included.
> > +	 */
> > +	if (unlikely(!clock))
> > +		return 0;
> > +
> >  	return clock->read(clock);
> >  }
> >  
> > --
> > 2.17.1
> > 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-03-08 13:59 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-04 12:58 [PATCH] arm64: smccc: Add trace events to SMC calls Rakesh Babu Saladi
2023-03-04 17:38 ` kernel test robot
2023-03-04 17:38 ` kernel test robot
2023-03-06 14:47 ` Mark Rutland
2023-03-08 12:25   ` [EXT] " Rakesh Babu Saladi
2023-03-08 13:42     ` Sudeep Holla
2023-03-08 13:58     ` Mark Rutland

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).