[Qemu-devel] [PATCH 00/10] target/arm: Some pieces of support for 32-bit Hyp mode

[Qemu-devel] [PATCH 00/10] target/arm: Some pieces of support for 32-bit Hyp mode

[Peter Maydell]

Now we have virtualization support in the GICv2 emulation,
I thought I'd have a look at how much we were still missing
for being able to enable EL2 support for AArch32.
This set of patches fixes some minor missing pieces:
 * various small bugs in cp15 registers or places where
   we were missing the 32-bit version of a 64-bit register
 * a bugfix for MSR/MRS (banked), which were not allowing
   Hyp mode to access ELR_Hyp
 * implementation of the ERET instruction for A32/T32
 * support for taking exceptions to Hyp mode (the largest
   of these missing bits)

This isn't complete, but I thought I'd push these patches
out for review. My test setup is that I have another
couple of patches, one which fixes up hw/arm/boot.c to
boot AArch32 kernels in Hyp mode if it exists, and one
which sets ARM_FEATURE_EL2 on our A15 model. With those I
can get an outer kernel to boot with KVM support and try
to run an inner guest kernel. The inner kernel boots OK
but gets random segfaults in its userspace -- I haven't
tracked down why this is yet...

Some bits that are definitely missing:
 * ATS1HR, ATS1HW address translation ops
 * I need to check that the trap semantics for AArch32
   regs line up with their AArch64 counterparts

I also noticed that we fail to implement really quite a lot
of the HCR_EL2 trap semantics for either AArch64 or AArch32,
to the extent that I'm surprised that nested guests work
under AArch64 :-)

This patchset is based on top of my target-arm.for-3.1
branch.

thanks
-- PMM

Peter Maydell (10):
  target/arm: Correct typo in HAMAIR1 regdef name
  target/arm: Add missing .cp = 15 to HMAIR1 and HAMAIR1 regdefs
  target/arm: Implement RAZ/WI HACTLR2
  target/arm: Implement AArch32 HVBAR
  target/arm: Implement AArch32 HCR and HCR2
  target/arm: Implement AArch32 Hyp FARs
  target/arm: Implement ESR_EL2/HSR for AArch32 and no-EL2
  target/arm: Permit accesses to ELR_Hyp from Hyp mode via MSR/MRS
    (banked)
  target/arm: Implement AArch32 ERET instruction
  target/arm: Implement support for taking exceptions to Hyp mode

 target/arm/helper.c    | 226 ++++++++++++++++++++++++++++++++++-------
 target/arm/op_helper.c |  22 ++--
 target/arm/translate.c |  41 +++++++-
 3 files changed, 236 insertions(+), 53 deletions(-)

-- 
2.18.0

[Qemu-devel] [PATCH 01/10] target/arm: Correct typo in HAMAIR1 regdef name

[Peter Maydell]

We implement the HAMAIR1 register as RAZ/WI; we had a typo in the
regdef, though, and were incorrectly naming it HMAIR1 (which is
a different register which we also implement as RAZ/WI).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 8b07bf214ec..2c5e02c0b1a 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -3773,7 +3773,7 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
       .opc0 = 3, .opc1 = 4, .crn = 10, .crm = 3, .opc2 = 0,
       .access = PL2_RW, .type = ARM_CP_CONST,
       .resetvalue = 0 },
-    { .name = "HMAIR1", .state = ARM_CP_STATE_AA32,
+    { .name = "HAMAIR1", .state = ARM_CP_STATE_AA32,
       .opc1 = 4, .crn = 10, .crm = 3, .opc2 = 1,
       .access = PL2_RW, .type = ARM_CP_CONST,
       .resetvalue = 0 },
@@ -3925,7 +3925,7 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
       .access = PL2_RW, .type = ARM_CP_CONST,
       .resetvalue = 0 },
     /* HAMAIR1 is mapped to AMAIR_EL2[63:32] */
-    { .name = "HMAIR1", .state = ARM_CP_STATE_AA32,
+    { .name = "HAMAIR1", .state = ARM_CP_STATE_AA32,
       .opc1 = 4, .crn = 10, .crm = 3, .opc2 = 1,
       .access = PL2_RW, .type = ARM_CP_CONST,
       .resetvalue = 0 },
-- 
2.18.0

Re: [Qemu-devel] [PATCH 01/10] target/arm: Correct typo in HAMAIR1 regdef name

[Edgar E. Iglesias]

On Tue, Aug 14, 2018 at 01:42:45PM +0100, Peter Maydell wrote:
> We implement the HAMAIR1 register as RAZ/WI; we had a typo in the
> regdef, though, and were incorrectly naming it HMAIR1 (which is
> a different register which we also implement as RAZ/WI).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  target/arm/helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 8b07bf214ec..2c5e02c0b1a 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -3773,7 +3773,7 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
>        .opc0 = 3, .opc1 = 4, .crn = 10, .crm = 3, .opc2 = 0,
>        .access = PL2_RW, .type = ARM_CP_CONST,
>        .resetvalue = 0 },
> -    { .name = "HMAIR1", .state = ARM_CP_STATE_AA32,
> +    { .name = "HAMAIR1", .state = ARM_CP_STATE_AA32,
>        .opc1 = 4, .crn = 10, .crm = 3, .opc2 = 1,
>        .access = PL2_RW, .type = ARM_CP_CONST,
>        .resetvalue = 0 },
> @@ -3925,7 +3925,7 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
>        .access = PL2_RW, .type = ARM_CP_CONST,
>        .resetvalue = 0 },
>      /* HAMAIR1 is mapped to AMAIR_EL2[63:32] */
> -    { .name = "HMAIR1", .state = ARM_CP_STATE_AA32,
> +    { .name = "HAMAIR1", .state = ARM_CP_STATE_AA32,
>        .opc1 = 4, .crn = 10, .crm = 3, .opc2 = 1,
>        .access = PL2_RW, .type = ARM_CP_CONST,
>        .resetvalue = 0 },
> -- 
> 2.18.0
> 

Re: [Qemu-devel] [PATCH 01/10] target/arm: Correct typo in HAMAIR1 regdef name

[Luc Michel]

[-- Attachment #1: Type: text/plain, Size: 361 bytes --]



On 8/14/18 2:42 PM, Peter Maydell wrote:
> We implement the HAMAIR1 register as RAZ/WI; we had a typo in the
> regdef, though, and were incorrectly naming it HMAIR1 (which is
> a different register which we also implement as RAZ/WI).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-By: Luc Michel <luc.michel@greensocs.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH 02/10] target/arm: Add missing .cp = 15 to HMAIR1 and HAMAIR1 regdefs

[Peter Maydell]

ARMCPRegInfo structs will not default to .cp = 15 if they
are ARM_CP_STATE_BOTH, but not if they are ARM_CP_STATE_AA32
(because a coprocessor number of 0 is valid for AArch32).
We forgot to explicitly set .cp = 15 for the HMAIR1 and
HAMAIR1 regdefs, which meant they would UNDEF when the guest
tried to access them under cp15.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
A quick grep suggests these are the only ones we got wrong.
---
 target/arm/helper.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 2c5e02c0b1a..466c8ae492e 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -3767,14 +3767,14 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
       .access = PL2_RW, .type = ARM_CP_CONST,
       .resetvalue = 0 },
     { .name = "HMAIR1", .state = ARM_CP_STATE_AA32,
-      .opc1 = 4, .crn = 10, .crm = 2, .opc2 = 1,
+      .cp = 15, .opc1 = 4, .crn = 10, .crm = 2, .opc2 = 1,
       .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
     { .name = "AMAIR_EL2", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .opc1 = 4, .crn = 10, .crm = 3, .opc2 = 0,
       .access = PL2_RW, .type = ARM_CP_CONST,
       .resetvalue = 0 },
     { .name = "HAMAIR1", .state = ARM_CP_STATE_AA32,
-      .opc1 = 4, .crn = 10, .crm = 3, .opc2 = 1,
+      .cp = 15, .opc1 = 4, .crn = 10, .crm = 3, .opc2 = 1,
       .access = PL2_RW, .type = ARM_CP_CONST,
       .resetvalue = 0 },
     { .name = "AFSR0_EL2", .state = ARM_CP_STATE_BOTH,
@@ -3917,7 +3917,7 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
       .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.mair_el[2]),
       .resetvalue = 0 },
     { .name = "HMAIR1", .state = ARM_CP_STATE_AA32,
-      .opc1 = 4, .crn = 10, .crm = 2, .opc2 = 1,
+      .cp = 15, .opc1 = 4, .crn = 10, .crm = 2, .opc2 = 1,
       .access = PL2_RW, .type = ARM_CP_ALIAS,
       .fieldoffset = offsetofhigh32(CPUARMState, cp15.mair_el[2]) },
     { .name = "AMAIR_EL2", .state = ARM_CP_STATE_BOTH,
@@ -3926,7 +3926,7 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
       .resetvalue = 0 },
     /* HAMAIR1 is mapped to AMAIR_EL2[63:32] */
     { .name = "HAMAIR1", .state = ARM_CP_STATE_AA32,
-      .opc1 = 4, .crn = 10, .crm = 3, .opc2 = 1,
+      .cp = 15, .opc1 = 4, .crn = 10, .crm = 3, .opc2 = 1,
       .access = PL2_RW, .type = ARM_CP_CONST,
       .resetvalue = 0 },
     { .name = "AFSR0_EL2", .state = ARM_CP_STATE_BOTH,
-- 
2.18.0

Re: [Qemu-devel] [PATCH 02/10] target/arm: Add missing .cp = 15 to HMAIR1 and HAMAIR1 regdefs

[Edgar E. Iglesias]

On Tue, Aug 14, 2018 at 01:42:46PM +0100, Peter Maydell wrote:
> ARMCPRegInfo structs will not default to .cp = 15 if they
                            ^^^
               I think you've got a typo here


Otherwise:
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> are ARM_CP_STATE_BOTH, but not if they are ARM_CP_STATE_AA32
> (because a coprocessor number of 0 is valid for AArch32).
> We forgot to explicitly set .cp = 15 for the HMAIR1 and
> HAMAIR1 regdefs, which meant they would UNDEF when the guest
> tried to access them under cp15.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> A quick grep suggests these are the only ones we got wrong.
> ---
>  target/arm/helper.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 2c5e02c0b1a..466c8ae492e 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -3767,14 +3767,14 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
>        .access = PL2_RW, .type = ARM_CP_CONST,
>        .resetvalue = 0 },
>      { .name = "HMAIR1", .state = ARM_CP_STATE_AA32,
> -      .opc1 = 4, .crn = 10, .crm = 2, .opc2 = 1,
> +      .cp = 15, .opc1 = 4, .crn = 10, .crm = 2, .opc2 = 1,
>        .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>      { .name = "AMAIR_EL2", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .opc1 = 4, .crn = 10, .crm = 3, .opc2 = 0,
>        .access = PL2_RW, .type = ARM_CP_CONST,
>        .resetvalue = 0 },
>      { .name = "HAMAIR1", .state = ARM_CP_STATE_AA32,
> -      .opc1 = 4, .crn = 10, .crm = 3, .opc2 = 1,
> +      .cp = 15, .opc1 = 4, .crn = 10, .crm = 3, .opc2 = 1,
>        .access = PL2_RW, .type = ARM_CP_CONST,
>        .resetvalue = 0 },
>      { .name = "AFSR0_EL2", .state = ARM_CP_STATE_BOTH,
> @@ -3917,7 +3917,7 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
>        .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.mair_el[2]),
>        .resetvalue = 0 },
>      { .name = "HMAIR1", .state = ARM_CP_STATE_AA32,
> -      .opc1 = 4, .crn = 10, .crm = 2, .opc2 = 1,
> +      .cp = 15, .opc1 = 4, .crn = 10, .crm = 2, .opc2 = 1,
>        .access = PL2_RW, .type = ARM_CP_ALIAS,
>        .fieldoffset = offsetofhigh32(CPUARMState, cp15.mair_el[2]) },
>      { .name = "AMAIR_EL2", .state = ARM_CP_STATE_BOTH,
> @@ -3926,7 +3926,7 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
>        .resetvalue = 0 },
>      /* HAMAIR1 is mapped to AMAIR_EL2[63:32] */
>      { .name = "HAMAIR1", .state = ARM_CP_STATE_AA32,
> -      .opc1 = 4, .crn = 10, .crm = 3, .opc2 = 1,
> +      .cp = 15, .opc1 = 4, .crn = 10, .crm = 3, .opc2 = 1,
>        .access = PL2_RW, .type = ARM_CP_CONST,
>        .resetvalue = 0 },
>      { .name = "AFSR0_EL2", .state = ARM_CP_STATE_BOTH,
> -- 
> 2.18.0
> 

Re: [Qemu-devel] [PATCH 02/10] target/arm: Add missing .cp = 15 to HMAIR1 and HAMAIR1 regdefs

[Peter Maydell]

On 14 August 2018 at 15:41, Edgar E. Iglesias <edgar.iglesias@xilinx.com> wrote:
> On Tue, Aug 14, 2018 at 01:42:46PM +0100, Peter Maydell wrote:
>> ARMCPRegInfo structs will not default to .cp = 15 if they
>                             ^^^
>                I think you've got a typo here

Yep, that 'not' should be deleted.

> Otherwise:
> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 02/10] target/arm: Add missing .cp = 15 to HMAIR1 and HAMAIR1 regdefs

