From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5E8AAC2D0E2 for ; Tue, 22 Sep 2020 12:49:31 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id B92E72395C for ; Tue, 22 Sep 2020 12:49:30 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="anTryI2S"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="pKqvZ2Yb" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B92E72395C Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:To:Subject:Message-ID:Date:From:In-Reply-To: References:MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=WIzWiYbbfDEnr4ZCCRYQN9CSCH37AAwNEGKMoW6d+Eo=; b=anTryI2SA/pW9kEkXCJ7/bQJi JINo4O4hYNzxyER4dubNiQXycvX2LwD+Kqstn/sOEbIi48VphucynxUk9KVDMDReqnwDKfVoJ6gFT wj0J2ToAivvqvakMdFS/WQq/kyg6tYJlRdrp9oxK3jwQeIV7nKIvnCfU3EZm0VN5ppeGPQk3ZGpBC 5ETf4qJdC5yROKZEPUKnZMFDkKZe50aqf9B9KcRtBCI7mzHcs6/vq6Y9NkTqQOpKfjzdmrK6RqeAo eGH9XnF9Cd1aBWc1SEHWpPymmay5huIQEY8n1sa8wcYwYebTKWo9g1b+uzh/aByJ6WawvyImezvQL Gevn3zWUA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kKhiA-0008Px-17; Tue, 22 Sep 2020 12:47:58 +0000 Received: from mail-ed1-x542.google.com ([2a00:1450:4864:20::542]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kKhi7-0008P7-Ca for linux-arm-kernel@lists.infradead.org; Tue, 22 Sep 2020 12:47:56 +0000 Received: by mail-ed1-x542.google.com with SMTP id t16so16041187edw.7 for ; Tue, 22 Sep 2020 05:47:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=tWR1oCkPYMQedUsxUI94T0F8nayvp4Fn7O40hQ5RvK4=; b=pKqvZ2YbTalt1k9RvEsecQ2BGA5wm5pZ6LuV4zWQflSXEXI0+oF1602ANHIhUWIdvn LV+qzn32GlNQamxpJl+N25XhG2oFvs5fZp+A+QAnEcfi8eUmtYyI2bug0XjyBNwSHo4Z /vq7UF1Y5e2EwicE6xIkz6YWz0BXT/HTw1HckbJPP5OfJvTZC9alXkIB7A5Mug6W1K56 1u2+OviasY+x9Ti/ptdVzCZ+77LXy4OCsJ2QAZE71XowrLC2bzYQ3Sjl72LFLdqZnqgu ELJKpbkwOzh8wLd4vnwkVvAnj6Iwl3gsk0pVXVqQ55sLWFA4Sf85R5Ms0gle1t3est3U pVmQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=tWR1oCkPYMQedUsxUI94T0F8nayvp4Fn7O40hQ5RvK4=; b=t/27MCWSkYG/9Barw1SwoatLWLbXMP7wo8m4H5cO/yrlil2skpbWUV4Z4RwXXRsY8L jZY70swVALEZFcOGvnIHQrO6iVgoAwPosCDrPvY1EkLEzuBZo/YUXlFz3+VVVIvEjGpi fnafypStO8WyoYmImvY3PvKse4j0zW1OP76LPZCwaHabRlq+hjpD/kujyoltE72Zl9zU UlvA3MR0uBlpPuUXigD0UEkX76zNESCll7nlebDXSEmdxmCpK8r48Mhp3TKoMXYRBzOb 6uIbt6u2b5kIMqpfB8H5R0zr9ly9RAMVrwXq0kOBPdl5vY69Mngo/y2ucmjbc5OYLcmK ukrg== X-Gm-Message-State: AOAM530LK7DCtZd6WNAovFdtb5RVhE3W6EHKbfJ2Y57q/xGkt1dgPtks W9kN8ftZA2wTVBmtxCx2uWW38cpSwgPlbwcLTrtfQQ== X-Google-Smtp-Source: ABdhPJxFByu0lTJR0OVCeRvNVtTDlbi1ksh4BAGHhfZswrRXaH0uVOWEua7FyaDlS9hVrqQ+0e/e8eTzYXnsJEJmvlg= X-Received: by 2002:a05:6402:176c:: with SMTP id da12mr3805915edb.386.1600778872116; Tue, 22 Sep 2020 05:47:52 -0700 (PDT) MIME-Version: 1.0 References: <20200911084119.1080694-1-suzuki.poulose@arm.com> <20200911084119.1080694-13-suzuki.poulose@arm.com> <22c9fccf-6d87-5d9f-465f-0ef487984f0d@arm.com> In-Reply-To: <22c9fccf-6d87-5d9f-465f-0ef487984f0d@arm.com> From: Mike Leach Date: Tue, 22 Sep 2020 13:47:41 +0100 Message-ID: Subject: Re: [PATCH 12/19] coresight: etm4x: Cleanup secure exception level masks To: Suzuki K Poulose X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200922_084755_569409_A116EAC1 X-CRM114-Status: GOOD ( 36.55 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Coresight ML , anshuman.khandual@arm.com, Mathieu Poirier , linux-arm-kernel , Leo Yan Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Suzuki, On Tue, 22 Sep 2020 at 11:51, Suzuki K Poulose wrote: > > On 09/18/2020 04:35 PM, Mike Leach wrote: > > On Fri, 11 Sep 2020 at 09:41, Suzuki K Poulose wrote: > >> > >> We rely on the ETM architecture version to decide whether > >> Secure EL2 is available on the CPU for excluding the level > >> for address comparators and viewinst main control register. > >> We must instead use the TRCDIDR3.EXLEVEL_S field to detect > >> the supported levels. > >> > >> Signed-off-by: Suzuki K Poulose > >> --- > >> drivers/hwtracing/coresight/coresight-etm4x.c | 13 +++---------- > >> drivers/hwtracing/coresight/coresight-etm4x.h | 6 ++++-- > >> 2 files changed, 7 insertions(+), 12 deletions(-) > >> > >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c > >> index 7feb57108bdc..439e9da41006 100644 > >> --- a/drivers/hwtracing/coresight/coresight-etm4x.c > >> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c > >> @@ -758,7 +758,6 @@ static void etm4_init_arch_data(void *info) > >> * TRCARCHMAJ, bits[11:8] architecture major versin number > >> */ > >> drvdata->arch = BMVAL(etmidr1, 4, 11); > >> - drvdata->config.arch = drvdata->arch; > >> > >> /* maximum size of resources */ > >> etmidr2 = etm4x_relaxed_read32(&csa, TRCIDR2); > >> @@ -774,6 +773,7 @@ static void etm4_init_arch_data(void *info) > >> drvdata->ccitmin = BMVAL(etmidr3, 0, 11); > >> /* EXLEVEL_S, bits[19:16] Secure state instruction tracing */ > >> drvdata->s_ex_level = BMVAL(etmidr3, 16, 19); > >> + drvdata->config.s_ex_level = drvdata->s_ex_level; > >> /* EXLEVEL_NS, bits[23:20] Non-secure state instruction tracing */ > >> drvdata->ns_ex_level = BMVAL(etmidr3, 20, 23); > >> > >> @@ -935,16 +935,9 @@ static u64 etm4_get_ns_access_type(struct etmv4_config *config) > >> static u64 etm4_get_access_type(struct etmv4_config *config) > >> { > >> u64 access_type = etm4_get_ns_access_type(config); > >> - u64 s_hyp = (config->arch & 0x0f) >= 0x4 ? ETM_EXLEVEL_S_HYP : 0; > >> > >> - /* > >> - * EXLEVEL_S, bits[11:8], don't trace anything happening > >> - * in secure state. > >> - */ > >> - access_type |= (ETM_EXLEVEL_S_APP | > >> - ETM_EXLEVEL_S_OS | > >> - s_hyp | > >> - ETM_EXLEVEL_S_MON); > >> + /* All supported secure ELs are excluded */ > >> + access_type |= (u64)config->s_ex_level << TRCACATR_EXLEVEL_SHIFT; > >> > > > > What is the << TRCACATR_EXLEVEL_SHIFT doing here? > > The config->s_ex_level is the EXLVEL mask from the TRCIDR3 shifted to bit 0, as above. > We need to make sure that we use the mask in the correct position for TRCACATR register. > Basically, we simply exclude all the secure levels supported by the ETM. > Sorry, should have been a little more explicit. This breaks the next patch! > > > >> return access_type; > >> } > >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h > >> index efd903688edd..407ad6647f36 100644 > >> --- a/drivers/hwtracing/coresight/coresight-etm4x.h > >> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h > >> @@ -522,6 +522,8 @@ > >> /* PowerDown Control Register bits */ > >> #define TRCPDCR_PU BIT(3) > >> > >> +#define TRCACATR_EXLEVEL_SHIFT 8 > >> + > >> /* secure state access levels - TRCACATRn */ > >> #define ETM_EXLEVEL_S_APP BIT(8) > >> #define ETM_EXLEVEL_S_OS BIT(9) > >> @@ -604,7 +606,7 @@ > >> * @vmid_mask0: VM ID comparator mask for comparator 0-3. > >> * @vmid_mask1: VM ID comparator mask for comparator 4-7. > >> * @ext_inp: External input selection. > >> - * @arch: ETM architecture version (for arch dependent config). > >> + * @s_ex_level: Secure ELs where tracing is supported. > >> */ > >> struct etmv4_config { > >> u32 mode; > >> @@ -648,7 +650,7 @@ struct etmv4_config { > >> u32 vmid_mask0; > >> u32 vmid_mask1; > >> u32 ext_inp; > >> - u8 arch; > >> + u8 s_ex_level; > >> }; > >> > >> /** > >> -- > >> 2.24.1 > >> > > > > Perhaps this patch could be combined with the next patch as it > > operates on the same set of flags. > > > > I agree that they both deal with the same set of masks. However, functionally > they have separate purposes. > > 1) This patch disconnects the usage of drvdata->arch field to determine > the secure exception level mask. This is more of a correctness, as a given > v4.4 implementation may not have a Secure EL2. > > 2) The next patch cleans up the way we define and use all the exception level > masks, both secure and non-secure. > Applying this and the next patch which moves to the bits being indexed from 0 for a common reusable field gets you the following code sequence:- static u64 etm4_get_ns_access_type(struct etmv4_config *config) { u64 access_type = 0; /* * EXLEVEL_NS, bits[15:12] * The Exception levels are: * Bit[12] Exception level 0 - Application * Bit[13] Exception level 1 - OS * Bit[14] Exception level 2 - Hypervisor * Bit[15] Never implemented */ [ MJL: at this point the comment is true to an extent but no longer applies to the values #defines below - which have been shifted by the change to the #defines to form a field that is indexed from bit 0] if (!is_kernel_in_hyp_mode()) { /* Stay away from hypervisor mode for non-VHE */ access_type = ETM_EXLEVEL_NS_HYP; if (config->mode & ETM_MODE_EXCL_KERN) access_type |= ETM_EXLEVEL_NS_OS; } else if (config->mode & ETM_MODE_EXCL_KERN) { access_type = ETM_EXLEVEL_NS_HYP; } if (config->mode & ETM_MODE_EXCL_USER) access_type |= ETM_EXLEVEL_NS_APP; return access_type; } static u64 etm4_get_access_type(struct etmv4_config *config) { u64 access_type = etm4_get_ns_access_type(config); /* All supported secure ELs are excluded */ access_type |= (u64)config->s_ex_level << TRCACATR_EXLEVEL_SHIFT; [MJL: Now we are OR ing a 0 bit index based field (NS access type) with another 0 index based field - but shifting it for some reason?] return access_type; } static u64 etm4_get_comparator_access_type(struct etmv4_config *config) { return etm4_get_access_type(config) << TRCACATR_EXLEVEL_SHIFT; [MJL: resulting in TRCACATR_EXLEVEL_SHIFT being applied twice.] } Hence my assertion that this should really be one patch, otherwise you have to add the shift in one patch and correct the problem in the second. Regards Mike > I understand that the subjects are quite similar. I could try to fix the > subject for this one to make (1) more explicit. This way it is more easier > to review. > > Suzuki -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel