CoreSight

coresight@lists.linaro.org

7 participants
1953 discussions

Re: [PATCH v7 08/10] coresight-tpdm: Add timestamp control register support for the CMB

by Suzuki K Poulose

On 13/02/2024 10:45, Arnd Bergmann wrote: > On Tue, Feb 13, 2024, at 11:22, Suzuki K Poulose wrote: > >> coresight: tpdm: Fix build break due to uninitialised field >> >> {CMB/DSB}_PATT_ENABLE_TS attributes do not use an index field. >> But some compilers complain for uninitialised "index" field in >> the struct tpdm_dataset_attribute: >> > ... >> >> Reported-by: Arnd Bergmann <arnd(a)arndb.de> >> Cc: Tao Zhang <quic_taozha(a)quicinc.com> >> Signed-off-by: Suzuki K Poulose <suzuki.poulose(a)arm.com> > > Works fine here with this, thanks for the fix! > > Acked-by: Arnd Bergmann <arnd(a)arndb.de> > > I had actually missed that this is a warning option that > is normally disabled even at W=1 level, but it does get > added by drivers/hwtracing/coresight/Makefile setting > -Wextra again because that comes after the > -Wno-missing-field-initializers from scripts/Makefile.extrawarn. > > Arnd Thanks, I will queue this. Suzuki

3 months

Re: [PATCH v7 08/10] coresight-tpdm: Add timestamp control register support for the CMB

by Suzuki K Poulose