[Luc Michel]

[-- Attachment #1: Type: text/plain, Size: 2794 bytes --]

On 8/14/18 2:42 PM, Peter Maydell wrote:
> ARMCPRegInfo structs will not default to .cp = 15 if they
"will default" as suggested by Edgar.
> are ARM_CP_STATE_BOTH, but not if they are ARM_CP_STATE_AA32
> (because a coprocessor number of 0 is valid for AArch32).
> We forgot to explicitly set .cp = 15 for the HMAIR1 and
> HAMAIR1 regdefs, which meant they would UNDEF when the guest
> tried to access them under cp15.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-By: Luc Michel <luc.michel@greensocs.com>

> ---
> A quick grep suggests these are the only ones we got wrong.
> ---
>  target/arm/helper.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 2c5e02c0b1a..466c8ae492e 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -3767,14 +3767,14 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
>        .access = PL2_RW, .type = ARM_CP_CONST,
>        .resetvalue = 0 },
>      { .name = "HMAIR1", .state = ARM_CP_STATE_AA32,
> -      .opc1 = 4, .crn = 10, .crm = 2, .opc2 = 1,
> +      .cp = 15, .opc1 = 4, .crn = 10, .crm = 2, .opc2 = 1,
>        .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>      { .name = "AMAIR_EL2", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .opc1 = 4, .crn = 10, .crm = 3, .opc2 = 0,
>        .access = PL2_RW, .type = ARM_CP_CONST,
>        .resetvalue = 0 },
>      { .name = "HAMAIR1", .state = ARM_CP_STATE_AA32,
> -      .opc1 = 4, .crn = 10, .crm = 3, .opc2 = 1,
> +      .cp = 15, .opc1 = 4, .crn = 10, .crm = 3, .opc2 = 1,
>        .access = PL2_RW, .type = ARM_CP_CONST,
>        .resetvalue = 0 },
>      { .name = "AFSR0_EL2", .state = ARM_CP_STATE_BOTH,
> @@ -3917,7 +3917,7 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
>        .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.mair_el[2]),
>        .resetvalue = 0 },
>      { .name = "HMAIR1", .state = ARM_CP_STATE_AA32,
> -      .opc1 = 4, .crn = 10, .crm = 2, .opc2 = 1,
> +      .cp = 15, .opc1 = 4, .crn = 10, .crm = 2, .opc2 = 1,
>        .access = PL2_RW, .type = ARM_CP_ALIAS,
>        .fieldoffset = offsetofhigh32(CPUARMState, cp15.mair_el[2]) },
>      { .name = "AMAIR_EL2", .state = ARM_CP_STATE_BOTH,
> @@ -3926,7 +3926,7 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
>        .resetvalue = 0 },
>      /* HAMAIR1 is mapped to AMAIR_EL2[63:32] */
>      { .name = "HAMAIR1", .state = ARM_CP_STATE_AA32,
> -      .opc1 = 4, .crn = 10, .crm = 3, .opc2 = 1,
> +      .cp = 15, .opc1 = 4, .crn = 10, .crm = 3, .opc2 = 1,
>        .access = PL2_RW, .type = ARM_CP_CONST,
>        .resetvalue = 0 },
>      { .name = "AFSR0_EL2", .state = ARM_CP_STATE_BOTH,
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH 03/10] target/arm: Implement RAZ/WI HACTLR2

[Peter Maydell]

The AArch32 HACTLR2 register maps to bits [63:32] of ACTLR_EL2.
We implement ACTLR_EL2 as RAZ/WI, so make HACTLR2 also RAZ/WI.
(We put the regdef next to ACTLR_EL2 as a reminder in case we
ever make ACTLR_EL2 something other than RAZ/WI).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 466c8ae492e..14fd78f587a 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5436,6 +5436,11 @@ void register_cp_regs_for_features(ARMCPU *cpu)
               .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 0, .opc2 = 1,
               .access = PL2_RW, .type = ARM_CP_CONST,
               .resetvalue = 0 },
