From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:45120)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <peter.maydell@linaro.org>) id 1aSnlz-0005bd-N9
	for qemu-devel@nongnu.org; Mon, 08 Feb 2016 10:30:44 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <peter.maydell@linaro.org>) id 1aSnly-0007YR-SA
	for qemu-devel@nongnu.org; Mon, 08 Feb 2016 10:30:43 -0500
Received: from mail-vk0-x22d.google.com ([2607:f8b0:400c:c05::22d]:36024)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <peter.maydell@linaro.org>) id 1aSnlx-0007YB-Kq
	for qemu-devel@nongnu.org; Mon, 08 Feb 2016 10:30:42 -0500
Received: by mail-vk0-x22d.google.com with SMTP id c3so50876176vkb.3
	for <qemu-devel@nongnu.org>; Mon, 08 Feb 2016 07:30:41 -0800 (PST)
MIME-Version: 1.0
In-Reply-To: <56B8B366.1040203@gmail.com>
References: <1454690704-16233-1-git-send-email-peter.maydell@linaro.org>
	<1454690704-16233-2-git-send-email-peter.maydell@linaro.org>
	<56B8B366.1040203@gmail.com>
From: Peter Maydell <peter.maydell@linaro.org>
Date: Mon, 8 Feb 2016 15:30:21 +0000
Message-ID: <CAFEAcA_t77uSZrT7fCvAKNeSPbJDyF8om8eMvHW7woXEvMcrvQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [Qemu-devel] [PATCH 1/6] target-arm: correct CNTFRQ access
	rights
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Sergey Fedorov <serge.fdrv@gmail.com>
Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, qemu-arm <qemu-arm@nongnu.org>, QEMU Developers <qemu-devel@nongnu.org>, Patch Tracking <patches@linaro.org>

On 8 February 2016 at 15:25, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> On 05.02.2016 19:44, Peter Maydell wrote:
>> Correct some corner cases we were getting wrong for
>> CNTFRQ access rights:
>>  * should UNDEF from 32-bit Secure EL1
>>  * only writable from the highest implemented exception level,
>>    which might not be EL1 now

>> +    switch (arm_current_el(env)) {
>> +    case 0:
>> +        if (!extract32(env->cp15.c14_cntkctl, 0, 2)) {
>> +            return CP_ACCESS_TRAP;
>> +        }
>> +        /* EL0 reads are forbidden by the .access fields */
>
> s/reads/writes/ ?

Yes.

>> +        break;
>> +    case 1:
>> +        if (!isread && (arm_feature(env, ARM_FEATURE_EL2)
>> +                        || arm_feature(env, ARM_FEATURE_EL3))) {
>> +            return CP_ACCESS_TRAP_UNCATEGORIZED;
>> +        }
>> +        if (!isread && ri->state == ARM_CP_STATE_AA32 &&
>> +            arm_is_secure_below_el3(env)) {
>> +            /* Accesses from 32-bit Secure EL1 UNDEF (*not* trap to EL3!) */
>> +            return CP_ACCESS_TRAP_UNCATEGORIZED;
>> +        }
>> +        break;
>> +    case 2:
>> +        if (!isread && arm_feature(env, ARM_FEATURE_EL3)) {
>> +            return CP_ACCESS_TRAP_UNCATEGORIZED;
>> +        }
>> +        break;
>> +    case 3:
>> +        break;
>>      }
>>      return CP_ACCESS_OK;
>>  }
>
> Maybe calculating "the highest implemented exception level" could
> simplify reading of the code a bit? E.g.:
>
>     int highest_el = arm_feature(env, ARM_FEATURE_EL3) ? 3 :
>                      arm_feature(env, ARM_FEATURE_EL2) ? 2 : 1;
>
> We would probably want to have a dedicated static inline function for
> this similar to HighestEL() from ARMv8 ARM pseudocode.

Mmm, that might look neater. I'll have a play with the code.

thanks
-- PMM