n 13/02/2024 09:50, Arnd Bergmann wrote: > On Sun, Feb 4, 2024, at 06:30, Tao Zhang wrote: > >> @@ -910,7 +1014,7 @@ static struct attribute *tpdm_dsb_patt_attrs[] = { >> DSB_PATT_MASK_ATTR(5), >> DSB_PATT_MASK_ATTR(6), >> DSB_PATT_MASK_ATTR(7), >> - &dev_attr_enable_ts.attr, >> + DSB_PATT_ENABLE_TS, >> &dev_attr_set_type.attr, >> NULL, >> }; >> @@ -964,6 +1068,7 @@ static struct attribute *tpdm_cmb_patt_attrs[] = { >> CMB_PATT_ATTR(1), >> CMB_PATT_MASK_ATTR(0), >> CMB_PATT_MASK_ATTR(1), >> + CMB_PATT_ENABLE_TS, >> NULL, >> }; >> >> @@ -158,6 +175,10 @@ >> tpdm_simple_dataset_rw(tpmr##nr, \ >> DSB_PATT_MASK, nr) >> >> +#define DSB_PATT_ENABLE_TS \ >> + tpdm_patt_enable_ts(enable_ts, \ >> + DSB_PATT) >> + >> #define DSB_MSR_ATTR(nr) \ >> tpdm_simple_dataset_rw(msr##nr, \ >> DSB_MSR, nr) > > This is causing build failures in linux-next now: > > drivers/hwtracing/coresight/coresight-tpdm.c:1055:2: error: missing field 'idx' initializer [-Werror,-Wmissing-field-initializers] > 1055 | DSB_PATT_ENABLE_TS, > | ^ > drivers/hwtracing/coresight/coresight-tpdm.h:184:3: note: expanded from macro 'DSB_PATT_ENABLE_TS' > 184 | tpdm_patt_enable_ts(enable_ts, \ > | ^ > drivers/hwtracing/coresight/coresight-tpdm.h:156:5: note: expanded from macro 'tpdm_patt_enable_ts' > 156 | } \ > | ^ > drivers/hwtracing/coresight/coresight-tpdm.c:1109:2: error: missing field 'idx' initializer [-Werror,-Wmissing-field-initializers] > 1109 | CMB_PATT_ENABLE_TS, > | ^ > drivers/hwtracing/coresight/coresight-tpdm.h:208:3: note: expanded from macro 'CMB_PATT_ENABLE_TS' > 208 | tpdm_patt_enable_ts(enable_ts, \ > | ^ > drivers/hwtracing/coresight/coresight-tpdm.h:156:5: note: expanded from macro 'tpdm_patt_enable_ts' > 156 | } \ > | ^ > > Not sure what is going on, so I reverted your patch locally > for my test setup. > Can you send a fix to make it build again? The idx field is not used for the {DSB,CMB}_PATT_ENABLE_TS attributes and as such the macros do not set it. May be something like this fixes the problem ? But any uninitiliased fields must be set to 0 implicitly ? --->8--- coresight: tpdm: Fix build break due to uninitialised field {CMB/DSB}_PATT_ENABLE_TS attributes do not use an index field. But some compilers complain for uninitialised "index" field in the struct tpdm_dataset_attribute: drivers/hwtracing/coresight/coresight-tpdm.c:1055:2: error: missing field 'idx' initializer [-Werror,-Wmissing-field-initializers] 1055 | DSB_PATT_ENABLE_TS, | ^ drivers/hwtracing/coresight/coresight-tpdm.h:184:3: note: expanded from macro 'DSB_PATT_ENABLE_TS' 184 | tpdm_patt_enable_ts(enable_ts, \ | ^ drivers/hwtracing/coresight/coresight-tpdm.h:156:5: note: expanded from macro 'tpdm_patt_enable_ts' 156 | } \ | ^ drivers/hwtracing/coresight/coresight-tpdm.c:1109:2: error: missing field 'idx' initializer [-Werror,-Wmissing-field-initializers] 1109 | CMB_PATT_ENABLE_TS, | ^ drivers/hwtracing/coresight/coresight-tpdm.h:208:3: note: expanded from macro 'CMB_PATT_ENABLE_TS' 208 | tpdm_patt_enable_ts(enable_ts, \ | ^ drivers/hwtracing/coresight/coresight-tpdm.h:156:5: note: expanded from macro 'tpdm_patt_enable_ts' 156 | } \ | ^ Reported-by: Arnd Bergmann <arnd(a)arndb.de> Cc: Tao Zhang <quic_taozha(a)quicinc.com> Signed-off-by: Suzuki K Poulose <suzuki.poulose(a)arm.com> --- drivers/hwtracing/coresight/coresight-tpdm.h | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h index f3a8f56d0fe7..e08d212642e3 100644 --- a/drivers/hwtracing/coresight/coresight-tpdm.h +++ b/drivers/hwtracing/coresight/coresight-tpdm.h @@ -153,6 +153,7 @@ __ATTR(name, 0644, enable_ts_show, \ enable_ts_store), \ mem, \ + 0, \ } \ })[0].attr.attr) -- 2.34.1

3 months

Re: [PATCH] coresight: make coresight_bustype const

by Suzuki K Poulose

On Fri, 5 Jan 2024 13:33:32 +0100, Greg Kroah-Hartman wrote: > Now that the driver core can properly handle constant struct bus_type, > move the coresight_bustype variable to be a constant structure as well, > placing it into read-only memory which can not be modified at runtime. > > Moved to next, thanks! [1/1] coresight: make coresight_bustype const https://git.kernel.org/coresight/c/dd95255d44c0 Best regards, -- Suzuki K Poulose <suzuki.poulose(a)arm.com>

3 months

[PATCH v2 00/12] coresight: Separate sysfs and Perf usage and some other cleanups

by James Clark

Changes since V1: * Clarify further "the selected sink" in _coresight_build_path() * Move etm4x's mode to coresight device which was missing from V1 * Use explicit initialisers in coresight_dev_type * Create functions for handling mode changes I've been finding it quite difficult to reason about some of the state and functions in coresight-core.c because they have generic names when they are actually only relevant to the sysfs usage of Coresight rather than usage through Perf. This is probably because sysfs came first and Perf was added later. This has caused a couple of issues where these things have been used in the wrong context, for example the first commit is a fixup. To fix this I've mainly just moved all of the sysfs stuff to the sysfs file and removed the 'enable' state, which was just for sysfs. While doing the refactor it became obvious that refcnt didn't need to be atomic either, so that can be simplified along with some other comment clarifications and simplifications. Hopefully it's also a step towards to removing all of the duplicate refcnt and mode tracking code from the individual devices. That tracking pretty much always results in a one-shot enable/disable and fixes the mode to either sysfs or Perf, and there is no reason that can't exist in the core layer outside of the devices. I tried to finish that in this set, but there turned out to be some complexities, so I cut it short at a point where I can be sure that there are no behavioral changes. James Clark (12): coresight: Fix issue where a source device's helpers aren't disabled coresight: Make language around "activated" sinks consistent coresight: Remove ops callback checks coresight: Move mode to struct coresight_device coresight: Remove the 'enable' field. coresight: Move all sysfs code to sysfs file coresight: Remove atomic type from refcnt coresight: Remove unused stubs coresight: Add explicit member initializers to coresight_dev_type coresight: Add helper for atomically taking the device coresight: Add a helper for getting csdev->mode coresight: Add helper for setting csdev->mode drivers/hwtracing/coresight/coresight-core.c | 494 +----------------- drivers/hwtracing/coresight/coresight-etb10.c | 29 +- .../hwtracing/coresight/coresight-etm-perf.c | 2 +- drivers/hwtracing/coresight/coresight-etm.h | 2 - .../coresight/coresight-etm3x-core.c | 23 +- .../coresight/coresight-etm3x-sysfs.c | 4 +- .../coresight/coresight-etm4x-core.c | 26 +- drivers/hwtracing/coresight/coresight-etm4x.h | 1 - drivers/hwtracing/coresight/coresight-priv.h | 9 +- drivers/hwtracing/coresight/coresight-stm.c | 30 +- drivers/hwtracing/coresight/coresight-sysfs.c | 391 ++++++++++++++ .../hwtracing/coresight/coresight-tmc-core.c | 2 +- .../hwtracing/coresight/coresight-tmc-etf.c | 46 +- .../hwtracing/coresight/coresight-tmc-etr.c | 33 +- drivers/hwtracing/coresight/coresight-tmc.h | 2 - drivers/hwtracing/coresight/coresight-tpda.c | 13 +- drivers/hwtracing/coresight/coresight-tpiu.c | 14 +- drivers/hwtracing/coresight/ultrasoc-smb.c | 22 +- drivers/hwtracing/coresight/ultrasoc-smb.h | 2 - include/linux/coresight.h | 146 ++---- 20 files changed, 606 insertions(+), 685 deletions(-) -- 2.34.1

3 months

[PATCH v3 0/4] coresight: Make current W=1 warnings default

by James Clark

Changes since v2: * Split code and Makefile changes * Add a fix for a comment warning in etm3x * Add some sparse checker fixes in coresight-cfg-afdo.c * Fix the 0 instead of NULL warnings reported by kernel test robot James Clark (4): coresight: Fix uninitialized struct warnings coresight: etm3x: Fix build warning coresight: Fix undeclared variable warnings from sparse checker coresight: Make current W=1 warnings default drivers/hwtracing/coresight/Makefile | 20 +++++++++++++++++++ .../hwtracing/coresight/coresight-cfg-afdo.c | 1 + .../hwtracing/coresight/coresight-cti-core.c | 2 +- drivers/hwtracing/coresight/coresight-etb10.c | 2 +- .../coresight/coresight-etm3x-core.c | 4 ++-- .../coresight/coresight-etm4x-core.c | 2 +- .../hwtracing/coresight/coresight-funnel.c | 4 ++-- .../coresight/coresight-replicator.c | 2 +- drivers/hwtracing/coresight/coresight-stm.c | 2 +- .../hwtracing/coresight/coresight-tmc-core.c | 2 +- drivers/hwtracing/coresight/coresight-tpda.c | 2 +- drivers/hwtracing/coresight/coresight-tpdm.c | 2 +- drivers/hwtracing/coresight/coresight-tpiu.c | 2 +- drivers/hwtracing/coresight/ultrasoc-smb.c | 2 +- 14 files changed, 35 insertions(+), 14 deletions(-) -- 2.34.1

3 months

[PATCH v7 00/10] Add support to configure TPDM CMB subunit

by Tao Zhang

Introduction of TPDM CMB(Continuous Multi Bit) subunit CMB subunit is responsible for creating a dataset element, and is also optionally responsible for packing it to fit multiple elements on a single ATB transfer if possible in the configuration. The TPDM Core Datapath requests timestamps be stored by the TPDA and then delivering ATB sized data (depending on ATB width and element size, this could be smaller or larger than a dataset element) to the ATB Mast FSM. The CMB makes trace elements in two modes. In �continuous� mode, every valid data cycle creates an element. In �trace on change� mode, when valid data changes on the bus, a trace element is created. In continuous mode, all cycles where this condition is true create trace elements. In trace on change mode, a data element is only when the previously sampled input is different from the current sampled input. The CMB subunit must be configured prior to enablement. This series adds support for TPDM to configure the configure CMB subunit. Once this series patches are applied properly, the new tpdm nodes for should be observed at the tpdm path /sys/bus/coresight/devices/tpdm* which supports CMB subunit. e.g. root@qemuarm64:/sys/devices/platform/soc@0/684c000.tpdm/tpdm0# ls -l -rw-r--r-- 1 root root 4096 Jan 1 00:00 cmb_mode drwxr-xr-x 2 root root 0 Jan 1 00:00 cmb_msr drwxr-xr-x 2 root root 0 Jan 1 00:00 cmb_patt drwxr-xr-x 2 root root 0 Jan 1 00:00 cmb_trig_patt -rw-r--r-- 1 root root 4096 Jan 1 00:00 cmb_trig_ts -rw-r--r-- 1 root root 4096 Jan 1 00:00 cmb_ts_all drwxr-xr-x 2 root root 0 Jan 1 00:00 connections drwxr-xr-x 2 root root 0 Jan 1 00:00 dsb_edge drwxr-xr-x 2 root root 0 Jan 1 00:00 dsb_msr drwxr-xr-x 2 root root 0 Jan 1 00:00 dsb_patt drwxr-xr-x 2 root root 0 Jan 1 00:00 dsb_trig_patt -rw-r--r-- 1 root root 4096 Jan 1 00:00 enable_source --w------- 1 root root 4096 Jan 1 00:00 integration_test drwxr-xr-x 2 root root 0 Ja? 1 00:00 power --w------- 1 root root 4096 Jan 1 00:00 reset_dataset lrwxrwxrwx 1 root root 0 Apr 5 2021 subsystem -> ../../../../../bus/coresight -rw-r--r-- 1 root root 4096 Apr 5 2021 uevent -r--r--r-- 1 root root 4096 Jan 1 00:00 waiting_for_supplier We can use the commands are similar to the below to configure the TPDMs which support CMB subunit. Enable coresight sink first. echo 1 > /sys/bus/coresight/devices/tmc_etf0/enable_sink echo 1 > /sys/bus/coresight/devices/tpdm0/reset_dataset echo 1 > /sys/bus/coresight/devices/tpdm0/cmb_mode echo 1 > /sys/bus/coresight/devices/tpdm0/cmb_patt/enable_ts echo 0xFFFFFFFF > /sys/bus/coresight/devices/tpdm0/cmb_patt/tpmr0 echo 0 > /sys/bus/coresight/devices/tpdm0/cmb_trig_ts echo 0xFFFFFFFF > /sys/bus/coresight/devices/tpdm0/cmb_trig_patt/xpr1 echo 1 > /sys/bus/coresight/devices/tpdm0/enable_source codelinaro link: https://git.codelinaro.org/clo/linux-kernel/coresight/-/commits/tpdm-cmb-v7 Changes in V7: 1. Rename "tpdm_clear_element_size" to "tpda_clear_element_size" in the patch#5. -- Suzuki K Poulose 2. Remove the stale comments in "tpda_enable_port". -- Suzuki K Poulose Changes in V6: 1. Clear all the relevant fields before setting them in the function "tpda_set_element_size". In patch#5. -- Suzuki K Poulose 2. Leave a space after/before '/*' & '*/'. In patch#7 #8. -- Suzuki K Poulose Changes in V5: 1. Return directly to refine the process of "tpdm_enable_dsb" and "tpdm_disable_dsb" if the TPDM does not support DSB dataset. -- Suzuki K Poulose 2. Change the new property name from "qcom,cmb-element-size" to "qcom,cmb-element-bits". And update the property name to the new tpdm example which supports the CMB dataset. -- Krzysztof 3. Drop unnecessary () around the drvdata member access in patch#5. -- Suzuki K Poulose 4. Replace the error code "-EEXIST" with the port number in detecting multiple TPDMs on one input port. In patch#5. -- Suzuki K Poulose 5. Due to the modification of the new property name, the searched property name was changed from "qcom,cmb-element-size" to "qcom,cmb-element-bits". In patch#5. 6. Replace the judgement "tpdm_has_cmb_dataset(drvdata)" with "drvdata->cmb" in the function "tpdm_reset_datasets". In patch#6. -- Suzuki K Poulose 7. Drop unnecessary () around kstrtoul() in the function "cmb_mode_store". In patch#6. -- Suzuki K Poulose 8. Update the date and the kernel version in the Document. -- Suzuki K Poulose 9. Initialize the return value to "-EINVAL" and remove the unnecessary code in the function "enable_ts_show". In patch#8. -- Suzuki K Poulose 10. Replace spin lock/unlock with guard(spinlock). In patch#8. -- Suzuki K Poulose 11. Drop _rw from the macro "tpdm_patt_enable_ts_rw". In patch#8. -- Suzuki K Poulose Changes in V4: 1. Replace spin lock/unlock to avoid forgetting to unlock when the function exits. -- Suzuki K Poulose 2. Move the helper "tpdm_has_dsb_dataset" to the header file. -- Suzuki K Poulose 3. Fix the incorrect property of the sample in the documents. -- James Clark 4. Clear the dsb/cmb element size directly in the clear helper. -- Suzuki K Poulose 5. Correct the comment of "tpdm_read_element_size". -- Suzuki K Poulose 6. Call the helper "tpdm_has_dsb/cmb_dataset" in TPDA driver to check what dataset the TPDM supports. -- Suzuki K Poulose 7. Refine the dsb/cmb dataset support check in enable/disable functions. -- Suzuki K Poulose 8. Get rid of redundant code in function "set_cmb_tier". -- Suzuki K Poulose 9. Since one SysFs file should follow "one value", use "dev_ext_attribute" to instead of the previous "enable_ts" Sysfs file approach. -- Suzuki K Poulose 10. Change the kernel version to 6.9 for the MSR related SysFs file. -- James Clark 11. Refine the function "tpdm_simple_dataset_store". -- Suzuki K Poulose Changes in V3: 1. Add 8-bit support to the description in the TPDM devicetree document. -- Rob Herring 2. Change how the result is produced in "tpdm_read_element_size". -- James Clark 3. Calling "tpdm_clear_element_size" at the beginning of "tpda_enable_port". -- James Clark 4. Use "dsb_esize" and "cmb_esize" to determine whether multiple TPDMs are detected on a TPDA input port in "tpda_get_element_size". -- James Clark 5. Modify the judgment logic in "tpda_enable_port". -- James Clark 6. Add more description of "cmb_mode" to TPDM SysFS document. -- James Clark Changes in V2: 1. Optimizate and modify this patch series based on the patch series "Add support to configure TPDM CMB subunit". 2. Modify the functions that read the element size of DSB/CMB in TPDA driver. Tao Zhang (10): coresight-tpdm: Optimize the store function of tpdm simple dataset coresight-tpdm: Optimize the useage of tpdm_has_dsb_dataset dt-bindings: arm: qcom,coresight-tpdm: Add support for CMB element size coresight-tpdm: Add CMB dataset support coresight-tpda: Add support to configure CMB element coresight-tpdm: Add support to configure CMB coresight-tpdm: Add pattern registers support for CMB coresight-tpdm: Add timestamp control register support for the CMB dt-bindings: arm: qcom,coresight-tpdm: Add support for TPDM CMB MSR register coresight-tpdm: Add msr register support for CMB .../testing/sysfs-bus-coresight-devices-tpdm | 87 ++++ .../bindings/arm/qcom,coresight-tpdm.yaml | 35 ++ drivers/hwtracing/coresight/coresight-tpda.c | 130 ++--- drivers/hwtracing/coresight/coresight-tpda.h | 6 + drivers/hwtracing/coresight/coresight-tpdm.c | 455 ++++++++++++++++-- drivers/hwtracing/coresight/coresight-tpdm.h | 113 +++++ 6 files changed, 729 insertions(+), 97 deletions(-) -- 2.17.1

3 months

[PATCH] coresight: etm4x: Set skip_power_up in etm4_init_arch_data function

by Mao Jinlong

skip_power_up is used in etm4_init_arch_data when set lpoverride. So need to set the value of it before calling using it. Signed-off-by: Mao Jinlong <quic_jinlmao(a)quicinc.com> --- drivers/hwtracing/coresight/coresight-etm4x-core.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index c5ea808ea662..8afc07d1dd23 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -1200,6 +1200,7 @@ static void etm4_init_arch_data(void *info) struct etm4_init_arg *init_arg = info; struct etmv4_drvdata *drvdata; struct csdev_access *csa; + struct device *dev = init_arg->dev; int i; drvdata = dev_get_drvdata(init_arg->dev); @@ -1213,6 +1214,10 @@ static void etm4_init_arch_data(void *info) if (!etm4_init_csdev_access(drvdata, csa)) return; + if (!csa->io_mem || + fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up")) + drvdata->skip_power_up = true; + /* Detect the support for OS Lock before we actually use it */ etm_detect_os_lock(drvdata, csa); @@ -2040,11 +2045,6 @@ static int etm4_add_coresight_dev(struct etm4_init_arg *init_arg) if (!drvdata->arch) return -EINVAL; - /* TRCPDCR is not accessible with system instructions. */ - if (!desc.access.io_mem || - fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up")) - drvdata->skip_power_up = true; - major = ETM_ARCH_MAJOR_VERSION(drvdata->arch); minor = ETM_ARCH_MINOR_VERSION(drvdata->arch); -- 2.41.0

