* [Qemu-devel] [PATCH] target-arm: implement CPACR register logic
@ 2014-05-16 10:01 Fabian Aggeler
2014-05-16 17:14 ` Peter Crosthwaite
2014-05-16 17:55 ` Peter Maydell
0 siblings, 2 replies; 3+ messages in thread
From: Fabian Aggeler @ 2014-05-16 10:01 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, peter.crosthwaite, greg.bellows
From: Sergey Fedorov <s.fedorov@samsung.com>
CPACR register allows to control access rights to coprocessor 0-13
interfaces. Bits corresponding to unimplemented coprocessors should be
RAZ/WI. QEMU implements only VFP coprocessor on ARMv6+ targets. So only
cp10 & cp11 bits are writable.
Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
---
This patch was previously part of the Security Extensions patchset and
got separated because it is not Sec-Ext specific.
In addition to a style fix of the comment I added qemu_log_mask() as
suggested.
target-arm/helper.c | 6 ++++++
target-arm/translate.c | 29 ++++++++++++++++++++++++++---
2 files changed, 32 insertions(+), 3 deletions(-)
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 417161e..3cb1ac0 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -477,6 +477,12 @@ static const ARMCPRegInfo not_v7_cp_reginfo[] = {
static void cpacr_write(CPUARMState *env, const ARMCPRegInfo *ri,
uint64_t value)
{
+ uint32_t mask = 0;
+
+ if (arm_feature(env, ARM_FEATURE_VFP)) {
+ mask |= 0x00f00000; /* VFP coprocessor: cp10 & cp11 */
+ }
+ value &= mask;
if (env->cp15.c1_coproc != value) {
env->cp15.c1_coproc = value;
/* ??? Is this safe when called from within a TB? */
diff --git a/target-arm/translate.c b/target-arm/translate.c
index a4d920b..9b298d0 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -6864,9 +6864,32 @@ static int disas_coproc_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
const ARMCPRegInfo *ri;
cpnum = (insn >> 8) & 0xf;
- if (arm_feature(env, ARM_FEATURE_XSCALE)
- && ((env->cp15.c15_cpar ^ 0x3fff) & (1 << cpnum)))
- return 1;
+ if (cpnum < 14) {
+ if (arm_feature(env, ARM_FEATURE_XSCALE)) {
+ if (~env->cp15.c15_cpar & (1 << cpnum)) {
+ return 1;
+ }
+ } else {
+ /* Bits [20:21] of CPACR control access to cp10
+ * Bits [23:22] of CPACR control access to cp11
+ */
+ switch ((env->cp15.c1_coproc >> (cpnum * 2)) & 3) {
+ case 0: /* access denied */
+ return 1;
+ case 1: /* privileged mode access only */
+ if (IS_USER(s)) {
+ return 1;
+ }
+ break;
+ case 2: /* reserved */
+ qemu_log_mask(LOG_GUEST_ERROR, "CPACR bits for cp%d set to "
+ "0b10 (unpredictable)\n", cpnum);
+ return 1;
+ case 3: /* privileged and user mode access */
+ break;
+ }
+ }
+ }
/* First check for coprocessor space used for actual instructions */
switch (cpnum) {
--
1.8.3.2
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [Qemu-devel] [PATCH] target-arm: implement CPACR register logic
2014-05-16 10:01 [Qemu-devel] [PATCH] target-arm: implement CPACR register logic Fabian Aggeler
@ 2014-05-16 17:14 ` Peter Crosthwaite
2014-05-16 17:55 ` Peter Maydell
1 sibling, 0 replies; 3+ messages in thread
From: Peter Crosthwaite @ 2014-05-16 17:14 UTC (permalink / raw)
To: Fabian Aggeler
Cc: Peter Maydell, qemu-devel@nongnu.org Developers, Greg Bellows
On Fri, May 16, 2014 at 10:01 AM, Fabian Aggeler <aggelerf@ethz.ch> wrote:
> From: Sergey Fedorov <s.fedorov@samsung.com>
>
> CPACR register allows to control access rights to coprocessor 0-13
> interfaces. Bits corresponding to unimplemented coprocessors should be
> RAZ/WI. QEMU implements only VFP coprocessor on ARMv6+ targets. So only
> cp10 & cp11 bits are writable.
>
> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> ---
>
> This patch was previously part of the Security Extensions patchset and
> got separated because it is not Sec-Ext specific.
> In addition to a style fix of the comment I added qemu_log_mask() as
> suggested.
>
> target-arm/helper.c | 6 ++++++
> target-arm/translate.c | 29 ++++++++++++++++++++++++++---
> 2 files changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 417161e..3cb1ac0 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -477,6 +477,12 @@ static const ARMCPRegInfo not_v7_cp_reginfo[] = {
> static void cpacr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> uint64_t value)
> {
> + uint32_t mask = 0;
> +
> + if (arm_feature(env, ARM_FEATURE_VFP)) {
> + mask |= 0x00f00000; /* VFP coprocessor: cp10 & cp11 */
> + }
> + value &= mask;
> if (env->cp15.c1_coproc != value) {
> env->cp15.c1_coproc = value;
> /* ??? Is this safe when called from within a TB? */
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index a4d920b..9b298d0 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -6864,9 +6864,32 @@ static int disas_coproc_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
> const ARMCPRegInfo *ri;
>
> cpnum = (insn >> 8) & 0xf;
> - if (arm_feature(env, ARM_FEATURE_XSCALE)
> - && ((env->cp15.c15_cpar ^ 0x3fff) & (1 << cpnum)))
> - return 1;
> + if (cpnum < 14) {
> + if (arm_feature(env, ARM_FEATURE_XSCALE)) {
> + if (~env->cp15.c15_cpar & (1 << cpnum)) {
> + return 1;
> + }
> + } else {
> + /* Bits [20:21] of CPACR control access to cp10
> + * Bits [23:22] of CPACR control access to cp11
> + */
Thinking about this a little more, there's nothing CP10/CP11 specific
about this logic. The comment should be more general than this (or
dropped). It would go stale if someone patched in a new CP. Perhaps
add a "..." to indicate that you are illustrating a repeating pattern
+ /* Bits [20:21] of CPACR control access to cp10
+ * Bits [23:22] of CPACR control access to cp11
+ * ...
+ */
With droppage or comment generalisation:
Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> + switch ((env->cp15.c1_coproc >> (cpnum * 2)) & 3) {
> + case 0: /* access denied */
> + return 1;
> + case 1: /* privileged mode access only */
> + if (IS_USER(s)) {
> + return 1;
> + }
> + break;
> + case 2: /* reserved */
> + qemu_log_mask(LOG_GUEST_ERROR, "CPACR bits for cp%d set to "
> + "0b10 (unpredictable)\n", cpnum);
> + return 1;
> + case 3: /* privileged and user mode access */
> + break;
> + }
> + }
> + }
>
> /* First check for coprocessor space used for actual instructions */
> switch (cpnum) {
> --
> 1.8.3.2
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [Qemu-devel] [PATCH] target-arm: implement CPACR register logic
2014-05-16 10:01 [Qemu-devel] [PATCH] target-arm: implement CPACR register logic Fabian Aggeler
2014-05-16 17:14 ` Peter Crosthwaite
@ 2014-05-16 17:55 ` Peter Maydell
1 sibling, 0 replies; 3+ messages in thread
From: Peter Maydell @ 2014-05-16 17:55 UTC (permalink / raw)
To: Fabian Aggeler; +Cc: Peter Crosthwaite, QEMU Developers, Greg Bellows
On 16 May 2014 11:01, Fabian Aggeler <aggelerf@ethz.ch> wrote:
> From: Sergey Fedorov <s.fedorov@samsung.com>
>
> CPACR register allows to control access rights to coprocessor 0-13
> interfaces. Bits corresponding to unimplemented coprocessors should be
> RAZ/WI. QEMU implements only VFP coprocessor on ARMv6+ targets. So only
> cp10 & cp11 bits are writable.
Last time I looked at this register I decided that we
were OK for v8 (since v8 RES0 behaviour allows reads-as-written
as a possible implementation choice) and that I didn't care about
whether we were precisely architecturally correct for v7.
However if you want to tighten up our handling for v7 I
don't object.
> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> ---
>
> This patch was previously part of the Security Extensions patchset and
> got separated because it is not Sec-Ext specific.
> In addition to a style fix of the comment I added qemu_log_mask() as
> suggested.
>
> target-arm/helper.c | 6 ++++++
> target-arm/translate.c | 29 ++++++++++++++++++++++++++---
> 2 files changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 417161e..3cb1ac0 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -477,6 +477,12 @@ static const ARMCPRegInfo not_v7_cp_reginfo[] = {
> static void cpacr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> uint64_t value)
> {
> + uint32_t mask = 0;
> +
> + if (arm_feature(env, ARM_FEATURE_VFP)) {
> + mask |= 0x00f00000; /* VFP coprocessor: cp10 & cp11 */
> + }
> + value &= mask;
This looks wrong -- you're not accounting for the ASEDIS and
TRCDIS bits, or D32DIS in v7. If you're deliberately taking
advantage of the IMPDEF choice to have these bits all be RAZ/WI
you should document that. (But note that v8 doesn't allow this
liberty for TRCDIS.)
> if (env->cp15.c1_coproc != value) {
> env->cp15.c1_coproc = value;
> /* ??? Is this safe when called from within a TB? */
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index a4d920b..9b298d0 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -6864,9 +6864,32 @@ static int disas_coproc_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
> const ARMCPRegInfo *ri;
>
> cpnum = (insn >> 8) & 0xf;
> - if (arm_feature(env, ARM_FEATURE_XSCALE)
> - && ((env->cp15.c15_cpar ^ 0x3fff) & (1 << cpnum)))
> - return 1;
> + if (cpnum < 14) {
> + if (arm_feature(env, ARM_FEATURE_XSCALE)) {
> + if (~env->cp15.c15_cpar & (1 << cpnum)) {
> + return 1;
> + }
> + } else {
> + /* Bits [20:21] of CPACR control access to cp10
> + * Bits [23:22] of CPACR control access to cp11
> + */
> + switch ((env->cp15.c1_coproc >> (cpnum * 2)) & 3) {
> + case 0: /* access denied */
> + return 1;
> + case 1: /* privileged mode access only */
> + if (IS_USER(s)) {
> + return 1;
> + }
> + break;
> + case 2: /* reserved */
> + qemu_log_mask(LOG_GUEST_ERROR, "CPACR bits for cp%d set to "
> + "0b10 (unpredictable)\n", cpnum);
> + return 1;
> + case 3: /* privileged and user mode access */
> + break;
This is all pointless because VFP instructions don't
get decoded via disas_coproc_insn(). So you'll only get
here for coprocessors which aren't 10, 11, 14 or 15,
and we know that there aren't any of those, so we'll
just undef anyway because the reginfo lookup will fail.
Honouring the CPACR bits for VFP/Neon access is done via
the CPACR_FPEN bit in the TB flags. (In fact we should
really stop flushing the tb for CPACR writes, since all
the bits are ignored except ones which we deal with via
TB flags anyway.)
thanks
-- PMM
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-05-16 17:55 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-05-16 10:01 [Qemu-devel] [PATCH] target-arm: implement CPACR register logic Fabian Aggeler
2014-05-16 17:14 ` Peter Crosthwaite
2014-05-16 17:55 ` Peter Maydell
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).