+            /* HACTLR2 maps to ACTLR_EL2[63:32] */
+            { .name = "HACTLR2", .state = ARM_CP_STATE_AA32,
+              .cp = 15, .opc1 = 4, .crn = 1, .crm = 0, .opc2 = 3,
+              .access = PL2_RW, .type = ARM_CP_CONST,
+              .resetvalue = 0 },
             { .name = "ACTLR_EL3", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 0, .opc2 = 1,
               .access = PL3_RW, .type = ARM_CP_CONST,
-- 
2.18.0

Re: [Qemu-devel] [PATCH 03/10] target/arm: Implement RAZ/WI HACTLR2

[Edgar E. Iglesias]

On Tue, Aug 14, 2018 at 01:42:47PM +0100, Peter Maydell wrote:
> The AArch32 HACTLR2 register maps to bits [63:32] of ACTLR_EL2.
> We implement ACTLR_EL2 as RAZ/WI, so make HACTLR2 also RAZ/WI.
> (We put the regdef next to ACTLR_EL2 as a reminder in case we
> ever make ACTLR_EL2 something other than RAZ/WI).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  target/arm/helper.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 466c8ae492e..14fd78f587a 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -5436,6 +5436,11 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>                .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 0, .opc2 = 1,
>                .access = PL2_RW, .type = ARM_CP_CONST,
>                .resetvalue = 0 },
> +            /* HACTLR2 maps to ACTLR_EL2[63:32] */
> +            { .name = "HACTLR2", .state = ARM_CP_STATE_AA32,
> +              .cp = 15, .opc1 = 4, .crn = 1, .crm = 0, .opc2 = 3,
> +              .access = PL2_RW, .type = ARM_CP_CONST,
> +              .resetvalue = 0 },
>              { .name = "ACTLR_EL3", .state = ARM_CP_STATE_AA64,
>                .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 0, .opc2 = 1,
>                .access = PL3_RW, .type = ARM_CP_CONST,
> -- 
> 2.18.0
> 

Re: [Qemu-devel] [PATCH 03/10] target/arm: Implement RAZ/WI HACTLR2

[Luc Michel]

[-- Attachment #1: Type: text/plain, Size: 1682 bytes --]

On 8/14/18 2:42 PM, Peter Maydell wrote:
> The AArch32 HACTLR2 register maps to bits [63:32] of ACTLR_EL2.
> We implement ACTLR_EL2 as RAZ/WI, so make HACTLR2 also RAZ/WI.
> (We put the regdef next to ACTLR_EL2 as a reminder in case we
> ever make ACTLR_EL2 something other than RAZ/WI).
Putting this regdef in the auxcr_reginfo[] array makes it defined when
the ARM_FEATURE_AUXCR is set (starting from ARM1026 ARMv5 I think).
However I believe this register only exists for ARMv8, for AArch32 mode
to be able to access upper bits of ACTLR_EL2. Should not we check for
ARM_FEATURE_ARMV8 being set so we don't define it for ARMv7?

-- 
Luc

> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 466c8ae492e..14fd78f587a 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -5436,6 +5436,11 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>                .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 0, .opc2 = 1,
>                .access = PL2_RW, .type = ARM_CP_CONST,
>                .resetvalue = 0 },
> +            /* HACTLR2 maps to ACTLR_EL2[63:32] */
> +            { .name = "HACTLR2", .state = ARM_CP_STATE_AA32,
> +              .cp = 15, .opc1 = 4, .crn = 1, .crm = 0, .opc2 = 3,
> +              .access = PL2_RW, .type = ARM_CP_CONST,
> +              .resetvalue = 0 },
>              { .name = "ACTLR_EL3", .state = ARM_CP_STATE_AA64,
>                .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 0, .opc2 = 1,
>                .access = PL3_RW, .type = ARM_CP_CONST,
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH 04/10] target/arm: Implement AArch32 HVBAR

[Peter Maydell]

Implement the AArch32 HVBAR register; we can do this just by
making the existing VBAR_EL2 regdefs be STATE_BOTH.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 14fd78f587a..b6412fe9d1f 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -3750,7 +3750,7 @@ static const ARMCPRegInfo v8_cp_reginfo[] = {
 
 /* Used to describe the behaviour of EL2 regs when EL2 does not exist.  */
 static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
-    { .name = "VBAR_EL2", .state = ARM_CP_STATE_AA64,
+    { .name = "VBAR_EL2", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .opc1 = 4, .crn = 12, .crm = 0, .opc2 = 0,
       .access = PL2_RW,
       .readfn = arm_cp_read_zero, .writefn = arm_cp_write_ignore },
@@ -3899,7 +3899,7 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
       .opc0 = 3, .opc1 = 4, .crn = 4, .crm = 0, .opc2 = 0,
       .access = PL2_RW,
       .fieldoffset = offsetof(CPUARMState, banked_spsr[BANK_HYP]) },
-    { .name = "VBAR_EL2", .state = ARM_CP_STATE_AA64,
+    { .name = "VBAR_EL2", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .opc1 = 4, .crn = 12, .crm = 0, .opc2 = 0,
       .access = PL2_RW, .writefn = vbar_write,
       .fieldoffset = offsetof(CPUARMState, cp15.vbar_el[2]),
-- 
2.18.0

Re: [Qemu-devel] [PATCH 04/10] target/arm: Implement AArch32 HVBAR

[Edgar E. Iglesias]

On Tue, Aug 14, 2018 at 01:42:48PM +0100, Peter Maydell wrote:
> Implement the AArch32 HVBAR register; we can do this just by
> making the existing VBAR_EL2 regdefs be STATE_BOTH.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  target/arm/helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 14fd78f587a..b6412fe9d1f 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -3750,7 +3750,7 @@ static const ARMCPRegInfo v8_cp_reginfo[] = {
>  
>  /* Used to describe the behaviour of EL2 regs when EL2 does not exist.  */
>  static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
> -    { .name = "VBAR_EL2", .state = ARM_CP_STATE_AA64,
> +    { .name = "VBAR_EL2", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .opc1 = 4, .crn = 12, .crm = 0, .opc2 = 0,
>        .access = PL2_RW,
>        .readfn = arm_cp_read_zero, .writefn = arm_cp_write_ignore },
> @@ -3899,7 +3899,7 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
>        .opc0 = 3, .opc1 = 4, .crn = 4, .crm = 0, .opc2 = 0,
>        .access = PL2_RW,
>        .fieldoffset = offsetof(CPUARMState, banked_spsr[BANK_HYP]) },
> -    { .name = "VBAR_EL2", .state = ARM_CP_STATE_AA64,
> +    { .name = "VBAR_EL2", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .opc1 = 4, .crn = 12, .crm = 0, .opc2 = 0,
>        .access = PL2_RW, .writefn = vbar_write,
>        .fieldoffset = offsetof(CPUARMState, cp15.vbar_el[2]),
> -- 
> 2.18.0
> 

Re: [Qemu-devel] [PATCH 04/10] target/arm: Implement AArch32 HVBAR

[Luc Michel]

[-- Attachment #1: Type: text/plain, Size: 1568 bytes --]



On 8/14/18 2:42 PM, Peter Maydell wrote:
> Implement the AArch32 HVBAR register; we can do this just by
> making the existing VBAR_EL2 regdefs be STATE_BOTH.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-By: Luc Michel <luc.michel@greensocs.com>

> ---
>  target/arm/helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 14fd78f587a..b6412fe9d1f 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -3750,7 +3750,7 @@ static const ARMCPRegInfo v8_cp_reginfo[] = {
>  
>  /* Used to describe the behaviour of EL2 regs when EL2 does not exist.  */
>  static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
> -    { .name = "VBAR_EL2", .state = ARM_CP_STATE_AA64,
> +    { .name = "VBAR_EL2", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .opc1 = 4, .crn = 12, .crm = 0, .opc2 = 0,
>        .access = PL2_RW,
>        .readfn = arm_cp_read_zero, .writefn = arm_cp_write_ignore },
> @@ -3899,7 +3899,7 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
>        .opc0 = 3, .opc1 = 4, .crn = 4, .crm = 0, .opc2 = 0,
>        .access = PL2_RW,
>        .fieldoffset = offsetof(CPUARMState, banked_spsr[BANK_HYP]) },
> -    { .name = "VBAR_EL2", .state = ARM_CP_STATE_AA64,
> +    { .name = "VBAR_EL2", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .opc1 = 4, .crn = 12, .crm = 0, .opc2 = 0,
>        .access = PL2_RW, .writefn = vbar_write,
>        .fieldoffset = offsetof(CPUARMState, cp15.vbar_el[2]),
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH 05/10] target/arm: Implement AArch32 HCR and HCR2

[Peter Maydell]

The AArch32 HCR and HCR2 registers alias HCR_EL2
bits [31:0] and [63:32]; implement them.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 39 +++++++++++++++++++++++++++++++++++----
 1 file changed, 35 insertions(+), 4 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index b6412fe9d1f..9701e413859 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -3754,11 +3754,15 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
       .opc0 = 3, .opc1 = 4, .crn = 12, .crm = 0, .opc2 = 0,
       .access = PL2_RW,
       .readfn = arm_cp_read_zero, .writefn = arm_cp_write_ignore },
-    { .name = "HCR_EL2", .state = ARM_CP_STATE_AA64,
+    { .name = "HCR_EL2", .state = ARM_CP_STATE_BOTH,
       .type = ARM_CP_NO_RAW,
       .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 0,
       .access = PL2_RW,
-      .readfn = arm_cp_read_zero, .writefn = arm_cp_write_ignore },
+      .type = ARM_CP_CONST, .resetvalue = 0 },
+    { .name = "HCR2", .state = ARM_CP_STATE_AA32,
+      .cp = 15, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 4,
+      .access = PL2_RW,
+      .type = ARM_CP_CONST, .resetvalue = 0 },
     { .name = "CPTR_EL2", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 2,
       .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
@@ -3872,10 +3876,26 @@ static void hcr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
      * HCR_PTW forbids certain page-table setups
      * HCR_DC Disables stage1 and enables stage2 translation
      */
-    if ((raw_read(env, ri) ^ value) & (HCR_VM | HCR_PTW | HCR_DC)) {
+    if ((env->cp15.hcr_el2 ^ value) & (HCR_VM | HCR_PTW | HCR_DC)) {
         tlb_flush(CPU(cpu));
     }
-    raw_write(env, ri, value);
+    env->cp15.hcr_el2 = value;
+}
+
+static void hcr_writehigh(CPUARMState *env, const ARMCPRegInfo *ri,
+                          uint64_t value)
+{
+    /* Handle HCR2 write, i.e. write to high half of HCR_EL2 */
+    value = deposit64(env->cp15.hcr_el2, 32, 32, value);
+    hcr_write(env, NULL, value);
+}
+
+static void hcr_writelow(CPUARMState *env, const ARMCPRegInfo *ri,
+                         uint64_t value)
+{
+    /* Handle HCR write, i.e. write to low half of HCR_EL2 */
+    value = deposit64(env->cp15.hcr_el2, 0, 32, value);
+    hcr_write(env, NULL, value);
 }
 
 static const ARMCPRegInfo el2_cp_reginfo[] = {
@@ -3883,6 +3903,17 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
       .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 0,
       .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.hcr_el2),
       .writefn = hcr_write },
+    { .name = "HCR", .state = ARM_CP_STATE_AA32,
+      .type = ARM_CP_ALIAS,
+      .cp = 15, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 0,
+      .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.hcr_el2),
+      .writefn = hcr_writelow },
+    { .name = "HCR2", .state = ARM_CP_STATE_AA32,
+      .type = ARM_CP_ALIAS,
+      .cp = 15, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 4,
+      .access = PL2_RW,
+      .fieldoffset = offsetofhigh32(CPUARMState, cp15.hcr_el2),
+      .writefn = hcr_writehigh },
     { .name = "ELR_EL2", .state = ARM_CP_STATE_AA64,
       .type = ARM_CP_ALIAS,
       .opc0 = 3, .opc1 = 4, .crn = 4, .crm = 0, .opc2 = 1,
-- 
2.18.0

Re: [Qemu-devel] [PATCH 05/10] target/arm: Implement AArch32 HCR and HCR2

[Edgar E. Iglesias]

On Tue, Aug 14, 2018 at 01:42:49PM +0100, Peter Maydell wrote:
> The AArch32 HCR and HCR2 registers alias HCR_EL2
> bits [31:0] and [63:32]; implement them.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 39 +++++++++++++++++++++++++++++++++++----
>  1 file changed, 35 insertions(+), 4 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index b6412fe9d1f..9701e413859 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -3754,11 +3754,15 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
>        .opc0 = 3, .opc1 = 4, .crn = 12, .crm = 0, .opc2 = 0,
>        .access = PL2_RW,
>        .readfn = arm_cp_read_zero, .writefn = arm_cp_write_ignore },
> -    { .name = "HCR_EL2", .state = ARM_CP_STATE_AA64,
> +    { .name = "HCR_EL2", .state = ARM_CP_STATE_BOTH,
>        .type = ARM_CP_NO_RAW,
>        .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 0,
>        .access = PL2_RW,
> -      .readfn = arm_cp_read_zero, .writefn = arm_cp_write_ignore },
> +      .type = ARM_CP_CONST, .resetvalue = 0 },
> +    { .name = "HCR2", .state = ARM_CP_STATE_AA32,
> +      .cp = 15, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 4,
> +      .access = PL2_RW,
> +      .type = ARM_CP_CONST, .resetvalue = 0 },
>      { .name = "CPTR_EL2", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 2,
>        .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> @@ -3872,10 +3876,26 @@ static void hcr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>       * HCR_PTW forbids certain page-table setups
>       * HCR_DC Disables stage1 and enables stage2 translation
>       */
> -    if ((raw_read(env, ri) ^ value) & (HCR_VM | HCR_PTW | HCR_DC)) {
> +    if ((env->cp15.hcr_el2 ^ value) & (HCR_VM | HCR_PTW | HCR_DC)) {
>          tlb_flush(CPU(cpu));
>      }
> -    raw_write(env, ri, value);
> +    env->cp15.hcr_el2 = value;
> +}
> +
> +static void hcr_writehigh(CPUARMState *env, const ARMCPRegInfo *ri,
> +                          uint64_t value)
> +{
> +    /* Handle HCR2 write, i.e. write to high half of HCR_EL2 */
> +    value = deposit64(env->cp15.hcr_el2, 32, 32, value);
> +    hcr_write(env, NULL, value);
> +}
> +
> +static void hcr_writelow(CPUARMState *env, const ARMCPRegInfo *ri,
> +                         uint64_t value)
> +{
> +    /* Handle HCR write, i.e. write to low half of HCR_EL2 */
> +    value = deposit64(env->cp15.hcr_el2, 0, 32, value);
> +    hcr_write(env, NULL, value);
>  }
>  
>  static const ARMCPRegInfo el2_cp_reginfo[] = {
> @@ -3883,6 +3903,17 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
>        .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 0,
>        .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.hcr_el2),
>        .writefn = hcr_write },
> +    { .name = "HCR", .state = ARM_CP_STATE_AA32,
> +      .type = ARM_CP_ALIAS,
> +      .cp = 15, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 0,
> +      .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.hcr_el2),
> +      .writefn = hcr_writelow },
> +    { .name = "HCR2", .state = ARM_CP_STATE_AA32,
> +      .type = ARM_CP_ALIAS,
> +      .cp = 15, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 4,
> +      .access = PL2_RW,
> +      .fieldoffset = offsetofhigh32(CPUARMState, cp15.hcr_el2),
> +      .writefn = hcr_writehigh },
>      { .name = "ELR_EL2", .state = ARM_CP_STATE_AA64,
>        .type = ARM_CP_ALIAS,
>        .opc0 = 3, .opc1 = 4, .crn = 4, .crm = 0, .opc2 = 1,
> -- 
> 2.18.0
> 

Re: [Qemu-devel] [PATCH 05/10] target/arm: Implement AArch32 HCR and HCR2

[Luc Michel]

[-- Attachment #1: Type: text/plain, Size: 3833 bytes --]

On 8/14/18 2:42 PM, Peter Maydell wrote:
> The AArch32 HCR and HCR2 registers alias HCR_EL2
> bits [31:0] and [63:32]; implement them.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 39 +++++++++++++++++++++++++++++++++++----
>  1 file changed, 35 insertions(+), 4 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index b6412fe9d1f..9701e413859 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -3754,11 +3754,15 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
>        .opc0 = 3, .opc1 = 4, .crn = 12, .crm = 0, .opc2 = 0,
>        .access = PL2_RW,
>        .readfn = arm_cp_read_zero, .writefn = arm_cp_write_ignore },
> -    { .name = "HCR_EL2", .state = ARM_CP_STATE_AA64,
> +    { .name = "HCR_EL2", .state = ARM_CP_STATE_BOTH,
>        .type = ARM_CP_NO_RAW,
>        .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 0,
>        .access = PL2_RW,
> -      .readfn = arm_cp_read_zero, .writefn = arm_cp_write_ignore },
> +      .type = ARM_CP_CONST, .resetvalue = 0 },
> +    { .name = "HCR2", .state = ARM_CP_STATE_AA32,
> +      .cp = 15, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 4,
> +      .access = PL2_RW,
> +      .type = ARM_CP_CONST, .resetvalue = 0 },
As for HACTLR2, shouldn't we avoid defining HCR2 for ARMv7?

>      { .name = "CPTR_EL2", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 2,
>        .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> @@ -3872,10 +3876,26 @@ static void hcr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>       * HCR_PTW forbids certain page-table setups
>       * HCR_DC Disables stage1 and enables stage2 translation
>       */
> -    if ((raw_read(env, ri) ^ value) & (HCR_VM | HCR_PTW | HCR_DC)) {
> +    if ((env->cp15.hcr_el2 ^ value) & (HCR_VM | HCR_PTW | HCR_DC)) {
>          tlb_flush(CPU(cpu));
>      }
> -    raw_write(env, ri, value);
> +    env->cp15.hcr_el2 = value;
> +}
> +
> +static void hcr_writehigh(CPUARMState *env, const ARMCPRegInfo *ri,
> +                          uint64_t value)
> +{
> +    /* Handle HCR2 write, i.e. write to high half of HCR_EL2 */
> +    value = deposit64(env->cp15.hcr_el2, 32, 32, value);
> +    hcr_write(env, NULL, value);
> +}
> +
> +static void hcr_writelow(CPUARMState *env, const ARMCPRegInfo *ri,
> +                         uint64_t value)
> +{
> +    /* Handle HCR write, i.e. write to low half of HCR_EL2 */
> +    value = deposit64(env->cp15.hcr_el2, 0, 32, value);
> +    hcr_write(env, NULL, value);
>  }
>  
>  static const ARMCPRegInfo el2_cp_reginfo[] = {
> @@ -3883,6 +3903,17 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
>        .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 0,
>        .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.hcr_el2),
>        .writefn = hcr_write },
> +    { .name = "HCR", .state = ARM_CP_STATE_AA32,
> +      .type = ARM_CP_ALIAS,
> +      .cp = 15, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 0,
> +      .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.hcr_el2),
> +      .writefn = hcr_writelow },
> +    { .name = "HCR2", .state = ARM_CP_STATE_AA32,
> +      .type = ARM_CP_ALIAS,
> +      .cp = 15, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 4,
> +      .access = PL2_RW,
> +      .fieldoffset = offsetofhigh32(CPUARMState, cp15.hcr_el2),
> +      .writefn = hcr_writehigh },
Also here. This one could actually be problematic as it allows ARMv7 to
write the upper 32-bits of HCR, since HCR_MASK in hcr_write() is greater
than UINT32_MAX.

-- 
Luc

>      { .name = "ELR_EL2", .state = ARM_CP_STATE_AA64,
>        .type = ARM_CP_ALIAS,
>        .opc0 = 3, .opc1 = 4, .crn = 4, .crm = 0, .opc2 = 1,
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 05/10] target/arm: Implement AArch32 HCR and HCR2

[Peter Maydell]

On 16 August 2018 at 09:55, Luc Michel <luc.michel@greensocs.com> wrote:
> On 8/14/18 2:42 PM, Peter Maydell wrote:
>> The AArch32 HCR and HCR2 registers alias HCR_EL2
>> bits [31:0] and [63:32]; implement them.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  target/arm/helper.c | 39 +++++++++++++++++++++++++++++++++++----
>>  1 file changed, 35 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index b6412fe9d1f..9701e413859 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -3754,11 +3754,15 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
>>        .opc0 = 3, .opc1 = 4, .crn = 12, .crm = 0, .opc2 = 0,
>>        .access = PL2_RW,
>>        .readfn = arm_cp_read_zero, .writefn = arm_cp_write_ignore },
>> -    { .name = "HCR_EL2", .state = ARM_CP_STATE_AA64,
>> +    { .name = "HCR_EL2", .state = ARM_CP_STATE_BOTH,
>>        .type = ARM_CP_NO_RAW,
>>        .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 0,
>>        .access = PL2_RW,
>> -      .readfn = arm_cp_read_zero, .writefn = arm_cp_write_ignore },
>> +      .type = ARM_CP_CONST, .resetvalue = 0 },
>> +    { .name = "HCR2", .state = ARM_CP_STATE_AA32,
>> +      .cp = 15, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 4,
>> +      .access = PL2_RW,
>> +      .type = ARM_CP_CONST, .resetvalue = 0 },
> As for HACTLR2, shouldn't we avoid defining HCR2 for ARMv7?

Yes. I was working off the v8 register list and should have
cross-checked what was present in v7.

thanks
-- PMM

[Qemu-devel] [PATCH 06/10] target/arm: Implement AArch32 Hyp FARs

[Peter Maydell]

The AArch32 virtualization extensions support these fault address
registers:
 * HDFAR: aliased with AArch64 FAR_EL2[31:0] and AArch32 DFAR(S)
 * HIFAR: aliased with AArch64 FAR_EL2[63:32] and AArch32 IFAR(S)

Implement the accessors for these. This fixes in passing a bug
where we weren't implementing the "RES0 from EL3 if EL2 not
implemented" behaviour for AArch64 FAR_EL2.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 9701e413859..d6e98e9d606 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -3847,6 +3847,13 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
     { .name = "HSTR_EL2", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 3,
       .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+    { .name = "FAR_EL2", .state = ARM_CP_STATE_BOTH,
+      .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 0,
+      .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+    { .name = "HIFAR", .state = ARM_CP_STATE_AA32,
+      .type = ARM_CP_CONST,
+      .cp = 15, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 2,
+      .access = PL2_RW, .resetvalue = 0 },
     REGINFO_SENTINEL
 };
 
@@ -3922,9 +3929,14 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
     { .name = "ESR_EL2", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 4, .crn = 5, .crm = 2, .opc2 = 0,
       .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.esr_el[2]) },
-    { .name = "FAR_EL2", .state = ARM_CP_STATE_AA64,
+    { .name = "FAR_EL2", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 0,
       .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.far_el[2]) },
+    { .name = "HIFAR", .state = ARM_CP_STATE_AA32,
+      .type = ARM_CP_ALIAS,
+      .cp = 15, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 2,
+      .access = PL2_RW,
+      .fieldoffset = offsetofhigh32(CPUARMState, cp15.far_el[2]) },
     { .name = "SPSR_EL2", .state = ARM_CP_STATE_AA64,
       .type = ARM_CP_ALIAS,
       .opc0 = 3, .opc1 = 4, .crn = 4, .crm = 0, .opc2 = 0,
-- 
2.18.0

Re: [Qemu-devel] [PATCH 06/10] target/arm: Implement AArch32 Hyp FARs

[Edgar E. Iglesias]

On Tue, Aug 14, 2018 at 01:42:50PM +0100, Peter Maydell wrote:
> The AArch32 virtualization extensions support these fault address
> registers:
>  * HDFAR: aliased with AArch64 FAR_EL2[31:0] and AArch32 DFAR(S)
>  * HIFAR: aliased with AArch64 FAR_EL2[63:32] and AArch32 IFAR(S)
> 
> Implement the accessors for these. This fixes in passing a bug
> where we weren't implementing the "RES0 from EL3 if EL2 not
> implemented" behaviour for AArch64 FAR_EL2.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  target/arm/helper.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 9701e413859..d6e98e9d606 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -3847,6 +3847,13 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
>      { .name = "HSTR_EL2", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 3,
>        .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> +    { .name = "FAR_EL2", .state = ARM_CP_STATE_BOTH,
> +      .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 0,
> +      .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> +    { .name = "HIFAR", .state = ARM_CP_STATE_AA32,
> +      .type = ARM_CP_CONST,
> +      .cp = 15, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 2,
> +      .access = PL2_RW, .resetvalue = 0 },
>      REGINFO_SENTINEL
>  };
>  
> @@ -3922,9 +3929,14 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
>      { .name = "ESR_EL2", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 4, .crn = 5, .crm = 2, .opc2 = 0,
>        .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.esr_el[2]) },
> -    { .name = "FAR_EL2", .state = ARM_CP_STATE_AA64,
> +    { .name = "FAR_EL2", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 0,
>        .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.far_el[2]) },
> +    { .name = "HIFAR", .state = ARM_CP_STATE_AA32,
> +      .type = ARM_CP_ALIAS,
> +      .cp = 15, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 2,
> +      .access = PL2_RW,
> +      .fieldoffset = offsetofhigh32(CPUARMState, cp15.far_el[2]) },
>      { .name = "SPSR_EL2", .state = ARM_CP_STATE_AA64,
>        .type = ARM_CP_ALIAS,
>        .opc0 = 3, .opc1 = 4, .crn = 4, .crm = 0, .opc2 = 0,
> -- 
> 2.18.0
> 

Re: [Qemu-devel] [PATCH 06/10] target/arm: Implement AArch32 Hyp FARs

[Luc Michel]

[-- Attachment #1: Type: text/plain, Size: 2463 bytes --]

On 8/14/18 2:42 PM, Peter Maydell wrote:
> The AArch32 virtualization extensions support these fault address
> registers:
>  * HDFAR: aliased with AArch64 FAR_EL2[31:0] and AArch32 DFAR(S)
>  * HIFAR: aliased with AArch64 FAR_EL2[63:32] and AArch32 IFAR(S)
> 
> Implement the accessors for these. This fixes in passing a bug
> where we weren't implementing the "RES0 from EL3 if EL2 not
> implemented" behaviour for AArch64 FAR_EL2.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-By: Luc Michel <luc.michel@greensocs.com>

> ---
>  target/arm/helper.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 9701e413859..d6e98e9d606 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -3847,6 +3847,13 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
>      { .name = "HSTR_EL2", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 3,
>        .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> +    { .name = "FAR_EL2", .state = ARM_CP_STATE_BOTH,
> +      .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 0,
> +      .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> +    { .name = "HIFAR", .state = ARM_CP_STATE_AA32,
> +      .type = ARM_CP_CONST,
> +      .cp = 15, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 2,
> +      .access = PL2_RW, .resetvalue = 0 },
>      REGINFO_SENTINEL
>  };
>  
> @@ -3922,9 +3929,14 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
>      { .name = "ESR_EL2", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 4, .crn = 5, .crm = 2, .opc2 = 0,
>        .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.esr_el[2]) },
> -    { .name = "FAR_EL2", .state = ARM_CP_STATE_AA64,
> +    { .name = "FAR_EL2", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 0,
>        .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.far_el[2]) },
> +    { .name = "HIFAR", .state = ARM_CP_STATE_AA32,
> +      .type = ARM_CP_ALIAS,
> +      .cp = 15, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 2,
> +      .access = PL2_RW,
> +      .fieldoffset = offsetofhigh32(CPUARMState, cp15.far_el[2]) },
>      { .name = "SPSR_EL2", .state = ARM_CP_STATE_AA64,
>        .type = ARM_CP_ALIAS,
>        .opc0 = 3, .opc1 = 4, .crn = 4, .crm = 0, .opc2 = 0,
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH 07/10] target/arm: Implement ESR_EL2/HSR for AArch32 and no-EL2

[Peter Maydell]

The AArch32 HSR is the equivalent of AArch64 ESR_EL2;
we can implement it by marking our existing ESR_EL2 regdef
as STATE_BOTH. It also needs to be "RES0 from EL3 if
EL2 not implemented", so add the missing stanza to
el3_no_el2_cp_reginfo.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index d6e98e9d606..80855302089 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -3763,6 +3763,10 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
       .cp = 15, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 4,
       .access = PL2_RW,
       .type = ARM_CP_CONST, .resetvalue = 0 },
+    { .name = "ESR_EL2", .state = ARM_CP_STATE_BOTH,
+      .opc0 = 3, .opc1 = 4, .crn = 5, .crm = 2, .opc2 = 0,
+      .access = PL2_RW,
+      .type = ARM_CP_CONST, .resetvalue = 0 },
     { .name = "CPTR_EL2", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 2,
       .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
@@ -3926,7 +3930,7 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
       .opc0 = 3, .opc1 = 4, .crn = 4, .crm = 0, .opc2 = 1,
       .access = PL2_RW,
       .fieldoffset = offsetof(CPUARMState, elr_el[2]) },
-    { .name = "ESR_EL2", .state = ARM_CP_STATE_AA64,
+    { .name = "ESR_EL2", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .opc1 = 4, .crn = 5, .crm = 2, .opc2 = 0,
       .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.esr_el[2]) },
     { .name = "FAR_EL2", .state = ARM_CP_STATE_BOTH,
-- 
2.18.0

Re: [Qemu-devel] [PATCH 07/10] target/arm: Implement ESR_EL2/HSR for AArch32 and no-EL2

[Edgar E. Iglesias]

On Tue, Aug 14, 2018 at 01:42:51PM +0100, Peter Maydell wrote:
> The AArch32 HSR is the equivalent of AArch64 ESR_EL2;
> we can implement it by marking our existing ESR_EL2 regdef
> as STATE_BOTH. It also needs to be "RES0 from EL3 if
> EL2 not implemented", so add the missing stanza to
> el3_no_el2_cp_reginfo.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index d6e98e9d606..80855302089 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -3763,6 +3763,10 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
>        .cp = 15, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 4,
>        .access = PL2_RW,
>        .type = ARM_CP_CONST, .resetvalue = 0 },
> +    { .name = "ESR_EL2", .state = ARM_CP_STATE_BOTH,
> +      .opc0 = 3, .opc1 = 4, .crn = 5, .crm = 2, .opc2 = 0,
> +      .access = PL2_RW,
> +      .type = ARM_CP_CONST, .resetvalue = 0 },
>      { .name = "CPTR_EL2", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 2,
>        .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> @@ -3926,7 +3930,7 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
>        .opc0 = 3, .opc1 = 4, .crn = 4, .crm = 0, .opc2 = 1,
>        .access = PL2_RW,
>        .fieldoffset = offsetof(CPUARMState, elr_el[2]) },
> -    { .name = "ESR_EL2", .state = ARM_CP_STATE_AA64,
> +    { .name = "ESR_EL2", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .opc1 = 4, .crn = 5, .crm = 2, .opc2 = 0,
>        .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.esr_el[2]) },
>      { .name = "FAR_EL2", .state = ARM_CP_STATE_BOTH,
> -- 
> 2.18.0
> 

Re: [Qemu-devel] [PATCH 07/10] target/arm: Implement ESR_EL2/HSR for AArch32 and no-EL2

[Luc Michel]

[-- Attachment #1: Type: text/plain, Size: 1828 bytes --]

On 8/14/18 2:42 PM, Peter Maydell wrote:
> The AArch32 HSR is the equivalent of AArch64 ESR_EL2;
> we can implement it by marking our existing ESR_EL2 regdef
> as STATE_BOTH. It also needs to be "RES0 from EL3 if
> EL2 not implemented", so add the missing stanza to
> el3_no_el2_cp_reginfo.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-By: Luc Michel <luc.michel@greensocs.com>

> ---
>  target/arm/helper.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index d6e98e9d606..80855302089 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -3763,6 +3763,10 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
>        .cp = 15, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 4,
>        .access = PL2_RW,
>        .type = ARM_CP_CONST, .resetvalue = 0 },
> +    { .name = "ESR_EL2", .state = ARM_CP_STATE_BOTH,
> +      .opc0 = 3, .opc1 = 4, .crn = 5, .crm = 2, .opc2 = 0,
> +      .access = PL2_RW,
> +      .type = ARM_CP_CONST, .resetvalue = 0 },
>      { .name = "CPTR_EL2", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 2,
>        .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> @@ -3926,7 +3930,7 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
>        .opc0 = 3, .opc1 = 4, .crn = 4, .crm = 0, .opc2 = 1,
>        .access = PL2_RW,
>        .fieldoffset = offsetof(CPUARMState, elr_el[2]) },
> -    { .name = "ESR_EL2", .state = ARM_CP_STATE_AA64,
> +    { .name = "ESR_EL2", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .opc1 = 4, .crn = 5, .crm = 2, .opc2 = 0,
>        .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.esr_el[2]) },
>      { .name = "FAR_EL2", .state = ARM_CP_STATE_BOTH,
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH 08/10] target/arm: Permit accesses to ELR_Hyp from Hyp mode via MSR/MRS (banked)

[Peter Maydell]

The MSR (banked) and MRS (banked) instructions allow accesses to ELR_Hyp
from either Monitor or Hyp mode. Our translate time check
was overly strict and only permitted access from Monitor mode.

The runtime check wo do in msr_mrs_banked_exc_checks() had the
correct code in it, but never got there because of the earlier
"currmode == tgtmode" check. Special case ELR_Hyp.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/op_helper.c | 22 +++++++++++-----------
 target/arm/translate.c | 10 +++++++---
 2 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index d550978b5b9..952b8d122b7 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -611,6 +611,14 @@ static void msr_mrs_banked_exc_checks(CPUARMState *env, uint32_t tgtmode,
      */
     int curmode = env->uncached_cpsr & CPSR_M;
 
+    if (regno == 17) {
+        /* ELR_Hyp: a special case because access from tgtmode is OK */
+        if (curmode != ARM_CPU_MODE_HYP && curmode != ARM_CPU_MODE_MON) {
+            goto undef;
+        }
+        return;
+    }
+
     if (curmode == tgtmode) {
         goto undef;
     }
@@ -638,17 +646,9 @@ static void msr_mrs_banked_exc_checks(CPUARMState *env, uint32_t tgtmode,
     }
 
     if (tgtmode == ARM_CPU_MODE_HYP) {
-        switch (regno) {
-        case 17: /* ELR_Hyp */
-            if (curmode != ARM_CPU_MODE_HYP && curmode != ARM_CPU_MODE_MON) {
-                goto undef;
-            }
-            break;
-        default:
-            if (curmode != ARM_CPU_MODE_MON) {
-                goto undef;
-            }
-            break;
+        /* SPSR_Hyp, r13_hyp: accessible from Monitor mode only */
+        if (curmode != ARM_CPU_MODE_MON) {
+            goto undef;
         }
     }
 
diff --git a/target/arm/translate.c b/target/arm/translate.c
index f845da7c638..3f5751d4826 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -4506,10 +4506,14 @@ static bool msr_banked_access_decode(DisasContext *s, int r, int sysm, int rn,
         }
         break;
     case ARM_CPU_MODE_HYP:
-        /* Note that we can forbid accesses from EL2 here because they
-         * must be from Hyp mode itself
+        /*
+         * SPSR_hyp and r13_hyp can only be accessed from Monitor mode
+         * (and so we can forbid accesses from EL2 or below). elr_hyp
+         * can be accessed also from Hyp mode, so forbid accesses from
+         * EL0 or EL1.
          */
-        if (!arm_dc_feature(s, ARM_FEATURE_EL2) || s->current_el < 3) {
+        if (!arm_dc_feature(s, ARM_FEATURE_EL2) || s->current_el < 2 ||
+            (s->current_el < 3 && *regno != 17)) {
             goto undef;
         }
         break;
-- 
2.18.0

Re: [Qemu-devel] [PATCH 08/10] target/arm: Permit accesses to ELR_Hyp from Hyp mode via MSR/MRS (banked)

[Edgar E. Iglesias]

On Tue, Aug 14, 2018 at 01:42:52PM +0100, Peter Maydell wrote:
> The MSR (banked) and MRS (banked) instructions allow accesses to ELR_Hyp
> from either Monitor or Hyp mode. Our translate time check
> was overly strict and only permitted access from Monitor mode.
> 
> The runtime check wo do in msr_mrs_banked_exc_checks() had the
> correct code in it, but never got there because of the earlier
> "currmode == tgtmode" check. Special case ELR_Hyp.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/op_helper.c | 22 +++++++++++-----------
>  target/arm/translate.c | 10 +++++++---
>  2 files changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index d550978b5b9..952b8d122b7 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -611,6 +611,14 @@ static void msr_mrs_banked_exc_checks(CPUARMState *env, uint32_t tgtmode,
>       */
>      int curmode = env->uncached_cpsr & CPSR_M;
>  
> +    if (regno == 17) {
> +        /* ELR_Hyp: a special case because access from tgtmode is OK */
> +        if (curmode != ARM_CPU_MODE_HYP && curmode != ARM_CPU_MODE_MON) {
> +            goto undef;
> +        }
> +        return;
> +    }
> +
>      if (curmode == tgtmode) {
>          goto undef;
>      }
> @@ -638,17 +646,9 @@ static void msr_mrs_banked_exc_checks(CPUARMState *env, uint32_t tgtmode,
>      }
>  
>      if (tgtmode == ARM_CPU_MODE_HYP) {
> -        switch (regno) {
> -        case 17: /* ELR_Hyp */
> -            if (curmode != ARM_CPU_MODE_HYP && curmode != ARM_CPU_MODE_MON) {
> -                goto undef;
> -            }
> -            break;
> -        default:
> -            if (curmode != ARM_CPU_MODE_MON) {
> -                goto undef;
> -            }
> -            break;
> +        /* SPSR_Hyp, r13_hyp: accessible from Monitor mode only */
> +        if (curmode != ARM_CPU_MODE_MON) {
> +            goto undef;
>          }
>      }
>  
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index f845da7c638..3f5751d4826 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -4506,10 +4506,14 @@ static bool msr_banked_access_decode(DisasContext *s, int r, int sysm, int rn,
>          }
>          break;
>      case ARM_CPU_MODE_HYP:
> -        /* Note that we can forbid accesses from EL2 here because they
> -         * must be from Hyp mode itself
> +        /*
> +         * SPSR_hyp and r13_hyp can only be accessed from Monitor mode
> +         * (and so we can forbid accesses from EL2 or below). elr_hyp
> +         * can be accessed also from Hyp mode, so forbid accesses from
> +         * EL0 or EL1.
>           */
> -        if (!arm_dc_feature(s, ARM_FEATURE_EL2) || s->current_el < 3) {
> +        if (!arm_dc_feature(s, ARM_FEATURE_EL2) || s->current_el < 2 ||
> +            (s->current_el < 3 && *regno != 17)) {
>              goto undef;
>          }
>          break;
> -- 
> 2.18.0
> 

Re: [Qemu-devel] [PATCH 08/10] target/arm: Permit accesses to ELR_Hyp from Hyp mode via MSR/MRS (banked)

[Luc Michel]

[-- Attachment #1: Type: text/plain, Size: 3094 bytes --]

On 8/14/18 2:42 PM, Peter Maydell wrote:
> The MSR (banked) and MRS (banked) instructions allow accesses to ELR_Hyp
> from either Monitor or Hyp mode. Our translate time check
> was overly strict and only permitted access from Monitor mode.
> 
> The runtime check wo do in msr_mrs_banked_exc_checks() had the
"we"
> correct code in it, but never got there because of the earlier
> "currmode == tgtmode" check. Special case ELR_Hyp.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-By: Luc Michel <luc.michel@greensocs.com>

> ---
>  target/arm/op_helper.c | 22 +++++++++++-----------
>  target/arm/translate.c | 10 +++++++---
>  2 files changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index d550978b5b9..952b8d122b7 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -611,6 +611,14 @@ static void msr_mrs_banked_exc_checks(CPUARMState *env, uint32_t tgtmode,
>       */
>      int curmode = env->uncached_cpsr & CPSR_M;
>  
> +    if (regno == 17) {
> +        /* ELR_Hyp: a special case because access from tgtmode is OK */
> +        if (curmode != ARM_CPU_MODE_HYP && curmode != ARM_CPU_MODE_MON) {
> +            goto undef;
> +        }
> +        return;
> +    }
> +
>      if (curmode == tgtmode) {
>          goto undef;
>      }
> @@ -638,17 +646,9 @@ static void msr_mrs_banked_exc_checks(CPUARMState *env, uint32_t tgtmode,
>      }
>  
>      if (tgtmode == ARM_CPU_MODE_HYP) {
> -        switch (regno) {
> -        case 17: /* ELR_Hyp */
> -            if (curmode != ARM_CPU_MODE_HYP && curmode != ARM_CPU_MODE_MON) {
> -                goto undef;
> -            }
> -            break;
> -        default:
> -            if (curmode != ARM_CPU_MODE_MON) {
> -                goto undef;
> -            }
> -            break;
> +        /* SPSR_Hyp, r13_hyp: accessible from Monitor mode only */
> +        if (curmode != ARM_CPU_MODE_MON) {
> +            goto undef;
>          }
>      }
>  
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index f845da7c638..3f5751d4826 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -4506,10 +4506,14 @@ static bool msr_banked_access_decode(DisasContext *s, int r, int sysm, int rn,
>          }
>          break;
>      case ARM_CPU_MODE_HYP:
> -        /* Note that we can forbid accesses from EL2 here because they
> -         * must be from Hyp mode itself
> +        /*
> +         * SPSR_hyp and r13_hyp can only be accessed from Monitor mode
> +         * (and so we can forbid accesses from EL2 or below). elr_hyp
> +         * can be accessed also from Hyp mode, so forbid accesses from
> +         * EL0 or EL1.
>           */
> -        if (!arm_dc_feature(s, ARM_FEATURE_EL2) || s->current_el < 3) {
> +        if (!arm_dc_feature(s, ARM_FEATURE_EL2) || s->current_el < 2 ||
> +            (s->current_el < 3 && *regno != 17)) {
>              goto undef;
>          }
>          break;
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH 09/10] target/arm: Implement AArch32 ERET instruction

[Peter Maydell]

ARMv7VE introduced the ERET instruction, which is necessary to
return from an exception taken to Hyp mode. Implement this.
In A32 encoding it is a completely new encoding; in T32 it
is an adjustment of the behaviour of the existing
"SUBS PC, LR, #<imm8>" instruction.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/translate.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 3f5751d4826..5ecc24f12fb 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -8887,6 +8887,25 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
             tcg_temp_free_i32(tmp2);
             store_reg(s, rd, tmp);
             break;
+        case 0x6: /* ERET */
+            if (op1 != 3) {
+                goto illegal_op;
+            }
+            if (!arm_dc_feature(s, ARM_FEATURE_V7VE)) {
+                goto illegal_op;
+            }
+            if ((insn & 0x000fff0f) != 0x0000000e) {
+                /* UNPREDICTABLE; we choose to UNDEF */
+                goto illegal_op;
+            }
+
+            if (s->current_el == 2) {
+                tmp = load_cpu_field(elr_el[2]);
+            } else {
+                tmp = load_reg(s, 14);
+            }
+            gen_exception_return(s, tmp);
+            break;
         case 7:
         {
             int imm16 = extract32(insn, 0, 4) | (extract32(insn, 8, 12) << 4);
@@ -11144,8 +11163,16 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
                         if (rn != 14 || rd != 15) {
                             goto illegal_op;
                         }
-                        tmp = load_reg(s, rn);
-                        tcg_gen_subi_i32(tmp, tmp, insn & 0xff);
+                        if (s->current_el == 2) {
+                            /* ERET from Hyp uses ELR_Hyp, not LR */
+                            if (insn & 0xff) {
+                                goto illegal_op;
+                            }
+                            tmp = load_cpu_field(elr_el[2]);
+                        } else {
+                            tmp = load_reg(s, rn);
+                            tcg_gen_subi_i32(tmp, tmp, insn & 0xff);
+                        }
                         gen_exception_return(s, tmp);
                         break;
                     case 6: /* MRS */
-- 
2.18.0

Re: [Qemu-devel] [PATCH 09/10] target/arm: Implement AArch32 ERET instruction

[Edgar E. Iglesias]

On Tue, Aug 14, 2018 at 01:42:53PM +0100, Peter Maydell wrote:
> ARMv7VE introduced the ERET instruction, which is necessary to
> return from an exception taken to Hyp mode. Implement this.
> In A32 encoding it is a completely new encoding; in T32 it
> is an adjustment of the behaviour of the existing
> "SUBS PC, LR, #<imm8>" instruction.


Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/translate.c | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 3f5751d4826..5ecc24f12fb 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -8887,6 +8887,25 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
>              tcg_temp_free_i32(tmp2);
>              store_reg(s, rd, tmp);
>              break;
> +        case 0x6: /* ERET */
> +            if (op1 != 3) {
> +                goto illegal_op;
> +            }
> +            if (!arm_dc_feature(s, ARM_FEATURE_V7VE)) {
> +                goto illegal_op;
> +            }
> +            if ((insn & 0x000fff0f) != 0x0000000e) {
> +                /* UNPREDICTABLE; we choose to UNDEF */
> +                goto illegal_op;
> +            }
> +
> +            if (s->current_el == 2) {
> +                tmp = load_cpu_field(elr_el[2]);
> +            } else {
> +                tmp = load_reg(s, 14);
> +            }
> +            gen_exception_return(s, tmp);
> +            break;
>          case 7:
>          {
>              int imm16 = extract32(insn, 0, 4) | (extract32(insn, 8, 12) << 4);
> @@ -11144,8 +11163,16 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
>                          if (rn != 14 || rd != 15) {
>                              goto illegal_op;
>                          }
> -                        tmp = load_reg(s, rn);
> -                        tcg_gen_subi_i32(tmp, tmp, insn & 0xff);
> +                        if (s->current_el == 2) {
> +                            /* ERET from Hyp uses ELR_Hyp, not LR */
> +                            if (insn & 0xff) {
> +                                goto illegal_op;
> +                            }
> +                            tmp = load_cpu_field(elr_el[2]);
> +                        } else {
> +                            tmp = load_reg(s, rn);
> +                            tcg_gen_subi_i32(tmp, tmp, insn & 0xff);
> +                        }
>                          gen_exception_return(s, tmp);
>                          break;
>                      case 6: /* MRS */
> -- 
> 2.18.0
> 

Re: [Qemu-devel] [PATCH 09/10] target/arm: Implement AArch32 ERET instruction

[Luc Michel]

[-- Attachment #1: Type: text/plain, Size: 2723 bytes --]

On 8/14/18 2:42 PM, Peter Maydell wrote:
> ARMv7VE introduced the ERET instruction, which is necessary to
> return from an exception taken to Hyp mode. Implement this.
> In A32 encoding it is a completely new encoding; in T32 it
> is an adjustment of the behaviour of the existing
> "SUBS PC, LR, #<imm8>" instruction.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-By: Luc Michel <luc.michel@greensocs.com>

> ---
>  target/arm/translate.c | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 3f5751d4826..5ecc24f12fb 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -8887,6 +8887,25 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
>              tcg_temp_free_i32(tmp2);
>              store_reg(s, rd, tmp);
>              break;
> +        case 0x6: /* ERET */
> +            if (op1 != 3) {
> +                goto illegal_op;
> +            }
> +            if (!arm_dc_feature(s, ARM_FEATURE_V7VE)) {
> +                goto illegal_op;
> +            }
> +            if ((insn & 0x000fff0f) != 0x0000000e) {
> +                /* UNPREDICTABLE; we choose to UNDEF */
> +                goto illegal_op;
> +            }
> +
> +            if (s->current_el == 2) {
> +                tmp = load_cpu_field(elr_el[2]);
> +            } else {
> +                tmp = load_reg(s, 14);
> +            }
> +            gen_exception_return(s, tmp);
> +            break;
>          case 7:
>          {
>              int imm16 = extract32(insn, 0, 4) | (extract32(insn, 8, 12) << 4);
> @@ -11144,8 +11163,16 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
>                          if (rn != 14 || rd != 15) {
>                              goto illegal_op;
>                          }
> -                        tmp = load_reg(s, rn);
> -                        tcg_gen_subi_i32(tmp, tmp, insn & 0xff);
> +                        if (s->current_el == 2) {
> +                            /* ERET from Hyp uses ELR_Hyp, not LR */
> +                            if (insn & 0xff) {
> +                                goto illegal_op;
> +                            }
> +                            tmp = load_cpu_field(elr_el[2]);
> +                        } else {
> +                            tmp = load_reg(s, rn);
> +                            tcg_gen_subi_i32(tmp, tmp, insn & 0xff);
> +                        }
>                          gen_exception_return(s, tmp);
>                          break;
>                      case 6: /* MRS */
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH 10/10] target/arm: Implement support for taking exceptions to Hyp mode

[Peter Maydell]

Implement the necessary support code for taking exceptions
to Hyp mode in AArch32.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 146 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 123 insertions(+), 23 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 80855302089..167203ac664 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -8013,6 +8013,123 @@ void aarch64_sync_64_to_32(CPUARMState *env)
     env->regs[15] = env->pc;
 }
 
+static void take_aarch32_exception(CPUARMState *env, int new_mode,
+                                   uint32_t mask, uint32_t offset,
+                                   uint32_t newpc)
+{
+    /* Change the CPU state so as to actually take the exception. */
+    switch_mode(env, new_mode);
+    /*
+     * For exceptions taken to AArch32 we must clear the SS bit in both
+     * PSTATE and in the old-state value we save to SPSR_<mode>, so zero it now.
+     */
+    env->uncached_cpsr &= ~PSTATE_SS;
+    env->spsr = cpsr_read(env);
+    /* Clear IT bits.  */
+    env->condexec_bits = 0;
+    /* Switch to the new mode, and to the correct instruction set.  */
+    env->uncached_cpsr = (env->uncached_cpsr & ~CPSR_M) | new_mode;
+    /* Set new mode endianness */
+    env->uncached_cpsr &= ~CPSR_E;
+    if (env->cp15.sctlr_el[arm_current_el(env)] & SCTLR_EE) {
+        env->uncached_cpsr |= CPSR_E;
+    }
+    env->daif |= mask;
+
+    if (new_mode == ARM_CPU_MODE_HYP) {
+        env->thumb = (env->cp15.sctlr_el[2] & SCTLR_TE) != 0;
+        env->elr_el[2] = env->regs[15];
+    } else {
+        /*
+         * this is a lie, as there was no c1_sys on V4T/V5, but who cares
+         * and we should just guard the thumb mode on V4
+         */
+        if (arm_feature(env, ARM_FEATURE_V4T)) {
+            env->thumb =
+                (A32_BANKED_CURRENT_REG_GET(env, sctlr) & SCTLR_TE) != 0;
+        }
+        env->regs[14] = env->regs[15] + offset;
+    }
+    env->regs[15] = newpc;
+}
+
+static void arm_cpu_do_interrupt_aarch32_hyp(CPUState *cs)
+{
+    /*
+     * Handle exception entry to Hyp mode; this is sufficiently
+     * different to entry to other AArch32 modes that we handle it
+     * separately here.
+     *
+     * The vector table entry used is always the 0x14 Hyp mode entry point,
+     * unless this is an UNDEF/HVC/abort taken from Hyp to Hyp.
+     * The offset applied to the preferred return address is always zero
+     * (see DDI0487C.a section G1.12.3).
+     * PSTATE A/I/F masks are set based only on the SCR.EA/IRQ/FIQ values.
+     */
+    uint32_t addr, mask;
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
+
+    switch (cs->exception_index) {
+    case EXCP_UDEF:
+        addr = 0x04;
+        break;
+    case EXCP_SWI:
+        addr = 0x14;
+        break;
+    case EXCP_BKPT:
+        /* Fall through to prefetch abort.  */
+    case EXCP_PREFETCH_ABORT:
+        env->cp15.ifar_s = env->exception.vaddress;
+        qemu_log_mask(CPU_LOG_INT, "...with HIFAR 0x%x\n",
+                      (uint32_t)env->exception.vaddress);
+        addr = 0x0c;
+        break;
+    case EXCP_DATA_ABORT:
+        env->cp15.dfar_s = env->exception.vaddress;
+        qemu_log_mask(CPU_LOG_INT, "...with HDFAR 0x%x\n",
+                      (uint32_t)env->exception.vaddress);
+        addr = 0x10;
+        break;
+    case EXCP_IRQ:
+        addr = 0x18;
+        break;
+    case EXCP_FIQ:
+        addr = 0x1c;
+        break;
+    case EXCP_HVC:
+        addr = 0x08;
+        break;
+    case EXCP_HYP_TRAP:
+        addr = 0x14;
+    default:
+        cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
+    }
+
+    if (cs->exception_index != EXCP_IRQ && cs->exception_index != EXCP_FIQ) {
+        env->cp15.esr_el[2] = env->exception.syndrome;
+    }
+
+    if (arm_current_el(env) != 2 && addr < 0x14) {
+        addr = 0x14;
+    }
+
+    mask = 0;
+    if (!(env->cp15.scr_el3 & SCR_EA)) {
+        mask |= CPSR_A;
+    }
+    if (!(env->cp15.scr_el3 & SCR_IRQ)) {
+        mask |= CPSR_I;
+    }
+    if (!(env->cp15.scr_el3 & SCR_IRQ)) {
+        mask |= CPSR_F;
+    }
+
+    addr += env->cp15.hvbar;
+
+    take_aarch32_exception(env, ARM_CPU_MODE_HYP, mask, 0, addr);
+}
+
 static void arm_cpu_do_interrupt_aarch32(CPUState *cs)
 {
     ARMCPU *cpu = ARM_CPU(cs);
@@ -8048,6 +8165,11 @@ static void arm_cpu_do_interrupt_aarch32(CPUState *cs)
         env->cp15.mdscr_el1 = deposit64(env->cp15.mdscr_el1, 2, 4, moe);
     }
 
+    if (env->exception.target_el == 2) {
+        arm_cpu_do_interrupt_aarch32_hyp(cs);
+        return;
+    }
+
     /* TODO: Vectored interrupt controller.  */
     switch (cs->exception_index) {
     case EXCP_UDEF:
@@ -8155,29 +8277,7 @@ static void arm_cpu_do_interrupt_aarch32(CPUState *cs)
         env->cp15.scr_el3 &= ~SCR_NS;
     }
 
-    switch_mode (env, new_mode);
-    /* For exceptions taken to AArch32 we must clear the SS bit in both
-     * PSTATE and in the old-state value we save to SPSR_<mode>, so zero it now.
-     */
-    env->uncached_cpsr &= ~PSTATE_SS;
-    env->spsr = cpsr_read(env);
-    /* Clear IT bits.  */
-    env->condexec_bits = 0;
-    /* Switch to the new mode, and to the correct instruction set.  */
-    env->uncached_cpsr = (env->uncached_cpsr & ~CPSR_M) | new_mode;
-    /* Set new mode endianness */
-    env->uncached_cpsr &= ~CPSR_E;
-    if (env->cp15.sctlr_el[arm_current_el(env)] & SCTLR_EE) {
-        env->uncached_cpsr |= CPSR_E;
-    }
-    env->daif |= mask;
-    /* this is a lie, as the was no c1_sys on V4T/V5, but who cares
-     * and we should just guard the thumb mode on V4 */
-    if (arm_feature(env, ARM_FEATURE_V4T)) {
-        env->thumb = (A32_BANKED_CURRENT_REG_GET(env, sctlr) & SCTLR_TE) != 0;
-    }
-    env->regs[14] = env->regs[15] + offset;
-    env->regs[15] = addr;
+    take_aarch32_exception(env, new_mode, mask, offset, addr);
 }
 
 /* Handle exception entry to a target EL which is using AArch64 */
-- 
2.18.0

Re: [Qemu-devel] [PATCH 10/10] target/arm: Implement support for taking exceptions to Hyp mode

[Edgar E. Iglesias]

On Tue, Aug 14, 2018 at 01:42:54PM +0100, Peter Maydell wrote:
> Implement the necessary support code for taking exceptions
> to Hyp mode in AArch32.

Hi Peter,

A general comment that I think this would be a little easier
to look at if it was split into two patches, one non-functional
change to break-out take_aarch32_exception() and then another
patch to add the new logic...

Another comment inline below

> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 146 +++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 123 insertions(+), 23 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 80855302089..167203ac664 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -8013,6 +8013,123 @@ void aarch64_sync_64_to_32(CPUARMState *env)
>      env->regs[15] = env->pc;
>  }
>  
> +static void take_aarch32_exception(CPUARMState *env, int new_mode,
> +                                   uint32_t mask, uint32_t offset,
> +                                   uint32_t newpc)
> +{
> +    /* Change the CPU state so as to actually take the exception. */
> +    switch_mode(env, new_mode);
> +    /*
> +     * For exceptions taken to AArch32 we must clear the SS bit in both
> +     * PSTATE and in the old-state value we save to SPSR_<mode>, so zero it now.
> +     */
> +    env->uncached_cpsr &= ~PSTATE_SS;
> +    env->spsr = cpsr_read(env);
> +    /* Clear IT bits.  */
> +    env->condexec_bits = 0;
> +    /* Switch to the new mode, and to the correct instruction set.  */
> +    env->uncached_cpsr = (env->uncached_cpsr & ~CPSR_M) | new_mode;
> +    /* Set new mode endianness */
> +    env->uncached_cpsr &= ~CPSR_E;
> +    if (env->cp15.sctlr_el[arm_current_el(env)] & SCTLR_EE) {
> +        env->uncached_cpsr |= CPSR_E;
> +    }
> +    env->daif |= mask;
> +
> +    if (new_mode == ARM_CPU_MODE_HYP) {
> +        env->thumb = (env->cp15.sctlr_el[2] & SCTLR_TE) != 0;
> +        env->elr_el[2] = env->regs[15];
> +    } else {
> +        /*
> +         * this is a lie, as there was no c1_sys on V4T/V5, but who cares
> +         * and we should just guard the thumb mode on V4
> +         */
> +        if (arm_feature(env, ARM_FEATURE_V4T)) {
> +            env->thumb =
> +                (A32_BANKED_CURRENT_REG_GET(env, sctlr) & SCTLR_TE) != 0;
> +        }
> +        env->regs[14] = env->regs[15] + offset;
> +    }
> +    env->regs[15] = newpc;
> +}
> +
> +static void arm_cpu_do_interrupt_aarch32_hyp(CPUState *cs)
> +{
> +    /*
> +     * Handle exception entry to Hyp mode; this is sufficiently
> +     * different to entry to other AArch32 modes that we handle it
> +     * separately here.
> +     *
> +     * The vector table entry used is always the 0x14 Hyp mode entry point,
> +     * unless this is an UNDEF/HVC/abort taken from Hyp to Hyp.
> +     * The offset applied to the preferred return address is always zero
> +     * (see DDI0487C.a section G1.12.3).
> +     * PSTATE A/I/F masks are set based only on the SCR.EA/IRQ/FIQ values.
> +     */
> +    uint32_t addr, mask;
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    CPUARMState *env = &cpu->env;
> +
> +    switch (cs->exception_index) {
> +    case EXCP_UDEF:
> +        addr = 0x04;
> +        break;
> +    case EXCP_SWI:
> +        addr = 0x14;
> +        break;
> +    case EXCP_BKPT:
> +        /* Fall through to prefetch abort.  */
> +    case EXCP_PREFETCH_ABORT:
> +        env->cp15.ifar_s = env->exception.vaddress;
> +        qemu_log_mask(CPU_LOG_INT, "...with HIFAR 0x%x\n",
> +                      (uint32_t)env->exception.vaddress);
> +        addr = 0x0c;
> +        break;
> +    case EXCP_DATA_ABORT:
> +        env->cp15.dfar_s = env->exception.vaddress;
> +        qemu_log_mask(CPU_LOG_INT, "...with HDFAR 0x%x\n",
> +                      (uint32_t)env->exception.vaddress);
> +        addr = 0x10;
> +        break;
> +    case EXCP_IRQ:
> +        addr = 0x18;
> +        break;
> +    case EXCP_FIQ:
> +        addr = 0x1c;
> +        break;
> +    case EXCP_HVC:
> +        addr = 0x08;
> +        break;
> +    case EXCP_HYP_TRAP:
> +        addr = 0x14;
> +    default:
> +        cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
> +    }
> +
> +    if (cs->exception_index != EXCP_IRQ && cs->exception_index != EXCP_FIQ) {
> +        env->cp15.esr_el[2] = env->exception.syndrome;
> +    }
> +
> +    if (arm_current_el(env) != 2 && addr < 0x14) {
> +        addr = 0x14;
> +    }
> +
> +    mask = 0;
> +    if (!(env->cp15.scr_el3 & SCR_EA)) {
> +        mask |= CPSR_A;
> +    }
> +    if (!(env->cp15.scr_el3 & SCR_IRQ)) {
> +        mask |= CPSR_I;
> +    }
> +    if (!(env->cp15.scr_el3 & SCR_IRQ)) {a
                                 ^^^^^^^
I think this should test for SCR_FIQ.

Other than those two comments I think this looks OK!

Thanks,
Edgar





> +        mask |= CPSR_F;
> +    }
> +
> +    addr += env->cp15.hvbar;
> +
> +    take_aarch32_exception(env, ARM_CPU_MODE_HYP, mask, 0, addr);
> +}
> +
>  static void arm_cpu_do_interrupt_aarch32(CPUState *cs)
>  {
>      ARMCPU *cpu = ARM_CPU(cs);
> @@ -8048,6 +8165,11 @@ static void arm_cpu_do_interrupt_aarch32(CPUState *cs)
>          env->cp15.mdscr_el1 = deposit64(env->cp15.mdscr_el1, 2, 4, moe);
>      }
>  
> +    if (env->exception.target_el == 2) {
> +        arm_cpu_do_interrupt_aarch32_hyp(cs);
> +        return;
> +    }
> +
>      /* TODO: Vectored interrupt controller.  */
>      switch (cs->exception_index) {
>      case EXCP_UDEF:
> @@ -8155,29 +8277,7 @@ static void arm_cpu_do_interrupt_aarch32(CPUState *cs)
>          env->cp15.scr_el3 &= ~SCR_NS;
>      }
>  
> -    switch_mode (env, new_mode);
> -    /* For exceptions taken to AArch32 we must clear the SS bit in both
> -     * PSTATE and in the old-state value we save to SPSR_<mode>, so zero it now.
> -     */
> -    env->uncached_cpsr &= ~PSTATE_SS;
> -    env->spsr = cpsr_read(env);
> -    /* Clear IT bits.  */
> -    env->condexec_bits = 0;
> -    /* Switch to the new mode, and to the correct instruction set.  */
> -    env->uncached_cpsr = (env->uncached_cpsr & ~CPSR_M) | new_mode;
> -    /* Set new mode endianness */
> -    env->uncached_cpsr &= ~CPSR_E;
> -    if (env->cp15.sctlr_el[arm_current_el(env)] & SCTLR_EE) {
> -        env->uncached_cpsr |= CPSR_E;
> -    }
> -    env->daif |= mask;
> -    /* this is a lie, as the was no c1_sys on V4T/V5, but who cares
> -     * and we should just guard the thumb mode on V4 */
> -    if (arm_feature(env, ARM_FEATURE_V4T)) {
> -        env->thumb = (A32_BANKED_CURRENT_REG_GET(env, sctlr) & SCTLR_TE) != 0;
> -    }
> -    env->regs[14] = env->regs[15] + offset;
> -    env->regs[15] = addr;
> +    take_aarch32_exception(env, new_mode, mask, offset, addr);
>  }
>  
>  /* Handle exception entry to a target EL which is using AArch64 */
> -- 
> 2.18.0
> 

Re: [Qemu-devel] [PATCH 10/10] target/arm: Implement support for taking exceptions to Hyp mode

[Peter Maydell]

On 15 August 2018 at 11:54, Edgar E. Iglesias <edgar.iglesias@xilinx.com> wrote:
> On Tue, Aug 14, 2018 at 01:42:54PM +0100, Peter Maydell wrote:
>> Implement the necessary support code for taking exceptions
>> to Hyp mode in AArch32.
>
> Hi Peter,
>
> A general comment that I think this would be a little easier
> to look at if it was split into two patches, one non-functional
> change to break-out take_aarch32_exception() and then another
> patch to add the new logic...

Yeah, you're right. I'll split it.

>> +    mask = 0;
>> +    if (!(env->cp15.scr_el3 & SCR_EA)) {
>> +        mask |= CPSR_A;
>> +    }
>> +    if (!(env->cp15.scr_el3 & SCR_IRQ)) {
>> +        mask |= CPSR_I;
>> +    }
>> +    if (!(env->cp15.scr_el3 & SCR_IRQ)) {a
>                                  ^^^^^^^
> I think this should test for SCR_FIQ.

Yep, cut-n-paste error.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 00/10] target/arm: Some pieces of support for 32-bit Hyp mode

[Edgar E. Iglesias]

On Tue, Aug 14, 2018 at 01:42:44PM +0100, Peter Maydell wrote:
> Now we have virtualization support in the GICv2 emulation,
> I thought I'd have a look at how much we were still missing
> for being able to enable EL2 support for AArch32.
> This set of patches fixes some minor missing pieces:
>  * various small bugs in cp15 registers or places where
>    we were missing the 32-bit version of a 64-bit register
>  * a bugfix for MSR/MRS (banked), which were not allowing
>    Hyp mode to access ELR_Hyp
>  * implementation of the ERET instruction for A32/T32
>  * support for taking exceptions to Hyp mode (the largest
>    of these missing bits)
> 
> This isn't complete, but I thought I'd push these patches
> out for review. My test setup is that I have another
> couple of patches, one which fixes up hw/arm/boot.c to
> boot AArch32 kernels in Hyp mode if it exists, and one
> which sets ARM_FEATURE_EL2 on our A15 model. With those I
> can get an outer kernel to boot with KVM support and try
> to run an inner guest kernel. The inner kernel boots OK
> but gets random segfaults in its userspace -- I haven't
> tracked down why this is yet...

Cool! :-)

> 
> Some bits that are definitely missing:
>  * ATS1HR, ATS1HW address translation ops
>  * I need to check that the trap semantics for AArch32
>    regs line up with their AArch64 counterparts
> 
> I also noticed that we fail to implement really quite a lot
> of the HCR_EL2 trap semantics for either AArch64 or AArch32,
> to the extent that I'm surprised that nested guests work
> under AArch64 :-)

Yes, we've have an entry for a while in our TODO list to improve
HCR trapping but haven't gotten around to it. IIRC, at the time
when we where bringing the EL2 stuff up, quite little was actually
being used by KVM/Xen.

Cheers,
Edgar



> 
> This patchset is based on top of my target-arm.for-3.1
> branch.
> 
> thanks
> -- PMM
> 
> Peter Maydell (10):
>   target/arm: Correct typo in HAMAIR1 regdef name
>   target/arm: Add missing .cp = 15 to HMAIR1 and HAMAIR1 regdefs
>   target/arm: Implement RAZ/WI HACTLR2
>   target/arm: Implement AArch32 HVBAR
>   target/arm: Implement AArch32 HCR and HCR2
>   target/arm: Implement AArch32 Hyp FARs
>   target/arm: Implement ESR_EL2/HSR for AArch32 and no-EL2
>   target/arm: Permit accesses to ELR_Hyp from Hyp mode via MSR/MRS
>     (banked)
>   target/arm: Implement AArch32 ERET instruction
>   target/arm: Implement support for taking exceptions to Hyp mode
> 
>  target/arm/helper.c    | 226 ++++++++++++++++++++++++++++++++++-------
>  target/arm/op_helper.c |  22 ++--
>  target/arm/translate.c |  41 +++++++-
>  3 files changed, 236 insertions(+), 53 deletions(-)
> 
> -- 
> 2.18.0
> 

Re: [Qemu-devel] [PATCH 00/10] target/arm: Some pieces of support for 32-bit Hyp mode

[Peter Maydell]

On 15 August 2018 at 12:04, Edgar E. Iglesias <edgar.iglesias@xilinx.com> wrote:
> On Tue, Aug 14, 2018 at 01:42:44PM +0100, Peter Maydell wrote:
>> I also noticed that we fail to implement really quite a lot
>> of the HCR_EL2 trap semantics for either AArch64 or AArch32,
>> to the extent that I'm surprised that nested guests work
>> under AArch64 :-)
>
> Yes, we've have an entry for a while in our TODO list to improve
> HCR trapping but haven't gotten around to it. IIRC, at the time
> when we where bringing the EL2 stuff up, quite little was actually
> being used by KVM/Xen.

We're also I think not correctly implementing some of
the required upgrades of TLB maintenance ops to inner-shareable,
so I suspect that operation in SMP configs will be flaky.

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 00/10] target/arm: Some pieces of support for 32-bit Hyp mode

[Peter Maydell]

On 14 August 2018 at 13:42, Peter Maydell <peter.maydell@linaro.org> wrote:
> Now we have virtualization support in the GICv2 emulation,
> I thought I'd have a look at how much we were still missing
> for being able to enable EL2 support for AArch32.
> This set of patches fixes some minor missing pieces:
>  * various small bugs in cp15 registers or places where
>    we were missing the 32-bit version of a 64-bit register
>  * a bugfix for MSR/MRS (banked), which were not allowing
>    Hyp mode to access ELR_Hyp
>  * implementation of the ERET instruction for A32/T32
>  * support for taking exceptions to Hyp mode (the largest
>    of these missing bits)
>
> This isn't complete, but I thought I'd push these patches
> out for review. My test setup is that I have another
> couple of patches, one which fixes up hw/arm/boot.c to
> boot AArch32 kernels in Hyp mode if it exists, and one
> which sets ARM_FEATURE_EL2 on our A15 model. With those I
> can get an outer kernel to boot with KVM support and try
> to run an inner guest kernel. The inner kernel boots OK
> but gets random segfaults in its userspace -- I haven't
> tracked down why this is yet...

I've put patches 1, 2, 4, 6, 7, 8, 9:

>   target/arm: Correct typo in HAMAIR1 regdef name
>   target/arm: Add missing .cp = 15 to HMAIR1 and HAMAIR1 regdefs
>   target/arm: Implement RAZ/WI HACTLR2
>   target/arm: Implement AArch32 HVBAR
>   target/arm: Implement AArch32 HCR and HCR2
>   target/arm: Implement AArch32 Hyp FARs
>   target/arm: Implement ESR_EL2/HSR for AArch32 and no-EL2
>   target/arm: Permit accesses to ELR_Hyp from Hyp mode via MSR/MRS
>     (banked)
>   target/arm: Implement AArch32 ERET instruction

into target-arm.next.

Patches 3, 5, 10 had issues in code review and I'll
rework those and send a v2 at some point:

>   target/arm: Implement RAZ/WI HACTLR2
>   target/arm: Implement AArch32 HCR and HCR2
>   target/arm: Implement support for taking exceptions to Hyp mode

thanks
-- PMM