3 months

Re: [PATCH v4 2/7] arm64: KVM: Use shared area to pass PMU event state to hypervisor

by James Clark

On 05/02/2024 14:52, Marc Zyngier wrote: > On Mon, 05 Feb 2024 14:17:10 +0000, > James Clark <james.clark(a)arm.com> wrote: >> >> On 05/02/2024 13:21, Oliver Upton wrote: >>> On Mon, Feb 05, 2024 at 01:15:36PM +0000, Marc Zyngier wrote: >>>> On Mon, 05 Feb 2024 13:04:51 +0000, >>>> Oliver Upton <oliver.upton(a)linux.dev> wrote: >>>>> >>>>> Unless someone has strong opinions about making this work in protected >>>>> mode, I am happy to see tracing support limited to the 'normal' nVHE >>>>> configuration. The protected feature as a whole is just baggage until >>>>> upstream support is completed. >>>> >>>> Limiting tracing to non-protected mode is a must IMO. Allowing tracing >>>> when pKVM is enabled is a sure way to expose secrets that should >>>> stay... secret. The only exception I can think of is when >>>> CONFIG_NVHE_EL2_DEBUG is enabled, at which point all bets are off. >>> >>> Zero argument there :) I left off the "and PMU" part of what I was >>> saying, because that was a feature that semi-worked in protected mode >>> before VM/VCPU shadowing support landed. >>> >> >> In that case I can hide all this behind CONFIG_NVHE_EL2_DEBUG for pKVM. >> This will also have the effect of disabling PMU again for pKVM because I >> moved that into this new shared area. > > I'm not sure what you have in mind, but dropping PMU support for > non-protected guests when protected-mode is enabled is not an > acceptable outcome. > > Hiding the trace behind a debug option is fine as this is a global > setting that has no userspace impact, but impacting guests isn't. > > M. > Hmmm in that case if there's currently no way to distinguish between normal VMs and pVMs in protected-mode then what I was thinking of probably won't work. I'll actually just leave PMU as it is and only have tracing disabled in protected-mode. My only question now is whether to: * Keep this new shared area and use it for both PMU and trace status (well, for PMU only in protected mode as trace would always be disabled and doesn't actually need any state) * Delete patch 2, add a new normal per-cpu struct just for trace status that's only used in non-protected mode and revert to copying the PMU status into the vCPU on guest switch as it was previously.

