From patchwork Fri Oct 24 22:43:47 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Greg Bellows X-Patchwork-Id: 39498 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-wg0-f69.google.com (mail-wg0-f69.google.com [74.125.82.69]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id 4804024026 for ; Fri, 24 Oct 2014 22:44:47 +0000 (UTC) Received: by mail-wg0-f69.google.com with SMTP id b13sf1135885wgh.0 for ; Fri, 24 Oct 2014 15:44:46 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:mime-version:in-reply-to:references :date:message-id:from:to:cc:subject:precedence:list-id :list-unsubscribe:list-archive:list-post:list-help:list-subscribe :errors-to:sender:x-original-sender :x-original-authentication-results:mailing-list:content-type; bh=mhIjeFyEA2k2YpLU1dIAu3L4lu0EyuOw1JVo6A8/YCQ=; b=B2XTLVmx5y6z/ng3lUnwNhp9DFpk3XSrxDrbEftNniSbuRCkg2nAr7cjelAsb063OX Ev8Y26+qfnmzHgGu2Iugo97BSUUMuXEptAGC5Db/KGzHhPhWJ6hfWxzTw07vcl4D5WfO 31PxjFrUNlnUXWUnuFlgzdisPik6HpW7tnQ1fSTrZFEkDCqUzBiNFF5FHUj48qQaBKh5 3T09vCBrGQa9U2WDUWtife55gODAZWZgQHuh9XxtrWe67vjBsTChWSd7I0A1Ycexw48M Z/iAFIoGj8KYrv3CPvAj5NHaf6BpVF5hStyvwK4BAqXwxLzDS8kiCjsdl6cyFZYstPbw 1wlg== X-Gm-Message-State: ALoCoQm2yqWVHmop1tFQKNKnOZy8iV4oxoVYBrKAsvE/Fx073bdsf0sw26pzvIa6ZUqPkSbL7cdI X-Received: by 10.112.168.40 with SMTP id zt8mr2954485lbb.0.1414190686383; Fri, 24 Oct 2014 15:44:46 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.152.87.4 with SMTP id t4ls502728laz.33.gmail; Fri, 24 Oct 2014 15:44:46 -0700 (PDT) X-Received: by 10.112.182.1 with SMTP id ea1mr7317584lbc.16.1414190686209; Fri, 24 Oct 2014 15:44:46 -0700 (PDT) Received: from mail-lb0-f172.google.com (mail-lb0-f172.google.com. [209.85.217.172]) by mx.google.com with ESMTPS id b2si8799731lbm.104.2014.10.24.15.44.45 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Fri, 24 Oct 2014 15:44:45 -0700 (PDT) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.217.172 as permitted sender) client-ip=209.85.217.172; Received: by mail-lb0-f172.google.com with SMTP id b6so3300344lbj.31 for ; Fri, 24 Oct 2014 15:44:45 -0700 (PDT) X-Received: by 10.152.7.7 with SMTP id f7mr7205294laa.57.1414190685285; Fri, 24 Oct 2014 15:44:45 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.112.84.229 with SMTP id c5csp514421lbz; Fri, 24 Oct 2014 15:44:44 -0700 (PDT) X-Received: by 10.224.135.196 with SMTP id o4mr10301938qat.35.1414190683424; Fri, 24 Oct 2014 15:44:43 -0700 (PDT) Received: from lists.gnu.org (lists.gnu.org. [2001:4830:134:3::11]) by mx.google.com with ESMTPS id d49si10047014qga.71.2014.10.24.15.44.42 for (version=TLSv1 cipher=RC4-SHA bits=128/128); Fri, 24 Oct 2014 15:44:43 -0700 (PDT) Received-SPF: pass (google.com: domain of qemu-devel-bounces+patch=linaro.org@nongnu.org designates 2001:4830:134:3::11 as permitted sender) client-ip=2001:4830:134:3::11; Received: from localhost ([::1]:51133 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xhnb7-0006I2-P2 for patch@linaro.org; Fri, 24 Oct 2014 18:44:41 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59084) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XhnaM-0005rc-IH for qemu-devel@nongnu.org; Fri, 24 Oct 2014 18:43:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XhnaH-0004hD-Aj for qemu-devel@nongnu.org; Fri, 24 Oct 2014 18:43:54 -0400 Received: from mail-qa0-f54.google.com ([209.85.216.54]:65454) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XhnaH-0004h8-1z for qemu-devel@nongnu.org; Fri, 24 Oct 2014 18:43:49 -0400 Received: by mail-qa0-f54.google.com with SMTP id u7so236185qaz.27 for ; Fri, 24 Oct 2014 15:43:48 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.224.26.5 with SMTP id b5mr10645339qac.12.1414190627949; Fri, 24 Oct 2014 15:43:47 -0700 (PDT) Received: by 10.96.136.168 with HTTP; Fri, 24 Oct 2014 15:43:47 -0700 (PDT) In-Reply-To: References: <1413910544-20150-1-git-send-email-greg.bellows@linaro.org> <1413910544-20150-8-git-send-email-greg.bellows@linaro.org> Date: Fri, 24 Oct 2014 17:43:47 -0500 Message-ID: From: Greg Bellows To: Peter Maydell X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 209.85.216.54 Cc: Sergey Fedorov , QEMU Developers , Fabian Aggeler , "Edgar E. Iglesias" Subject: Re: [Qemu-devel] [PATCH v7 07/32] target-arm: extend async excp masking X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: , List-Help: , List-Subscribe: , Errors-To: qemu-devel-bounces+patch=linaro.org@nongnu.org Sender: qemu-devel-bounces+patch=linaro.org@nongnu.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: greg.bellows@linaro.org X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.217.172 as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 Hi Peter, Based on our discussion, I looked into a table lookup approach to replace the arm_phys_excp_target_el() as we discussed. I have something working but still need to verify it is 100% correct. Before I went much further, I thought I'd share my findings. In order to do the table in a way that it does not blow-up with a bunch of duplicate data, we still need a function for accessing the table. The function will still have a good part of what the existing arm_phys_excp_target_el() has at the beginning. This is necessary as we need to decide which of the SCR and HCR bits need to be plugged into the table. In addition, we need the following: - The table has to include special values to account for the cases where an interrupt is not taken so we will need logic on the other end to catch the special table value and return cur_el. - Either another dimension needs to be added to the table or a second table to handle the differences between 32/64-bit EL3. (Still needed) - Another dimension is needed to include HCR_TGE eventually. Basically, I'm not sure that it buys us much other than a static table. Otherwise, we are swapping one bunch of logic for a different set. Below are the changes (convert to a fixed font for better table format): Greg */ @@ -4035,69 +4073,28 @@ static inline uint32_t arm_phys_excp_target_el(CPUState *cs, ui uint32_t cur_el, bool secure) { CPUARMState *env = cs->env_ptr; - uint32_t target_el = 1; - - /* There is no SCR or HCR routing unless the respective EL3 and EL2 - * extensions are supported. This initial setting affects whether any - * other conditions matter. - */ - bool scr_routing = arm_feature(env, ARM_FEATURE_EL3); /* IRQ, FIQ, EA */ - bool hcr_routing = arm_feature(env, ARM_FEATURE_EL2); /* IMO, FMO, AMO */ - - /* Fast-path if EL2 and EL3 are not enabled */ - if (!scr_routing && !hcr_routing) { - return target_el; - } + uint32_t rw = ((env->cp15.scr_el3 & SCR_RW) == SCR_RW); + uint32_t scr; + uint32_t hcr; + uint32_t target_el; switch (excp_idx) { case EXCP_IRQ: - scr_routing &= ((env->cp15.scr_el3 & SCR_IRQ) == SCR_IRQ); - hcr_routing &= ((env->cp15.hcr_el2 & HCR_IMO) == HCR_IMO); + scr = ((env->cp15.scr_el3 & SCR_IRQ) == SCR_IRQ); + hcr = ((env->cp15.hcr_el2 & HCR_IMO) == HCR_IMO); break; case EXCP_FIQ: - scr_routing &= ((env->cp15.scr_el3 & SCR_FIQ) == SCR_FIQ); - hcr_routing &= ((env->cp15.hcr_el2 & HCR_FMO) == HCR_FMO); - } - - /* If SCR routing is enabled we always go to EL3 regardless of EL3 - * execution state - */ - if (scr_routing) { - /* IRQ|FIQ|EA == 1 */ - return 3; - } - - /* If HCR.TGE is set all exceptions that would be routed to EL1 are - * routed to EL2 (in non-secure world). - */ - hcr_routing &= (env->cp15.hcr_el2 & HCR_TGE) == HCR_TGE; + scr = ((env->cp15.scr_el3 & SCR_FIQ) == SCR_FIQ); + hcr = ((env->cp15.hcr_el2 & HCR_FMO) == HCR_FMO); + break; + default: + scr = ((env->cp15.scr_el3 & SCR_EA) == SCR_EA); + hcr = ((env->cp15.hcr_el2 & HCR_AMO) == HCR_AMO); + break; + }; - /* Determine target EL according to ARM ARMv8 tables G1-15 and G1-16 */ - if (arm_el_is_aa64(env, 3)) { - /* EL3 in AArch64 */ - if (!secure) { - /* If non-secure, we may route to EL2 depending on other state. - * If we are coming from the secure world then we always route to - * EL1. - */ - if (hcr_routing || - (cur_el == 2 && !(env->cp15.scr_el3 & SCR_RW))) { - /* If HCR.FMO/IMO is set or we already in EL2 and it is not - * configured to be AArch64 then route to EL2. - */ - target_el = 2; - } - } - } else { - /* EL3 in AArch32 */ - if (secure) { - /* If coming from secure always route to EL3 */ - target_el = 3; - } else if (hcr_routing || cur_el == 2) { - /* If HCR.FMO/IMO is set or we are already EL2 then route to EL2 */ - target_el = 2; - } - } + target_el = aarch64_target_el[scr][rw][hcr][!secure][cur_el]; + target_el = (target_el > 3) ? cur_el : target_el; return target_el; } On 24 October 2014 11:25, Peter Maydell wrote: > On 21 October 2014 17:55, Greg Bellows wrote: > > From: Fabian Aggeler > > > > This patch extends arm_excp_unmasked() according to ARM ARMv7 and > > ARM ARMv8 (all EL running in AArch32) and adds comments. > > > > If EL3 is using AArch64 IRQ/FIQ masking is ignored in > > all exception levels other than EL3 if SCR.{FIQ|IRQ} is > > set to 1 (routed to EL3). > > > > Signed-off-by: Fabian Aggeler > > Signed-off-by: Greg Bellows > > > > ========== > > > > v5 -> v6 > > - Globally change Aarch# to AArch# > > - Fixed comment termination > > > > v4 -> v5 > > - Merge with v4 patch 10 > > > > Signed-off-by: Greg Bellows > > --- > > target-arm/cpu.h | 117 > ++++++++++++++++++++++++++++++++++++++++++++++++++----- > > 1 file changed, 107 insertions(+), 10 deletions(-) > > > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > > index cb6ec5c..1a564b9 100644 > > --- a/target-arm/cpu.h > > +++ b/target-arm/cpu.h > > @@ -1246,11 +1246,8 @@ static inline bool arm_excp_unmasked(CPUState > *cs, unsigned int excp_idx) > > { > > CPUARMState *env = cs->env_ptr; > > unsigned int cur_el = arm_current_el(env); > > - unsigned int target_el = arm_excp_target_el(cs, excp_idx); > > - /* FIXME: Use actual secure state. */ > > - bool secure = false; > > - /* If in EL1/0, Physical IRQ routing to EL2 only happens from NS > state. */ > > - bool irq_can_hyp = !secure && cur_el < 2 && target_el == 2; > > + bool secure = arm_is_secure(env); > > + > > /* ARMv7-M interrupt return works by loading a magic value > > * into the PC. On real hardware the load causes the > > * return to occur. The qemu implementation performs the > > @@ -1265,19 +1262,119 @@ static inline bool arm_excp_unmasked(CPUState > *cs, unsigned int excp_idx) > > && (!IS_M(env) || env->regs[15] < 0xfffffff0); > > > > /* Don't take exceptions if they target a lower EL. */ > > - if (cur_el > target_el) { > > + if (cur_el > arm_excp_target_el(cs, excp_idx)) { > > return false; > > } > > > > + /* ARM ARMv7 B1.8.6 Asynchronous exception masking (table > B1-12/B1-13) > > + * ARM ARMv8 G1.11.3 Asynchronous exception masking controls > > + * (table G1-18/G1-19) > > + */ > > switch (excp_idx) { > > case EXCP_FIQ: > > - if (irq_can_hyp && (env->cp15.hcr_el2 & HCR_FMO)) { > > - return true; > > + if (arm_feature(env, ARM_FEATURE_EL3) && arm_el_is_aa64(env, > 3)) { > > + /* If EL3 is using AArch64 and FIQs are routed to EL3 > masking is > > + * ignored in all exception levels except EL3. > > + */ > > + if ((env->cp15.scr_el3 & SCR_FIQ) && cur_el < 3) { > > Why are we recalculating whether the target level is EL3 by looking > at SCR_EL3.SCR_FIQ, rather than using the target_el which > arm_excp_target_el() returns? > > > + return true; > > + } > > + /* If we are in EL3 but FIQs are not routed to EL3 the > exception > > + * is not taken but remains pending. > > + */ > > + if (!(env->cp15.scr_el3 & SCR_FIQ) && cur_el == 3) { > > + return false; > > Isn't this unreachable? If SCR_FIQ is clear then arm_excp_target_el() > will have returned either 1 or 2, and so we'll have returned false > due to the check on cur_el earlier. > > > + } > > + } > > + if (!secure) { > > + if (arm_feature(env, ARM_FEATURE_EL2)) { > > + if (env->cp15.hcr_el2 & HCR_FMO) { > > + /* CPSR.F/PSTATE.F ignored if > > + * - exception is taken from Non-secure state > > + * - HCR.FMO == 1 > > + * - either: - not in Hyp mode > > + * - SCR.FIQ routes exception to > monitor mode > > + * (EL3 in AArch32) > > + */ > > + if (cur_el < 2) { > > + return true; > > + } else if (arm_feature(env, ARM_FEATURE_EL3) && > > + (env->cp15.scr_el3 & SCR_FIQ) && > > + !arm_el_is_aa64(env, 3)) { > > + return true; > > + } > > + } else if (arm_el_is_aa64(env, 3) && > > + (env->cp15.scr_el3 & SCR_RW) && > > + cur_el == 2) { > > + /* FIQs not routed to EL2 but currently in EL2 > (A64). > > + * Exception is not taken but remains pending. */ > > + return false; > > + } > > + } > > + /* In ARMv7 only applies if both Security Extensions (EL3) > and > > + * Hypervirtualization Extensions (EL2) implemented, while > > + * for ARMv8 it applies also if only EL3 implemented. > > + */ > > + if (arm_feature(env, ARM_FEATURE_EL3) && > > + (arm_feature(env, ARM_FEATURE_EL2) || > > + arm_feature(env, ARM_FEATURE_V8))) { > > + /* CPSR.F/PSTATE.F ignored if > > + * - exception is taken from Non-secure state > > + * - SCR.FIQ routes exception to monitor mode > > + * - SCR.FW bit is set to 0 > > + * - HCR.FMO == 0 (if EL2 implemented) > > + */ > > + if ((env->cp15.scr_el3 & SCR_FIQ) && > > + !(env->cp15.scr_el3 & SCR_FW)) { > > Something odd here -- in ARMv8 SCR_EL3 bit 4 is RES1, so this test > should never pass -- either this check is wrong or the check on > ARM_FEATURE_V8 in the outer if() is wrong, presumably. > > > + if (!arm_feature(env, ARM_FEATURE_EL2)) { > > + return true; > > + } else if (!(env->cp15.hcr_el2 & HCR_FMO)) { > > + return true; > > + } > > + } > > + } > > } > > return !(env->daif & PSTATE_F); > > case EXCP_IRQ: > > - if (irq_can_hyp && (env->cp15.hcr_el2 & HCR_IMO)) { > > - return true; > > + if (arm_feature(env, ARM_FEATURE_EL3) && arm_el_is_aa64(env, > 3)) { > > + /* If EL3 is using AArch64 and IRQs are routed to EL3 > masking is > > + * ignored in all exception levels except EL3. > > + */ > > + if ((env->cp15.scr_el3 & SCR_IRQ) && cur_el < 3) { > > + return true; > > + } > > + /* If we are in EL3 but IRQ s are not routed to EL3 the > exception > > + * is not taken but remains pending. > > + */ > > + if (!(env->cp15.scr_el3 & SCR_IRQ) && cur_el == 3) { > > + return false; > > + } > > + } > > + if (!secure) { > > + if (arm_feature(env, ARM_FEATURE_EL2)) { > > + if (env->cp15.hcr_el2 & HCR_IMO) { > > + /* CPSR.I/PSTATE.I ignored if > > + * - exception is taken from Non-secure state > > + * - HCR.IMO == 1 > > + * - either: - not in Hyp mode > > + * - SCR.IRQ routes exception to > monitor mode > > + * (EL3 in AArch32) > > + */ > > + if (cur_el < 2) { > > + return true; > > + } else if (arm_feature(env, ARM_FEATURE_EL3) && > > + (env->cp15.scr_el3 & SCR_IRQ) && > > + !arm_el_is_aa64(env, 3)) { > > + return true; > > + } > > + } else if (arm_el_is_aa64(env, 3) && > > + (env->cp15.scr_el3 & SCR_RW) && > > + cur_el == 2) { > > + /* IRQs not routed to EL2 but currently in EL2 > (A64). > > + * Exception is not taken but remains pending. */ > > + return false; > > + } > > + } > > } > > return irq_unmasked; > > case EXCP_VFIQ: > > -- > > 1.8.3.2 > > I have to say I find this set of nested conditionals pretty confusing, > and hard to relate to the tables in the ARM ARM. Maybe it would be better > if we actually had a set of data tables in our implementation which > we used to look up whether the exception should be always taken, > remain pending, or honour the PSTATE mask flag ? > > I think it would also be good if our implementation tried to keep > the same separation of routing [ie "which exception level is this > exception going to go to?"] and masking [ie "do we take this exception > at this time?"] which the ARM ARM has. At the moment we sort of > have that in the arm_excp_target_el() and this function, but a lot > of the code here seems to be repeating bits of the routing calculation. > > thanks > -- PMM > ============================= diff --git a/target-arm/helper.c b/target-arm/helper.c index b4db1a5..fd3d637 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -4028,6 +4028,44 @@ void switch_mode(CPUARMState *env, int mode) env->spsr = env->banked_spsr[i]; } +const unsigned int aarch64_target_el[2][2][2][2][4] = { + /* Physical Interrupt Target EL Lookup Table + * + * [ From ARM ARM section D1.14.1 (Table D1-11) ] + * + * The below multi-dimensional table is used for looking up the target + * exception level given numerous condition criteria. Specifically, the + * target EL is based on SCR and HCR routing controls as well as the + * currently executing EL and secure state. + * + * The table values are as such: + * 0-3 = EL0-EL3 + * 8 = Cannot occur + * 9 = Interrupt not taken + * + * SCR HCR + * EA AMO + * IRQ IMO FROM + * FIQ RW FMO NS EL0 EL1 EL2 EL3 + */ + {{{{ /* 0 0 0 0 */ 1, 1, 8, 9 }, + { /* 0 0 0 1 */ 1, 1, 2, 8 },}, + {{ /* 0 0 1 0 */ 1, 1, 8, 9 }, + { /* 0 0 1 1 */ 2, 2, 2, 8 },},}, + {{{ /* 0 1 0 0 */ 1, 1, 8, 9 }, + { /* 0 1 0 1 */ 1, 1, 8, 8 },}, + {{ /* 0 1 1 0 */ 1, 1, 8, 9 }, + { /* 0 1 1 1 */ 2, 2, 2, 8 },},},}, + {{{{ /* 1 0 0 0 */ 3, 3, 8, 3 }, + { /* 1 0 0 1 */ 3, 3, 3, 8 },}, + {{ /* 1 0 1 0 */ 3, 3, 8, 3 }, + { /* 1 0 1 1 */ 3, 3, 3, 8 },},}, + {{{ /* 1 1 0 0 */ 3, 3, 8, 3 }, + { /* 1 1 0 1 */ 3, 3, 3, 8 },}, + {{ /* 1 1 1 0 */ 3, 3, 8, 3 }, + { /* 1 1 1 1 */ 3, 3, 3, 8 },},},}, +}; + /* * Determine the target EL for physical exceptions