3 months, 1 week

Re: [PATCH v4 2/7] arm64: KVM: Use shared area to pass PMU event state to hypervisor

by James Clark

On 05/02/2024 13:21, Oliver Upton wrote: > On Mon, Feb 05, 2024 at 01:15:36PM +0000, Marc Zyngier wrote: >> On Mon, 05 Feb 2024 13:04:51 +0000, >> Oliver Upton <oliver.upton(a)linux.dev> wrote: >>> >>> Unless someone has strong opinions about making this work in protected >>> mode, I am happy to see tracing support limited to the 'normal' nVHE >>> configuration. The protected feature as a whole is just baggage until >>> upstream support is completed. >> >> Limiting tracing to non-protected mode is a must IMO. Allowing tracing >> when pKVM is enabled is a sure way to expose secrets that should >> stay... secret. The only exception I can think of is when >> CONFIG_NVHE_EL2_DEBUG is enabled, at which point all bets are off. > > Zero argument there :) I left off the "and PMU" part of what I was > saying, because that was a feature that semi-worked in protected mode > before VM/VCPU shadowing support landed. > In that case I can hide all this behind CONFIG_NVHE_EL2_DEBUG for pKVM. This will also have the effect of disabling PMU again for pKVM because I moved that into this new shared area. The same place will be used to store the state for normal nVHE and at least then there is some code re-use and flexibility to use trace and PMU for debugging if needed. And the copy on every switch gets deleted.

3 months, 1 week

Re: [PATCH v4 2/7] arm64: KVM: Use shared area to pass PMU event state to hypervisor

by James Clark

On 02/02/2024 22:00, Oliver Upton wrote: > On Thu, Jan 04, 2024 at 04:27:02PM +0000, James Clark wrote: > > [...] > >> diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c >> index c50f8459e4fc..89147a9dc38c 100644 >> --- a/arch/arm64/kvm/hyp/nvhe/switch.c >> +++ b/arch/arm64/kvm/hyp/nvhe/switch.c >> @@ -130,13 +130,18 @@ static void __hyp_vgic_restore_state(struct kvm_vcpu *vcpu) >> } >> } >> >> +static struct kvm_pmu_events *kvm_nvhe_get_pmu_events(struct kvm_vcpu *vcpu) >> +{ >> + return &kvm_host_global_state[vcpu->cpu].pmu_events; >> +} >> + >> /* >> * Disable host events, enable guest events >> */ >> #ifdef CONFIG_HW_PERF_EVENTS >> static bool __pmu_switch_to_guest(struct kvm_vcpu *vcpu) >> { >> - struct kvm_pmu_events *pmu = &vcpu->arch.pmu.events; >> + struct kvm_pmu_events *pmu = kvm_nvhe_get_pmu_events(vcpu); >> >> if (pmu->events_host) >> write_sysreg(pmu->events_host, pmcntenclr_el0); >> @@ -152,7 +157,7 @@ static bool __pmu_switch_to_guest(struct kvm_vcpu *vcpu) >> */ >> static void __pmu_switch_to_host(struct kvm_vcpu *vcpu) >> { >> - struct kvm_pmu_events *pmu = &vcpu->arch.pmu.events; >> + struct kvm_pmu_events *pmu = kvm_nvhe_get_pmu_events(vcpu); >> >> if (pmu->events_guest) >> write_sysreg(pmu->events_guest, pmcntenclr_el0); > > This now allows the host to program event counters for a protected > guest. That _might_ be a useful feature behind some debug option, but is > most definitely *not* something we want to do for pVMs generally. Unless I'm missing something, using PMUs on protected guests was added by 722625c6f4c5b ("KVM: arm64: Reenable pmu in Protected Mode"). This change is just a refactor that will allow us to add the same behavior for a similar feature (tracing) without adding yet another copy of some state before the guest switch. > > Do we even need to make this shared data work at all for pKVM? The rest > of the shared data between pKVM and the kernel is system information, > which (importantly) doesn't have any guest context in it. > Probably not, Marc actually mentioned on one of the first versions of that this could be hidden behind a debug flag. To be honest one of the reasons I didn't do that was because I wasn't sure what the appropriate debug setting was. NVHE_EL2_DEBUG didn't seem quite right. DEBUG_KERNEL maybe? Or a new one? And then I suppose I got distracted by trying to make it have feature parity with PMUs and forgot about the debug only thing. > I'm perfectly happy leaving these sorts of features broken for pKVM and > using the 'normal' way of getting percpu data to the nVHE hypervisor > otherwise. > I can do that. But do I also disable PMU at the same time in a new commit? Now that both PMU and tracing is working maybe it would be a waste to throw that away and hiding it behind an option is better. Or I can leave the PMU as it is and just keep tracing disabled in pKVM. I don't mind either way, my main goal was to get exclude/include guest tracing working for normal VMs. For pKVM I don't have a strong opinion.

3 months, 1 week

← Newer
1
2
3
4
5
6
7
8
9
...
196
Older →

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight